-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
base: main
Are you sure you want to change the base?
Conversation
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() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document here as well.
There was a problem hiding this comment.
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:
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added here
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; | |
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@@ -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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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.
Adding libavif to MSYS2 fails to compile due to a few missing defines ( |
@nulano it looks like those defines were only added in libavif 0.8.3. I'll figure some |
@nulano Is it okay if I cherry-pick your MSYS commit into this PR? |
There was a problem hiding this 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
'cmd.exe /c "aom.cmd"', | ||
"if errorlevel 1 echo AOM build failed! && exit /B 1", | ||
'cmd.exe /c "dav1d.cmd"', | ||
"if errorlevel 1 echo dav1d build failed! && exit /B 1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last line of aom.cmd
is cd ../..
which will never fail, so the errorlevel
checks have absolutely no effect. The build fails on my system as I don't have git
on PATH, yet it continues anyway. However, there should also be a "clean" step, probably cmd_rmdir
, so that it is possible to re-run the generated scripts when debugging build_prepare
locally.
'cmd.exe /c "aom.cmd"', | |
"if errorlevel 1 echo AOM build failed! && exit /B 1", | |
'cmd.exe /c "dav1d.cmd"', | |
"if errorlevel 1 echo dav1d build failed! && exit /B 1", | |
cmd_rmdir("aom"), | |
'cmd.exe /c "aom.cmd"', | |
cmd_rmdir("dav1d"), | |
'cmd.exe /c "dav1d.cmd"', |
I would prefer to build these as separate steps (similar to libpng and freetype, or raqm and its dependencies), but AFAICT AOM can only be downloaded using git, so it's probably simpler this way. But the extra dependencies (git, meson, ninja) should at least be noted in the winbuild/build.rst
and winbuild/readme.md
documents as well as maybe docs/installation.rst
(Bulding on Windows section). There should also be a way to disable these similar to the --no-imagequant
and --no-raqm
flags, such as --no-avif
.
@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. |
7efcefc
to
3ae762e
Compare
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.
|
b433571
to
ff56a9c
Compare
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
LIBYUV_UNLIMITED_DATA was a change introduced in libyuv in the last month - https://chromium.googlesource.com/libyuv/libyuv/+/ba033a11e3948e4b3%5E%21/#F2 |
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. |
a9b00e0
to
09567f6
Compare
b851ca6
to
649f5f3
Compare
* Added type hints * Updated nasm to 2.16.03 * Removed duplicate meson install * Simplified code * Sort formats alphabetically * tile is already an empty list --------- Co-authored-by: Andrew Murray <radarhere@users.noreply.github.com>
da466a6
to
e5494a2
Compare
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
@fdintino are you taking PayPal donations? I want to send over my love from the Django community. |
@codingjoe Can you explain why this is good for Django? Would love to have a good answer I can point other folks too. Thank you 🙏 |
Sure, I maintain django-pictures, a responsive cross-browser image library. I guess every good web engineer wants to serve fast, responsive, high-res images. And since AVIF is part of Baseline 2024, folks are eager to get started. I know I am 😁 |
def test_heif_raises_unidentified_image_error(self) -> None: | ||
with pytest.raises(UnidentifiedImageError): | ||
with Image.open("Tests/images/avif/rgba10.heif"): | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the relevance of this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is intended to test this bit of logic:
Pillow/src/PIL/AvifImagePlugin.py
Lines 36 to 44 in ee3c46a
if major_brand in container_brands: | |
# We accept files with AVIF container brands; we can't yet know if | |
# the ftyp box has the correct compatible brands, but if it doesn't | |
# then the plugin will raise a SyntaxError which Pillow will catch | |
# before moving on to the next plugin that accepts the file. | |
# | |
# Also, because this file might not actually be an AVIF file, we | |
# don't raise an error if AVIF support isn't properly compiled. | |
return True |
If the ftyp box is avif
or avis
then this is definitely an AVIF image, and so it is sensible to return a string from the accept function to trigger an error. But mif1
and msf1
are valid for both AVIF and HEIF, and the initial bytes passed to the accept function are not long enough to determine which of the two it is. In order to be interoperable with pyheif I cannot raise an error in the accept, and must instead raise a SyntaxError in _open
if AVIF is not compiled. This would cause Pillow to failover to any other plugins that accept the file, or—if none are found—cause it to raise an UnidentifiedImageError
. Perhaps the test would be clearer in purpose and more useful if instead I created a mock plugin that would serve as the failover, and then asserted that it is called.
def test_decoder_strict_flags(self) -> None: | ||
# This would fail if full avif strictFlags were enabled | ||
with Image.open("Tests/images/avif/chimera-missing-pixi.avif") as im: | ||
assert im.size == (480, 270) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it possible that someone could build libavif with those strict flags enabled, and then build Pillow from source to use it? So in that scenario, this test would fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strictFlags
are passed at runtime, they're not set at compile time. By default it strictly enforces various specification rules, but two in particular are often found in the wild and so are regularly disabled:
Lines 797 to 802 in 8b8bbba
// Turn off libavif's 'clap' (clean aperture) property validation. | |
self->decoder->strictFlags &= ~AVIF_STRICT_CLAP_VALID; | |
// Allow the PixelInformationProperty ('pixi') to be missing in AV1 image | |
// items. libheif v1.11.0 and older does not add the 'pixi' item property to | |
// AV1 image items. | |
self->decoder->strictFlags &= ~AVIF_STRICT_PIXI_REQUIRED; |
Chromium disables these two flags and WebKit disables AVIF_STRICT_PIXI_REQUIRED
.
The test_decoder_strict_flags
test verifies that the strictFlags are set appropriately so that these somewhat common images can load without throwing an error.
) | ||
exif_orientation = exif_data.get(orientation_tag) or 0 | ||
|
||
xmp = info.get("xmp", im.info.get("xmp") or im.info.get("XML:com.adobe.xmp")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're saving XMP and EXIF data from the info
dictionary.
With the exception of JPEG and XMP, potentially to be reversed by #8483, none of our other plugins currently do this.
|
||
Then see ``depends/install_raqm_cmake.sh`` to install libraqm. | ||
See ``depends/install_raqm_cmake.sh`` to install libraqm and | ||
``depends/install_libavif.sh`` to install libavif. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a reader, I find this (with the context of the lines before it) confusing. It makes me wonder if it I should pkg install libavif
or depends/install_libavif.sh
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've suggested removing the reference to install_libavif.sh
from this sentence in fdintino#6
@codingjoe I've never solicited donations for my open-source work, but I certainly wouldn't mind if someone was motivated to give something. The best place to send me personal messages and questions is fdintino@gmail.com, which is also what you can use to find me on PayPal. Thanks! |
* Removed unnecessary meson install * Use the same Python as the build script * Use python3 * Simplified code * Updated meson --------- Co-authored-by: Andrew Murray <radarhere@users.noreply.github.com>
literly every single embedded mapping library (think google maps) ... atm png and jpeg are defecto formats for the map tiles (chunks) |
* Removed unused C values * Set default max threads in Python --------- Co-authored-by: Andrew Murray <radarhere@users.noreply.github.com>
* Use filename placeholder in URL * Removed unused upsampling setting * Use has_transparency_data * Removed unnecessary load() * Test getexif() change --------- Co-authored-by: Andrew Murray <radarhere@users.noreply.github.com>
Is there a chance to merge this pull request into Pillow 11.1? |
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 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: