Use SDL_AllocFormat instead of creating it manually #3639
+41
−58
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
📝 WalkthroughWalkthroughReplaces stack-allocated SDL_PixelFormat usage with dynamically allocated SDL_PixelFormat* via SDL_AllocFormat(SDL_MasksToPixelFormatEnum(...)) across surf_convert, surf_convert_alpha, and pgSurface_Blit in src_c/surface.c; applies palettes with SDL_SetPixelFormatPalette(...) for indexed formats and ensures SDL_FreeFormat(...) is called on all paths. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Blit/Convert caller
participant Src as Source surface
participant MaskEnum as SDL_MasksToPixelFormatEnum
participant Alloc as SDL_AllocFormat
participant Pal as SDL_SetPixelFormatPalette
participant Conv as SDL_ConvertSurface / PG_ConvertSurface
participant Free as SDL_FreeFormat
participant Dest as Destination surface
Caller->>MaskEnum: compute format_enum from masks/bpp
MaskEnum-->>Caller: format_enum
Caller->>Alloc: SDL_AllocFormat(format_enum)
Alloc-->>Caller: format*
alt indexed format
Caller->>Pal: SDL_SetPixelFormatPalette(format*, palette)
end
Caller->>Conv: convert source using format* -> converted_src
Conv-->>Caller: converted_src
Caller->>Free: SDL_FreeFormat(format*)
Caller->>Dest: SDL_BlitSurface(converted_src -> Dest)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
ankith26
approved these changes
Nov 8, 2025
Member
ankith26
left a comment
ankith26
left a comment
There was a problem hiding this comment.
LGTM, thanks for contributing to pygame-ce! 🎉
marcin-serwin
force-pushed
the
push-wmkrxtzwskyk
branch
from
f4fb43e to
3e3fbdc
Compare
Author
|
I've run into the same issue in |
ankith26
reviewed
Nov 9, 2025
marcin-serwin
force-pushed
the
push-wmkrxtzwskyk
branch
from
3e3fbdc to
8b0eda1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (1)
src_c/surface.c (1)
4533-4545: Handle NULL from SDL_AllocFormat in blit pathThe blit fallback has the same risk: if
SDL_MasksToPixelFormatEnumyields something unsupported or we hit OOM,SDL_AllocFormatreturnsNULL. Passing that straight intoPG_ConvertSurfacewill dereference it and crash. Guard the allocation, only blit once a converted surface exists, and propagate the error otherwise.else { SDL_PixelFormat *fmt = src->format; - SDL_PixelFormat *newfmt = - SDL_AllocFormat(SDL_MasksToPixelFormatEnum( - fmt->BitsPerPixel, fmt->Rmask, fmt->Gmask, fmt->Bmask, 0)); - - src = PG_ConvertSurface(src, newfmt); - - SDL_FreeFormat(newfmt); - if (src) { + Uint32 tmp_format_enum = + SDL_MasksToPixelFormatEnum(fmt->BitsPerPixel, fmt->Rmask, + fmt->Gmask, fmt->Bmask, 0); + SDL_PixelFormat *newfmt = SDL_AllocFormat(tmp_format_enum); + SDL_Surface *converted = NULL; + + if (!newfmt) { + result = -1; + } + else { + converted = PG_ConvertSurface(src, newfmt); + SDL_FreeFormat(newfmt); + } + + if (converted) { + src = converted; #if SDL_VERSION_ATLEAST(3, 0, 0) result = SDL_BlitSurface(src, srcrect, dst, dstrect) ? 0 : -1; #else result = SDL_BlitSurface(src, srcrect, dst, dstrect); #endif SDL_FreeSurface(src); } else { result = -1; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src_c/surface.c(3 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/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/surface.c
🧬 Code graph analysis (1)
src_c/surface.c (2)
src_c/base.c (1)
pg_UintFromObjIndex(591-604)src_c/surflock.c (1)
pgSurface_Unprep(51-59)
marcin-serwin
force-pushed
the
push-wmkrxtzwskyk
branch
2 times, most recently
from
3186ee9 to
ffd60e2
Compare
ankith26
reviewed
Nov 10, 2025
marcin-serwin
force-pushed
the
push-wmkrxtzwskyk
branch
from
ffd60e2 to
035dd86
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src_c/surface.c (1)
1846-1846: Consider conditional palette allocation.The palette is allocated unconditionally at line 1846 but only used for indexed formats (lines 1944-1956). This means non-indexed conversions allocate and free an unused palette. The SDL3 path (lines 1779-1790) demonstrates a more efficient approach by only allocating the palette when
SDL_ISPIXELFORMAT_INDEXED(format_enum)is true.Consider adopting the SDL3 pattern:
- SDL_Palette *palette = SDL_AllocPalette(default_palette_size); + SDL_Palette *palette = NULL; Uint32 format_enum = 0; if (pg_IntFromObj(argobject, &bpp)) { // ... compute format_enum ... } // ... other branches ... + SDL_PixelFormat *format = SDL_AllocFormat(format_enum); if (!format) { - SDL_FreePalette(palette); pgSurface_Unprep(self); return RAISE(pgExc_SDLError, SDL_GetError()); } if (SDL_ISPIXELFORMAT_INDEXED(format_enum)) { if (SDL_ISPIXELFORMAT_INDEXED(PG_SURF_FORMATENUM(surf))) { SDL_SetPixelFormatPalette(format, surf->format->palette); } else { + palette = SDL_AllocPalette(default_palette_size); + if (!palette) { + SDL_FreeFormat(format); + pgSurface_Unprep(self); + return RAISE(pgExc_SDLError, SDL_GetError()); + } SDL_SetPaletteColors(palette, default_palette_colors, 0, default_palette_size); SDL_SetPixelFormatPalette(format, palette); } } newsurf = PG_ConvertSurface(surf, format); SDL_SetSurfaceBlendMode(newsurf, SDL_BLENDMODE_NONE); SDL_FreeFormat(format); - SDL_FreePalette(palette); + if (SDL_ISPIXELFORMAT_INDEXED(format_enum) && + !SDL_ISPIXELFORMAT_INDEXED(PG_SURF_FORMATENUM(surf))) { + SDL_FreePalette(palette); + }Also applies to: 1960-1960
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src_c/surface.c(3 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/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/surface.c
🧬 Code graph analysis (1)
src_c/surface.c (2)
src_c/base.c (1)
pg_UintFromObjIndex(591-604)src_c/surflock.c (1)
pgSurface_Unprep(51-59)
According to the docs, `SDL_PixelFormat` is a read-only structure. Creating it manually leaves out some important fields like `format` and `next` pointer to be undefined. Signed-off-by: Marcin Serwin <marcin@serwin.dev>
marcin-serwin
force-pushed
the
push-wmkrxtzwskyk
branch
from
035dd86 to
a7ff64e
Compare
Starbuck5
reviewed
Nov 11, 2025
Member
Starbuck5
left a comment
Starbuck5
left a comment
There was a problem hiding this comment.
I feel like I wanted to do stuff like this when porting SDL3 but I (probably with Ankith's encouragement), decided not to, in order to just leave the old code functioning how it is.
What is the justification for changing this code?
Author
Since libsdl-org/sdl2-compat#521 sdl2-compat relies on the |
Member
|
It is interesting to note that the PR has been (partially) reverted as of libsdl-org/sdl2-compat#545 which was merged just a few days ago, which may change things. However I think this PR is still worth merging because it is making the code more robust. |
13 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
According to the docs,
SDL_PixelFormatis a read-only structure. Creating it manually leaves out some important fields likeformatandnextpointer to be undefined.