GoogleChromeLabs / squoosh

Make images smaller using best-in-class codecs, right in the browser.
https://squoosh.app
Apache License 2.0
21.82k stars 1.53k forks source link

AVIF encoding is broken on browsers without multi-threaded support #1397

Closed jamsinclair closed 1 month ago

jamsinclair commented 8 months ago

Describe the bug When the browser does not support SharedArrayBuffer/Multi-threading, Squoosh fails to encode AVIF images.

To Reproduce Steps to reproduce the behavior:

  1. Clone the repository
  2. Remove the Cross-Origin-Opener-Policy and Cross-Origin-Embedder-Policy to force the dev environment to use the single threaded logic
  3. Run npm run dev
  4. Open the dev page at http://localhost:5000
  5. Upload an image. Choose the AVIF compress option.
  6. Observe the following error is thrown in the console and the image is not encoded to AVIF
    Uncaught (in promise) RuntimeError: indirect call to null
    invoke_iiiiiiiiii http://localhost:5000/c/avif_enc-8f7d01d5.js:10
    invoke_iii http://localhost:5000/c/avif_enc-8f7d01d5.js:10
    encode http://localhost:5000/c/avif_enc-8f7d01d5.js line 10 > Function:11
    encode http://localhost:5000/c/features-worker-2348e279.js:276

Expected behavior The image would still be encoded to AVIF with single threaded browser.

Version:

Additional context, screenshots, screencasts This issue was introduced in the recent update of the AVIF version #1381

jakearchibald commented 8 months ago

Thanks for the report. I'm trying to figure out the impact of this bug. How did you encounter it? Is there an environment where this is impacting users that aren't deliberately disabling features in their browser?

jamsinclair commented 8 months ago

Thanks @jakearchibald. I didn't expect such a prompt reply. I think the impact of this bug is quite small, it's definitely low priority.

How did you encounter it?

I was just playing around with it 😅 . I have a hobby project that shares these codecs as individual ES modules (with licenses and correct attributions).

Is there an environment where this is impacting users that aren't deliberately disabling features in their browser?

No, I don't think that's possible. A small number of older browsers might be affected, but I doubt you see much traffic for them on squoosh.app.

jakearchibald commented 8 months ago

Makes sense. We'll work on a fix, but it doesn't seem to warrant a rollback.

Thanks for playing around with the project and finding the issue!

AshleyScirra commented 6 months ago

Oof, just lost a couple of hours to trying to figure out why avif_enc.wasm (the single threaded version) wasn't working in a different project - this looks like the cause. The previous version seems to work fine. For the benefit of anyone else searching, the error message I get is null function or function signature mismatch.

aryanpingle commented 6 months ago

Quick way to reproduce this - In avifEncode.ts, change:

if (await checkThreadsSupport()) {
  const avifEncoder = await import('codecs/avif/enc/avif_enc_mt');
  return initEmscriptenModule<AVIFModule>(avifEncoder.default);
}

to:

if (false && await checkThreadsSupport()) {
  const avifEncoder = await import('codecs/avif/enc/avif_enc_mt');
  return initEmscriptenModule<AVIFModule>(avifEncoder.default);
}

which enables only single-threaded encoding.

image

image


Looking into this 👍

tyaqing commented 6 months ago

I have encountered the same problem.

aryanpingle commented 6 months ago

@wantehchang I've narrowed this issue down to this line in avif_enc.cpp:

avifResult encodeResult = avifEncoderWrite(encoder.get(), image.get(), &output);

encoder.get() and image.get() are not returning null pointers, so I believe it's something with the avifEncoderWrite function itself. Any idea what it might be?

wantehchang commented 6 months ago

Aryan: We will need to debug it. You can start by logging the return value of avifEncoderWrite(). If the return value is not AVIF_RESULT_OK, log the diagnostic message encoder->diag.error.

jamsinclair commented 6 months ago

I could be doing something wrong, but when I enabled some debugging flags like -g it seems to have helped reveal some of the method names in the stack trace. The last 5 calls being:

indirect call to null
av1_predict_intra_block@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[485]:0x57f65
av1_predict_intra_block_facade@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[487]:0x58444
intra_model_rd@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[920]:0x108514
av1_rd_pick_intra_mode_sb@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[949]:0x11c2b6
pick_sb_modes@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[1126]:0x184c60

It seems to fail before the AVIF_RESULT_OK check and there's no string data for encoder->diag.error.

I am sure there's more debugging to be done, but perhaps there's something up with how av1 is being compiled?

Full stack trace ``` indirect call to null av1_predict_intra_block@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[485]:0x57f65 av1_predict_intra_block_facade@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[487]:0x58444 intra_model_rd@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[920]:0x108514 av1_rd_pick_intra_mode_sb@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[949]:0x11c2b6 pick_sb_modes@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[1126]:0x184c60 av1_rd_pick_partition@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[1149]:0x193397 av1_rd_pick_partition@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[1149]:0x193e49 av1_rd_pick_partition@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[1149]:0x193e49 av1_rd_pick_partition@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[1149]:0x193e49 av1_encode_sb_row@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[1050]:0x15ab46 av1_encode_tile@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[1044]:0x1565ba encode_frame_internal@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[1161]:0x1b3b93 av1_encode_frame@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[1160]:0x1af4d1 encode_with_recode_loop_and_filter@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[1469]:0x20f322 av1_encode@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[1179]:0x1c739d av1_encode_strategy@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[1170]:0x1c0550 invoke_iiiiiiiiii@webpack://with-webpack-example/../../packages/avif/dist/codec/enc/avif_enc.js?:3410:36 av1_get_compressed_data@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[1042]:0x15598f invoke_iii@webpack://with-webpack-example/../../packages/avif/dist/codec/enc/avif_enc.js?:3421:36 encoder_encode@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[1504]:0x220a15 aom_codec_encode@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[1435]:0x1fcdfc aomCodecEncodeImage@http://localhost:9000/6e2d8511161e40ca5061.wasm:wasm-function[142]:0x13524 ```
jamsinclair commented 6 months ago

Aha! Well that was a fun one to debug 😅

Looks like the issue was with the compilation of libsharpyuv. We need to add -DCMAKE_DISABLE_FIND_PACKAGE_Threads=1 in the makefile for single thread builds. Single thread encodes work successfully with that flag.

Do we need to compile different versions of libsharpyuv? Or would it be ok to use a single threaded copy for both?

surma commented 6 months ago

Brilliant work, @jamsinclair. Thanks so much.

We should definitely compile a separate copy of libsharpyuv, as we might have to add more permutations in the future. In fact, IIRC, we already do this for libaom in the helper.Makefile.

Do you have the time and would you be willing to whip up a PR for the fix at hand?

wantehchang commented 6 months ago

@jamsinclair wrote:

Looks like the issue was with the compilation of libsharpyuv. We need to add -DCMAKE_DISABLE_FIND_PACKAGE_Threads=1 in the makefile for single thread builds.

Please try libsharpyuv's (libwebp's) cmake option -DWEBP_USE_THREAD=OFF instead.

@jzern FYI.

jamsinclair commented 6 months ago

Do you have the time and would you be willing to whip up a PR for the fix at hand?

Sure thing, I'll whip that up later 🖖

Please try libsharpyuv's (libwebp's) cmake option -DWEBP_USE_THREAD=OFF instead.

Thank you! I'll give that a go.