apache / incubator-pagespeed-mod

Apache module for rewriting web pages to reduce latency and bandwidth.
http://modpagespeed.com
Apache License 2.0
696 stars 158 forks source link

Image Optimization: Does MPS optimize <source> images? #1876

Closed bastianschwarz closed 1 year ago

bastianschwarz commented 5 years ago

Hey everyone,

I did some tests and read the docs (and even digged through the code a litte) but I'm still not sure so I thought I ask for clarification:

Does MPS actually optimize images in

<picture>
  <source srcset="" />
</picture>

or does it just optimize images within <img srcset="" />?

Thanks in advance!

hit1205 commented 5 years ago

I have same question, found that only <img srcset> images were optimized by the mod, it seems does not optimize <source srcset> images.

Lofesa commented 5 years ago

Hi In the next release maybe you can configure pagespeed AddUrlValuedAttribute source srcset srcsetimage Is in a previous state as PR #1929

FalkoHilbert commented 4 years ago

hi @Lofesa , but the mentioned PR does not solve the problem. Do you have any information about when picture tags will be supported? I think the problem could be solved if someone marks the source-tag as an image, depending on the parent element , at line: https://github.com/apache/incubator-pagespeed-mod/blob/b3349ab11b9a6345f589fba244fcfbc11af69a5a/net/instaweb/rewriter/resource_tag_scanner.cc#L162

Lofesa commented 4 years ago

Hi @FalkoHilbert The PR is intended to solve the data-srcset issue, not to add the picture element. I have no tried, cause I don´t use picture elements in my site, but with

pagespeed AddUrlValuedAttribute source srcset srcsetimage
pagespeed AddUrlValuedAttribute source data-srcset srcsetimage
pagespeed AddUrlValuedAttribute source data-src image
pagespeed AddUrlValuedAttribute source src image

must work if you use these combinations if you only use picture, but if you use audio or video this can´t be used, cause it uses source src. As far as I know adding the picture element is not planned, people that works in the module are in other things. Any chance you make a proposal PR for this?

FalkoHilbert commented 4 years ago

Hi @Lofesa, I would like to support you here, however I cannot estimate the side-effects of those changes. I also lack the knowledge of unit testing in C++.

Basically to your answer: So if I do NOT use video or audio elements in a page, should the following statement work? pagespeed AddUrlValuedAttribute source src image

Lofesa commented 4 years ago

Hi @FalkoHilbert I have no tested it, but theoretically yes. The PR introduces a new category for srcset and data-srcset. The AddUrlValuedAttribute takes a html element (source) with an attribute (src) and treat it as an (image). If you have<picture> <source src=xxxxx > you can probe it w/o apply the PR cause src is a category stablished before the PR, only if you have srcset or data-srcset need the PR. I will say, now w/o any change to pagespeed you can try: pagespeed AddUrlValuedAttribute source src image. I will try to include the picture element to no need this hack if I have time and make a PR for that. Try this option and come here to comment if it works.

Lofesa commented 4 years ago

Hi @FalkoHilbert @bastianschwarz @hit1205 I have taken a look in to the picture spec and have some questions about the use cases. I´m not sure pagespeed need to optimize the picture element, or all most not in all the cases. I took a look at this doc. If picture element is used to ofert to the browser choose whetbeen webp or jpg format, whats is supposed that pagespeed can do? If transforming all images to webp, the picture element is useless. If only used for ofert different sizes in different viewports, why don´t use img srcset? Maybe the only use case that pagespeed can optimize is when different images are served for diferent sizes. I think is not easy to determine when pagespeed need to only resize images, or resize and convert to webp.... I´m not a developer and don´t know how hard to do this can become.

bastianschwarz commented 4 years ago

Example output:

<picture><!--[if IE 9]>
  <video style="display: none;"><![endif]-->
  <source srcset="[omitted]/images/[unique-image-id]/[seo-friendly-filename].jpeg" media="(min-width: 1800px)">
  <source srcset="[omitted]/images/[unique-image-id]/[seo-friendly-filename].jpeg" media="(min-width: 1500px)">
  <source srcset="[omitted]/images/[unique-image-id]/[seo-friendly-filename].jpeg" media="(min-width: 1200px)">
  <source srcset="[omitted]/images/[unique-image-id]/[seo-friendly-filename].jpeg" media="(min-width: 900px)">
  <source srcset="[omitted]/images/[unique-image-id]/[seo-friendly-filename].jpeg" media="(min-width: 600px)">
  <source srcset="[omitted]/images/[unique-image-id]/[seo-friendly-filename].jpeg" media="(min-width: 300px)">
  <source srcset="[omitted]/images/[unique-image-id]/[seo-friendly-filename].jpeg" media="(min-width: 0px)">
  <!--[if IE 9]></video><![endif]-->
  <img src="[omitted]/images/[unique-image-id]/[seo-friendly-filename].jpeg" srcset="[omitted]/images/[unique-image-id]/[seo-friendly-filename].jpeg" alt="..." title="..."/>
</picture>
<noscript>
  <img src="[omitted]/images/[unique-image-id]/[seo-friendly-filename].jpeg" alt="..." title="..."/>
</noscript>

We have implemented a component that creates a lot of different sizes based on a predefined combination of configurations, user settings and the image given in the backend (Includes cropping, masks, focal points). The user can make a lot of decisions for each individual image in our backend.

Because we gave the user that much choice, we want as much control over what images are displayed by the browser as possible. AFAIK scrset and sizes only give the browser hints as to what you want and the browser decides what it really wants to show.

The images are also only generated on the server if they are requested by the browser in that specific size, so we didn't want to implement a serverside recompression since we would duplicate what MPS already does.

What we want MPS to do (and which it does really well for "regular" img tags) is to recompress/change the format (if the browser supports it) of the "final" images. Or insert them as base64 into the source code if possible ... all the good stuff that already happens in a lot of places. Now, we just need it for the sources within the image. ;)

I havn't had the time yet to try your AddUrlValuedAttribute configuration. Maybe this already solves our usecase.

Lofesa commented 4 years ago

Have you enabled pagespeed in this site? Are the <img src= rewrited? both, the img element in picture and in noscript. Pagespeed identifies images with <img src In the source element, srcset are not true srcset, cause it have only 1 image, so maybe you can try the AddUrlValuedAttribute source srcset image; .