-
Notifications
You must be signed in to change notification settings - Fork 101
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
Convert uploaded PNG files to AVIF or WebP #1421
Convert uploaded PNG files to AVIF or WebP #1421
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @jzern, @nikolasholm, @ficod, @predaytor, @benoitfouc, @bolach, @youri--. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
note: the broken tests are because I changed the default transforms array; I will fix that. |
…ter_returns_non_array_type test
Fixed in 358e1bf |
For non-photographic content, lossless will often be smaller. If you decide to |
Its hard to know if an image is photographic or not - PNG might be used just to get the transparency for a product image for example. I could add documentation, or we could add a "Use lossless output for PNG uploads" option that would trigger WebP lossless output when PNGs are uploaded. |
True, detecting the content is involved and error prone. You could assume that since it's a PNG the author intended it to be lossless, but that too, isn't always the case. |
The PNG in this case should be the fallback, correct? |
Right, and maybe the checkbox language should be updated to something like: Also generate fallback images in original upload format Then, when uploading PNGs, if it is checked we would generate both avif or webp versions, as well as the PNG versions. |
For now, I'm going to try to add documentation explaining how to control lossless output with filters and set PNGs to lossless. Side note: with uploaded WebPs we detect if the upload is a lossless format and if so, use lossless output. |
When testing fallbacks I noticed that without the fallback enabled, all sizes are generated for a large upload: However, with the fallback enabled, the two largest sizes (which are not technically part of the original core default and are added by filter I believe) do not get We may be working on this elsewhere, cc: @mukeshpanchal27 were you looking at this issue already? |
I did some manual testing and verified things are working well here (other than the missing largest images noted above):
I tested with a 800x600 PNG with transparency and got the following results:
|
@adamsilverstein For the additional mime type plugin only accounts for allowed sizes given in https://github.com/WordPress/performance/blob/trunk/plugins/webp-uploads/hooks.php#L708-L732 function and add_filter( 'webp_uploads_image_sizes_with_additional_mime_type_support', static function ( $allowed_sizes ): array {
$allowed_sizes['1536x1536'] = true;
$allowed_sizes['2048x2048'] = true;
return $allowed_sizes;
}); |
Ok, thanks for clarifying @mukeshpanchal27. I understand these sizes are different than the core default sizes since they were added later and using a filter so they can be unhooked - still, I feel like they should be part of the default images we create in the plugin? Did we decide not to add them? I would say that unless another plugin has explicitly removed the core default hook ( |
# Conflicts: # plugins/webp-uploads/picture-element.php
# Conflicts: # plugins/webp-uploads/settings.php
Patch refreshed after other PRs were merged. Ready for review/merge. |
@adamsilverstein In PR #415, we added a control for which image sizes to generate additional MIME type versions. The feedback #415 (comment) suggests removing those sizes:
@felixarntz, is there any specific reason for this suggestion? |
Thanks for the additional context @mukeshpanchal27 - it looks like in that comment the assumption is we will call In short, we need to ensure that the same set of images is generated for AVIF/WebP as would be generated for JPEG. We should generate ALL of the same sizes, including everything added via |
@mukeshpanchal27 @adamsilverstein
The currently expected behavior is that sizes for the additional MIME type are only generated if opted in. This decision was mostly made in regards to file size / storage concerns. Many WordPress sites have 10 or more, sometimes very specific image sizes, and it shouldn't be necessary to generate the additional MIME type for each of them. But we don't know in which context any additional image size is used, so therefore this is opt-in. If we were to land this in core, this would be an additional parameter on Keep in mind that this only applies if the option for fallback images is enabled. If the primary MIME type is transformed, it'll always generate all image sizes because in this scenario there's no additional storage concern compared to what would regularly happen. That's why More specifically though, I think the feedback that the two specific core sizes WordPress core adds those image sizes via Let's do that via a separate issue though. |
Ok, that makes more sense.
+1 - the savings could be significant when these images are served.
I still feel like since storage is plentiful file size concerns should be less of an issue with the feature plugin - so make it the other way and users should have to opt out of generating theme sizes. My expectation as a site owner installing this plugin is that picture element would "just work" if I want AVIF with JPEG fallbacks, I would want all sizes to make it the most effective. Maybe we could add a checkbox option and/or cover any image sizes added by core themes? |
I think the file size concerns don't really differ based on whether this is core or not, so I'd prefer to stick with the opt-in way, as it's safer. That said, providing a checkbox option to end users to effectively auto opt-in all sizes could be a good solution. It keeps it opt-in, but makes it more trivial and allows for an end-user to make the decision. (The checkbox would only be relevant if the other checkbox for fallback images is enabled.) This could be implemented in a way that it only applies to image sizes that don't express a preference too, so that programmatic control is still possible, as in: If an image size has explicitly specified I suggest we decouple those two things in two separate issues, one to automatically opt in the special core sizes |
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
I will create these follow up issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I've left two non-blocking feedback points. Pre-approved.
Summary
Maps PNG uploads to output with the current modern image format which defaults to AVIF when supported by the server and falls back to WebP (which is supported 98%+ of servers).
Note: this does not change the image quality, so the output images will be lossy images. I did consider explicitly using lossless output for these images (by setting quality to 100), I wasn't sure how this might impact file size savings and compression times.
Also note that animated output is not supported in PHP currently, so animated PNGs may not work as expected.
This could use more testing to see how these PNG image types are handled for both WebP and AVIF output. Do the created images look right? Is transparency maintained? How does the filesize compare?
Also: I didn't test the "Also generate JPEGs" or picture element features, how should those work with PNG images?
Fixes #371
Relevant technical choices
Could probably use some tests as well.