The choices of images to add to srcset shouldn't be via a ratio

[smerriman]

In #34810:

As the difference between the widths being compared increases, the probability that rounding will introduce false positives also grows. We could make the aspect ratio variance we allow relative to the difference between the widths we're comparing, but the extra calculation is probably unnecessary unless the difference between image widths is in the tens of thousands.

This is not quite true. The issue does not arise when serving huge images with tens of thousands of pixels difference - but when serving small images.

Suppose I upload a 768x1055 image. The default 300x300 medium thumbnail will be 218x300, which differs in ratio by just over 0.002, thus will not be added to the srcset.

Given 1/0.002 = 500, any image over 500px wide will never cause an issue, and with most mobiles etc being retina, you could argue this will never occur. However, there are still cases where you would want it - eg non retina desktop screens where an image jumps from a large version for some screen sizes to a small version for another.

Rather than checking if |b/a - d/c|<0.002, checking if |b - d*c/a|<=1 will ensure images are allowed if the resulting height will differ by no more than 1 pixel, which seems a better approach.

[smerriman]

Just a clarification - I see the other thread was about ensuring we don't get false positives. This is ensuring the opposite - that sizes which *should* show up actually do.

[joemcgill]

Thanks for the report @smerriman.

This looks like it's similar to the issue brought up in #34931, and I agree we need a better way to test for ratios. If I recall correctly, we were using a method similar to the one you're suggesting early in the development of this feature and we ran into some issues. Let me read back through the history and see if I can remember what the issue was and then we can come up with a fix.

[joemcgill]

34955.diff uses wp_constrain_dimensions() to calculate the expected dimensions of the larger size being compared if it were resized to the width of the smaller image. By comparing the expected dimensions to the actual dimensions of the smaller image, we should be able to reliably tell if the current image size has the same aspect ratio and, if so, include it in the srcset attribute.

I've added a test using the example from this ticket to show the failing behavior, which is fixed by this patch.

[jaspermdegroot]

In #34810 I added a link to the PR in the Responsive Images plugin repo (where early development of this feature took place) where we switched to using aspect ratio. The problem reported by the OP was that a full size image was 4px less high than expected based on our calculation so it wasn't included in the srcset (max allowed difference was 2px at that time) while it should have been.
In #34810 I also linked to an issue reported on the plugin support forum, about the accepted aspect ratio difference being too permissive. My calculation shows that an image size was 4px less high than expected, but in this case it shouldn't have been included in the srcset.
I think this shows that we shouldn't simply switch back to allowing a certain amount of pixels difference.

Using wp_constrain_dimensions() might be the solution, but maybe we have to use the width and height of the full size image as parameters when it's a soft cropped image. When the src image is an intermediate size it's width or height already has been rounded so if we pass that to the function that again does some rounding we might get undesired results.

Ps. @joemcgill - There is a typo in the inline doc in your patch: contstrained --> constrained

[joemcgill]

Thanks @jaspermdegroot.

34955.2.diff​ fixes the typo in the inline doc. While I share your concern about possible edge cases with double rounding in the process, we can't rely on the full size image because this also needs to be useful in cases where we are finding possible srcset matches to a hard cropped image in the src. In those cases, the full size would have the wrong aspect ratio.

I wish I could think of a better way of testing for this edge case. While the test included does illustrate the issue, it still feels a bit arbitrary.

[jaspermdegroot]

Yes, this would only apply if it's a soft cropped image so we would have to check that first.
Let's first see if the double rounding actually does cause issues. I will do some testing.

[smerriman]

What if you do use a pixel difference, but always in reference to shrinking the larger size to the smaller size, whichever way around it is?

That is, if your current image is axb and you want to see if cxd should be added to the srcset:

If c>a and the srcset image was used, the axb image would be replaced by ax(d*a/c), so check if |b-d*a/c|<=1.

If c<a, the only time the srcset image will be used if it has been shrunk with CSS to at most c wide already. So it's actually being display at cx(b*c/a) initially and being replaced by cxd, so check if |d-b*c/a|<=1.

I think this solves the original issue of 480x188 and 4000x1563 - because rather than comparing 4000x1537 with 4000x1567.66 which fails, you'd be comparing 480x188 and 480x187.56, which succeeds.

[jaspermdegroot]

I have tested 34955.2.diff with all cases mentioned in this ticket, #34931, and #34810. It fixes the cases where images currently don't get a srcset or an incomplete one, and I didn't see sources in the srcset attributes that shouldn't have been included.

I also tested with some other image sizes. The only problem I could find has to do with the 1px difference that we allow.
For example: If you set 400x320px hard cropped as post-thumbnail size and upload a 800x642px image, wp_constrain_dimensions() will return an array with 400 and 321 which is only 1px difference so we include it in the srcset and it will be used on HD screens. Currently we would not include this image because the aspect ratio difference is 0.0025.

As long as you don't override the width and height attributes in your theme CSS the image will still be shown at 400x320px. But many themes, including the WordPress default themes, set img { max-width: 100%; height: auto; } which will make the image element expand to 400x321px. If your theme layout depends on the exact height of an image you will have to make sure you don't override the height attribute.

I think that no matter what solution we use, we will always have to allow a small difference somewhere. Using wp_constrain_dimensions() seems to be the best solution, assuming it doesn't affect performance too much, but I suggest that we make an exception for square images because for those it's easy to avoid including a source that is one pixel off.

I also suggest to change variable $image to $image_size in the foreach loop to make the code easier to read.

[azaozz]

In 36031:

Responsive images: fix calculations when determining whether to include particular image file in srcset.

Props joemcgill.
Fixes #34955 for trunk.

[azaozz]

Reopen for 4.4.

[joemcgill]

Per this ​Slack conversation, @mikeschroder suggested that we only include this improved source selection algorithm in 4.5 and not in 4.4.1, since this is not exactly a regression. Since this is now fixed in Trunk, I'm going to tag this with 'close' and we can make a final decision before about merging this fix into 4.4.1 before it is released.

[kirasong]

Replying to joemcgill:

Per this ​Slack conversation, @mikeschroder suggested that we only include this improved source selection algorithm in 4.5 and not in 4.4.1 <snip>

I'm open to being convinced otherwise, but it's my current view that if there is any chance that a change introduced here could start including incorrect images, we wait for that change until 4.5 to allow testing time. I'd much rather be cautious about user content for a minor release. My understanding is that the worst case for the scope of this ticket is that users encounter the same behavior as in 4.3.

[azaozz]

Agreed, lets keep this out of 4.4.1.

[joemcgill]

#35193 was marked as a duplicate.

[rachelbaker]

#35193 was marked as a duplicate.