`->srcset()` generates wrong `w` descriptor (when not upscaled)

[seb-celinedesign]

Kirby Team summary

Status quo

->srcset() lists wrong w descriptors if requested size is smaller than original (as it doesn't upscale but the w descriptor is used as if it were).

Solution

• [ ] https://github.com/getkirby/kirby/issues/3548
• [ ] Change file dimensions to actual result of action (e.g. if image wasn't upscaled, it needs to have the smaller dimensions)
• [ ] Use actual file/thumb dimensions for srcset __w desciptor
• [ ] Remove duplicate width entries from srcset, e.g. if from 3 requested widths only one width could be created (due to no upscaling), only use one entry for srcset

Original post from @seb-celinedesign

I've an image, it's original width is 360px. Then the following code : $image->srcset([760]) will produce html code : srcset="/medias/pages/…/-760x.jpg 760w" But the /medias/pages/…/-760x.jpg image is in fact 360px large, not 760px large as specified by the descriptor. The resize isn't enlarging image (hopefully). Maybe it's minor since browsers still download an optimized images (360px); In fact all my srcset images will be 360px large. But maybe there are other edge case that will occur with those wrong generated descriptors width.

[moritzebeling]

Images are not being enlarged, the -760x.jpg file will most likely still have 360px in width. When Thumbs are requested in a bigger size, than what the original image provides, a thumbnail file is still generated and placed in the media folder (which is perfectly good).

What you describe, sounds exactly like the expected result: Calling a srcset of [760] returns exactly that (where in fact the image only pretends to be 760 but technically is only 360). But I think it’s not a bug if one calls a bigger image than there is. Try uploading bigger images or adjust your srcset , e.g. like [180,240,360].

[lukasbestle]

Well, but the browser will think that that image version is 760px wide when it isn't. So while the filename itself is fine, it should correctly generate the markup to use the actual thumb width.

[seb-celinedesign]

@distantnative This issue turn out to be severe in certain case. If we consider my initial example above, on large screen or HDPI screen a 360px wide image will be displayed where my layout was expecting a 760px image. For me, this issue make the srcset method completely unreliable. I think you should reconsider this issue priority. Thanks.

[bastianallgeier]

@CelineDesign what would be your expected result?

[seb-celinedesign]

Don't cheat to the browser in the width descriptor : srcset="/medias/pages/…/-760x.jpg 360w"

[bastianallgeier]

I'm trying to translate that to pseudo-code:

1. use the specified width (760 in that case) when the image is larger or as large
2. use the actual width of the image (360 in that case) if it is smaller.

Correct?

[seb-celinedesign]

Why not put (in all cases) the real generated image width in the w descriptor ?

[seb-celinedesign]

Just to make it clear, i've just created an example of the issue : you can see it with a 2x dpi screen (with dev tool) or in Chrome : https://codepen.io/dreddy/pen/NWWGmZe The second image is not 162px wide as it should because the srcset width descriptor is wrong. This is what happen with Kirby actually with an original image width < 324px.

[moritzebeling]

I agree, the width descriptor should always be the true width of that image, even if it pretends to be larger or smaller. The example shows, that otherwise, browsers will render the image at wrong sizes.

Request (324,162) srcset, both image sizes exists, everything good:

https://via.placeholder.com/162x100 162w,
https://via.placeholder.com/324x200 324w

Request (324,162) srcset, only low resolution exists while width descriptor reflects the request instead of the response, browser shows image at 81x50px, which is unexpected:

https://via.placeholder.com/162x100 162w,
https://via.placeholder.com/162x100 324w

Expected: Request (324,162) srcset, only low resolution exists, width descriptor should reflect the true width, in order to be rendered correctly by the browser:

https://via.placeholder.com/162x100 162w,
https://via.placeholder.com/162x100 162w

Which raises the question, if the srcset should contain duplicates? Maybe both lines could be compressed into one: Request (324,162) srcset, only low resolution exists, only that size is included in the srcset:

https://via.placeholder.com/162x100 162w

[seb-celinedesign]

Which raises the question, if the srcset should contain duplicates? Maybe both lines could be compressed into one: Request (324,162) srcset, only low resolution exists, only that size is included in the srcset:

https://via.placeholder.com/162x100 162w

👍 Duplicates in srcset attribute cause errors on w3c validator, so it'll be even better if Kirby can help at this point.

[moritzebeling]

When looking at Kirby instead of the placeholder images, they won’t look like duplicates at first glance.

An image, which only exists at 162px in width but is requested as a (162,324) srcset will then (when the changes suggested above are in place) look like:

media/pages/…/…-162x.jpg 162w
media/pages/…/…-324x.jpg 162w // true width is 162px

But the second image is only 162px in width, so the two lines don’t look like duplicates but technically they are. But when generating the srcset, Kirby will know that the second line is a fake and could not include it in the srcset.

[bastianallgeier]

I really wanted to have this fixed for 3.3.0 but I just hit a serious roadblock. In order to find out if a described width exceeds the width of the image Kirby needs to read the image width to create the correct srcset attribute. We had this before for thumbnails where this led to performance issues. We need to cache the file dimensions before we can move on with this. I'm sorry that you have to wait longer.

[moritzebeling]

I don‘t want to stress anybody, but it would be really nice, if this topic would be back on the table at some point. Thumb images really look bad if the original is smaller than the requested size.

[silllli]

I am using this function as part of a file method plugin in all recent projects and didn’t notice any major performance hits:

'slGetSrcset' => function() {
  // get image presets from config
  $presets = option('thumbs')['presets'];

  foreach ($presets as $preset) {
    // check if actual width is larger than resize
    if ($preset['width'] < $this->dimensions()->width()) {
      // add size to array
      $sources[] = $this->thumb($preset)->url() . ' ' . $preset['width'] . 'w';
    } else {
      // add actual width as last entry
      $sources[] = $this->url() . ' ' . $this->dimensions()->width() . 'w';

      // stop the loop
      break;
    };
  };

  return implode(', ', $sources);
}

Now I am wondering if I just got used to it. 🤔

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[silllli]

Has this been fixed?

[lukasbestle]

This issue requires either caching of the image dimensions (discussed in https://github.com/getkirby/kirby/issues/3548) or a well-performing way to determine image dimensions on the fly without caching. To continue with the implementation, we first need a performance test comparing caching to dynamically determining the dimensions. Help is very welcome here.

[distantnative]

@lukasbestle we abandoned the idea of caching over here https://github.com/getkirby/kirby/issues/3548 - but we never did really test the performance hit from calculating on-the-fly, did we?

[lukasbestle]

I don't think so. Bastian wrote in this comment that we had performance issues with a similar feature before, but @silllli writes that a performance hit is not noticeable. I think we'd really have to test this with real world code.

[silllli]

I don't think so. Bastian wrote in this comment that we had performance issues with a similar feature before, but @silllli writes that a performance hit is not noticeable. I think we'd really have to test this with real world code.

I realized that I’m always using caching in production so maybe that’s why I haven’t noticed any performance problems in the wild. Testing is probably the way to go.

[silllli]

I realized that I’m always using caching in production so maybe that’s why I haven’t noticed any performance problems in the wild. Testing is probably the way to go.

Just started updating an old site and noticed that there actually is a major performance hit on pages with many images. It’s not as noticeable if there are only a few images and can be forgotten about when using the cache.

But yeah, it’s no good.

Edit: I have to take this back… replaced my custom function with the builtin srcset() and it’s only slightly better performing, although I don’t really get why. This probably needs some fundamental testing to find out what’s going on.

[jonaskuske]

FWIW, I also experience massive performance hits when using (the builtin) ->srcset() more than a couple times.