WordPress.org

Make WordPress Core

Opened 3 weeks ago

Closed 3 weeks ago

Last modified 3 weeks ago

#53653 closed defect (bug) (fixed)

Scaled down WebP images may be with much larger file size than the original

Reported by: azaozz Owned by: azaozz
Milestone: 5.8 Priority: normal
Severity: normal Version: 5.8
Component: Media Keywords: has-patch fixed-major commit dev-reviewed
Focuses: Cc:

Description

As reported in https://wordpress.org/support/topic/webp-very-large-filesize-5-8-rc2/.

It appears that when ImageMagick is used to create the WebP sub-sizes, the output is set to lossless even though the original image is with lossy compression.

Attachments (1)

53653.diff (846 bytes) - added by adamsilverstein 3 weeks ago.

Download all attachments as: .zip

Change History (15)

#1 @azaozz
3 weeks ago

Looking a bit more, seems all WebP images exported by Gimp 2.10 (latest) are detected as animated-alpha by wp_get_webp_info() despite that they were saved as lossy and lossless (no animation).

Looking in the file headers, the "type bits" are indeed VP8X. Seems this is a problem with the JPEG => WebP converting software or these bits cannot be used to reliably detect lossy and lossless WebP images.

Last edited 3 weeks ago by azaozz (previous) (diff)

#2 @adamsilverstein
3 weeks ago

Interesting, our code assumes that images with the VP8X header are animated or alpha, lossy should have the VP8 header.

Maybe @marylauc or @blobfolio can shed some light on this.

Can you provide some test images so we can see what you are working with? When you are exporting your image, can you verify that lossy is enabled?

In my testing I converted my jpeg to webp with https://squoosh.app before uploading (using lossless). My subsizes were all lossless, and each webp image was smaller than the jpeg counterpart.

#3 follow-ups: @adamsilverstein
3 weeks ago

In 53653.diff

  • Only use lossless format for WebP output when header detection determines image is lossless.

Fixes previous logic to favor using lossy format as default, only using lossless when the image engine supports it and we know the image is lossless. Image engines don't support alpha or transparency for webp yet as far as I know anyway.

#4 follow-up: @mikeschroder
3 weeks ago

Moving some of my findings over here from Slack (beginning of conversation):

My reading of the spec (https://developers.google.com/speed/webp/docs/riff_container#example_file_layouts) agrees with @azaozz's finding that VP8X doesn't necessarily mean lossless or animated.

Right now, core is assuming VP8X always means animated.

This happens when wp_get_webp_info() is called here:
https://github.com/WordPress/wordpress-develop/blob/master/src/wp-includes/class-wp-image-editor-imagick.php#L206

Then, VP8X sets animated-alpha as the type:
https://github.com/WordPress/wordpress-develop/blob/4eb17be18cdf7b2cf8e4dd4e4511dda76a5e9a11/src/wp-includes/media.php#L5170

So a lossy WebP from Gimp, for instance, is detected as animated, then saved as lossless.

webpinfo indicates that the files from Gimp, whether lossy or lossless, are valid, and all use VP8X, regardless of the extra info I tell it to include.

Lastly, I think this a separate issue, but to make sure it's documented --
When a JPEG is loaded, then saved as a WebP, only the JPEG compression settings are used, as set_quality() is run only on load, as far as I am aware.
https://github.com/WordPress/wordpress-develop/blob/28ea7b18cf23e9e373a52cad21a739b030e3fced/src/wp-includes/class-wp-image-editor-imagick.php#L200

#5 in reply to: ↑ 3 @mikeschroder
3 weeks ago

Replying to adamsilverstein:

In 53653.diff

  • Only use lossless format for WebP output when header detection determines image is lossless.

Fixes previous logic to favor using lossy format as default <snip>

Tested this, and it looks to do as expected so far.

It would be even better if we could detect lossless images more accurately (there are still VP8L images located within files that begin with VP8X sections, which core is currently not detecting), but I agree that it's much better to default to assuming we should save as lossy than lossless.

#6 @desrosj
3 weeks ago

  • Keywords has-patch added; needs-patch removed

This ticket was mentioned in Slack in #core by desrosj. View the logs.


3 weeks ago

#8 in reply to: ↑ 3 @azaozz
3 weeks ago

Replying to adamsilverstein:

Patch works as expected here too, thanks.

Only use lossless format for WebP output when header detection determines image is lossless.

That leaves the opposite case: VP8X with a VP8L "chunk" will produce lossy sub-sizes. However agree this is a smaller issue. Opened #53663 to try to fix WebP features detection, can even try it for 5.8.1.

#9 in reply to: ↑ 4 @azaozz
3 weeks ago

Replying to mikeschroder:

Thanks for the detailed explanation!

It would be even better if we could detect lossless images more accurately (there are still VP8L images located within files that begin with VP8X sections, which core is currently not detecting)

Yes, wp_get_webp_info() needs fixing/update so it can detect the rest of the info when the type is VP8X. Can see some js code for that, nothing in PHP though. See #53663.

#10 @azaozz
3 weeks ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

In 51435:

Media: When resizing WebP images set the compression to "lossy" by default. Fixes a bug where the compression was set to "lossless" when the uploaded WebP images have extended file format (VP8X).

Props adamsilverstein, mikeschroder, mmxxi, linux4me2.
Fixes #53653.

#11 @azaozz
3 weeks ago

  • Keywords fixed-major commit added; needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopen for the 5.8 branch.

#12 @desrosj
3 weeks ago

  • Keywords dev-reviewed added

#13 @desrosj
3 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 51437:

Media: When resizing WebP images set the compression to "lossy" by default. Fixes a bug where the compression was set to "lossless" when the uploaded WebP images have extended file format (VP8X).

Props adamsilverstein, mikeschroder, mmxxi, linux4me2, azaozz.
Merges [51435] to the 5.8 branch.
Fixes #53653.

This ticket was mentioned in Slack in #core-media by desrosj. View the logs.


3 weeks ago

Note: See TracTickets for help on using tickets.