Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 18 months ago

#53653 closed defect (bug) (fixed)

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

Reported by: azaozz's profile azaozz Owned by: azaozz's profile 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 years ago.

Download all attachments as: .zip

Change History (16)

#1 @azaozz
3 years 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 years ago by azaozz (previous) (diff)

#2 @adamsilverstein
3 years 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 years 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: @kirasong
3 years 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 @kirasong
3 years 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 years ago

  • Keywords has-patch added; needs-patch removed

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


3 years ago

#8 in reply to: ↑ 3 @azaozz
3 years 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 years 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 years 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 years 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 years ago

  • Keywords dev-reviewed added

#13 @desrosj
3 years 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 years ago

#15 @abitofmind
18 months ago

Note: This issue here avoided to create responsive images that are larger in file size than their original by avoiding to create lossless responsive images from lossy originals.

Now as extreme as this may seem but in #57238 there's one case where the reverse is happening! The first 2 next smaller sized versions in the responsive images set are lossy (!) but nevertheless larger in file size than their larger original which is lossless and with an alpha channel. Only the 3rd responsive image is then smaller in file size than the original. Check it out!

Note: See TracTickets for help on using tickets.