Closed Bug 728284 Opened 8 years ago Closed 8 years ago

Main container layer has siblings that are drawn in the background

Categories

(Firefox for Android :: General, defect, P1, blocker)

11 Branch
x86
macOS
defect

Tracking

()

RESOLVED FIXED
Firefox 14
Tracking Status
blocking-fennec1.0 --- beta+

People

(Reporter: joe, Assigned: tnikkel)

References

Details

(Whiteboard: MAPLE mwc-demo [layout])

Attachments

(3 files, 1 obsolete file)

The left and right edges of the viewport (when you pan all the way to the edge of the viewport, or zoom far far out) are either black or have the texture repeated inside them (or are otherwise corrupt).

This was on Ehsan's Galaxy Nexus.
If this is a general issue, it'll presumably need to be fixed before the mwc demo
Whiteboard: MAPLE mwc-demo
Do you mean the "void beneath the web" -- i.e. not content itself, but the decorations?
I *think* so. It certainly looked as though all the content that was meant to be there was, indeed, there.
Ok. This is an issue with the Java compositor integration then.
Blocks: 728249
who should be assigned to this bug? As far as I understand, this is a mwc blocker.
Severity: normal → blocker
Priority: -- → P1
Patrick/Chris Lord/any of the graphics team who are free (which currently is the empty set).
Can't see this anymore.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
Ehsan still sees this on his Galaxy Nexus.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Still an issue here as well. Picking this up.
Assignee: nobody → bgirard
Partial fix:
https://hg.mozilla.org/projects/maple/rev/40b477c54da3

This is a small improvement but there are several bugs at play here.
No longer blocks: 728249
Assignee: bgirard → nobody
Assignee: nobody → joe
blocking-fennec1.0: --- → beta+
Assignee: joe → gwright
My findings so far:

- Putting an early return in ThebesLayerOGL::RenderLayer, ImageLayerOGL::RenderLayer and ColorLayerOGL::RenderLayer seems to make the rogue layer rendering go away, so it's one of these functions that's doing it.

- Best way to reproduce I find is to early return in ThebesLayerOGL and ImageLayerOGL, and watch for the rogue ColorLayerOGL to be rendered. Use http://www.cnn.com as a testcase, pinch zoom in to somewhere around 70% of max zoom, and scroll to the far right of the page. You will see a ColorLayer that represents the page's background in the overscroll region to the right of the page

- From the graphics side, it looks to me like we're doing the right thing and rendering the layer at the correct size and place, so I'm suspecting an issue with layouting. Joe has advised I ask Mats if he can take a look at this, so adjusting CC accordingly.

- The rogue layer isn't visible when zoomed in to the max or zoomed out to the max. I believe this is due to the x-position we're drawing the layer at; at min zoom the layer draws straight on top of the content and at max zoom it renders off to the right of the viewport. At zoom levels in between you can quite clearly see the layer moving to the left as the zoom level decreases, which suggests to me that we're applying the wrong transform to the page coordinates for the layer to compensate for zoom level.
Comment on attachment 604549 [details] [diff] [review]
Bug 728284 - Hide any sibling layers to the main container layer, as these are created by the XUL UI which is not applicable on Fennec. r=BenWa?

Can we just not create the layers?
(In reply to Timothy Nikkel (:tn) from comment #13)
> Comment on attachment 604549 [details] [diff] [review]
> Bug 728284 - Hide any sibling layers to the main container layer, as these
> are created by the XUL UI which is not applicable on Fennec. r=BenWa?
> 
> Can we just not create the layers?

I believe there's a separate bug for that, but I'm not sure off hand. We've looked at that and have no idea where to do it, and would love that patch and/or any pointers...
Attachment #604549 - Flags: review?(matt.woodrow)
Comment on attachment 604549 [details] [diff] [review]
Bug 728284 - Hide any sibling layers to the main container layer, as these are created by the XUL UI which is not applicable on Fennec. r=BenWa?

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

I agree that removing these layers is the ideal fix, but I'm not sure where that is. I'd like to this patch meanwhile.
Attachment #604549 - Flags: review?(bgirard) → review+
If you desperately need this for a deadline, then I'd be fine landing this, otherwise I think we need to look into stopping them being created in the first place.
We need someone from layout to figure out how we can not create these layers. -> mats
Assignee: gwright → matspal
Whiteboard: MAPLE mwc-demo → MAPLE mwc-demo [gfx]
Duplicate of this bug: 735263
Summary: Edges of viewport are black/repeated/corrupt → Main container layer has siblings that are drawn in the background
Duplicate of this bug: 735303
Whiteboard: MAPLE mwc-demo [gfx] → MAPLE mwc-demo [layout]
Duplicate of this bug: 737407
Status: REOPENED → ASSIGNED
Workaround landed for now (with giant FIXME comment) until a real fix is applied.
Do we feel good enough about the workaround that the remaining work needn't block fennec beta? If so, any chance of resolving this and moving the real fix to a follow up, if only for the glee of cleaner bookkeeping?
Mats/tn: please review the FIXME code and comment on its viability. We're burning cycles creating these bogus layers but it's unclear how bad that is.
(Leaving open for comment 23, comment 24)

https://hg.mozilla.org/mozilla-central/rev/c210ff20f371
Target Milestone: --- → Firefox 14
Attachment #604549 - Flags: review?(matt.woodrow)
Attachment #604549 - Flags: feedback?(matt.woodrow)
Attachment #604549 - Flags: feedback?(matt.woodrow) → feedback?(matspal)
Attachment #604549 - Flags: feedback?(matspal) → feedback?(roc)
I don't know this code well enough to fix this, sorry.
Assignee: matspal → nobody
This patch is obviously wrong. We just want to find out what's generating these layers, so we know how to make them go away.
A display list dump (with layer dump) (gfxUtils::sDumpPaintList set to true in a debug build) should shed light on why the rogue layers are being reated.
Assignee: nobody → gwright
Attached file paintlist
Dump as requested by roc
Attachment #613684 - Attachment filename: paintlist.dump → paintlist.html
Attachment #613684 - Attachment mime type: text/plain → text/html
Assigning to tn as we've just worked through this and he has a plan of attack.
Assignee: gwright → tnikkel
George and I debugged this and figured out what was going on.

We have code that makes sure we draw something opaque to the whole window when we are drawing to an opaque widget. If the content in that window (specified by XUL/HTML/CSS) fails to draw something opaque over the whole window then we use the default background color as defined by the prescontext to draw to the window. The problem is that the layer manager draws the checkerboarding by calling nsWindow::DrawWindowUnderlay just before drawing the layer tree. So it needs there to be nothing drawn by the content in the layer tree on top of the checkerboarding that nsWindow::DrawWindowUnderlay draws.
(In reply to Timothy Nikkel (:tn) from comment #31)
> George and I debugged this and figured out what was going on.
> 
> We have code that makes sure we draw something opaque to the whole window
> when we are drawing to an opaque widget. If the content in that window
> (specified by XUL/HTML/CSS) fails to draw something opaque over the whole
> window then we use the default background color as defined by the
> prescontext to draw to the window.

Why does the code that makes sure we draw something opaque need to use a separate layer? Why can't it just make sure the root ThebesLayer is filled with this solid color?
The code I'm talking about uses the display list infrastructure, so it goes through the same process as all display items. We have code that uses a color layer if the created thebes layer would be a single uniform color. If we disable this code we would just get a single thebes layer that had this color. But that wouldn't fix this problem.
Attached patch patch? (obsolete) — Splinter Review
I tested this with a backout of the hack but I couldn't see what the problems were even without this patch (but with the backout).
(In reply to Timothy Nikkel (:tn) from comment #34)
> Created attachment 613847 [details] [diff] [review]
> patch?
> 
> I tested this with a backout of the hack but I couldn't see what the
> problems were even without this patch (but with the backout).

I'll give it a whirl tomorrow on my test device
I wonder if this is impacted by Joe's change to a solid color background?
(In reply to JP Rosevear [:jpr] from comment #36)
> I wonder if this is impacted by Joe's change to a solid color background?

Nope. That's done by the java code which doesn't know anything about what's going on in layout.
\o/ the patch seems to work (tested on an asus transformer running ics)
(In reply to Timothy Nikkel (:tn) from comment #33)
> The code I'm talking about uses the display list infrastructure, so it goes
> through the same process as all display items. We have code that uses a
> color layer if the created thebes layer would be a single uniform color. If
> we disable this code we would just get a single thebes layer that had this
> color. But that wouldn't fix this problem.

Here lies my confusion:

Assumptions:
1. The content in a browser element should always be opaque.
2. We don't create layers for content that is completely obscured by other content.

Shouldn't this imply that we should not be creating this layer in the first place? Or, under what circumstances would we normally see this color layer and with your patch won't checkerboard or what ever is under the content show through?
Comment on attachment 613847 [details] [diff] [review]
patch?

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

My understanding of this bug is that we're currently painting a background color over the displayport *of the chrome document*. Is that correct?

Can we not fix this just by telling the mobile browser's chrome document to make its widget transparent?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #40)
> Can we not fix this just by telling the mobile browser's chrome document to
> make its widget transparent?

We could do that, but the widget isn't actually transparent, and it would have other consequences, but I'm not sure if they are important consequences or not.
Comment on attachment 613847 [details] [diff] [review]
patch?

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

Alright, let's do this, but call the method LayerManagerPaintsBackground instead of NeedOpaqueContent.
Although I suppose it would make more sense for LayerManagerPaintsBackground to actually be a method on LayerManager. What do you think?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #43)
> Although I suppose it would make more sense for LayerManagerPaintsBackground
> to actually be a method on LayerManager. What do you think?

Yeah that makes the most sense, but the widget is the one that has the DrawWindowUnderlay function that actually paints the background, so it is the one that knows if it paints the background or not, not the layer manager.

Jeff thinks we should investigate why the problematic display item isn't optimized away as not visible. His argument is that from layout's point of view it should actually be completely covered by the content document (even though this isn't true in reality, layout should think it's true based on the information that is given to layout).
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #42)
> Alright, let's do this, but call the method LayerManagerPaintsBackground
> instead of NeedOpaqueContent.

Alright, let's call it WidgetPaintsBackground.
Attached patch patchSplinter Review
Attachment #613847 - Attachment is obsolete: true
Attachment #615526 - Flags: review?(roc)
Attachment #613847 - Flags: review?(roc)
Comment on attachment 615526 [details] [diff] [review]
patch

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

::: widget/nsIWidget.h
@@ +1596,5 @@
>      virtual bool HasGLContext() { return false; }
>  
> +    /**
> +     * Returns true to indicate that this widget paints an opaque background
> +     * so layout doesn't need to worry about that.

"paints an opaque background that we want to be visible under the page, so layout should not force a default background."
Attachment #615526 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/8951d7e2d564
Status: ASSIGNED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Backout of previous hack: https://hg.mozilla.org/integration/mozilla-inbound/rev/6b917c007510
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: FIXED → ---
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
(In reply to George Wright (:gw280) from comment #50)
> Backout of previous hack:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6b917c007510

https://hg.mozilla.org/mozilla-central/rev/6b917c007510

(Landed with the wrong bug number)
Could this have cause bug 765677?
You need to log in before you can comment on or make changes to this bug.