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

Improved GIF optimize condition #6378

Merged
merged 5 commits into from Jun 26, 2022
Merged

Conversation

raygard
Copy link
Contributor

@raygard raygard commented Jun 19, 2022

Palette can be optimized if number of colors can be reduced by half or more.

Fixes #6362 .

Changes proposed in this pull request:

  • Fix test for palette "hole" by changing comparison max(used_palette_colors) > len(used_palette_colors) to use >=
  • Also allow optimize if number of used colors is not more than half the number of current colors
  • Added a test for the latter that fails with current _get_optimize() but passes with this change.

I note that this change should obviate the condition len(used_palette_colors) <= 128 but some tests fail if I remove it. I would rather leave it than try to update those tests in test_file_gif.py.
As noted in #6362, this assumes that im.palette.palette exists, is not None and is actually a byte string or array of RGB or RGBA colors. If this is possibly not the case please let me know!

raygard and others added 2 commits June 18, 2022 18:07
Palette can be optimized if number of colors can be reduced by half or more.
else len(im.palette.palette) // 3
)
# Round up to power of 2 but at least 4
num_palette_colors = max(4, 1 << (num_palette_colors - 1).bit_length())
Copy link
Member

Choose a reason for hiding this comment

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

Why at least 4?

Copy link
Contributor Author

@raygard raygard Jun 19, 2022

Choose a reason for hiding this comment

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

Perhaps I didn't think it through carefully enough. The idea is that this condition would trigger only when the number of used colors is low enough to reduce the GIF color table. For example, if the palette now has 30 colors, the GIF would have 32 without optimization, so if the used colors are 16 or fewer, then it is worth remapping down. So I was thinking that the right side of the len(used_palette_colors) <= num_palette_colors // 2 condition should never be less than 2 because that's the smallest size GIF color table.

Now I see that this could mean attempting to optimize if the actual palette is 1 or 2 colors and and only one is used (an odd case but not impossible), which is probably silly. Maybe it should instead have a test to prevent optimization if there are fewer than 3 colors in the palette? Reducing a 4-color palette to 2 is the minimum reasonable case for optimization. Maybe replace

# Round up to power of 2 but at least 4
num_palette_colors = max(4, 1 << (num_palette_colors - 1).bit_length())

with something like

# Round up to power of 2 (0 -> 2 but that's OK)
num_palette_colors = 1 << (num_palette_colors - 1).bit_length()
# Suppress if less than 4
num_palette_colors = 0 if num_palette_colors < 4 else num_palette_colors

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I've added another commit to https://github.com/raygard/Pillow/pull/3 with my suggestion. See what you think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks good! I merged into my own repo and the changes apparently flow automatically into this PR. I'm still learning how github works...
Thank you for helping me get this working right and clean. I would not have been able to fix the tests myself.
Next up: I have mods about ready (I hope) to finally allow the "LZW minimum code size" (part 22 of the GIF spec) to be less than 8 in files written by Pillow.

@radarhere radarhere changed the title Improve test in _get_optimize() Improve GIF optimize condition Jun 19, 2022
@radarhere radarhere changed the title Improve GIF optimize condition Improved GIF optimize condition Jun 19, 2022
@radarhere radarhere merged commit fc497ff into python-pillow:main Jun 26, 2022
@raygard raygard deleted the fix_get_optimize branch July 13, 2022 21:23
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.

GIF: _get_optimize() missed opportunities
2 participants