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 WebP for uploads module #32

Merged
merged 21 commits into from
Dec 7, 2021
Merged

Add WebP for uploads module #32

merged 21 commits into from
Dec 7, 2021

Conversation

adamsilverstein
Copy link
Member

@adamsilverstein adamsilverstein commented Dec 2, 2021

Fixes #4
Also addresses #22

With this module enabled, uploaded JPEG images are saved as WebP sub-sizes.

  • Adds test that:
    • Validate the addition of the jpeg -> webp conversion mapping to the image_editor_output_format filter
    • Verify the filter is only applied when system supports WebP.

@adamsilverstein adamsilverstein changed the title Add a webp-default module, first pass Add a webp-default module Dec 2, 2021
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Awesome to see the first module! 🎉

Functionality-wise this mostly looks good, left a few comments on implementation details. In addition, I have two higher-level comments:

  • I'm not sure about the webp-default (WebP default) name, as it's a bit unprecise. Honestly I also don't have a better idea yet, but let's brainstorm this a bit more? Maybe something like webp-image-uploads?
  • It would be great to add unit tests. The functionality here is super simple, but since this is the first module it might still be good to cover the filter addition call and set a precedence for how to add module tests. It would also allow properly testing the infrastructure from Complete PHPUnit infrastructure for out-of-the-box local and CI testing, including initial tests #29 for modules.

load.php Outdated Show resolved Hide resolved
modules/webp-default/load.php Outdated Show resolved Hide resolved
modules/webp-default/class-webp-default.php Outdated Show resolved Hide resolved
@adamsilverstein
Copy link
Member Author

I'm not sure about the webp-default (WebP default) name, as it's a bit unprecise. Honestly I also don't have a better idea yet, but let's brainstorm this a bit more? Maybe something like webp-image-uploads?

how about webp-uploads?

It would be great to add unit tests. The functionality here is super simple, but since this is the first module it might still be good to cover the filter addition call and set a precedence for how to add module tests. It would also allow properly testing the infrastructure from #29 for modules.

Will do, I'll start with a simple test to verify the filter is added as expected then look at adding a test that actually verifies webp is output when processing a jpeg.

@adamsilverstein adamsilverstein force-pushed the add/webp-default-module branch from 237fc7e to 9429e06 Compare December 3, 2021 16:05
update_option( PERFLAB_MODULES_SETTING, $new_value );
perflab_load_active_modules();

if ( ! wp_image_editor_supports( array( 'mime_type' => 'image/webp' ) ) ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

would be ideal to run this test against several PHP versions, some that support WebP and some that don't. That way we can validate that the filter is only applied when the system supports WebP.

this check may be overly defensive as I think we do a similar check in core before actually switching, but I thik still worth having.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Right now the test suite only runs on latest WordPress and PHP 7.4, maybe let's open a separate issue to evaluate what versions we should run on? We also need to decide in general which WordPress and PHP versions the plugin should support overall, so that could be discussed in the same issue.

@adamsilverstein adamsilverstein marked this pull request as ready for review December 3, 2021 17:14
modules/webp-uploads/load.php Outdated Show resolved Hide resolved
tests/webp-uploads-test.php Outdated Show resolved Hide resolved
tests/webp-uploads-test.php Outdated Show resolved Hide resolved
update_option( PERFLAB_MODULES_SETTING, $new_value );
perflab_load_active_modules();

if ( ! wp_image_editor_supports( array( 'mime_type' => 'image/webp' ) ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Right now the test suite only runs on latest WordPress and PHP 7.4, maybe let's open a separate issue to evaluate what versions we should run on? We also need to decide in general which WordPress and PHP versions the plugin should support overall, so that could be discussed in the same issue.

tests/webp-uploads-test.php Outdated Show resolved Hide resolved
modules/webp-uploads/load.php Outdated Show resolved Hide resolved
@felixarntz felixarntz changed the title Add a webp-default module Add a webp-uploads module Dec 3, 2021
@adamsilverstein adamsilverstein force-pushed the add/webp-default-module branch from 1bb7a9c to 155989b Compare December 3, 2021 20:00
@felixarntz felixarntz added the [Type] Feature A new feature within an existing module label Dec 6, 2021
@felixarntz felixarntz added this to the Initial plugin release milestone Dec 6, 2021
@felixarntz felixarntz changed the title Add a webp-uploads module Add WebP for uploads module Dec 6, 2021
felixarntz and others added 6 commits December 6, 2021 12:03
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
@adamsilverstein adamsilverstein force-pushed the add/webp-default-module branch from cfcfdcf to f8c9e2f Compare December 7, 2021 20:56
@adamsilverstein
Copy link
Member Author

  • Committed suggestions and removed unnecessary filter cleanup.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

🎉

@felixarntz felixarntz merged commit dcbea35 into trunk Dec 7, 2021
@JustinyAhin
Copy link
Member

Hello @adamsilverstein. It's great to see this PR merged 🎉.

I've made a few tests, and I have a few things to mention.

First I've uploaded a medium size JPEG image (width = 1280px) in the media library. The image remains a JPEG in the media library. And I added it in a post. Which output a WebP image in the frontend. I think this is the expected behavior right?
But I noticed that the srcet attribute of the image contains the JPEG image (for 1280w) in addition to the other sizes that are in WebP. Is that normal?

For the second test, I uploaded a very large image (width>2560px). So the image have been scaled down to 2560px. And while being scaled down the image have been converted to WebP in the media library.
But there is one inconsistency there: the filename ends with .webp but the file type remains JPEG (see https://cln.sh/4An7Qp).

@erikyo
Copy link

erikyo commented Dec 13, 2021

if I may interfere, in the past I have already built a filter that saves (and removes) a webp copy of all images uploaded in wordpress (using imagemagik), it works with document previews if they are created by WordPress (as it happens with pdf for example).
I use the "wp_generate_attachment_metadata" filter because there is no real way to add formats to the image generation but with that hook I can, however, get the data after the image has been generated (but the metadata are saved twice)
https://gist.github.com/erikyo/ebd78f0d5d1735175e5ab9ad1d4d0590

In my personal experience, compressing jpgs into webp could be just a waste of time and space for users because imho in terms of size and quality these two formats are quite similar and would create only a useless duplication of the same file, while, with the conversion of transparent png the advantage is very huge (about 1/10 of the original file)

@JustinyAhin I guess the filename.ext.webp is the most common name variation used for the webp copy, because it's very easy to hijack the request with nginx

@adamsilverstein
Copy link
Member Author

I've made a few tests, and I have a few things to mention.
Thanks for testing!

First I've uploaded a medium size JPEG image (width = 1280px) in the media library. The image remains a JPEG in the media library. And I added it in a post. Which output a WebP image in the frontend. I think this is the expected behavior right?
right

But I noticed that the srcet attribute of the image contains the JPEG image (for 1280w) in addition to the other sizes that are in WebP. Is that normal?
This might be expected. if your image isn't large enough to create certain sizes, the original image would be served instead.

For the second test, I uploaded a very large image (width>2560px). So the image have been scaled down to 2560px. And while being scaled down the image have been converted to WebP in the media library.
But there is one inconsistency there: the filename ends with .webp but the file type remains JPEG (see https://cln.sh/4An7Qp).

oh, good catch - that looks wrong, might be a bug there. the post meta still records the jpeg mime type. the image is actually a wepb though i expect... i'll take a look.

@adamsilverstein
Copy link
Member Author

Hey @erikyo - thank you sharing your experience and the code snippet, very interesting.

I noticed in your code, you used a very high quality setting for creating the images (95 for jpegs for example) - did you try with lower quality?

I use the "wp_generate_attachment_metadata" filter because there is no real way to add formats to the image generation but with that hook I

with this approach, I assume WordPress does not add your WebP images to the post meta sizes array, right? I wonder if we could get WordPress to generate these images instead.

In my personal experience, compressing jpgs into webp could be just a waste of time and space for users because imho in terms of size and quality these two formats are quite similar and would create only a useless duplication of the same file

With a setting of 82 quality, the files should be around 30% smaller, that seems worthwhile, especially since the current approach generates the WebP sub sizes instead of the jpeg sub sizes, not in addition to them.

I'm curious how your approach works on the front end, do you replace all urls in the default img src/srcset with '.webp'?

and would create only a useless duplication of the same file

right this is why we only create the webp version and serve that.. it might be useful to have extra mime types/files for avif or jpegxl though

@adamsilverstein
Copy link
Member Author

@JustinyAhin - I created a follow up task to investigate the mime type data issue further - #75

@erikyo
Copy link

erikyo commented Jan 1, 2022

hi @adamsilverstein,

you used a very high quality setting for creating the images (95 for jpegs for example) - did you try with lower quality?

Yes and for what it looks to me with lower compressions the image was degraded (I can be wrong!) and I preferred a 1:1 ratio between the weight of the jpg and webp (in practice the only advantage is with png where instead i got a about 15% of the original weight). Much later i read the article you posted here, i'll try with these settings, is likely that i was wrong.

I assume WordPress does not add your WebP images to the post meta sizes array, right?

Yes, exactly... but i wrote this almost 9 months ago (for a customer with heavy pagespeed issues caused by png images) and in the meantime things have changed. My solution was to was to convert all images with "shadow" webp copies and serve them with nginx if they were available otherwise the original one was served.
Your solution was definitively a better idea and replacing the resized images with a webp is better, but i'd like at least to keep the original image in the original format for future use (for example a resize starting from an image that has not already been "degraded"). this was suggested also here

I'm curious how your approach works on the front end, do you replace all urls in the default img src/srcset with '.webp'?

You can see how it works in my blog: //modul-r.codekraft.it/... this is an heavily lazy-loaded page (i'm testing my script here).
Generally I add to the image name the suffix ".webp" and this allows me to check if there is that image, and in the case that there is not have a fallback. This happened more in the past because not all browsers can read the webp format so I needed the original version in case they were not accepted by the browser
In the past i've used nginx in this way
Recently i wrote a lazyload plugin for wordpress to replace images with the webp copy. (unreleased and experimental)

edit: added some info in the last point above

erikyo added a commit to erikyo/easylazy that referenced this pull request Jan 3, 2022
…s/performance#32 (comment))

To improve the optimization (where possible) I try to use the same compression quality that is used in the jpg so, if this has been compressed with a quality of 50% also the webp will have this quality
@erikyo
Copy link

erikyo commented Jan 4, 2022

hi @adamsilverstein
I tried your module which is really clean and cool and noticed two possible issues:

  • Only the resize images are converted to webp but not the full image. This is good but, in some cases, also the copy of the full size is required and needs to be converted and added to the Wordpress resize array.
    To reproduce this issue you must set the resize "large" and "medium" to a large size, eg. 1600, than upload an image of 1200 px width. Add that image to a post, then check the src and srcset: the only image converted will be the thumbnail and the src will show the jpg image.
    This also causes the jpg image to be loaded when the image you need is the larger.

  • The quality of the webp doesn't follow the jpg image quality while I think it should when it is possible. If I upload an image with a very low quality (e.g. 20%) the scaled webp image has a filesize larger than the original. Probably this is an irrelevant issue because the gain of webp in terms of weight is better in 99% of the cases, however you could still save some kb if the jpg quality is lower than 82. I have already searched if there is a method to determine the quality of jpgs and there isn't a real one, you can only estimate the quality. Imagemagick has this but it doesn't always work when exif data is missing.

@adamsilverstein
Copy link
Member Author

@erikyo good points. Since subsequent image regeneration will use the "full" image, we keep it in the original format. Good point about that being used/loaded by the browser. Maybe we can improve the markup > which theme are you testing with, and which browser?

The quality of the webp doesn't follow the jpg image quality while I think it should when it is possible

We are researching the best quality setting to use for WebP in #7 and are also discussing setting quality by mime type (and possibly size) which is not currently the case.

@erikyo
Copy link

erikyo commented Jan 19, 2022

which theme are you testing with, and which browser?

the theme was "Modul-r" (i am the author) and I use Chrome Canary (Version 99.0.4819.0 (Official Build) canary (64-bit))

We are researching the best quality setting to use for WebP

It is not essential but adapting the quality of the webp, from what I have been able to test, you can cut an additional 10-20% of the weight of the page depending on the size of the images (larger images = larger gain)

@adamsilverstein
Copy link
Member Author

thanks for the additional details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Feature A new feature within an existing module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create WebP module and port the applicable code
4 participants