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

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

Closed
lovell opened this issue Sep 15, 2020 · 15 comments · Fixed by #16
Closed

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

lovell opened this issue Sep 15, 2020 · 15 comments · Fixed by #16
Assignees

Comments

@lovell
Copy link
Member

lovell commented Sep 15, 2020

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 lovell/sharp#2289 for more context.

Can anyone think of a reason not to move these?

@kleisauke
Copy link
Member

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 (libvips/libvips#1808) that needs further investigation. This in combination with strukturag/libheif@b22820a could make <16 pixels AVIF images unprocessable.

@lovell
Copy link
Member Author

lovell commented Sep 22, 2020

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
Copy link
Member Author

lovell commented Sep 23, 2020

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
Copy link
Member

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
Copy link
Member Author

lovell commented Sep 24, 2020

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
Copy link
Member Author

lovell commented Sep 24, 2020

Upstream PR to optionally disable libde265 and x265 at strukturag/libheif#337

@kleisauke
Copy link
Member

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

@lovell
Copy link
Member Author

lovell commented Sep 27, 2020

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
Copy link
Member Author

lovell commented Oct 1, 2020

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
Copy link
Member

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
Copy link
Member Author

lovell commented Oct 3, 2020

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
Copy link
Member

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
Copy link
Member Author

lovell commented Oct 3, 2020

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

@kleisauke
Copy link
Member

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 (strukturag/libheif@f231c7a). 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
Copy link
Member Author

lovell commented Oct 4, 2020

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.

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

Successfully merging a pull request may close this issue.

2 participants