Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue with converting surfaces to 8 bit surfaces with palettes #2031

Merged
merged 6 commits into from
Jun 5, 2023

Conversation

Temmie3754
Copy link
Member

This resolves issue pygame/pygame#2477 by checking if the new surface needs a palette and creating the default one for it if it does.

@Temmie3754 Temmie3754 requested a review from a team as a code owner March 17, 2023 14:08
@yunline yunline added Surface pygame.Surface bugfix PR that fixes bug labels Mar 17, 2023
src_c/surface.c Outdated Show resolved Hide resolved
src_c/surface.c Outdated Show resolved Hide resolved
Copy link
Contributor

@yunline yunline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src_c/surface.c Outdated Show resolved Hide resolved
Copy link
Member

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there are chance you are leaking memory here for the all white palette? The SDL functions for setting the surface palette are careful to destroy/free (the functions have recently been renamed) any existing palettes before assigning a new one.

It looks like here you might potentially be just replacing the all white palette with the default palette leaving a dangling allocation for the old white palette.

On a wider scale it might be cool if this conversion function actually generated an appropriate 8 bit palette for the image being converted using something like the Median cut algorithm. See here for one example. This would be beyond the scope of this PR though.

@Temmie3754
Copy link
Member Author

Thanks for catching that, it does indeed look as if there is a memory leak, I'll work on fixing that.

src_c/surface.c Outdated Show resolved Hide resolved
src_c/surface.c Outdated Show resolved Hide resolved
@Temmie3754 Temmie3754 linked an issue Apr 25, 2023 that may be closed by this pull request
@Starbuck5
Copy link
Member

This should get a unit test to make sure the issue doesn't regress in the future.

@Temmie3754
Copy link
Member Author

Added a unit test

Copy link
Member

@Starbuck5 Starbuck5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Starbuck5 Starbuck5 merged commit e757512 into pygame-community:main Jun 5, 2023
29 checks passed
@Starbuck5 Starbuck5 added this to the 2.3 milestone Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix PR that fixes bug Surface pygame.Surface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blank palette for surfaces converted to 8-bit in Pygame 2 (2477)
4 participants