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

Improved GIF optimize condition #6378

Merged
merged 5 commits into from Jun 26, 2022
Merged

Improved GIF optimize condition #6378

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
128ed18
Improve test in _get_optimize()
raygard Jun 19, 2022
f656711
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 19, 2022
7097444
Optimise palettes with more than 128 colors
radarhere Jun 19, 2022
847ad8c
Clarify check that palette is not already at its smallest
radarhere Jun 26, 2022
7c839d8
Merge pull request #3 from radarhere/fix_get_optimize
raygard Jun 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions Tests/test_file_gif.py
View file
Open in desktop
Expand Up @@ -180,6 +180,21 @@ def test_optimize_full_l():
assert im.mode == "L"


def test_optimize_if_palette_can_be_reduced_by_half():
with Image.open("Tests/images/test.colors.gif") as im:
# Reduce because original is too big for _get_optimize()
im = im.resize((591, 443))
imrgb = im.convert("RGB")
out = BytesIO()
imrgb.save(out, "GIF", optimize=False)
with Image.open(out) as reloaded:
assert len(reloaded.palette.palette) // 3 == 256
out = BytesIO()
imrgb.save(out, "GIF", optimize=True)
with Image.open(out) as reloaded:
assert len(reloaded.palette.palette) // 3 == 8


def test_roundtrip(tmp_path):
out = str(tmp_path / "temp.gif")
im = hopper()
Expand Down
10 changes: 9 additions & 1 deletion src/PIL/GifImagePlugin.py
View file
Open in desktop
Expand Up @@ -824,9 +824,17 @@ def _get_optimize(im, info):
if count:
used_palette_colors.append(i)

num_palette_colors = (
len(im.palette.palette) // 4
if im.palette.mode == "RGBA"
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 •
edited by radarhere

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.

if optimise or (
len(used_palette_colors) <= 128
and max(used_palette_colors) > len(used_palette_colors)
and max(used_palette_colors) >= len(used_palette_colors)
or len(used_palette_colors) <= num_palette_colors // 2
):
return used_palette_colors

Expand Down