Bug 210990 - Rendering update steps should use Seconds for the timestamps
Summary: Rendering update steps should use Seconds for the timestamps
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-24 13:28 PDT by Said Abou-Hallawa
Modified: 2020-04-28 22:39 PDT (History)
24 users (show)

See Also:


Attachments
Patch (15.19 KB, patch)
2020-04-24 13:34 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (16.63 KB, patch)
2020-04-24 14:07 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (24.65 KB, patch)
2020-04-24 22:58 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (24.67 KB, patch)
2020-04-25 00:39 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (25.15 KB, patch)
2020-04-25 01:36 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (25.14 KB, patch)
2020-04-26 16:43 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (11.85 KB, patch)
2020-04-28 12:37 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (6.17 KB, patch)
2020-04-28 17:33 PDT, Said Abou-Hallawa
sabouhallawa: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2020-04-24 13:28:00 PDT
It has been confusing to deal with DOMHighResTimeStamp since the units can be not deduced easily. Some places use it as seconds other places use it as milliseconds. We will change DOMWindow::nowTimestamp() to return Seconds and change the callers accordingly.
Comment 1 Said Abou-Hallawa 2020-04-24 13:34:36 PDT
Created attachment 397499 [details]
Patch
Comment 2 Said Abou-Hallawa 2020-04-24 14:07:15 PDT
Created attachment 397506 [details]
Patch
Comment 3 Said Abou-Hallawa 2020-04-24 22:58:00 PDT
Created attachment 397542 [details]
Patch
Comment 4 Said Abou-Hallawa 2020-04-25 00:39:01 PDT
Created attachment 397548 [details]
Patch
Comment 5 Said Abou-Hallawa 2020-04-25 01:36:23 PDT
Created attachment 397552 [details]
Patch
Comment 6 Daniel Bates 2020-04-25 08:14:13 PDT
Comment on attachment 397552 [details]
Patch

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

Patch looks good.

> Source/WebCore/dom/Document.cpp:7679
> +                    (*timestamp).milliseconds(),

Ok as is. No change needed. Optimal solution is to use -> because it is more idiomatic and aestically pleasing.
Comment 7 Said Abou-Hallawa 2020-04-26 16:43:51 PDT
Created attachment 397634 [details]
Patch
Comment 8 EWS 2020-04-26 18:03:01 PDT
Committed r260736: <https://trac.webkit.org/changeset/260736>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397634 [details].
Comment 9 Radar WebKit Bug Importer 2020-04-26 18:04:12 PDT
<rdar://problem/62416809>
Comment 10 Truitt Savell 2020-04-27 13:30:31 PDT
It looks like the changes in https://trac.webkit.org/changeset/260736/webkit

broke imported/w3c/web-platform-tests/web-animations/timing-model/timelines/document-timelines.html

history:
https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fweb-animations%2Ftiming-model%2Ftimelines%2Fdocument-timelines.html

Diff:
--- /Volumes/Data/slave/catalina-release-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/web-animations/timing-model/timelines/document-timelines-expected.txt
+++ /Volumes/Data/slave/catalina-release-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/web-animations/timing-model/timelines/document-timelines-actual.txt
@@ -1,4 +1,4 @@
 
-PASS Document timelines report current time relative to navigationStart 
+FAIL Document timelines report current time relative to navigationStart assert_equals: The current time matches requestAnimationFrame time expected 13.000000000000002 but got 13
 PASS Child frames do not report negative initial times 
 

I was able to reproduce this with 

run-webkit-tests --iterations 2000 --exit-after-n-failures 1 --exit-after-n-crashes-or-timeouts 1 --debug-rwt-logging --no-retry --force --no-build -f imported/w3c/web-platform-tests/web-animations/timing-model/timelines/document-timelines.html
Comment 11 Said Abou-Hallawa 2020-04-28 11:09:50 PDT
We used to convert Performance::now() which is milliseconds to seconds in DOMWindow::nowTimestamp() and then we were converting these seconds back to milliseconds in serviceRequestAnimationFrameCallbacks(). So I thought we needed to round the timestamp to the nearest milliseconds.

When converting the timestamps from double to Seconds, I thought we do not need this rounding anymore. It turned out AnimationTimeline::bindingsCurrentTime() does similar rounding even with Seconds when it calls secondsToWebAnimationsAPITime().
Comment 12 Said Abou-Hallawa 2020-04-28 11:10:19 PDT
Reopen to attach a new patch.
Comment 13 Said Abou-Hallawa 2020-04-28 12:37:48 PDT
Created attachment 397873 [details]
Patch
Comment 14 Simon Fraser (smfr) 2020-04-28 15:48:31 PDT
Comment on attachment 397873 [details]
Patch

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

> Source/WTF/wtf/Seconds.h:62
> +        auto milliseconds = std::round(this->microseconds()) / 1000;
> +        if (milliseconds == -0)
> +            return 0;
> +        return milliseconds;

Now that I see that this is really a Web Animations-specific behavior, I'm not sure it belongs here. I think a native roundedMilliseconds() that just returns std::round(milliseconds()) here is OK, but the -0 check is odd.

> Source/WebCore/animation/AnimationEffect.cpp:337
> -    computedTiming.delay = secondsToWebAnimationsAPITime(m_delay);
> -    computedTiming.endDelay = secondsToWebAnimationsAPITime(m_endDelay);
> +    computedTiming.delay = m_delay.roundedMilliseconds();
> +    computedTiming.endDelay = m_endDelay.roundedMilliseconds();

I think you should keep secondsToWebAnimationsAPITime() and have it call roundedMilliseconds()

> Source/WebCore/animation/AnimationTimeline.cpp:94
>  Optional<double> AnimationTimeline::bindingsCurrentTime()

Should these return DOMHighResTimestamp (in a later patch)?

> Source/WebCore/animation/DeclarativeAnimation.cpp:353
>      auto event = createEvent(eventType, time, pseudoId, timelineTime);

So event time is in seconds I guess. So confusing.

> Source/WebCore/animation/WebAnimation.cpp:364
>  Optional<double> WebAnimation::bindingsCurrentTime() const

DOMHighResTimestamp?
Comment 15 Said Abou-Hallawa 2020-04-28 17:33:36 PDT
Created attachment 397912 [details]
Patch
Comment 16 Darin Adler 2020-04-28 17:40:05 PDT
Comment on attachment 397912 [details]
Patch

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

> Source/WTF/wtf/Seconds.h:57
> +    double roundedMilliseconds() const { return std::round(microseconds()) / 1000; }

This is milliseconds rounded to the nearest microsecond.

Rounding to the nearest millisecond would be:

    return std::round(microseconds() * 0.001);

Also, I think we prefer "* 0.001" over "/ 1000".

So I think this needs a better name.
Comment 17 Said Abou-Hallawa 2020-04-28 17:42:25 PDT
Comment on attachment 397873 [details]
Patch

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

>> Source/WTF/wtf/Seconds.h:62
>> +        return milliseconds;
> 
> Now that I see that this is really a Web Animations-specific behavior, I'm not sure it belongs here. I think a native roundedMilliseconds() that just returns std::round(milliseconds()) here is OK, but the -0 check is odd.

I removed the -0 check, but

Why is it Web Animations-specific behavior? The specs https://drafts.csswg.org/web-animations-1/#precision-of-time-values does not say we have to do it for Web Animations. Timestamps should be positive but the result of a floating point operations can be -0 which is also equal to +0. So ensuring the timestamp is always +ve is a good thing and it does not affect the result. Besides the test imported/w3c/web-platform-tests/web-animations/timing-model/timelines/document-timelines.html checks whether the timestamp of the rAF is equal to the currentTime of the DocumentTimeline. So how we guarantee the two times are equal if we round them differently?

>> Source/WebCore/animation/AnimationEffect.cpp:337
>> +    computedTiming.endDelay = m_endDelay.roundedMilliseconds();
> 
> I think you should keep secondsToWebAnimationsAPITime() and have it call roundedMilliseconds()

Done.

>> Source/WebCore/animation/AnimationTimeline.cpp:94
>>  Optional<double> AnimationTimeline::bindingsCurrentTime()
> 
> Should these return DOMHighResTimestamp (in a later patch)?

Done.

>> Source/WebCore/animation/WebAnimation.cpp:364
>>  Optional<double> WebAnimation::bindingsCurrentTime() const
> 
> DOMHighResTimestamp?

Done.
Comment 18 Said Abou-Hallawa 2020-04-28 18:01:17 PDT
Comment on attachment 397912 [details]
Patch

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

>> Source/WTF/wtf/Seconds.h:57
>> +    double roundedMilliseconds() const { return std::round(microseconds()) / 1000; }
> 
> This is milliseconds rounded to the nearest microsecond.
> 
> Rounding to the nearest millisecond would be:
> 
>     return std::round(microseconds() * 0.001);
> 
> Also, I think we prefer "* 0.001" over "/ 1000".
> 
> So I think this needs a better name.

You are right. This calculation is not right even for exposing the timestamps to JavaScript. The timestamps are reduced resolution times to the nearest milliseconds; see Performance::now(). So it does not make sense to round them later to the nearest microsecond.
Comment 19 Said Abou-Hallawa 2020-04-28 22:38:37 PDT
I fixed the flakiness by putting std::round back in serviceRequestAnimationFrameCallbacks() since the Seconds::roundedMilliseconds() change is taking longer than I expected.
Comment 20 Said Abou-Hallawa 2020-04-28 22:39:02 PDT
The flakiness is fixed in:

Committed r260870: <https://trac.webkit.org/changeset/260870>