[wp-trac] [WordPress Trac] #34955: The choices of images to add to srcset shouldn't be via a ratio

[WordPress Trac]

#34955: The choices of images to add to srcset shouldn't be via a ratio
--------------------------+------------------------
 Reporter:  smerriman     |       Owner:  joemcgill
     Type:  defect (bug)  |      Status:  assigned
 Priority:  normal        |   Milestone:  4.4.1
Component:  Media         |     Version:  4.4
 Severity:  normal        |  Resolution:
 Keywords:  needs-patch   |     Focuses:
--------------------------+------------------------
Changes (by joemcgill):

 * status:  new => assigned
 * owner:   => joemcgill
 * milestone:  Awaiting Review => 4.4.1
 * keywords:   => needs-patch
 * type:  enhancement => defect (bug)


Old description:

> 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.

New description:

 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.

--

Comment:

 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.

--
Ticket URL: <https://core.trac.wordpress.org/ticket/34955#comment:2>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform