Fix SRCALPHA TGA image saving #3432
Conversation
|
Note that with this PR my aim is to not change existing behaviour for other types of images, even though I have noticed inconsistencies with the code.
|
|
What do you think about dropping TGA in 3.0? It's not exactly a popular format. |
|
Given the inconsistencies and the fact that we maintain this code on our side, it makes sense to drop it in a breaking release IMO |
ankith26
force-pushed
the
ankith26-fix-alpha-tga
branch
from
310feac to
3f9d8b6
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughInternal SaveTGA_RW logic now preserves/restores SDL surface state using blend modes instead of alpha/color-key toggles. It fetches and sets the surface blend mode around the TGA save operation. A new test validates saving/loading a SRCALPHA surface preserves pixel data. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant SaveTGA_RW
participant Surface
participant SDL
Test->>SaveTGA_RW: Save(surface, rw)
SaveTGA_RW->>Surface: PG_GetSurfaceBlendMode()
alt success
SaveTGA_RW->>Surface: PG_SetSurfaceBlendMode(SDL_BLENDMODE_NONE)
SaveTGA_RW->>SDL: Copy/Write TGA pixel data
SaveTGA_RW->>Surface: PG_SetSurfaceBlendMode(original)
SaveTGA_RW-->>Test: result
else failure
SaveTGA_RW-->>Test: error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 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
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
src_c/image.c (1)
1825-1848: Don’t silently succeed on partial write/blit failures; return an error.Currently, failures inside the row loop use break, but the function then restores the blend mode and returns 0 (success). This can mask I/O or blit errors (e.g., disk full), leading to corrupted files reported as successful saves.
Change the break statements to jump to an error path that restores the original blend mode and returns -1.
Apply this diff to ensure errors propagate correctly:
@@ - for (r.y = 0; r.y < surface->h; r.y++) { + for (r.y = 0; r.y < surface->h; r.y++) { int n; void *buf; #if SDL_VERSION_ATLEAST(3, 0, 0) - if (!SDL_BlitSurface(surface, &r, linebuf, NULL)) + if (!SDL_BlitSurface(surface, &r, linebuf, NULL)) #else - if (SDL_BlitSurface(surface, &r, linebuf, NULL) < 0) + if (SDL_BlitSurface(surface, &r, linebuf, NULL) < 0) #endif - break; + goto write_failure; @@ #if SDL_VERSION_ATLEAST(3, 0, 0) - if (!SDL_WriteIO(out, buf, n)) + if (!SDL_WriteIO(out, buf, n)) #else - if (!SDL_RWwrite(out, buf, n, 1)) + if (!SDL_RWwrite(out, buf, n, 1)) #endif - break; + goto write_failure; } - /* restore flags */ + /* restore flags */ if (!PG_SetSurfaceBlendMode(surface, surf_blendmode)) { goto error; } @@ SDL_FreeSurface(linebuf); return 0; +write_failure: + /* restore flags before error exit */ + (void)PG_SetSurfaceBlendMode(surface, surf_blendmode); + goto error; error: free(rlebuf); SDL_FreeSurface(linebuf); return -1;Note: We intentionally restore the blend mode in the failure path to avoid leaving the caller’s surface mutated on error.
🧹 Nitpick comments (2)
test/image_test.py (1)
418-450: Seed randomness for deterministic tests to avoid flakiness.The test compares load/save round-trip values, so any random data works. However, leaving RNG unseeded can make the test nondeterministic across runs and platforms, which complicates reproducing failures. Use a local seeded Random to keep the sample stable while retaining good coverage across RGBA values.
Apply this diff to make the test deterministic without relying on global RNG state:
def test_save_tga_srcalpha(self): WIDTH = 10 HEIGHT = 10 s = pygame.Surface((WIDTH, HEIGHT), pygame.SRCALPHA) + rand = random.Random(0) pixels = [ [ ( - random.randint(0, 255), - random.randint(0, 255), - random.randint(0, 255), - random.randint(0, 255), + rand.randint(0, 255), + rand.randint(0, 255), + rand.randint(0, 255), + rand.randint(0, 255), ) for _ in range(WIDTH) ] for _ in range(HEIGHT) ]src_c/image.c (1)
1724-1726: Blend-mode push/pop around the blit is the right fix for SRCALPHA; confirm colorkey interaction.Setting SDL_BLENDMODE_NONE before blitting and restoring afterwards correctly prevents per-pixel alpha blending during the TGA copy. One caveat: in SDL, colorkey handling is independent of the blend mode. If the source surface has a colorkey, SDL_BlitSurface may still omit colorkey pixels, leaving stale data in linebuf. The old code explicitly toggled colorkey off during the save; this change replaces that with only a blend-mode change.
Please verify on 8bpp (indexed) surfaces with a colorkey that the saved TGA’s pixel indices are copied verbatim (and transparency is expressed via the palette’s alpha as you already do), not via skipped blits. If needed, consider temporarily disabling and restoring the colorkey around the blit (with SDL2’s SDL_SetColorKey(surface, SDL_FALSE/TRUE, key) or SDL3’s SDL_SetSurfaceColorKey equivalent) to guarantee raw copies.
Would you like me to draft a unit test that sets a palette colorkey on an 8bpp surface, saves to TGA, reloads, and asserts that colorkey pixels come back with alpha 0 and correct color?
Also applies to: 1817-1820, 1851-1853
📜 Review details
Configuration used: CodeRabbit UI
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 (2)
src_c/image.c(4 hunks)test/image_test.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/image_test.py (3)
src_py/__init__.py (1)
Surface(233-234)buildconfig/stubs/pygame/image.pyi (2)
save(158-178)load(81-114)test/test_utils/png.py (1)
save(1299-1327)
src_c/image.c (1)
src_c/_pygame.h (2)
PG_GetSurfaceBlendMode(257-261)PG_SetSurfaceBlendMode(251-255)
🔇 Additional comments (1)
test/image_test.py (1)
7-7: Import looks correct and scoped to the new test.No issues spotted with adding random for data generation.
This PR is a fix for an issue @fladd raised on the og pygame repo.
Essentially, images with per pixel alpha were not being saved to TGA correctly because there was blending happening in intermediate steps that should not happen. This was being prevented for blanket alpha and color key alpha cases already in the code, but not for per pixel alpha cases. This PR implements a more generic blendmode based approach so that all cases are handled correctly.
Reproducer code for this issue (by @fladd, from the original issue)
Summary by CodeRabbit
Bug Fixes
Tests