Bug 70564 – [Chromium] Make root layer always opaque

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[Chromium] Make root layer always opaque

[Dana Jansens]

Created attachment 111875 [details]
Patch

[Dana Jansens]

Attempt to make the root layer marked as opaque, for #70085.

[James Robinson]

Comment on attachment 111875 [details]
Patch

I'd rather we set the bit instead of overriding what 'opaque()' returns.  Can you look at where we set the "NonComposited" bit and call setOpaque() there instead (http://google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/platform/graphics/chromium/NonCompositedContentHost.cpp&q=setIsNonCompo&exact_package=chromium&l=45)?

[Dana Jansens]

Here's why I thought it would be better to override opaque() instead:  If we set the bit for a NonComposited surface, we will have have to put checks in the code in every other place that unsets the bit to check if it's NonComposited.  And that makes more special case code than we have now. Am I wrong about this?

If not, we could check isNonComposited() rather than !m_parent in opaque().

I had thought you meant to remove the NonCompositedLayer entirely when you talked about removing special cases, but I guess that wasn't your intent.

[James Robinson]

(In reply to comment #4)
> Here's why I thought it would be better to override opaque() instead:  If we set the bit for a NonComposited surface, we will have have to put checks in the code in every other place that unsets the bit to check if it's NonComposited.  And that makes more special case code than we have now. Am I wrong about this?
> 
> If not, we could check isNonComposited() rather than !m_parent in opaque().
> 
> I had thought you meant to remove the NonCompositedLayer entirely when you talked about removing special cases, but I guess that wasn't your intent.

Sorry, I wasn't expressive enough.

The current state of the world is that we have a NonCompositedContentHost object that hosts a GraphicsLayer and so, indirectly, a LayerChromium.  We want this LayerChromium to have some special behaviors (blending disabled, no border texels).  The way that is done today is by setting a special flag on the LayerChromium which is then propagated through the system and used in various places.

I think this is wrong and we should instead have NonCompositedContentHost set the more relevant bits for the behaviors it wants.  For example, to disable blending it should just set the opaque bit and then the compositor would know to disable blending.  Similarly we should be able to detect when we need border texels without relying on this bit (although that's a separate issue).

So my feedback is this: change NonCompositedContentHost to set the opaque bit correctly on its LayerChromium and then use that to control blending instead of the isNonCompositedContent() bit.  You shouldn't be messing around with how isOpaque() works, that should definitely still just be a bool on LayerChromium that works in the normal way.

[Dana Jansens]

One question: should a root layer that is rotated 45 degrees have blending enabled to anti-alias its edges?

[Dana Jansens]

Created attachment 117277 [details]
Patch

I didn't change the blending decision in this patch, because the blending decision in LayerRenderChromium needs to be made based on rotation transform, and I'm not clear if this should affect non-composited layers or how. Bug #72965 adds some functions to decide if a layer is being drawn opaque, and they could be used if the decision should be the same for noncompositing opaque layers as for other opaque layers. There are no tests for this right now, since the opaque flag is overridden by isNonComposited still, so layout tests won't see any change. I could make a unit test but I'm not sure it's possible to properly instantiate a NonCompositedContentHost in a unit test since it touches non-chromium WebCore stuff, is it?

[James Robinson]

(In reply to comment #6)
> One question: should a root layer that is rotated 45 degrees have blending enabled to anti-alias its edges?

We don't support rotating the root layer or edge AA'ing it.

[James Robinson]

Sorry for being thick but why can't we just call GraphicsLayer::setContentsOpaque() from NonCompositedContentHost?

[James Robinson]

Example: http://google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit2/WebProcess/WebPage/ca/LayerTreeHostCA.cpp&q=setContentsOpaque&ct=rc&exact_package=chromium&cd=14&sq=&l=68

[Dana Jansens]

(In reply to comment #9)
> Sorry for being thick but why can't we just call GraphicsLayer::setContentsOpaque() from NonCompositedContentHost?

Hm. I thought we can't because the RenderLayerBacking will be setting contentsOpaque true/false based on the contents. But perhaps there is no backing since it is non-composited. I will read more code and verify this, thanks.

[Dana Jansens]

(In reply to comment #11)
> (In reply to comment #9)
> > Sorry for being thick but why can't we just call GraphicsLayer::setContentsOpaque() from NonCompositedContentHost?
> 
> Hm. I thought we can't because the RenderLayerBacking will be setting contentsOpaque true/false based on the contents. But perhaps there is no backing since it is non-composited. I will read more code and verify this, thanks.

So the non-composited content host does have a backing.
RenderLayerCompositor::updateBacking() creates the NonCompositingContentHost, via RenderLayerCompositor::enableCompositingMode(). Then it creates a backing for it via RenderLayer::ensureBacking().

So, if we can know the NCCH will have contents that cause it to be set opaque in RenderLayerBacking eventually (eg. solid background color, background-clip:border-box), I can just add a call in NCCH to setContentsOpaque(true) until the RenderLayerBacking stuff lands, and then remove it. I am not sure how to verify this one though.

[Dana Jansens]

(In reply to comment #12)
> So, if we can know the NCCH will have contents that cause it to be set opaque in RenderLayerBacking eventually (eg. solid background color, background-clip:border-box), I can just add a call in NCCH to setContentsOpaque(true) until the RenderLayerBacking stuff lands, and then remove it.

The NCCH has a background color of 0, which is transparent. I am guessing this is the default color given to things which cannot be styled. This makes the RenderLayerBacking mark the GraphicsLayer as non-opaque, over-riding the flag set in the NCCH constructor.

It would be possible to change the color on the NCCH, though I think this is easier. Maybe you know something more about the situation also, so what do you think of this all?

[James Robinson]

The NCCH's GraphicsLayer does not have any associated RenderLayerBacking. I'm a bit surprised that RenderLayerCompositor can manipulate it at all. Can you show a callstack of RLB overriding the opaque flag? I wonder if we're confusing some layers somewhere.

The GraphicsLayer tree should look something like this:

(A) GraphicsLayer - NonCompositedContentHost
  (B) GraphicsLayer - root of composited tree

RLC/RLB should only be messing with GraphicsLayer (B) and below. The GraphicsLayerClient for (A) is the NonCompositedContentHost

[Dana Jansens]

Wall of text incoming.

Here's backtrace for creating the NCCH:
#0  WebCore::NonCompositedContentHost::create (contentPaint=...) at ../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/NonCompositedContentHost.h:51
#1  0x00007f907f77be4a in WebKit::WebViewImpl::setIsAcceleratedCompositingActive (this=0x7f906f7ddc80, active=true) at ../../third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp:2857
#2  0x00007f907f77b439 in WebKit::WebViewImpl::setRootGraphicsLayer (this=0x7f906f7ddc80, layer=0x7f906f8f8200) at ../../third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp:2717
#3  0x00007f907f6b2e5b in WebKit::ChromeClientImpl::attachRootGraphicsLayer (this=0x7f906f7ddcd0, frame=0x7f90737b2800, graphicsLayer=0x7f906f8f8200) at ../../third_party/WebKit/Source/WebKit/chromium/src/ChromeClientImpl.cpp:813
#4  0x00007f90803ac8c6 in WebCore::RenderLayerCompositor::attachRootLayer (this=0x7f906f7bfe70, attachment=WebCore::RenderLayerCompositor::RootLayerAttachedViaChromeClient)
    at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp:1957
#5  0x00007f90803ac348 in WebCore::RenderLayerCompositor::ensureRootLayer (this=0x7f906f7bfe70) at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp:1897
#6  0x00007f90803a5a6f in WebCore::RenderLayerCompositor::enableCompositingMode (this=0x7f906f7bfe70, enable=true) at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp:127
#7  0x00007f90803a64ac in WebCore::RenderLayerCompositor::updateBacking (this=0x7f906f7bfe70, layer=0x7f906f7f2178, shouldRepaint=WebCore::RenderLayerCompositor::CompositingChangeRepaintNow)
    at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp:354
#8  0x00007f90803a677b in WebCore::RenderLayerCompositor::updateLayerCompositingState (this=0x7f906f7bfe70, layer=0x7f906f7f2178, shouldRepaint=WebCore::RenderLayerCompositor::CompositingChangeRepaintNow)
    at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp:424
#9  0x00007f908039665e in WebCore::RenderLayer::styleChanged (this=0x7f906f7f2178, oldStyle=0x0) at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayer.cpp:4296

The RenderLayer is 0x7f906f7f2178.

Here's the backtrace for creating the RLB on the same layer:
#0  WebCore::RenderLayer::ensureBacking (this=0x7f906f7f2178) at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayer.cpp:3867
#1  0x00007f90803a651e in WebCore::RenderLayerCompositor::updateBacking (this=0x7f906f7bfe70, layer=0x7f906f7f2178, shouldRepaint=WebCore::RenderLayerCompositor::CompositingChangeRepaintNow)
    at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp:366
#2  0x00007f90803a677b in WebCore::RenderLayerCompositor::updateLayerCompositingState (this=0x7f906f7bfe70, layer=0x7f906f7f2178, shouldRepaint=WebCore::RenderLayerCompositor::CompositingChangeRepaintNow)
    at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp:424
#3  0x00007f908039665e in WebCore::RenderLayer::styleChanged (this=0x7f906f7f2178, oldStyle=0x0) at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayer.cpp:4296

Here's the backtrace setting opaque in the backing owned by that same layer:
#0  WebCore::GraphicsLayerChromium::setContentsOpaque (this=0x7f906f8f8e00, opaque=false) at ../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:262
#1  0x00007f90803a1d62 in WebCore::RenderLayerBacking::updateContentsOpaque (this=0x7f906aff0850, opaqueRegion=...) at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayerBacking.cpp:1009
#2  0x00007f908039e6e0 in WebCore::RenderLayerBacking::updateCompositedBounds (this=0x7f906aff0850) at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayerBacking.cpp:236
#3  0x00007f90803a8d7a in WebCore::RenderLayerCompositor::rebuildCompositingLayerTree (this=0x7f906f7bfe70, layer=0x7f906f7f2178, compositingState=..., childLayersOfEnclosingLayer=...)
    at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp:948
#4  0x00007f90803a9112 in WebCore::RenderLayerCompositor::rebuildCompositingLayerTree (this=0x7f906f7bfe70, layer=0x7f906f7f22d8, compositingState=..., childLayersOfEnclosingLayer=...)
    at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp:1009
#5  0x00007f90803a9112 in WebCore::RenderLayerCompositor::rebuildCompositingLayerTree (this=0x7f906f7bfe70, layer=0x7f906f7f29b8, compositingState=..., childLayersOfEnclosingLayer=...)
    at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp:1009
#6  0x00007f90803a62eb in WebCore::RenderLayerCompositor::updateCompositingLayers (this=0x7f906f7bfe70, updateType=WebCore::CompositingUpdateAfterLayoutOrStyleChange, updateRoot=0x7f906f7f29b8)
    at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp:318

Here you can see the owning layer matches:
(gdb) frame 1
#1  0x00007f90803a1d62 in WebCore::RenderLayerBacking::updateContentsOpaque (this=0x7f906aff0850, opaqueRegion=...) at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayerBacking.cpp:1009
1009	    m_graphicsLayer->setContentsOpaque(layerRegion.isEmpty());
(gdb) print m_owningLayer
$7 = (WebCore::RenderLayer *) 0x7f906f7f2178

It does the updateCompositedBounds here because the layer has a backing. I am not sure what the other layers are in the rebuildCompositingLayerTree callstack, or if that is important information.

[James Robinson]

007f908039665e in WebCore::RenderLayer::styleChanged (this=0x7f906f7f2178, oldStyle=0x0) at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayer.cpp:4296
> 
> Here's the backtrace setting opaque in the backing owned by that same layer:
> #0  WebCore::GraphicsLayerChromium::setContentsOpaque (this=0x7f906f8f8e00, opaque=false) at ../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:262
> #1  0x00007f90803a1d62 in WebCore::RenderLayerBacking::updateContentsOpaque (this=0x7f906aff0850, opaqueRegion=...) at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayerBacking.cpp:1009
> #2  0x00007f908039e6e0 in WebCore::RenderLayerBacking::updateCompositedBounds (this=0x7f906aff0850) at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayerBacking.cpp:236
> #3  0x00007f90803a8d7a in WebCore::RenderLayerCompositor::rebuildCompositingLayerTree (this=0x7f906f7bfe70, layer=0x7f906f7f2178, compositingState=..., childLayersOfEnclosingLayer=...)
>     at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp:948

This part of the callstack (in particular RenderLayerBacking::updateContentsOpaque) does not exist in my checkout. Do you have some other patches applied locally? I think this code needs to be aware of the frameview.

Are you sure this is pushing to the NCCH::m_graphicsLayer? Remember that the GraphicsLayer and RenderLayerBacking trees are *not* isomorphic.

If that's the case, this leads me to believe that we probably shouldn't do anything at all in NonCompositedContentHost. We should just rely on the normal contents opaque setting logic to detect opaque FrameViews correctly and set the contentsOpaque bit.

[Dana Jansens]

If we move contentsOpaque (or whatever other incarnation) to CCLayerDelegate, as brought up in bug #72964, I think we can dodge this whole bullet, at least for this CL. We can just set the opaque flag directly on the LayerChromium then and there is no conflict to worry about.

[Dana Jansens]

Created attachment 118051 [details]
Patch

Just sets the opaque flag, and uses it to avoid blending. The backing can avoid conflicting with this by not setting contentsOpaque when drawsContent is false.

The NCCH flag is still used to decide to use the opaque shader program or not. I think we should use the methods in bug #72965 to make that decision instead. I don't want to start duplicating the logic from LRC for checking "draw this opaque" over here.

[James Robinson]

Comment on attachment 118051 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 118051 [details]
Patch

Clearing flags on attachment: 118051

Committed r102164: <http://trac.webkit.org/changeset/102164>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.