Bug 50192 – Compositing overlap testing can throw layers into compositing when they should not be.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

The overlap testing logic in RenderLayerCompositor can throw layers into compositing when they should not be. This is wrong; overlap testing should only ever allow layers to be uncomposited.

[Simon Fraser (smfr)]

Created attachment 75091 [details]
Testcase (throw into LayoutTests/compositing/layer-creation)

[Simon Fraser (smfr)]

Need to make sure that LayoutTests/compositing/geometry/video-opacity-overlay.html doesn't regress when fixing this.

[Simon Fraser (smfr)]

<rdar://problem/9473408>

[Simon Fraser (smfr)]

When fixed, these tests should have new results (they changed due to bug 49857):
compositing/tiling/huge-layer-add-remove-child.html
compositing/tiling/huge-layer-resize.html
compositing/tiling/huge-layer.html

[Simon Fraser (smfr)]

Created attachment 122786 [details]
Corrected testcase

Corrected testcase, with z-index on .container to make it a stacking context.

[Simon Fraser (smfr)]

I think I can switch the overlap map to using a Region, and subtract out the rect for a stacking context's bounds to fix this.

[Adrienne Walker]

The overlap map definitely seems to be doing the wrong thing here.  I think it serves a good purpose in general in promoting non-composited layers into composited layers for correctness, but it shouldn't be promoting already-composited layers into getting a new backing.

I wonder if computeCompositingRequirements is just checking the overlap map in too many cases, and needs to take into consideration whether there's already a compositing ancestor.

[Simon Fraser (smfr)]

Created attachment 123032 [details]
Handy interactive testcase (BYO video)

[Simon Fraser (smfr)]

Created attachment 123860 [details]
WIP patch; does not compile

This is my current thinking about how to address this, but it's not quite working yet.

Basically it changes the OverlapMap to be a Region, so that we can copy the region, and subtract out rectangles to avoid a child layer thinking that it has to overlap its composited parent. This also involves keeping a stack of OverlapMaps as we descend into composting layers with children. The part that isn't working is adding to the OverlapMaps on the stack when we do end up throwing layers into compositing.

[Adrienne Walker]

(In reply to comment #9)
> Created an attachment (id=123860) [details]
> WIP patch; does not compile
> 
> This is my current thinking about how to address this, but it's not quite working yet.
> 
> Basically it changes the OverlapMap to be a Region, so that we can copy the region, and subtract out rectangles to avoid a child layer thinking that it has to overlap its composited parent. This also involves keeping a stack of OverlapMaps as we descend into composting layers with children. The part that isn't working is adding to the OverlapMaps on the stack when we do end up throwing layers into compositing.

Can I make a simplifying suggestion that also solves the issue of grandparent layers becoming composited later? Keep a single OverlapMap as now, and make a layer composited if it overlaps anything in it.  However, change the timing of when layers get added to it.

You could do this by keeping a single vector of bounding rects that you haven't added to the overlap map yet and pass a reference to it along recursively.  When you start processing a layer, append it to the vector.  At the end of processing a layer and all of its children, if that layer is composited and gets its own backing, pop all the bounding rects that were added to the list after you started processing that layer off the vector and add them to the OverlapMap.

This approach will let you have the full set of rects that you need to add at all times without requiring any bad O(n^2) behavior to reprocess an entire layer subtree.

[Simon Fraser (smfr)]

(In reply to comment #10)
> (In reply to comment #9)
> > Created an attachment (id=123860) [details] [details]
> > WIP patch; does not compile
> > 
> > This is my current thinking about how to address this, but it's not quite working yet.
> > 
> > Basically it changes the OverlapMap to be a Region, so that we can copy the region, and subtract out rectangles to avoid a child layer thinking that it has to overlap its composited parent. This also involves keeping a stack of OverlapMaps as we descend into composting layers with children. The part that isn't working is adding to the OverlapMaps on the stack when we do end up throwing layers into compositing.
> 
> Can I make a simplifying suggestion that also solves the issue of grandparent layers becoming composited later? Keep a single OverlapMap as now, and make a layer composited if it overlaps anything in it.  However, change the timing of when layers get added to it.
> 
> You could do this by keeping a single vector of bounding rects that you haven't added to the overlap map yet and pass a reference to it along recursively.  When you start processing a layer, append it to the vector.  At the end of processing a layer and all of its children, if that layer is composited and gets its own backing, pop all the bounding rects that were added to the list after you started processing that layer off the vector and add them to the OverlapMap.
> 
> This approach will let you have the full set of rects that you need to add at all times without requiring any bad O(n^2) behavior to reprocess an entire layer subtree.

I think that's close to where I was going with this patch, but using Regions rather than a vector of rects. FWIW, this patch doesn't add any O(n^2) behavior, and I don't think finishing it up would require that.

[Adrienne Walker]

(In reply to comment #11)

> I think that's close to where I was going with this patch, but using Regions rather than a vector of rects. FWIW, this patch doesn't add any O(n^2) behavior, and I don't think finishing it up would require that.

Ok! I'll be interested to see how you finish it up, then.

The downside of regions is that all the rects are baked together, so to handle the case where a grandparent render layer becomes composited due to a grandchild I was thinking you'd either need to use more memory (start a new region per render layer as you descend) or spend more time (go back and recurse through the tree again, like we unfortunately do now).

[Simon Fraser (smfr)]

The benefit of regions is that you can subtract out rects as well as add them.

[Simon Fraser (smfr)]

*** Bug 78942 has been marked as a duplicate of this bug. ***

[Adrienne Walker]

smfr: I had another question about your approach.

Correct me if I'm wrong, but subtracting doesn't seem like enough.  The only time a layer should be promoted to become a composited layer is if it overlaps something between it and its composited ancestor.

As soon as a parent layer becomes composited, then for the purposes of its subtree, the overlap map shouldn't consider any layers considered prior to that parent.  In other words: rather than subtract, we should start with an empty region.

Am I missing something?

[Simon Fraser (smfr)]

(In reply to comment #15)
> smfr: I had another question about your approach.
> 
> Correct me if I'm wrong, but subtracting doesn't seem like enough.  The only time a layer should be promoted to become a composited layer is if it overlaps something between it and its composited ancestor.

No, it might overlap something outside of the composited bounds of its composited ancestor.

> As soon as a parent layer becomes composited, then for the purposes of its subtree, the overlap map shouldn't consider any layers considered prior to that parent.  In other words: rather than subtract, we should start with an empty region.
> 
> Am I missing something?

Yes, I think so; see above.

[Adrienne Walker]

(In reply to comment #16)
> (In reply to comment #15)
> > smfr: I had another question about your approach.
> > 
> > Correct me if I'm wrong, but subtracting doesn't seem like enough.  The only time a layer should be promoted to become a composited layer is if it overlaps something between it and its composited ancestor.
> 
> No, it might overlap something outside of the composited bounds of its composited ancestor.

(Do you mean absolute bounds? As far as I can tell, composited bounds are calculated by taking the union recursively of all children that are not composited themselves.  So, composited bounds can't be calculated until after determining what gets composited.)

However, why does a layer need to become composited if it overlaps something outside of its composited ancestor's absolute bounds? That's what seems unnecessary to me.  Here's the example I have in mind:

Sideways render layer hierarchy, leftward is the root:

a + - B
  |
  - - C - d

Spatial diagram of absolute bounds:

a
+-------------+
| B       C   |
| +--+    +-+ |
| | d|    | | |
| | +--+  +-+ |
| +-|+ |      |
|   +--+      |
|             |
+-------------+

Textual description: 

a is the parent of B and C.  C draws after B.  C is the parent of d.  B and C require compositing, a and d do not.  d overlaps B, but d's bounds are not contained by the bounds of C.

I believe that d should not get its own backing and should instead go into the backing of C.  Your code on line 760 will yield an overlap map that contains the absolute bounds of B minus the absolute bounds of C when processing layer d.  Since d overlaps B but is not contained by C, this code will cause d to get its own backing even when it is not necessary.

(Whether to split up a sparse backing is another question, but one that I don't think should not be decided by overlap tests.)

[Simon Fraser (smfr)]

Right, and you(?) changed the overlap map to only contain absolute layer bounds, not composited bounds, which helps here.

[Adrienne Walker]

(In reply to comment #18)
> Right, and you(?) changed the overlap map to only contain absolute layer bounds, not composited bounds, which helps here.

Looking in history at least back to 2009-07-07, the overlap map has always used absolute layer bounds.

I have added some code to add missing layers to the overlap map (e.g. https://bugs.webkit.org/show_bug.cgi?id=62465), and maybe that's what you're thinking of?

[Simon Fraser (smfr)]

Sorry, yes, I am.

[Adrienne Walker]

Created attachment 129927 [details]
Patch

[Adrienne Walker]

(In reply to comment #21)
> Created an attachment (id=129927) [details]
> Patch

Not to be rude and steal this bug from you Simon, but I'd been staring at this code enough that I felt motivated enough to sit down and put code to words.  :)

It passes the test you attached above.  It fails some compositing layout tests in Chromium, which I'm sure EWS will inform me about.  However, the ~5 image differences (on Chromium) are all due to the difference between drawing text into a layer and then rotating that layer vs. drawing rotated text onto a non-rotated layer and the ~6 text differences in are all due to layers being merged together that now can be.

[Simon Fraser (smfr)]

Comment on attachment 129927 [details]
Patch

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

This looks good. I would like to see an additional test that tests > 1 level of nesting; I'll attach something to the bug.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:123
> +    void startCompositingAncestor()
> +    {
> +        m_overlapStack.append(Region());
> +    }
> +
> +    void endCompositingAncestor()
> +    {
> +        m_overlapStack[m_overlapStack.size() - 2].unite(m_overlapStack.last());
> +        m_overlapStack.removeLast();
> +    }

I think these methods could have better names. "startCompositingAncestor" implies "start to composite the ancestor". It's really a push()/pop().

Maybe pushCompositingContainer/popCompositingContainer?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:803
> +    if (overlapMap && childState.m_compositingAncestor && !childState.m_compositingAncestor->isRootLayer())
> +        addToOverlapMap(*overlapMap, layer, absBounds, haveComputedBounds);

I'd like to see a comment above these lines saying why you add this layer to the overlap map even if it is not being composited.

> LayoutTests/compositing/layer-creation/stacking-context-overlap.html:10
> +            -webkit-transform:translateZ(0);

Space after : please.

[Simon Fraser (smfr)]

Here's the other test. It needs converting to a layout test:


<!DOCTYPE html>

<html>
<head>
  <!-- <meta name="viewport" content="initial-scale=0.75"> -->
  <style>
    body {
      margin: 0;
    }
    .box {
      position: absolute;
      top: 20px;
      left: 20px;
      height: 100px;
      width: 100px;
/*      overflow: hidden;*/
      background-color: red;
    }

    .composited {
      -webkit-transform: translateZ(0);
      background-color: green;
      outline: 10px solid transparent; /* inflate compositing layer bounds */
    }

    .box > .box {
      top: 50px;
      left: 50px;
      width: 200px;
      background-color: rgba(255, 0, 0, 0.6);
    }
    
    #indicator {
      position: absolute;
      top: 75px;
      left: 75px;
      height: 56px;
      width: 56px;
      background-color: blue;
    }
  </style>
</head>
<body>

<!-- <div class="box">
  <div class="box">
  </div>
</div> -->

<div class="composited box">
  <div class="composited box"></div>
</div>

<div id="indicator"></div>

</body>
</html>

[WebKit Review Bot]

Comment on attachment 129927 [details]
Patch

Attachment 129927 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11797208

New failing tests:
compositing/geometry/limit-layer-bounds-transformed.html
compositing/reflections/reflection-on-composited.html
compositing/geometry/limit-layer-bounds-positioned.html
compositing/geometry/limit-layer-bounds-transformed-overflow.html
compositing/geometry/limit-layer-bounds-fixed-positioned.html
compositing/geometry/limit-layer-bounds-positioned-transition.html
compositing/shadows/shadow-drawing.html
compositing/reflections/nested-reflection-transformed.html
fast/repaint/block-selection-gap-in-composited-layer.html
compositing/geometry/limit-layer-bounds-overflow-root.html

[Adrienne Walker]

Created attachment 130232 [details]
Pre/post patch layer comparison

Before landing this, I discovered this issue during manual testing on maps.  The left of the image is before the patch (many, tight-fitting layers) and the right of the image is after the patch (one, too large layer).  The problem visually is that it seems weird that the layer is so large.

My conclusion is that this is a separate bug with calculateCompositedBounds.  I'm wondering if there are layers that don't actually draw anything that are contributing to the size of a backing.  Previously, these layers would overlap something else, get their own backing, but then not draw.

I think this patch is still enough of a positive change to land on its own though, so I will file this issue separately.

[Adrienne Walker]

(In reply to comment #26)
> Created an attachment (id=130232) [details]
> Pre/post patch layer comparison

I should add that this is on http://goo.gl/eYGqX with Chrome, using accelerated canvas as a compositing trigger.

[Simon Fraser (smfr)]

Yes, this patch may cause some layers to get bigger. You can see this in the interactive testcase.

We should detect when it would be more efficient to composite a descendant layer separately based on layer area.

[James Robinson]

(In reply to comment #26)
> Created an attachment (id=130232) [details]
> Pre/post patch layer comparison
> 
> Before landing this, I discovered this issue during manual testing on maps.  The left of the image is before the patch (many, tight-fitting layers) and the right of the image is after the patch (one, too large layer).  The problem visually is that it seems weird that the layer is so large.
> 
> My conclusion is that this is a separate bug with calculateCompositedBounds.  I'm wondering if there are layers that don't actually draw anything that are contributing to the size of a backing.  Previously, these layers would overlap something else, get their own backing, but then not draw.
> 


The <div> being composited has children that extend down that far, but they're clipped by a (software-rendered) ancestor.  When I load the page this part of the DOM looks like:
<div id="views-control" style="z-index: 0"> // this is composited
  <div>
    <div>
      <div class="mv-scroller" style="overflow-y: hidden; height: 26px"> // this clips
        <div>Traffic</div>
        <div>Transit</div>
        <div>Photos</div>
...


Only the Traffic div is visible because of mv-scroller, but the other <div>s still occupy layout space and grow this layer's bounds.

[Simon Fraser (smfr)]

I guess the overlap map isn't accounting for clipping at all.

[Adrienne Walker]

(In reply to comment #30)
> I guess the overlap map isn't accounting for clipping at all.

I think it should: https://bugs.webkit.org/show_bug.cgi?id=63493

I wonder if calculateCompositedBounds isn't also respecting the clipRect, and is unioning layers or parts of layers that would otherwise be clipped.

[Adrienne Walker]

Created attachment 130258 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 130258 [details]
Patch for landing

Attachment 130258 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11839116

New failing tests:
fast/repaint/block-selection-gap-in-composited-layer.html

[Adrienne Walker]

(In reply to comment #33)
> (From update of attachment 130258 [details])
> Attachment 130258 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11839116
> 
> New failing tests:
> fast/repaint/block-selection-gap-in-composited-layer.html

Oops, missed one.  I'll fix that on landing.

I've filed the clipping issue separately as https://bugs.webkit.org/show_bug.cgi?id=80372 and am inclined to land this patch first and follow up fixing that afterwards.

[Adrienne Walker]

Committed r109851: <http://trac.webkit.org/changeset/109851>

(In reply to comment #35)
> Committed r109851: <http://trac.webkit.org/changeset/109851>

This caused bug 87674.