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

Add AVIF plugin (decoder + encoder using libavif) #5201

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

fdintino
Copy link

@fdintino fdintino commented Jan 11, 2021

Resolves #7983

This adds support for AVIF encoding and decoding, including AVIF image sequences.

I've added tests, and integrated libavif into the windows, linux, and mac CI builds. I haven't done anything to integrate with the docker-images repo.

I chose libavif rather than libheif because the former has been embraced by AOMedia and it's what Chromium uses. Packaging support is spotty at the moment, but I expect that to change soon (currently it's in Debian testing, Fedora rawhide, Ubuntu hirsute, and Alpine edge).

A few notes on the implementation here:

  • The plugin currently only supports encoding 8-bit images, and all images are decoded to 8-bit RGB(A). I wasn't totally clear on how best to deal with higher bit depths (somewhat related issue: Add support for high bit depth multichannel images #1888)
  • The RGB to YUV conversion isn't exposed to python at all. Chroma subsampling and the presence of an alpha channel make it non-trivial to return decoded images as YCbCr. It would not be difficult to permit encoding from a YCbCr source.
  • Since there isn't a way to pass parameters to Image.open (Parameters for Image.open() #569), I'm using module globals in AvifImagePlugin.py to make decoder codec choice and chroma upsampling configurable. I suspect there's a better way to do this.

The star.avifs test file is licensed as CC-BY

I linted the C code with the new clang-format settings, but made the following change so that it didn't make PyObject_HEAD and the threading macros look wonky:

diff --git a/.clang-format b/.clang-format
index be32e6d1..300f8e54 100644
--- a/.clang-format
+++ b/.clang-format
@@ -18,3 +18,7 @@ SpaceBeforeParens: ControlStatements
 SpacesInParentheses: false
 TabWidth: 4
 UseTab: Never
+StatementMacros:
+  - PyObject_HEAD
+  - Py_BEGIN_ALLOW_THREADS
+  - Py_END_ALLOW_THREADS

Tests/helper.py Outdated
@@ -206,6 +207,7 @@ def _test_leak(self, core):
start_mem = self._get_mem_usage()
for cycle in range(self.iterations):
core()
gc.collect()
Copy link
Member

Choose a reason for hiding this comment

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

Did you want to talk about why you added this?

Copy link
Author

Choose a reason for hiding this comment

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

I accidentally left this in here while I was debugging. I'll remove it.

Copy link
Author

@fdintino fdintino Jan 12, 2021

Choose a reason for hiding this comment

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

Actually, I realized now why I added this: without it the leak tests are non-deterministic. I could pad the memory limit to counteract the fact that it may not have hit the gc generation threshold before it checks the memory, but forcing garbage collection after each iteration ensures that the test is deterministic.

src/_avif.c Outdated
}

avifRGBImageAllocatePixels(&rgb);
memcpy(rgb.pixels, rgb_bytes, size);
Copy link
Member

Choose a reason for hiding this comment

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

Please document in a comment that this is safe for r/w, and potentially add an explict check that the rgb_bytes/rgb.pixels is large enough.

src/_avif.c Outdated
return NULL;
}

memcpy(self->data, avif_bytes, size);
Copy link
Member

Choose a reason for hiding this comment

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

Document here as well.

Copy link
Author

Choose a reason for hiding this comment

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

I wasn't entirely sure what you wanted documented for this line. I added this, let me know if it's what you had in mind:

Pillow/src/_avif.c

Lines 484 to 485 in b84a8e0

// We need to allocate storage for the decoder for the lifetime of the object
// (avifDecoderSetIOMemory does not copy the data passed into it)

Copy link
Author

Choose a reason for hiding this comment

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

I was able to avoid a memcpy here by having PyArg_ParseTuple pass in a PyBytesObject and incrementing the reference in the new / decrementing in the dealloc. That also avoids an unnecessary malloc during decoding.

Copy link
Author

Choose a reason for hiding this comment

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

Realized it would probably be better to have you resolve these conversations, to confirm that the feedback has indeed been addressed.

return NULL;
}

size = rgb.rowBytes * rgb.height;
Copy link
Member

Choose a reason for hiding this comment

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

Is this guaranteed to not overflow, even in the face of invalid input?

Copy link
Author

@fdintino fdintino Jan 12, 2021

Choose a reason for hiding this comment

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

libavif currently restricts images to a maximum of 2^28 pixels. If the dimensions are larger than 16384x16384 then the function that sets decoder->image->width and decoder->image->height fails. So I suppose that a 4-channel 16384x16384 8-bit image could overflow on a 32-bit platform. I'm not certain because the codecs used by libavif have their own overflow limit checks. For instance, dav1d enforces a maximum of 2^26 pixels on 32-bit systems. Should I add a check against PY_SSIZE_T_MAX to be sure? (edit: answering my own question and adding this check)

Copy link
Author

Choose a reason for hiding this comment

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

Added here

Pillow/src/_avif.c

Lines 619 to 622 in b84a8e0

if (rgb.height > PY_SSIZE_T_MAX / row_bytes) {
PyErr_SetString(PyExc_MemoryError, "Integer overflow in pixel size");
return NULL;
}

Copy link
Member

Choose a reason for hiding this comment

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

Basically, I'm the one who will get a CVE on this if there's a problem, and I'd like really clear guidelines about what the assumptions are for sizes of things and where they come from for dangerous operations like memset, malloc, and pointer reads/writes. This isn't so much for now, but a couple years down the line, things need to be clear. This will be fuzzed, this will be run under valgrind, so hopefully there won't be problems.

I've basically had to reverse engineer how SgiRleDecode works over the last month or so, and I'd like to be preventing that sort of experience in the future.

Copy link
Author

Choose a reason for hiding this comment

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

Does raising a MemoryError if rgb.height > PY_SSIZE_T_MAX / row_bytes (as I have in the latest PR push) suffice to address that concern?

.ci/install.sh Outdated Show resolved Hide resolved
.ci/install.sh Outdated
@@ -29,6 +30,7 @@ python3 -m pip install -U pytest
python3 -m pip install -U pytest-cov
python3 -m pip install pyroma
python3 -m pip install test-image-results
python3 -m pip install meson
Copy link
Member

Choose a reason for hiding this comment

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

Does meson need to be added to the requirements.txt?

Copy link
Author

Choose a reason for hiding this comment

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

I don't think so. It's a build dependency of dav1d so it's used when building libavif in CI, but it's not otherwise required to run tests.



@skip_unless_feature("avif")
class TestAvifLeaks(PillowLeakTestCase):
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer not iterating a leak test in the standard test suite, as that can be expensive from a time POV. It's ok for the initial cut, but I'd rather not have it long term.

@nulano
Copy link
Contributor

nulano commented Jan 11, 2021

Adding libavif to MSYS2 fails to compile due to a few missing defines (AVIF_CHROMA_UPSAMPLING_AUTOMATIC, AVIF_CHROMA_UPSAMPLING_FASTEST, AVIF_CHROMA_UPSAMPLING_BEST_QUALITY): https://github.com/nulano/Pillow/runs/1683386633?check_suite_focus=true#step:5:77

@fdintino
Copy link
Author

@nulano it looks like those defines were only added in libavif 0.8.3. I'll figure some #if version checks around their usage.

@fdintino fdintino requested a review from wiredfool January 12, 2021 15:53
@fdintino
Copy link
Author

@nulano Is it okay if I cherry-pick your MSYS commit into this PR?

Copy link
Contributor

@nulano nulano left a comment

Choose a reason for hiding this comment

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

@nulano Is it okay if I cherry-pick your MSYS commit into this PR?

Of course, cherry-pick away!

I have a few nitpicks for winbuild/build_prepare, I haven't looked at the rest yet.

winbuild/build_prepare.py Outdated Show resolved Hide resolved
winbuild/build_prepare.py Outdated Show resolved Hide resolved
winbuild/build_prepare.py Outdated Show resolved Hide resolved
winbuild/build_prepare.py Outdated Show resolved Hide resolved
@fdintino
Copy link
Author

@radarhere @wiredfool @nulano I think I've addressed all feedback (except for the requests for docs on building), but I've left it up to you all to resolve conversations (or not).

Is this PR generally on the right track? I've held off on writing docs until I've gotten a signal one way or the other.

@fdintino
Copy link
Author

fdintino commented Feb 15, 2021

Since it's been a month since I asked my question without response, I'll try to reframe it as more specific questions that might be more answerable.

  1. Is the general structure of this plugin acceptable? I tried to hew closely to the conventions elsewhere in the repo, so I assume so, but would appreciate confirmation.
  2. Is the test coverage sufficient? The gaps are all in the error handling. I'd be happy to try to add test cases for those to make it more complete.
  3. I'd appreciate feedback on the encoder settings. For instance: should yuv_format be renamed subsampling to be consistent with the Jpeg plugin? Should I offer a jpeg-like 0-100 quality setting that maps the min/max quantizer? (The colorist library has such a quality setting that still allows qmin/qmax as an advanced override option). Should I eliminate any options? (My vote would be to get rid of qmin_alpha and qmax_alpha).
  4. What would you like to see, CI-wise, with this pull request? While waiting for feedback I worked on putting this plugin in its own package, which builds manylinux wheels in the same manner as the pillow-wheels repo. I setup builds for all the codecs supported by libavif, on all platforms, and also added cached dependencies (see pillow-avif-plugin-depends). That includes a crate vendor tarball for rav1e, as I've found crates.io to be too unreliable for frequent CI builds. Would you want to wait until this PR is wrapped up and merged before I opened a pull request against pillow-wheels?
    • Note: I think licensing shouldn't be an issue for anything: AOM, rav1e, dav1d, SVT-AV1 and libyuv are all BSD-2 licensed, libgav1 is Apache, and they all are covered under the Alliance for Open Media Patent License.
    • It might be overkill to include all codecs in the manylinux and windows wheels. In particular, SVT-AV1 is far from ready for prime-time. My recommendation would be to include AOM, dav1d, and rav1e by default. AOM, being the reference implementation, is the most complete and has the highest quality, while dav1d and rav1e are the fastest (setting aside SVT-AV1).

@fdintino fdintino force-pushed the libavif-plugin branch 3 times, most recently from b433571 to ff56a9c Compare February 24, 2021 03:54
Tests/test_file_avif.py Outdated Show resolved Hide resolved
Tests/test_file_avif.py Outdated Show resolved Hide resolved
@radarhere
Copy link
Member

radarhere commented Mar 26, 2021

This might be a libavif bug, but I find that if I run this PR, libavif has stopped working for macOS.

https://github.com/radarhere/Pillow/runs/2201531959#step:8:1174

/Users/runner/work/Pillow/Pillow/depends/libavif-0.8.4/ext/libyuv/include/libyuv/row.h:750:5: error: 'LIBYUV_UNLIMITED_DATA' is not defined, evaluates to 0 [-Werror,-Wundef]
#if LIBYUV_UNLIMITED_DATA
^
1 error generated.

LIBYUV_UNLIMITED_DATA was a change introduced in libyuv in the last month - https://chromium.googlesource.com/libyuv/libyuv/+/ba033a11e3948e4b3%5E%21/#F2

@wiredfool
Copy link
Member

We're going to need to add the required libraries to the docker images as well, and we're going to need to add these to the oss-fuzz builder to get fuzzer support.

Might as well make a PR to the Pillow-wheels for whatever needs to happen on build. That will also be potentially helpful for getting the dependencies into oss-fuzz.

@doublex
Copy link

doublex commented Dec 7, 2024

@radarhere
Thank you so much for all of your efforts!

radarhere and others added 4 commits December 7, 2024 12:28
* Do not ignore SyntaxError when saving EXIF data

* Do not save orientation in EXIF data

* Do not save XMP and EXIF data from info dictionary

---------

Co-authored-by: Andrew Murray <[email protected]>
* Allow libavif to install rav1e, except on manylinux2014 and aarch64

* Allow libavif to install rav1e on Windows

---------

Co-authored-by: Andrew Murray <[email protected]>
Co-authored-by: Andrew Murray <[email protected]>
radarhere and others added 4 commits December 13, 2024 22:39
* Removed unittest mock

* Updated license

* Increased timeout

---------

Co-authored-by: Andrew Murray <[email protected]>
* Use cmds_cmake

* Added libsharpyuv

* Combine meson into libavif install script

* Simplified condition

---------

Co-authored-by: Andrew Murray <[email protected]>
@fdintino
Copy link
Author

fdintino commented Dec 13, 2024

Since this PR is so far along in review, I figure I should run any non-trivial changes past the reviewers before undertaking them:

libaom is a very capable codec, but it has absolutely awful performance and compression efficiency with default settings. This gets reported as an issue to pillow-avif-plugin, and my advice is usually to use rav1e. How would people feel about changing the order of encoder codecs such that rav1e is preferred over aom?

@codingjoe
Copy link

Since this PR is so far along in review, I figure I should run any non-trivial changes past the reviewers before undertaking them:

libaom is a very capable codec, but it has absolutely awful performance and compression efficiency with default settings. This gets reported as an issue to pillow-avif-plugin, and my advice is usually to use rav1e. How would people feel about changing the order of encoder codecs such that rav1e is preferred over aom?

I can second that. rav1e isn't only faster in my experience, but some might consider it more open…

@radarhere
Copy link
Member

If it is more performant, then I raise no objection to changing the order of preference.

radarhere and others added 4 commits December 15, 2024 08:15
* Removed unused attributes

* Decrement reference count

---------

Co-authored-by: Andrew Murray <[email protected]>
* Use break in switch

* Use walrus operator

* Do not add irot and imir flags if orientation is default

* Do not potentially call Exif tobytes() twice

* Simplified code by only setting info["exif"] once

---------

Co-authored-by: Andrew Murray <[email protected]>
@radarhere
Copy link
Member

Just for awareness, do you know why libavif doesn't also prefer rav1e over aom?

@stalkerg
Copy link

@radarhere libaom it's reference implementation from standard owner https://aomedia.org/ which can decode and encode. rav1e it mostly encoder. But libaom as decoder is slow, all browsers currently use dav1d for decoding.
libavif is also AOMedia project, same as libaom... and because libaom support dec/enc it's obvious choice.

@fdintino
Copy link
Author

fdintino commented Dec 16, 2024

libaom is the most configurable and tunable of the encoding codecs, and with the right parameters it is very competitive—if not superior—to the other codecs, particularly for video. But for whatever reason the default encoder options are suboptimal. One of rav1e's strengths is that it has very sensible defaults. And some of the features aom has that rav1e lacks, like film grain synthesis, are less relevant when encoding still images.

I should note that in libavif, dav1d is preferred over libaom's decoder, so I don't think the fact that aom has both an encoder and decoder figures into the choice. I think it has more to do with aom being the reference AV1 codec (hence, for instance, why rav1e has a tracking ticket for feature parity with aomenc). And also, as @stalkerg noted above, that libaom and libavif are both AOMedia projects.

@vrabaud
Copy link

vrabaud commented Dec 16, 2024

Hi, libavif maintainer here. dav1d is indeed what we recommend for decoding. For encoding, libaom just saw some recent patches to greatly improve quality performance for images, cf https://aomedia.googlesource.com/aom/+/2d2f644e475ce348722ea8b199b49baea285ed04 and all the patches above dealing with SSIMULACRA2 and will become the best recommednation for encoding in libavif. The work that got merged into libaom is by the same great people who did it for SVT-AV1, cf AOMediaCodec/libavif#2412.

@Eskuero
Copy link

Eskuero commented Dec 16, 2024

Nobody argued for it so it may be pointless but I will add an extra argument against SVT as the default. They seem focused only on video so you end up hitting some of their barriers when using images with non standard resolutions. To name a few that I have hit myself:

  • Width can't be bigger than 16384 pixels
  • Height can't be bigger than 8704 pixels.
  • Height * width can't be bigger than 21.012.480 pixels so per example a 8000*3000 can't be encoded either
  • Neither dimension can be odd

So you end with checks like this

# Decide codec to use
if image.width < 64 or image.height < 64 or image.height > 8704 or image.width > 16384 or (image.width * image.height) > 21012480 or (image.width % 2) == 1 or (image.height) % 2 == 1:
	image_codec = "aom"
else:
	image_codec = "svt"

To avoid exceptions like:

Svt[error]: Error Instance 1: Source Width must be even for YUV_420 colorspaces
....
RuntimeError: Failed to encode image: Encoding of color planes failed


That being said on a personal note for anybody's whose usercase is the same as mine: archiving with lossless quality and max compression, I found svt to always beat the other codecs, with aom coming next and rav1e dead last (probably because they do not support lossless.

[eskuero@4cx4]$ pacman -Q | grep -E "rav1e|svt-av1|aom"
aom 3.11.0-1
rav1e 0.7.1-1
svt-av1 2.2.1-1
(venv) [eskuero@4cx4]$ time python convert.py svt
Codec svt output is 19.49% of the original size

real    0m10.453s
user    0m38.610s
sys     0m1.916s
(venv) [eskuero@4cx4]$ time python convert.py aom
Codec aom output is 32.96% of the original size

real    0m14.470s
user    3m11.148s
sys     0m0.261s
(venv) [eskuero@4cx4]$ time python convert.py rav1e
Codec rav1e output is 33.23% of the original size

real    2m42.493s
user    15m44.806s
sys     0m0.458s
(venv) [eskuero@4cx4]$ du -sh *
2.0M    aom.avif
2.1M    rav1e.avif
6.1M    sample.png
1.2M    svt.avif

convert.py

from PIL import Image
import pillow_avif
import sys
import os

os.environ['SVT_LOG'] = '1'
image_codec = sys.argv[1]

image = Image.open("sample.png")
image.save(f"{image_codec}.avif", "AVIF", quality = 100, speed = 0, codec = image_codec)

file_size_og = os.path.getsize("sample.png")
file_size_new = os.path.getsize(f"{image_codec}.avif")
percent = round(file_size_new / file_size_og * 100, 2)

print(f"Codec {image_codec} output is {percent}% of the original size")

@jnoring-pw
Copy link

jnoring-pw commented Dec 16, 2024

Does it make sense to follow suit with whatever the default is in libavif, which is the library doing the dispatching to a specific codec implementation? I feel like that project is going to have the best pulse on what is ideal for defaults.

In my own testing, I stuck with libaom for encoding. What I think is probably most important is A) stability in the default encode/decode choices, B) the ability to customize the codec, and C) good documentation. On point C, what I've found most difficult about AVIF hasn't been the pillow plugin (a thousand thanks to @fdintino for writing this!), but even figuring out the various settings that can be plumbed through, and what they even mean in real-world terms.

@fdintino
Copy link
Author

fdintino commented Dec 17, 2024

I just ran some benchmarks with libavif linked against the aom main branch, and in all of my tests it comes out ahead of rav1e (without having to pass any codec-specific options). So I've reverted the commit that switched the encoder codec priority order. Thanks for the input and context, @vrabaud .

Vincent, do you know when that's likely to end up in a libavif release? It looks like the SSIMULACRA2 commits just missed making it into the last aom release, but I know libavif has at times used a commit SHA for the aom dependency.

"libavif": {
"url": f"https://github.com/AOMediaCodec/libavif/archive/v{V['LIBAVIF']}.zip",
"filename": f"libavif-{V['LIBAVIF']}.zip",
"dir": f"libavif-{V['LIBAVIF']}",
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
"dir": f"libavif-{V['LIBAVIF']}",

This is no longer needed, thanks to #8567

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to open AVIF images