Scroll animator changes to nail the framerate
Created attachment 102835 [details]
Comment on attachment 102835 [details]
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?
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.
(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?
> 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.
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.
Yup, you get one call when switching back.
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().
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.
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?
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 on attachment 102835 [details]
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.
> 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.
> + 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?
> + 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
Created attachment 103097 [details]
Cleaned up a bit, went to the single timer for both directions.
Comment on attachment 103097 [details]
Attachment 103097 [details] did not pass chromium-ews (chromium-xvfb):
Created attachment 103111 [details]
Comment on attachment 103111 [details]
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.
> + bool result = data.updateDataFromParameters(orientation, step, multiplier, scrollableSize, WTF::monotonicallyIncreasingTime(), ¶meters);
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.
Created attachment 103264 [details]
Comment on attachment 103264 [details]
Clearing flags on attachment: 103264
Committed r92639: <http://trac.webkit.org/changeset/92639>
All reviewed patches have been landed. Closing bug.