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

[11.x] Fix ratio validation for high ratio images #51296

Merged
merged 6 commits into from
May 5, 2024

Conversation

ahmedbally
Copy link
Contributor

@ahmedbally ahmedbally commented May 5, 2024

When uploading images with resolution of 1366x768 which has ratio 16/9 validation fails
so i go with solution to solve the issue provided with test case of the 16/9 ratio
its also increase the precise for other ratios

@taylorotwell taylorotwell merged commit a8829f7 into laravel:11.x May 5, 2024
28 checks passed
func0der pushed a commit to func0der/framework that referenced this pull request May 6, 2024
* fix image ration validation for high ratio images

* fix style ci

* enhance comments

* enhance precision equation

* enhance precision equation

* enhance precision equation
@vlakoff
Copy link
Contributor

vlakoff commented May 7, 2024

I have a bit of trouble to understand this change, and to determine if it is really correct.

To summarize, you have replaced:
max($width, $height)
with:
max(($width + $height) / 2, $height)

(and the larger the result is, the lowest the precision (i.e. tolerance) is, and it turn the "stricter" the aspect ratio validation is)

The points I have trouble with:

  • You are discarding $width. That seems to introduce a bias. Previously, $width and $height had the same "importance". My point is that 16/9 (1366x768 image) and 9/16 (768x1366 image) should validate exactly the same. Your code changed handling of the former, but changed nothing with the latter.
  • Considering a 1366x768 image, ($width + $height) / 2 gives 1067. So this value 1067 is the new max, whereas previously it was 1366. I have just a bit of trouble to understand the rationale behind this calculated value.

Provided that the value of ($width + $height) / 2 is correct to consider, I think the proper fix would not be max(($width + $height) / 2, $height), but just ($width + $height) / 2.

@vlakoff
Copy link
Contributor

vlakoff commented May 7, 2024

I'm thinking of the following approach:

$desiredRatio = $numerator / $denominator;

$actualRatio = $width / $height;

$otherRatio1 = ($width + 1) / $height;
$otherRatio2 = ($width - 1) / $height;
$otherRatio3 = $width / ($height + 1);
$otherRatio4 = $width / ($height - 1); // FIXME: avoid division by 0 if $height === 1

$success = abs($desiredRatio - $actualRatio) === min(
	abs($desiredRatio - $actualRatio),
	abs($desiredRatio - $otherRatio1),
	abs($desiredRatio - $otherRatio2),
	abs($desiredRatio - $otherRatio3),
	abs($desiredRatio - $otherRatio4),
);

We test if the ratio of the image is the closest possible to the desired ratio. We try with width or height changed by 1 pixel, and if we were the closest possible, these other 4 cases will have ratios that are further to the desired ratio.

This approach seems to be correct, but I hope there is some way to simplify the code.

@vlakoff
Copy link
Contributor

vlakoff commented May 8, 2024

The above snippet fails with a 1366x768 image, because 1365x768 would be actually closer to the desired 16/9 ratio.

So, I tried another approach: providing the actual width, or the height, calculate the other dimension using the desired ratio, and test if the result matches the actual other dimension. For instance:

Desiring a 16/9 ratio:

  • an image with a 1366 px width should have a 768.375 px height (calculated by $width * denominator / $numerator), which is then rounded to 768 px.
  • an image with a 768 px height should have a 1365.333 px width (calculated by $height * numerator / $denominator), which is then rounded to 1365 px.

Though, note how our 1366x768 image succeeds using the former calculation, but fails using the latter!

A solution could be, when the other dimension is between two integers (i.e. the image can't perfectly achieve the desired ratio), to accept both the lower and the upper integer. But this would allow for too much imprecision with small images, for instance, desiring a 16/9 ratio with an image of 5 px width, we should really have a 3 px height, not 2 px.

@vlakoff
Copy link
Contributor

vlakoff commented May 8, 2024

(still thinking of loud, hope it doesn't bother you)

Considering the pitfalls I exposed in my above post, I figured out this approach:

$calculatedHeight = round($width * $denominator / $numerator);
$calculatedWidth = round($height * $numerator / $denominator);

if ($height === $calculatedHeight || $width === $calculatedWidth) {
    // Pass
}

If at least one of the two calculations matches, we're good. As shown previously, a correct image could have only one calculation that matches. Of course, it is possible that both calculations match. And contrarily, if no calculation matches, we are certain there exists a more appropriate width-height pair.

Nervertheless, I figured out another issue: if a calculated width (or height) has a ".5" decimal (e.g. 42.5), there is no particular reason to round it up, i.e. it should be equally acceptable to round it down. Sadly, supporting that case leads to a more complicated code:

$calculatedHeight = $width * $denominator / $numerator;
$calculatedWidth = $height * $numerator / $denominator;

if (
    $height === round($calculatedHeight, 0, PHP_ROUND_HALF_UP)
    || $height === round($calculatedHeight, 0, PHP_ROUND_HALF_DOWN)
    || $width === round($calculatedWidth, 0, PHP_ROUND_HALF_UP)
    || $width === round($calculatedWidth, 0, PHP_ROUND_HALF_DOWN)
) {
    // Pass
}

@ahmedbally
Copy link
Contributor Author

I see your concern but i tried to find an equation to solve almost of ratios, without breaking changes
I thinks of almost all these cases of small images and large images
Your solution may need more tests in some images there is over 4 pixel for difference
You can create pull request with your solution

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

Successfully merging this pull request may close these issues.

3 participants