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

AVIF support #115

Merged
merged 3 commits into from
Jul 10, 2023
Merged

AVIF support #115

merged 3 commits into from
Jul 10, 2023

Conversation

salty-ivy
Copy link
Member

@salty-ivy salty-ivy commented Jun 25, 2023

Fixed #111

Before

image

After

image

image
  • figure out how the pillow heic imports functionality
  • figure out how Wagtial uses Pillow HEIC for to see how to copy the approach
  • sketch out a design or update this issue with some ideas
  • write an implementation for using this in a fork of Wagtail
  • add some tests
  • show example of use
  • figure out how to get into this into Bakery Demo
  • figure out next steps

@salty-ivy
Copy link
Member Author

@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.

@salty-ivy salty-ivy marked this pull request as ready for review June 25, 2023 11:09
@Stormheg
Copy link
Member

Thanks @salty-ivy 👏

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

Copy link
Member

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

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

Looking really solid @salty-ivy! This is going to be such a big change for us 🥳 @zerolab could you review this as well as we discussed?

I gave it a try and it worked a treat. There are a few things to change in the code and as you noted documentation (README, reference.rst).

I think we should additionally add save_as_heic to the README – and mention for it and save_as_avif that they’re only available in Pillow for people who install pillow-heif.


Record of my trial for future ref, with walnuts.jpg from our bakerydemo.

source = open('tests/images/walnuts.jpg', 'rb')
out_avif = open('walnuts.avif', 'wb')
out_webp = open('walnuts.webp', 'wb')
source_img = Image.open(source)
source_img.save_as_webp(out_webp)
source_img.save_as_avif(out_avif)
out_webp.close()
out_avif.close()
out_avif_40 = open('walnuts-40.avif', 'wb')
out_webp_40 = open('walnuts-40.webp', 'wb')
source_img.save_as_avif(out_avif_40, quality=40)
out_avif_40.close()
source_img.save_as_webp(out_webp_40, quality=40)
out_webp_40.close()

File sizes (rounded) for ref:

192K	tests/images/walnuts.jpg
 40K	walnuts-40.avif
 60K	walnuts-40.webp
160K	walnuts.avif
128K	walnuts.webp

tests/images/tree-avif.avif Outdated Show resolved Hide resolved
willow/image.py Show resolved Hide resolved
willow/image.py Show resolved Hide resolved
tests/test_pillow.py Outdated Show resolved Hide resolved
tests/test_image.py Show resolved Hide resolved
willow/image.py Outdated
@@ -252,6 +252,7 @@ def __init__(self, f, dom=None):
self.dom = ElementTree.parse(f)
f.seek(0)
else:

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Copy link
Member Author

Choose a reason for hiding this comment

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

should we go for chroma=420 default way? then.

Comment on lines 219 to 223
def save_as_avif(self, f, quality=80, lossless=False):
if lossless:
self.image.save(f, 'AVIF', quality=-1, chroma=444)
else:
self.image.save(f, 'AVIF', quality=quality)
Copy link
Member

Choose a reason for hiding this comment

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

Might be one for later but I’m surprised we’d not make the chroma subsampling configurable 🤔 I don’t know whether the default is 4:2:2 or 4:2:0 but I could imagine scenarios where we’d want to change it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

note to self: https://pillow-heif.readthedocs.io/en/latest/options.html mentions chroma=444 is to be using lossless encoding.

there's no stopping us from doing

@Image.operation
def save_as_avif(self, f, quality=80, lossless=False, chroma: int = 420):
    if lossless:
        self.image.save(f, 'AVIF', quality=-1, chroma=444)
    else:
        self.image.save(f, 'AVIF', quality=quality, chroma=chroma)

at the same time, we will want to document some best practices on chroma and to be honest this is not something I fully understand (yet)

Found https://github.com/bigcat88/pillow_heif/blob/891d3fcef08eb03066f557ccef8fdc2492fd1c40/CHANGELOG.md?plain=1#L191 which mentions 420 (4:2:0) is the default

It would be good to document the quality kwarg, and what the different values mean (Possible values: None, -1, range(0-100). Set -1 for lossless quality or from 0 to 100, where 0 is lowest and 100 is highest.)

If we are to support pillow-avif-plugin as an alternative to pillow-heif, then this becomes more complicated - https://github.com/fdintino/pillow-avif-plugin/blob/master/src/pillow_avif/AvifImagePlugin.py#L130-L137. The latter plugin has a corresponding PR for Pillow - python-pillow/Pillow#5201

@salty-ivy
Copy link
Member Author

salty-ivy commented Jun 27, 2023

Also this considering this comment by @mrchrisadams wagtail/wagtail#10486 (comment)

should we try to save them as lossless=True ?

@zerolab
Copy link
Collaborator

zerolab commented Jun 27, 2023

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

.gitignore Outdated Show resolved Hide resolved
@zerolab
Copy link
Collaborator

zerolab commented Jun 28, 2023

In addition, it would be good to add a note about lossless AVIF in docs/guide/save.rst (similar to

Lossless WebP
-----------------
You can encode the image to WebP without any loss by setting the
``lossless`` keyword argument to ``True``:
.. code-block:: python
with open('lossless.webp', 'wb') as f:
i.save_as_webp(f, lossless=True)
)

@salty-ivy
Copy link
Member Author

salty-ivy commented Jun 28, 2023

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

@zerolab
Copy link
Collaborator

zerolab commented Jun 28, 2023

@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
Copy link
Member Author

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
Copy link
Member Author

@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

setup.py Outdated Show resolved Hide resolved
@salty-ivy salty-ivy requested a review from zerolab June 29, 2023 15:28
setup.py Outdated Show resolved Hide resolved
tests/test_pillow.py Outdated Show resolved Hide resolved
@Image.operation
def save_as_avif(self, f, quality=80, lossless=False):
if lossless:
self.image.save(f, "AVIF", quality=-1, chroma=444)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: since we're basing this on pillow-heif, it would be great to document what these magic values mean

https://github.com/bigcat88/pillow_heif/blob/891d3fcef08eb03066f557ccef8fdc2492fd1c40/pillow_heif/options.py#L21-L26

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the lossless save test for Avif is failing for some reason need to look at that as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

This is so close, needs a bit more documentation and I think we're good

@salty-ivy
Copy link
Member Author

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

@zerolab
Copy link
Collaborator

zerolab commented Jun 30, 2023

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

tests/test_image.py Outdated Show resolved Hide resolved
tests/test_pillow.py Outdated Show resolved Hide resolved
@salty-ivy salty-ivy requested a review from zerolab July 1, 2023 11:08
@salty-ivy
Copy link
Member Author

salty-ivy commented Jul 1, 2023

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?

Copy link
Collaborator

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

Added further suggestions.
I am not sure why the webp test fails locally yet, but will check and get to you later

docs/guide/save.rst Outdated Show resolved Hide resolved
-----------------

You can encode the image to WebP without any loss by setting the
You can encode the image to Avif, Heic (only pillow) and WebP without any loss by setting the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
You can encode the image to Avif, Heic (only pillow) and WebP without any loss by setting the
You can encode the image to AVIF, HEIC (Pillow-only) and WebP without any loss by setting the

note that ImageMagick (thus Wand) supports HEIC via libheif since version 7.1.0. Opened #119

Copy link
Member Author

Choose a reason for hiding this comment

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

we can change the docs further in that PR, would you mind if I take up that issue as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Happy for you to take it on, once this and the Wagtail continuation are done

docs/reference.rst Outdated Show resolved Hide resolved
tests/test_pillow.py Outdated Show resolved Hide resolved
tests/test_pillow.py Outdated Show resolved Hide resolved
tests/test_pillow.py Outdated Show resolved Hide resolved
tests/test_wand.py Outdated Show resolved Hide resolved
tests/test_wand.py Outdated Show resolved Hide resolved
tests/test_wand.py Outdated Show resolved Hide resolved
willow/plugins/wand.py Outdated Show resolved Hide resolved
Copy link
Contributor

@mrchrisadams mrchrisadams left a comment

Choose a reason for hiding this comment

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

Hi @salty-ivy as discussed, I've had a quick look over this - I only saw a few small typos, but I had one qn - if one of the benefits of the AVIF support is that the files are smaller, do you think it might be worth having a test to demontrate this?

I know it's venturing into testing the properties of format rather than the library implementing it, but it may be useufl to demonstrating the benefits it's supposed to bring,.

tests/test_wand.py Outdated Show resolved Hide resolved
docs/reference.rst Outdated Show resolved Hide resolved
docs/guide/save.rst Outdated Show resolved Hide resolved
@salty-ivy
Copy link
Member Author

salty-ivy commented Jul 3, 2023

@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
Copy link
Contributor

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!

@salty-ivy salty-ivy requested a review from zerolab July 3, 2023 16:11
tests/test_wand.py Outdated Show resolved Hide resolved
willow/plugins/wand.py Outdated Show resolved Hide resolved
tests/test_wand.py Outdated Show resolved Hide resolved
@salty-ivy salty-ivy requested a review from zerolab July 4, 2023 16:02
@zerolab
Copy link
Collaborator

zerolab commented Jul 10, 2023

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

@zerolab zerolab merged commit e076c34 into wagtail:main Jul 10, 2023
6 checks passed
@Stormheg
Copy link
Member

Yay! Thanks all for your work! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🎉 Done
Status: Done
Development

Successfully merging this pull request may close these issues.

AVIF support
5 participants