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

Support for animated formats (gif/webp/apng) #628

Merged
merged 11 commits into from
Sep 11, 2024

Conversation

benzkji
Copy link
Contributor

@benzkji benzkji commented Feb 5, 2024

A first draft. Test still passing, no new tests for the moment.

I don't think that a wrapper (processors leaving as is, abstract things away for processors) would be a good solution, as some processors could still not operate like this. For example, the autocrop would autocrop each frame individually, and if a processor creates a new Image(), and pastes the original into it, frames would still be lost - as the background processor does it.

With my proposal, animated GIF support would need to be supported by each processor individually. Most of the times, this would mean a simple usage of the new helper function instead of directly calling im.whatever().

Using

THUMBNAIL_IMAGE_SAVE_OPTIONS = {
    "GIF": {"save_all": True}, 
}
THUMBNAIL_PRESERVE_EXTENSIONS = ("gif", )

And animated GIFs work (on my machine), hopefully animated WEBP would as well. A first feedback for the approach is very welcome.

@benzkji
Copy link
Contributor Author

benzkji commented Feb 19, 2024

I've had to add a BytesIO use, did not work otherwise. I've tried many things to treat the file in place, but it's not working till now...even asked on SO: https://stackoverflow.com/questions/78020254/python-pillow-treat-an-animated-gif-or-webp-in-place

Feedback welcome, I'm not so sure about not leaving open file pointers around?

@benzkji
Copy link
Contributor Author

benzkji commented Feb 19, 2024

...also, will remove _call_pil_method soon.

@benzkji
Copy link
Contributor Author

benzkji commented Feb 19, 2024

It has tests now, for all builtin processors. Though, it only tests if frames are preserved, and some of the basic functionalities (as resize/crop) - I'm afraid I don't fully understand all of the processors, and especially not all of the edge cases involved.

If the used BytesIO workaround is ok for @SmileyChris I think it could be merged?

@benzkji
Copy link
Contributor Author

benzkji commented Feb 19, 2024

...remaining to add: docs. Also, I would add save_all=True for all images with more than one frame, it would be better to support them without having to set/use THUMBNAIL_IMAGE_SAVE_OPTIONS.

@benzkji
Copy link
Contributor Author

benzkji commented Feb 20, 2024

...also, docs: how to make them work again?

@benzkji benzkji requested a review from SmileyChris February 20, 2024 11:54
@benzkji benzkji changed the title anigif (and probably webp) support WIP Support for animated formats (gif/webp/apng) Feb 27, 2024
@benzkji
Copy link
Contributor Author

benzkji commented Mar 8, 2024

@SmileyChris feedback welcome :) or maybe shall I reach out to @jrief ?

@jrief
Copy link
Collaborator

jrief commented Sep 9, 2024

@benzkji Hi Ben, you're changing code and code-styling in the same pull request. This makes a review extremly difficult to understand. Can this be split into separate PRs?

And btw., I prefer single quotes for strings which are not read by humans, but this is my personal opinion.

@benzkji
Copy link
Contributor Author

benzkji commented Sep 9, 2024

@jrief Hi Jacob. Sorry, that slipped in, I probably had black active on save. Will try to revert these changes.

@benzkji
Copy link
Contributor Author

benzkji commented Sep 9, 2024

things remaining (code style is reverted):

  • wether to enable "save_all" by default if multiple frames are detected?
  • write docs
  • test in real live (done that a few months before with django-filer, but it did not always work...), with webp, etc.

@benzkji
Copy link
Contributor Author

benzkji commented Sep 9, 2024

real world feedback with django-filer:

  • animated png/gif/webp all work, when preserving their extensions and setting save options
  • some animated gifs could not be saved as WEBP, because of a problem during saving the image in PIL.
  • gif/webp>png is not preserving the animation
  • but in general, gif>web and png>webp was working
  • bigger images could block your server

@benzkji
Copy link
Contributor Author

benzkji commented Sep 9, 2024

Thinking about it, it would be nice if one could define if animations should be preserved, when using easy-thumbnails...not an "always on/off" solution, but rather some kind of crop=1 animated=1/0 ?! @jrief next step?!

@benzkji
Copy link
Contributor Author

benzkji commented Sep 9, 2024

if anyone interested:

PIL/WebPImagePlugin.py, line 199, in _save_all > not enough values to unpack (expected 3, got 0) is the error, when trying a special GIF (probably because of a reduced palette) to .webp.

@benzkji
Copy link
Contributor Author

benzkji commented Sep 9, 2024

but, without "save_all" enabled for everyone automatically, I guess this could be released as a beta, if no issues arise. People could check manually, with preserve extensions and save_all, if it works. hopefully no existing features would be broken, aka we have a testsuite - I don't know how extensive it is?!

Copy link
Collaborator

@jrief jrief left a comment

Choose a reason for hiding this comment

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

Thanks for this improvement.

Would be great if you could add a few lines of documentation and mention this feature in the ChangeLog.

@@ -108,7 +149,8 @@ def autocrop(im, autocrop=False, **kwargs):
bg = Image.new('L', im.size, 255)
bbox = ImageChops.difference(bw, bg).getbbox()
if bbox:
im = im.crop(bbox)
# im = im.crop(bbox)
Copy link
Collaborator

Choose a reason for hiding this comment

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

please remove commented code

@@ -274,7 +318,9 @@ def scale_and_crop(im, size, crop=False, upscale=False, zoom=None, target=None,
diff_y = diff_y - add - remove
box = (left, top, right, bottom)
# Finally, crop the image!
im = im.crop(box)
# im = im.crop(box)
Copy link
Collaborator

Choose a reason for hiding this comment

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

detto

easy_thumbnails/processors.py Outdated Show resolved Hide resolved
@benzkji
Copy link
Contributor Author

benzkji commented Sep 9, 2024

@jrief done so (docs & changelog). BTW, do you know what must be done to get readthedocs up and running again? https://easy-thumbnails.readthedocs.io/en/latest/ is for 2.7, no word of SVG et al?

@benzkji
Copy link
Contributor Author

benzkji commented Sep 9, 2024

ah, shall I merge master into my branch? My current local tests are passing (without python3.7 ;), but that's not up to date at all.

@benzkji
Copy link
Contributor Author

benzkji commented Sep 9, 2024

thx. will check the testsuite.

@jrief
Copy link
Collaborator

jrief commented Sep 9, 2024

@benzkji I assume that the local behavior of colorspace is different from what the tests on GitHub-Actions tell us. Any idea what could be the problem?

@benzkji
Copy link
Contributor Author

benzkji commented Sep 9, 2024

not yet. After the merge it fails localy on my machine as well.

@benzkji
Copy link
Contributor Author

benzkji commented Sep 9, 2024

test pass now. it is strange, though, it seems PIL eliminates empty frames. but also, my test PNG and WEBPs seem empty (when saving them out), but when using filer it works, and in tests it works as well, now. Looks like a "writing the tests is the hardest part" problem. I must say that I'm not really familiar with creating images from within PIL, so this could be the issue.

@jrief jrief merged commit acb666f into SmileyChris:master Sep 11, 2024
5 checks passed
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