Bug 232013

Summary: Integer interpolation in animations should be rounded towards positive infinity, not away from zero
Product: WebKit Reporter: Joonghun Park <jh718.park>
Component: AnimationsAssignee: Joonghun Park <jh718.park>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, clopez, cmarcelo, darin, dino, ews-watchlist, graouts, ntim, simon.fraser, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://github.com/web-platform-tests/wpt/pull/47333
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch for landing
none
Patch
ews-feeder: commit-queue-
Patch
none
Add tests for overflow check
none
Patch none

Joonghun Park
Reported 2021-10-20 08:09:59 PDT
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.
Attachments
Patch (10.16 KB, patch)
2021-10-20 08:17 PDT, Joonghun Park
ews-feeder: commit-queue-
Patch (10.14 KB, patch)
2021-10-20 08:29 PDT, Joonghun Park
no flags
Patch for landing (10.21 KB, patch)
2021-10-20 17:25 PDT, Joonghun Park
no flags
Patch (11.01 KB, patch)
2021-10-21 00:26 PDT, Joonghun Park
ews-feeder: commit-queue-
Patch (10.84 KB, patch)
2021-10-21 08:01 PDT, Joonghun Park
no flags
Add tests for overflow check (15.91 KB, patch)
2021-10-22 08:31 PDT, Joonghun Park
no flags
Patch (16.01 KB, patch)
2021-10-22 15:15 PDT, Joonghun Park
no flags
Joonghun Park
Comment 1 2021-10-20 08:17:12 PDT
EWS Watchlist
Comment 2 2021-10-20 08:18:37 PDT
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
Comment 3 2021-10-20 08:29:04 PDT
Darin Adler
Comment 4 2021-10-20 10:46:01 PDT
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 5 2021-10-20 17:14:19 PDT
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
Comment 6 2021-10-20 17:25:35 PDT
Created attachment 441963 [details] Patch for landing
Darin Adler
Comment 7 2021-10-20 18:03:35 PDT
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
Comment 8 2021-10-20 18:05:33 PDT
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
Comment 9 2021-10-20 19:02:59 PDT
(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
Comment 10 2021-10-20 19:08:19 PDT
Additionally, I left |roundTowardsPositiveInfinity| in MathExtras.h for possible future usage of that function.
Joonghun Park
Comment 11 2021-10-20 19:24:48 PDT
(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
Comment 12 2021-10-21 00:26:16 PDT
Joonghun Park
Comment 13 2021-10-21 05:23:35 PDT
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
Comment 14 2021-10-21 08:01:27 PDT
Darin Adler
Comment 15 2021-10-21 11:03:12 PDT
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
Comment 16 2021-10-22 08:31:28 PDT
Created attachment 442165 [details] Add tests for overflow check
Joonghun Park
Comment 17 2021-10-22 08:35:25 PDT
(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
Comment 18 2021-10-22 14:11:10 PDT
Looks good.
EWS
Comment 19 2021-10-22 14:34:28 PDT
/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
Comment 20 2021-10-22 15:15:36 PDT
EWS
Comment 21 2021-10-22 16:22:05 PDT
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
Comment 22 2021-10-22 16:23:22 PDT
Tim Nguyen (:ntim)
Comment 23 2024-07-29 10:35:24 PDT
Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/47333
Note You need to log in before you can comment on or make changes to this bug.