[Part 5] SDL3: image+scrap+transform+debug: runtime fixes #3580
Conversation
📝 WalkthroughWalkthroughSDL3-compatibility updates across C and Python: unify pixel-format loss access via PG_FORMAT_* macros and swap SDL_GetSurfacePalette for PG_GetSurfacePalette in image paths; adjust SDL_SetClipboardText return semantics handling in scrap modules; align boolean parsing by using int locals in transform functions; make _debug mixer imports lazy with fallbacks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor PyCode as Python caller
participant Scrap as scrap(_scrap_put_text/pygame_scrap_put)
participant SDL as SDL
PyCode->>Scrap: set_clipboard_text(text)
Scrap->>SDL: SDL_SetClipboardText(text)
alt SDL >= 3.0.0
SDL-->>Scrap: return non-zero on success
alt success (non-zero)
Scrap-->>PyCode: success (return)
else failure (zero)
Scrap-->>PyCode: raise error
end
else SDL < 3.0.0
SDL-->>Scrap: return 0 on success
alt success (zero)
Scrap-->>PyCode: success (return)
else failure (non-zero)
Scrap-->>PyCode: raise error
end
end
sequenceDiagram
autonumber
participant Img as image.c (image_tobytes/SaveTGA_RW)
participant Surf as SDL_Surface
participant Fmt as Format details
Img->>Surf: access pixel format
note over Img,Fmt: Use PG_FORMAT_* macros for *_LOSS fields
Img->>Fmt: PG_FORMAT_[RGBA]_LOSS(...)
Img->>Surf: PG_GetSurfacePalette(surf)
Img-->>Img: proceed with serialization (unchanged)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src_c/scrap.c (1)
412-420: Handle NULL from SDL_GetClipboardText to avoid crash.SDL_GetClipboardText can return NULL on failure; dereferencing
*textwould segfault. Raise on NULL before using*text.- char *text = SDL_GetClipboardText(); + char *text = SDL_GetClipboardText(); + if (text == NULL) { + return RAISE(pgExc_SDLError, SDL_GetError()); + }src_c/image.c (1)
699-703: Channel-loss bug in RGB/32bpp path (G/B use Rloss).
image_tobytes(..., "RGB")for 32bpp surfaces assigns G/B usingRloss, producing wrong colors.- data[1] = (char)(((color & Gmask) >> Gshift) << Rloss); - data[2] = (char)(((color & Bmask) >> Bshift) << Rloss); + data[1] = (char)(((color & Gmask) >> Gshift) << Gloss); + data[2] = (char)(((color & Bmask) >> Bshift) << Bloss);
🧹 Nitpick comments (2)
src_py/_debug.py (1)
66-69: Graceful mixer imports + small output nit.
- Dynamic imports with fallbacks are a solid robustness improvement.
- Add a trailing newline after “Mixer Driver” for consistent formatting.
- if mixer_init(): - debug_str += f"Mixer Driver:\t\t{get_mixer_driver()}" + if mixer_init(): + debug_str += f"Mixer Driver:\t\t{get_mixer_driver()}\n"Also applies to: 110-120, 121-139, 149-151, 181-184
src_c/transform.c (1)
3741-3744: Switching “p”-parsed flags to int is correct; cast at call sites.
PyArg_ParseTuple(..., "p")writes to int. Consider explicit casts when passing to functions expecting SDL_bool to avoid warnings and make intent clear.- average_color(surf, surf_format, x, y, w, h, &r, &g, &b, &a, - consider_alpha); + average_color(surf, surf_format, x, y, w, h, &r, &g, &b, &a, + (SDL_bool)consider_alpha);- new_surf = - blur(src_surf_obj, dst_surf_obj, radius, repeat_edge_pixels, 'b'); + new_surf = + blur(src_surf_obj, dst_surf_obj, radius, (SDL_bool)repeat_edge_pixels, 'b');- new_surf = - blur(src_surf_obj, dst_surf_obj, radius, repeat_edge_pixels, 'g'); + new_surf = + blur(src_surf_obj, dst_surf_obj, radius, (SDL_bool)repeat_edge_pixels, 'g');Also applies to: 4055-4064, 4087-4097
📜 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 (5)
src_c/image.c(3 hunks)src_c/scrap.c(1 hunks)src_c/scrap_sdl2.c(1 hunks)src_c/transform.c(3 hunks)src_py/_debug.py(2 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.
🔇 Additional comments (3)
src_c/scrap.c (1)
444-450: SDL3/SDL2 return-semantics guard looks correct.Good: SDL3 treats 0/false as failure; SDL2 treats non‑zero as failure. Error path returns a Python exception.
src_c/scrap_sdl2.c (1)
112-117: Clipboard set: version-aware success check is correct.SDL3 path treats truthy return as success; SDL2 path treats 0 as success. Function returns 1 on success consistently.
src_c/image.c (1)
457-461: Nice unification: PG_FORMAT_ macros and PG_GetSurfacePalette.*
- Using PG_FORMAT_* removes SDL2/SDL3 field-branching.
- Switching to PG_GetSurfacePalette aligns with the rest of the codebase and SDL3.
Also applies to: 555-565, 566-570, 1714-1715
| @@ -565,19 +558,15 @@ image_tobytes(PyObject *self, PyObject *arg, PyObject *kwarg) | |||
| if (!format_details) { | |||
There was a problem hiding this comment.
This whole SDL_VERSION_ATLEAST could be removed by using PG_GetSurfaceDetails, same with the block below (+PG_PixelFormatEnum) for that one, if you're reducing redundancy on these.
There was a problem hiding this comment.
Good point. But the main reason for me doing these loss changes is not reducing redundancy, it is fixing behavior because bits != loss and our compat macro handles the conversion.
I want to keep the diff minimal in these runtime fixes PRs, cleanups and stuff can always be done in future PRs.
probably the easiest pr of the lot to review