libvips / build-win64-mxe

76 stars 15 forks source link

Proposal: move aom and libheif to "web" dependencies #13

Closed lovell closed 3 years ago

lovell commented 4 years ago

I'm (slowly) setting up the infrastructure to ensure the aom+libheif+libvips combo is safe (fuzz, leak and regression tested) for binary distribution and therefore allow the prebuilt binaries provided by sharp to read and write AVIF files. See https://github.com/lovell/sharp/issues/2289 for more context.

Can anyone think of a reason not to move these?

kleisauke commented 4 years ago

Great! I have no objections to move these dependencies to the "web" variant. I made some local changes that switches the libheif build to autotools. This removes the need of patching the CMake build and makes it more inline with sharp-libvips repo.

As an aside, There is still an issue in libvips (https://github.com/libvips/libvips/issues/1808) that needs further investigation. This in combination with https://github.com/strukturag/libheif/commit/b22820a276d7597f7f633bc57c34c28212701cdb could make <16 pixels AVIF images unprocessable.

lovell commented 4 years ago

Thanks, I'll prepare a PR for this.

We'll want to wait for the next release of libheif anyway as there's at least one security-related fix and many colour-related (nclx) improvements.

lovell commented 4 years ago

I took a quick look at this - there's currently no way of telling libheif not to include x265 support if it finds it. I'm happy to submit a PR to libheif to add --without-x265 and --without-de265 options (and their cmake equivalents), but it got me thinking of something else...

Given all the patent problems surrounding HEVC, are we happy that this repo is currently providing binaries that can encode and decode HEIC/HEIF images? I'm not letting these anywhere near the pre-compiled binaries provided by sharp as they are download millions of times, significantly more than the "royalty free 100,000 devices" limit the MPEG LA patent pool allows.

kleisauke commented 4 years ago

I'm fine with removing support for encoding and decoding HEIC/HEIF images in the "all" variant by default. I could move these libraries (i.e. libde265 and x265) to a plugin (just like MozJPEG) and let users build their own patent-encumbered HEVC binaries, does that sound OK?

FYI: ImageMagick seems to distribute binaries linked against libde265 on their website, I'm not sure how they deal with such legal issues.

lovell commented 4 years ago

Great, that sounds like a plan. We'll still need to add new --without-x265 etc. flags to libheif either way so I'll get on with that shortly.

It's possible some open source projects think they are under the protection of a 5 year "software policy" offered by HEVC Advance (one of the patent pools) intended for non-commercial end users and currently due to expire this year.

It looks like HEVC Advance have now removed this policy from their website (ominous or accidental?) but a salient part is:

This policy replaces all prior Policy Statements (original issued December 1, 2016), and will continue through the initial five-year term of the HEVC Advance PPL ending on December 31, 2020. HEVC Advance intends to extend this policy indefinitely thereafter if it is achieving the objective of driving adoption of HEVC hardware in mobile device and desktop personal computers at initial sale.

http://web.archive.org/web/20190808043505/https://www.hevcadvance.com/pdfnew/Software_Policy_092017.pdf

My understanding is that whilst this policy might offer ImageMagick some protection from some HEVC patents when distributing binaries, a commercial entity using those binaries to decode/encode HEVC would not enjoy that protection.

lovell commented 4 years ago

Upstream PR to optionally disable libde265 and x265 at https://github.com/strukturag/libheif/pull/337

kleisauke commented 4 years ago

WIP branch to move the HEVC dependencies to a plugin at https://github.com/libvips/build-win64-mxe/tree/exclude-hevc-deps.

lovell commented 4 years ago

The libheif PR has now been merged so the --disable-libde265 and --disable-x265 flags we need will be available in the next version.

lovell commented 4 years ago

WIP branch to move aom and libheif to web dependencies at https://github.com/lovell/build-win64-mxe/tree/web-add-aom-libheif

This will require the forthcoming libheif v1.9.2 (or v1.10.0, whichever comes first).

kleisauke commented 4 years ago

The WIP branch looks good to me. Autotools only warns when unrecognized options are passed, so I think it should already work for libheif v1.9.1.

lovell commented 4 years ago

The WIP branch builds libheif v1.9.1 with HEVC as it finds x265 in the environment, so we will need to wait for flags to prevent that. from happening.

kleisauke commented 4 years ago

That's curious, it should only build x265 and libde265 if the --with-hevc flag is passed. Maybe these dependencies are a leftover of an older build (prior to PR #14)? In that case, I usually delete the entire mxe directory.

lovell commented 4 years ago

Ah, yes, that'll be it, thank you. This is probably ready for a PR then.

kleisauke commented 4 years ago

WIP branch to adapt libvips' test suite for AVIF images at https://github.com/kleisauke/libvips/commits/testsuite-avif. This allows to run the test suite with the pre-built binaires.

While doing this, I noticed something that might not be desired. It seems that libheif always includes dependency information in the image as metadata (https://github.com/strukturag/libheif/commit/f231c7a941d64cb3528f373ae9f0580c61dbb80b). See for example:

$ heif-info avif-orientation-6.avif -d | grep hdlr -A 6
| Box: hdlr -----
| size: 85   (header size: 12)
| version: 0
| flags: 0
| pre_defined: 0
| handler_type: pict
| name: libheif (1.9.1) / AOMedia Project AV1 Encoder v2.0.0

Also, the time spent on the test suite increases from 81.25s to 301.99s(!), see: https://travis-ci.org/github/libvips/libvips/jobs/732559106#L2479 https://travis-ci.org/github/kleisauke/libvips/jobs/732586151#L2484

lovell commented 4 years ago

We should open a new question/issue in libheif to ask if the maintainers would consider a PR to make the name of the hdlr box optional (HEIF headers are already verbose enough).

In terms of testing time, the speed property makes quite a big difference to AVIF compression times, plus reducing image entropy (e.g. using blank images) might help too.