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

behavior of responsive images when original image is smaller than target size #48

Closed
frontendschlampe opened this issue Jan 18, 2019 · 28 comments
Assignees
Labels

Comments

@frontendschlampe
Copy link

Affected version(s)
4.6.13 (I think in all 4.x)

Description
Too small images will be upscaled in frontend which causes an extreme loss of quality

How to reproduce
I have an image with a width of 71px. I make an image size setting with a media query (min-width: 1200px) and the following settings: width: 200px, Resize mode: Proportional, Pixel densities/scale factors: 1x, 1.5x, 2x (all other settings are empty). If I include the image with the image size, the image will be scaled to the target width of 200px, because there's a scale of 0.355x:

<picture>
<source srcset="files/xxx.jpg 0.355x" media="(min-width: 1200px)">
<img src="files/xxx.jpg" srcset="files/xxx.jpg 0.355x" alt="xxx" itemprop="image">
</picture>

bildschirmfoto 2019-01-18 um 12 27 38

This behavior is not correct, because the image should not be additional upscaled. The upscaling for the retina screen is at least 2x by default and additionally 1/0,355 = 2,817. So the final upscaling of the image will be 2*1/0,355 = 5,633802817.

If I don't use the media query setting and add my settings to the default setting of the image size, the behavior is correct: there's no output of scaling and the image is not bigger than the original size.

bildschirmfoto 2019-01-18 um 12 27 10

So the behavior should be identical and there should be no scaling factor output if the original image is smaller than the target image size.

/cc @ausi

@fritzmg
Copy link

fritzmg commented Jan 18, 2019

@frontendschlampe it does not look like anything was scaled by Contao. Contao 4 does not upscale images at all anymore.

As you can see in the HTML code you have posted, the original image is used, not a scaled image. The srcset attribute with 0.355x is correct, since the original image is 71px wide and the target size would be 200px. 200px * 0.355 = 71px.

The only issue I can see here is that the srcset is superfluous, since it's just the orignal image again. May be it could be ommitted automatically.

@frontendschlampe
Copy link
Author

Contao 4 does not upscale images at all anymore.

As you see: they will be upscaled!

0.355x isn't correct, because the image doesn't have the target size - so it is upscaled. And as I wrote: if I use the main image size settings, the behavior is different to the behavior of using the media query setting.

If the image is too small to fit the target size, there shouldn't be any upscaling - so there shouldn't be any scaling factor like 0.355x.

@fritzmg
Copy link

fritzmg commented Jan 18, 2019

As you see: they will be upscaled!

But they aren't. Your HTML code proves that ;).

@frontendschlampe
Copy link
Author

But they aren't. Your HTML code proves that ;).

The image aren't upscaled - you're right. But with 0.355x they will be upscaled by the browser, but if the image is too small, the original image should be dispensed without any changes like it is with the main image size settings.

@fritzmg
Copy link

fritzmg commented Jan 18, 2019

But with 0.355x they will be upscaled by the browser

True, but isn't that what you want? You have set the image size to 200 after all, so it's expected that images using this image setting should be displayed at that size.

@frontendschlampe
Copy link
Author

I don't think so. If we want this, we didn't have to exclude the functionality of upscaling images. ;-)

If the original image is too small to fit the target size, it must not be upscaled - no matter in which way.

And as you see: we have two different behaviors: one with "upscaling" and one with no "upscaling".

@fritzmg
Copy link

fritzmg commented Jan 18, 2019

If we want this, we didn't have to exclude the functionality of upscaling images. ;-)

Yes we do, because it doesn't make any sense to upscale images on the server side. They should simply be upscaled on the client side.

If the original image is too small to fit the target size, it must not be upscaled - no matter in which way.

Contao cannot prevent this. You wight have width: 100%; height: auto; on your img ;).

And as you see: we have two different behaviors: one with "upscaling" and one with no "upscaling".

May be this behavior is browser specific. Have you tried different browsers? Also have you tried changing

<source srcset="files/xxx.jpg 0.355x" media="(min-width: 1200px)">

to

<source src="files/xxx.jpg" srcset="files/xxx.jpg 0.355x" media="(min-width: 1200px)">

and see if the browser still scales the image?

@fritzmg
Copy link

fritzmg commented Jan 18, 2019

I have made some tests with https://jsfiddle.net/x5tyz29e/

Firefox: upscales the image.
Chrome: upscales the image.
Edge: does not upscale the image.
Safari: upscales the image.

Note: it does not make any difference whether or not src="https://via.placeholder.com/150" is present in the <source>.

@frontendschlampe
Copy link
Author

Yes we do, because it doesn't make any sense to upscale images on the server side. They should simply be upscaled on the client side.

I agree

Contao cannot prevent this. You wight have width: 100%; height: auto; on your img ;).

I agree, too - but with the described behavior I have no chance to change the scaling on client side - it's everytime width: 100%; height: auto; with 0.355x

May be this behavior is browser specific. Have you tried different browsers?

You understand me wrong. If there's a scale factor 0.355x there's everytime the same behavior. But I have two different settings in Contao backend for the image sizes: (1) main image settings and (2) media query image settings.

bildschirmfoto 2019-01-18 um 14 06 17

@fritzmg
Copy link

fritzmg commented Jan 18, 2019

@frontendschlampe and I are discussing this on Slack ;)

The basic questions I would ask are:

  • Should Contao omit any srcset density sizes, if they are just the original image (both on <img> and <source> elements).
  • Should Contao omit any <source> elements, if they only contain the original image and no additional sizes attribute is present.

If Contao removes these instances, then the browser will not automatically upscale the small image according to the density definition when using <source> elements, which might be technically incorrect. But it would be more convenient for the user.

You can always still use the sizes attribute, to size (and even upscale) the image as you like.

@ausi
Copy link
Member

ausi commented Jan 18, 2019

Should Contao omit any srcset density sizes, if they are just the original image (both on <img> and <source> elements).

I think this can be simplified: Remove the x-descriptor if the srcset only contains one image. srcset="files/xxx.jpg 0.355x" would then just get srcset="files/xxx.jpg"

Should Contao omit any <source> elements, if they only contain the original image and no additional sizes attribute is present.

I don’t think <source>s can/should be omitted because the aspect ratio and zoom level might be different to other <source>s or the <img>

@fritzmg
Copy link

fritzmg commented Jan 18, 2019

I think this can be simplified: Remove the x-descriptor if the srcset only contains one image. srcset="files/xxx.jpg 0.355x" would then just get srcset="files/xxx.jpg"

In case of Contao, yes, because Contao currently always adds the 1x image, even though it's not necessary.

I don’t think <source>s can/should be omitted because the aspect ratio and zoom level might be different to other <source>s or the <img>

I meant only if only the original image is used.

@ausi
Copy link
Member

ausi commented Jan 18, 2019

I don’t think <source>s can/should be omitted because the aspect ratio and zoom level might be different to other <source>s or the <img>

I meant only if only the original image is used.

Then the image size itself would be misconfigured anyway I think, because the original image gets used only if aspect ratio and zoom-level matches in which case <source> shouldn’t be used.

@fritzmg
Copy link

fritzmg commented Jan 18, 2019

Well, see @frontendschlampe 's example. If the source image happens to be smaller than the target size for the responsive image size, then only the original image will be used in the <source>.

@ausi
Copy link
Member

ausi commented Jan 18, 2019

If the source image happens to be smaller than the target size for the responsive image size, then only the original image will be used in the <source>.

This would only be true if the aspect ratio of the responsive image size matches the image. Otherwise Contao would still crop the image to the correct aspect ratio, but without upscaling it.

@frontendschlampe simplified his example for this issue, in the real world example the <source> points to a generated image (small but cropped).

@fritzmg
Copy link

fritzmg commented Jan 18, 2019

Hm, you are right.

@frontendschlampe what was your original goal? i.e. what is the real world example?

@frontendschlampe
Copy link
Author

The real world example isn't far away. ;-)

I just have the setting with width, height and resize mode "crop".

@fritzmg
Copy link

fritzmg commented Jan 18, 2019

I just have the setting with width, height and resize mode "crop".

But in that case the responsive image should be cropped (assuming it has a different aspect ratio).

@frontendschlampe
Copy link
Author

But in that case the responsive image should be cropped (assuming it has a different aspect ratio).

Yes ... cropped ... but not resized. The problem remains the same. ;-)

@fritzmg
Copy link

fritzmg commented Jan 20, 2019

Can you please post the complete configuration and the result?

@frontendschlampe
Copy link
Author

Can you please post the complete configuration and the result?

Yes, I can - see below

Image Setting "XXX" (original image size: 71x100px)

bildschirmfoto-2019-01-20-um-16 31 08

Setting 1

bildschirmfoto 2019-01-20 um 16 28 23

Setting 2

bildschirmfoto 2019-01-20 um 16 27 41

Output:

<picture>
<!--[if IE 9]><video style="display: none;"><![endif]-->
<source srcset="assets/images/9/xxx.jpg 0.355x" media="(min-width: 1200px)">
<!--[if IE 9]></video><![endif]-->
<img src="assets/images/9/xxx.jpg" alt="" itemprop="image">
</picture>

Image is resized.

Image Setting "YYY" (original image size: 71x100px)

bildschirmfoto-2019-01-20-um-16 51 39

Setting 1 (no more settings)

bildschirmfoto 2019-01-20 um 16 51 23

Output:

<img src="assets/images/9/yyy.jpg" srcset="assets/images/9/yyy.jpg 0.355x" width="71" height="36" alt="" itemprop="image">

Image is NOT resized.

But when I think about it, we have some more problems. ;-)

  1. The results of both settings are different: with "XXX" the image is cropped and upscaled to target width 200x100px - with "YYY" the image is cropped, not upscaled, but downscaled to 71x36px, because the image is not bigger. I think the expected behavior should be the same in both settings?!

  2. In the case, that I want to crop the image to 200x100px and the image just have a size of 71x100px: Should images been cropped, if they are too small? Upscaled or not?

  3. What should be the behavior if I just have the settings: width: 200px, Resize mode: Proportional, Pixel densities/scale factors: 1x, 1.5x, 2x (my initial settings)? Should images be upscaled or output in original size?

The problem is more complicated as I think in the beginning. :-D

@fritzmg
Copy link

fritzmg commented Jan 20, 2019

That's still not a real world example. Both image size settings are exactly the same. Why would you need that?

@frontendschlampe
Copy link
Author

That's still not a real world example. Both image size settings are exactly the same. Why would you need that?

What do you mean?

@fritzmg
Copy link

fritzmg commented Jan 20, 2019

Your responsive size settings are the same as your regular size settings.

@frontendschlampe
Copy link
Author

Your responsive size settings are the same as your regular size settings.

In responsive settings, I have scale factors. But if it's better for you, you can change the responsive size settings to 90x190px ... the problems remains the same. ;-)

@fritzmg
Copy link

fritzmg commented Jan 20, 2019

In responsive settings, I have scale factors.

Yes, but why would you define that in a responsive image setting, and not in the regular size?

However, that's not the point anyway ;)

@ausi the problem does indeed only arise, if you use pixel densities in the responsive image setting (other than 1x). If you leave that setting empty, the output will be

<picture>
  <source srcset="assets/images/f/50x50-86e4173d.png" media="(min-width: 1200px)">
  <img src="assets/images/3/50x50-be9e7bb2.png" alt="" itemprop="image">
</picture>

If you set it to 2x for example, the output will be

<picture>
  <source srcset="assets/images/f/50x50-86e4173d.png 0.263x" media="(min-width: 1200px)">
  <img src="assets/images/3/50x50-be9e7bb2.png" alt="" itemprop="image">
</picture>

And in the latter case, the browser will upscale the image from 50x24 to 190x90. In the former case it will not.

The output is almost the same in both cases, the only difference is, that Contao adds 0.263x in the latter case, as @frontendschlampe already said :).

Imho, the output should be the same in both cases. It should not matter, that you requested a 2x image in the latter case for the responsive image - if the image is too small anyway.

@ausi
Copy link
Member

ausi commented Jan 20, 2019

I agree. So my proposed fix

Remove the x-descriptor if the srcset only contains one image. srcset="files/xxx.jpg 0.355x" would then just get srcset="files/xxx.jpg"

would solve it I think.

@ausi ausi transferred this issue from contao/contao Jan 27, 2019
@ausi ausi added the bug label Jan 27, 2019
@ausi ausi closed this as completed in 8e89bae Jan 27, 2019
@ausi
Copy link
Member

ausi commented Jan 27, 2019

Fixed in version 0.3.8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants