Closed Bug 1013385 Opened 6 years ago Closed 6 years ago

Checkerboarding shows garbage

Categories

(Core :: Panning and Zooming, defect)

All
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla35
blocking-b2g 2.1+
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: mstange, Assigned: kats)

References

Details

Attachments

(4 files, 5 obsolete files)

Attached file seizure-inducing testcase (obsolete) —
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.
Attachment #8425582 - Attachment is obsolete: true
Duplicate of this bug: 785933
[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.
blocking-b2g: --- → 2.1?
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.
blocking-b2g: 2.1? → 2.1+
Assignee: nobody → bugmail.mozilla
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.
Any objections to this approach?
Attachment #8486740 - Flags: feedback?(matt.woodrow)
(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?
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.
(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.
(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.
(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.
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.
Attachment #8486740 - Attachment is obsolete: true
Attachment #8486740 - Flags: feedback?(matt.woodrow)
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.
Attachment #8487262 - Attachment is obsolete: true
Attachment #8487270 - Flags: feedback?(bgirard)
Sorry, that was the wrong file.
Attachment #8487270 - Attachment is obsolete: true
Attachment #8487270 - Flags: feedback?(bgirard)
Attachment #8487272 - Flags: feedback?(bgirard)
Attachment #8487272 - Attachment is obsolete: true
Attachment #8487272 - Flags: feedback?(bgirard)
Attachment #8487930 - Flags: review?(matt.woodrow)
Attachment #8487930 - Flags: review?(bgirard)
Attachment #8487934 - Flags: review?(matt.woodrow)
Attachment #8487934 - Flags: review?(bgirard)
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.
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).
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.
Attachment #8487930 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8487934 - Flags: review?(matt.woodrow) → review+
(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.
Attachment #8487930 - Flags: review?(bgirard) → review+
Attachment #8487934 - Flags: review?(bgirard) → review+
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.
No longer blocks: 1038214
Duplicate of this bug: 1038214
Please request Aurora approval on these patches when you get a chance.
Flags: needinfo?(bugmail.mozilla)
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
Attachment #8487930 - Flags: approval-mozilla-aurora?
Flags: needinfo?(bugmail.mozilla)
Attachment #8487934 - Flags: approval-mozilla-aurora?
No longer blocks: 1058506
Comment on attachment 8487930 [details] [diff] [review]
Add layer bounds and use them for checkerboarding (v3)

Aurora+
Attachment #8487930 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8487934 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 1070993
No longer blocks: 1070993
Depends on: 1070993
Depends on: 1074389
Depends on: 1072698
Depends on: 1073698
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.