Skip to content

cgifsave: reuse global palette, if possible #2797

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

Merged
merged 14 commits into from
May 23, 2022

Conversation

dloebl
Copy link
Contributor

@dloebl dloebl commented May 10, 2022

For reference: 2576#comment
Reuse the global palette in gifsave if it has been attached during gifload.
This should both improve the file size and processing time for many GIF -> GIF operations.

@jcupitt Notes / Questions:

  1. Quantizing the attached global palette to get a VipsQuantiseResult feels a bit hacky. Maybe there is a better way to do it?
  2. As we probably don't want to enable this new behaviour for any kind of GIF -> GIF operation, should we add a parameter to gifsave to enable/disable global palette reuse?
  3. Dithering seems to cause colour artifacts with certain GIFs:
    $ vipsthumbnail --size 200x phone.gif[n=-1] -o out.gif

Results:
GIF used: earth.gif

Before:
$ time vipsthumbnail --size 200x earth.gif[n=-1] -o old.gif
518ms total

After:
$ time vipsthumbnail --size 200x earth.gif[n=-1] -o new.gif
219ms total

$ du -h old.gif new.gif
580K old.gif
456K new.gif

jcupitt and others added 2 commits May 9, 2022 15:46
@jcupitt
Copy link
Member

jcupitt commented May 11, 2022

Oh very cool, I hadn't thought of using the palette as an image source. @DarthSim, do you have an opinion on the best way to initialise the quantisation result from an existing palette?

On new switches to control behaviour:

  1. We could add an argument to not use the global palette from the metadata (perhaps reoptimise?), or alternatively users could delete gif-palette before saving. I suppose having a switch would be more convenient.
  2. We could add a switch to enable dynamic palette generation (perhaps local_palette?). Or I suppose we could just delete all that code and only support a single global palette.

@jcupitt jcupitt mentioned this pull request May 11, 2022
13 tasks
@jcupitt jcupitt added this to the 8.13 milestone May 11, 2022
@DarthSim
Copy link
Contributor

I hadn't thought of using the palette as an image source. @DarthSim, do you have an opinion on the best way to initialise the quantisation result from an existing palette?

That's exactly want I'd do :)

@jcupitt
Copy link
Member

jcupitt commented May 12, 2022

I think we should reuse by default. The most common case must be GIF -> GIF resize, and that should reuse with no extra parameters.

How about something like reoptimise (or maybe optimise_palette?) to generate a new global palette.

@dloebl
Copy link
Contributor Author

dloebl commented May 12, 2022

@jcupitt Good point. I'll introduce a reoptimise parameter.

generate a new global palette

For now, I would prefer to keep the current method of determining palettes (in case no palette has been attached or reoptimise == TRUE), because that keeps the possibility to output chunks of GIF-data to the target (streaming) in gifsave. Maybe quantizing to a single global palette is something for a different PR?

@jcupitt
Copy link
Member

jcupitt commented May 12, 2022

Maybe quantizing to a single global palette is something for a different PR?

OK, sounds reasonable.

@dloebl
Copy link
Contributor Author

dloebl commented May 13, 2022

Ready for review from my side.
One additional improvement we could do, is to attach local palettes on gifload as well. This should both improve file sizes and processing time for the affected GIFs again. @jcupitt Do you think that would make sense for a follow-up PR?

Additional notes:
I noticed artifacts with certain GIFs depending on the quantizr / libimagequant version:
original image
Command: $ vipsthumbnail --size 200x input.gif[n=-1] -o out.gif

quantizr v1.2.0 imagequant v2.17 imagequant v4.0
quant 217 400

The issue is gone with the latest libimagequant version (v4.0), but I just wanted to mention this.

@dloebl dloebl marked this pull request as ready for review May 13, 2022 13:03
@jcupitt
Copy link
Member

jcupitt commented May 15, 2022

Hi again, could you explain the motivation behind the use_lct feature? Does this help with a significant percentage of images? It looks like a rather niche thing at first glance.

@dloebl
Copy link
Contributor Author

dloebl commented May 16, 2022

While testing this change with a dataset of GIFs, I noticed that about 25% of the animated GIFs we process have at least one local colour table (might be because some of the GIFs come from libvips originally).
Therefore I would like to extend this improvement to GIFs with local colour tables. But I guess that's something for a follow-up PR - I'll remove the use_lct feature for now :-)

Copy link
Member

@jcupitt jcupitt left a comment

Choose a reason for hiding this comment

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

Just two tiny comments.

@jcupitt
Copy link
Member

jcupitt commented May 21, 2022

25% is higher than I expected! Sure, let's do that in another PR.

@dloebl
Copy link
Contributor Author

dloebl commented May 23, 2022

Sounds good 👍
All done. I also updated the documentation/changelog.

@jcupitt jcupitt merged commit f26f0b0 into libvips:master May 23, 2022
@jcupitt
Copy link
Member

jcupitt commented May 23, 2022

Great! It seems to work well, this is a nice improvement. Good job!

@jcupitt
Copy link
Member

jcupitt commented May 23, 2022

With reoptimise (more or less the old behaviour) I see:

$ time vips copy 3198.gif[n=-1] x.gif[reoptimise]
real	0m1.076s
user	0m1.386s
sys	0m0.013s
$ ls -l x.gif
-rw-r--r-- 1 john john 4153849 May 23 13:05 x.gif

And with this patch:

$ time vips copy 3198.gif[n=-1] x.gif
real	0m0.886s
user	0m1.206s
sys	0m0.039s
$ ls -l x.gif
-rw-r--r-- 1 john john 4041446 May 23 13:05 x.gif

So in many cases: smaller GIFs, lower CPU load, same memory use.

@dloebl dloebl deleted the gifsave-use-attached-palette branch May 23, 2022 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants