Bug 232013 – Integer interpolation in animations should be rounded towards positive infinity, not away from zero

• Component: Animations
• Status: RESOLVED FIXED

[Joonghun Park]

According to the spec, https://drafts.csswg.org/css-values-4/#combine-integers,
integer interpolation should be rounded towards positive infinity, not away from zero.

[Joonghun Park]

Created attachment 441880 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Joonghun Park]

Created attachment 441884 [details]
Patch

[Darin Adler]

Comment on attachment 441884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=441884&action=review

> Source/WTF/wtf/MathExtras.h:147
> +inline double roundHalfTowardsPositiveInfinity(double value)
> +{
> +    return std::floor(value + 0.5);
> +}
> +
> +inline float roundHalfTowardsPositiveInfinity(float value)
> +{
> +    return std::floor(value + 0.5f);
> +}

I don’t think these function names need the word "half" in them. I also suggest using the one-line format that the functions both above and below use.

[Joonghun Park]

Comment on attachment 441884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=441884&action=review

>> Source/WTF/wtf/MathExtras.h:147
>> +}
> 
> I don’t think these function names need the word "half" in them. I also suggest using the one-line format that the functions both above and below use.

Ok, I will change these functions to one-line format and rename it to not include 'half' in the next patch. Thank you for your review:)

[Joonghun Park]

Created attachment 441963 [details]
Patch for landing

[Darin Adler]

Comment on attachment 441963 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=441963&action=review

> Source/WebCore/platform/animation/AnimationUtilities.h:46
> +    return static_cast<int>(roundTowardsPositiveInfinity(static_cast<double>(from) + static_cast<double>(to - from) * context.progress));

Three unimportant thoughts:

1) Not new to this patch, but these two static_cast<double> aren’t needed and have no effect. Multiplying an int by a double in C++ results in a double. And adding an int to a double results in a double. So the fact that context.progress is a double already converts everything to double without any explicit casts.

2) Not new to this patch, but the static_cast<int> also doesn’t have any effect on the value returned. I think it can be omitted too, but maybe it’s quieting some kind of 64-to-32-bit conversion warning, and if so it’s OK to leave it in.

3) It’s unnecessary to call std::floor after adding 0.5 before converting to an int since converting a double to an int performs a floor operation as part of the conversion. It’s more correct and elegant that the roundTowardsPositiveInfinity does a std::floor, and could be important elsewhere when the operation is not immediately followed by conversion to integer. But here, it costs a tiny bit of performance and sadly it seems likely the compiler probably can’t figure that out and compile it out even though everything is inlined, partly because we know that progress is always in the range [0,1], but the compiler does not know that.

No change is needed, but if it was me, I would at least remove the unneeded static_cast<double> when touching this code.

[Darin Adler]

It also occurs to me that this function won’t give a correct result if `to` is the maximum integer and `from` is the minimum integer. To make that case work correctly, we’d need to convert either `to` or `from` to a double *before* subtracting them. I wonder if that case can be tested in a WPT test, or if there’s some limit on range that prevents it from occurring.

[Joonghun Park]

(In reply to Darin Adler from comment #7)
> Comment on attachment 441963 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441963&action=review
> 
> > Source/WebCore/platform/animation/AnimationUtilities.h:46
> > +    return static_cast<int>(roundTowardsPositiveInfinity(static_cast<double>(from) + static_cast<double>(to - from) * context.progress));
> 
> Three unimportant thoughts:
> 
> 1) Not new to this patch, but these two static_cast<double> aren’t needed
> and have no effect. Multiplying an int by a double in C++ results in a
> double. And adding an int to a double results in a double. So the fact that
> context.progress is a double already converts everything to double without
> any explicit casts.
> 
Ok, I removed the unnecessary static_cast<double>s.

> 2) Not new to this patch, but the static_cast<int> also doesn’t have any
> effect on the value returned. I think it can be omitted too, but maybe it’s
> quieting some kind of 64-to-32-bit conversion warning, and if so it’s OK to
> leave it in.
> 
As you commented, it seems that explicit conversion would be good to quiet possble implicit bit down conversion warning, so I will leave it in.

> 3) It’s unnecessary to call std::floor after adding 0.5 before converting to
> an int since converting a double to an int performs a floor operation as
> part of the conversion. It’s more correct and elegant that the
> roundTowardsPositiveInfinity does a std::floor, and could be important
> elsewhere when the operation is not immediately followed by conversion to
> integer. But here, it costs a tiny bit of performance and sadly it seems
> likely the compiler probably can’t figure that out and compile it out even
> though everything is inlined, partly because we know that progress is always
> in the range [0,1], but the compiler does not know that.
> 
Reading your concern, I changed the code as below.

inline int blend(int from, int to, const BlendingContext& context)
{  
    // This static_cast<int> followed by adding 0.5 works as same with
    // roundTowardsPositiveInfinity in MathExtras.h,
    // but allows us to omit std::floor step.
    return static_cast<int>((from + (to - from) * context.progress) + 0.5);
}

I originally appended "constexpr" to roundTowardsPositiveInfinity, but it encountered compiler error, so I removed it and it seems that it caused you to concern regarding this.

> No change is needed, but if it was me, I would at least remove the unneeded
> static_cast<double> when touching this code.

Ok, I applied it in the next patch. I only focused on rounding towards positive integer so I missed out the details you pointed out. Thank you for your review, Darin.

[Joonghun Park]

Additionally, I left |roundTowardsPositiveInfinity| in MathExtras.h for possible future usage of that function.

[Joonghun Park]

(In reply to Darin Adler from comment #8)
> It also occurs to me that this function won’t give a correct result if `to`
> is the maximum integer and `from` is the minimum integer. To make that case
> work correctly, we’d need to convert either `to` or `from` to a double
> *before* subtracting them. I wonder if that case can be tested in a WPT
> test, or if there’s some limit on range that prevents it from occurring.

I was checking if any spec exists specifying how the <integer> range should be,
but it seems that there is no spec for it anymore as described in
https://developer.mozilla.org/en-US/docs/Web/CSS/integer.

So it seems ambiguous to write wpt integer max/min range check,
but I could change the code as below in the next patchset.

inline int blend(int from, int to, const BlendingContext& context)
{  
    // This static_cast<int> followed by adding 0.5 works as same with
    // roundTowardsPositiveInfinity in MathExtras.h,
    // but allows us to omit std::floor step.
    return static_cast<int>((from + (static_cast<double>(to) - from) * context.progress) + 0.5);
}

I will apply this change in the next patchset.

[Joonghun Park]

Created attachment 441993 [details]
Patch

[Joonghun Park]

3) It’s unnecessary to call std::floor after adding 0.5 before converting to an int since converting a double to an int performs a floor operation as part of the conversion. 

It seems that it would be good to use roundTowardsPositiveInfinity including std::floor, because for positive integer int works same as converting double to an int, but for negative integer its rounding direction is opposite each other.
e.g. std::floor(-0.6) === -1 but static_cast<int>(-0.6) === 0, and it causes some layout test regression. I will use roundTowardsPositiveInfinity in blend in the next patchset and upload it.

[Joonghun Park]

Created attachment 442023 [details]
Patch

[Darin Adler]

Comment on attachment 442023 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=442023&action=review

> Source/WebCore/platform/animation/AnimationUtilities.h:49
> +    return static_cast<int>(roundTowardsPositiveInfinity(from + (static_cast<double>(to) - from) * context.progress));

OK to land it like this, but for the longer term, I really don’t like the idea of casting to double here in these functions to make the edge case work, yet not testing those edge cases. I understand that the specification doesn’t tell us what the legal range of values is, but even so we should create test cases, even if they are browser-specific at first. Eventually they should exist in WPT and the specification should be clear on this point. Real world compatibility often depends on edge case tests and stress tests. Edge cases are *accidentally* exercised by website developers, and if not consistent in behavior, they turn into interoperability bugs where something works only on the most-tested browser and not the others.

[Joonghun Park]

Created attachment 442165 [details]
Add tests for overflow check

[Joonghun Park]

(In reply to Darin Adler from comment #15)
> Comment on attachment 442023 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442023&action=review
> 
> > Source/WebCore/platform/animation/AnimationUtilities.h:49
> > +    return static_cast<int>(roundTowardsPositiveInfinity(from + (static_cast<double>(to) - from) * context.progress));
> 
> OK to land it like this, but for the longer term, I really don’t like the
> idea of casting to double here in these functions to make the edge case
> work, yet not testing those edge cases. I understand that the specification
> doesn’t tell us what the legal range of values is, but even so we should
> create test cases, even if they are browser-specific at first. Eventually
> they should exist in WPT and the specification should be clear on this
> point. Real world compatibility often depends on edge case tests and stress
> tests. Edge cases are *accidentally* exercised by website developers, and if
> not consistent in behavior, they turn into interoperability bugs where
> something works only on the most-tested browser and not the others.

Thank you for your comments. I added 2 overflow check tests in the latest patchset and I confirmed that it overflowed without the static_cast<double>(to) casting.
If the spec regarding <integer> range is settled, then we could add wit tests for it.
Could you please review the tests for overflow check? If it looks ok, then I could land this change.

[Darin Adler]

Looks good.

[EWS]

/Volumes/Data/worker/Commit-Queue/build/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Joonghun Park]

Created attachment 442202 [details]
Patch

[EWS]

Committed r284725 (243437@main): <https://commits.webkit.org/243437@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442202 [details].

[Radar WebKit Bug Importer]

<rdar://problem/84567974>

[Tim Nguyen (:ntim)]

Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/47333