Reduce main-thread scroll event latency to 1 frame

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(4 attachments)

In Orlando we came up with a plan to have the scroll event be one frame behind the APZ animation, rather than two (which is what it is now). The plan was basically to advance the APZ animations after sampling them, rather than before. This effectively delays the animation by one composite, but doesn't affect the main-thread scroll event. Therefore they become one frame closer. This will help with pages that have scroll-linked effects because the main-thread effect will be closer to the APZ scrolling.
I think all that we need to do here is move the line at [1] down to the bottom of the enclosing |if| condition, so that it happens after the ApplyAsyncContentTransformToTree/TransformScrollableLayer calls.

[1] https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/gfx/layers/composite/AsyncCompositionManager.cpp#1375
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #0)
> The
> plan was basically to advance the APZ animations after sampling them, rather
> than before. This effectively delays the animation by one composite

We also talked about adding the vsync interval to the sample timestamp so that the animation doesn't end up being delayed at all. We can do that as a separate step, though.
Posted file Test page
Here is a page we can use to test the frame latency. Scrolling the left div will cause JS to scroll the right div. By comparing the shadow-transforms of the two divs in the layers dump, we can see many frames off they are. If I start a local m-c build and load that page as the first thing, the left div has scrollId 4 and the right div has scrollId 5, so I can print out the shadow transforms like so:

./mach run 2>&1 | awk '/scrollId=4/ { v1 = $13 } /scrollId=5/ { print v1, $13 }'

... and it seems like they are already off by only one frame. Not really sure why yet. Maybe this is only the case for wheel scrolling on OS X, I forget if it starts an animation or not.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> ./mach run 2>&1 | awk '/scrollId=4/ { v1 = $13 } /scrollId=5/ { print v1,
> $13 }'

This requires layers.dump=true
Wheel scrolling on OS X using a traditional scroll wheel starts an animation. Touchpad or magic mouse scrolling does not.
This is a bit more robust than piping the layers dump through awk. With a external mouse on OS X I do indeed see a two frame latency (plus rounding). Output looks like this for example:

TransformYs:  -0.112978 -386.75 -387
TransformYs:  -0.331403 -386.267 -387
TransformYs:  -0.587478 -385.7 -387
TransformYs:  -0.534754 -384.817 -386
TransformYs:  -1.21262 -383.317 -386
TransformYs:  -1.54402 -381.583 -385
TransformYs:  -1.61935 -379.417 -383
TransformYs:  -3.05793 -375.233 -382
TransformYs:  -4.20276 -369.7 -379
TransformYs:  -5.18944 -363.517 -375
TransformYs:  -5.98028 -356.767 -370
TransformYs:  -7.22304 -348.017 -364
TransformYs:  -8.77459 -337.583 -357

In the last line for example the -357 is the JS-provided scroll offset corresponding to the APZ -356.767 two lines above.
And with the SampleAPZAnimations call moved down the output looks like this:

TransformYs:  3.55503 -308.867 -301
TransformYs:  3.2161 -316.117 -309
TransformYs:  3.43452 -323.6 -316
TransformYs:  3.91655 -332.667 -324
TransformYs:  0.0903876 -342.2 -342
TransformYs:  4.48144 -351.917 -342
TransformYs:  4.15758 -361.2 -352
TransformYs:  3.90902 -369.65 -361
TransformYs:  3.55503 -377.867 -370
TransformYs:  3.20104 -385.083 -378

which has a one frame latency, as expected.
Note that the call to SampleAPZAnimations also calls ReportCheckerboard on the APZCs. Previously the ordering was:

- report checkerboard
- advance animations
- update shadow transform

and the new ordering is:

- update shadow transform
- report checkerboard
- advance animations

I think both are valid, since the "report checkerboard" step happens right after the "update shadow transform" step. It's just that before the checkerboard being reported was for the previous frame and now it's for the current frame. IMO the new code is actually more correct because if non-animation-driven updates to the scroll position happen the "report checkerboard" step will now take them into account whereas before it wouldn't properly do that.
Attachment #8701836 - Flags: review?(mstange)
(In reply to Markus Stange [:mstange] from comment #2)
> We also talked about adding the vsync interval to the sample timestamp so
> that the animation doesn't end up being delayed at all. We can do that as a
> separate step, though.

This patch implements this. I added the check for TimeDuration::Forever to robustify against vlad's upcoming vsync rewrite in bug 1184283 - specifically [1].

[1] https://hg.mozilla.org/try/file/e54337dfe53a/gfx/thebes/gfxVsync.h#l149
Attachment #8701839 - Flags: review?(mstange)
Attachment #8701836 - Attachment description: Advance animations after updating shadow transforms → Part 1 - Advance animations after updating shadow transforms
Attachment #8701836 - Flags: review?(mstange) → review+
Attachment #8701839 - Flags: review?(mstange) → review+
https://hg.mozilla.org/mozilla-central/rev/d7fafae852fb
https://hg.mozilla.org/mozilla-central/rev/1d5a16d3683c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.