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.

Sign up for GitHub

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

Jump to bottom

Set C palette to be empty by default #7289

Merged
merged 3 commits into from Oct 5, 2023
Merged

Set C palette to be empty by default #7289

merged 3 commits into from Oct 5, 2023

Conversation

radarhere
Copy link
Member

Resolves #7288

The image in the issue has indexes that occur outside of the palette. This PR ensures that they are black.

@radarhere radarhere mentioned this pull request Jul 16, 2023
Closed
@Yay295
Copy link
Contributor

Yay295 commented Jul 17, 2023

This seems inefficient. ImagingPaletteNew already uses calloc to get its data, but then initializes it. It would be faster to just not initialize it in the first place. ImagingPaletteNew isn't used in very many places, so I think the cleaner solution would be to change ImagingPaletteNew to not initialize its values, and create a new function ImagingPaletteNewRamp that first calls ImagingPaletteNew and then initializes the data as ImagingPaletteNew does currently. There's another already existing function ImagingPaletteNewBrowser that does essentially that, but with different starting values.

https://github.com/python-pillow/Pillow/blob/b9451540973ca7915b0ce68e6bbacbed665caec2/src/libImaging/Palette.c#L23C1-L95

@radarhere
Copy link
Member Author

Ok, I've pushed a new commit to set the palette to be empty by default.

This is now a C sequel to 4d36fee

@radarhere radarhere changed the title Set undefined palette values to black Set C palette to be empty by default Jul 17, 2023
Yay295
Yay295 reviewed Jul 17, 2023
View reviewed changes
src/libImaging/Palette.c Outdated
@@ -40,10 +40,8 @@ ImagingPaletteNew(const char *mode) {
palette->mode[IMAGING_MODE_LENGTH - 1] = 0;

/* Initialize to ramp */
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is now outdated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I've pushed a commit to remove it.

@hugovk hugovk merged commit 5f04b3d into python-pillow:main Oct 5, 2023
55 checks passed
@radarhere radarhere deleted the undefined_palette branch October 5, 2023 21:26
This was referenced Nov 15, 2023
Closed
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yay295 Yay295 Yay295 left review comments

Assignees
No one assigned
Labels
Palette
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrect transparency after converting indexed image to RGB
3 participants
@radarhere @Yay295 @hugovk