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.

Sign up for GitHub

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

Jump to bottom

feat(image-optimizer): Add support for AVIF #27432

Closed
wants to merge 11 commits into from
Closed

feat(image-optimizer): Add support for AVIF #27432

wants to merge 11 commits into from

Conversation

paambaati
Copy link
Contributor

This PR resurrects #20765, now that conditional support for sharp has been added back.

Bug

  • Related issues linked using fixes #number
  • Integration tests added
  • Errors have helpful link attached, see contributing.md

Feature

  • Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • Related issues linked using fixes #number
  • Integration tests added
  • Documentation added
  • Telemetry added. In case of a feature if it's used or not.
  • Errors have helpful link attached, see contributing.md

Documentation / Examples

  • Make sure the linting passes

@paambaati paambaati mentioned this pull request Jul 23, 2021
Closed
@ijjk

This comment has been minimized.

Sign in to view
@ijjk

This comment has been minimized.

Sign in to view
@paambaati
Copy link
Contributor Author

paambaati commented Jul 23, 2021 •
edited

@styfle @atcastle AFAIK, AVIF doesn't have a magic number.

How would you recommend I handle the failing test? Nevermind, removed the test.

@ijjk

This comment has been minimized.

Sign in to view
@ijjk

This comment has been minimized.

Sign in to view
@ijjk

This comment has been minimized.

Sign in to view
@ijjk

This comment has been minimized.

Sign in to view
@ijjk

This comment has been minimized.

Sign in to view
@ijjk

This comment has been minimized.

Sign in to view
@ijjk

This comment has been minimized.

Sign in to view
@paambaati paambaati marked this pull request as draft July 26, 2021 05:47
@ijjk

This comment has been minimized.

Sign in to view
@ijjk

This comment has been minimized.

Sign in to view
@ijjk
Copy link
Member

ijjk commented Jul 26, 2021

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary paambaati/next.js canary Change
buildDuration 14.8s 14.6s -244ms
buildDurationCached 3.4s 3.4s ⚠️ +34ms
nodeModulesSize 50.3 MB 50.3 MB ⚠️ +292 B
Page Load Tests Overall increase ✓
vercel/next.js canary paambaati/next.js canary Change
/ failed reqs 0 0 ✓
/ total time (seconds) 2.457 2.426 -0.03
/ avg req/sec 1017.35 1030.4 +13.05
/error-in-render failed reqs 0 0 ✓
/error-in-render total time (seconds) 1.348 1.323 -0.03
/error-in-render avg req/sec 1854.99 1889.35 +34.36
Client Bundles (main, webpack, commons)
vercel/next.js canary paambaati/next.js canary Change
359.HASH.js gzip 2.96 kB 2.96 kB ✓
745.HASH.js gzip 180 B 180 B ✓
framework-HASH.js gzip 42.2 kB 42.2 kB ✓
main-HASH.js gzip 21 kB 21 kB ✓
webpack-HASH.js gzip 1.53 kB 1.53 kB ✓
Overall change 67.9 kB 67.9 kB ✓
Legacy Client Bundles (polyfills)
vercel/next.js canary paambaati/next.js canary Change
polyfills-HASH.js gzip 31.1 kB 31.1 kB ✓
Overall change 31.1 kB 31.1 kB ✓
Client Pages
vercel/next.js canary paambaati/next.js canary Change
_app-HASH.js gzip 803 B 803 B ✓
_error-HASH.js gzip 3.06 kB 3.06 kB ✓
amp-HASH.js gzip 554 B 554 B ✓
css-HASH.js gzip 329 B 329 B ✓
dynamic-HASH.js gzip 2.52 kB 2.52 kB ✓
head-HASH.js gzip 2.28 kB 2.28 kB ✓
hooks-HASH.js gzip 903 B 903 B ✓
image-HASH.js gzip 5.63 kB 5.63 kB ✓
index-HASH.js gzip 261 B 261 B ✓
link-HASH.js gzip 1.66 kB 1.66 kB ✓
routerDirect..HASH.js gzip 319 B 319 B ✓
script-HASH.js gzip 387 B 387 B ✓
withRouter-HASH.js gzip 320 B 320 B ✓
bb14e60e810b..30f.css gzip 125 B 125 B ✓
Overall change 19.1 kB 19.1 kB ✓
Client Build Manifests
vercel/next.js canary paambaati/next.js canary Change
_buildManifest.js gzip 489 B 489 B ✓
Overall change 489 B 489 B ✓
Rendered Page Sizes
vercel/next.js canary paambaati/next.js canary Change
index.html gzip 531 B 531 B ✓
link.html gzip 542 B 542 B ✓
withRouter.html gzip 525 B 525 B ✓
Overall change 1.6 kB 1.6 kB ✓
Webpack 4 Mode (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary paambaati/next.js canary Change
buildDuration 11.5s 11.5s -41ms
buildDurationCached 4.5s 4.4s -116ms
nodeModulesSize 50.3 MB 50.3 MB ⚠️ +292 B
Page Load Tests Overall decrease ⚠️
vercel/next.js canary paambaati/next.js canary Change
/ failed reqs 0 0 ✓
/ total time (seconds) 2.421 2.566 ⚠️ +0.15
/ avg req/sec 1032.46 974.31 ⚠️ -58.15
/error-in-render failed reqs 0 0 ✓
/error-in-render total time (seconds) 1.363 1.392 ⚠️ +0.03
/error-in-render avg req/sec 1834.63 1796.02 ⚠️ -38.61
Client Bundles (main, webpack, commons)
vercel/next.js canary paambaati/next.js canary Change
17.HASH.js gzip 2.98 kB 2.98 kB ✓
18.HASH.js gzip 185 B 185 B ✓
677f882d2ed8..HASH.js gzip 13.8 kB 13.8 kB ✓
framework.HASH.js gzip 41.9 kB 41.9 kB ✓
main-HASH.js gzip 8.4 kB 8.4 kB ✓
webpack-HASH.js gzip 1.22 kB 1.22 kB ✓
Overall change 68.5 kB 68.5 kB ✓
Legacy Client Bundles (polyfills)
vercel/next.js canary paambaati/next.js canary Change
polyfills-HASH.js gzip 31.3 kB 31.3 kB ✓
Overall change 31.3 kB 31.3 kB ✓
Client Pages
vercel/next.js canary paambaati/next.js canary Change
_app-HASH.js gzip 791 B 791 B ✓
_error-HASH.js gzip 3.76 kB 3.76 kB ✓
amp-HASH.js gzip 552 B 552 B ✓
css-HASH.js gzip 333 B 333 B ✓
dynamic-HASH.js gzip 2.71 kB 2.71 kB ✓
head-HASH.js gzip 2.97 kB 2.97 kB ✓
hooks-HASH.js gzip 911 B 911 B ✓
index-HASH.js gzip 231 B 231 B ✓
link-HASH.js gzip 1.64 kB 1.64 kB ✓
routerDirect..HASH.js gzip 298 B 298 B ✓
script-HASH.js gzip 3.02 kB 3.02 kB ✓
withRouter-HASH.js gzip 294 B 294 B ✓
e025d2764813..52f.css gzip 125 B 125 B ✓
Overall change 17.6 kB 17.6 kB ✓
Client Build Manifests
vercel/next.js canary paambaati/next.js canary Change
_buildManifest.js gzip 500 B 500 B ✓
Overall change 500 B 500 B ✓
Rendered Page Sizes
vercel/next.js canary paambaati/next.js canary Change
index.html gzip 576 B 576 B ✓
link.html gzip 588 B 588 B ✓
withRouter.html gzip 569 B 569 B ✓
Overall change 1.73 kB 1.73 kB ✓
Commit: a131ffe

@ijjk

This comment has been minimized.

Sign in to view
LetItRock
LetItRock reviewed Jul 27, 2021
View reviewed changes
packages/next/server/image-optimizer.ts
Comment on lines +355 to +357
if (contentType === AVIF) {
transformer.avif({ quality })
} else if (contentType === WEBP) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you've just added the support for the case when sharp exists in deps, but what about the squoosh fallback?
in the official squoosh repository, I do see that AVIF is supported ;) should we bring it to Next repo? @styfle @timneutkens wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

Yes it will need to be in both. I'm going to update squoosh soon after we get a few bugs fixed

@ijjk

This comment has been minimized.

Sign in to view
@ijjk
Copy link
Member

ijjk commented Aug 3, 2021

Failing test suites

Commit: b36c66a

test/integration/route-load-cancel/test/index.test.js

  • next/dynamic > dev mode > should cancel slow page loads on re-navigation
Expand output

● next/dynamic › dev mode › should cancel slow page loads on re-navigation

NoSuchElementError: no such element: Unable to locate element: {"method":"css selector","selector":"#page-text"}
  (Session info: headless chrome=92.0.4515.107)

  31 |     await waitFor(1000)
  32 |
> 33 |     const text = await browser.elementByCss('#page-text').text()
     |                  ^
  34 |     expect(text).toMatch(/2/)
  35 |     expect(await browser.eval('window.routeCancelled')).toBe('yes')
  36 |   })

  at Object.throwDecodedError (../node_modules/selenium-webdriver/lib/error.js:550:15)
  at parseHttpResponse (../node_modules/selenium-webdriver/lib/http.js:565:13)
  at Executor.execute (../node_modules/selenium-webdriver/lib/http.js:491:26)
  at thenableWebDriverProxy.execute (../node_modules/selenium-webdriver/lib/webdriver.js:700:17)
  at Object.<anonymous> (integration/route-load-cancel/test/index.test.js:33:18)

@addyosmani
Copy link

In case interesting, the latest version of sharp (0.29.0+) has faster AVIF encoding support: https://twitter.com/lovell/status/1427655568102133760

@paambaati paambaati mentioned this pull request Aug 23, 2021
Closed
@styfle
Copy link
Member

styfle commented Aug 23, 2021 •
edited

One thing to consider here is that only Chrome and Firefox support AVIF. Safari and Edge do not.

This could reduce cache hit ratios because now each image could to be converted to both WebP and AVIF (multiply that by the number of deviceSizes and you can see this dramatically increases the amount of possibilities).

In particular, external images configured with domains would be most affected and statically imported images would not be as big of an issue.

But every cache is finite so at some point, cached images need to be removed so in those cases any website with many images will be affected.

So we'll probably need to introduce AVIF under a flag so users can opt-in. Then once Safari and Edge add support we can change the default behavior.

@styfle styfle mentioned this pull request Oct 6, 2021
Merged
8 tasks
@kodiakhq kodiakhq bot closed this in #29683 Oct 11, 2021
kodiakhq bot pushed a commit that referenced this pull request Oct 11, 2021
@styfle
Add support for AVIF to next/image (#29683)
cc1f3b8 
Add support for AVIF to `next/image`

- Fixes #27882 
- Closes #27432 

## Feature

- [x] Implements an existing feature request
- [x] Related issues linked
- [x] Integration tests added
- [x] Documentation added
- [x] Update manifest output
- [x] Warn when `sharp` is outdated
- [x] Errors & Warnings have helpful link attached
- [ ] Remove `image-size` in favor of `squoosh`/`sharp` (optional, need to benchmark)
@vercel vercel locked as resolved and limited conversation to collaborators Jan 27, 2022
natew pushed a commit to natew/next.js that referenced this pull request Feb 16, 2022
@styfle @natew
Add support for AVIF to next/image (vercel#29683)
1948495 
Add support for AVIF to `next/image`

- Fixes vercel#27882 
- Closes vercel#27432 

## Feature

- [x] Implements an existing feature request
- [x] Related issues linked
- [x] Integration tests added
- [x] Documentation added
- [x] Update manifest output
- [x] Warn when `sharp` is outdated
- [x] Errors & Warnings have helpful link attached
- [ ] Remove `image-size` in favor of `squoosh`/`sharp` (optional, need to benchmark)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@LetItRock LetItRock LetItRock left review comments

@meck93 meck93 meck93 left review comments

@huozhi huozhi Awaiting requested review from huozhi

@ijjk ijjk Awaiting requested review from ijjk

@leerob leerob Awaiting requested review from leerob

@lfades lfades Awaiting requested review from lfades

@padmaia padmaia Awaiting requested review from padmaia

@shuding shuding Awaiting requested review from shuding

@styfle styfle Awaiting requested review from styfle

@timneutkens timneutkens Awaiting requested review from timneutkens

Assignees
No one assigned
Labels
type: next
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@paambaati @ijjk @addyosmani @styfle @LetItRock @meck93