Updated: Comment #0

Problem/Motivation

When setting up a picture mapping with the 3 default image styles (Small, Medium, Large), a problem arises because those three image styles allow upscaling.

The problem: when the source image is smaller than one or more of the image styles (e.g. a 60x60 image), then due to the allowed upscaling, responsive image magic is applied at all three breakpoints: the small image is upscaled to Small (100x100), Medium (220x220) and Large (480x480). This is inappropriate.

Not only does it look crappy, it also results in pointless image derivatives being generated. I realize this is somewhat of an edge case, but the experience is very unpleasant.

Proposed resolution

Make the Picture formatter smart enough to only apply those image styles (with associated breakpoints) that don't cause upscaling.

Remaining tasks

TBD.

User interface changes

None.

API changes

None.

#2061377: [drupalImage] Optionally apply image style to images uploaded in CKEditor 5

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

attiks’s picture

If I understand correctly, you mean that picture needs to figure out the max dimensions as defined by the image style and make sure that the breakpoint does not cause any upscaling?

So given a breakpoint "all and (min-width: 560px) and (max-width: 850px)" this might be doable, but given a breakpoint "all and (min-width: 22em) and (max-width: 38em)" this will get pretty nasty?

Wim Leers’s picture

#1: The nice thing is that we don't need to look at breakpoints, which could indeed be em-based, but at the image styles, which are always px-based :)

attiks’s picture

#2 Two options

  1. We check the values defined in each image style effect, this will add a lot of logic to accommodate missing values, or like for the scale effect it behaves different if the image is portrait or landscape.
  2. We check the actual size of the image, but it might be that it doesn't exists yet, and it will have to check the file system each time.

The other option is that people are smart enough to not map small images to 'big' breakpoints. I understand the unpleasantness but I'm afraid this will complicate picture module.

Wim Leers’s picture

Well, either there must be a huge disclaimer for the Picture module, to explain that you should not ever check the "Allow Upscaling" checkbox for the "scale" effect, and we should disable upscaling for Drupal 8's default image styles.

That, or Picture module should indeed handle all this additional complexity, simply because it causes terrible results.

(I just demoed a prototype for #2061377: [drupalImage] Optionally apply image style to images uploaded in CKEditor 5 to Angie and this problem was the very first thing she asked about, even though I demoed with a large image and did not even hint at this issue!)

attiks’s picture

#4 IMOH we just should remove the upscale option all together, or at least make it non-default behavior. I never used the option to upscale any image and I guess most site builders/themers disable is as well.

PS: I would not expect any less from Angie.

Wim Leers’s picture

+1 for making it non-default.

Jelle_S’s picture

The first patch (imagestyle_default_no_upscale) basicly makes the image style not upscale by default (I wasn't sure if I had to change _image_update_get_default_styles() in image.install as well as it is used in image_update_8000(), so I didn't change them there).

The second patch (picture_no_upscale) tries to be smart and see whether or not the images will be upscaled. If so, the original images will be used.
Couple of notes:

  • This patch does not update tests. PictureFieldDisplayTest will search for all image styles in the output of the picture element. Some might be missing because of this new behavior so this test will most probably fail. I wasn't sure how this new behavior should be tested so I left that out for now (just so we have a patch to start from).
  • If the applied image styles, for any reason you might think of, contain any other effects besides those that alter the dimensions (e.g. desaturate), these will not be applied when the image would be upscaled, as in that case the original image will be used without any effects added (not sure if there is a use case for this, but I just thought I'd mention it).
  • I tested this patch manually with a 400x300 image. The image styles (medium(220x220) and thumbnail(100x100)) got applied when I made my window smaller. The original image (what would previously have been the large image style (480x480)) was used when my window was bigger.

Let's see what the bot has to say about these

Jelle_S’s picture

+++ b/core/modules/picture/picture.module
@@ -369,6 +371,43 @@ function picture_get_image_dimensions($variables) {
+  ¶

trailing spaces... As said in my previous comment this patch will not pass tests anyway, so I'm not going to fix that right now :-)

Wim Leers’s picture

The imagestyle_default_no_upscale patch looks fine :) You don't want to modify people's image styles in the upgrade path — that would amount to data loss. So this is good to go AFAICT.

RE: the second patch, I'm not sure I understand the second bullet. In general, it's not very clear what it does. I assume this is about handling the case of upscaling being enabled and preventing that from being applied to a small image?
I wonder what attiks thinks about this approach.


Generally though, it feels like we should only do one of the two things. And I'm inclined to agree with what attiks was saying in #5: just changing the default image styles to not use upscaling should be fine. If people choose to use upscaling *and* Picture module, it's their responsibility.

The last submitted patch, 7: drupal8-picture.module-picture_no_upscale-2123225-7.patch, failed testing.

Status: Needs review » Needs work
Jelle_S’s picture

Ok, to clarify on the second bullet:

For example:
Say you have an image style 'Large' and added the 'Desaturate' effect and the scale effect (480x480, upscaling allowed). Then you mapped this image style to the 'wide' breakpoint of Bartik. With this patch, when this breakpoint is active, following scenarios occur:

  1. The original image is larger than 480x480:
    The image derivative that the user will get to see will be scaled to 480x480 and desaturated.
  2. The original image is smaller than 480x480 (e.g. 400x300):
    The user will not see a derivative, but the original image (size 400x300 and not desaturated)

I hope that clarifies things.

RainbowArray’s picture

It seems to me that the default no upscale approach is sufficient.

Picking the right image dimensions for a particular breakpoint isn't simple. It's not just a matter of checking if the image's width (e.g. 400x400) matches up with a breakpoint (max-width: 400px), because in most scenarios the image's width won't be taking up the entire width of the viewport, which is what the media query for the breakpoint is testing. So to test if upscaling should be disallowed, you'd really need to know the percentage of the viewport that an image was taking up at a breakpoint, and that's way too complicated, in my opinion.

So I'd suggest the way to go here is to get the default no upscaling patch working correctly and stick with that as a solution.

attiks’s picture

Issue tags: +Needs reroll

FYI: picture is renamed to responsive_image so this needs a reroll once #2124377-74: Rename "Picture" module to "Responsive Image" module is committed

Eli-T’s picture

Component: picture.module » responsive_image.module
attiks’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
attiks’s picture

Title: Picture mapping + small images may result in unnecessary upscaling » Do not upscale by default
Component: responsive_image.module » image system

Re-titling this issue, since the easiest solution is to disable the upscale by default

attiks’s picture

Status: Needs work » Needs review

Go bot

bradklaver’s picture

FileSize
1.41 KB

Rerolling Jelle_S's #7 patch that simply sets true to false in the Image style's yml files.

Status: Needs review » Needs work

The last submitted patch, 20: node-2123225-20.patch, failed testing.

Risse’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.41 KB

Rerolled previous patch.

Status: Needs review » Needs work

The last submitted patch, 22: node-2123225-22.patch, failed testing.

RainbowArray’s picture

Failures are in ImageFieldDisplayTest and ResponsiveImageFieldDisplayTest if somebody wants to look into this.

Status: Needs work » Needs review

attiks queued 22: node-2123225-22.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 22: node-2123225-22.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review
FileSize
3.25 KB

tests adapted

attiks’s picture

Status: Needs review » Reviewed & tested by the community

No brainer patch

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

I think we need a change notice to tell site builders that the default image style provided by core no longer upscale by default. Also what happens when someone sets them to upscale?

attiks’s picture

Status: Needs work » Reviewed & tested by the community

I think a change notice is a bit OTT, but I'll create one anyway ([#2335637]). The only thing that will happen if somebody enables the upscale is that they end up with ugly images. I think most people disable this setting when using Drupal 7.

Back to RTBC?

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Related issues: +#872206: Add possibility to not upscale for "Scale and crop" effect

Discussed with @xjm and who pointed out that this change improves actual site builder experience. @xjm also pointed out the existence of #872206: Add possibility to not upscale for "Scale and crop" effect

Committed 535c590 and pushed to 8.0.x. Thanks!

  • alexpott committed 535c590 on 8.0.x
    Issue #2123225 by Jelle_S, attiks, Risse, bradklaver | Wim Leers: Fixed...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.