Use monotonic clock for requestAnimationFrame's time parameter
Created attachment 104695 [details] Patch
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 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).
Hey James, how can we pick up the correct zero time? Where should that be stored?
Totally agreed. :)
(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 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.
Does this have a chance of moving forward? Is it blocked on technical issues?
(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.
Created attachment 138255 [details] Patch
Comment on attachment 138255 [details] Patch Ugh, wrong patch.
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
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
Created attachment 138263 [details] Patch
Comment on attachment 138263 [details] Patch Attachment 138263 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12480512
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 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
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
Created attachment 138747 [details] Patch
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 on attachment 138747 [details] Patch Attachment 138747 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12521440
Comment on attachment 138747 [details] Patch Attachment 138747 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12527485
Comment on attachment 138747 [details] Patch Attachment 138747 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12478006
Created attachment 138752 [details] One that builds, hopefully
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
Created attachment 138769 [details] debugging via ews
Comment on attachment 138769 [details] debugging via ews Attachment 138769 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12521487
Created attachment 138893 [details] fix mac
(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.
(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 :)
Re 1/10th, seems like @simonjam has the right bits of information in his head to make a yes/no call here. Wdyt?
(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.
(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 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.
Created attachment 139305 [details] Patch for landing
Comment on attachment 139305 [details] Patch for landing Clearing flags on attachment: 139305 Committed r115525: <http://trac.webkit.org/changeset/115525>
All reviewed patches have been landed. Closing bug.
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.
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.
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)
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.
(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 ;)
(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.
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>
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?
Yes
Created attachment 168288 [details] Patch
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 on attachment 168288 [details] Patch Clearing flags on attachment: 168288 Committed r131131: <http://trac.webkit.org/changeset/131131>