Bug 65645 - Scroll animator changes to nail the framerate
Summary: Scroll animator changes to nail the framerate
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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-03 15:02 PDT by Scott Byer
Modified: 2011-08-09 16:56 PDT (History)
9 users (show)

See Also:


Attachments
Patch (9.70 KB, patch)
2011-08-03 15:04 PDT, Scott Byer
no flags Details | Formatted Diff | Diff
Patch (13.11 KB, patch)
2011-08-05 12:51 PDT, Scott Byer
no flags Details | Formatted Diff | Diff
Patch (13.40 KB, patch)
2011-08-05 14:15 PDT, Scott Byer
no flags Details | Formatted Diff | Diff
Patch (13.45 KB, patch)
2011-08-08 10:42 PDT, Scott Byer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Scott Byer 2011-08-03 15:02:27 PDT
Scroll animator changes to nail the framerate
Comment 1 Scott Byer 2011-08-03 15:04:46 PDT
Created attachment 102835 [details]
Patch
Comment 2 James Robinson 2011-08-03 15:12:55 PDT
Comment on attachment 102835 [details]
Patch

Hmm, I didn't realize this was all Timer based.  You will not get a solid framerate off of Timers, no matter what you do.  Have you looked into use the requestAnimationFrame machinery?
Comment 3 Scott Byer 2011-08-03 15:58:25 PDT
Ah, so the idea would be to register an animation callback with the parent Document's ScriptedAnimationController instead of using the Timer directly? Are the frame rate tolerances for that tighter than for what I'm doing with Timer? This gets the Timer use behaving pretty nicely.
Comment 4 James Robinson 2011-08-03 16:05:13 PDT
(In reply to comment #3)
> Ah, so the idea would be to register an animation callback with the parent Document's ScriptedAnimationController instead of using the Timer directly?

Yep.

> Are the frame rate tolerances for that tighter than for what I'm doing with Timer? This gets the Timer use behaving pretty nicely.

The ScriptedAnimationController callbacks are tied directly into the paint/compositing loops and are vsync-driven when GPU composited.  Timers have no explicit links to the rendering loop, so they just fire whenever.  This might line up with the display loop, or (much more likely) it might have all sorts of aliasing issues if it's close to but not exactly synchronized with the display loop.  Think lots and lots of dropped and doubled frames.

One major difference is that the ScriptedAnimationController callbacks will not fire if the tab is not visible, so you need to be aware of that in your logic.
Comment 5 Scott Byer 2011-08-03 16:10:08 PDT
That shouldn't be a problem - the math in here is all time domain based - I will get one call when the tab becomes visible? I can write up a test to cover that case. Let me give the ScriptedAnimationController a shot.
Comment 6 James Robinson 2011-08-03 16:15:35 PDT
Yup, you get one call when switching back.
Comment 7 James Robinson 2011-08-03 16:18:18 PDT
Note that in layout tests, in order to get those callbacks to fire you'll have to manually kick the display system by calling layoutTestController.display().
Comment 8 Scott Byer 2011-08-04 17:17:44 PDT
Getting back to this; is it appropriate for something in platform to be using something from dom? Part of the problem is getting to the Document to register the animation callback; it seems like a layering violation. It seems like a lot of plumbing would be needed before this code could be switched over.
Comment 9 James Robinson 2011-08-04 17:21:01 PDT
Ah yes, you're right - that would be a layering violation :( if platform/ depended on dom/ (or page/), and it would be a lot of plumbing to hook that all up.  Still, in the end it would produce a much better visual result if we were to get that working.

Would you like to proceed with this patch in the meantime?
Comment 10 Scott Byer 2011-08-04 17:23:52 PDT
Yes, I would. For the longer term, I think I'd want to talk over potential designs on how scrolling could be hooked up to the animator; there's another layering violation I keep running across in the code that should be cleaned up at the same time.
Comment 11 James Robinson 2011-08-04 17:43:08 PDT
Comment on attachment 102835 [details]
Patch

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

In that case I think this looks fine, although I left some comments for you to consider before landing.

> Source/WebCore/platform/ScrollAnimatorNone.cpp:346
>      double currentTime = WTF::currentTime();

could you use WTF::monotonicallyIncreasingTime() instead for this logic?  That's what WebCore::Timer is based off of.  The main differences between it and WTF::currentTime() are:
- monotonicallyIncreasingTime() does not go backwards when the system clock is adjusted (which is rare in practice, but does happen due to NTP and such)
- monotonicallyIncreasingTime() does not go crazy on windows when the clocks drift (which is not so rare in practice)
- monotonicallyIncreasingTime() can be a lot cheaper to call on some systems, primarily windows

this might not be an appropriate change for this patch, but something to think about for the future.

> Source/WebCore/platform/ScrollAnimatorNone.cpp:347
> +    PerAxisData* data = (timer == &m_horizontalData.m_animationTimer) ? &m_horizontalData : &m_verticalData;

this stuff is slightly confusing - could you perhaps use a temporary bool to keep track of which scrollbar we're updating here, and maybe use const references to the relevant PerAxisData instead of a pointer (since it seems we don't mutate it at all).

Do we never use the same timer for both scrollbars?

> Source/WebCore/platform/ScrollAnimatorNone.cpp:350
> +        double nextTimerInterval = max(kMinimumTimerInterval, deltaToNextFrame - WTF::currentTime() + currentTime);

this seems to be trying to account for the time delta between the top of this function and hitting this line, which is a little confusing - do you expect the operations in animateScroll() to be expensive enough to have to worry about this delta?  querying the clock is not necessarily cheap on all platforms
Comment 12 Scott Byer 2011-08-05 12:51:10 PDT
Created attachment 103097 [details]
Patch
Comment 13 Scott Byer 2011-08-05 12:52:22 PDT
Cleaned up a bit, went to the single timer for both directions.
Comment 14 WebKit Review Bot 2011-08-05 12:59:14 PDT
Comment on attachment 103097 [details]
Patch

Attachment 103097 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9323064
Comment 15 Scott Byer 2011-08-05 14:15:53 PDT
Created attachment 103111 [details]
Patch
Comment 16 James Robinson 2011-08-05 19:18:43 PDT
Comment on attachment 103111 [details]
Patch

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

R=me, one nit.  You don't have to upload another patch for review, just upload a new patch that has "Reviewed by James Robinson." in the ChangeLogs instead of NOBODY (OOPS!) and mark the patch cq? but not review? and any committer can flag it for review. I've come down with some sort of illness so I might not be able to get back to this soon.

> Source/WebCore/platform/ScrollAnimatorNone.cpp:318
> +    bool result = data.updateDataFromParameters(orientation, step, multiplier, scrollableSize, WTF::monotonicallyIncreasingTime(), &parameters);

nitpick: "result" is not very illuminating.  what does it mean when updateDataFromParameters() returns false?  was there some problem with the input parameters, or internal error in the calculations? did the scroll happen or not?  I'd suggest renaming this variable to describe what it means.
Comment 17 Scott Byer 2011-08-08 10:42:02 PDT
Created attachment 103264 [details]
Patch
Comment 18 WebKit Review Bot 2011-08-08 15:01:28 PDT
Comment on attachment 103264 [details]
Patch

Clearing flags on attachment: 103264

Committed r92639: <http://trac.webkit.org/changeset/92639>
Comment 19 WebKit Review Bot 2011-08-08 15:01:34 PDT
All reviewed patches have been landed.  Closing bug.