Bug 66683 - Expose high-resolution on requestAnimationFrame callback
Summary: Expose high-resolution on requestAnimationFrame callback
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nat Duca
URL:
Keywords:
Depends on: 58354 66684 84912
Blocks: 84386
  Show dependency treegraph
 
Reported: 2011-08-22 10:39 PDT by Nat Duca
Modified: 2012-10-11 19:45 PDT (History)
17 users (show)

See Also:


Attachments
Patch (9.05 KB, patch)
2011-08-22 10:49 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Patch (8.98 KB, patch)
2012-04-21 15:04 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (6.17 MB, application/zip)
2012-04-21 16:59 PDT, WebKit Review Bot
no flags Details
Patch (17.27 KB, patch)
2012-04-21 20:02 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (6.04 MB, application/zip)
2012-04-23 15:35 PDT, WebKit Review Bot
no flags Details
Patch (18.14 KB, patch)
2012-04-25 00:24 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
One that builds, hopefully (20.01 KB, patch)
2012-04-25 00:52 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
debugging via ews (20.22 KB, patch)
2012-04-25 02:37 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
fix mac (20.71 KB, patch)
2012-04-25 16:22 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Patch for landing (20.71 KB, patch)
2012-04-27 16:38 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Patch (21.45 KB, patch)
2012-10-11 15:29 PDT, James Simonsen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nat Duca 2011-08-22 10:39:52 PDT
Use monotonic clock for requestAnimationFrame's time parameter
Comment 1 Nat Duca 2011-08-22 10:49:39 PDT
Created attachment 104695 [details]
Patch
Comment 2 Nat Duca 2011-08-22 10:51:31 PDT
Putting this up mainly as a proof of concept and to shake out any conceptual issues that arise from making this change. I definitely think we should let the topic of clocks work its way through the perf wg before committing this..

On the bright side, raf tests show exactly 60fps with this patch. This makes me happy. :)
Comment 3 James Robinson 2011-08-22 11:11:51 PDT
Comment on attachment 104695 [details]
Patch

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

R- for lack of timebase.

I'll start up the standards discussion on public-web-perf@ about these changes.

> Source/WebCore/ChangeLog:8
> +        https://bugs.webkit.org/show_bug.cgi?id=66683
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * dom/Document.cpp:

need some words here - explain what's changing.  In particular, highlight that the type of the parameter is changing as well

> Source/WebKit/chromium/src/WebViewImpl.cpp:1058
> +        frameBeginTime = webKitClient()->monotonicallyIncreasingTime();

this doesn't have the right timebase - we need to set the zero explicitly (probably to the root level navigation time).
Comment 4 James Robinson 2011-08-22 11:12:14 PDT
Hey James, how can we pick up the correct zero time?  Where should that be stored?
Comment 5 Nat Duca 2011-08-22 11:16:36 PDT
Totally agreed. :)
Comment 6 James Simonsen 2011-08-22 11:28:30 PDT
(In reply to comment #4)
> Hey James, how can we pick up the correct zero time?  Where should that be stored?

Not yet. It'll be added in bug 58354.

You'll access it via:

document->loader()->timing()->convertMonotonicTimeToDocumentTime()

Perhaps we should make this more accessible if it's widely used.
Comment 7 James Simonsen 2011-08-22 11:40:07 PDT
Comment on attachment 104695 [details]
Patch

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

> Source/WebCore/dom/ScriptedAnimationController.cpp:97
> +    // Report times to callbacks in terms of milliseconds.

You might mention that this is because JavaScript times are in ms. Otherwise, the code itself is self-documenting and this comment isn't needed.
Comment 8 Vincent Scheib 2012-03-21 20:21:39 PDT
Does this have a chance of moving forward? Is it blocked on technical issues?
Comment 9 Nat Duca 2012-03-21 22:16:53 PDT
(In reply to comment #8)
> Does this have a chance of moving forward? Is it blocked on technical issues?

Nope, just free time to finish the work I started.
Comment 10 Nat Duca 2012-04-21 15:04:52 PDT
Created attachment 138255 [details]
Patch
Comment 11 Nat Duca 2012-04-21 15:06:08 PDT
Comment on attachment 138255 [details]
Patch

Ugh, wrong patch.
Comment 12 WebKit Review Bot 2012-04-21 16:58:56 PDT
Comment on attachment 138255 [details]
Patch

Attachment 138255 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12477534

New failing tests:
fast/dom/Window/window-properties-performance.html
Comment 13 WebKit Review Bot 2012-04-21 16:59:02 PDT
Created attachment 138259 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 14 Nat Duca 2012-04-21 20:02:46 PDT
Created attachment 138263 [details]
Patch
Comment 15 Build Bot 2012-04-21 20:58:42 PDT
Comment on attachment 138263 [details]
Patch

Attachment 138263 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12480512
Comment 16 James Robinson 2012-04-23 10:58:31 PDT
Comment on attachment 138263 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +        This changes requestAnimationFrame's animationStartTime argument
> +        to be a high resolution DOM timestamp, per disucssion here:

I don't see any IDL change in here, so the value of the parameter is going to be of type DOMTimeStamp which the binding code may round to integer - did you check for that?

I think we want to clamp to something like 1/10th of a second for now, right?

> Source/WebCore/dom/ScriptedAnimationController.cpp:130
> +            callback->handleEvent(highResNow);

DOMTimeStamp is milliseconds (as is DOMHighResTimeStamp), it looks like this time is seconds.

> Source/WebCore/dom/ScriptedAnimationController.cpp:189
> +#if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)

this #if should be nested inside REQUEST_ANIMATION_FRAME_TIMER, not parallel (weird as that is)
Comment 17 WebKit Review Bot 2012-04-23 15:35:35 PDT
Comment on attachment 138263 [details]
Patch

Attachment 138263 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12525049

New failing tests:
fast/animation/request-animation-frame-timestamps-advance.html
Comment 18 WebKit Review Bot 2012-04-23 15:35:50 PDT
Created attachment 138438 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 19 Nat Duca 2012-04-25 00:24:03 PDT
Created attachment 138747 [details]
Patch
Comment 20 Nat Duca 2012-04-25 00:26:23 PDT
What's the bit about 1/10th? Does that apply to performance.now as well? My memory isn't what it used to be :)
Comment 21 Philippe Normand 2012-04-25 00:36:14 PDT
Comment on attachment 138747 [details]
Patch

Attachment 138747 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12521440
Comment 22 Early Warning System Bot 2012-04-25 00:41:31 PDT
Comment on attachment 138747 [details]
Patch

Attachment 138747 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12527485
Comment 23 Early Warning System Bot 2012-04-25 00:46:30 PDT
Comment on attachment 138747 [details]
Patch

Attachment 138747 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12478006
Comment 24 Nat Duca 2012-04-25 00:52:58 PDT
Created attachment 138752 [details]
One that builds, hopefully
Comment 25 Build Bot 2012-04-25 01:53:51 PDT
Comment on attachment 138752 [details]
One that builds, hopefully

Attachment 138752 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12528497
Comment 26 Nat Duca 2012-04-25 02:37:39 PDT
Created attachment 138769 [details]
debugging via ews
Comment 27 Build Bot 2012-04-25 03:37:38 PDT
Comment on attachment 138769 [details]
debugging via ews

Attachment 138769 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12521487
Comment 28 Nat Duca 2012-04-25 16:22:05 PDT
Created attachment 138893 [details]
fix mac
Comment 29 James Robinson 2012-04-25 17:05:55 PDT
(In reply to comment #20)
> What's the bit about 1/10th? Does that apply to performance.now as well? My memory isn't what it used to be :)

We had toyed with the idea of clamping the floating point values we expose to JS to the nearest 1/10th of a second to provide a bit of isolation from the differences in precision (not accuracy) of the underlying timesource on  different platforms and perhaps stave off some of the timing attack concerns.  I'm not sure if we reached any consensus on it, and I don't feel terribly strongly about it.
Comment 30 James Robinson 2012-04-25 17:11:14 PDT
(In reply to comment #29)
> (In reply to comment #20)
> > What's the bit about 1/10th? Does that apply to performance.now as well? My memory isn't what it used to be :)
> 
> We had toyed with the idea of clamping the floating point values we expose to JS to the nearest 1/10th of a second

1/10th of a MILLIsecond, rather.  1/10th of a second isn't useful for anyone of course :)
Comment 31 Nat Duca 2012-04-25 17:19:06 PDT
Re 1/10th, seems like @simonjam has the right bits of information in his head to make a yes/no call here. Wdyt?
Comment 32 James Simonsen 2012-04-25 17:32:13 PDT
(In reply to comment #31)
> Re 1/10th, seems like @simonjam has the right bits of information in his head to make a yes/no call here. Wdyt?

Even at 1ms resolution, we don't have platform parity. On Windows, we need to switch to QueryPerformanceCounter to guarantee that. But, QPC doesn't work properly on all systems. For some subset of users, we'll still have to fall back to nearest tick, which may be 15 ms.

Given that, and no red flags from security, I think it's okay to go as is. If anyone disagrees, I don't see any harm in clamping either though.
Comment 33 James Robinson 2012-04-26 13:23:22 PDT
(In reply to comment #32)
> (In reply to comment #31)
> > Re 1/10th, seems like @simonjam has the right bits of information in his head to make a yes/no call here. Wdyt?
> 
> Even at 1ms resolution, we don't have platform parity. On Windows, we need to switch to QueryPerformanceCounter to guarantee that. But, QPC doesn't work properly on all systems. For some subset of users, we'll still have to fall back to nearest tick, which may be 15 ms.
> 
> Given that, and no red flags from security, I think it's okay to go as is. If anyone disagrees, I don't see any harm in clamping either though.

That's convincing enough for me.
Comment 34 James Robinson 2012-04-26 13:31:31 PDT
Comment on attachment 138893 [details]
fix mac

R=me but please don't land until the 0 time for this time source is the document load start time.  I only want to change the semantics of the rAF callback argument once to make the transition as easy as possible on authors.  It's probably good to make sure window.performance.webkitNow() lands first as well.
Comment 35 Nat Duca 2012-04-27 16:38:13 PDT
Created attachment 139305 [details]
Patch for landing
Comment 36 WebKit Review Bot 2012-04-27 18:27:23 PDT
Comment on attachment 139305 [details]
Patch for landing

Clearing flags on attachment: 139305

Committed r115525: <http://trac.webkit.org/changeset/115525>
Comment 37 WebKit Review Bot 2012-04-27 18:27:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 38 Kenneth Russell 2012-05-01 15:11:17 PDT
Note: this patch seems to have broken Closure animations using requestAnimationFrame. See http://code.google.com/p/chromium/issues/detail?id=125575 . Looking through this patch, it looks harmless (still providing a number in milliseconds to the callback, but a floating-point number rather than an integer), so I am not sure whether this indicates a bug in Closure or a compatibility problem with this change.
Comment 39 Nat Duca 2012-05-02 00:20:53 PDT
I'll try to look into it. This is the "scary part" where we figure out if we can weather the storm of issues resulting from the change.

We're trying to move the w3c standard to pass a DOMHighResTimestamp to the raf callback even though this in the past was a DOMTimeStamp. This is a long term net good thing for the platform.

The challenge now is figuring out the issues that crop up, and figure out if the number of issues make it impossible to move the web to the new style raf.
Comment 40 Lars Knudsen 2012-05-07 05:40:47 PDT
in qt-wk2 MiniBrowser, requestAnimationFrame now returns strange numbers.  I didn't go into the code yet - but could this patch be the reason?

Please check:  http://dothisathome.com/scaryone/ that has some high negative value as the first readout (killing the animation) and this one:  http://dothisathome.com/DiceFPS/ - also giving some strange output (-1 FPS as first reading)
Comment 41 Nat Duca 2012-05-07 09:24:13 PDT
That was the intent. DOMHighResTimeStamp is a high resolution timestamp that is zero at page load and not convertable back and forth between DOMTimeStamp. The goal here was to allow pages to obtain the frame begin time at a high resolution than is currently possible.

Based on data we're getting back from the field, a surprisingly large number of devs have made the assumption that the callback returns DOMTimeStamp. When you make that assumption, things break. :)

I'm leaning toward reverting this patch and having a chat with WebPerf WG about how to proceeed.
Comment 42 Lars Knudsen 2012-05-07 10:25:00 PDT
(In reply to comment #41)
> That was the intent. DOMHighResTimeStamp is a high resolution timestamp that is zero at page load and not convertable back and forth between DOMTimeStamp. The goal here was to allow pages to obtain the frame begin time at a high resolution than is currently possible.

It sure would be nice with better timers for games and such.
 
> 
> Based on data we're getting back from the field, a surprisingly large number of devs have made the assumption that the callback returns DOMTimeStamp. When you make that assumption, things break. :)

Well - would it be possible to pass as a 2nd parameter (DOMTimeStamp being the first)? - then it wouldn't break much and still deliver the high resolution timers for the apps needing them.  I am assuming this is for fast and smooth canvas based animations primarily?

> 
> I'm leaning toward reverting this patch and having a chat with WebPerf WG about how to proceeed.

I think a big part of the problem is the popular tutorials and libs describing exactly how this behaves, e.g. http://paulirish.com/2011/requestanimationframe-for-smart-animating/ and more  - if the core behavior is changed, it should 1. be compatible with other browsers and 2. a heads-up sent to the authors of these tutorials... just an idea ;)
Comment 43 Nat Duca 2012-05-07 10:33:08 PDT
(In reply to comment #42)
> Well - would it be possible to pass as a 2nd parameter (DOMTimeStamp being the first)? - then it wouldn't break much and still deliver the high resolution timers for the apps needing them.  I am assuming this is for fast and smooth canvas based animations primarily?

Yep, its mainly for people who care about animating precisely, down to millisecond levels. Or, for people who use raf to guess frame rate  --- 60fps requires sub-millisecond timings --- rounding timestamps to ms will give you 58 or 62 fps :(


> 
> > 
> > I'm leaning toward reverting this patch and having a chat with WebPerf WG about how to proceeed.
> 
> I think a big part of the problem is the popular tutorials and libs describing exactly how this behaves

Hehe yep! Totally good point.


I think in the interests of keeping Webkit's Tip-of-Tree always valid, I will revert this patch for now, to give us time to discuss the "right way" to proceed.
Comment 44 Nat Duca 2012-05-07 10:36:47 PDT
Reverted r115525 for reason:

Too many pages rely on DOMTimeStamp as first argument. Reverting while we consider next steps.

Committed r116319: <http://trac.webkit.org/changeset/116319>
Comment 45 Paul Irish 2012-05-29 07:35:53 PDT
Was this resolved based on the thread here http://lists.w3.org/Archives/Public/public-web-perf/2012May/thread.html#msg36 that it'd go back in as the first argument?
Comment 46 James Robinson 2012-05-29 21:29:17 PDT
Yes
Comment 47 James Simonsen 2012-10-11 15:29:51 PDT
Created attachment 168288 [details]
Patch
Comment 48 James Simonsen 2012-10-11 15:34:07 PDT
FYI, I've revived Nat's patch and plan to land it shortly. We'd backed it out because developers weren't using performance.now(). Now that that's been resolved and is properly supported in WebKit, we need to go forward with this to match the rAF spec.
Comment 49 WebKit Review Bot 2012-10-11 19:45:10 PDT
Comment on attachment 168288 [details]
Patch

Clearing flags on attachment: 168288

Committed r131131: <http://trac.webkit.org/changeset/131131>
Comment 50 WebKit Review Bot 2012-10-11 19:45:16 PDT
All reviewed patches have been landed.  Closing bug.