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

Add aom and libheif to 'web' group dependencies for AVIF support #16

Merged
merged 10 commits into from
Oct 9, 2020

Conversation

lovell
Copy link
Member

@lovell lovell commented Oct 3, 2020

Resolves #13

@lovell lovell requested a review from kleisauke October 3, 2020 14:30
@lovell
Copy link
Member Author

lovell commented Oct 6, 2020

@kleisauke Thanks for adding the libheif.pc patch. (I finally managed to get libheif's CI to pass with that change.)

@kleisauke
Copy link
Member

Great! I'm currently debugging a segfault in SSE2 code that happens on Windows i686 in combination with GCC (v10.2 and 9.3) and aom (master and v2.0.0). The binaries produced by LLVM works without any problems.

Details
Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=0d5d7e70 ebx=00000072 ecx=0321e4f8 edx=00000630 esi=0d5d7e70 edi=00000000
eip=6a450370 esp=0321e460 ebp=0321e460 iopl=0         nv up ei pl nz na pe nc
cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00010206
libvips_42!aom_dc_left_predictor_8x16_sse2+0x10:
6a450370 660f6f01        movdqa  xmm0,xmmword ptr [ecx] ds:002b:0321e4f8=72727272727272727272727272727272

I suspect this is an assembler bug, I'm now trying to compile GCC v10.2 with binutils v2.35.1 as the binutils provided by MXE is old (v2.28, released on 2017-03-06). If that doesn't work, I think we need to disable the SIMD optimizations for i686 (i.e. compile with -DAOM_TARGET_CPU=generic).

@lovell
Copy link
Member Author

lovell commented Oct 6, 2020

Did you see aom's -DENABLE_SSE2=0 flag? https://aomedia.googlesource.com/aom/+/master/build/cmake/cpu.cmake#70

@kleisauke
Copy link
Member

Ah, I missed that. Upgrading binutils to the latest version didn't help, unfortunately. I disabled SSE2 with commit 050e4eb but it seems that this also disables the remaining optimizations (possible due to that disable_remaining_flavors flag).

i686 now almost passes the test suite, it fails with the last heifsave test here:
https://github.com/libvips/libvips/pull/1845/files#diff-ad635814429f3e0cbd2aa94557b49443R1083-R1090

Failed to initialize encoder: Memory allocation error

(this needs a patch for libheif to improve the error reporting, see: 21ae186)

@kleisauke
Copy link
Member

Oh, I just noticed that position-independent code is disabled by default within aom, see:
https://aomedia.googlesource.com/aom/+/master/build/cmake/aom_config_defaults.cmake#81

Perhaps forcing the -fPIC flags here:

export CFLAGS := -s -O3 -fPIC
export CXXFLAGS := -s -O3 -fPIC

could collide with the CONFIG_PIC variable in aom's CMake config. I'll have a try to build aom with -DCONFIG_PIC=1 (Firefox and Chrome also seems to force PIC for 32-bit builds) and revert that SSE2 change, perhaps that will resolve the above mentioned segfault.

@lovell
Copy link
Member Author

lovell commented Oct 6, 2020

Perhaps aom's Not Invented Here approach is part of their patent defence program? ;)

@kleisauke
Copy link
Member

The SSE2 segfault turns out to be a stack alignment bug, commit 755c615 adds a patch to fix this. I also incorporated patches from dav1d/webp with commit afdf49b and backported a security fix with commit 36bfc16.

Windows x86 still has a OOM failure on the heifsave test suite, I'm not sure if there is an easy way to fix that. x64 passes the test suite without problems. I think this is ready to be included for the libvips 8.10.2 binaries, are you OK with that?

@lovell
Copy link
Member Author

lovell commented Oct 8, 2020

Fantastic, thank you Kleis, I'll let you press the big green merge button when you're ready.

@kleisauke
Copy link
Member

kleisauke commented Oct 9, 2020

For the record, test suite currently fails on Windows ARM64 when checking the absolute difference between loading and saving the same AVIF image. I could also reproduce this on the command line (on Windows ARM64), when I do this:

$ vips copy avif-orientation-6.avif x2.avif[compression=av1]

Output: https://t0.nl/x2.avif
Expected output: https://github.com/kleisauke/libvips/blob/testsuite-avif/test/test-suite/images/avif-orientation-6.avif

Anyway, it works fine on Linux ARM64 (see libvips/libvips@c4f9bdc and Travis build), so this is ready for merging.

@kleisauke kleisauke merged commit 9078dc3 into libvips:master Oct 9, 2020
@lovell lovell deleted the web-add-aom-libheif branch October 9, 2020 11:10
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 this pull request may close these issues.

Proposal: move aom and libheif to "web" dependencies
2 participants