Bug 62465 - Overlap test needs to consider children of composited layers
Summary: Overlap test needs to consider children of composited layers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-10 10:48 PDT by Adrienne Walker
Modified: 2011-06-13 14:33 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.49 KB, patch)
2011-06-10 10:51 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (5.85 KB, patch)
2011-06-13 11:39 PDT, Adrienne Walker
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2011-06-10 10:48:06 PDT
Overlap test needs to consider children of composited elements
Comment 1 Adrienne Walker 2011-06-10 10:51:19 PDT
Created attachment 96759 [details]
Patch
Comment 2 Simon Fraser (smfr) 2011-06-10 10:53:01 PDT
Can you explain more how this is different from bug 62181?
Comment 3 Adrienne Walker 2011-06-10 11:07:38 PDT
(In reply to comment #2)
> Can you explain more how this is different from bug 62181?

Sure! Consider this layer tree:

A (empty layer bounds)
-B (layer that should be underneath D)
-C
D (layer that should be on top of B)

If it's not clear, B and C are children of A.  All layers can be composited.

In bug 62181, A has a webkit-transform (requiring compositing if any descendants are composited) and C is an accelerated canvas.

In this bug, A is composited itself because it has an animation.  C is irrelevant.

In both cases, layer B does not get added to the overlap map, but there are different reasons why this happens.  In both cases, B does not need to be composited.

computeCompositingRequirements visits the parent layer before the children.  The difference between the two cases is whether we know that we are compositing when we process layer B.  In bug 62181, A is not known to be composited until we have processed all of its children.  In this bug, A is already known to be composited when we process B.

In bug 62181, we belatedly find out that A is composited after we have already processed B, which is why we have to go back and rewalk the tree to add them to the overlap map.  In this bug, we know that A is composited already, so we can just add B to the overlap map during the initial traversal.

(This bug could also be fixed by having needsCompositing walk up the parent tree to see if any parent layer needed to be composited.)
Comment 4 Adrienne Walker 2011-06-10 11:12:30 PDT
On a side note, I also think that both of these patches are the correct way to handle what the "extend empty layer bounds to 1x1" hack is trying to do in the RenderLayerCompositor::addToOverlapMap function.  The more I stare at that line of code, the more I think it is unneeded and incorrect.
Comment 5 Simon Fraser (smfr) 2011-06-10 13:48:21 PDT
What real-world site is affected by this?
Comment 6 James Robinson 2011-06-10 13:57:25 PDT
In Chromium, this causes portions of the maps.google.com interface to vanish when zooming in or out of the map, which triggers an animation of the -webkit-transform: scale() property.

This doesn't seem to affect Safari, possibly because of differences in how CoreAnimation handles this transition vs handling the intermediate frames from within WebKit.
Comment 7 Simon Fraser (smfr) 2011-06-10 14:31:00 PDT
Comment on attachment 96759 [details]
Patch

I guess this isn't an issue in Safari because RenderLayerCompositor::didStartAcceleratedAnimation() does a setCompositingConsultsOverlap(false)

Can you make a testcase that will work for safari too? Try using <video> to cause compositing.
Comment 8 Adrienne Walker 2011-06-10 14:35:59 PDT
(In reply to comment #7)
> (From update of attachment 96759 [details])
> I guess this isn't an issue in Safari because RenderLayerCompositor::didStartAcceleratedAnimation() does a setCompositingConsultsOverlap(false)
> 
> Can you make a testcase that will work for safari too? Try using <video> to cause compositing.

I tried to make a test case triggering compositing not using animation, but couldn't do it.  This has to be triggered by a composited parent whose bounds don't contain a non-composited child.

I could be wrong, but when I added child elements to <video> or <canvas> they get ignored.  Transformed layers ignore overlap, so that's not an option.  All the other compositing triggers (frames, plugins, clipping) all clip to their bounds, so that doesn't work either.  Animation seems like the only case.
Comment 9 Simon Fraser (smfr) 2011-06-10 14:38:07 PDT
If I fix bug 49857 that will change.
Comment 10 Simon Fraser (smfr) 2011-06-10 14:54:27 PDT
Comment on attachment 96759 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Any child layer with a compositing ancestor will be put into a
> +        composited layer even though they themselves don't need compositing.

Are you describing the symptoms of bug 50192 here?
Comment 11 Adrienne Walker 2011-06-10 15:05:43 PDT
(In reply to comment #10)
> (From update of attachment 96759 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96759&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Any child layer with a compositing ancestor will be put into a
> > +        composited layer even though they themselves don't need compositing.
> 
> Are you describing the symptoms of bug 50192 here?

No.  I should have been more precise in my language: s/put/drawn/.  (Is there better language to describe this?)

This bug and bug 62181 are about layers not being made composited when they should be, because the overlap map does not contain enough information about non-composited layers with a composited parent.

Bug 50192 is about layers that don't need their own composited layer, but get one anyway.
Comment 12 Simon Fraser (smfr) 2011-06-10 17:27:59 PDT
I have committed the fix for 49857, so you can have a testcase that works in Safari as well now.
Comment 13 Adrienne Walker 2011-06-10 17:36:01 PDT
(In reply to comment #12)
> I have committed the fix for 49857, so you can have a testcase that works in Safari as well now.

Thanks! Is an updated test case the only thing blocking an R+ from you at this point?
Comment 14 Simon Fraser (smfr) 2011-06-10 18:07:41 PDT
On this one, yes. I'll think more about the other one.
Comment 15 Adrienne Walker 2011-06-13 11:39:40 PDT
Created attachment 96981 [details]
Patch
Comment 16 Adrienne Walker 2011-06-13 14:33:25 PDT
Committed r88698: <http://trac.webkit.org/changeset/88698>