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

When uploading, generate both WebP and jpeg format images by default #142

Closed
Tracked by #22
adamsilverstein opened this issue Feb 1, 2022 · 17 comments
Closed
Tracked by #22
Assignees
Labels
[Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) [Type] Enhancement A suggestion for improvement of an existing feature
Milestone

Comments

@adamsilverstein
Copy link
Member

adamsilverstein commented Feb 1, 2022

Feature Description

Based on the decision in #96 (comment) we need to work on enabling generating both formats by default.

Specifically we need to address:

1. When the user uploads a JPEG image, we generate the smaller image sizes in both JPEG and WebP by default.
2. When inserting an image into a post or including an image in the frontend through any other means, we use the WebP version whenever it is available.
3. Developers can opt out of generating JPEG or WebP, e.g. if they are certain they won't need one of these versions.

  1. Generate both formats
  • We can hook on wp_generate_attachment_metadata to generate the additional mime type images (some example code from @erikyo).
  • If the user uploads a jpeg and generates jpeg sub sizes, we would hook in and also generate WebP sizes. If the user uploads a WebP core will generate WebP sub sizes, we would hook in and generate jpeg sub sizes
  • The behavior should be filterable, I propose a new filter named image_editor_output_formats which would return an array of mime types. By default this would return ['image/webp', 'image/jpeg'].
    • The first/default mime type would be used for the default sub-sizes generated by default and stored in the sizes array (If only one mime type is given, only that type is generated).
    • When more than one mime type is returned, the second and subsequent types are used to generate additional sub sizes and their data stored in the additional_sizes array...

1b. In addition to generating the new images, we also need to store meta data for new mime type sizes

  • for backwards compatibility reasons, I suggest we store a single mime type in the sizes array
  • additional mime type sizes we generate should be stored separately, in a key I propose naming additional_sizes
  • mime type data will go into a sources array inside each size
  • additional handling will be required for these additional sizes:
    • when the original image is deleted, these sub sizes should also be deleted
    • when missing sub sizes are regenerated, this should include all mime types
    • when the image is cropped/rotated/flipped in the media library, the action should apply to these images as well
    • eventually, functions should be extended or added that leverage these new sizes to generate a picture element
  1. Use WebP on front end
  • This can be achieved by having the default sizes sub sizes be WebP by default, which will be the case with ['image/webp', 'image/jpeg'] as the default value for image_editor_output_formats
  • We should also consider adding a mime type argument or a filter for functions like get_image_tag or wp_get_attachment_image_src as a more direct way for developers to specify the mime type they want to use.
  1. Developers can opt out
  • Adding add_filter( 'image_editor_output_formats', '__return_false' ); would ensure only the uploaded mime type would be use for sub sizes (current WordPress behavior).
  • Adding add_filter( 'image_editor_output_formats', function() { return array( 'image/jpeg', 'image/webp' ); } ); would ensure jpeg and WebP were generated, with jpeg stored in sizes and used as the default output format
  • Adding add_filter( 'image_editor_output_formats', function() { return array( 'image/webp' ); } ); would ensure only WebP sub sizes were generated (current behavior of the webp-uploads module).
  • The filter will reference the original image for context, so output formats could be dynamic

Feedback welcome as well as contributions, please let us know what you plan to work on by leaving a comment.

@adamsilverstein adamsilverstein added [Type] Enhancement A suggestion for improvement of an existing feature [Focus] Images Needs Dev Anything that requires development (e.g. a pull request) [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) Needs Discussion Anything that needs a discussion/agreement labels Feb 1, 2022
@adamsilverstein
Copy link
Member Author

One important note here: based on #75 (comment) we need to ensure that both the image src and srcset use the mime type we want to display to ensure that mime type is actually loaded (in some cases, the browser might load the src image). this suggests the possibility of a filter on the output side as well.

@adamsilverstein
Copy link
Member Author

I'm going to work on a PR for the generating images part.

@eclarke1 eclarke1 removed the Needs Dev Anything that requires development (e.g. a pull request) label Feb 2, 2022
@adamsilverstein
Copy link
Member Author

adamsilverstein commented Feb 2, 2022

Good suggestion from @nosilver4u on slack:
we should generate a full sized image in the output mime type (if it doesn't match the original image mime type).

For example, if you uploaded 'file.jpeg', for images below big_image_size_threshold we would store a 'file-webp.webp' version of the full sized image; for larger than big_image_size_threshold files where WordPress is creating a '-scaled' version, we should also create a -webp version of the scaled image. These full sized images could then be used in place of the original or -scaled images for the img src tag.

@nosilver4u
Copy link

With #143 in mind, if folks use this: add_filter( 'image_editor_output_formats', function() { return array( 'image/webp' ); } );
Then it seems the full-size should be stored in the WebP format whether it's above big_image_size_threshold or not.
Does that potentially cause issues where images over 2560 have -scaled, and those under 2560 have -webp in the filename? Or is it irrelevant so long as the proper path/filename is stored in the attachment metadata?

@adamsilverstein
Copy link
Member Author

Then it seems the full-size should be stored in the WebP format whether it's above big_image_size_threshold or not.
we would probably store both versions in this case, using the WebP version for the src attribute of the image tag (matching the srcset).

in spots like the media library or certain functions, we may still want to treat the file matching the uploaded mime type as the "original" file. so even though there is a full size WebP image, if you go to the media editor, it will use the full size jpeg image (assuming you start with a jpeg upload... that would be the original or a file with -scaled appended).

mitogh added a commit that referenced this issue Feb 3, 2022
The property is added to every single image size available,
each value inside the properties array is a reference to
a different image mime type.

If the image size was created as a JPEG version a WebP version
would be created, if a WebP version exists a JPEG version
would be created instead based on the details from ticket #142

This mechanism would allow extending the sources properties for
more additional mime types.

The value for each mime type is an associative array with `file` as
the only property for now. And the plan is to include more meaningful
properties such as `filesize` to a different mime type.
@mitogh
Copy link
Member

mitogh commented Feb 3, 2022

Created a draft version for a sources property approach @adamsilverstein

@mitogh
Copy link
Member

mitogh commented Feb 3, 2022

Adding a link with different alternatives in how the different image mime types should be stored in the database entry for each attachment:

Add your comments, suggestions, and feedback.

Thanks.

@adamsilverstein
Copy link
Member Author

Hey @mitogh - thank you for working on this! I like the way the new approach puts the image data right inside the existing sizes array.

I still wonder about the idea of storing the data right in the sizes array alongside the original data, the big benefit is all internal functionality would work as is. The disadvantage is we would probably break things :)

Noting a few places we will need to extend core to use the data from sources:

  • When generating and getting missing sub sizes (if all sizes can't be created with the initial request) - wp_update_image_subsizes is filtered with wp_get_missing_image_subsizes which is used in wp_get_missing_image_subsizes
  • When deleting images (delete the additional mime type files)

What other places will we need to integrate with this new data? What other core functions are used to get all the image sub-sizes, or do we need a new one?

Created a draft version for a sources property approach @adamsilverstein
Nice! I started working on code for generating the additional images based on the image_editor_output_formats idea in this WIP: #151 - I didn't really look at meta there yet, so we can integrate the approaches.

I'm going to create follow up tasks for deleting and regenerating missing images based on the new data we will store

@mitogh
Copy link
Member

mitogh commented Feb 4, 2022

Thanks, @adamsilverstein based on your comments I've decided to explore some other alternatives and end up with a new approach that is a mix of the described approaches but is basically storing the JPEG versions in _wp_attachment_backup_sizes instead, in the same WordPress stores references to previous edits on an image when using the editor for WordPress.

And because the JPEG is actually backup of the WebP versions this seems like a more solid solution due to this already solves the problem of removal of images due during the removal of an attachment all the backup sizes are removed as well, additionally to this when using this property, third-party plugins like S3 would upload all the backup sizes as well so those would be available.

And because the only usage of the JPEG version is via custom places, it seems like a more sensible approach. I'm creating a new PR with these changes for you to take a look at, so you can provide some feedback.

@mitogh
Copy link
Member

mitogh commented Feb 4, 2022

Adding an update here with the implementation:

I'm adding the findings in this document https://docs.google.com/document/d/1nVFhMXacydGce1ZjSvh14MQeKPS26TcQDeIPw0vmuig/edit?usp=sharing to make sure this approach is documented.

@mitogh
Copy link
Member

mitogh commented Feb 8, 2022

Thanks @adamsilverstein , I'm using the recommended hook wp_generate_attachment_metadata:

However, there might be other cases that we need to cover like when the image is rotated or cropped should we need to support the same backup mechanism for this type of edit. And this hook is not fired in those scenarios so an update would be required to the PR above to take into account those type of modifications to the original image from within the WordPress editor.

I'm opening a new issue for this describing the behavior so we can discuss the alternatives here but replicating what core does for the original image on additional mime types seems like the right path here.

@adamsilverstein
Copy link
Member Author

other cases that we need to cover like when the image is rotated or cropped

Right - we will probably need to handle these explicitly.

@adamsilverstein
Copy link
Member Author

Quick update on this issue: I have been researching approaches to async image generation and digging into the core cropping behavior.

  • We will want to create new functions to generate the additional mime types, core's existing endpoints for cropping or regenerating missing images are good starting points but are too specific to work for our use case. We'll probably need new callbacks as well.
  • We need to carefully test every function that uses _wp_attachment_backup_sizes if we wind up using that approach. It is used in wp_crop_image and also wp_restore_image and a few other places. I reviewed these and they seem to stick to expecting a specific suffix so should be ok
  • In the media library we can extend wp.Uploader to fire a callback shortly after the initial callback completes, this allows us to pass the attachment id to the callback as well. example https://gist.github.com/adamsilverstein/9b384db1ff9f596e465712cdb6d017d3 - code could include retries similar to core missing mages regeneration on errors. we will have to test if this works in Gutenberg or add an alternate implementation there
  • Alternately we could flag images that need mime type generation with a meta field, then look for those images in the heartbeat callback, but that might be a heavier approach

@eclarke1 eclarke1 added Needs Review and removed Needs Discussion Anything that needs a discussion/agreement labels Feb 15, 2022
mitogh added a commit that referenced this issue Feb 16, 2022
The logic to handle the case for image edits like: rotation, flip
and crop would be handled in a separate PR.

This PR addresses the concern for #142 and #150.
@felixarntz felixarntz mentioned this issue Feb 18, 2022
5 tasks
@mitogh
Copy link
Member

mitogh commented Feb 22, 2022

Ready for review:

Please take a look at the code above and provide any feedback on the code or the feature itself, give it a try locally and let us know in case something is not working as expected.

@felixarntz
Copy link
Member

Reopening because support for the retry mechanism still has to be added (already underway in #188).

@felixarntz felixarntz reopened this Feb 24, 2022
@felixarntz
Copy link
Member

Fixed via #147 and #188 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants