[Part 3] SDL3: surface+pixelarray+font+draw: runtime fixes #3578
Conversation
📝 WalkthroughWalkthroughReplaces several surface-related macros with SDL-version-aware inline functions, unifies palette access via PG_GetSurfacePalette, changes conversion callsites to pass surfaces, 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 PB as PG_BlitSurface
participant SDL as SDL
App->>PB: PG_BlitSurface(src, srcrect, dst, dstrect)
alt SDL3
PB->>PB: adjust/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
sequenceDiagram
autonumber
participant Call as Caller
participant PC as PG_ConvertSurface
participant SDL as SDL
Call->>PC: PG_ConvertSurface(surface, dst_surface)
alt SDL3
PC->>SDL: SDL_ConvertSurfaceAndColorspace(surface, dst->format, dst->palette, dst->colorspace)
else SDL2
PC->>SDL: SDL_ConvertSurface(surface, dst->format, 0)
end
SDL-->>PC: converted_surface or NULL
PC-->>Call: converted_surface or NULL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![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.
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.