Bug 66683

Summary:

Expose high-resolution on requestAnimationFrame callback

Product:

WebKit

Reporter:

Nat Duca <nduca>

Component:

New Bugs

Assignee:

Nat Duca <nduca>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, dglazkov, gustavo, jamesr, kbr, larsgk, mifenton, ojan, paulirish, pnormand, rwlbuis, scheib, scottbyer, simonjam, tonikitoo, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

58354, 66684, 84912

Bug Blocks:

84386

Attachments:

Description	Flags
Patch	none
Patch	none
Archive of layout-test-results from ec2-cr-linux-03	none
Patch	none
Archive of layout-test-results from ec2-cr-linux-03	none
Patch	none
One that builds, hopefully	none
debugging via ews	none
fix mac	none
Patch for landing	none
Patch	none

Nat Duca

Reported 2011-08-22 10:39:52 PDT

Use monotonic clock for requestAnimationFrame's time parameter

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
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Nat Duca

Comment 1 2011-08-22 10:49:39 PDT

Created attachment 104695 [details] Patch

Nat Duca

Comment 2 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. :)

James Robinson

Comment 3 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).

James Robinson

Comment 4 2011-08-22 11:12:14 PDT

Hey James, how can we pick up the correct zero time? Where should that be stored?

Nat Duca

Comment 5 2011-08-22 11:16:36 PDT

Totally agreed. :)

James Simonsen

Comment 6 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.

James Simonsen

Comment 7 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.

Vincent Scheib

Comment 8 2012-03-21 20:21:39 PDT

Does this have a chance of moving forward? Is it blocked on technical issues?

Nat Duca

Comment 9 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.

Nat Duca

Comment 10 2012-04-21 15:04:52 PDT

Created attachment 138255 [details] Patch

Nat Duca

Comment 11 2012-04-21 15:06:08 PDT

Comment on attachment 138255 [details] Patch Ugh, wrong patch.

WebKit Review Bot

Comment 12 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

WebKit Review Bot

Comment 13 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

Nat Duca

Comment 14 2012-04-21 20:02:46 PDT

Created attachment 138263 [details] Patch

Build Bot

Comment 15 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

James Robinson

Comment 16 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)

WebKit Review Bot

Comment 17 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

WebKit Review Bot

Comment 18 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

Nat Duca

Comment 19 2012-04-25 00:24:03 PDT

Created attachment 138747 [details] Patch

Nat Duca

Comment 20 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 :)

Philippe Normand

Comment 21 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

Early Warning System Bot

Comment 22 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

Early Warning System Bot

Comment 23 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

Nat Duca

Comment 24 2012-04-25 00:52:58 PDT

Created attachment 138752 [details] One that builds, hopefully

Build Bot

Comment 25 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

Nat Duca

Comment 26 2012-04-25 02:37:39 PDT

Created attachment 138769 [details] debugging via ews

Build Bot

Comment 27 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

Nat Duca

Comment 28 2012-04-25 16:22:05 PDT

Created attachment 138893 [details] fix mac

James Robinson

Comment 29 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.

James Robinson

Comment 30 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 :)

Nat Duca

Comment 31 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?

James Simonsen

Comment 32 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.

James Robinson

Comment 33 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.

James Robinson

Comment 34 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.

Nat Duca

Comment 35 2012-04-27 16:38:13 PDT

Created attachment 139305 [details] Patch for landing

WebKit Review Bot

Comment 36 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>

WebKit Review Bot

Comment 37 2012-04-27 18:27:45 PDT

All reviewed patches have been landed. Closing bug.

Kenneth Russell

Comment 38 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.

Nat Duca

Comment 39 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.

Lars Knudsen

Comment 40 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)

Nat Duca

Comment 41 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.

Lars Knudsen

Comment 42 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 ;)

Nat Duca

Comment 43 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.

Nat Duca

Comment 44 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>

Paul Irish

Comment 45 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?

James Robinson

Comment 46 2012-05-29 21:29:17 PDT

Yes

James Simonsen

Comment 47 2012-10-11 15:29:51 PDT

Created attachment 168288 [details] Patch

James Simonsen

Comment 48 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.

WebKit Review Bot

Comment 49 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>

WebKit Review Bot

Comment 50 2012-10-11 19:45:16 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.