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 support for AVIF #20765

Closed
wants to merge 15 commits into from
Closed

Add support for AVIF #20765

wants to merge 15 commits into from

Conversation

paambaati
Copy link
Contributor

Pass 2 of #20381 after I did a rebase (my bad, sorry).

This PR adds support for generating AVIF images, now that Sharp v0.27.0 supports the format.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

This image was generated with -

avif --input='./test/integration/image-optimizer/public/test.svg' --quality=90  --output='./test/integration/image-optimizer/public' --overwrite --verbose
@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@paambaati
Copy link
Contributor Author

The new Azure tests seem a little flakey, but otherwise all tests pass and this PR is ready for review.

@ijjk

This comment has been minimized.

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good 👍

Have you measured performance for converting to avif compared to webp?

If we are going to start using avif as the default, then we don't want to slow down image processing and lose the magic of this API.

Additionally, it looks like browsers don't support images smaller than 16x16: lovell/sharp#2289 (comment)

@paambaati
Copy link
Contributor Author

paambaati commented Jan 5, 2021

@styfle Thanks for reviewing the PR.

Have you measured performance for converting to avif compared to webp?

No; however, in my limited unscientific usage, there’s no discernible difference. That said, I’d be more than happy to run and publish some benchmarks here - are you looking for specific metrics? Just ops/second or CPU/memory usage too?

Additionally, it looks like browsers don't support images smaller than 16x16

How do you propose I handle this? Read the width prop and use WebP if it’s below 16?

Actually, following the linked issues, it looks like they’ve all been fixed in sharp v0.27?

@ijjk

This comment has been minimized.

@styfle
Copy link
Member

styfle commented Jan 5, 2021

Just ops/second or CPU/memory usage too?

Good question! Lets do both 🙂

I think the important thing to test is how it handles different sized source images as well as different source formats.

@ijjk

This comment has been minimized.

@paambaati
Copy link
Contributor Author

paambaati commented Jan 6, 2021

@styfle I ran some benchmarks, and they're now available at https://github.com/paambaati/avif-benchmark#avif-vs-webp-conversion-benchmark

tl;dr
Converting to AVIF is ever so slightly ⚡️ faster than WebP! There's almost zero impact to CPU/RAM usage.

Benchmark summary

Average calculated over 1000 runs.

Source size Source format Average WebP conversion time Average AVIF conversion time
1920 × 2504 JPG .2961174730 ms .2261837420 ms 🏎
1280 × 1669 JPG .2761548210 ms .2373165780 ms 🏎
640 × 834 JPG .2731627210 ms .2406256660 ms 🏎
1275 × 916 PNG .2583528300 ms .2347853600 ms 🏎

@ijjk

This comment has been minimized.

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR and benchmarks!

We can use this to run some more benchmarks with real websites and see how it performs 👍

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@Timer
Copy link
Member

Timer commented Jan 14, 2021

FYI you don't need to keep this PR merged with canary, we'll update and land it when we're ready!

@timneutkens
Copy link
Member

This PR will need a rework as sharp has been removed in favor of the squoosh.app wasm binaries.

@paambaati
Copy link
Contributor Author

This PR will need a rework as sharp has been removed in favor of the squoosh.app wasm binaries.

@timneutkens I'll close this PR for now. I looked at the PR that removed sharp and replaced it with the WASM binaries from the Squoosh project (#22253), and I echo @developit's sentiment (see #22253 (comment)).

The change delta is huge and I'm not even sure where to start.

@paambaati paambaati closed this Mar 11, 2021
@ArianHamdi
Copy link
Contributor

Can you re-open this PR because of adding Sharp support in v11.0.2-canary.20 #27346 ?

@paambaati
Copy link
Contributor Author

@ArianHamdi BAM! #27432

@vercel vercel locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants