Bug 49233 – [chromium] Composited layers don't handle opacity properly

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Vangelis Kokkevis]

The correct way to implement layer opacity is to render the layer and all its descendants into a blank surface and then apply the opacity value as that surface is composited on top of the existing page contents. Currently chromium (incorrectly) computes an opacity value per layer by multiplying the layer's opacity by the opacity of its parent. This results in compositing/geometry/abs-position-inside-opacity.html failing.

[Vangelis Kokkevis]

Created attachment 73337 [details]
proposed patch

[Vangelis Kokkevis]

Created attachment 73434 [details]
Patch - fixing mac and linux compile issues

[Kenneth Russell]

Comment on attachment 73434 [details]
Patch - fixing mac and linux compile issues

View in context: https://bugs.webkit.org/attachment.cgi?id=73434&action=review

Great work overall in this substantial rewrite of the layer renderer to be more compliant. It can't have been easy to redo all of the math as well as substantially changing how the rendering works in certain situations. I'm marking this r- only because of some minor stylistic issues. I do have a higher-level question about VRAM consumption with the new algorithm, but after getting through the entire patch I think I've answered my own questions, so please tell me if so.

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:165
> +    // For now we apply the layer layer treatment only for layers that are either untransformed

"layer layer"?

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:168
> +    if (!layerOriginTransform.isInvertible())
> +        ASSERT_NOT_REACHED();

Why not ASSERT(layerOriginTransform.isInvertible())?

> WebCore/platform/graphics/chromium/LayerChromium.cpp:485
> +    for (size_t i = 0; i < sublayers.size(); i++)

Prefer preincrement (++i).

> WebCore/platform/graphics/chromium/LayerChromium.cpp:498
> +    for (size_t i = 0; i < sublayers.size(); i++)

Prefer preincrement.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:66
> +    ortho.setM43(0);

Strictly speaking the setM43 is unnecessary since the TransformationMatrix is initialized to the identity.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:72
> +static bool isScaleOrTranslation(const TransformationMatrix& m)

If this is a generally useful query, please add it to the TransformationMatrix class.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:295
> +        m_defaultRenderSurface = m_rootLayer->createRenderSurface();

Does this mean that we are now have two textures associated with the root layer, m_rootLayerTextureId and the texture inside this RenderSurface? If so, I think this is too much of an increase in VRAM requirements. There must be some way to merge the two. (Note, I think I see how this works now; see comment in useRenderSurface.)

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:347
> +    for (int surfaceIndex = renderSurfaceLayerList.size() - 1; surfaceIndex >= 0 ; surfaceIndex--) {

Prefer predecrement.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:365
> +        ASSERT(layerList.size() > 0);

size_t is unsigned, so this should be ASSERT(layerList.size).

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:366
> +        for (unsigned layerIndex = 0; layerIndex < layerList.size(); layerIndex++)

Prefer preincrement.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:443
> +// Recursively walks the layer tree starting at the given node and updates their
> +// drawing transform matrix.

To be a little clearer, "their" -> "each layer's".

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:586
> +    for (size_t i = 0; i < sublayers.size(); i++) {

Preincrement.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:593
> +            FloatRect sublayerRect(-0.5 * sublayerRenderSurface->m_contentRect.width(), -0.5 * sublayerRenderSurface->m_contentRect.height(),
> +                                   sublayerRenderSurface->m_contentRect.width(), sublayerRenderSurface->m_contentRect.height());

These would be easier to read if you had a local variable for sublayerRenderSurface->m_contentRect.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:608
> +        FloatPoint surfaceCenter = FloatRect(renderSurface->m_contentRect).center();

Why not add "float RenderSurfaceChromium::contentRectCenter()"?

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:634
> +        // If the layer starts a new render surface then we need to adjust its
> +        // scissor rect to be expressed in the new surface's coordinate system.
> +        layer->m_scissorRect = layer->m_drawableContentRect;

The comment indicates that some condition should be tested in the following line, but none is. If we're already in logic where the layer starts a new render surface, could you write "Since the layer..."?

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:669
> +        return true;

Is this where we avoid creating two textures for the root layer?

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:708
> +    // Set the current scissor rect.

Useless comment.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:709
> +    scissorToRect(layer->m_scissorRect);

Consider renaming this setScissorToRect. Right now it sounds like a routine which converts a scissor to a rect.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:717
> +    // FIXME: Need to take into account the transform of the containtaining

containtaining -> containing

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:718
> +    // RenderSurface here.

What does this FIXME imply? What is the incorrect rendering result as a consequence?

> WebCore/platform/graphics/chromium/RenderSurfaceChromium.cpp:16
> + * this software without specific prior written permission.

This is the wrong license header. See e.g. WebKit/chromium/tests/PODIntervalTreeTest.cpp.

> WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:16
> + * this software without specific prior written permission.

Wrong license header.

> WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:60
> +    IntRect m_contentRect;
> +    unsigned m_contentsTextureId;
> +    float m_drawOpacity;
> +    IntSize m_allocatedTextureSize;
> +    TransformationMatrix m_drawTransform;
> +    IntRect m_scissorRect;
> +    Vector<LayerChromium*> m_layerList;

These public data members are really not WebKit style for a class. You could, I think, just as easily make this a struct, with the only downside the exposure of the layerRenderer() and m_owningLayer members. I do however see that exposing these publicly makes the LayerRendererChromium more terse.

[Vangelis Kokkevis]

(In reply to comment #3)
> (From update of attachment 73434 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73434&action=review
> 
> Great work overall in this substantial rewrite of the layer renderer to be more compliant. It can't have been easy to redo all of the math as well as substantially changing how the rendering works in certain situations. I'm marking this r- only because of some minor stylistic issues. I do have a higher-level question about VRAM consumption with the new algorithm, but after getting through the entire patch I think I've answered my own questions, so please tell me if so.

Thanks for the review!  I realize this must have been somewhat tedious.

> 
> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:165
> > +    // For now we apply the layer layer treatment only for layers that are either untransformed
> 
> "layer layer"?
> 
Done.

> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:168
> > +    if (!layerOriginTransform.isInvertible())
> > +        ASSERT_NOT_REACHED();
> 
> Why not ASSERT(layerOriginTransform.isInvertible())?
> 
Done.

> > WebCore/platform/graphics/chromium/LayerChromium.cpp:485
> > +    for (size_t i = 0; i < sublayers.size(); i++)
> 
> Prefer preincrement (++i).
> 
Done.

> > WebCore/platform/graphics/chromium/LayerChromium.cpp:498
> > +    for (size_t i = 0; i < sublayers.size(); i++)
> 
> Prefer preincrement.
> 
Done.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:66
> > +    ortho.setM43(0);
> 
> Strictly speaking the setM43 is unnecessary since the TransformationMatrix is initialized to the identity.
> 
Done.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:72
> > +static bool isScaleOrTranslation(const TransformationMatrix& m)
> 
> If this is a generally useful query, please add it to the TransformationMatrix class.

Not sure how generally useful it is, really. I'm leaving it here for now, if that's ok.


> 
> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:295
> > +        m_defaultRenderSurface = m_rootLayer->createRenderSurface();
> 
> Does this mean that we are now have two textures associated with the root layer, m_rootLayerTextureId and the texture inside this RenderSurface? If so, I think this is too much of an increase in VRAM requirements. There must be some way to merge the two. (Note, I think I see how this works now; see comment in useRenderSurface.)

createRenderSurface itself doesn't allocate a texture. m_defaultRenderSurface is "special" in that it only looks like a render surface but when using it you're essentially rendering onto the backbuffer. So, no, there's no increase in VRAM usage.


> 
> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:347
> > +    for (int surfaceIndex = renderSurfaceLayerList.size() - 1; surfaceIndex >= 0 ; surfaceIndex--) {
> 
> Prefer predecrement.
> 
Done.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:365
> > +        ASSERT(layerList.size() > 0);
> 
> size_t is unsigned, so this should be ASSERT(layerList.size).
> 
Done.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:366
> > +        for (unsigned layerIndex = 0; layerIndex < layerList.size(); layerIndex++)
> 
> Prefer preincrement.
> 
Done.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:443
> > +// Recursively walks the layer tree starting at the given node and updates their
> > +// drawing transform matrix.
> 
> To be a little clearer, "their" -> "each layer's".
> 
Done.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:586
> > +    for (size_t i = 0; i < sublayers.size(); i++) {
> 
> Preincrement.
> 
Done.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:593
> > +            FloatRect sublayerRect(-0.5 * sublayerRenderSurface->m_contentRect.width(), -0.5 * sublayerRenderSurface->m_contentRect.height(),
> > +                                   sublayerRenderSurface->m_contentRect.width(), sublayerRenderSurface->m_contentRect.height());
> 
> These would be easier to read if you had a local variable for sublayerRenderSurface->m_contentRect.
> 
Done.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:608
> > +        FloatPoint surfaceCenter = FloatRect(renderSurface->m_contentRect).center();
> 
> Why not add "float RenderSurfaceChromium::contentRectCenter()"?
> 
Done.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:634
> > +        // If the layer starts a new render surface then we need to adjust its
> > +        // scissor rect to be expressed in the new surface's coordinate system.
> > +        layer->m_scissorRect = layer->m_drawableContentRect;
> 
> The comment indicates that some condition should be tested in the following line, but none is. If we're already in logic where the layer starts a new render surface, could you write "Since the layer..."?
> 
Done.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:669
> > +        return true;
> 
> Is this where we avoid creating two textures for the root layer?
> 
Yes, that's right.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:708
> > +    // Set the current scissor rect.
> 
> Useless comment.
Done.

> 
> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:709
> > +    scissorToRect(layer->m_scissorRect);
> 
> Consider renaming this setScissorToRect. Right now it sounds like a routine which converts a scissor to a rect.
Done.


> 
> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:717
> > +    // FIXME: Need to take into account the transform of the containtaining
> 
> containtaining -> containing
> 
Done.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:718
> > +    // RenderSurface here.
> 
> What does this FIXME imply? What is the incorrect rendering result as a consequence?
Updated the FIXME comment to indicate that back-face culling of single sided layers won't always work properly.

> 
> > WebCore/platform/graphics/chromium/RenderSurfaceChromium.cpp:16
> > + * this software without specific prior written permission.
> 
> This is the wrong license header. See e.g. WebKit/chromium/tests/PODIntervalTreeTest.cpp.
> 
Done.

> > WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:16
> > + * this software without specific prior written permission.
> 
> Wrong license header.
> 
Done.

> > WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:60
> > +    IntRect m_contentRect;
> > +    unsigned m_contentsTextureId;
> > +    float m_drawOpacity;
> > +    IntSize m_allocatedTextureSize;
> > +    TransformationMatrix m_drawTransform;
> > +    IntRect m_scissorRect;
> > +    Vector<LayerChromium*> m_layerList;
> 
> These public data members are really not WebKit style for a class. You could, I think, just as easily make this a struct, with the only downside the exposure of the layerRenderer() and m_owningLayer members. I do however see that exposing these publicly makes the LayerRendererChromium more terse.

RenderSurfaceChromium is really a helper class that's used by the LayerRendererChromium exclusively.  I made the members private and made LayerRendererChromium a friend class (much like LayerChromium works as well).

[Vangelis Kokkevis]

Created attachment 73456 [details]
Patch addressing review comments

Addressed review comments and added a fix in the calculation of the layer scissor rects in LayerRendererChromium::updateLayersRecursive() (lines 513-521 and 619-622)

[Kenneth Russell]

Comment on attachment 73456 [details]
Patch addressing review comments

This looks good to me. Very nice work.

[Vangelis Kokkevis]

Committed r72021: <http://trac.webkit.org/changeset/72021>