Markus Stange [:mstange] Reporter
	Description • 10 years ago

This is with the patches in bug 944938 applied:

By scrolling quickly enough, for example in the attached testcase, you can move parts of the page into view that haven't been painted yet. Usually I see these parts being filled with white.
However, sometimes I see frantic flashing of the previous window contents in these parts. Apparently the old window content doesn't get erased at all! The quick flashing is probably a result of double buffering.

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

[Blocking Requested - why for this release]: I think this should probably be blocking-b2g: backlog but I can't request backlog? so requesting 2.1?

It's important enough because not having a proper checkerboarding indicator results in visual glitches like bug 1038214.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 5 • 10 years ago

Why 2.1+ : Showing "random" stuff on the screen (see also bug 1058506 for a related STR) is both a major issue, and technically a regression.

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

This bug is actually turning out to be quite hard to fix, and I don't think there's enough information on the layers API to actually cover up the checkerboard area in the compositor.

My method of reproducing this is to use a desktop OS X build with APZ enabled, the attached patch applied, going to http://blog.wanderview.com/ and scrolling down. At some point the displayport ends and you end up being able to see the fixed-position layer underneath instead of seeing checkerboarding.

What I would expect to happen here is that the scrolling content layer should be "filled in" with the background color or a checkerboard pattern, but we don't know how large the content in that layer is supposed to be. The ContainerLayer that holds it has a FrameMetrics, but the ContainerLayer has 4 children, only one of which should have the checkerboard. Distinguishing that one out of the 4 in a generic way seems nontrivial to do.

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

Any objections to this approach?

	Matt Woodrow (:mattwoodrow)
	Comment 8 • 10 years ago

(In reply to Markus Stange [:mstange] from comment #0)
> Created attachment 8425582 [details]
> seizure-inducing testcase
> 
> This is with the patches in bug 944938 applied:
> 
> By scrolling quickly enough, for example in the attached testcase, you can
> move parts of the page into view that haven't been painted yet. Usually I
> see these parts being filled with white.
> However, sometimes I see frantic flashing of the previous window contents in
> these parts. Apparently the old window content doesn't get erased at all!
> The quick flashing is probably a result of double buffering.

We should be doing glClear() in CompositorOGL::BeginFrame, I'd expect that to stop this from happening?

	Matt Woodrow (:mattwoodrow)
	Comment 9 • 10 years ago

Comment on attachment 8486740 [details] [diff] [review]
Add layer bounds and use them for checkerboarding

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

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +266,5 @@
> +    // TODO: do a better job of skipping this for layers that don't need it,
> +    // and clipping the rect to the user-visible portion. Also restrict it to
> +    // thebes layers that can be async-scrolled? Also Make sure we still run
> +    // this code if the layer is entirely offscreen (culling code needs updating)
> +    gfxRGBA check(1.0f, 0.0f, 0.0f, 1.0f); // TODO: use a proper checkerboarding pattern or background color

What do we do if the layer is transparent?

::: layout/base/FrameLayerBuilder.cpp
@@ +2332,5 @@
>    FLB_LOG_THEBES_DECISION(this, "Accumulating dp=%s(%p), f=%p against tld=%p\n", aItem->Name(), aItem, aItem->Frame(), this);
>  
> +  bool snap;
> +  nsRect itemBounds = aItem->GetBounds(aState->mBuilder, &snap);
> +  mBounds.OrWith(aState->ScaleToOutsidePixels(itemBounds, snap));

For layers that have a single background item that cover the whole area then this should work. Transparent layers that don't have a background color might need all the display items available in order to get the correct size. Since we cull the display list to only things visible within the display-port then we might be missing some here.

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #9)
> What do we do if the layer is transparent?

Keep walking up until we find a non-transparent background? Or I could just default to white or something. If we have a checkerboard pattern to use (UX preferred this) then we would just use that.

> For layers that have a single background item that cover the whole area then
> this should work. Transparent layers that don't have a background color
> might need all the display items available in order to get the correct size.
> Since we cull the display list to only things visible within the
> display-port then we might be missing some here.

Yeah, I'm hoping that in practice we won't run into this case too often. We should only run into it for transparent layers, like you said, in which case it should be more acceptable to display the layers underneath instead of checkerboard anyway.

	Markus Stange [:mstange] Reporter
	Comment 11 • 10 years ago

(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> (In reply to Markus Stange [:mstange] from comment #0)
> > Created attachment 8425582 [details]
> > seizure-inducing testcase
> > 
> > This is with the patches in bug 944938 applied:
> > 
> > By scrolling quickly enough, for example in the attached testcase, you can
> > move parts of the page into view that haven't been painted yet. Usually I
> > see these parts being filled with white.
> > However, sometimes I see frantic flashing of the previous window contents in
> > these parts. Apparently the old window content doesn't get erased at all!
> > The quick flashing is probably a result of double buffering.
> 
> We should be doing glClear() in CompositorOGL::BeginFrame, I'd expect that
> to stop this from happening?

It's broken at the moment due to bug 1065256.

	Matt Woodrow (:mattwoodrow)
	Comment 12 • 10 years ago

(In reply to Markus Stange [:mstange] from comment #11)
> It's broken at the moment due to bug 1065256.

Ok, makes sense. My patch in bug 1063776 will fix this too.

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

Updated to restrict drawing the background color to when we know we're checkerboarding. Also updated the culling code to not cull checkerboarding layers because otherwise they get skipped entirely and we don't draw the background.

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

I think this is pretty close to the final patch I would want for this, so requesting feedback in case I screwed up somewhere. I will write some tests this evening (PTO this afternoon). If we want to merge ColorLayer::mBounds and CanvasLayer::mBounds into the new mLayerBounds then I'd rather do that in a follow-up to reduce uplift risk. I'm still not sure if that should be done or not, since I haven't looked closely at where the Color/Canvas bounds are set and used.

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

Sorry, that was the wrong file.

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

Pending try push with just the tests to demonstrate they fail without any changes:
https://tbpl.mozilla.org/?tree=Try&rev=3b09d84269e3

Pending try push with patch + tests:
https://tbpl.mozilla.org/?tree=Try&rev=6d075e4ccbfc

I verified these tests with my local B2G emulator setup and they do what I expect them to.

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

Try builds also show the expected behaviour (the first one in comment 18 has the three new tests failing, the second one in comment 18 has them passing).

	Matt Woodrow (:mattwoodrow)
	Comment 20 • 10 years ago

Comment on attachment 8487930 [details] [diff] [review]
Add layer bounds and use them for checkerboarding (v3)

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

r+, for the layers/FrameLayerBuilder changes.

	Matt Woodrow (:mattwoodrow)
	Comment 21 • 10 years ago

Should we restrict async scrolling to the range that we have layer bounds for? Otherwise it seems like we could still hit this bug in some cases.

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

(In reply to Matt Woodrow (:mattwoodrow) (PTO until 22nd Sept) from comment #21)
> Should we restrict async scrolling to the range that we have layer bounds
> for? Otherwise it seems like we could still hit this bug in some cases.

Interesting thought. I'm not sure that would always be correct, though, because the layer bounds might legitimately not take up the full scrollable rect. For example, consider a scrollable div which contains a transparent div sized to 10000px tall, but only contains one line of text. The scrollable rect would still be 10000px tall but the bounds of the only scrolling layer would be that one line of text. Restricting the async scrolling here would be wrong, I think.

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

Rebased to master and pushed a new try push just to make sure stuff is still good:
https://tbpl.mozilla.org/?tree=Try&rev=62d50b7f3902

Will land on inbound if that's clean.

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

It was clean but inbound was closed, so landed on b2g-inbound:

https://hg.mozilla.org/integration/b2g-inbound/rev/0a53e55db53d
https://hg.mozilla.org/integration/b2g-inbound/rev/3d4b867d6bc7

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 25 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/0a53e55db53d
https://hg.mozilla.org/mozilla-central/rev/3d4b867d6bc7

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 10 years ago

Please request Aurora approval on these patches when you get a chance.

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

Comment on attachment 8487930 [details] [diff] [review]
Add layer bounds and use them for checkerboarding (v3)

Approval Request Comment
[Feature/regressing bug #]: async scrolling
[User impact if declined]: when encountering "checkerboarding" by scrolling too fast, the checkerboarded area might show random garbage or other content from underneath which shouldn't be visible. this is a transient state but still very noticeable on some pages (see dupes)
[Describe test coverage new/current, TBPL]: added reftests in the other patch; tested locally as well
[Risks and why]: low-ish risk; i wrote the patch to be conservative so that we we err on the side of false-negatives rather than false-positives. That is, the patch will not fix all the occurrences of this bug, but it shouldn't impact any cases where the bug isn't present.
[String/UUID change made/needed]: none

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 29 • 10 years ago

Comment on attachment 8487930 [details] [diff] [review]
Add layer bounds and use them for checkerboarding (v3)

Aurora+

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/315cb80abb1e
https://hg.mozilla.org/releases/mozilla-aurora/rev/6d8a53b071f7

	Corinne Innes [:CorinneI]
	Comment 31 • 10 years ago

Issue is verified fixed on 2.1 and 2.2

Flame 2.1 

Device: Flame 2.1 KK (319mb) (Full Flash)
BuildID: 20141012001201
Gaia: d18e130216cd3960cd327179364d9f71e42debda
Gecko: 610ee0e6a776
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Flame 2.2 

Device: Flame 2.2 Master  KK (319mb) (Full Flash)
BuildID: 20141012040203
Gaia: 717ad4e8b7fc10ab8248500d00ba5ba0977fa8ab
Gecko: 44168a7af20d
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Viewed attachment 8425586 [details] in browser app and quickly scrolled up/down and left/right. Page loaded as it should.