Skip to content

Conversation

ankith26
Copy link
Contributor

@ankith26 ankith26 commented Jun 3, 2025

This PR is an attempt to make the behaviour of SavePNG more consistent across backends (miniz and libpng). With this PR, images that don't need alpha will now be stored as RGB instead of RGBA.

@ankith26 ankith26 force-pushed the ankith26-save-rgb-png branch 3 times, most recently from 25cdca2 to c996880 Compare June 4, 2025 10:53
@ankith26
Copy link
Contributor Author

ankith26 commented Jun 4, 2025

As a side goal of this PR, the code is now more robust and handles arbitrary pitches. It seems like miniz always expects a tight pitch (and generates corrupted PNGs if this isn't the case), but the input surface or the surface output of SDL_ConvertSurfaceFormat need not always give out a surface with tight pitch (especially in the RGB case where it always seemingly aligns the pitch to a multiple of 4)

@ankith26 ankith26 changed the title Save non-alpha pngs as RGB not RGBA (miniz backend) Save non-alpha pngs as RGB not RGBA, handle arbitrary surface pitch properly (miniz backend) Jun 4, 2025
@ankith26 ankith26 force-pushed the ankith26-save-rgb-png branch from c996880 to 6a42626 Compare June 17, 2025 11:26
@sezero sezero requested a review from slouken August 1, 2025 15:46
@slouken
Copy link
Collaborator

slouken commented Aug 1, 2025

SDL_PIXELFORMAT_RGB24 is the slowest format to convert to. Does the libpng code actually do this?

@ankith26
Copy link
Contributor Author

ankith26 commented Aug 2, 2025

Yes, the libpng backend does convert to SDL_PIXELFORMAT_RGB24 for non-alpha cases.

@ankith26 ankith26 requested a review from slouken August 18, 2025 07:43
@slouken
Copy link
Collaborator

slouken commented Aug 25, 2025

I think this change would be okay in SDL3, but we shouldn't change SDL2 behavior at this point. Feel free to open a new PR against main if it's still relevant.

@slouken slouken closed this Aug 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants