requestFrameAnimation() callback timestamp should be very close to Performance.now()

[Said Abou-Hallawa]

The current implementation of requestFrameAnimation() passes the sum of timeUntilOutput and Performance.now() to the JS callback. timeUntilOutput is the difference between the outputTime and the now time which are passed to the CVDisplayLink callback. This addition causes the timestamp which is passed to the requestFrameAnimation() callback to be larger than the Performance.now() by almost 33 ms in the future.

[Said Abou-Hallawa]

<rdar://problem/26933562>

[Simon Fraser (smfr)]

Created attachment 281869 [details]
Testcase

[Said Abou-Hallawa]

Created attachment 281875 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 281875 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        * platform/graphics/mac/DisplayRefreshMonitorMac.cpp:
> +        (WebCore::displayLinkCallback):
> +        (WebCore::DisplayRefreshMonitorMac::displayLinkFired):

You need to fix DisplayRefreshMonitorIOS too.

> Source/WebCore/platform/graphics/mac/DisplayRefreshMonitorMac.cpp:95
> +    setMonotonicAnimationStartTime(monotonicallyIncreasingTime());

I don't think this is the right level at which to set the time; it only fixes the Mac DisplayRefreshMonitor.

I think you could remove passing the timestamps all the way down to ScriptedAnimationController::serviceScriptedAnimations(), and grab the current time there. Also, you should grab it via Performance::now(), not just monotonicallyIncreasingTime(), since it should be quantized in the same way.

[Simon Fraser (smfr)]

I would also like to see a test that checks that multiple rAF callbacks fired at the same time get the same timestamp (if we don't have one already).

[Said Abou-Hallawa]

Created attachment 281921 [details]
Patch

[Said Abou-Hallawa]

Created attachment 281927 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 281927 [details]
Patch

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

> Source/WebCore/dom/ScriptedAnimationController.cpp:152
> +    double legacyHighResNowMs = 1000 * m_document->loader()->timing().windowTimeToPseudoWallTime(timestamp);

I don't know if it's worth preserving this legacy behavior. If it is, I would rename windowTimeToPseudoWallTime() to something like timeForLegacyRequestAnimationFrame() (or move the computation to here).

> Source/WebCore/page/DOMWindow.cpp:757
> +    if (!timestamp && document()) {
> +        DocumentLoader* loader = document()->loader();
> +        timestamp = loader ? loader->timing().monotonicTimeToZeroBasedDocumentTime(monotonicallyIncreasingTime()) : 0;
> +    }

Is timestamp only 0 when ENABLE(WEB_TIMING) is off? If so, I'd just #ifdef the entire block.

> Source/WebCore/page/FrameView.cpp:2832
> +void FrameView::serviceScriptedAnimations(double timestamp)

Are you passing the timestamp through here just for coordinated graphics? I'm not sure why we'd leave their rAF callbacks with a different behavior.

> LayoutTests/animations/animation-callback-timestamp-expected.txt:3
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE

Would be nice if the test result said something like 'PASS: requestAnimationFrame callback time was within blah blah"

> LayoutTests/animations/animation-multiple-callbacks-timestamp-expected.txt:3
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE

Ditto.

[Said Abou-Hallawa]

Created attachment 281933 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 281927 [details]
Patch

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

>> Source/WebCore/dom/ScriptedAnimationController.cpp:152
>> +    double legacyHighResNowMs = 1000 * m_document->loader()->timing().windowTimeToPseudoWallTime(timestamp);
> 
> I don't know if it's worth preserving this legacy behavior. If it is, I would rename windowTimeToPseudoWallTime() to something like timeForLegacyRequestAnimationFrame() (or move the computation to here).

The function DocumentLoadTiming::referenceWallTime() was added and the calculation was moved to here.

>> Source/WebCore/page/DOMWindow.cpp:757
>> +    }
> 
> Is timestamp only 0 when ENABLE(WEB_TIMING) is off? If so, I'd just #ifdef the entire block.

The entire function is now surrounded by  #if #endif directives which is similar to HTMLMediaElement.cpp had before.

>> Source/WebCore/page/FrameView.cpp:2832
>> +void FrameView::serviceScriptedAnimations(double timestamp)
> 
> Are you passing the timestamp through here just for coordinated graphics? I'm not sure why we'd leave their rAF callbacks with a different behavior.

The timestamp is not passed to FrameView::serviceScriptedAnimations() anymore.

>> LayoutTests/animations/animation-callback-timestamp-expected.txt:3
>> +TEST COMPLETE
> 
> Would be nice if the test result said something like 'PASS: requestAnimationFrame callback time was within blah blah"

Fixed.

>> LayoutTests/animations/animation-multiple-callbacks-timestamp-expected.txt:3
>> +TEST COMPLETE
> 
> Ditto.

Fixed.

[Said Abou-Hallawa]

*** Bug 115333 has been marked as a duplicate of this bug. ***

[WebKit Commit Bot]

Comment on attachment 281933 [details]
Patch

Clearing flags on attachment: 281933

Committed r202399: <http://trac.webkit.org/changeset/202399>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.