Bug 62181 – Incorrect compositing overlap test

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Adrienne Walker]

I think there is something incorrect with the RenderLayerCompositor's overlap test.  However, I'm not quite sure that I know enough to confidently fix it, so I will explain what I've found and hopefully hear from somebody who knows this code better than me.

I boiled down Chromium bug 84254 to the following attached test case.  There's (in z-order) a red div, a WebGL canvas, and a green div.  The green div entirely overlaps the red div, from the perspective of the final rendering result.  In software mode, only the green square is visible.  When composited, the green square does not pass the overlap test in RenderLayerCompositor, does not get composited, and ends up incorrectly drawing underneath the red div.

Here's why the overlap test fails.  In this test case, only two layers get added to the overlap map.  There's the 300x150 canvas, which gets added to the overlap map as IntRect(408, 8, 300, 150).  There's also the 1017x0 layer containing the red div and the canvas.  This ends up getting added to the overlap map as IntRect(8, 8, 1, 1).  (Empty rects are grown to 1,1 in RenderLayerCompositor::addToOverlapMap.) The green div at IntRect(10, 0, 300, 300) doesn't overlap either of these, and so does not end up being composited.

Are the layer bounds incorrect on the parent div for the canvas here? Should we maybe using composited bounds in the overlap map instead of layer bounds?

[Adrienne Walker]

Created attachment 96186 [details]
Test case

[Simon Fraser (smfr)]

Bug 50192?

[Adrienne Walker]

(In reply to comment #2)
> Bug 50192?

Unless I am misunderstanding, I think this is a different bug.

I don't quite follow what is expected from your attached test case there, but when I open it up in Chromium, container gets composited because it overlaps the video, and child box gets composited because it overlaps container.  This seems like exactly what I would expect.

This bug's test case has two layers which get rendered as overlapping but from the point of view of RenderLayerCompositor they are not.

[Simon Fraser (smfr)]

(In reply to comment #3)
> (In reply to comment #2)
> > Bug 50192?
> 
> Unless I am misunderstanding, I think this is a different bug.
> 
> I don't quite follow what is expected from your attached test case there, but when I open it up in Chromium, container gets composited because it overlaps the video, and child box gets composited because it overlaps container.  This seems like exactly what I would expect.

Actually, in that testcase the child box doesn't need to be in a separate compositing layer; that's the bug.

> This bug's test case has two layers which get rendered as overlapping but from the point of view of RenderLayerCompositor they are not.

Hm, it does look wrong.

[Adrienne Walker]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Bug 50192?
> > 
> > Unless I am misunderstanding, I think this is a different bug.
> > 
> > I don't quite follow what is expected from your attached test case there, but when I open it up in Chromium, container gets composited because it overlaps the video, and child box gets composited because it overlaps container.  This seems like exactly what I would expect.
> 
> Actually, in that testcase the child box doesn't need to be in a separate compositing layer; that's the bug.

Ah, so it's an issue of what gets its own compositing layer and not about what gets composited at all.

> > This bug's test case has two layers which get rendered as overlapping but from the point of view of RenderLayerCompositor they are not.
> 
> Hm, it does look wrong.

The problem here seems to me that the overlap test wants to test whether a layer's bounds overlaps a layer that's been composited.  Given that multiple layers can go into the same composited layer, intuitively it seems that we should be using the composited bounds for the comparison rather than the layer bounds.

I would assume that every layer that gets added to the overlap test will be composited (even if it is not already), but it looks like we might also have to update the backing size more proactively in order to have this information available.

Unless this approach sounds entirely wrong, I'll look into uploading a patch that implements this suggestion.

[Simon Fraser (smfr)]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > Bug 50192?
> > > 
> > > Unless I am misunderstanding, I think this is a different bug.
> > > 
> > > I don't quite follow what is expected from your attached test case there, but when I open it up in Chromium, container gets composited because it overlaps the video, and child box gets composited because it overlaps container.  This seems like exactly what I would expect.
> > 
> > Actually, in that testcase the child box doesn't need to be in a separate compositing layer; that's the bug.
> 
> Ah, so it's an issue of what gets its own compositing layer and not about what gets composited at all.

Yeah. The problem there is that the overlap map doesn't track which layer contributed which rect to the map, so has no way to know that the layer being overlapped is an ancestor into which the current RL could just draw.

> > > This bug's test case has two layers which get rendered as overlapping but from the point of view of RenderLayerCompositor they are not.
> > 
> > Hm, it does look wrong.
> 
> The problem here seems to me that the overlap test wants to test whether a layer's bounds overlaps a layer that's been composited.  Given that multiple layers can go into the same composited layer, intuitively it seems that we should be using the composited bounds for the comparison rather than the layer bounds.
> 
> I would assume that every layer that gets added to the overlap test will be composited (even if it is not already), but it looks like we might also have to update the backing size more proactively in order to have this information available.
> 
> Unless this approach sounds entirely wrong, I'll look into uploading a patch that implements this suggestion.

It sounds OK, but updating the backing store size more proactively may be tricky.

[Adrienne Walker]

Created attachment 96477 [details]
Patch

[Adrienne Walker]

Comment on attachment 96477 [details]
Patch

Taking off review, because I want to add a test case first.  I'm just putting this up to get any initial comments.  This passes the test case above, fixes the Chromium bug, and passes all the compositing layout tests (for Chromium).

smfr: You were quite right that updating the backing was tricky.  Instead of using larger bounds in the overlap map, I thought it would be more direct to just add more layers to the overlap map.

[Simon Fraser (smfr)]

Comment on attachment 96477 [details]
Patch

Sucks to have to walk the layer tree from inside a layer tree traverse.

[Adrienne Walker]

(In reply to comment #9)
> (From update of attachment 96477 [details])
> Sucks to have to walk the layer tree from inside a layer tree traverse.

That's for sure.  It's better than walking the layer tree on every overlap test to calculate the composited bounds, though.  That's what my initial proposal would have required.  :(

An alternate solution here would be to accumulate a list of layers in CompositingState that would need to be considered composited if some ancestor layer became composited.  Then we could just walk that list, rather than the tree.  I'm not convinced this is all that much better.

[Adrienne Walker]

Created attachment 96500 [details]
Patch

[Adrienne Walker]

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

Added test case.

Also, added early out in recursion if a layer couldn't be composited or if it was already marked as needing to be composited.

[Gyuyoung Kim]

Comment on attachment 96500 [details]
Patch

Attachment 96500 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8812688

[Adrienne Walker]

(In reply to comment #13)
> (From update of attachment 96500 [details])
> Attachment 96500 [details] did not pass efl-ews (efl):
> Output: http://queues.webkit.org/results/8812688

That looks like flake to me.

[Adrienne Walker]

Created attachment 96627 [details]
Patch

[Adrienne Walker]

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

Ah, in looking at this further, this wasn't recursing through the negative z-index children of a layer that got belated composited when it should have been.

[Simon Fraser (smfr)]

Comment on attachment 96627 [details]
Patch

I wonder if it might be better to have computeCompositingRequirements() itself track whether descendent RLs need to be added to the overlap map, to avoid the potentially n^2 recursion

[Adrienne Walker]

(In reply to comment #17)
> (From update of attachment 96627 [details])
> I wonder if it might be better to have computeCompositingRequirements() itself track whether descendent RLs need to be added to the overlap map, to avoid the potentially n^2 recursion

By n^2, you're talking about the case where (potentially) every layer requiresCompositingWhenDescendantsAreCompositing() and at each step as we walk back up the tree, we iterate through the whole tree again because some leaf caused the whole stack to become composited?

I looked into trying to keep a Vector of layers, but copying them back and forth between CompositingState objects as we descended the tree got complex pretty quickly.

I think the simplest thing to do is to just early out in addToOverlapMapRecursive when a layer has already been added to the overlap map.  That way, if some child has already added itself and all of its children to the overlap map, if the parent becomes composited it won't re-traverse that same tree.  At worst, we will traverse the layer tree twice.

[Adrienne Walker]

Created attachment 96644 [details]
Patch

[Simon Fraser (smfr)]

I plan to review this bug need to think about it some more.

[Simon Fraser (smfr)]

Apparently this affects Google maps is canvases force compositing layers. Steps are:
1. maps.google.com
2. Enter directions
3. Scroll around.

[James Robinson]

This is the related chromium bug: http://code.google.com/p/chromium/issues/detail?id=84254

[Simon Fraser (smfr)]

Comment on attachment 96644 [details]
Patch

I'm hesitating here because I wonder if we can fix this, bug 62465 and bug 50192 by doing something better.

[Adrienne Walker]

(In reply to comment #23)
> (From update of attachment 96644 [details])
> I'm hesitating here because I wonder if we can fix this, bug 62465 and bug 50192 by doing something better.

Simon, do you have any more thoughts on this patch? I have addressed the O(n^2) issue that you brought up ealier and in the worst case we now only walk the tree twice.

If you think there might a better solution but need more time, I would prefer to commit this so that we can be correct now and then address any performance or efficiency concerns in a future patch.

[Simon Fraser (smfr)]

Comment on attachment 96644 [details]
Patch

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

> LayoutTests/compositing/layer-creation/overlap-transformed-layer.html:36
> +        // Trigger accelerated compositing *not* using a transform
> +        document.getElementsByTagName('canvas')[0].getContext('2d');

Can't we use a translateZ(0) now? This would allow the test to work on platforms without accelerated canvases.

I also think that you should dump the layer tree in the text so that you don't have to rely on pixel testing.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:581
> +    layer->setMustOverlapCompositedLayers(true);

I don't think you want to do this on every descendent layer, since it will put them each into their own layer, even when they don't need to be (see bug 50192).

Shouldn't this stop at a layer which is already compositing?

If I understand correctly, this method is really addLayerAndNonCompositedDescendentsToOverlapMap().

[Adrienne Walker]

(In reply to comment #25)
> (From update of attachment 96644 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96644&action=review
> 
> > LayoutTests/compositing/layer-creation/overlap-transformed-layer.html:36
> > +        // Trigger accelerated compositing *not* using a transform
> > +        document.getElementsByTagName('canvas')[0].getContext('2d');
> 
> Can't we use a translateZ(0) now? This would allow the test to work on platforms without accelerated canvases.

Oh, whoops.  I thought I had uploaded the patch I had locally with that change, but I was wrong.  Let me make the additional change you suggested below and I'll upload a new patch.

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:581
> > +    layer->setMustOverlapCompositedLayers(true);
> 
> I don't think you want to do this on every descendent layer, since it will put them each into their own layer, even when they don't need to be (see bug 50192).

Ah, ok.  I think I misunderstood what that flag did.
 
> Shouldn't this stop at a layer which is already compositing?

It does implicitly, because it terminates the recursion at any layer that has been added to the overlap map.

> If I understand correctly, this method is really addLayerAndNonCompositedDescendentsToOverlapMap().

That's exactly it.

[Adrienne Walker]

Created attachment 98065 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 98065 [details]
Patch

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

r=me but please consider renaming the method and tweaking the test.

> LayoutTests/compositing/layer-creation/overlap-transformed-layer.html:46
> +        layoutTestController.dumpAsText(false);

I think pixel results would be useful here too (they can be cross-platform since they don't contain text).

> LayoutTests/compositing/layer-creation/overlap-transformed-layer.html:50
> +        layoutTestController.waitUntilDone();
> +        window.addEventListener('load', function() {
> +            document.getElementById("layertree").innerText = layoutTestController.layerTreeAsText();
> +            layoutTestController.notifyDone();

You don't need waitUntilDone/notifyDone when the test does work during the load event.

> LayoutTests/compositing/layer-creation/overlap-transformed-layer.html:58
> +        <!-- This red square should not be visible -->
> +        <div id="red"></div>

I usually use #indicator for such elements.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:572
> +void RenderLayerCompositor::addToOverlapMapRecursive(OverlapMap& overlapMap, RenderLayer* layer)

I prefer the method name i suggested previously.

[Adrienne Walker]

(In reply to comment #28)
> (From update of attachment 98065 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98065&action=review
> 
> r=me but please consider renaming the method and tweaking the test.
> 
> > LayoutTests/compositing/layer-creation/overlap-transformed-layer.html:46
> > +        layoutTestController.dumpAsText(false);
> 
> I think pixel results would be useful here too (they can be cross-platform since they don't contain text).

Will do.

> > LayoutTests/compositing/layer-creation/overlap-transformed-layer.html:50
> > +        layoutTestController.waitUntilDone();
> > +        window.addEventListener('load', function() {
> > +            document.getElementById("layertree").innerText = layoutTestController.layerTreeAsText();
> > +            layoutTestController.notifyDone();
> 
> You don't need waitUntilDone/notifyDone when the test does work during the load event.

Oh! Good to know.  Will do.

> > LayoutTests/compositing/layer-creation/overlap-transformed-layer.html:58
> > +        <!-- This red square should not be visible -->
> > +        <div id="red"></div>
> 
> I usually use #indicator for such elements.

Will do.
 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:572
> > +void RenderLayerCompositor::addToOverlapMapRecursive(OverlapMap& overlapMap, RenderLayer* layer)
> 
> I prefer the method name i suggested previously.

I thought about it, and I think that name is slightly incorrect.  The function recursively adds that layer and all of its descendants, compositing or not, to the overlap map.  It does skip re-adding layers that have already been added to the overlap map, but that's not a check for compositing.

It's a property of the caller and not of the function itself that the only layers that are not already in the overlap map are non-compositing.  It's more addLayerAndDescendantsToOverlapMapIfLayerCanBeCompositedAndUnlessLayerAlreadyInOverlapMap, but that's a little wordy.

[Simon Fraser (smfr)]

OK.

[Adrienne Walker]

Committed r89394: <http://trac.webkit.org/changeset/89394>