[Part 3] SDL3: surface+pixelarray+font+draw: runtime fixes #3578
Conversation
📝 WalkthroughWalkthroughThe patch replaces several surface-related macros with inline functions, introduces a PG_BlitSurface wrapper for SDL3-aware blitting, updates call sites to pass destination surfaces instead of formats for conversions, unifies palette access via PG_GetSurfacePalette, adjusts an error code path in surface_fill, and corrects a documentation return-code description. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant PB as PG_BlitSurface
participant S as SDL
C->>PB: blit(src, srcrect, dst, dstrect)
alt SDL3
PB->>PB: Clip/adjust dstrect (SDL3 semantics)
PB->>S: SDL_BlitSurface(src, srcrect, dst, dstrect)
S-->>PB: status
else SDL2
PB->>S: SDL_BlitSurface(src, srcrect, dst, dstrect)
S-->>PB: status (dstrect modified by SDL)
end
PB-->>C: 0 on success, non-zero on error
sequenceDiagram
autonumber
participant C as Caller
participant PC as PG_ConvertSurface
participant S as SDL
C->>PC: convert(surface, dst_surface)
alt SDL3
PC->>S: SDL_ConvertSurfaceAndColorspace(surface, dst.format, dst.palette, dst.colorspace)
else SDL2
PC->>S: SDL_ConvertSurface(surface, dst.format, 0)
end
S-->>PC: converted_surface or NULL
PC-->>C: result
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
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.