1192919 - Need to schedule paints for scrolling inside inactive layers

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Markus Stange [:mstange]]

STR:
 1. Put lots and lots of text into your URL bar.
 2. Using your touchpad that supports horizontal scrolling, scroll to the center
    of the URL bar textbox and wait for the parent process refresh driver to idle.
 3. Scroll left and right.

Expected results:
Good scrolling.

Actual results:
Bad scrolling.


The URL bar textbox has an SVG mask applied to it, which we don't yet support on the compositor, so scrolling happens by APZ "scrolling" the ScrollInfoLayer APZC and syncing the scroll position to the main thread.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Isn't what you described in comment 0 the desired behavior? I'm not sure what you mean by the text in the bug summary and how that would improve things.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Oh I guess this is one of the scenarios referred to in bug 1187432 comment 7 that was made worse?

Comment 3

[Markus Stange [:mstange]]

Exactly. I didn't do a very good job of describing the problem, did I?

The problem is that we don't support SVG masks and filters in the compositor, so APZ can't async scroll anything. We need a main thread paint in order to reflect any scrolling to the screen. And with bug 1187432, we only do main thread paints when the display port changes.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, annotate FrameMetrics

This puts a bit on FrameMetrics telling us whether or not it's for a ScrollInfoLayer.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, minimize displayports

This patch forces us to have empty margins for frames with ScrollInfoLayers, so we don't waste time painting extra stuff that will never be scrolled asynchronously.

GetDisplayPortFromMarginsData does some alignment that can expand the displayport further - this patch bypasses that as well.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3, fix throttling

Normally when scrolling we throttle paint requests, which is okay since while layout is idle, APZ can still scroll content in the displayport.

With scrollinfo layers, APZ has no content to scroll, but it is still responsible for driving animation. In this scenario, we need the paint requests to arrive as soon as possible. Currently it looks like these paint requests are getting throttled, so animation is extremely janky.

Normally the first paint request would change the displayport, but not the scroll position, and this would cause a repaint. With empty margins and no scroll position change, we won't get any layers update from content, and the throttler keeps cancelling and accepting new requests without ever dispatching any.

This patch just stops throttling when using scrollinfo layers, and the scrolling quality on bhphotovideo.com and loop.ableton.com is *much* better. I'm not sure if this is the right approach though. It's possible we just need to unthrottle a few requests, or synchronize it with the compositor - or, defer the animation to the main thread entirely (but this wouldn't work with apz-only animations).

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8695702 [details] [diff] [review]
part 1, annotate FrameMetrics

Review of attachment 8695702 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +3216,1 @@
>      mFrameMetrics.SetLineScrollAmount(aLayerMetrics.GetLineScrollAmount());

Move this down to the end of the pile so that it's in roughly the same order as everywhere else

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8695702 [details] [diff] [review]
part 1, annotate FrameMetrics

Review of attachment 8695702 [details] [diff] [review]:
-----------------------------------------------------------------

Actually this would probably be more accurate as mIsScrollInfoLayer than mHasScrollInfoLayer.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8695706 [details] [diff] [review]
part 2, minimize displayports

Review of attachment 8695706 [details] [diff] [review]:
-----------------------------------------------------------------

Minusing until we agree on the nsLayoutUtils change.

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +42,5 @@
>  {
> +  if (aFrameMetrics.HasScrollInfoLayer()) {
> +      // Don't perform any adjustment - we don't want a large displayport
> +      // if the scroll cannot be performed asynchronously at all.
> +      return;

So the code in this function doesn't actually increase the *size* of the displayport, but it does adjust it to account for any residual difference between the APZ scroll offset and the content scroll offset. There are two main reasons this residual difference is nonzero:

1) If the APZ scroll offset is not perfectly on an app-unit boundary, then it will be slightly different from the content scroll offset which is always an integer number of app units
2) In the case of overflow:hidden but zoomable top-level frames, we allow the user to zoom and then pan around in the APZ, without changing the scroll offset in content. (This was added in bug 975962).

There are probably some other conditions where this might happen as well, but in all cases it's the difference between the APZ scroll offset and the content scroll offset.

The important thing here is that for scrollinfo layers, the APZ scroll offset doesn't really matter because it's not user-visible, so we want the displayport margins to apply to the content scroll offset. All this to say that the code you're adding here is correct, but the comment is wrong. Please reword it to say:

// In cases where the APZ scroll offset is different from the
// content scroll offset, we want to interpret the margins
// as relative to the APZ scroll offset except when the frame
// is not scrollable by APZ. Therefore, if the layer is a scroll
// info layer, we leave the margins as-is and they will be
// interpreted as relative to the content scroll offset.

Also, ironic that you fixed the indent in the existing code but not in the new code you added :p

::: layout/base/nsLayoutUtils.cpp
@@ +1013,5 @@
>  
> +  // If we're not tiling and our margins are empty, we probably don't want
> +  // extra displayport at all so don't inflate the screen rect.
> +  if (gfxPrefs::LayersTilesEnabled() || aMarginsData->mMargins != ScreenMargin()) {
> +    // Round-out the display port to the nearest alignment (tiles)

I'm not sure if I agree with this. In bug 1204136 BenWa specifically made the displayport align to "virtual" 128x128 tiles even if tiling was disabled, so that the perf characteristics would be consistent. I think this change undoes some of that work, and for a negligible benefit.

If we keep this code as it was originally, then at worst we'll pad out the displayport to the next tile which shouldn't be that much extra painting. The bulk of the displayport area was eliminated in the change you made to AsyncPanZoomController.cpp in this patch. And as the user scrolls around on this scrollinfo layer, *not* having this tile alignment will cause a lot of actual repaints, but having the tile aligned displayport should only cause an actual repaint as the user rolls onto another tile. This will give us perf characteristics similar to everything else, and I think we should keep it as-is.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8695761 [details] [diff] [review]
part 3, fix throttling

Review of attachment 8695761 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +2831,1 @@
>      mPaintThrottler->PostTask(

So on the one hand I see that we want to be sending these repaint requests more frequently. But I'm not sure this is the right place to do this.

The PaintThrottler throttles the paint requests so that there's only one inflight at a time. If we bypass this, we could flood the content process with repaint requests (which don't get coalesced in IPDL) which is going to create a ton of work for the content process, much more than it needs to do. It could end up lagging behind, doing stale paints when the compositor is already far ahead. I imagine that with this patch it *looks* smoother because the main thread is painting every single frame, but the entire animation will run slower (for the same reason).

That might be right tradeoff but I'm not convinced, particularly since it means a lot of extra work for the content process.

What I'd like you to try first is instead of removing this throttling, drop the APZPanRepaintInterval, APZFlingRepaintInterval, and APZSmoothScrollRepaintInterval prefs to 0 (or comment out the code with the TimeSinceLastRequest calls) and see if that also results in better scrolling on bhphotovideo and loop.ableton.com. If it does, we can make that repaint interval conditional on !HasScrollInfoLayer and keep this throttling.

Note that you'll need the SchedulePaint you added to APZCCallbackHelper still.

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +148,5 @@
>  
> +  // If APZ cannot scroll this frame, force a paint.
> +  if (aMetrics.HasScrollInfoLayer()) {
> +    if (nsIFrame* frame = aContent->GetPrimaryFrame()) {
> +      frame->SchedulePaint();

I think this belongs inside the |if (scrollUpdated)| condition above, after the call to AdjustDisplayPortForScrollDelta. In fact, you could combine this with the part 2 patch, so that the code looks like this:

if (scrollUpdated) {
  if (aMetrics.HasScrollInfoLayer()) {
    // the code comment from comment 9
    if (nsIFrame* frame = aContent->GetPrimaryFrame()) {
      frame->SchedulePaint();
    }
  } else {
    AdjustDisplayPortForScrollDelta(...)
  }
} else {
  ...
}

(Ideally we could handle this by just passing a RepaintMode::Repaint as the last argument at [1] but it looks like the nsLayoutUtils function will early-exit if the margins didn't change, so that won't work.)

[1] https://dxr.mozilla.org/mozilla-central/rev/528fc18a86d3f6e6bec37190522a46853a846665/gfx/layers/apz/util/APZCCallbackHelper.cpp#167

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2526,5 @@
> +    // If we don't have displayport margins, assume we're trying to minimize
> +    // the displayport, in which case we should always repaint since APZ has
> +    // nothing to asynchronously scroll.
> +    if (!displayPort.IsEqualEdges(oldDisplayPort) ||
> +        nsLayoutUtils::HasZeroDisplayPortMargins(mOuter->GetContent()))

I'm not sure why this would be needed if we are already calling SchedulePaint from APZCCallbackHelper.cpp

::: layout/xul/nsSliderFrame.cpp
@@ +766,1 @@
>      SchedulePaint();

Ditto, not sure why this is needed

Comment 11

[Kartikaya Gupta (email:[e-mail])]

I was looking at this code for another bug and realized that there's another reason we can't just not throttle, at least not the way you did in the patch. It looks like when we send an unthrottled request (which we do from only one place currently, in FlushRepaintForNewInputBlock) we have to do a bunch of other things as well, such as cancel the pending tasks in the throttler and update mLastPaintRequestMetrics. Otherwise things will get badly out of sync. This code should probably be moved into RequestContentRepaint and made conditional on !throttled, so that we don't have to do it at call sites but I just wanted to document that here.

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, minimize displayports

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8696770 [details] [diff] [review]
part 2, minimize displayports

Review of attachment 8696770 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, thanks

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3, fix throttling v2

Drop the repaint interval, and dispatch throttled paint tasks on each vsync.

We reasoned this is okay since (1) content is repainting on vsync anyway, and (2) content is also eliding paints that don't change the displayport.

Comment 15

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3, schedule paints

Always schedule paints on the thread main when we're updating a scroll info layer.

With all of these patches, scrolling loop.abletoon is very smooth.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8697383 [details] [diff] [review]
part 3, fix throttling v2

Review of attachment 8697383 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like it's on the right track. After thinking it over some more and talking it over to Botond we came to the conclusion that it should be ok to rip out the TaskThrottler entirely. The repaint requests can get sent unthrottled always to the content process and shouldn't significantly increase the work on the main thread, because the repaints themselves are still batched up based on vsync. If our repaint requests are triggering flushes or any other significant work on the main thread then we should stop that.

The other main job of the TaskThrottler is to compute the estimated paint duration to use as part of the displayport heuristic. Botond and I talked about it and we think it makes sense to just disable this heuristic for now, and reimplement it later using a different method (passing around repaint request sequence numbers) if it will help. (We will need to land the checkerboard recording stuff first, to evaluate how much this would actually help). So I'm ok with flipping apz.use_paint_duration to false everywhere and just using the hard-coded alternate 50ms time in CalculatePendingDisplayPort.

Comment 17

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4, disable paint duration code

This removes code related to estimating the APZ paint duration, and turns off the pref that enabled it.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8697648 [details] [diff] [review]
part 4, disable paint duration code

Review of attachment 8697648 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpref/init/all.js
@@ -595,5 @@
>  pref("apz.smooth_scroll_repaint_interval", 16);
>  pref("apz.test.logging_enabled", false);
>  pref("apz.touch_start_tolerance", "0.1");
>  pref("apz.touch_move_tolerance", "0.03");
> -pref("apz.use_paint_duration", true);

If we're ripping this out entirely, remove it from gfxPrefs.h and the documentation at the top of AsyncPanZoomController.cpp as well.

Comment 19

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 5, don't throttle

This patch doesn't remove the throttler - but it removes usage in AsyncPanZoomController. Instead paints are dispatched immediately. I think this also obsoletes mLastPaintRequestMetrics. Except for an edge case where there is no GeckoContentController and mLastDispatchedPaintMetrics won't be updated, it is otherwise superceded.

I'll do the bulk of the TaskThrottler code removal in another patch so it's easier to review.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8697652 [details] [diff] [review]
part 5, don't throttle

Review of attachment 8697652 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. Two questions:
1) Does this still result in good scrolling on loop.ableton?
2) Does this cause test failures? Please do a try push with at least linux,b2g,fennec. There's a pretty high risk for failures with this patch, particularly in some of the gtests.

Comment 21

[David Anderson [:dvander] - inactive, e-mail if emergency]

Yup, scrolling still looks quite good on loop.ableton - well, as good as it's going to get without compositor help. Will post a try run shortly since this does seem risky.

Comment 22

[David Anderson [:dvander] - inactive, e-mail if emergency]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26a5ef75b80e

Comment 23

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 6, rm TaskThrottler

assuming part 5 passes (or can be made to pass) try...

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8697652 [details] [diff] [review]
part 5, don't throttle

Review of attachment 8697652 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +2798,5 @@
>  
>    // If we're trying to paint what we already think is painted, discard this
>    // request since it's a pointless paint.
> +  const FrameMetrics& previous = mLastDispatchedPaintMetrics;
> +  ScreenMargin marginDelta = (previous.GetDisplayPortMargins()

Actually I just realized that using mLastDispatchedPaintMetrics here as the compare-to metrics might result in badness. Consider the case where:

1) APZ sends a repaint request with metrics A
2) APZ sends a repaint request with metrics B
3) NotifyLayersUpdated gets called with metrics A
4) Content processes the metrics from (1) and (2), leave it with metrics B
5) APZ tries to send a repaint request with metrics A

In this sequence, at step 3, mLastDispatchedPaintMetrics could get overwritten from B back to A, which would make step 5 short-circuit and not send it. Content would have B as the latest metrics and APZ would have A as the latest metrics, and that would leave them out of sync.

I'm not sure if this will actually happen in practice, but I think it could - we've seen similar issues in the past. I'd recommend renaming mLastDispatchedPaintMetrics to something more representative like mExpectedGeckoMetrics (as in, what APZ expects the gecko metrics to be), and bring back mLastPaintRequestMetrics just to track the last metrics that actually got sent in RequestContentRepaint. The DispatchRepaintRequest would update both mExpectedGeckoMetrics and mLastPaintRequestMetrics.

I'm ok with doing this in an extra patch at the end of the queue so you don't have to rebase.

Comment 25

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 5.1, partial revert

Hrm, yeah. This is a partial revert of part 5, with the renaming suggested. I'll fold this in when landing.

Comment 26

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8697770 [details] [diff] [review]
part 5.1, partial revert

Review of attachment 8697770 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +2782,5 @@
>    aFrameMetrics.SetUseDisplayPortMargins();
>  
>    // If we're trying to paint what we already think is painted, discard this
>    // request since it's a pointless paint.
> +  const FrameMetrics& mLastPaintRequestMetrics = mExpectedGeckoMetrics;

This line ain't right, it creates a local mLastPaintRequestMetrics which shadows the instance variable. I think this line just needs to be removed, and the rest of the code can use the real mLastPaintRequestMetrics.

Comment 27

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 5.2, fix gtests

Fixes for gtest failures on try. Since we no longer have "floating" paint requests, some call counts changed, and tests checking the async transform had to be changed too.

If you think we need any additional tests for new behavior let me know.

Comment 28

[David Anderson [:dvander] - inactive, e-mail if emergency]

Try run looks okay: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fff63888680

Comment 29

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8697890 [details] [diff] [review]
part 5.2, fix gtests

Review of attachment 8697890 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine. Although part of me thinks it would be nice to still have a test to exercise async transform stuff, the other part of me says it's not really needed if we don't run into that scenario any more because we're aggressively pushing out paint requests. So I don't think we need to do anything else here.

::: gfx/layers/apz/test/gtest/TestAsyncPanZoomController.cpp
@@ +2426,5 @@
>    EXPECT_EQ(apzcroot, hit.get());
>    // transformToApzc doesn't unapply the root's own async transform
>    EXPECT_EQ(ParentLayerPoint(75, 75), transformToApzc * ScreenPoint(75, 75));
> +  // transformToGecko unapplies the full async transform of -100 pixels
> +  EXPECT_EQ(ScreenPoint(75, 75), transformToGecko * ParentLayerPoint(75, 75));

The comment here isn't right, I think. Because of the repaint request, the async transform should be empty at this point, which is why the transformToGecko doesn't have any effect. If it were really unapplying an async transform of -100 pixels then the expected output would be ScreenPoint(75, 175). So update the comment to be:

// transformToGecko should be empty because we sent the repaint request already

@@ +2434,5 @@
>    EXPECT_EQ(apzcroot, hit.get());
>    // transformToApzc doesn't unapply the root's own async transform
>    EXPECT_EQ(ParentLayerPoint(25, 25), transformToApzc * ScreenPoint(25, 25));
> +  // transformToGecko unapplies the full async transform of -100 pixels
> +  EXPECT_EQ(ScreenPoint(25, 25), transformToGecko * ParentLayerPoint(25, 25));

Ditto here

Comment 30

[Kartikaya Gupta (email:[e-mail])]

For the record, we're waiting on this until bug 1209970 lands, because otherwise the extra repaint requests from this patch will trigger more scroll events to web content, because of the code path at [1], invoked from [2].

[1] https://dxr.mozilla.org/mozilla-central/rev/e6463ae7eda2775bc84593bb4a0742940bb87379/layout/base/nsPresContext.cpp#3279
[2] https://dxr.mozilla.org/mozilla-central/rev/e6463ae7eda2775bc84593bb4a0742940bb87379/layout/generic/nsGfxScrollFrame.cpp#4256

Comment 32

[Kartikaya Gupta (email:[e-mail])]

Updated try push with dvander's github branch rebased combined with the patch from bug 1209970: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a82f9955e6e2

Comment 33

[David Anderson [:dvander] - inactive, e-mail if emergency]

Rebased again, on a dedicated branch, just in case:

https://github.com/dvander/gecko-dev/commits/rm-throttler

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdb8366d1bbc
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec1a48001e87
https://hg.mozilla.org/integration/mozilla-inbound/rev/1521ec5f317e
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ea5348f6695
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ec64106acb3
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1654855ca07

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bdb8366d1bbc
https://hg.mozilla.org/mozilla-central/rev/ec1a48001e87
https://hg.mozilla.org/mozilla-central/rev/1521ec5f317e
https://hg.mozilla.org/mozilla-central/rev/8ea5348f6695
https://hg.mozilla.org/mozilla-central/rev/3ec64106acb3
https://hg.mozilla.org/mozilla-central/rev/e1654855ca07

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8a45fa2e11e

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e8a45fa2e11e