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

fdintino · 2021-01-11T01:52:55Z

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

radarhere · 2021-01-11T02:21:53Z

Tests/helper.py

@@ -206,6 +207,7 @@ def _test_leak(self, core):
        start_mem = self._get_mem_usage()
        for cycle in range(self.iterations):
            core()
+            gc.collect()


Did you want to talk about why you added this?

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

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.

This line has been moved into test_file_avif.py

wiredfool · 2021-01-11T20:17:46Z

src/_avif.c

+    }
+
+    avifRGBImageAllocatePixels(&rgb);
+    memcpy(rgb.pixels, rgb_bytes, size);


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.

wiredfool · 2021-01-11T20:20:51Z

src/_avif.c

+        return NULL;
+    }
+
+    memcpy(self->data, avif_bytes, size);


Document here as well.

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)

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.

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

wiredfool · 2021-01-11T20:29:01Z

src/_avif.c

+        return NULL;
+    }
+
+    size = rgb.rowBytes * rgb.height;


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

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)

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;

}

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.

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

wiredfool · 2021-01-11T20:38:19Z

Tests/test_file_avif.py

+
+
+@skip_unless_feature("avif")
+class TestAvifLeaks(PillowLeakTestCase):


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 · 2021-01-11T20:56:26Z

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 · 2021-01-11T23:44:35Z

@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 · 2021-01-12T15:55:15Z

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

nulano

@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

fdintino · 2021-01-18T17:23:25Z

@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 · 2021-02-15T18:25:03Z

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.

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

Tests/test_file_avif.py

radarhere · 2021-03-26T12:14:56Z

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 · 2021-03-28T13:53:16Z

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.

* Removed skip_unless_feature on methods when class is already skipped * Test speed less than slowest and greater than fastest * Updated type hints * Only access angle when AVIF_TRANSFORM_IROT flag is present * Added AVIF_ROOT * Only define normalize_quantize_value if it will be used * Build libavif after libjpeg * Use rgb.rowBytes in overflow check * Group EXIF info * Removed __loaded * If brew is not installed, use /usr prefix * Sort AVIF codecs alphabetically * Updated rav1e license * Fixed catching warning, as per python-pillow#8505 * Simplified code * Fixed typos * Test further scenarios * Use y* to parse bytes --------- Co-authored-by: Andrew Murray <[email protected]>

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

src/_avif.c

* Simplify Python code by receiving tuple from C, as per python-pillow#8740 * Use default PyTypeObject value * Removed AVIF_TRUE * Width and height are already set on first frame * Removed memset * Depth is set by avifRGBImageSetDefaults * Replace PyObject with int * After a failed pixel allocation, destroy non-first frame * Added error if avifImageCreateEmpty returns NULL * Python images cannot have negative dimensions * Test invalid canvas dimensions * Use boolean format argument * Handle avifDecoderCreate and avifEncoderCreate errors * tileRowsLog2 and tileColsLog2 are ignored if autotiling is enabled * Only define _add_codec_specific_options if it may be used * Test non-string advanced value * Simplified error handling in AvifEncoderNew * Corrected heading --------- Co-authored-by: Andrew Murray <[email protected]>

radarhere · 2025-02-15T03:34:17Z

src/PIL/AvifImagePlugin.py

+    range_ = info.get("range", "full")
+    tile_rows_log2 = info.get("tile_rows", 0)
+    tile_cols_log2 = info.get("tile_cols", 0)
+    alpha_premultiplied = bool(info.get("alpha_premultiplied", False))


Do we need alpha_premultiplied as an argument? To my simple way of thinking, it would cause the saved image to no longer be accurate to the Pillow image being saved. We do have a separate mode for premultiplied alpha, RGBa, see https://pillow.readthedocs.io/en/stable/handbook/concepts.html#modes

I've created fdintino#23

I receive a comment on my PR that

I think if the underlying pixel data in the AVIF image was RGBa then it would make sense, because you would then presumably get RGBa back from the decoder. But because it is converting to YUV, and because alphaPremultiply is specified separately for the RGB and YUV image, with the former only existing so that libavif can do alpha multiplying and unmultiplying if necessary, I'm not so sure. To the extent that the RGBa mode is used in pillow, I imagine it is mostly for image compositing operations. But for an AVIF image it really only serves to get more efficient compression on images with detailed alpha planes, in a way that doesn't compromise perceptual quality. I think it wouldn't be obvious that the intended way to enable (what amounts to) a lossy compression flag would be to convert to a different mode, particularly when that mode is different from what is actually stored in the image.

It's not the default behaviour of the plugin, so I'm not going to fight too hard against this. I've closed my PR.

radarhere

Ubuntu Jammy has libavif 0.9.3, so I guess it makes sense to support versions before 1.0.0.

I would like someone else from the core team to review this for any license implications

I've realised that this has essentially copied avifImageGetExifOrientationFromIrotImir and avifImageExtractExifOrientationToIrotImir from libavif. Granted, they are relatively simple functions.
By introducing new dependencies - not just libavif, but also various codecs - there are different licenses to consider. Remember that we don't distribute libimagequant with our wheels

https://pillow.readthedocs.io/en/stable/installation/building-from-source.html#building-from-source

Libimagequant is licensed GPLv3, which is more restrictive than the Pillow license, therefore we will not be distributing binaries with libimagequant support enabled.

vrabaud · 2025-03-03T14:13:25Z

.github/workflows/wheels-dependencies.sh

@@ -50,6 +50,7 @@ LIBWEBP_VERSION=1.5.0
 BZIP2_VERSION=1.0.8
 LIBXCB_VERSION=1.17.0
 BROTLI_VERSION=1.1.0
+LIBAVIF_VERSION=1.1.1


Version 1.2.0 just got released, please let us know if that works for you.

It failed one of our linux aarch64 builds. I've opened a bug report to libyuv here

Updates the pillow-avif-plugin code to more closely match the current state of the open Pillow PR, python-pillow/Pillow#5201. The differences that remain have to do with python 2.7 compatibility. Most of the code changes from the Pillow PR are stylistic, not functional, but there are two bug fixes included: - AvifImagePlugin.CHROMA_UPSAMPLING is now actually used by the decoder. Previously, although it was passed into the decoder, it did not have any effect. Note that this is different from the Pillow PR, where this functionality was removed instead. - AVIF images with irot and imir now have those values converted to an EXIF orientation when decoded. EXIF orientation has been preserved by the encoder since 1.4.2, which is when we started setting irot and imir. But if such an image was converted to another format, the orientation would have been lost.

Updates the pillow-avif-plugin code to more closely match the current state of the open Pillow PR, python-pillow/Pillow#5201. The differences that remain have to do with python 2.7 compatibility. Most of the code changes from the Pillow PR are stylistic, not functional, but there are two bug fixes included: - AvifImagePlugin.CHROMA_UPSAMPLING is now actually used by the decoder. Previously, although it was passed into the decoder, it did not have any effect. Note that this is different from the Pillow PR, where this functionality was removed instead. - AVIF images with irot and imir now have those values converted to an EXIF orientation when decoded. EXIF orientation has been preserved by the encoder since 1.4.2, which is when we started setting irot and imir. But if such an image was converted to another format the orientation would have been lost.

radarhere added the Enhancement label Jan 11, 2021

radarhere reviewed Jan 11, 2021

View reviewed changes

wiredfool requested changes Jan 11, 2021

View reviewed changes

fdintino requested a review from wiredfool January 12, 2021 15:53

nulano reviewed Jan 12, 2021

View reviewed changes

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 mentioned this pull request Jan 13, 2021

AVIF support thumbor/thumbor#1314

Closed

fdintino force-pushed the libavif-plugin branch from 7efcefc to 3ae762e Compare January 23, 2021 16:16

fdintino mentioned this pull request Jan 30, 2021

Fix memcpy/sizeof typo in avifImageCopy AOMediaCodec/libavif#483

Merged

fdintino force-pushed the libavif-plugin branch 3 times, most recently from b433571 to ff56a9c Compare February 24, 2021 03:54

japsu mentioned this pull request Mar 2, 2021

Replace WebP previews with AVIF con2/edegal#197

Open

2 tasks

radarhere reviewed Mar 26, 2021

View reviewed changes

Tests/test_file_avif.py Outdated Show resolved Hide resolved

radarhere reviewed Mar 26, 2021

View reviewed changes

Tests/test_file_avif.py Outdated Show resolved Hide resolved

radarhere mentioned this pull request Mar 26, 2021

Updated libavif to 0.9.0 fdintino/Pillow#1

Merged

fdintino force-pushed the libavif-plugin branch from a9b00e0 to 09567f6 Compare March 28, 2021 16:37

fdintino force-pushed the libavif-plugin branch 5 times, most recently from b851ca6 to 649f5f3 Compare April 12, 2021 12:44

This was referenced Apr 12, 2021

Add libavif dependencies python-pillow/pillow-depends#37

Open

Add libavif build python-pillow/pillow-wheels#193

Closed

Removed qmin and qmax (#17)

Loading
Loading status checks…

1410d23

This was referenced Jan 30, 2025

Use rgb.rowBytes in overflow check fdintino/Pillow#18

Merged

Use aom LICENSE instead of PATENTS fdintino/Pillow#19

Merged

radarhere and others added 3 commits February 1, 2025 21:29

Merge branch 'main' into libavif-plugin

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

Loading
Loading status checks…

6cbad27

Use aom LICENSE instead of PATENTS (#19)

Loading
Loading status checks…

4508f37

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

radarhere mentioned this pull request Feb 5, 2025

Use member names to initialize modules #8734

Merged

Merge branch 'main' into libavif-plugin

Loading
Loading status checks…

7de1212

radarhere reviewed Feb 8, 2025

View reviewed changes

src/_avif.c Outdated Show resolved Hide resolved

wantehchang reviewed Feb 8, 2025

View reviewed changes

src/_avif.c Outdated Show resolved Hide resolved

src/_avif.c Outdated Show resolved Hide resolved

radarhere mentioned this pull request Feb 8, 2025

Removed memset and ignoreAlpha fdintino/Pillow#20

Merged

Removed memset and ignoreAlpha (#20)

Loading
Loading status checks…

e1509ee

radarhere mentioned this pull request Feb 9, 2025

Use member names to initialize PyTypeObjects #8741

Merged

wantehchang reviewed Feb 9, 2025

View reviewed changes

src/_avif.c Outdated Show resolved Hide resolved

src/_avif.c Outdated Show resolved Hide resolved

radarhere mentioned this pull request Feb 12, 2025

Handle avifDecoderCreate and avifEncoderCreate errors fdintino/Pillow#21

Merged

radarhere and others added 2 commits February 12, 2025 15:35

Merge branch 'main' into libavif-plugin

Loading
Loading status checks…

5761b44

radarhere reviewed Feb 15, 2025

View reviewed changes

radarhere mentioned this pull request Feb 15, 2025

Sort formats alphabetically in documentation fdintino/Pillow#22

Closed

radarhere approved these changes Feb 15, 2025

View reviewed changes

radarhere added 2 commits February 21, 2025 18:45

Sort formats alphabetically

38b9941

Simplified code

Loading
Loading status checks…

10dfa63

radarhere mentioned this pull request Feb 21, 2025

Removed alpha_premultiplied fdintino/Pillow#23

Closed

vrabaud reviewed Mar 3, 2025

View reviewed changes

vrabaud mentioned this pull request Mar 3, 2025

Compilation failure on some aarch64 for Pillow AOMediaCodec/libavif#2659

Closed

Merge branch 'main' into libavif-plugin

Loading
Loading status checks…

9abfdbc

radarhere mentioned this pull request Mar 3, 2025

Use default PyTypeObject values fdintino/Pillow#25

Open

fdintino mentioned this pull request Mar 5, 2025

chore: sync with changes from python-pillow/Pillow#5201 fdintino/pillow-avif-plugin#70

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

fdintino commented Jan 11, 2021 •

edited by radarhere

Loading

radarhere Jan 11, 2021

fdintino Jan 11, 2021

fdintino Jan 12, 2021 •

edited

Loading

radarhere Dec 26, 2024 •

edited

Loading

wiredfool Jan 11, 2021

wiredfool Jan 11, 2021

fdintino Jan 12, 2021

fdintino Jan 23, 2021

fdintino Sep 24, 2023

wiredfool Jan 11, 2021

fdintino Jan 12, 2021 •

edited

Loading

fdintino Jan 12, 2021

wiredfool Jan 12, 2021

fdintino Jan 12, 2021

wiredfool Jan 11, 2021

nulano commented Jan 11, 2021

fdintino commented Jan 11, 2021

fdintino commented Jan 12, 2021

nulano left a comment

fdintino commented Jan 18, 2021

fdintino commented Feb 15, 2021 •

edited

Loading

radarhere commented Mar 26, 2021 •

edited

Loading

wiredfool commented Mar 28, 2021

radarhere Feb 15, 2025

radarhere Feb 24, 2025

radarhere Feb 26, 2025

radarhere left a comment •

edited

Loading

vrabaud Mar 3, 2025

fdintino Mar 3, 2025

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

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



		@skip_unless_feature("avif")
		class TestAvifLeaks(PillowLeakTestCase):

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

Are you sure you want to change the base?

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

Conversation

fdintino commented Jan 11, 2021 • edited by radarhere Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fdintino Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

radarhere Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fdintino Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nulano commented Jan 11, 2021

fdintino commented Jan 11, 2021

fdintino commented Jan 12, 2021

nulano left a comment

Choose a reason for hiding this comment

fdintino commented Jan 18, 2021

fdintino commented Feb 15, 2021 • edited Loading

radarhere commented Mar 26, 2021 • edited Loading

wiredfool commented Mar 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radarhere left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fdintino commented Jan 11, 2021 •

edited by radarhere

Loading

fdintino Jan 12, 2021 •

edited

Loading

radarhere Dec 26, 2024 •

edited

Loading

fdintino Jan 12, 2021 •

edited

Loading

fdintino commented Feb 15, 2021 •

edited

Loading

radarhere commented Mar 26, 2021 •

edited

Loading

radarhere left a comment •

edited

Loading