Fix no palette 8 bit surface gfxdraw segfault

[ankith26]

Fixes #586

Yes, gfxdraw is scheduled for eventual removal and is generally unmaintained, but we should still not segfault.

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request fixes segfaults in gfxdraw functions when operating on 8-bit surfaces without palettes. The fix replaces direct palette access in _putPixelAlpha and _filledRectAlpha with SDL_GetRGB-based color extraction and alpha blending. A new test validates that various gfxdraw functions work correctly on palette-less 8-bit surfaces without raising exceptions.

Changes

Cohort / File(s)	Change Summary
Palette-less 8-bit surface fix src_c/SDL_gfx/SDL_gfxPrimitives.c	Replaced palette-based color extraction in _putPixelAlpha and _filledRectAlpha with SDL_GetRGB calls to prevent segfaults when surfaces lack palettes. Uses temporary RGB component variables for destination and source pixels to perform alpha blending and format mapping.
Test coverage for palette-less 8-bit surfaces test/gfxdraw_test.py	Added test_no_pal_8bit_surf test method that verifies 20+ gfxdraw functions (pixel, hline, vline, line, rectangle, box, circle, aacircle, filled_circle, ellipse, aaellipse, filled_ellipse, arc, pie, trigon, aatrigon, filled_trigon, polygon, aapolygon, filled_polygon, textured_polygon) execute without exceptions on 8-bit surfaces with no palette.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Pay special attention to the RGB extraction and alpha blending logic in _putPixelAlpha and _filledRectAlpha to ensure color components are correctly computed and mapped back to surface format
• Verify the temporary variable declarations and calculations don't introduce off-by-one errors or precision issues
• Confirm the test adequately exercises the fixed code paths with diverse gfxdraw functions

Suggested labels

bugfix, image

Suggested reviewers

• MyreMylar
• Starbuck5

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title "Fix no palette 8 bit surface gfxdraw segfault" is specific and directly summarizes the main purpose of the changeset. It clearly identifies the problem being addressed (segfaults in gfxdraw functions on 8-bit surfaces without palettes) and aligns with the linked issue #586. The title is concise, descriptive, and would be immediately understood by someone reviewing the repository history.
Linked Issues Check	✅ Passed	The code changes directly address the requirements from issue #586. The modification to SDL_gfxPrimitives.c replaces palette-based color extraction with SDL_GetRGB-based retrieval in _putPixelAlpha and _filledRectAlpha, which fixes the root cause of segfaults when no palette exists on 8-bit surfaces. The new test method test_no_pal_8bit_surf validates the fix by calling various gfxdraw functions (including aacircle, aapolygon, aatrigon, aaellipse, and others) on 8-bit surfaces without palettes and verifying no exceptions are raised, directly covering the scenarios described in the issue.
Out of Scope Changes Check	✅ Passed	All changes are directly in scope and relate to fixing the reported issue. The modifications to SDL_gfxPrimitives.c address the root cause of the segfault by replacing palette-dependent code with SDL_GetRGB calls, and the test additions in gfxdraw_test.py validate the fix and prevent regressions. There are no unrelated changes, refactoring outside the scope of this fix, or modifications to other areas of the codebase.
Description Check	✅ Passed	The description is related to the changeset, references the issue being fixed (#586), and provides context explaining why this fix is important despite gfxdraw being scheduled for removal. While brief, the description clearly communicates that the changes address a segfault issue in gfxdraw functions when operating on 8-bit surfaces without palettes.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch ankith26-fix-no-pal-8bit

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

test/gfxdraw_test.py (1)

875-913: Good regression test for no-palette segfault fix.

The test correctly creates an 8-bit surface without a palette (matching the issue #586 reproduction case) and verifies that all gfxdraw functions can be called without crashing. The comprehensive list of 18 functions tested is excellent coverage.

The bare except Exception (line 912) is flagged by static analysis, but for a regression test focused on preventing segfaults, this approach is acceptable since any exception is re-raised with context via self.fail(). However, if you want to be more specific, you could catch only the expected exception types or use a more targeted approach.

Note: This test validates that the functions don't crash, but doesn't verify the visual output is correct on no-palette surfaces.

If you'd like to make the exception handling more specific, consider:

try:
    func = getattr(pygame.gfxdraw, name)
    func(*args)
except (SystemError, MemoryError, AttributeError) as e:
    self.fail(f"gfxdraw.{name} raised an exception: {e}")

This would catch segfault-related errors while allowing other exceptions to propagate normally for debugging.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7325850 and 55ef0b9.

📒 Files selected for processing (2)

• src_c/SDL_gfx/SDL_gfxPrimitives.c (2 hunks)
• test/gfxdraw_test.py (1 hunks)

🧰 Additional context used

🧠 Learnings (3)

📓 Common learnings

Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_pygame.h:108-133
Timestamp: 2025-09-01T20:18:57.500Z
Learning: In pygame-ce PR #3573, PG_SURF_BitsPerPixel was changed from a simple macro to a static inline function to handle SDL2/SDL3 differences. SDL2's BitsPerPixel includes padding bits (e.g., RGBX => 32) while SDL3's SDL_BITSPERPIXEL excludes padding (e.g., RGBX => 24). The new implementation bridges this gap rather than removing the helper entirely.

Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_pygame.h:115-126
Timestamp: 2025-09-01T20:22:31.010Z
Learning: In pygame-ce PR #3573, the PG_SURF_BitsPerPixel function implementation uses hardcoded return values of 32 for YUY2/UYVY/YVYU FOURCC formats because it's copying the exact logic from SDL's internal SDL_GetMasksForPixelFormat function. This ensures perfect compatibility between SDL2 and SDL3 behaviors rather than using the technically more accurate values that might be returned by SDL_GetPixelFormatDetails.

📚 Learning: 2025-09-01T20:18:57.500Z

Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_pygame.h:108-133
Timestamp: 2025-09-01T20:18:57.500Z
Learning: In pygame-ce PR #3573, PG_SURF_BitsPerPixel was changed from a simple macro to a static inline function to handle SDL2/SDL3 differences. SDL2's BitsPerPixel includes padding bits (e.g., RGBX => 32) while SDL3's SDL_BITSPERPIXEL excludes padding (e.g., RGBX => 24). The new implementation bridges this gap rather than removing the helper entirely.

Applied to files:

• src_c/SDL_gfx/SDL_gfxPrimitives.c

📚 Learning: 2025-09-01T20:22:31.010Z

Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_pygame.h:115-126
Timestamp: 2025-09-01T20:22:31.010Z
Learning: In pygame-ce PR #3573, the PG_SURF_BitsPerPixel function implementation uses hardcoded return values of 32 for YUY2/UYVY/YVYU FOURCC formats because it's copying the exact logic from SDL's internal SDL_GetMasksForPixelFormat function. This ensures perfect compatibility between SDL2 and SDL3 behaviors rather than using the technically more accurate values that might be returned by SDL_GetPixelFormatDetails.

Applied to files:

• src_c/SDL_gfx/SDL_gfxPrimitives.c

🧬 Code graph analysis (1)

test/gfxdraw_test.py (2)

buildconfig/stubs/pygame/rect.pyi (1)

• Rect (307-307)

src_c/gfxdraw.c (1)

• gfxdraw (1046-1079)

🪛 Ruff (0.14.2)

test/gfxdraw_test.py

912-912: Do not catch blind exception: Exception

(BLE001)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)

• GitHub Check: build (ubuntu-22.04)
• GitHub Check: msys2 (mingw64, x86_64)
• GitHub Check: msys2 (ucrt64, ucrt-x86_64)
• GitHub Check: aarch64
• GitHub Check: i686
• GitHub Check: x86_64
• GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
• GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
• GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
• GitHub Check: Debian (Bookworm - 12) [armv7]
• GitHub Check: Pyodide build
• GitHub Check: build (macos-14)
• GitHub Check: Debian (Bookworm - 12) [armv6]
• GitHub Check: Debian (Bookworm - 12) [s390x]
• GitHub Check: Debian (Bookworm - 12) [ppc64le]
• GitHub Check: build (ubuntu-24.04)
• GitHub Check: AMD64
• GitHub Check: dev-check
• GitHub Check: x86

🔇 Additional comments (2)

src_c/SDL_gfx/SDL_gfxPrimitives.c (2)

296-308: LGTM - Correct fix for no-palette 8-bit surfaces.

The change correctly addresses the segfault by using SDL_GetRGB to extract color components instead of directly accessing the palette. The approach:

• Works for both palette and non-palette 8-bit surfaces
• Performs proper alpha blending on RGB components
• Uses SDL_MapRGB to convert back to the surface format

The alpha blending math is correct and the fix is minimal.

---

603-619: LGTM - Consistent fix for filled rectangle alpha blending.

This change applies the same SDL_GetRGB approach to fix the segfault in filled rectangle drawing. Good optimization to extract the source color RGB components once outside the loop (line 606) while extracting destination pixel RGB per iteration (line 613).