-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add AVIF to wheels using only aomenc and dav1d AVIF codecs for reduced size #8858
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
base: main
Are you sure you want to change the base?
Conversation
I'm going to see what effect @wantehchang's suggestion has on the wheel sizes. |
50c6633
to
86464ca
Compare
Also build libavif as a shared library. When it is built as a static library, the dependency library files are combined into a single archive. But when they are linked as a shared library, the linker is able to remove unused objects. This yields a modest but not insignificant file size reduction.
86464ca
to
8b4e66e
Compare
-DCMAKE_BUILD_TYPE=Release \ | ||
-DBUILD_SHARED_LIBS=OFF \ | ||
-DBUILD_SHARED_LIBS=ON \ |
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 mean to change BUILD_SHARED_LIBS
to ON
? It is still OFF
in winbuild/build_prepare.py.
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 did. Using the linker to generate the shared library file allows it to remove unused objects. There is a mechanism for including shared library dependencies of a python extension inside a wheel file on macOS and linux (delocate and auditwheel, respectively) but there is no such standard mechanism for doing the same on windows. So the windows library has to be built statically.
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 savings are modest, about 0.5 MB, but there isn't any cost to switching to a shared library.
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.
When you say 0.5mb, I presume that's per wheel. Which wheel(s) were you using to check that?
ENABLE_NASM=ON is unnecessary...it forces the use of nasm over yasm if both are available, but only nasm is installed in the wheel build environments. And CMAKE_MODULE_PATH was only necessary when we used pre-built rav1e binaries.
Frankie: Here are some other ideas to further reduce the binary size.
|
8328cd4
to
7696f13
Compare
7696f13
to
f5d13cd
Compare
f5d13cd
to
deb1c9d
Compare
The alternative #8869 removed installation of things like |
They have been removed here as well. The only difference between this PR and that one is that this one uses the dav1d decoder instead of AOM's. And it leaves open the possibility of users compiling for more codecs |
@hugovk Another difference between this PR and the alternative #8869 is that this PR has additional changes to the build system to reduce the binary size, specifically:
For a fair comparison, these changes should be applied to the alternative #8869. |
if [[ "$MB_ML_VER" == 2014 ]] && [[ "$PLAT" == "x86_64" ]]; then | ||
build_type=Release | ||
fi | ||
libavif_cmake_flags+=(-DCMAKE_SHARED_LINKER_FLAGS_INIT="-Wl,--strip-all,-z,relro,-z,now") |
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.
If I compare the wheel sizes from https://github.com/python-pillow/Pillow/actions/runs/14773502497?pr=8858,
with a commit where I remove this line, I find that https://github.com/radarhere/Pillow/actions/runs/14773542213 is actually 8kb smaller.
If this line is purely about reducing file size, then I don't think it's fulfilling that role.
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.
Ah, it's because there is a typo. I'll fix 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.
It actually had to do with a cmake policy that was disabling LTO on linux. See the build here: https://github.com/fdintino/Pillow/actions/runs/14777836226
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.
In the PR description, I've updated the paragraph that begins with "In absolute terms,..." and the last column of the table with the latest wheel and shared library file sizes. You can look at the github diff under the list of edits to see the change.
# CONFIG_AV1_DECODER=0 is a flag for libaom (included as a subproject of | ||
# libavif) to disable the compilation and inclusion of aom's AV1 decoder. | ||
# CONFIG_AV1_HIGHBITDEPTH=0 is another flag for libaom that disables support | ||
# for encoding high bit depth images. |
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.
We should not disable support for encoding high bit depth images. Most AVIF HDR images are 10 bits. Can we avoid 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.
Pillow does not yet support high bit depth images (see #1888). It is currently a work-in-progress, but at the moment there can only ever be 8-bit images passed to the C API.
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.
Thanks for the reply.
Nit: Change "another flag" to "a flag" because we are removing the previous paragraph (lines 131-132).
We can look into disabling support for decoding high bit depth images in the dav1d library. It has this build option:
option('bitdepths',
type: 'array',
choices: ['8', '16'],
description: 'Enable only specified bitdepths')
But it seems hard to pass this build option to dav1d through libavif's cmake command line.
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 think we might still want to allow decoding of 10-bit images, even if that necessarily means downsampling to 8 bit to load them into Pillow.
* Added release notes * Simplified code * AVIF works on Windows x86 --------- Co-authored-by: Andrew Murray <[email protected]>
This PR was about x1.8 in April (#8856 (comment)), and we're getting close to the next release, how is it looking now? |
Yes, please. |
The size of Pillow 11.2.1 artefacts (wheels and sdist zips) was 321.03mb The size from main is currently 374.13mb - it's expected that would go up though, as we now include Python 3.14 beta wheels. The size from this PR is currently 591.47mb - so that's 1.58 times main. |
I expect libsharpyuv doesn't have a license file here because it comes from libwebp. |
Co-authored-by: Hugo van Kemenade <[email protected]>
Fixes #8856
This removes all libavif codecs except for AOM for encoding and dav1d for decoding. It also builds libavif as a shared library on linux and macOS, which modestly decreases the file size because it allows the linker to omit unused objects from the codec libraries. Building with
-Os
and-flto
reduces the size by another further 0.7-1.5MB.Below is a comparison of the average CPython wheel sizes by platform for the wheels (MiB) prior to the merge of the avif pull request, the current 11.2.0 wheels, and the wheels from this pull request. An additional few-hundred K could be saved by using aom's decoder instead of dav1d, but I think the benefits of dav1d outweigh those cost savings.
Since Pillow does not yet support multi-channel high bit depth images, I've compiled libaom without high-bit-depth support. This further reduces wheel sizes by ~200KiB.
In absolute terms, the uncompressed libavif shared library file size is somewhere between 2.8MiB (macOS ARM) and 5.0MiB (musllinux x86_64), with the exception of manylinux2014 x86_64 which, because of a gcc bug, is missing some optimizations and so is 7.4MiB.
-flto -Os