Bug 210990 – Rendering update steps should use Seconds for the timestamps

• Component: Animations
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 397499 [details]
Patch

[Said Abou-Hallawa]

Created attachment 397506 [details]
Patch

[Said Abou-Hallawa]

Created attachment 397542 [details]
Patch

[Said Abou-Hallawa]

Created attachment 397548 [details]
Patch

[Said Abou-Hallawa]

Created attachment 397552 [details]
Patch

[Daniel Bates]

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.

[Said Abou-Hallawa]

Created attachment 397634 [details]
Patch

[EWS]

Committed r260736: <https://trac.webkit.org/changeset/260736>

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

[Radar WebKit Bug Importer]

<rdar://problem/62416809>

[Truitt Savell]

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

[Said Abou-Hallawa]

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

[Said Abou-Hallawa]

Reopen to attach a new patch.

[Said Abou-Hallawa]

Created attachment 397873 [details]
Patch

[Simon Fraser (smfr)]

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?

[Said Abou-Hallawa]

Created attachment 397912 [details]
Patch

[Darin Adler]

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.

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

I fixed the flakiness by putting std::round back in serviceRequestAnimationFrameCallbacks() since the Seconds::roundedMilliseconds() change is taking longer than I expected.

[Said Abou-Hallawa]

The flakiness is fixed in:

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