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

Convert uploaded PNG files to AVIF or WebP #1421

Merged
merged 22 commits into from
Sep 4, 2024

Conversation

adamsilverstein
Copy link
Member

@adamsilverstein adamsilverstein commented Jul 31, 2024

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?

  • Transparent images
  • Animated images
  • 8 bit/24 bit images

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

  • Mapping PNG to the the current output format using the primary filter

Could probably use some tests as well.

@adamsilverstein adamsilverstein added [Type] Enhancement A suggestion for improvement of an existing feature no milestone PRs that do not have a defined milestone for release labels Jul 31, 2024
Copy link

github-actions bot commented Jul 31, 2024

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 props-bot label.

Unlinked Accounts

The 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.

Unlinked contributors: jzern, nikolasholm, ficod, predaytor, benoitfouc, bolach, youri--.

Co-authored-by: adamsilverstein <[email protected]>
Co-authored-by: felixarntz <[email protected]>
Co-authored-by: mukeshpanchal27 <[email protected]>
Co-authored-by: westonruter <[email protected]>
Co-authored-by: erikyo <[email protected]>
Co-authored-by: maisondasilva <[email protected]>
Co-authored-by: phanduynam <[email protected]>
Co-authored-by: y-guyon <[email protected]>
Co-authored-by: mxbclang <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@adamsilverstein
Copy link
Member Author

note: the broken tests are because I changed the default transforms array; I will fix that.

@adamsilverstein
Copy link
Member Author

adamsilverstein commented Jul 31, 2024

note: the broken tests are because I changed the default transforms array; I will fix that.

Fixed in 358e1bf

@jzern
Copy link

jzern commented Jul 31, 2024

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.

For non-photographic content, lossless will often be smaller. If you decide to
try this, you should only do it for WebP. AVIF lossless is less efficient than
WebP.

@adamsilverstein
Copy link
Member Author

For non-photographic content, lossless will often be smaller.

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.

@jzern
Copy link

jzern commented Jul 31, 2024

For non-photographic content, lossless will often be smaller.

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.

@westonruter
Copy link
Member

Also: I didn't test the "Also generate JPEGs" or picture element features, how should those work with PNG images?

The PNG in this case should be the fallback, correct?

@adamsilverstein
Copy link
Member Author

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.

@adamsilverstein
Copy link
Member Author

For non-photographic content, lossless will often be smaller.

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.

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.

@adamsilverstein
Copy link
Member Author

I updated the language around the checkbox (and under picture element) making it clear that the checkbox enables generating fallback images in the original upload format. Option, filter and function names remain unchanged for backwards compatibility.

Here is the settings screen after the changes:

image

@adamsilverstein
Copy link
Member Author

When testing fallbacks I noticed that without the fallback enabled, all sizes are generated for a large upload:

image

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 avif versions:

image

We may be working on this elsewhere, cc: @mukeshpanchal27 were you looking at this issue already?

@adamsilverstein
Copy link
Member Author

adamsilverstein commented Aug 8, 2024

I did some manual testing and verified things are working well here (other than the missing largest images noted above):

  • Output images (both WebP and AVIF) maintain transparency
  • Output images generally much smaller than png versions.

I tested with a 800x600 PNG with transparency and got the following results:

  • 768x576 image:
    • PNG: 225 KB
    • WebP: 61 KB (approximately 3.7x smaller than PNG)
    • AVIF: 13 KB (approximately 17x smaller than PNG)
  • 150x150 image:
    • PNG: 28 KB
    • WebP: 10 KB (approximately 2.8x smaller than PNG)
    • AVIF: 4 KB (approximately 7x smaller than PNG)
  • 300x225 image:
    • PNG: 54 KB
    • WebP: 18 KB (approximately 3x smaller than PNG)
    • AVIF: 6 KB (approximately 9x smaller than PNG)
  • Original image (800x600):
    • PNG: 227 KB
    • WebP: 70k (approximately 3x smaller than PNG)
    • AVIF: 14 KB (approximately 16x smaller than PNG)

@mukeshpanchal27
Copy link
Member

@mukeshpanchal27 were you looking at this issue already?

@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 1536x1536 and 2048x2048 additional sizes that should added through the filter in core. If anyone wants those sizes then we can enable it through below filter code.

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;
});

@adamsilverstein
Copy link
Member Author

@mukeshpanchal27 were you looking at this issue already?

@adamsilverstein For the additional mime type plugin only accounts for allowed sizes given in trunk/plugins/webp-uploads/hooks.php#L708-L732 function and 1536x1536 and 2048x2048 additional sizes that should added through the filter in core. If anyone wants those sizes then we can enable it through below filter code.

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 (add_action( 'plugins_loaded', '_wp_add_additional_image_sizes', 0 )) we should add these sizes by default (maybe with the filter?) - what do you think?

# Conflicts:
#	plugins/webp-uploads/picture-element.php
@adamsilverstein
Copy link
Member Author

Patch refreshed after other PRs were merged. Ready for review/merge.

@mukeshpanchal27
Copy link
Member

Did we decide not to add them?

@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:

For the additional core sizes 1536x1536 and 2048x2048, they also are added via add_image_size so the default here would be automatically overwritten by the following foreach loop anyway. So let's remove these as well. We should later pass the parameter as true there from within core.

@felixarntz, is there any specific reason for this suggestion?

@adamsilverstein
Copy link
Member Author

adamsilverstein commented Aug 28, 2024

Did we decide not to add them?

@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:

For the additional core sizes 1536x1536 and 2048x2048, they also are added via add_image_size so the default here would be automatically overwritten by the following foreach loop anyway. So let's remove these as well. We should later pass the parameter as true there from within core.

@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 wp_get_additional_image_sizes at some point to include the additional sizes that are added by core as well as any additional sizes added by theme (or plugin) code. I'm not sore that is happening though?

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 add_image_size. I believe this isn't working as expected so I will open a follow up issue to investigate.

@felixarntz
Copy link
Member

felixarntz commented Aug 28, 2024

@mukeshpanchal27 @adamsilverstein

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 add_image_size. I believe this isn't working as expected so I will open a follow up issue to investigate.

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 add_image_size(). I think this overall approach still makes sense.

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 webp_uploads_get_image_sizes_additional_mime_type_support() uses wp_get_additional_image_sizes(), which includes all sizes other than the 5 hard-coded ones in the function, including core's own 1536x1536 and 2048x2048.

More specifically though, I think the feedback that the two specific core sizes 1536x1536 and 2048x2048 should also be generated makes sense, since those are general sizes. So I think we could opt them in to the behavior via our own API surface. Here's how that would work, demonstrated by a current test: https://github.com/WordPress/performance/blob/3.4.0/plugins/webp-uploads/tests/test-load.php#L906

WordPress core adds those image sizes via add_action( 'plugins_loaded', '_wp_add_additional_image_sizes', 0 ), so we could add a hook right after that which sets the configuration value to opt in.

Let's do that via a separate issue though.

@adamsilverstein
Copy link
Member Author

Keep in mind that this only applies if the option for fallback images is enabled.

Ok, that makes more sense.

More specifically though, I think the feedback that the two specific core sizes 1536x1536 and 2048x2048 should also be generated makes sense

+1 - the savings could be significant when these images are served.

This decision was mostly made in regards to file size / storage concerns.... 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 add_image_size(). I think this overall approach still makes sense.

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?

@felixarntz
Copy link
Member

felixarntz commented Aug 28, 2024

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 true (opt-in) or false (opt-out), always respect that preference. But if an image size has not specified any preference (which is by far the majority of course), then we would follow the checkbox option.

I suggest we decouple those two things in two separate issues, one to automatically opt in the special core sizes 1536x1536 and 2048x2048, and another to allow end users to opt-in to generate all sizes in the fallback format as well.

@adamsilverstein
Copy link
Member Author

I suggest we decouple those two things in two separate issues, one to automatically opt in the special core sizes 1536x1536 and 2048x2048, and another to allow end users to opt-in to generate all sizes in the fallback format as well.

I will create these follow up issues.

@adamsilverstein
Copy link
Member Author

Follow up issues:

#1512
#1513

@felixarntz felixarntz added this to the webp-uploads n.e.x.t milestone Aug 30, 2024
@felixarntz felixarntz removed the no milestone PRs that do not have a defined milestone for release label Aug 30, 2024
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a 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.

plugins/webp-uploads/helper.php Show resolved Hide resolved
plugins/webp-uploads/helper.php Show resolved Hide resolved
@adamsilverstein adamsilverstein merged commit 9d93a06 into WordPress:trunk Sep 4, 2024
13 checks passed
@adamsilverstein adamsilverstein deleted the add/png-conversion branch September 4, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert PNG to AVIF/WebP on upload
5 participants