See bug 1367770 comment 2 for an explanation of why we may want to do this. As this does delay the responsiveness of scrolling to user input by 1 frame, we may want to start by landing this change under a pref.

Progress update here: I now have a WIP patch series for this that is passing all tests except for dom/events/test/test_continuous_wheel_events.html, which I'm still investigating: https://treeherder.mozilla.org/#/jobs?repo=try&revision=70ad6eaafd484a0117f69b9c7a4785bfc8ee3bdb

Have a clean Try push now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=76cafd39945c0da4ab27ec3f5132f31f07423f15 I'd still like to investigate the following issue, mentioned in bug 1367770 comment 4: > *very* occasionally, I see [the scroll-linked effect] being 1 frame > *ahead* [of the async scroll]

(In reply to Botond Ballo [:botond] from comment #2) > I'd still like to investigate the following issue, mentioned in bug 1367770 > comment 4: > > > *very* occasionally, I see [the scroll-linked effect] being 1 frame > > *ahead* [of the async scroll] What's happening here is that, while composites and refresh driver ticks are synchronized with each other, they do not occur exactly at the same time. Sometimes, the refresh driver tick occurs a tenth of a millisecond or so later than the composite, and sometimes, that's just enough time for the compositor to sample the async scroll offset for the next composite (thus locking it in such that further async scrollig until the next composite is ignored), then process an input event, send the resulting update to the scroll offset to content, and have that arrive in time to be picked up by the paint triggered by the refresh driver tick, resulting in the painted offset being rendered a frame earlier than the async offset. As this happens very rarely, I don't know whether we care about it - after all, it's just like the main thread sometimes being a frame behind the compositor thread (which we've been OK with for a long time), just in reverse. Since we're landing this under a pref anyways, I'm not going to block the landing on this issue being resolved (if we decide we care, we can block the flipping of the pref on it being resolved).

Also, just wanted to follow up on this IRC conversation fragment: > 16:50 kats so the actual delay is the part that concerns me > 16:51 kats botond|away: because the actual delay means that what gets composited into the snapshot doesn't reflect the touchmoves, until we get a vsync > 16:52 kats if you compare the first and the last snapshots, that will fix the first problem (that of the vsync-triggered composite making two snapshots identical in the middle of the test) > 16:52 kats but that won't solve the problem of the snapshots not reflecting the change in position because we never got a vsync at all > 16:52 botond|sf kats: will the snapshot not call TransformShadowTree? > 16:53 kats botond|away: it will. are you not using the timestamp from that to determine when the visual scroll position changes? > 16:53 kats or are you just using calls to TransformShadowTree > 16:53 kats oh i guess it must be the latter > 16:53 kats it all makes sense now > 16:54 kats ok so yes, comparing the first and the last snapshot should be fine > 16:54 botond|sf kats: so i do re-use the timestamp-based filtering to avoid sampling an APZC multiple times for multiple layers > 16:55 botond|sf kats: so if the timestamp doesn't change from one snapshot to the next, we may skip the sampling. but that seems like an implementation artifact that we can work around > 16:55 botond|sf kats: like, we can use a different technique to make sure we only sample an APZC once per composite, but still make sure to sample it every composite > 16:55 kats ok, fair enough I found I did not need to employ a different technique to make sure we only sample an APZC once per composite, because CompositeBridgeParent::RecvMakeSnapshot() calles ForceComposeToTarget() [1] which updates the composite's timestamp [2], so each snapshot will in fact have a different timestamp even if no vsyncs have occurred between them. [1] http://searchfox.org/mozilla-central/rev/5e1e8d2f244bd8c210a578ff1f65c3b720efe34e/gfx/layers/ipc/CompositorBridgeParent.cpp#527 [2] http://searchfox.org/mozilla-central/rev/5e1e8d2f244bd8c210a578ff1f65c3b720efe34e/gfx/layers/ipc/CompositorVsyncScheduler.cpp#295

Try pushed: With the patch series as posted: https://treeherder.mozilla.org/#/jobs?repo=try&revision=090750c17cf55b1086cf315ac64d0f2b99a836a7 With the pref enabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8f4844cccdb5c1474431984863c4d75df3e81f5

Comment on attachment 8883758 [details] Bug 1375949 - Repurpose AsyncPanZoomController::AsyncMode into a more general AsyncTransformConsumer enum. https://reviewboard.mozilla.org/r/154694/#review159904

Comment on attachment 8883759 [details] Bug 1375949 - Delay application of async scroll offset by one composite, to give content a chance to remain in sync. https://reviewboard.mozilla.org/r/154696/#review159906 ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:3734 (Diff revision 1) > + mCompositedZoom.xScale *= (totalResolutionChange / presShellResolutionChange).width; > + mCompositedZoom.yScale *= (totalResolutionChange / presShellResolutionChange).height; Isn't this equivalent to mCompositedZoom = mFrameMetrics.GetZoom()? In fact, instead of updating mCompositedZoom and mCompositedScrollOffset throughout this function, can we just move the two assignments from the top of the function down to the bottom?

Comment on attachment 8883760 [details] Bug 1375949 - Fix an incorrect comment in helper_touch_action_regions.html. https://reviewboard.mozilla.org/r/154698/#review159908

Comment on attachment 8883761 [details] Bug 1375949 - Fix helper_touch_action_regions.html. https://reviewboard.mozilla.org/r/154700/#review159910 ::: commit-message-bb6c9:3 (Diff revision 1) > +Bug 1375949 - Fix helper_touch_action_regions.html. r=kats > + > +The test was assuming hat processing an input event that causes async s/hat/that/ ::: gfx/layers/apz/test/mochitest/helper_touch_action_regions.html:199 (Diff revision 1) > ok(waitFor('touchmove', i), "Touchmove processed in chrome process"); > > - var snapshot = getSnapshot(rect); > - if (i == 1) { > - // The first touchmove is consumed to get us into the panning state, so > - // no actual panning occurs > + // Take a snapshot after each touch move event. This forces > + // a composite each time, even we don't get a vsync in this > + // interval. > + lastSnapshot = getSnapshot(rect); I think it might be good to compare each pair of snapshots and report (via dump or something) the number of snapshots pairs that where different. If the test starts failing intermittently we can look back at successful logs and see if the number of different snapshot pairs was closer to 10 or closer to 0.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11) > ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:3734 > (Diff revision 1) > > + mCompositedZoom.xScale *= (totalResolutionChange / presShellResolutionChange).width; > > + mCompositedZoom.yScale *= (totalResolutionChange / presShellResolutionChange).height; > > Isn't this equivalent to mCompositedZoom = mFrameMetrics.GetZoom()? No; we are doing the same thing to mCompositedZoom that we are doing to mFrameMetrics.mZoom (multiplying by |totalResolutionChange / presShellResolutionChange|), so if they were different before, they'll be different after. My guiding principle when writing this part of the patch was: do the same thing to the composited values that we do to the mFrameMetrics values. So if we overwrite the mFrameMetrics value, then overwrite the composited value too; if we scale by the mFrameMetrics value by an amount, scale the composited value by the same amount. > In fact, instead of updating mCompositedZoom and mCompositedScrollOffset > throughout this function, can we just move the two assignments from the top > of the function down to the bottom? In addition to the above, there is one other important reason we don't want to do this: we only overwrite the mFrameMetrics scroll offset if |scrollOffsetUpdated| is true; likewise, we only want to overwrite mCompositedScrollOffset in that case.

Ok, that makes sense. (I misread the patch before and missed the fact that the unconditional assignment of mCompositedZoom = mFrameMetrics.GetZoom() was in a different branch)

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13) > Comment on attachment 8883761 [details] > Bug 1375949 - Fix helper_touch_action_regions.html. > > https://reviewboard.mozilla.org/r/154700/#review159910 > > ::: commit-message-bb6c9:3 > (Diff revision 1) > > +Bug 1375949 - Fix helper_touch_action_regions.html. r=kats > > + > > +The test was assuming hat processing an input event that causes async > > s/hat/that/ Fixed. > ::: gfx/layers/apz/test/mochitest/helper_touch_action_regions.html:199 > (Diff revision 1) > > ok(waitFor('touchmove', i), "Touchmove processed in chrome process"); > > > > - var snapshot = getSnapshot(rect); > > - if (i == 1) { > > - // The first touchmove is consumed to get us into the panning state, so > > - // no actual panning occurs > > + // Take a snapshot after each touch move event. This forces > > + // a composite each time, even we don't get a vsync in this > > + // interval. > > + lastSnapshot = getSnapshot(rect); > > I think it might be good to compare each pair of snapshots and report (via > dump or something) the number of snapshots pairs that where different. If > the test starts failing intermittently we can look back at successful logs > and see if the number of different snapshot pairs was closer to 10 or closer > to 0. This is done in the updated patch.

Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7f4579b24b30 Repurpose AsyncPanZoomController::AsyncMode into a more general AsyncTransformConsumer enum. r=kats https://hg.mozilla.org/integration/autoland/rev/db00deaf0765 Delay application of async scroll offset by one composite, to give content a chance to remain in sync. r=kats https://hg.mozilla.org/integration/autoland/rev/c24ae4f969ec Fix an incorrect comment in helper_touch_action_regions.html. r=kats https://hg.mozilla.org/integration/autoland/rev/3dc1cd7a957c Fix helper_touch_action_regions.html. r=kats

https://hg.mozilla.org/mozilla-central/rev/7f4579b24b30 https://hg.mozilla.org/mozilla-central/rev/db00deaf0765 https://hg.mozilla.org/mozilla-central/rev/c24ae4f969ec https://hg.mozilla.org/mozilla-central/rev/3dc1cd7a957c