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

core(modern-image-formats): ignore images CF claims are bigger as webp #14358

Closed
wants to merge 1 commit into from

Conversation

connorjclark
Copy link
Collaborator

closes #14265

#14265 (comment) points out that while hand-tuning could really result in some savings, it seems OK to defer to CF for users of their automatic conversion feature.

@connorjclark connorjclark requested a review from a team as a code owner September 6, 2022 20:32
@connorjclark connorjclark requested review from brendankenny and removed request for a team September 6, 2022 20:32
@brendankenny
Copy link
Member

brendankenny commented Sep 6, 2022

One source of confusion for users is that modern-image-formats is actually estimating savings when converting to AVIF since #12682 (in the 8.1.0 release), but the description still includes "WebP" (in fact, it puts "WebP" before "AVIF").

We discussed in #12682 (review) the original plan of moving the WebP estimate to uses-optimized-images, but decided not to and just leave the WebP estimate in the JSON, but we left WebP in the description. Maybe we just need a description update? That means we don't have a recommendation for WebP anywhere, though...

@connorjclark
Copy link
Collaborator Author

connorjclark commented Sep 8, 2022

couple things we may want to do here (not mutually exclusive)

(1) drop WEBP from description
(2) add "webp savings" column (and denote the existing savings column as "avif savings")
(3) do an actual conversion in OptimizedImages (needs protocol support)


(1) in isolation means we no longer mention webp in the report anywhere
(2) in isolation means we just duplicate information, since one value is computed from the other
(2) + (3) seems good but....

(3) is a lot of work.

https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/src/images/SkImageEncoder.cpp;l=58;drc=a442c20f87b3f20ec085cc20f86938f82b8c6914 shows all the formats skia supports for encoding

there is of course a avif codec in skia, but it only decodes https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/src/codec/SkAvifCodec.cpp;l=1?q=src%2Fcodec%2FSkAvifCodec.cpp&ss=chromium%2Fchromium%2Fsrc:third_party%2Fskia%2F

skia uses libavif which does support encoding https://github.com/AOMediaCodec/libavif so it'd be a matter of tapping into that, then exposing this to the protocol in the same way the other formats are.

should we pursue (3)?


back to the PR... it seems that we can just close this given we aren't even comparing the same thing (avif savings vs CF's webp)

@connorjclark
Copy link
Collaborator Author

closing for #14592

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable WebP warning when Cloudflare Polish says WebP is bigger
3 participants