Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Description • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 1 • 9 years ago

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

	Markus Stange [:mstange]
	Comment 2 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 3 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 4 • 9 years ago

(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

	Markus Stange [:mstange]
	Comment 5 • 9 years ago

Wheel scrolling on OS X using a traditional scroll wheel starts an animation. Touchpad or magic mouse scrolling does not.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 6 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 7 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 8 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 9 • 9 years ago

(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

	Pulsebot
	Comment 10 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7fafae852fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d5a16d3683c

	Carsten Book [:Tomcat]
	Comment 11 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/d7fafae852fb
https://hg.mozilla.org/mozilla-central/rev/1d5a16d3683c