Bug 50192

Summary: Compositing overlap testing can throw layers into compositing when they should not be.
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Layout and RenderingAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, dino, enne, jamesr, klobag, mitz, simon.fraser, vangelis, wangxianzhu, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Testcase (throw into LayoutTests/compositing/layer-creation)
none
Corrected testcase
none
Handy interactive testcase (BYO video)
none
WIP patch; does not compile
none
Patch
none
Pre/post patch layer comparison
none
Patch for landing webkit.review.bot: commit-queue-

Description Simon Fraser (smfr) 2010-11-29 17:07:52 PST
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.
Comment 1 Simon Fraser (smfr) 2010-11-29 17:32:52 PST
Created attachment 75091 [details]
Testcase (throw into LayoutTests/compositing/layer-creation)
Comment 2 Simon Fraser (smfr) 2010-11-30 16:43:37 PST
Need to make sure that LayoutTests/compositing/geometry/video-opacity-overlay.html doesn't regress when fixing this.
Comment 3 Simon Fraser (smfr) 2011-05-19 17:18:57 PDT
<rdar://problem/9473408>
Comment 4 Simon Fraser (smfr) 2011-06-10 17:15:51 PDT
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
Comment 5 Simon Fraser (smfr) 2012-01-17 11:25:23 PST
Created attachment 122786 [details]
Corrected testcase

Corrected testcase, with z-index on .container to make it a stacking context.
Comment 6 Simon Fraser (smfr) 2012-01-17 11:45:40 PST
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.
Comment 7 Adrienne Walker 2012-01-17 14:36:05 PST
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.
Comment 8 Simon Fraser (smfr) 2012-01-18 16:26:53 PST
Created attachment 123032 [details]
Handy interactive testcase (BYO video)
Comment 9 Simon Fraser (smfr) 2012-01-24 17:45:47 PST
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.
Comment 10 Adrienne Walker 2012-01-24 21:09:20 PST
(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.
Comment 11 Simon Fraser (smfr) 2012-01-25 07:26:05 PST
(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.
Comment 12 Adrienne Walker 2012-01-25 10:29:47 PST
(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).
Comment 13 Simon Fraser (smfr) 2012-01-25 10:46:11 PST
The benefit of regions is that you can subtract out rects as well as add them.
Comment 14 Simon Fraser (smfr) 2012-02-23 16:27:56 PST
*** Bug 78942 has been marked as a duplicate of this bug. ***
Comment 15 Adrienne Walker 2012-02-23 16:41:12 PST
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?
Comment 16 Simon Fraser (smfr) 2012-02-27 10:34:55 PST
(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.
Comment 17 Adrienne Walker 2012-02-27 17:58:24 PST
(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.)
Comment 18 Simon Fraser (smfr) 2012-02-27 18:02:51 PST
Right, and you(?) changed the overlap map to only contain absolute layer bounds, not composited bounds, which helps here.
Comment 19 Adrienne Walker 2012-02-27 18:35:06 PST
(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?
Comment 20 Simon Fraser (smfr) 2012-02-27 20:27:37 PST
Sorry, yes, I am.
Comment 21 Adrienne Walker 2012-03-02 11:23:45 PST
Created attachment 129927 [details]
Patch
Comment 22 Adrienne Walker 2012-03-02 11:31:22 PST
(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.
Comment 23 Simon Fraser (smfr) 2012-03-02 15:06:47 PST
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.
Comment 24 Simon Fraser (smfr) 2012-03-02 15:19:09 PST
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>
Comment 25 WebKit Review Bot 2012-03-02 15:48:26 PST
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
Comment 26 Adrienne Walker 2012-03-05 16:45:24 PST
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.
Comment 27 Adrienne Walker 2012-03-05 16:47:36 PST
(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.
Comment 28 Simon Fraser (smfr) 2012-03-05 17:13:39 PST
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.
Comment 29 James Robinson 2012-03-05 17:22:14 PST
(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.
Comment 30 Simon Fraser (smfr) 2012-03-05 17:30:09 PST
I guess the overlap map isn't accounting for clipping at all.
Comment 31 Adrienne Walker 2012-03-05 17:49:16 PST
(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.
Comment 32 Adrienne Walker 2012-03-05 18:25:16 PST
Created attachment 130258 [details]
Patch for landing
Comment 33 WebKit Review Bot 2012-03-05 19:41:58 PST
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
Comment 34 Adrienne Walker 2012-03-05 20:22:12 PST
(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.
Comment 35 Adrienne Walker 2012-03-05 21:13:21 PST
Committed r109851: <http://trac.webkit.org/changeset/109851>
Comment 36 mitz 2012-05-28 12:29:57 PDT
(In reply to comment #35)
> Committed r109851: <http://trac.webkit.org/changeset/109851>

This caused bug 87674.