Fix PG_SURF_BitsPerPixel SDL3 #3573
Conversation
📝 WalkthroughWalkthroughReplaced the SDL3-path Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Code using surface
participant Func as PG_SURF_BitsPerPixel(surf)
participant Surf as SDL_Surface
Caller->>Func: call with surf
Func->>Surf: inspect surf->format (FourCC?)
alt format is YUY2/UYVY/YVYU
Func-->>Caller: return 32
else format is other FourCC
Func-->>Caller: return 0
else not FourCC
Func->>Surf: bytes = SDL_BYTESPERPIXEL(surf->format)
alt bytes <= 2
Func-->>Caller: return SDL_BITSPERPIXEL(surf->format)
else bytes > 2
Func-->>Caller: return bytes * 8
end
end
note right of Func #EEF6FF: Inline function replaces prior macro for SDL3 path
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src_c/transform.c (1)
959-963: Handle allocation/blit failures before calling rotozoomSurfacePG_CreateSurface or SDL_BlitSurface can fail; today these errors fall through and may crash or corrupt output. Check and bail with a clear error, freeing intermediates.
Apply this diff inside the else-branch:
- Py_BEGIN_ALLOW_THREADS; - surf32 = PG_CreateSurface(surf->w, surf->h, SDL_PIXELFORMAT_ABGR8888); - SDL_BlitSurface(surf, NULL, surf32, NULL); - Py_END_ALLOW_THREADS; + Py_BEGIN_ALLOW_THREADS; + surf32 = PG_CreateSurface(surf->w, surf->h, SDL_PIXELFORMAT_ABGR8888); + int blit_rc = 0; +#if SDL_VERSION_ATLEAST(3, 0, 0) + if (surf32) { + blit_rc = SDL_BlitSurface(surf, NULL, surf32, NULL) ? 0 : -1; + } else { + blit_rc = -1; + } +#else + if (surf32) { + blit_rc = SDL_BlitSurface(surf, NULL, surf32, NULL); + } else { + blit_rc = -1; + } +#endif + Py_END_ALLOW_THREADS; + if (!surf32 || blit_rc < 0) { + if (surf32) SDL_FreeSurface(surf32); + PyErr_SetString(pgExc_SDLError, SDL_GetError()); + return NULL; + }
🧹 Nitpick comments (2)
src_c/surface.c (1)
3122-3128: get_bitsize now reflects “bits used” (not padding) — verify docs/testsReturning PG_FORMAT_BitsPerPixel(fmt) matches SDL3’s semantics (e.g., RGBX => 24). Ensure DOC_SURFACE_GETBITSIZE and tests expect 24 (not 32) for padded 4-byte formats on SDL3 to avoid confusion.
I can update/add tests asserting (surface.get_bitsize(), surface.get_bytesize()) == (24, 4) for RGBX/XRGB on SDL3 and adjust docs accordingly. Want a patch?
src_c/rotozoom.c (1)
527-529: BytesPerPixel-based checks eliminate the SDL3 mismatch—good change.Minor nit: compute once to avoid the second macro call and improve readability.
- is32bit = (PG_SURF_BytesPerPixel(src) == 4); - if ((is32bit) || (PG_SURF_BytesPerPixel(src) == 1)) { + int src_bpp_bytes = PG_SURF_BytesPerPixel(src); + is32bit = (src_bpp_bytes == 4); + if (is32bit || src_bpp_bytes == 1) {
📜 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 (7)
src_c/_camera.c(1 hunks)src_c/_pygame.h(0 hunks)src_c/display.c(1 hunks)src_c/image.c(1 hunks)src_c/rotozoom.c(1 hunks)src_c/surface.c(3 hunks)src_c/transform.c(1 hunks)
💤 Files with no reviewable changes (1)
- src_c/_pygame.h
🧰 Additional context used
🧬 Code graph analysis (1)
src_c/surface.c (1)
src_c/_pygame.h (2)
PG_GetSurfaceFormat(129-133)PG_GetSurfaceFormat(241-245)
⏰ 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_64
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: build
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: AMD64
- GitHub Check: x86
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: dev-check
- GitHub Check: aarch64
- GitHub Check: i686
- GitHub Check: x86_64
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (4)
src_c/display.c (1)
1808-1814: Depth check switch to BytesPerPixel is correct for SDL3 semantics.Using PG_SURF_BytesPerPixel(surface->surf) * 8 avoids the SDL3 BitsPerPixel ambiguity (e.g., XRGB8888: 32 vs 24 used-bits). Change looks good.
src_c/transform.c (1)
954-957: Correct switch to BytesPerPixel for SDL3 semanticsUsing PG_SURF_BytesPerPixel(surf) == 4 correctly handles RGBX/XRGB (24bpp with 4-byte stride) on SDL3, fixing false negatives from BitsPerPixel==32 checks. Good call.
src_c/surface.c (1)
444-448: Use format details to compute bit size (SDL3-safe)
- Fetching the pixel format via PG_GetSurfaceFormat with NULL-check is correct.
- Switching to PG_FORMAT_BitsPerPixel(fmt) in surface_str keeps the repr accurate under SDL3.
Also applies to: 473-474
src_c/image.c (1)
1711-1713: Approve: switch to PG_SURF_BytesPerPixel*8 is correct and SDL3-safe
Verified no remaining PG_SURF_BitsPerPixel calls insrc_c. LGTM.
Starbuck5
force-pushed
the
sdl3_bitsperpixel
branch
from
1276caa to
6da0210
Compare
Starbuck5
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src_c/_pygame.h (4)
108-112: Clarify the intent of this helper and its SDL2/SDL3 semantics.The current comment suggests this is an “implementation from SDL_GetMasksForPixelFormat”, but the function re-derives BPP with extra heuristics and special-cases. Please tighten the doc to state the exact contract (SDL2-style total bits vs SDL3 “significant bits”), and why the heuristics exist.
Apply:
-// Implementation from SDL_GetMasksForPixelFormat, which is used by -// SDL_InitPixelFormatDetails in SDL_pixels.c -// Created to match surf->format->BitsPerPixel in SDL2, -// details->bits_per_pixel in SDL3. +// Helper to produce SDL2-style “total bits per pixel” from an SDL3 surface. +// SDL3's SDL_BITSPERPIXEL() may exclude pad bits on some formats (e.g., RGBX), +// so we re-derive a value consistent with SDL2's surf->format->BitsPerPixel. +// See SDL_InitPixelFormatDetails / SDL_PixelFormatDetails for reference.
128-132: Avoid heuristic threshold; use PixelFormatDetails to prevent drift.The “<= 2 bytes use BITSPERPIXEL else bytes*8” rule is brittle (e.g., 12/15 bpp corner cases, future formats). Prefer SDL_PixelFormatDetails as the single source of truth and only fall back if unavailable.
-static inline int -PG_SURF_BitsPerPixel(SDL_Surface *surf) -{ - if (SDL_ISPIXELFORMAT_FOURCC(surf->format)) { - // however, some of these are packed formats, and can legit declare - // bits-per-pixel! - switch (surf->format) { - case SDL_PIXELFORMAT_YUY2: - case SDL_PIXELFORMAT_UYVY: - case SDL_PIXELFORMAT_YVYU: - return 32; - default: - return 0; // oh well. - } - } - - if (SDL_BYTESPERPIXEL(surf->format) <= 2) { - return SDL_BITSPERPIXEL(surf->format); - } - return SDL_BYTESPERPIXEL(surf->format) * 8; -} +static inline int +PG_SURF_BitsPerPixel(const SDL_Surface *surf) +{ + const SDL_PixelFormatDetails *d = SDL_GetPixelFormatDetails(surf->format); + if (d) { + // SDL2-style “total bits”: prefer bytes_per_pixel*8 for wide formats, + // otherwise use details->bits_per_pixel. + return (d->bytes_per_pixel > 2) ? (int)(d->bytes_per_pixel * 8) + : (int)d->bits_per_pixel; + } + return 0; +}
112-114: Minor signature tweak: accept const SDL_Surface.*Function does not mutate the surface; const improves call-site flexibility and communicates intent.
-static inline int -PG_SURF_BitsPerPixel(SDL_Surface *surf) +static inline int +PG_SURF_BitsPerPixel(const SDL_Surface *surf)
108-133: Add tests covering representative formats across SDL2/SDL3.Lock in behavior with a small matrix: INDEX8, RGB565, RGB24, RGBX8888/XRGB8888, RGBA8888, YUY2/UYVY/YVYU, and a 64bpp format. Assert consistency with the intended “total bits” semantics.
📜 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 (1)
src_c/_pygame.h(1 hunks)
⏰ 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: arm64
- GitHub Check: x86_64
- GitHub Check: aarch64
- GitHub Check: AMD64
- GitHub Check: x86
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: x86_64
- GitHub Check: dev-check
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: i686
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: build (ubuntu-24.04)
|
@ankith26 You gave feedback on the first pass of this PR that you were uneasy about changing SDL2 codepaths. I've reworked this PR to fix the macro instead of removing it, after doing a bit more research (I didn't think it could be fixed initially). This PR now is highly correct and also doesn't change the SDL2 path at all. EDIT: Had to force push again because I misnamed the commit |
Starbuck5
force-pushed
the
sdl3_bitsperpixel
branch
from
6da0210 to
833fd08
Compare
It's inconsistent between SDL2 and SDL3,
and I see no way to fix it.EDIT: looked into the source code and looked at how the pixelformat bpp gets populated, in both SDL2 and SDL3, and took the SDL3 implementation to implement here.
In SDL2, (surf->format->BitsPerPixel), the value means the number of bits each pixel occupies in total. For example, RGBX => 32.
In SDL3, (SDL_BITSPERPIXEL(surf->format)), the value means the number of pixels occupied by the pixel's data, not including padding. So RGBX => 24.
These changes reduce SDL3 unit test failures.