search

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

Fix single threaded avif encoding #1409

Closed jamsinclair closed 1 month ago

jamsinclair commented 6 months ago

Fixes #1397

As discussed in the issue above, we need to compile a copy of libsharpyuv with threads disabled for the single threaded codec.

I am not that familiar with makefiles or C/C++ compilation. Please feel free to make relevant changes to improve this 🙏

As always, big thanks to the creators/maintainers of Squoosh 🙇‍♂️

google-cla[bot] commented 6 months ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

jzern commented 5 months ago

Maybe it's unrelated, but even with this change I see AVIF encode errors in a stock Linux Chrome (123.0.6312.105 (Official Build) (64-bit)). WebP and others encode all right.

I rebuilt the avif wasm and the main app with:

$ cd codecs/avif
$ npm run build
$ cd ../../
$ npm run build
Compress-e2686f9a.js:4210 Uncaught (in promise) Error: Encoding error
    at encode (features-worker-83baeae9.js:278:19)
deserialize @ util-06ce6ead.js:59
fromWireValue @ util-06ce6ead.js:265
await in fromWireValue (async)
updateImage @ Compress-e2686f9a.js:4106
(anonymous) @ Compress-e2686f9a.js:3931
setTimeout (async)
queueUpdateImage @ Compress-e2686f9a.js:3931
componentDidUpdate @ Compress-e2686f9a.js:3916
(anonymous) @ VM4:78
(anonymous) @ VM4:78
(anonymous) @ VM4:78
j @ VM4:78
(anonymous) @ VM4:78
g @ VM4:78
handleMouseUp_ @ unknown
Promise.then (async)
k @ VM4:78
d.setState @ VM4:78
Compress.onEncoderTypeChange @ Compress-e2686f9a.js:3730
Options$9.onEncoderTypeChange @ Compress-e2686f9a.js:2973
z @ VM4:78
handleMouseUp_ @ unknown
features-worker-83baeae9.js:92 jxlEncode: 10146.291015625 ms
features-worker-83baeae9.js:92 jxlDecode: 3007.024169921875 ms

This matches the behavior with the checked in wasm files.

jamsinclair commented 5 months ago

Maybe it's unrelated, but even with this change I see AVIF encode errors in a stock Linux Chrome.

@jzern I can't replicate this behaviour. Were you building from the forked branch?

There's a deploy preview generated by the CLI that you can test with

And I've also hosted a version that will always use the single-threaded variants

If you get a chance to try with those, do you get the same result?

jzern commented 5 months ago

Maybe it's unrelated, but even with this change I see AVIF encode errors in a stock Linux Chrome.

@jzern I can't replicate this behaviour. Were you building from the forked branch?

I had tried both the tip of tree and this PR, they both had the same issue.

There's a deploy preview generated by the CLI that you can test with

This fails in the same way using any of the sample images.

And I've also hosted a version that will always use the single-threaded variants

This passes with all of them.

If you get a chance to try with those, do you get the same result?

jzern commented 5 months ago

Maybe it's unrelated, but even with this change I see AVIF encode errors in a stock Linux Chrome.

@jzern I can't replicate this behaviour. Were you building from the forked branch?

I had tried both the tip of tree and this PR, they both had the same issue.

There's a deploy preview generated by the CLI that you can test with

This fails in the same way using any of the sample images.

Works on Windows 10: 123.0.6312.122 (Official Build) (64-bit) (cohort: M123 Rollout).

And I've also hosted a version that will always use the single-threaded variants

This passes with all of them.

If you get a chance to try with those, do you get the same result?

jzern commented 5 months ago

Maybe it's unrelated, but even with this change I see AVIF encode errors in a stock Linux Chrome.

@jzern I can't replicate this behaviour. Were you building from the forked branch?

I had tried both the tip of tree and this PR, they both had the same issue.

There's a deploy preview generated by the CLI that you can test with

This fails in the same way using any of the sample images.

Works on Windows 10: 123.0.6312.122 (Official Build) (64-bit) (cohort: M123 Rollout).

macOS: 123.0.6312.123 (Official Build) (arm64) Linux (intel): 123.0.6312.122 (Official Build) (64-bit) both pass. The failing machine has a similar version of Chrome and Linux, but is AMD; I'm not sure if that's the issue, though. Let me disable any experiments I'm in there.

And I've also hosted a version that will always use the single-threaded variants

This passes with all of them.

If you get a chance to try with those, do you get the same result?

jzern commented 5 months ago

Maybe it's unrelated, but even with this change I see AVIF encode errors in a stock Linux Chrome.

@jzern I can't replicate this behaviour. Were you building from the forked branch?

I had tried both the tip of tree and this PR, they both had the same issue.

There's a deploy preview generated by the CLI that you can test with

This fails in the same way using any of the sample images.

Works on Windows 10: 123.0.6312.122 (Official Build) (64-bit) (cohort: M123 Rollout).

macOS: 123.0.6312.123 (Official Build) (arm64) Linux (intel): 123.0.6312.122 (Official Build) (64-bit) both pass. The failing machine has a similar version of Chrome and Linux, but is AMD; I'm not sure if that's the issue, though. Let me disable any experiments I'm in there.

I don't have any flags set actually. This seems like a separate issue to this PR, so I'll file an issue if I can get more detail.

And I've also hosted a version that will always use the single-threaded variants

This passes with all of them.

If you get a chance to try with those, do you get the same result?

Branchverse commented 1 month ago

I came across the same issue explained in https://github.com/GoogleChromeLabs/squoosh/issues/1397 which is supposed to be solved with this PR, I see that it has been approved. Is there something missing for the completion since I won't be the only one that tries to solve this for a few hours before realizing there is a fix ready for 4 months?

laoneo commented 1 month ago

Thanks!

AshleyScirra commented 1 month ago

Thanks for the fix folks! Appreciate it.