Closed Bug 1251638 Opened 8 years ago Closed 8 years ago

Bad checkerboarding when scrolling upwards on horizontally scrollable code on GitHub

Categories

(Core :: Panning and Zooming, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox46 --- wontfix
firefox47 --- verified
firefox48 --- verified

People

(Reporter: botond, Assigned: kats)

References

()

Details

(Keywords: perf, Whiteboard: [gfx-noted])

Attachments

(4 files, 3 obsolete files)

STR:
  1. Load https://github.com/mozilla/gecko-dev/blob/master/layout/base/FrameLayerBuilder.cpp
  2. Scroll the code horizontally, to trigger the creation of a displayport 
     on the horizontally scrollable subframe.
  3. Scroll the page up and down (before the subframe displayport expires).

Expected results:
  The page checkerboards as little as when you haven't scrolled the subframe.

Actual results:
  The page checkerboards significantly more.
  This is particularly visible when scrolling up, where the checkerboarding is really bad.
(This was observed by Jeff on his HiDPI MacBook. It's much harder to reproduce on my regular DPI Linux laptop.)
See Also: → 1256133
I investigated this. I can reproduce "really bad" checkerboarding when scrolling back up to the top of the page. In particular, after I reach the top of the page, the inner frame is usually blank for a noticeable long time, like a second, before it fills in.

I think what's happening here is that the displayport base for the inner frame doesn't get updated frequently enough while we're scrolling the outer frame. There's only one place in the code that updates the displayport base [1], and that only gets run when we do a display list build. If we're scrolling upwards fast, the velocity bias keeps the displayport for the outer frame locked to the top, so as of bug 1192910, we no longer do paints (because the displayport isn't moving), and so the inner frame's displayport base doesn't get updated. Once we reach the top, something or the other eventually triggers a paint, and that's when the displayport base gets updated and it fills in.

Note that I can also intermittently reproduce a perma-checkerboard state on the inner frame if I scroll upwards at a medium speed, presumably for the same reason - there is nothing updating the inner frame's displayport base.
Assignee: nobody → bugmail.mozilla
As expected, turning off paint skipping fixes both the lag and the perma-checkerboard state, because we do paints more frequently, and so the inner frame's displayport base gets updated more frequently. However I don't see the same problem when I hit the bottom of the page, which I would expect, so there's more going on here. Still digging.
Ah the problem is only visible when scrolling upwards, because the displayport margins on the inner frame are entirely in the "bottom" direction. The scroll-y position of the inner frame is always 0, so the margins calculation always puts the margins on the bottom. So anytime you scroll slowly upwards on the page you're liable to go into checkerboarding on the inner frame before the outer frame rolls over to the next displayport "tile" and triggers a repaint. So actually just centering the displayport margins around the displayport base on the layout side would mostly solve this problem, but may not be the best solution.
Also I suspect fixing this will aggravate bug 1256133 and bug 1243081, which I suspect are the same issue, caused by us basically uploading the content area twice. The layer is marked as [componentAlpha] for me, so presumably we have to upload that area twice (once in each layer) if we want it layerized for APZ scrolling. But we also landed bug 1254273 which should help somewhat with that.
This fixes the issue when you're partway down the page and scrolling upwards, because it allows the subframe to have some "top" margin even though the y-scroll-offset in the subframe is 0. There's still the issue that as you get to the top of the page, and the displayport can't move up any more, we stop triggering paints. This leaves the displayport base rect on the subframe a little too far down for the displayport to extend all the way to the top of the subframe. Thinking about what to do for that.
Summary: Bad checkerboarding when (vertically) scrolling horizontally scrollable code on GitHub → Bad checkerboarding when scrolling upwards on horizontally scrollable code on GitHub
The patch on bug 1259593 (combined with the one I posted here) seems to help things enough that I can't reproduce long checkerboards any more.
Depends on: 1259593
Attachment #8734477 - Flags: review?(botond)
Comment on attachment 8734477 [details] [diff] [review]
Part 1 - Don't clamp displayport on the compositor side

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

This is some subtle stuff!

My understanding of the issue is, that the *position* of the displayport as calculated in APZ (based on the scroll frame's scroll offset plus some predicted movement amount) can be effectively bogus, as the displayport is *actually* positioned not relative to origin of the scroll port, but relative to the intersection of the scroll port with the root composition bounds. Since the position is potentially wrong, we shouldn't be clamping based on it.

Assuming I've understood correctly, r+, but please add a comment to CalculatePendingDisplayPort() about the position being inaccurate in this way, so we don't write code in the future that relies on an accurate position there.
Attachment #8734477 - Flags: review?(botond) → review+
Yeah that's correct. We can actually simplify the code somewhat by removing the "scrollOffset" addition/subtraction entirely, and I found it easier to follow if I switched around a couple of the statements. I also discovered that this patch causes some gtests to fail because now we're calling RequestContentRepaint some more times, so I need to update the tests to match.
The clamping already happens on the content side, in nsLayoutUtils::GetDisplayPort
and friends. The clamping there is more accurate as it reflects the latest main-
thread information about the size of the page and position of the displayport
base rect, which the compositor thread does not have.

Since we are not clamping the displayport on the compositor side, it can "slosh
around" a bit more and ends up sending a few more repaint requests when scrolling
near the edges of the scrollable frame. This causes some gtests to fail because
of the "extra" repaint requests. Disabling the velocity bias removes the
sloshing around and fixes the test failures.

Review commit: https://reviewboard.mozilla.org/r/43333/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43333/
Attachment #8736514 - Flags: review?(botond)
Attachment #8736515 - Flags: review?(botond)
Comment on attachment 8734477 [details] [diff] [review]
Part 1 - Don't clamp displayport on the compositor side

Whoops, for some reason I thought I had submitted this via MozReview initally.
Attachment #8734477 - Attachment is obsolete: true
Comment on attachment 8736514 [details]
MozReview Request: Bug 1251638 - Don't clamp the displayport to the scrollable rect on the compositor side. r?botond

https://reviewboard.mozilla.org/r/43333/#review39953

::: gfx/layers/apz/test/gtest/TestPanning.cpp:83
(Diff revision 1)
>    }
>  };
>  
>  TEST_F(APZCPanningTester, Pan) {
>    SCOPED_GFX_PREF(TouchActionEnabled, bool, false);
> +  SCOPED_GFX_PREF(APZVelocityBias, float, 0.0); // Velocity bias can cause extra repaint requests

Stay tuned for an alternative that avoids this repetition.
Attachment #8736514 - Flags: review?(botond) → review+
Comment on attachment 8736515 [details]
MozReview Request: Bug 1251638 - Do a bit of cleanup on the displayport margin computation in APZC; no functional changes. r?botond

https://reviewboard.mozilla.org/r/43335/#review39951

Much nicer, thanks!

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:2830
(Diff revision 1)
>  
> +  // We calculate a "displayport" here which is relative to the scroll offset.
> +  // Note that the scroll offset we have here in the APZ code may not be the
> +  // same as the base rect that gets used on the layout side when the displayport
> +  // margins are actually applied, so it is important to only consider the
> +  // displayport as margins relative to something rather than an absolute rect.

"relative to something rather than an absolute rect" is pretty vague. How about "relative to the scroll offset rather than relative to the scrollable rect origin"?
Attachment #8736515 - Flags: review?(botond) → review+
The promised alternative that avoids the repetitve setting of the velocity bias pref. Feel free to use if you like it.
Comment on attachment 8736519 [details] [diff] [review]
Support for scoped prefs that are per-fixture rather than per-test

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

Nifty :) If we end up doing this a lot it seems like a good idea, but for now I feel like it might be overkill.
Final patch for landing, to go into the landing queue.
Attachment #8736514 - Attachment is obsolete: true
Attachment #8736883 - Flags: review+
Attached patch Part 2 - CleanupSplinter Review
Attachment #8736515 - Attachment is obsolete: true
Attachment #8736886 - Flags: review+
Comment on attachment 8736883 [details] [diff] [review]
Part 1 - Don't clamp displayport on the compositor side

Approval Request Comment
[Feature/regressing bug #]: APZ
[User impact if declined]: On some pages with large scrollable subframes it's easier to checkerboard when scrolling around
[Describe test coverage new/current, TreeHerder]: covered by tests
[Risks and why]: pretty low risk, the actual change is just one line
[String/UUID change made/needed]: none
Attachment #8736883 - Flags: approval-mozilla-aurora?
Attachment #8736886 - Flags: approval-mozilla-aurora?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> The patch on bug 1259593 (combined with the one I posted here) seems to help
> things enough that I can't reproduce long checkerboards any more.

I backed out bug 1259593 because it made things worse in other cases. However I think the patches on this bug are still a net win and worth uplifting. There is still an issue with the subframe's base rect not shifting when scrolling the outer frame near the end of its scroll range, I'll file another bug for that.
No longer depends on: 1259593
Comment on attachment 8736883 [details] [diff] [review]
Part 1 - Don't clamp displayport on the compositor side

APZ related recent regression, has automated tests, Aurora47+
Attachment #8736883 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8736886 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Botond, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(botond)
(In reply to Ritu Kothari (:ritu) from comment #25)
> Botond, could you please verify this issue is fixed as expected on a latest
> Nightly build? Thanks!

The issue appears fixed on my Linux laptop, but given comment 1...

(In reply to Botond Ballo [:botond] from comment #1)
> (This was observed by Jeff on his HiDPI MacBook. It's much harder to
> reproduce on my regular DPI Linux laptop.)

... we better ask Jeff to be sure.
Flags: needinfo?(botond) → needinfo?(jmuizelaar)
Scrolling on github is still bad but I believe the original problem is fixed.
Flags: needinfo?(jmuizelaar)
Scrolling on github looks better on Firefox 47 beta 9 and latest Aurora 48.0a2 that in Nightly 47.0a1.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.