Closed Bug 728153 Opened 12 years ago Closed 12 years ago

smooth scrolling waits 16ms before doing anything

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: tnikkel, Unassigned)

References

Details

(Whiteboard: [Snappy:P2])

Attachments

(1 file, 2 obsolete files)

In nsGfxScrollFrameInner::ScrollTo we start a TYPE_REPEATING_SLACK timer with delay 1000 / 60 = 16 ms to perform each step of the smooth scroll. My understanding of the timer code is that this first fires after (approx) 16 ms. So we don't even start doing anything until after 16 ms have elapsed.
The best way to solve this might be to do smooth scrolling of the refresh driver.
(In reply to Timothy Nikkel (:tn) from comment #1)
> The best way to solve this might be to do smooth scrolling of the refresh
> driver.

For that see bug 702463, specially comment 12
(aand you already knew about 702463, carry on..)
To clarify, this is a benefit of using the refresh driver that Ehsan and I didn't know about when we were discussing bug 702463.
Whiteboard: [Snappy] → [Snappy:P2]
Depends on: 702463
(In reply to Timothy Nikkel (:tn) from comment #0)
> In nsGfxScrollFrameInner::ScrollTo we start a TYPE_REPEATING_SLACK timer
> with delay 1000 / 60 = 16 ms to perform each step of the smooth scroll. My
> understanding of the timer code is that this first fires after (approx) 16
> ms. So we don't even start doing anything until after 16 ms have elapsed.

First of all, this only applies to start of a scroll from a standing page (else, it only extends the deadline and target position, while the timer just keeps firing regularly).

Also, even if we added a one shot for 0ms to start immediately (instead of after 16ms), it would still start only after 16ms, because the first callback will be invoked (hopefully) after 0ms, and the position function will return the initial position (for delta of 0ms), so only the 2nd callback (first of the TYPE_REPEATING_SLACK timer) will actually move the page, which is what happens now anyway.

To address this, we could initiate the smooth scrolling with "startTime" of (now-16ms). If we use this approach together with an initial 0ms one-shot, we can start moving the page immediately, with zero deterioration in smoothness, and with added responsiveness of 16ms.

If this bug eventually gets fixed by hooking smooth scroll to the refresh driver, than instead of startTime=now-16ms, we should use startTime=refreshDriver.lastTriggerTimestamp() (I didn't check if it actually has this function). This would also have zero deterioration of smoothness, and the gained responsiveness would average 1/2 of refresh driver intervals.
(In reply to Avi Halachmi (:avih) from comment #5)
> To address this, we could initiate the smooth scrolling with "startTime" of
> (now-16ms). If we use this approach together with an initial 0ms one-shot,
> we can start moving the page immediately, with zero deterioration in
> smoothness, and with added responsiveness of 16ms.

After the smooth scrolling timer fires (whichever one) we still have to wait for the refresh driver to fire before we paint, so there is still a delay.
(In reply to Timothy Nikkel (:tn) from comment #6)
> After the smooth scrolling timer fires (whichever one) we still have to wait
> for the refresh driver to fire before we paint, so there is still a delay.

True for any approach we choose. But the gains in responsiveness (16ms or vsync-intervals/2) despite zero deterioration of smoothness is still there.
(In reply to Avi Halachmi (:avih) from comment #7)
> (In reply to Timothy Nikkel (:tn) from comment #6)
> > After the smooth scrolling timer fires (whichever one) we still have to wait
> > for the refresh driver to fire before we paint, so there is still a delay.
> 
> True for any approach we choose.

No. If we use the refresh driver for smooth scrolling than there is no delay between the smooth scroll timer firing and the refresh driver tick because they are the same. If smooth scrolling uses its own timer than there will always be a delay between it and the next refresh driver tick.
Well, FWIW, after bug 702463 lands and we use the refresh driver notifications, the theoretically correct solution would be to InitSmoothScroll with start=driver->MostRecentRefresh() (instead of now(), which is -8ms on average for 60hz). However, for new observers, the refresh driver fakes most recent as now(), so we can't actually know the last refresh timestamp where it matters (=when starting a new scroll, but it does work when extending a scroll with an existing observer, though we don't need it there, and actually should not use it there).

So I've experimented with arbitrary "in the past" offsets, and observed that below 30ms offset it's not noticeable at all, even for quick 150ms overall duration. This is mostly due to the "ramp up" of the spline, which starts pretty slow anyway.

I'd say that we can wrap it up by starting at fixed 16ms "in the past", regardless of refresh rate or MostRecentRefresh or overall duration. It's completely unnoticeable (for better or worse), but in our hearts we'll know that we've increased responsiveness in 16ms ;)

Let me know if you want this patch.
(In reply to Avi Halachmi (:avih) from comment #9)
> Let me know if you want this patch.

If you've got the patch, you might as well upload it :)
Attached patch WIP of patch (obsolete) — Splinter Review
Avi, is this what you were explaining in comment #9?
Attachment #619629 - Flags: feedback?(chuku_regs)
Attached patch WIP of patch (obsolete) — Splinter Review
Forgot to qrefresh lol
Attachment #619629 - Attachment is obsolete: true
Attachment #619635 - Flags: feedback?(chuku_regs)
Attachment #619629 - Flags: feedback?(chuku_regs)
Attached patch WIP v2 of patchSplinter Review
This should only subtract the 16ms when the the scroll is on the first iteration.
Attachment #619635 - Attachment is obsolete: true
Attachment #619665 - Flags: feedback?(chuku_regs)
Attachment #619635 - Flags: feedback?(chuku_regs)
Attachment #619665 - Flags: feedback?(chuku_regs) → review?(tnikkel)
Comment on attachment 619665 [details] [diff] [review]
WIP v2 of patch

So I'm not really a fan of this approach. If we think that starting the animation advanced 16 ms makes things feel snappier, then why shouldn't we start at 32 ms, or 50 ms, or 100 ms... and make it even snappier?

We have a spline we use to define how smooth scrolling should animate. If we think that it doesn't feel fast enough at the start we should change the spline to ramp up quicker.
This is too minor to spend time on IMO. Either someone fixes it properly (which seems harder that it initially appeared to due to the faked MostRecentRefresh), or just leave it be. It really is unnoticeable...
Note that this bug doesn't really exist anymore since bug 702463 landed.
(In reply to Timothy Nikkel (:tn) from comment #16)
> Note that this bug doesn't really exist anymore since bug 702463 landed.

There's still a theoretical responsiveness improvement without any theoretical jump, if we set start to MostRecentRefresh. This is 8ms on average. But this is indeed too minor to spend time on, especially since it's not readily available, as far as I can tell.
Comment on attachment 619665 [details] [diff] [review]
WIP v2 of patch

Clearing review request based on comment 14.
Attachment #619665 - Flags: review?(tnikkel)
Closing this as WONTFIX due to comment #14, #15 and #17.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
I would say that bug 702463 either fixed this or made it invalid.
Thanks Timothy.
Resolution: WONTFIX → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: