[Part 3] SDL3: surface+pixelarray+font+draw: runtime fixes #3578
Conversation
📝 WalkthroughWalkthroughReplaces surface-related macros with SDL-version-aware inline functions, unifies palette access via PG_GetSurfacePalette, changes conversion callsites to pass SDL_Surface* targets, adds PG_BlitSurface for SDL3-aware blitting, tweaks surface_fill error return, and corrects pgSurface_Blit docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Caller
participant Conv as PG_ConvertSurface
participant SDL as SDL
App->>Conv: PG_ConvertSurface(surface, dst_surface)
alt SDL3
Conv->>SDL: SDL_ConvertSurfaceAndColorspace(surface, dst->format, dst->palette, dst->colorspace)
else SDL2
Conv->>SDL: SDL_ConvertSurface(surface, dst->format, 0)
end
SDL-->>Conv: converted_surface or NULL
Conv-->>App: converted_surface or NULL
sequenceDiagram
autonumber
participant App as Caller
participant PB as PG_BlitSurface
participant SDL as SDL
App->>PB: PG_BlitSurface(src, srcrect, dst, dstrect)
alt SDL3
PB->>PB: compute/clip dstrect (SDL3 semantics)
PB->>SDL: SDL_BlitSurface(src, srcrect, dst, dstrect)
SDL-->>PB: status
else SDL2
PB->>SDL: SDL_BlitSurface(src, srcrect, dst, dstrect)
SDL-->>PB: status (may modify dstrect)
end
PB-->>App: 0 on success, non-zero on error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Potential attention points:
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src_c/draw.c (1)
1334-1346: Fix memory leak and lock the converted pattern surface correctly
- pattern is a newly allocated surface from PG_ConvertSurface and is never freed.
- The lock/unlock uses pgSurface_Lock/Unlock on pat_surfobj, which does not lock pattern. Use SDL_LockSurface/SDL_UnlockSurface on pattern instead.
Apply:
@@ - pattern = PG_ConvertSurface(pat_surfobj->surf, surf); + pattern = PG_ConvertSurface(pat_surfobj->surf, surf); @@ - SDL_SetSurfaceRLE(pattern, SDL_FALSE); + SDL_SetSurfaceRLE(pattern, SDL_FALSE); @@ - if (pattern && SDL_MUSTLOCK(pattern)) { - did_lock_pat = SDL_TRUE; - if (!pgSurface_Lock(pat_surfobj)) { - if (did_lock_surf) { - pgSurface_Unlock(surfobj); - } - return RAISE(PyExc_RuntimeError, "error locking pattern surface"); - } - } + if (pattern && SDL_MUSTLOCK(pattern)) { + did_lock_pat = SDL_TRUE; + if (SDL_LockSurface(pattern) != 0) { + if (did_lock_surf) { + pgSurface_Unlock(surfobj); + } + SDL_FreeSurface(pattern); + return RAISE(PyExc_RuntimeError, "error locking pattern surface"); + } + } @@ - /* no allocated pattern surface to free */ - - if (did_lock_pat) { - if (!pgSurface_Unlock(pat_surfobj)) { - if (did_lock_surf) { - pgSurface_Unlock(surfobj); - } - return RAISE(PyExc_RuntimeError, - "error unlocking pattern surface"); - } - } + if (did_lock_pat) { + SDL_UnlockSurface(pattern); + } if (did_lock_surf) { - if (!pgSurface_Unlock(surfobj)) { - return RAISE(PyExc_RuntimeError, "error unlocking surface"); - } + if (!pgSurface_Unlock(surfobj)) { + if (pattern) { + SDL_FreeSurface(pattern); + } + return RAISE(PyExc_RuntimeError, "error unlocking surface"); + } } + /* free the temporary converted pattern surface */ + if (pattern) { + SDL_FreeSurface(pattern); + pattern = NULL; + } @@ - if (flood_fill_result == -1) { + if (flood_fill_result == -1) { return NULL; /* error already set by flood_fill_inner */ }Also applies to: 1361-1369, 1376-1384, 1385-1389, 1391-1394
src_c/pixelarray_methods.c (1)
161-168: Locking logic inverted: pixels may be written without required locknew_surf pixels are accessed below, but the code locks when SDL_MUSTLOCK(new_surf) == 0 and skips locking when it’s required. This is a pre-existing issue, not introduced here, but it can cause undefined behavior on RLE-accelerated surfaces.
Apply:
- /* Acquire a temporary lock. */ - if (SDL_MUSTLOCK(new_surf) == 0) { - SDL_LockSurface(new_surf); - } + /* Acquire a temporary lock if required. */ + if (SDL_MUSTLOCK(new_surf)) { + SDL_LockSurface(new_surf); + } @@ - if (SDL_MUSTLOCK(new_surf) == 0) { - SDL_UnlockSurface(new_surf); - } + if (SDL_MUSTLOCK(new_surf)) { + SDL_UnlockSurface(new_surf); + }Given the PR’s SDL3-compat scope, feel free to land this in a follow-up if you prefer to keep this diff minimal.
Also applies to: 240-243
src_c/surface.c (1)
1793-1804: Prevent NULL deref when conversion fails.SDL_ConvertSurfaceAndColorspace may fail; SDL_SetSurfaceBlendMode(newsurf, …) is called unconditionally and can crash.
Apply:
- SDL_SetSurfaceBlendMode(newsurf, SDL_BLENDMODE_NONE); + if (newsurf) { + SDL_SetSurfaceBlendMode(newsurf, SDL_BLENDMODE_NONE); + }
🧹 Nitpick comments (2)
src_c/_pygame.h (1)
116-126: Clarify the comment to avoid confusion about “24-bit images.”The intent is to preserve SDL2’s 32 bpp reporting for formats stored in 4 bytes per pixel. Suggest wording-only tweak.
- /* Compat with SDL2: for 24-bit images SDL3 returns 24 when SDL2 - * returned 32 */ + /* Compat with SDL2: some 24-bit formats are stored in 4 bytes/pixel + * (e.g., RGB888). SDL3 reports 24 bits for these, whereas SDL2 + * reported 32. Preserve SDL2 behavior when BytesPerPixel == 4. */src_c/draw.c (1)
1361-1369: Optional: remove did_lock_pat entirely if RLE is disabledSince SDL_SetSurfaceRLE(pattern, SDL_FALSE) guarantees SDL_MUSTLOCK(pattern) is false, the extra lock branch is redundant. You can simplify by dropping it. Your call.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
docs/reST/c_api/surface.rst(1 hunks)src_c/_pygame.h(4 hunks)src_c/draw.c(1 hunks)src_c/font.c(1 hunks)src_c/pixelarray_methods.c(1 hunks)src_c/pixelcopy.c(1 hunks)src_c/surface.c(10 hunks)src_c/surface_fill.c(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Starbuck5
PR: pygame-community/pygame-ce#3573
File: src_c/_camera.c:129-131
Timestamp: 2025-08-30T21:11:00.196Z
Learning: When doing SDL2 to SDL3 compatibility changes, Starbuck5 prefers to maintain exact existing behaviors even when other improvements could be made, focusing solely on the compatibility aspect rather than mixing in behavioral fixes.
🧬 Code graph analysis (5)
src_c/pixelarray_methods.c (1)
src_c/_pygame.h (2)
PG_ConvertSurface(82-88)PG_ConvertSurface(226-230)
src_c/font.c (1)
src_c/_pygame.h (2)
PG_GetSurfacePalette(139-149)PG_GetSurfacePalette(283-287)
src_c/pixelcopy.c (1)
src_c/_pygame.h (2)
PG_GetSurfacePalette(139-149)PG_GetSurfacePalette(283-287)
src_c/draw.c (1)
src_c/_pygame.h (2)
PG_ConvertSurface(82-88)PG_ConvertSurface(226-230)
src_c/surface.c (1)
src_c/_pygame.h (2)
PG_ConvertSurface(82-88)PG_ConvertSurface(226-230)
🔇 Additional comments (16)
docs/reST/c_api/surface.rst (1)
59-60: Ensure pgSurface_Blit only returns 0 or 1
All branches in the C API’spgSurface_Blitimplementation (src_c/surface.c around its definition) must return0on success and1on exception. Manually inspect everyreturnin that function to confirm there are no stray-1or SDL error codes leaking through.src_c/surface_fill.c (1)
1146-1148: Good: don’t rely on SDL_SetError’s return value.Explicitly setting
result = -1is clearer and avoids potential API drift. LGTM.src_c/_pygame.h (4)
139-150: LGTM: lazy palette creation matches SDL2-era expectations.Creating a palette for indexed formats when missing restores implicit SDL2 behavior and should reduce surprises at call sites.
226-231: LGTM: SDL2 wrapper keeps prior behavior.Passing
dst->formatinto SDL_ConvertSurface maintains the old conversion semantics on SDL2.
155-158: Audit callers of PG_GetSurfaceDetails for null-palette handling. PG_GetSurfaceDetails may return true with*palette_p == NULL(e.g. non-indexed formats or allocation failure)—ensure no caller dereferencesSDL_Palette *palettewithout first checking for NULL.
81-89: SDL3 API names and parameter order are correct
The wrapper matches SDL3’s signatures—SDL_ConvertSurfaceAndColorspace takes(SDL_Surface*, SDL_PixelFormat, SDL_Palette*, SDL_Colorspace, SDL_PropertiesID), and the getter is correctly namedSDL_GetSurfaceColorspace(SDL_Surface*). No changes needed.src_c/pixelcopy.c (1)
1200-1210: Unify palette access via PG_GetSurfacePalette — looks goodThe switch to PG_GetSurfacePalette(surf) matches the new SDL3-compatible helper and keeps behavior for indexed formats. Safe under both SDL2 and SDL3.
src_c/font.c (1)
718-726: Use PG_GetSurfacePalette for SDL2/SDL3 parity — good changeCentralizes palette access and allows lazy creation on indexed surfaces; guarded by a null check before writes. No behavioral regressions expected.
src_c/pixelarray_methods.c (2)
139-147: Correct PG_ConvertSurface call site — destination is the surface, not its formatMatches the new inline API and ensures colorspace/palette are taken from the destination surface.
139-147: No lingering PG_ConvertSurface(...->format) call sites. All existing calls now pass SDL_Surface pointers only—no instances of the old->formatsignature remain.src_c/surface.c (6)
1626-1626: LGTM: copy via PG_ConvertSurface(surf, surf).Passing the dest surface to copy palette/format is correct for SDL3.
1683-1688: LGTM: convert-to-surface path uses PG_ConvertSurface.Using the destination surface to derive format/palette matches the new helper.
Also applies to: 1840-1841
1965-1965: LGTM: SDL2 branch calls SDL_ConvertSurface with flags=0.
3771-3771: LGTM: premul path copies via PG_ConvertSurface before modifying.
4546-4549: Return-path change acknowledged.Returning 1 on PG_GetSurfaceClipRect failure matches the function’s “non-zero error” convention.
Please confirm downstream callers don’t rely on distinguishing -1 vs 1 here.
4639-4641: LGTM contingent on PG_BlitSurface fix.Switching to PG_BlitSurface is correct for SDL3 as long as the wrapper passes the adjusted rects and return code.
Once the wrapper fix lands, please run a quick blit test to ensure dstrect returned from Surface.blit reflects clipping like SDL2 did.
Also applies to: 4668-4669
ankith26
force-pushed
the
ankith26-sdl3-3
branch
from
9ad82e8 to
14eafac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src_c/surface.c (2)
4456-4524: Fix PG_BlitSurface (SDL3): wrong return code, wrong rects, missing src adjust on dst clip
- Inverts SDL_BlitSurface return (success reported as -1).
- Calls SDL_BlitSurface with original srcrect/dstrect instead of computed r_src/r_dst.
- Doesn’t adjust r_src when r_dst is clipped.
int PG_BlitSurface(SDL_Surface *src, const SDL_Rect *srcrect, SDL_Surface *dst, SDL_Rect *dstrect) { #if SDL_VERSION_ATLEAST(3, 0, 0) - /* SDL3 doesn't modify dstrect, so compat for that. - * Below logic taken from SDL2 source with slight modifications */ - SDL_Rect r_src, r_dst; - - r_src.x = 0; - r_src.y = 0; - r_src.w = src->w; - r_src.h = src->h; - - if (dstrect) { - r_dst.x = dstrect->x; - r_dst.y = dstrect->y; - } - else { - r_dst.x = 0; - r_dst.y = 0; - } - - /* clip the source rectangle to the source surface */ - if (srcrect) { - SDL_Rect tmp; - if (SDL_IntersectRect(srcrect, &r_src, &tmp) == SDL_FALSE) { - goto end; - } - - /* Shift dstrect, if srcrect origin has changed */ - r_dst.x += tmp.x - srcrect->x; - r_dst.y += tmp.y - srcrect->y; - - /* Update srcrect */ - r_src = tmp; - } - - /* There're no dstrect.w/h parameters. It's the same as srcrect */ - r_dst.w = r_src.w; - r_dst.h = r_src.h; - - /* clip the destination rectangle against the clip rectangle */ - { - SDL_Rect tmp, clip_rect; - SDL_GetSurfaceClipRect(dst, &clip_rect); - if (SDL_IntersectRect(&r_dst, &clip_rect, &tmp) == SDL_FALSE) { - goto end; - } - - /* Update dstrect */ - r_dst = tmp; - } - - if (r_dst.w > 0 && r_dst.h > 0) { - if (dstrect) { /* update output parameter */ - *dstrect = r_dst; - } - return SDL_BlitSurface(src, srcrect, dst, dstrect) ? 0 : -1; - } -end: - if (dstrect) { - dstrect->w = dstrect->h = 0; - } - return 0; + /* SDL3 doesn't modify dstrect; emulate SDL2 behavior. */ + SDL_Rect r_src = {0, 0, src->w, src->h}; + SDL_Rect r_dst = {0, 0, 0, 0}; + SDL_Rect tmp, clip_rect; + int old_dst_x, old_dst_y; + + if (dstrect) { + r_dst.x = dstrect->x; + r_dst.y = dstrect->y; + } + /* Clip source to source surface if srcrect provided */ + if (srcrect) { + if (SDL_IntersectRect(srcrect, &r_src, &tmp) == SDL_FALSE) { + goto end_zero; + } + /* Shift dst if srcrect origin changed */ + r_dst.x += tmp.x - srcrect->x; + r_dst.y += tmp.y - srcrect->y; + r_src = tmp; + } + /* dst size follows source size */ + r_dst.w = r_src.w; + r_dst.h = r_src.h; + + /* Clip destination to its clip rect and adjust source accordingly */ + SDL_GetSurfaceClipRect(dst, &clip_rect); + old_dst_x = r_dst.x; + old_dst_y = r_dst.y; + if (SDL_IntersectRect(&r_dst, &clip_rect, &tmp) == SDL_FALSE) { + goto end_zero; + } + /* Adjust source by how much destination was moved */ + r_src.x += tmp.x - old_dst_x; + r_src.y += tmp.y - old_dst_y; + r_src.w = tmp.w; + r_src.h = tmp.h; + r_dst = tmp; + + /* Perform blit; propagate final dstrect to caller */ + { + int rc = SDL_BlitSurface(src, &r_src, dst, &r_dst); + if (dstrect) { + *dstrect = r_dst; + } + return rc; /* 0 on success, -1 on error */ + } +end_zero: + if (dstrect) { + dstrect->w = 0; + dstrect->h = 0; + } + return 0; #else return SDL_BlitSurface(src, srcrect, dst, dstrect); #endif }
4601-4609: SDL3 SDL_ConvertSurface missing flags argumentPass the third flags parameter (0) to match SDL3 signature.
- src = fmt ? SDL_ConvertSurface(src, - SDL_GetPixelFormatForMasks( - fmt->bits_per_pixel, fmt->Rmask, - fmt->Gmask, fmt->Bmask, 0)) + src = fmt ? SDL_ConvertSurface( + src, + SDL_GetPixelFormatForMasks(fmt->bits_per_pixel, + fmt->Rmask, fmt->Gmask, + fmt->Bmask, 0), + 0) : NULL;test/surface_test.py (1)
412-416: Apply the same version-gating helper hereMirror the change made above to avoid duplicating the tuple ranges.
- @unittest.skipIf( - (2, 32, 50) <= pygame.version.SDL <= (2, 32, 56) - or (3, 0, 0) <= pygame.version.SDL <= (3, 2, 22), - "This test was briefly broken on SDL3 (and sdl2-compat) but got fixed.", - ) + @unittest.skipIf( + _is_rle_regression_sdl(), + "Skip due to SDL RLE regression: affected SDL versions 2.32.50–2.32.56 and 3.0.0–3.2.22.", + )
🧹 Nitpick comments (2)
src_c/surface.c (1)
1547-1555: RLE strip hack looks fineRect init order is unconventional (h before w) but harmless. No functional concerns.
test/surface_test.py (1)
379-383: Consolidate SDL version gating and clarify range in skip messageTuple comparisons are fine, but inlining the ranges twice is brittle. Factor the predicate into a small helper and reuse it. Also make the skip reason state the exact affected ranges for CI clarity.
Apply this diff to the decorator:
- @unittest.skipIf( - (2, 32, 50) <= pygame.version.SDL <= (2, 32, 56) - or (3, 0, 0) <= pygame.version.SDL <= (3, 2, 22), - "This test was briefly broken on SDL3 (and sdl2-compat) but got fixed.", - ) + @unittest.skipIf( + _is_rle_regression_sdl(), + "Skip due to SDL RLE regression: affected SDL versions 2.32.50–2.32.56 and 3.0.0–3.2.22.", + )Add this helper near the imports (module scope):
def _is_rle_regression_sdl() -> bool: v = pygame.version.SDL return ((2, 32, 50) <= v <= (2, 32, 56)) or ((3, 0, 0) <= v <= (3, 2, 22))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
docs/reST/c_api/surface.rst(1 hunks)src_c/_pygame.h(3 hunks)src_c/draw.c(1 hunks)src_c/font.c(1 hunks)src_c/pixelarray_methods.c(1 hunks)src_c/pixelcopy.c(1 hunks)src_c/surface.c(12 hunks)src_c/surface_fill.c(1 hunks)test/surface_test.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src_c/pixelarray_methods.c
- src_c/font.c
- docs/reST/c_api/surface.rst
- src_c/draw.c
- src_c/surface_fill.c
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Starbuck5
PR: pygame-community/pygame-ce#3573
File: src_c/_pygame.h:108-133
Timestamp: 2025-09-01T20:18:57.470Z
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
PR: pygame-community/pygame-ce#3573
File: src_c/_pygame.h:115-126
Timestamp: 2025-09-01T20:22:30.959Z
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.470Z
Learnt from: Starbuck5
PR: pygame-community/pygame-ce#3573
File: src_c/_pygame.h:108-133
Timestamp: 2025-09-01T20:18:57.470Z
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/pixelcopy.ctest/surface_test.pysrc_c/surface.csrc_c/_pygame.h
📚 Learning: 2025-09-01T20:22:30.959Z
Learnt from: Starbuck5
PR: pygame-community/pygame-ce#3573
File: src_c/_pygame.h:115-126
Timestamp: 2025-09-01T20:22:30.959Z
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/pixelcopy.csrc_c/surface.csrc_c/_pygame.h
📚 Learning: 2025-08-30T21:11:00.196Z
Learnt from: Starbuck5
PR: pygame-community/pygame-ce#3573
File: src_c/_camera.c:129-131
Timestamp: 2025-08-30T21:11:00.196Z
Learning: When doing SDL2 to SDL3 compatibility changes, Starbuck5 prefers to maintain exact existing behaviors even when other improvements could be made, focusing solely on the compatibility aspect rather than mixing in behavioral fixes.
Applied to files:
src_c/surface.c
🧬 Code graph analysis (2)
src_c/pixelcopy.c (1)
src_c/_pygame.h (2)
PG_GetSurfacePalette(154-164)PG_GetSurfacePalette(298-302)
src_c/surface.c (2)
src_c/_pygame.h (2)
PG_ConvertSurface(82-88)PG_ConvertSurface(241-245)src_c/alphablit.c (1)
pygame_Blit(1582-1687)
⏰ 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). (5)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: x86
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: dev-check
🔇 Additional comments (11)
src_c/_pygame.h (3)
154-165: Lazy palette creation for indexed surfaces: LGTMCreates a palette on-demand for indexed formats, aligning SDL3 with SDL2 expectations.
166-173: Use new PG_GetSurfacePalette in details helper: LGTMEnsures callers consistently obtain a palette where applicable.
241-246: SDL2 inline PG_ConvertSurface wrapper: LGTMCorrect flags usage and format sourcing.
src_c/surface.c (7)
1623-1624: Copy via PG_ConvertSurface: LGTMCopies format/palette/colorspace from source surface as intended.
1684-1684: Convert-to-surface signature: LGTMPassing the destination surface to copy its format/palette is correct.
1837-1838: SDL2 convert-to-surface path: LGTMKeeps parity with SDL3 surface-based conversion.
1962-1962: SDL2 newfmt convert: LGTMIncludes required flags argument (0); OK.
2200-2206: Normalize SDL2/SDL3 fill return to -1/0Subtracting 1 standardizes error to -1. Verify PG_FillSurfaceRect returns 1/0 across both versions in all build configs.
3017-3024: RLE flags reporting across SDL2/SDL3PG_SurfaceHasRLE indicates requested RLE; SDL_MUSTLOCK is used here to infer active RLE. Double-check SDL3’s SDL_MUSTLOCK semantics so PGS_RLEACCEL isn’t set spuriously on non-RLE surfaces.
4647-4656: Fast-path pygame_Blit vs PG_BlitSurface split: LGTM (after PG_BlitSurface fix)Once PG_BlitSurface is corrected, these paths will produce correct rects and errors.
src_c/pixelcopy.c (1)
1201-1210: Unified palette access in make_surface: LGTMPG_GetSurfacePalette ensures an attached palette for INDEX8, matching the new helper’s contract.
| if (!ret && SDL_ISPIXELFORMAT_INDEXED(surf->format)) { | ||
| /* Palette doesn't exist but is expected, so create it. | ||
| * SDL will associate the newly created palette with the surface */ | ||
| ret = SDL_CreateSurfacePalette(surf); | ||
| } |
There was a problem hiding this comment.
I don't think this is how we should be doing this. If our surfaces are missing palettes we deal with that when the surface is not created, not randomly at any point as a side effect of a getter function. And/or make the consumers okay with index formats without palettes, since that's a thing in SDL3 and therefore we will want to support it anyway.
There was a problem hiding this comment.
I think this being part of the getter makes sense considering that the getter is responsible for returning a palette, and we must ensure it doesn't return NULL when it should actually return a palette. But sure, I suppose it being part of PG_CreateSurface could also work (but then we'd also have to think about all the other ways a surface is created, which means more places to modify)
| // TODO SDL3: figure out how to port this or if it's relevant to SDL3. | ||
| #if !SDL_VERSION_ATLEAST(3, 0, 0) | ||
| if ((surf->flags & SDL_RLEACCEL) && (!(flags & PGS_RLEACCEL))) { | ||
| if (SDL_MUSTLOCK(surf) && (!(flags & PGS_RLEACCEL))) { | ||
| /* hack to strip SDL_RLEACCEL flag off surface immediately when | ||
| it is not requested */ | ||
| SDL_Rect sdlrect; | ||
| sdlrect.x = 0; | ||
| sdlrect.y = 0; | ||
| sdlrect.h = 0; | ||
| sdlrect.w = 0; | ||
| sdlrect.h = 1; | ||
| sdlrect.w = 1; | ||
|
|
||
| SDL_Surface *surface = | ||
| PG_CreateSurface(1, 1, PG_SURF_FORMATENUM(surf)); | ||
|
|
||
| SDL_LowerBlit(surf, &sdlrect, surface, &sdlrect); | ||
| SDL_FreeSurface(surface); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
This is some extremely confusing historic code to be changing without a comment or a note in the description or anything about how this makes sense and doesn't change SDL2 behavior.
There was a problem hiding this comment.
It doesn't change SDL2 behaviour at all because it is changing the srcrect argument of a blit whose destination is a temporary surface that is immediately destroyed after the said blit.
On SDL3 the same code was segfaulting because of the 0,0,0,0 rect, and hence I thought the fix is worth doing on both SDL2 and SDL3 codepaths because it is likely to segfault under SDL2-compat as well.
And yes, I have verified that this hack is still needed on SDL3, it was needed to get a unit test to pass.
ankith26
force-pushed
the
ankith26-sdl3-3
branch
from
14eafac to
697c4c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src_c/surface.c (2)
4450-4518: This issue was already flagged in a previous review.The PG_BlitSurface implementation still has three bugs identified in the earlier review:
- Inverted return code at line 4508 (returns 0 on error instead of -1)
- Passes original srcrect/dstrect to SDL_BlitSurface instead of computed r_src/r_dst
- Missing adjustment of r_src when r_dst is clipped at lines 4492-4502
Refer to the previous review comment for the detailed fix.
4594-4622: This issue was already flagged in a previous review.The SDL3 SDL_ConvertSurface call at lines 4597-4601 is missing the required flags argument (should be 3 arguments, currently has 2). Refer to the previous review comment for the fix.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/reST/c_api/surface.rst(1 hunks)src_c/_pygame.h(3 hunks)src_c/draw.c(1 hunks)src_c/font.c(1 hunks)src_c/pixelarray_methods.c(1 hunks)src_c/pixelcopy.c(1 hunks)src_c/surface.c(10 hunks)src_c/surface_fill.c(1 hunks)test/surface_test.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src_c/surface_fill.c
- src_c/font.c
- docs/reST/c_api/surface.rst
- src_c/draw.c
- src_c/pixelcopy.c
- src_c/_pygame.h
🧰 Additional context used
🧠 Learnings (4)
📓 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.
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_camera.c:129-131
Timestamp: 2025-08-30T21:11:00.240Z
Learning: When doing SDL2 to SDL3 compatibility changes, Starbuck5 prefers to maintain exact existing behaviors even when other improvements could be made, focusing solely on the compatibility aspect rather than mixing in behavioral fixes.
Learnt from: oddbookworm
Repo: pygame-community/pygame-ce PR: 0
File: :0-0
Timestamp: 2025-11-03T19:49:28.177Z
Learning: For translation pull requests in the pygame-ce repository: Focus only on verifying direct translation accuracy from the English source. Do not suggest new content or enhancements during translation PRs. New content should be added to the English version first in a separate pull request.
Learnt from: damusss
Repo: pygame-community/pygame-ce PR: 3219
File: src_c/window.c:11-13
Timestamp: 2025-10-17T08:53:32.449Z
Learning: SDL3's first stable release is version 3.2.0 (released January 21, 2025). SDL 3.0.x and 3.1.x were preview/pre-release versions not intended for production use, so version checks for SDL3 features in pygame-ce should use SDL_VERSION_ATLEAST(3, 2, 0) to target the first stable release.
📚 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:
test/surface_test.pysrc_c/pixelarray_methods.csrc_c/surface.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/pixelarray_methods.csrc_c/surface.c
📚 Learning: 2025-08-30T21:11:00.240Z
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_camera.c:129-131
Timestamp: 2025-08-30T21:11:00.240Z
Learning: When doing SDL2 to SDL3 compatibility changes, Starbuck5 prefers to maintain exact existing behaviors even when other improvements could be made, focusing solely on the compatibility aspect rather than mixing in behavioral fixes.
Applied to files:
src_c/surface.c
🧬 Code graph analysis (2)
src_c/pixelarray_methods.c (1)
src_c/_pygame.h (2)
PG_ConvertSurface(82-88)PG_ConvertSurface(241-245)
src_c/surface.c (4)
src_c/_pygame.h (2)
PG_ConvertSurface(82-88)PG_ConvertSurface(241-245)src_c/base.c (1)
pg_EnvShouldBlendAlphaSDL2(1969-1973)src_c/transform.c (1)
_PgSurface_SrcAlpha(94-116)src_c/alphablit.c (1)
pygame_Blit(1582-1687)
⏰ 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). (20)
- GitHub Check: x86
- GitHub Check: AMD64
- GitHub Check: dev-check
- GitHub Check: Pyodide build
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.10.17)
- GitHub Check: i686
- GitHub Check: x86_64
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: aarch64
- GitHub Check: build (macos-14)
- GitHub Check: build (windows-latest)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (5)
test/surface_test.py (1)
412-416: Same verification needed for SDL3 upper bound.This decorator has the same broad SDL3 version range (through 3.2.22) as the previous test. Please verify if this upper bound is accurate or if it could be narrowed based on when the fix was actually released.
src_c/pixelarray_methods.c (1)
140-140: LGTM! API change aligns with SDL3 palette handling.The change from
PG_ConvertSurface(temp_surf, surf->format)toPG_ConvertSurface(temp_surf, surf)correctly adopts the new API signature that passes the destination surface object instead of just its format. This enables the SDL3 path to copy the palette from the destination surface, which is necessary because SDL3 separates the palette from the pixel format structure.Based on learnings
src_c/surface.c (3)
1623-1623: LGTM: Surface-to-surface conversion for palette copying.The change from
PG_ConvertSurface(surf, surf->format)toPG_ConvertSurface(surf, surf)correctly implements the new signature that copies the palette from the source surface, aligning with the PR objectives.
3010-3018: LGTM: Improved RLE flag checking comments.The added comments clearly distinguish between RLE requested (PGS_RLEACCELOK) and RLE actually present (PGS_RLEACCEL via SDL_MUSTLOCK), and note the SDL2 implementation detail.
4532-4534: LGTM: Consistent error return convention.The change from
return -1;toreturn 1;aligns with the function's return convention at line 4661 (return result != 0;), where 0 indicates success and 1 indicates failure.
PG_ConvertSurfacenow takes a surface as its second argument so it can copy the palette from it, to compat for the SDL2 path already doing it because there palette was part of the format.PG_GetSurfacePalettenow creates a palette when the surface is indexed, SDL2 did it implicitly.PG_BlitSurfacebecause SDL3 doesn't modify dstrect anymore.