Bug 159038 - requestFrameAnimation() callback timestamp should be very close to Performance.now()
Summary: requestFrameAnimation() callback timestamp should be very close to Performanc...
Status: RESOLVED FIXED
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
: 115333 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-06-22 14:23 PDT by Said Abou-Hallawa
Modified: 2016-06-23 15:46 PDT (History)
5 users (show)

See Also:


Attachments
Testcase (1.60 KB, text/html)
2016-06-22 14:35 PDT, Simon Fraser (smfr)
no flags Details
Patch (5.90 KB, patch)
2016-06-22 14:57 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (30.80 KB, patch)
2016-06-23 11:47 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (30.82 KB, patch)
2016-06-23 12:24 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (32.51 KB, patch)
2016-06-23 14:47 PDT, Said Abou-Hallawa
no flags 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 2016-06-22 14:23:34 PDT
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.
Comment 1 Said Abou-Hallawa 2016-06-22 14:24:33 PDT
<rdar://problem/26933562>
Comment 2 Simon Fraser (smfr) 2016-06-22 14:35:22 PDT
Created attachment 281869 [details]
Testcase
Comment 3 Said Abou-Hallawa 2016-06-22 14:57:55 PDT
Created attachment 281875 [details]
Patch
Comment 4 Simon Fraser (smfr) 2016-06-22 16:01:25 PDT
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.
Comment 5 Simon Fraser (smfr) 2016-06-22 16:33:16 PDT
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).
Comment 6 Said Abou-Hallawa 2016-06-23 11:47:30 PDT
Created attachment 281921 [details]
Patch
Comment 7 Said Abou-Hallawa 2016-06-23 12:24:36 PDT
Created attachment 281927 [details]
Patch
Comment 8 Simon Fraser (smfr) 2016-06-23 13:23:52 PDT
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.
Comment 9 Said Abou-Hallawa 2016-06-23 14:47:51 PDT
Created attachment 281933 [details]
Patch
Comment 10 Said Abou-Hallawa 2016-06-23 14:53:49 PDT
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.
Comment 11 Said Abou-Hallawa 2016-06-23 14:56:42 PDT
*** Bug 115333 has been marked as a duplicate of this bug. ***
Comment 12 WebKit Commit Bot 2016-06-23 15:46:42 PDT
Comment on attachment 281933 [details]
Patch

Clearing flags on attachment: 281933

Committed r202399: <http://trac.webkit.org/changeset/202399>
Comment 13 WebKit Commit Bot 2016-06-23 15:46:46 PDT
All reviewed patches have been landed.  Closing bug.