wagtail / Willow

A wrapper that combines the functionality of multiple Python image libraries into one API
https://willow.wagtail.org/
BSD 3-Clause "New" or "Revised" License
273 stars 53 forks source link

AVIF support #115

Closed salty-ivy closed 1 year ago

salty-ivy commented 1 year ago

Fixed https://github.com/wagtail/Willow/issues/111

Before

image

After

image

image
salty-ivy commented 1 year ago

@thibaudcolas I have opened a draft, only tests are left although I am not sure about quality and lossless in function save_as_avif I do understand these properties, just no sure about what values should be passed in this case.

Stormheg commented 1 year ago

Thanks @salty-ivy 👏

It makes me very happy to see AVIF support added to Wagtail.

salty-ivy commented 1 year ago

Also this considering this comment by @mrchrisadams https://github.com/wagtail/wagtail/issues/10486#issuecomment-1606686458

should we try to save them as lossless=True ?

zerolab commented 1 year ago

should we try to save them as lossless=True ?

We don't do this for any other types, so.. no. Only for the original rendition in Wagtail

zerolab commented 1 year ago

In addition, it would be good to add a note about lossless AVIF in docs/guide/save.rst (similar to https://github.com/wagtail/Willow/blob/4eac48ad0e40d9a53a421397c7833c2642348388/docs/guide/save.rst?plain=1#L49-L58 )

salty-ivy commented 1 year ago

@zerolab apologies if the question is too obvious but are we supporting avif with lossless=True ?

zerolab commented 1 year ago

@zerolab apologies if the question is too obvious but are we supporting avif with lossless=True ?

That is what https://github.com/wagtail/Willow/pull/115/files#diff-a804be079e5771c0e82b11e493ad11d319d34d042b33871a729f5eb151ace339R228-R229 does, no?

salty-ivy commented 1 year ago

yup, I thought by default that Is default value.

@zerolab apologies if the question is too obvious but are we supporting avif with lossless=True ?

That is what https://github.com/wagtail/Willow/pull/115/files#diff-a804be079e5771c0e82b11e493ad11d319d34d042b33871a729f5eb151ace339R228-R229 does, no?

Yes I thought of it as default( default argument to True ) or not but I guess I understand now! as we wouldn't do it by default that is the right way and how other save_as have been configured.

salty-ivy commented 1 year ago

@zerolab @thibaudcolas Could you guys confirm or have any idea. Looks like there's some issue while running runtests.py

Traceback (most recent call last):
  File "/Users/apple/Desktop/open-source/wagtail/Willow/runtests.py", line 11, in <module>
    from tests.test_wand import *  # noqa: F403
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/apple/Desktop/open-source/wagtail/Willow/tests/test_wand.py", line 16, in <module>
    no_webp_support = not WandImage.is_format_supported("WEBP")
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/apple/Desktop/open-source/wagtail/Willow/willow/plugins/wand.py", line 64, in is_format_supported
    return bool(_wand_version().formats(image_format))
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/apple/Desktop/open-source/wagtail/willow-dev/lib/python3.11/site-packages/wand/version.py", line 264, in formats
    library.MagickWandGenesis()
    ^^^^^^^
NameError: name 'library' is not defined

seems to be originating from wand from this line https://github.com/emcconville/wand/blob/11e37fc9d7f9e3375f34e381b3a73cbbdc8517ec/wand/version.py#L263

salty-ivy commented 1 year ago

@zerolabn both heic and avif seems to fail at saving the file as completely lossless I.e lossless=True

zerolab commented 1 year ago

@salty-ivy will have a look later today and advise on next steps

salty-ivy commented 1 year ago

Few finale questions....

  1. Should we have mention WebP, AVIF and HEIC all at once in LOSSLESS section in save.rst.
  2. Should I also include lossless test for AVIF in test_wand.py as well?
  3. Why does lossless test for webP in test_wand.py fails in local testing?
salty-ivy commented 1 year ago

@mrchrisadams That's a great idea! How about having a test where where we open one format and check the size and then save as Avif and assert the new size to be smaller ? Or something like that if I have understood it correctly?

mrchrisadams commented 1 year ago

hi @salty-ivy - yes, that was what I was thinking. I was a bit wary that it might go outside the scope of adding support, so I would defer to the maintainers.

Hope that helps!

zerolab commented 1 year ago

A few notes on avif/webp conversion using ImageMagick:

convert tests/images/transparent.png -define heic:lossless=true tests/images/lossless.avif
magick compare tests/images/transparent.png tests/images/lossless.avif -compose Src tests/images/compare-avif.png

convert tests/images/tree.avif -define heic:lossless=true tests/images/lossless.avif
magick compare tests/images/tree.avif tests/images/lossless.avif -compose Src tests/images/compare.avif

and

convert tests/images/transparent.png -define webp:lossless=true tests/images/lossless.webp
# or
convert tests/images/transparent.png -define webp:lossless=true -define webp:exact=true tests/images/lossless.webp

magick compare tests/images/transparent.png tests/images/lossless.webp -compose Src tests/images/compare-webp.png

convert tests/images/tree.avif -define heic:lossless=true tests/images/lossless.avif
magick compare tests/images/tree.avif tests/images/lossless.avif -compose Src tests/images/compare.avif

will show some artifacts, depending on the ImageMagick version. Which explain the test failures.

Anyhow, this now looks good. Thank you for all your work, @salty-ivy! And everyone for your inputs

Stormheg commented 1 year ago

Yay! Thanks all for your work! ❤️