Add thumbnailing support for animated GIFs #662
Conversation
| # https://pillow.readthedocs.io/en/stable/handbook/image-file-formats.html#gif | ||
| if to_return.format == "GIF" and getattr(to_return, "is_animated", False): | ||
| for i in range(to_return.n_frames): | ||
| to_return.seek(i) |
There was a problem hiding this comment.
I've struggled with Pillow APIs from the beginning, but reading the docs suggests that visiting these frames with seek should fix the mode, ist that correct?
There was a problem hiding this comment.
That's correct. Seeking will fix the mode, but the loading behaviour can be modified by global Pillow setting.
I think we could also check GifImagePlugin.LOADING_STRATEGY and skip this second seek loop if it's set correctly. But I'd need to test this. e.g.:
if (
to_return.format == "GIF" and
to_return.is_animated and
GifImagePlugin.LOADING_STRATEGY != GifImagePlugin.LoadingStrategy.RGB_ALWAYS
):
# ...Users would just need to set this to avoid the extra seek loop.
from PIL import GifImagePlugin
GifImagePlugin.LOADING_STRATEGY = GifImagePlugin.LoadingStrategy.RGB_ALWAYSThere was a problem hiding this comment.
Is seeking a big overhead? If it generates too much complexity for a not so big performance gain, I would strive for "settings to disable animation support", as processing all these frames before will be a real performance hit already? But that would be another PR, I guess.
There was a problem hiding this comment.
As I understand it, seeking through the frames is basically converting the index colors (e.g. palette colors) to the RGB(A) image data in memory.
This is just an edge case for animated GIFs due to how Pillow GifImagePlugin works. For animated GIFs, a FrameAware operation means the frames are read, operation applied and read again (to correct the mode). My view: It's probably not a big deal performance-wise, but if there's a way to reduce compute with an opt-in setting, that's great. I can image an application that works with animated GIFs a lot would want to try to be as efficient as possible and take this opt-in. (My guess is LoadingStrategy.RGB_ALWAYS uses more memory in some cases which is why it's not the default - this really is a decision for application developers to take.)
I made an update to check the loading strategy as well in the if condition. It's also fine for me without that check. I think you have a better understanding of how the animation support fits together, so just let me know what you think is best.
There was a problem hiding this comment.
Agreed. I would keep your solution. This probably needs (very little) documentation?
There was a problem hiding this comment.
Ok cool. I'll add some documentation either later today, or tomorrow morning.
| # Should not fail because of wrong mode and should still be animated. | ||
| # https://github.com/SmileyChris/easy-thumbnails/issues/653 | ||
| thumbnail = t.get_thumbnail({'size': (500, 50), 'crop': True}) | ||
| self.assertTrue(getattr(thumbnail.image, "is_animated", False)) |
There was a problem hiding this comment.
shouldn't it remain "animated"?
There was a problem hiding this comment.
is_animated is True. I used getattr with a fallback to False because is_animated isn't always set.
I'll change it to using is_animated directly for clarity. i.e:
self.assertTrue(thumbnail.image.is_animated)There was a problem hiding this comment.
haha, yes. sorry, too early :)
There was a problem hiding this comment.
No worries... I usually need a strong coffee in the morning before I'm ready to go. 😄
I'll also change the code to use to_return.is_animated since GIFs will always have this property.
|
thx for the additional deep digging! |
|
Just a small ping to ask if this PR could be merged? (and eventually publish a release including this PR) Due to this bug, we have to stick to easy-thumbnails 2.9.0. However, this version cannot work with Python 3.13 (due to dependency on imghdr) and thus prevents to upgrade to Debian 13... |
benkonrath
force-pushed
the
animated-gif-p-mode
branch
from
cf73129 to
8983890
Compare
|
I just rebased this branch with master and the now tests fail. The failure happens when installing the svg dependencies so it's unrelated to the changes in this PR. |
|
everybody is very busy, but sometimes pinging @SmileyChris or @jrief may help? |
|
I didn't mean to complain about the slowdown in maintenance on this project. I definitely understand that people's priorities change, and that's just how things go. I just meant that the CI failure is likely unrelated to the changes in this PR. I posted the note more as a notice to anyone using this fix. I'm not planning to investigate CI failure because I feel that even if I did spend time fixing it, it would be just another ignored PR. |
|
I did not take it as a complain either. I just feel a bit responsible, as I was the one who broke things, and a few people are waiting for a release, so I thought maybe I could help to speed things up. |
3 participants
This fixes the bug in #653 and also ensures animated GIFs are thumbnailed properly instead of thumbnailing only the first frame like the solution in #657.
Reading through the Pillow GIF docs, I think we could also set the GIF loading strategy to
RGB_ALWAYS. i.e.https://pillow.readthedocs.io/en/stable/handbook/image-file-formats.html#gif
I don't thinks this is a good option because add this Pillow configuration change would apply to the whole Django application, and we don't know if other parts of a user's application would be affected by this.
@benzkji If you have time, it would be great to get some feedback on this PR. Thanks.