Bug 65645 – Scroll animator changes to nail the framerate

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Scott Byer]

Scroll animator changes to nail the framerate

[Scott Byer]

Created attachment 102835 [details]
Patch

[James Robinson]

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?

[Scott Byer]

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.

[James Robinson]

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

[Scott Byer]

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.

[James Robinson]

Yup, you get one call when switching back.

[James Robinson]

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().

[Scott Byer]

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.

[James Robinson]

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?

[Scott Byer]

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.

[James Robinson]

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

[Scott Byer]

Created attachment 103097 [details]
Patch

[Scott Byer]

Cleaned up a bit, went to the single timer for both directions.

[WebKit Review Bot]

Comment on attachment 103097 [details]
Patch

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

[Scott Byer]

Created attachment 103111 [details]
Patch

[James Robinson]

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.

[Scott Byer]

Created attachment 103264 [details]
Patch

[WebKit Review Bot]

Comment on attachment 103264 [details]
Patch

Clearing flags on attachment: 103264

Committed r92639: <http://trac.webkit.org/changeset/92639>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.