Closed Bug 1375949 Opened 7 years ago Closed 7 years ago

Delay the compositor's application of async scrolling by 1 frame

Categories

(Core :: Panning and Zooming, enhancement, P3)

Product:
Core
Component:
Panning and Zooming
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: botond, Assigned: botond)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(4 files)

Bug 1375949 - Repurpose AsyncPanZoomController::AsyncMode into a more general AsyncTransformConsumer enum.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1375949 - Delay application of async scroll offset by one composite, to give content a chance to remain in sync.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1375949 - Fix an incorrect comment in helper_touch_action_regions.html.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1375949 - Fix helper_touch_action_regions.html.
59 bytes, text/x-review-board-request
kats
: review+
Details 
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.
Assignee: nobody → botond
Priority: -- → P3
Whiteboard: [gfx-noted]
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
Comment on attachment 8883758 [details]
Bug 1375949 - Repurpose AsyncPanZoomController::AsyncMode into a more general AsyncTransformConsumer enum.

https://reviewboard.mozilla.org/r/154694/#review159904
Attachment #8883758 - Flags: review?(bugmail) → review+
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?
Attachment #8883759 - Flags: review?(bugmail) → review+
Comment on attachment 8883760 [details]
Bug 1375949 - Fix an incorrect comment in helper_touch_action_regions.html.

https://reviewboard.mozilla.org/r/154698/#review159908
Attachment #8883760 - Flags: review?(bugmail) → review+
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.
Attachment #8883761 - Flags: review?(bugmail) → review+
(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
Blocks: 1383912
Blocks: 1384708
Blocks: 1400440
See Also: → 1485591
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: