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

image->size with disabled upscaling -> wrong aspect ratio #628

Open
thomasaull opened this Issue Jun 29, 2018 · 16 comments

Comments

Projects
None yet
5 participants
@thomasaull

thomasaull commented Jun 29, 2018

Short description of the issue

Default Image field with one image with Dimensions of 1200x1400 px (Width x Height). In my template I'm exporting this image in different resolutions to use it as an responsive image but with a fixed aspect ratio of 16:9

$options = ['upscaling' => false];
$image->size(1920, 1080, $options);
$image->size(960, 540, $options);
$image->size(480, 270, $options);

Expected behavior

Images which are to small in dimensions for the target image size should still maintain the expected aspect ratio. The two smaller images can be cropped without problems, the larger image would have to be upscaled to meet the required dimensions (which is disabled). I would expect this image to have dimensions of 1200x675 (crop to maximum possible dimensions without upscaling but still maintain the aspect ratio of 16:9)

Actual behavior

The image just uses the original dimensions as a maximum independently. The image with a target size of 1920x1080 gets cropped to 1200x1080. So I end up with the two smaller images in the correct aspect ratio of 16:9 and with the largest image with a completely arbitrary aspect ratio.

Setup/Environment

  • ProcessWire version: 3.0.105
@ryancramerdesign

This comment has been minimized.

Show comment
Hide comment
@ryancramerdesign

ryancramerdesign Jul 17, 2018

Contributor

I hear what you are saying, and it seems like it would be a preferable result for this case, but in terms of what is the correct return value for the size() function, I'm not so sure. You've got upscaling turned off, so it is impossible to achieve your requested dimension. As a result, it either has to throw an Exception, or return an image that doesn't match the request. While it can't achieve your width dimension, it can achieve your height dimension. So it's returning a image that at least matches your requested height dimension. On the other hand, if it were to return what you suggested, then it would be returning an image that doesn't match either of your width or height dimension. Which result is more correct, I'm not really sure. It comes down to a question of whether it's the requested dimensions that are more important, or the proportion created by those combined dimensions. The coder side of me wants preference to the numbers in the arguments (current behavior), while the designer side of me wants preference to the proportion. I suppose it depends on context, but maybe proportion fits our context better? @horst-n what do you think?

Contributor

ryancramerdesign commented Jul 17, 2018

I hear what you are saying, and it seems like it would be a preferable result for this case, but in terms of what is the correct return value for the size() function, I'm not so sure. You've got upscaling turned off, so it is impossible to achieve your requested dimension. As a result, it either has to throw an Exception, or return an image that doesn't match the request. While it can't achieve your width dimension, it can achieve your height dimension. So it's returning a image that at least matches your requested height dimension. On the other hand, if it were to return what you suggested, then it would be returning an image that doesn't match either of your width or height dimension. Which result is more correct, I'm not really sure. It comes down to a question of whether it's the requested dimensions that are more important, or the proportion created by those combined dimensions. The coder side of me wants preference to the numbers in the arguments (current behavior), while the designer side of me wants preference to the proportion. I suppose it depends on context, but maybe proportion fits our context better? @horst-n what do you think?

@ryancramerdesign

This comment has been minimized.

Show comment
Hide comment
@ryancramerdesign

ryancramerdesign Jul 17, 2018

Contributor

Btw, here's where the logic where it determines what to do with this particular case: https://github.com/processwire/processwire/blob/dev/wire/core/ImageSizerEngine.php#L672

Contributor

ryancramerdesign commented Jul 17, 2018

Btw, here's where the logic where it determines what to do with this particular case: https://github.com/processwire/processwire/blob/dev/wire/core/ImageSizerEngine.php#L672

@horst-n

This comment has been minimized.

Show comment
Hide comment
@horst-n

horst-n Jul 20, 2018

@ryancramerdesign My opinion is: when it can't be rendered because it is to small, it is an exception. I would expect that a designer / developer defines the min settings for those input fields to match at least the max needed ones! Or he allows upscaling.

Without that, you ever get the wrong and maybe totally useless results: f.e., if you want to get variations of 900x600 and 1800x1200, but your original image only is 600x500, what should be done then? Cropping it two times to 600x400 and serve it?

Maybe I'm to simple minded, but I don't see any advandtages. In every site, where I have special interest in serving fine images, (that's the case when I disable upscaling!) I restrict the users only to be able to upload images matching the needed min dimensions.

horst-n commented Jul 20, 2018

@ryancramerdesign My opinion is: when it can't be rendered because it is to small, it is an exception. I would expect that a designer / developer defines the min settings for those input fields to match at least the max needed ones! Or he allows upscaling.

Without that, you ever get the wrong and maybe totally useless results: f.e., if you want to get variations of 900x600 and 1800x1200, but your original image only is 600x500, what should be done then? Cropping it two times to 600x400 and serve it?

Maybe I'm to simple minded, but I don't see any advandtages. In every site, where I have special interest in serving fine images, (that's the case when I disable upscaling!) I restrict the users only to be able to upload images matching the needed min dimensions.

@thomasaull

This comment has been minimized.

Show comment
Hide comment
@thomasaull

thomasaull Jul 20, 2018

In my use case it was for responsive images, in my frontend I define a srcset for small, medium, high, ultra-high resolution. But not for every picture there is an ultra-high resolution available, so to save my clients frustration (and phone calls) I would allow to upload pictures with a minimum of medium/high resolution. For the responsive image I do not want to upscale the image to high-resolution, since there would be more bandwidth needed, without any benefit

Without that, you ever get the wrong and maybe totally useless results: f.e., if you want to get variations of 900x600 and 1800x1200, but your original image only is 600x500, what should be done then? Cropping it two times to 600x400 and serve it?

In my opinion and if upscaling is not allowed: Yes

thomasaull commented Jul 20, 2018

In my use case it was for responsive images, in my frontend I define a srcset for small, medium, high, ultra-high resolution. But not for every picture there is an ultra-high resolution available, so to save my clients frustration (and phone calls) I would allow to upload pictures with a minimum of medium/high resolution. For the responsive image I do not want to upscale the image to high-resolution, since there would be more bandwidth needed, without any benefit

Without that, you ever get the wrong and maybe totally useless results: f.e., if you want to get variations of 900x600 and 1800x1200, but your original image only is 600x500, what should be done then? Cropping it two times to 600x400 and serve it?

In my opinion and if upscaling is not allowed: Yes

@horst-n

This comment has been minimized.

Show comment
Hide comment
@horst-n

horst-n Jul 20, 2018

@thomasaull Using another imagefield at places where you need highres variations is not possible?

horst-n commented Jul 20, 2018

@thomasaull Using another imagefield at places where you need highres variations is not possible?

@horst-n

This comment has been minimized.

Show comment
Hide comment
@horst-n

horst-n Jul 20, 2018

@thomasaull Another question in this regard: Have you compared a medium sized variation upscaled through GD or Imagick to highres and displayed fullwidth in the browser, against a mediumsized image that gets upscaled to fullwidth by the browser directly?
I can't see better results here on desktop monitors with the latter.

Why not optionally using enabled upscaling for the Highres only:

$image->size(1920, 1080, ['upscaling'=>true, 'sharpening'=>'none']);
$image->size(960, 540, ['upscaling'=>false]);
$image->size(480, 270, ['upscaling'=>false]);

horst-n commented Jul 20, 2018

@thomasaull Another question in this regard: Have you compared a medium sized variation upscaled through GD or Imagick to highres and displayed fullwidth in the browser, against a mediumsized image that gets upscaled to fullwidth by the browser directly?
I can't see better results here on desktop monitors with the latter.

Why not optionally using enabled upscaling for the Highres only:

$image->size(1920, 1080, ['upscaling'=>true, 'sharpening'=>'none']);
$image->size(960, 540, ['upscaling'=>false]);
$image->size(480, 270, ['upscaling'=>false]);
@thomasaull

This comment has been minimized.

Show comment
Hide comment
@thomasaull

thomasaull Jul 20, 2018

@horst-n I can sure implement workarounds for my cases, that's not the issue here. It is more about the way the system behaves in general, which for me, is leading a bit to unpredictable results

thomasaull commented Jul 20, 2018

@horst-n I can sure implement workarounds for my cases, that's not the issue here. It is more about the way the system behaves in general, which for me, is leading a bit to unpredictable results

@LostKobrakai

This comment has been minimized.

Show comment
Hide comment
@LostKobrakai

LostKobrakai Jul 20, 2018

Collaborator

I'd support @thomasaull standpoint. I feel like maintaining the aspect ratio is the way more common expectation to "at least have one pixel dimension correct", aspect ratio is just the implicit dimension, which is currently incorrect after the resizing. In responsive designs images are hardly displayed at their native resolution anyways, so differences at the pixel dimensions are imho less a problem than differences at the aspect ratio, which will break any layout which depends on images being of a certain aspect ratio.

Collaborator

LostKobrakai commented Jul 20, 2018

I'd support @thomasaull standpoint. I feel like maintaining the aspect ratio is the way more common expectation to "at least have one pixel dimension correct", aspect ratio is just the implicit dimension, which is currently incorrect after the resizing. In responsive designs images are hardly displayed at their native resolution anyways, so differences at the pixel dimensions are imho less a problem than differences at the aspect ratio, which will break any layout which depends on images being of a certain aspect ratio.

@horst-n

This comment has been minimized.

Show comment
Hide comment
@horst-n

horst-n Jul 20, 2018

I'm aware of all these arguments, but I don't get the advantage of disabling upscaling when the image gets upscaled through the browser anyways. At least both looks the same to me, so that we also then can override the disabled upscaling option.

horst-n commented Jul 20, 2018

I'm aware of all these arguments, but I don't get the advantage of disabling upscaling when the image gets upscaled through the browser anyways. At least both looks the same to me, so that we also then can override the disabled upscaling option.

@thomasaull

This comment has been minimized.

Show comment
Hide comment
@thomasaull

thomasaull Jul 20, 2018

Filesize. The image get's upscaled without any additional information, so it does not really looks better, but the user has to download more data without any real benefit

thomasaull commented Jul 20, 2018

Filesize. The image get's upscaled without any additional information, so it does not really looks better, but the user has to download more data without any real benefit

@ryancramerdesign

This comment has been minimized.

Show comment
Hide comment
@ryancramerdesign

ryancramerdesign Jul 20, 2018

Contributor

Thanks for the feedback. It does seem like maintaining the aspect ratio would be better for the context of most PW installs (responsive web sites) when upscaling is disabled. But there might be a different context and good use case for the current behavior, where it will try to at least keep one of the dimensions if possible, when upscaling is disabled. I don't know what the use case is, but it seems like there could be one. If there is, then chances are someone (or many), are relying upon it. In which case, we should keep it (since it's the current behavior), and if important, maybe add an alternative option that sticks to the proportion. On the other hand, if there is no use case for the current behavior, then it's probably okay to change it.

I haven't tried it yet, but also wondering what the behavior is if upscaling is off and neither width nor height dimension can be achieved.

I'm aware of all these arguments, but I don't get the advantage of disabling upscaling when the image gets upscaled through the browser anyways. At least both looks the same to me, so that we also then can override the disabled upscaling option.

I think the reason someone might disable upscaling is so that they aren't creating images that consume more space (and thus bandwidth) than what is necessary. Letting the browser upscale will produce the same result visually, but with fewer bytes/bandwidth. I could be wrong though, if an upscaled image consumes the same amount of bytes as one that isn't, then there's probably no value in disabling upscaling for a case like this.

Contributor

ryancramerdesign commented Jul 20, 2018

Thanks for the feedback. It does seem like maintaining the aspect ratio would be better for the context of most PW installs (responsive web sites) when upscaling is disabled. But there might be a different context and good use case for the current behavior, where it will try to at least keep one of the dimensions if possible, when upscaling is disabled. I don't know what the use case is, but it seems like there could be one. If there is, then chances are someone (or many), are relying upon it. In which case, we should keep it (since it's the current behavior), and if important, maybe add an alternative option that sticks to the proportion. On the other hand, if there is no use case for the current behavior, then it's probably okay to change it.

I haven't tried it yet, but also wondering what the behavior is if upscaling is off and neither width nor height dimension can be achieved.

I'm aware of all these arguments, but I don't get the advantage of disabling upscaling when the image gets upscaled through the browser anyways. At least both looks the same to me, so that we also then can override the disabled upscaling option.

I think the reason someone might disable upscaling is so that they aren't creating images that consume more space (and thus bandwidth) than what is necessary. Letting the browser upscale will produce the same result visually, but with fewer bytes/bandwidth. I could be wrong though, if an upscaled image consumes the same amount of bytes as one that isn't, then there's probably no value in disabling upscaling for a case like this.

@thomasaull

This comment has been minimized.

Show comment
Hide comment
@thomasaull

thomasaull Jul 20, 2018

I just tested it with Sketch:
original: 202kb
scaled x3: 905kb

Should be the same for any other upscaling by php libraries, more pixels = more data

thomasaull commented Jul 20, 2018

I just tested it with Sketch:
original: 202kb
scaled x3: 905kb

Should be the same for any other upscaling by php libraries, more pixels = more data

@horst-n

This comment has been minimized.

Show comment
Hide comment
@horst-n

horst-n Jul 20, 2018

@thomasaull Filesize is a valid argument. (I hadn't thought on)

@ryancramerdesign When implementing the new behaviour, there seems to be no reason to keep the old one besides it. At least one get an unwanted result. It was thought as a compromise to not break sites completly by throwing exceptions. But it definetly is a handling of misconfigurations. One sort seems to be enough to me. And then, after the discussion, better that with keeping aspect ratios. :)

horst-n commented Jul 20, 2018

@thomasaull Filesize is a valid argument. (I hadn't thought on)

@ryancramerdesign When implementing the new behaviour, there seems to be no reason to keep the old one besides it. At least one get an unwanted result. It was thought as a compromise to not break sites completly by throwing exceptions. But it definetly is a handling of misconfigurations. One sort seems to be enough to me. And then, after the discussion, better that with keeping aspect ratios. :)

@horst-n

This comment has been minimized.

Show comment
Hide comment
@horst-n

horst-n commented Aug 14, 2018

@thomasaull, @ryancramerdesign
I sent a fix for this to the repo: processwire/processwire#118

@horst-n

This comment has been minimized.

Show comment
Hide comment
@horst-n

horst-n Aug 29, 2018

@netcarver The status should be changed to suggest a possible fix: PR #118 :)

horst-n commented Aug 29, 2018

@netcarver The status should be changed to suggest a possible fix: PR #118 :)

ryancramerdesign added a commit to processwire/processwire that referenced this issue Sep 12, 2018

@ryancramerdesign

This comment has been minimized.

Show comment
Hide comment
@ryancramerdesign

ryancramerdesign Sep 12, 2018

Contributor

Thanks @horst-n and @thomasaull — I have added Horst's PR and tested it here and seems to work nicely!

Contributor

ryancramerdesign commented Sep 12, 2018

Thanks @horst-n and @thomasaull — I have added Horst's PR and tested it here and seems to work nicely!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment