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

Ensure WebP/Image/Optimization plugins can integrate & use existing WebP images #160

Closed
Tracked by #22
adamsilverstein opened this issue Feb 10, 2022 · 25 comments
Closed
Tracked by #22
Assignees
Labels
Needs Review Anything that requires code review [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) [Type] Enhancement A suggestion for improvement of an existing feature

Comments

@adamsilverstein
Copy link
Member

adamsilverstein commented Feb 10, 2022

Feature Description

Existing WebP/Image plugins in the repository already support WebP, and many sites are already generating and using WebP sub sized images. In fact, around 13% of WordPress desktop pages in the httparchive dataset use WebP images!

Once core starts generating and outputting WebP images by default, it would be good to use the existing WebP images users have already generated on their sites. To enable this, we can give plugins a filter they can use to adjust the alternate images used, so when we generate a webp image tag and go to check that the image has the WebP sub sizes we need, plugins could return their own webp image data instead.

Original raised by Anton in slack.

Related to #22

Sub-issues:

Note: discussion happening in this doc

@adamsilverstein adamsilverstein added [Type] Enhancement A suggestion for improvement of an existing feature [Focus] Images [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) labels Feb 10, 2022
@adamsilverstein adamsilverstein added this to Backlog in [Focus] Images via automation Feb 10, 2022
@adamsilverstein adamsilverstein changed the title Ensure WebP/Image/Optimization plugins can integrate, use existing WebP images Feb 10, 2022
@mitogh mitogh self-assigned this Feb 15, 2022
@av3nger
Copy link

av3nger commented Feb 15, 2022

Just to add to the above. A good idea would be to try and unify the format of how WebP images are stored. Having multiple implementations of WebP across WordPress and plugins does not benefit anyone. However, if we were to all agree on a location and format for WebP images (adding support to core?), maybe, image optimization plugins could utilise that location and format as well. In that case, when the feature does make it to core, we won't need to worry about filters, locations and re-generating files. It would be as simple as populating the meta fields in the database. But, again, if this support were to make it to core before the actual implementation of generating and serving WebP, this could be simplified even further. Hope this makes sense :-)

As far as I know, at the moment, each plugin has its own implementation. Some will replace .jpeg images on the front-end with .webp alternatives, some use JavaScript fallback mechanisms. In Smush, for example, we store in wp-content/smush-webp/ folder, which mimics the wp-content/uploads/ location. Then image are served via server redirects. If there's an image uploads/2022/02/image.jpeg, the server will try to replace the image and headers to smush-webp/2022/02/image.jpeg.webp. The downside of this - sometimes there are conflicts with caching and a WebP image will be cached as a .jpeg file. Which then won't work on browsers that do not support WebP.

@eclarke1 eclarke1 moved this from Backlog to To do in [Focus] Images Feb 22, 2022
@gagan0123
Copy link

gagan0123 commented Mar 1, 2022

In fact, around 13% of WordPress desktop pages

Some of this can be attributed to the CDNs providing WebP support like Cloudflare, and user does not have to do anything to start serving WebP images.

@BlogAid
Copy link

BlogAid commented Mar 1, 2022

What happens if the site owner deletes their current image optimization plugin and wants to return to their original images?

FYI, some of these plugins serve images from the paid service's CDN, so they are not even on the site.

@gagan0123
Copy link

Adding to what @BlogAid said, we cannot trust the images generated by third party plugins, some plugins generate on the current site's server itself, some send the images to their own server to generate WebP images and then return the files back to the site for which we can't even know what compression settings they used, were the images even generated successfully or not, also some simply keep the files on their own server, using some sort of SAAS model.

So instead of that, we can generate our own WebP images and provide hooks/API for other plugins to modify that behavior to their needs.

@mitogh
Copy link
Member

mitogh commented Mar 1, 2022

What happens if the site owner deletes their current image optimization plugin and wants to return to their original images?

  • Original images are not removed, only additional images are generated instead.
  • A filter would be added in place to disable/change this behavior. Allow developers to tweak which image formats should be used #187
  • Plugin deactivation does not remove media files, only delete of the main attachment
  • FYI, some of these plugins serve images from the paid service's CDN, so they are not even on the site.

FYI, some of these plugins serve images from the paid service's CDN, so they are not even on the site.

Right, a filter would be in place in order to deactivate this behavior and use any existing plugin or just keep on serving JPEG instead. #174

Hooks would exists in place so plugins can indicate the presence of a WebP image or disable the flow to continue as today.

@bethanylang
Copy link
Contributor

👋 @mitogh Can you confirm that we have a planned approach here and are ready to begin development? If so, are there any dependencies to be aware of and what's the LOE for this work? Thanks!

@mitogh
Copy link
Member

mitogh commented Mar 4, 2022

For this one we are in the exploration phase still reviewing more plugins, the idea for this is based on the data collected from the plugins this week we can have a design proposal for how we can go about this one next week, once we have a design document we can discuss how we can go about the implementation detail and create tickets for that work, but at this point, the current of this stage is the work in progress to review and document the findings for existing plugins:

The document can be found in this link:

cc @bethanylang

@bethanylang bethanylang moved this from To do to In progress in [Focus] Images Mar 9, 2022
@bethanylang bethanylang moved this from In progress to To do in [Focus] Images Mar 9, 2022
@adamsilverstein
Copy link
Member Author

@mitogh The document you linked mostly inventories the plugins available, can you add a proposed approach there or here. What I'm thinking is:

  • a filter that plugins could use to provide their own WebP images for the front end, right when we are looking for them in the_content replacement. plus a dev note, example of using
  • a way for plugins to control image generation, probably just a dev note existing filter should cover this
@mitogh
Copy link
Member

mitogh commented Mar 10, 2022

Currently I'm working on this document and can be found at this link:

Still a work in progress but is mostly going towards what you are suggesting, so with the data from the first document, I'm working on this document to provide the next steps so we can discuss and then create actionable items.

Let me know in case you have any feedback on that one, still not complete but worth taking a look in case you have any concerns, questions.

Thanks @adamsilverstein

@bethanylang bethanylang moved this from To do to In progress in [Focus] Images Mar 16, 2022
@EdithAllison
Copy link

@mitogh Hi, a site I'm looking after is using Litespeed Cache as part of WP managed hosting by Krystal Hosting (UK). I noticed in your document you could not review it as it requires a key. Here's my feedback, happy to provide info:

Benefits

  • Plugin generates optimised compressed versions of each image and webP
  • it serves webP to compatible browsers, everyone else gets the original image format
  • optimisation runs reliably in the background in batches

Problems

  • for SEO scoring, Google sometimes doesn't get the webP version and the complains about missing "next gen image formats", otherwise it's been problemfree for us

Images are stored in the same folder as the original like so:
Bildschirmfoto 2022-03-29 um 09 26 02

Settings are explained here: https://docs.litespeedtech.com/lscache/lscwp/imageopt/#image-optimization-settings-tab

My main objective would to be to avoid duplication. The current system works very well for us, so if WP brings out its own webP generator I'd like to disable it.

@adamsilverstein
Copy link
Member Author

My main objective would to be to avoid duplication. The current system works very well for us, so if WP brings out its own webP generator I'd like to disable it.

@EdithAllison that makes sense to disable core's feature in favor of your own established pattern, you will be able to do that with the provided filters. One thing to consider would be in the future when users install your plugin who are already using the core feature and may have some WebP images already for some uploads. In that case it might make sense to leave the core handling intact for those images, or serve those images to users using your mechanism.

@mitogh
Copy link
Member

mitogh commented Mar 29, 2022

Hi, a site I'm looking after is using Litespeed Cache as part of WP managed hosting by Krystal Hosting (UK). I noticed in your document you could not review it as it requires a key. Here's my feedback, happy to provide info:

Thanks @EdithAllison I've updated the permissions of the document, feel free to take a look and provide some feedback.

@mmuyskens
Copy link

you will be able to do that with the provided filters

Seriously?

You'd like to force WebP on new installs, fine. But let's not force it on existing installs with a stupid filter to turn it off. HELLO! WordPress already HAS an admin area, what's wrong with having an actual toggle in the UI to turn it ON.

RIP to any user that has a hosting account thats based on inode count.

@mitogh
Copy link
Member

mitogh commented Mar 30, 2022

Seriously?

Correct, filters are in place to allow customizations and can be used from within the themes or plugins to perform those actions.

You'd like to force WebP on new installs, fine. But let's not force it on existing installs
with a stupid filter to turn it off. HELLO! WordPress already HAS an admin area, what's wrong with having an actual toggle in the UI to turn it ON.

This comes from the philosophical principles from WordPress, "Decisions, not Options" - https://wordpress.org/about/philosophy/

Agree that a UI would be great, at this time tho the main idea of this plugin is to explore and test if you feel like a UI would be more beneficial please do let us know as all points of view are important and are taken into account when the core for the patch is created.

As I mentioned before this is a plugin for test and experiment scenarios and some of them might not be ideal at this stage but the intention is to make the most beneficial solution at the end of the day.

On the other hand, I would advise to review the Code of Conduct when providing feedback. Thanks for taking the time to give us your point of view and let us know in case you have any additional feedback.

@jb510
Copy link

jb510 commented Apr 5, 2022

I think I understand the altruistic intent of this specific issue. It would be nice to reuse already generated webp files, but this seems fraught with long-term issues.

A different approach would be to ensure users have a way to remove the old webp images and adopt whatever new format core implements. This inturn really seems to be plugin territory where the existing webp generating plugins out to handle migration their own existing webp files into whatever structure core supports.

The benefit to this would be that disabling that webp plugin and whatever "extra" features it provides would cleanly revert back to core functionality supporting webp.

For example files in /smush-webp/ don't respect define('UPLOADS', ''.'my-uploads');. So they're not compatible with plugins that offload images to services like S3, or a different drive, etc. Getting plugins to do things in a way that is most compataibly with core is a long term benefit.

Finally, presuming webp generation becomes opt-in on existing sites (maybe?), that also solves a large number of plugin compatibility issues. Existing webp generation plugins can check if core's webp generation flog is set to on or off and behave, or migrate, accordingly. Pushing this migration responsibility to the plugins where it belongs. Core needing to repurpose/move/leverage/accommodate existing webp assets is really only an issue if webp generation is turned on by default on old sites that have existing webp images.

@mitogh
Copy link
Member

mitogh commented Apr 7, 2022

Thanks for your feedback @jb510

Agree with you on your points, however, I think the main concern relies on backward compatibility, to allow existing sites to keep on using the system of their preference in case the WebP versions of the images already exist in their WordPress installation.

This system would allow external plugins to give the reference of those images back to WordPress so WordPress stores the information of the existing WebP images, in addition to the previous point, this gives the opportunity for other plugins like S3 to identify additional sources (images) associated with a particular attachment with it can be translated in external plugins being able to upload additional sources even if located in a different path.

@jb510
Copy link

jb510 commented Apr 12, 2022

Backward compatibility with plugins just really seems to me like it ought to be plugin territory. Most of those plugins are well maintained and ought to have to problem shifting THEIR structures to match core. I don't think they should continue using a "alternative" structure long term just because that's what a plugin imposed.

I do think I follow what you're saying though, that media ought to be able to point it's meta data for each of it's thumbnails to any path and keep track of that path, even if that path was set by a plugin pre-webp in core.

I can't really think of any cases where core went out of its way to accommodate plugin behavior like is being proposed here. WooMenus maybe? Just seems needless engineering effort and hassle that ought to (again) be pushed back to plugins.

@mitogh
Copy link
Member

mitogh commented Apr 13, 2022

Backward compatibility with plugins just really seems to me like it ought to be plugin territory. Most of those plugins are well maintained and ought to have to problem shifting THEIR structures to match core. I don't think they should con tinue using a "alternative" structure long term just because that's what a plugin imposed.

Agree with you here, the aim of the filters would be to enable it but is up to the plugins to either migrate to the new structure and take advantage of this structure or support his own structure, which means they need to provide support using the newly introduced filters.

I do think I follow what you're saying though, that media ought to be able to point it's meta data for each of it's thumbnails to any path and keep track of that path, even if that path was set by a plugin pre-webp in core.

Each image on the media library has metadata stored in the database this metadata is used to keep track of all the references to sub sizes created by WordPress, currently no WebP plugin stores any references to those images back into the database, by storing this information into the database we allow for additional things like being able to remove all images when the main image is removed or detect which image format is more performant.

I can't really think of any cases where core went out of its way to accommodate plugin behavior like is being proposed here. WooMenus maybe? Just seems needless engineering effort and hassle that ought to (again) be pushed back to plugins.

Agree this is not mostly to accommodate for specific plugins but rather to let them replace the logic provided by this plugin in case they want to use a different engine, similar to how core allows for developers to change the default image editor.

@BlogAid
Copy link

BlogAid commented Apr 13, 2022

@mitogh Thank you for this info about the meta being stored in the database.

That must be how the Enable Media Replace plugin works. It offers several choices with file name, path, and date options. It does not have a database table of its own, and does no redirects.

One of the choices is to simply replace the image with a new one and retain the original file name, date, and upload location. You do have to replace it with a newly uploaded one, which generates the new thumbnails. It then deletes the original image and all of its thumbnails.

It must be using the meta that is already stored in the database to do all of that.

With plugins like ShortPixel and EWWW, they do have their own database tables, as well as subfolders for storing the original image files. I have not poked around their database tables to see what info they are storing, though.

@mitogh
Copy link
Member

mitogh commented Apr 14, 2022

Thanks @BlogAid yes most plugins usually have their own tables to store information about additional images.

This approach expand the existing metadata of the subsizes to register where any additional subsize is located and how big it is (meaning the file size) per mime type.

@felixarntz
Copy link
Member

@mitogh Following up on your comment in #318 (review):

While my first reaction in #318 (review) was to implement this indeed, I'm now wondering whether we really need it. My thinking is the following:

  • The filesize is also part of the filter, so WordPress core wouldn't need to get the file size from what the filter returns, the filter should be responsible for determining the file size. So from that perspective, no need to return a path.
  • WordPress stores the file value under the sources, and you're right that we wouldn't be able to do anything with that if it's not relative to the regular WP uploads directory. However, we also have the parallel filter to replace the sources in the frontend, which is especially there to support such scenarios where extra logic is needed, e.g. in case the files are not within the WP uploads directory but in let's say a CDN.

So my take here is that we should now already have everything necessary for plugins to integrate, and I'd say we can close this issue. Curious to hear your thoughts.

@mitogh
Copy link
Member

mitogh commented May 13, 2022

Thanks for the questions @felixarntz

I think the problem relies on the fact that the filter is an option for third party developers to provide an image to be used as a reference instead of relying on WordPress mechanism to create one, this works fine if the images and logic lives in the same locations as the WordPress installation however the current logic of the filter assumes this is the case by using filesize.

I think the simplest approach would be to switch to wp_filesize which is a filterable function that would allow to determine the filesize externally.

I think the only "problem" is the fact that the size would be 0 for external images.

@felixarntz
Copy link
Member

Thanks @mitogh, my thinking here was off, you're completely right.

Regarding the approach:

I think the simplest approach would be to switch to wp_filesize which is a filterable function that would allow to determine the filesize externally.

That would be a bit complicated because it would require to use yet another filter for this (plus wp_filesize() was just recently introduced so may not be available in core yet). Also, for CDNs you may not be able to even return a path, since the only meaningful thing there would maybe be an external URL.

I think a simpler solution would be to allow the existing filter to specify either a path or the filesize directly. The path is only needed to get the filesize anyway, so if the filesize is returned from the filter, it wouldn't need to return the path. I've opened #333 to implement this enhancement.

@mitogh
Copy link
Member

mitogh commented May 14, 2022

Thanks I agree that behavior would work best, thanks @felixarntz

@felixarntz
Copy link
Member

With #333 completed, this overarching issue is now complete as well. 🎉

[Focus] Images automation moved this from Review to Done Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review Anything that requires code review [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) [Type] Enhancement A suggestion for improvement of an existing feature