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

Fixed GIF remapping to palette with duplicate entries #6548

Merged
merged 2 commits into from Sep 21, 2022

Conversation

radarhere
Copy link
Member

Addresses #6389

remap_palette cannot map multiple palette indexes to the same output index, by its very definition. It accepts a list of palette indexes that it transforms to the position in the list. There cannot be more than one palette index at a given position in the list.

If a palette is supplied when saving a GIF image, im.save("out.gif", palette=...), and that palette has duplicate entries, then the following GifImagePlugin code will try to prepare a list that maps the first of the duplicated entries to multiple locations, leaving the rest of the duplicate entries to be not strictly defined in the behaviour of remap_palette.

used_palette_colors = []
for i in range(0, len(source_palette), 3):
source_color = tuple(source_palette[i : i + 3])
try:
index = im.palette.colors[source_color]
except KeyError:
index = None
used_palette_colors.append(index)

This PR fixes that, by leaving the duplicated indexes as they are - setting index to None, the next loop in GifImagePlugin

for i, index in enumerate(used_palette_colors):
if index is None:
for j in range(len(used_palette_colors)):
if j not in used_palette_colors:
used_palette_colors[i] = j
break

will tell remap_palette to map the index to itself, leaving it untouched.

Comment on lines 523 to 527
index = im.palette.colors[source_color]
except KeyError:
index = None
if index in used_palette_colors:
index = None
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the if could be inside the try, since if there's an exception it will have already been set to None.

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually have a different idea of how to simplify the code. In my latest commit, I replace the try block with get().

@hugovk hugovk merged commit 920bcec into python-pillow:main Sep 21, 2022
@radarhere radarhere deleted the gif_palette branch September 21, 2022 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants