Don't create render passes for transparent images.

Don't create render passes for transparent images.

That patch speeds up Google Maps use-case smoke test by a factor of 10x.
It also reduces memory pressure on the GPU. See the related bugs for more info.


BUG=161434, 160871
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170487

[danakj, 2012-11-26 18:35:26]

https://codereview.chromium.org/11316171/diff/1/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/11316171/diff/1/cc/layer_tree_host_common.cc#...
cc/layer_tree_host_common.cc:490: if (layer->parent()) {
This should be replaced with a check for something else.. like:

if (layer->parent() && !layer->parent()->renderSurface())

When the parent owns a surface, the opacity will be applied at draw time of the
surface, and we don't need to inherit it.

[danakj, 2012-11-26 18:36:32]

+shawn

[shawnsingh, 2012-11-26 18:52:57]

Nice one slavi@ =)

https://codereview.chromium.org/11316171/diff/1/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/11316171/diff/1/cc/layer_tree_host_common.cc#...
cc/layer_tree_host_common.cc:490: if (layer->parent()) {
On 2012/11/26 18:35:26, danakj wrote:
> This should be replaced with a check for something else.. like:
> 
> if (layer->parent() && !layer->parent()->renderSurface())
> 
> When the parent owns a surface, the opacity will be applied at draw time of
the
> surface, and we don't need to inherit it.
> 

danakj@ is this true?  we're using the parent layer's drawOpacity, which already
accounts for whether or not its own opacity was re-parented by its
renderSurface.   I think the additional if-statement portion you're proposing
would be a no-op, wouldn't it?   

If what I said is correct, I'm still OK with whichever way seems to make the
semantics more obvious.  But a comment is definitely needed here, and maybe that
makes it clear enough the situation without the additional if-statement.

[shawnsingh, 2012-11-26 18:55:54]

Also, given this is a bugfix, I think we ought to add a unit-test for this.  
For now the unit test should be something that reproduces the bug behavior.

I'm torn about whether we should eventually add a tighter unit test that
directly tests subtreeShouldRenderToSeparateSurface more completely.


On 2012/11/26 18:52:57, shawnsingh wrote:
> Nice one slavi@ =)
> 
> https://codereview.chromium.org/11316171/diff/1/cc/layer_tree_host_common.cc
> File cc/layer_tree_host_common.cc (right):
> 
>
https://codereview.chromium.org/11316171/diff/1/cc/layer_tree_host_common.cc#...
> cc/layer_tree_host_common.cc:490: if (layer->parent()) {
> On 2012/11/26 18:35:26, danakj wrote:
> > This should be replaced with a check for something else.. like:
> > 
> > if (layer->parent() && !layer->parent()->renderSurface())
> > 
> > When the parent owns a surface, the opacity will be applied at draw time of
> the
> > surface, and we don't need to inherit it.
> > 
> 
> danakj@ is this true?  we're using the parent layer's drawOpacity, which
already
> accounts for whether or not its own opacity was re-parented by its
> renderSurface.   I think the additional if-statement portion you're proposing
> would be a no-op, wouldn't it?   
> 
> If what I said is correct, I'm still OK with whichever way seems to make the
> semantics more obvious.  But a comment is definitely needed here, and maybe
that
> makes it clear enough the situation without the additional if-statement.

[slavi, 2012-11-26 18:56:06]

On 2012/11/26 18:35:26, danakj wrote:
> https://codereview.chromium.org/11316171/diff/1/cc/layer_tree_host_common.cc
> File cc/layer_tree_host_common.cc (right):
> 
>
https://codereview.chromium.org/11316171/diff/1/cc/layer_tree_host_common.cc#...
> cc/layer_tree_host_common.cc:490: if (layer->parent()) {
> This should be replaced with a check for something else.. like:
> 
> if (layer->parent() && !layer->parent()->renderSurface())
> 
> When the parent owns a surface, the opacity will be applied at draw time of
the
> surface, and we don't need to inherit it.

Done. I thought that might not be quite right but all Layout Tests passed so I
left it as it is. I added  the check for a parent render surface before I saw
Shawn's comment and all Layout Tests still pass.

Let me know what you think this check is necessary.

[danakj, 2012-11-26 18:57:34]

A test would be good, yes :)

There are cases where it is known that things /should/ create a surface. And I
think we have tests for those generally. A test that creates a situation where
we don't need a surface and makes sure we don't get one, seems valuable.

https://codereview.chromium.org/11316171/diff/1/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/11316171/diff/1/cc/layer_tree_host_common.cc#...
cc/layer_tree_host_common.cc:490: if (layer->parent()) {
On 2012/11/26 18:52:57, shawnsingh wrote:
> On 2012/11/26 18:35:26, danakj wrote:
> > This should be replaced with a check for something else.. like:
> > 
> > if (layer->parent() && !layer->parent()->renderSurface())
> > 
> > When the parent owns a surface, the opacity will be applied at draw time of
> the
> > surface, and we don't need to inherit it.
> > 
> 
> danakj@ is this true?  we're using the parent layer's drawOpacity, which
already
> accounts for whether or not its own opacity was re-parented by its
> renderSurface.   I think the additional if-statement portion you're proposing
> would be a no-op, wouldn't it?   
> 
> If what I said is correct, I'm still OK with whichever way seems to make the
> semantics more obvious.  But a comment is definitely needed here, and maybe
that
> makes it clear enough the situation without the additional if-statement.

Oh that is true.

I thought we had the preserves3D here because then you know the parent didn't
own a surface. This check is from so long ago and has just been bumped around
over time.

Since we use the drawOpacity, this change seems fine as is to me also.

[slavi, 2012-11-26 18:57:43]

On 2012/11/26 18:55:54, shawnsingh wrote:
> Also, given this is a bugfix, I think we ought to add a unit-test for this.  
> For now the unit test should be something that reproduces the bug behavior.
> 
> I'm torn about whether we should eventually add a tighter unit test that
> directly tests subtreeShouldRenderToSeparateSurface more completely.

I'll look into it. Thanks for the suggestion =-)

[shawnsingh, 2012-11-26 19:03:08]

One more thing for now...

https://codereview.chromium.org/11316171/diff/6002/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/11316171/diff/6002/cc/layer_tree_host_common....
cc/layer_tree_host_common.cc:497: if (layer->parent() &&
!layer->parent()->renderSurface()) {
I think our discussion also applies to this if-statement, too, right?   i.e. the
XXXIsAnimating() accessors are computed properties that are reparented by a
surface, not properties given by the user of the compositor.

[danakj, 2012-11-26 19:09:44]

https://codereview.chromium.org/11316171/diff/6002/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/11316171/diff/6002/cc/layer_tree_host_common....
cc/layer_tree_host_common.cc:497: if (layer->parent() &&
!layer->parent()->renderSurface()) {
On 2012/11/26 19:03:08, shawnsingh wrote:
> I think our discussion also applies to this if-statement, too, right?   i.e.
the
> XXXIsAnimating() accessors are computed properties that are reparented by a
> surface, not properties given by the user of the compositor.

Yes, the check was added to the wrong if statement here. It would break the
screenSpaceTransformIsAnimating case here.

[slavi, 2012-11-26 23:19:02]

PTAL

I've added a unit test that verifies that we don't create a render passes for
transparent images.

I've also fixed the failing unit tests that relied on the old behavior.

[jamesr, 2012-11-27 01:34:45]

https://codereview.chromium.org/11316171/diff/6008/cc/delegated_renderer_laye...
File cc/delegated_renderer_layer_impl.cc (left):

https://codereview.chromium.org/11316171/diff/6008/cc/delegated_renderer_laye...
cc/delegated_renderer_layer_impl.cc:32: return
!m_renderPassesInDrawOrder.isEmpty();
is this logic preserved somewhere or just gone? are there tests?

https://codereview.chromium.org/11316171/diff/6008/cc/layer_tree_host_common_...
File cc/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/11316171/diff/6008/cc/layer_tree_host_common_...
cc/layer_tree_host_common_unittest.cc:4639: // Make sure that we don't create
render passes for transparent images.
this test doesn't appear to have anything to do with image layers in particular,
it's more about opacity with exactly 1 descendant that draws - could you refine
the comment + test name?

[slavi, 2012-11-27 01:45:49]

On 2012/11/27 01:34:45, jamesr wrote:
>
https://codereview.chromium.org/11316171/diff/6008/cc/delegated_renderer_laye...
> File cc/delegated_renderer_layer_impl.cc (left):
> 
>
https://codereview.chromium.org/11316171/diff/6008/cc/delegated_renderer_laye...
> cc/delegated_renderer_layer_impl.cc:32: return
> !m_renderPassesInDrawOrder.isEmpty();
> is this logic preserved somewhere or just gone? are there tests?

No, it's unused. It can always be resurrected if needed. Although I believe that
DelegatedRendererLayerImpl would never by passed to
subtreeShouldRenderToSeparateSurface because render passes must have been
already decided to be delegated.

>
https://codereview.chromium.org/11316171/diff/6008/cc/layer_tree_host_common_...
> File cc/layer_tree_host_common_unittest.cc (right):
> 
>
https://codereview.chromium.org/11316171/diff/6008/cc/layer_tree_host_common_...
> cc/layer_tree_host_common_unittest.cc:4639: // Make sure that we don't create
> render passes for transparent images.
> this test doesn't appear to have anything to do with image layers in
particular,
> it's more about opacity with exactly 1 descendant that draws - could you
refine
> the comment + test name?

Done.

[jamesr, 2012-11-27 01:46:35]

Makes sense. lgtm

[commit-bot: I haz the power, 2012-11-27 01:47:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skaslev@chromium.org/11316171/12

[danakj, 2012-11-27 01:52:16]

https://chromiumcodereview.appspot.com/11316171/diff/12/cc/layer.h
File cc/layer.h (left):

https://chromiumcodereview.appspot.com/11316171/diff/12/cc/layer.h#oldcode270
cc/layer.h:270: bool descendantDrawsContent();
You should replace this with numDescendantsThatDrawContent() and have the
ubercomp layer return the number of sharedQuadStates it owns - 1.

[slavi, 2012-11-27 01:57:36]

On 2012/11/27 01:52:16, danakj wrote:
> https://chromiumcodereview.appspot.com/11316171/diff/12/cc/layer.h
> File cc/layer.h (left):
> 
> https://chromiumcodereview.appspot.com/11316171/diff/12/cc/layer.h#oldcode270
> cc/layer.h:270: bool descendantDrawsContent();
> You should replace this with numDescendantsThatDrawContent() and have the
> ubercomp layer return the number of sharedQuadStates it owns - 1.

You're looking at an old patch. I removed Layer::descendantDrawsContent,
LayerImpl::descendantDrawsContent and
DelegatedRendererLayerImpl::descendantDrawsContent all together in favor of a
static function in cc/layer_tree_host_common.cc.

To have it work for DelegatedRendererLayerImpl, there should be an overloaded
version in cc/layer_tree_host_common.cc. I didn't save
DelegatedRendererLayerImpl::descendantDrawsContent because
1. It's unused
2. It's a one liner
3. It has a big FIXME comment next to it

We can always resurrect/rewrite it when it'll be needed for UC.

[danakj, 2012-11-27 01:57:49]

Though.. even better would be bool hasMoreThan1DescendantThatDrawsContent(),
since that is what the function does.

One comment nit as well.

I removed it from the CQ for now, because this breaks ubercomp. I guess this
points at a missing test.

https://chromiumcodereview.appspot.com/11316171/diff/12/cc/layer_tree_host_co...
File cc/layer_tree_host_common.cc (right):

https://chromiumcodereview.appspot.com/11316171/diff/12/cc/layer_tree_host_co...
cc/layer_tree_host_common.cc:145: // descendants that draw content is >1, the
result if guaranteed to by >1 but
nit: "to be"

[danakj, 2012-11-27 01:59:52]

On 2012/11/27 01:57:36, slavi wrote:
> On 2012/11/27 01:52:16, danakj wrote:
> > https://chromiumcodereview.appspot.com/11316171/diff/12/cc/layer.h
> > File cc/layer.h (left):
> > 
> >
https://chromiumcodereview.appspot.com/11316171/diff/12/cc/layer.h#oldcode270
> > cc/layer.h:270: bool descendantDrawsContent();
> > You should replace this with numDescendantsThatDrawContent() and have the
> > ubercomp layer return the number of sharedQuadStates it owns - 1.
> 
> You're looking at an old patch. I removed Layer::descendantDrawsContent,
> LayerImpl::descendantDrawsContent and
> DelegatedRendererLayerImpl::descendantDrawsContent all together in favor of a
> static function in cc/layer_tree_host_common.cc.
> 
> To have it work for DelegatedRendererLayerImpl, there should be an overloaded
> version in cc/layer_tree_host_common.cc. I didn't save
> DelegatedRendererLayerImpl::descendantDrawsContent because
> 1. It's unused

It was used before, this CL is removing the call site.

> 2. It's a one liner
> 3. It has a big FIXME comment next to it

The FIXME states that it can make a smarter decision than what it is now. What
it does now is over conservative but always correct. Removing it makes it not
correct.

> We can always resurrect/rewrite it when it'll be needed for UC.

It's introducing a bug in rendering as we fill in the missing pieces for UC so
please don't remove it.

I will write a unit test to demonstrate the use case for it in the meantime.

[jamesr, 2012-11-27 02:30:30]

Why don't DelegatedRendererLayer(Impl) just override drawsContent() ?

[danakj, 2012-11-27 02:36:34]

On 2012/11/27 02:30:30, jamesr wrote:
> Why don't DelegatedRendererLayer(Impl) just override drawsContent() ?

Right, we need to think of DRLayer as the root of the delegated renderer's tree.
It's not a single layer, even though it appears to be one. The quads it produces
come from multiple layers, still.

[jamesr, 2012-11-27 02:42:35]

On 2012/11/27 02:36:34, danakj wrote:
> On 2012/11/27 02:30:30, jamesr wrote:
> > Why don't DelegatedRendererLayer(Impl) just override drawsContent() ?
> 
> Right, we need to think of DRLayer as the root of the delegated renderer's
tree.
> It's not a single layer, even though it appears to be one. The quads it
produces
> come from multiple layers, still.

The code as it exists now is misleading, though, since it's answering a question
about layers (do your descendants draw contents) by examining quads.  Hmmmm...

[danakj, 2012-11-27 03:38:37]

On 2012/11/27 02:42:35, jamesr wrote:
> On 2012/11/27 02:36:34, danakj wrote:
> > On 2012/11/27 02:30:30, jamesr wrote:
> > > Why don't DelegatedRendererLayer(Impl) just override drawsContent() ?
> > 
> > Right, we need to think of DRLayer as the root of the delegated renderer's
> tree.
> > It's not a single layer, even though it appears to be one. The quads it
> produces
> > come from multiple layers, still.
> 
> The code as it exists now is misleading, though, since it's answering a
question
> about layers (do your descendants draw contents) by examining quads.  Hmmmm...

It's true, the function name isn't ideal. It just inherited the name from
LayerTreeHostCommon. Something like "subtreeProducesOverlappingContent" would be
more precise, I suppose. In this case the layer is a subtree of one.

I guess scrollbar layers should return true for that also.. in case they ever
have opacity < 1.

[shawnsingh, 2012-11-27 07:16:09]

https://chromiumcodereview.appspot.com/11316171/diff/12/cc/layer_tree_host_co...
File cc/layer_tree_host_common.cc (right):

https://chromiumcodereview.appspot.com/11316171/diff/12/cc/layer_tree_host_co...
cc/layer_tree_host_common.cc:284: && (layer->drawsContent() ||
numDescendantsDrawContent > 1))
Sorry, I'm not following why this is our desired semantics.  Without yet hearing
your explanation, my immediate reaction is "we shouldn't need to know how many
descendants draw content"...  Can you please explain?

For example, what happens when a layer + one child both draw content?  Why would
that be a reasonable case for us to not make a renderSurface, where having 2
children that draw content is a reasonable case?

And whatever you explain should probably also be added in a comment here, since
it's not obvious...

[jamesr, 2012-11-27 17:57:53]

How about having "Layer::hasOverlapingQuads" ?

Shawn - the idea here is if a layer has opacity but no overlapping quads within
the subtree where opacity is applied, we don't need to create a render surface -
we can just draw with the opacity on the quad(s).  The simpler version of this
optimization is if the there is only one layer that draws contents in the
subtree (barring DelegatingLayerRenderer)s, we know there isn't any overlap.

[shawnsingh, 2012-11-27 18:52:38]

On 2012/11/27 17:57:53, jamesr wrote:
> How about having "Layer::hasOverlapingQuads" ?
> 
> Shawn - the idea here is if a layer has opacity but no overlapping quads
within
> the subtree where opacity is applied, we don't need to create a render surface
-
> we can just draw with the opacity on the quad(s).  The simpler version of this
> optimization is if the there is only one layer that draws contents in the
> subtree (barring DelegatingLayerRenderer)s, we know there isn't any overlap.

If that's the case, then the layer itself needs to be included in this count...
with a comment saying that this is our "quick way of estimating if layers might
overlap."

[shawnsingh, 2012-11-27 18:54:34]

In particular, if a layer drawsContent but no children do, I don't think we need
to make a surface either, but the code on this recent patch would create one.

[piman, 2012-11-27 23:53:36]

On Tue, Nov 27, 2012 at 10:54 AM, <shawnsingh@chromium.org> wrote:

> In particular, if a layer drawsContent but no children do, I don't think
> we need
> to make a surface either, but the code on this recent patch would create
> one.
>

I don't think so. The new code tests that either the layer draws and at
least one of the descendants draws, or that at least 2 descendants draw.


>
>
https://chromiumcodereview.**appspot.com/11316171/<https://chromiumcodereview...
>
> --
>
>
>

[commit-bot: I haz the power, 2012-11-28 22:44:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skaslev@chromium.org/11316171/14007

[danakj, 2012-11-28 22:48:11]

https://chromiumcodereview.appspot.com/11316171/diff/14007/cc/delegated_rende...
File cc/delegated_renderer_layer_impl.cc (right):

https://chromiumcodereview.appspot.com/11316171/diff/14007/cc/delegated_rende...
cc/delegated_renderer_layer_impl.cc:32: return
m_renderPassesInDrawOrder.isEmpty() ? 0 : 1;
This should be 0:2 shouldn't it?

[slavi, 2012-11-28 22:52:41]

On 2012/11/28 22:48:11, danakj wrote:
>
https://chromiumcodereview.appspot.com/11316171/diff/14007/cc/delegated_rende...
> File cc/delegated_renderer_layer_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11316171/diff/14007/cc/delegated_rende...
> cc/delegated_renderer_layer_impl.cc:32: return
> m_renderPassesInDrawOrder.isEmpty() ? 0 : 1;
> This should be 0:2 shouldn't it?

Done. I had the wrong impression from the offline chat yesterday it should be 1.

[commit-bot: I haz the power, 2012-11-28 22:54:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skaslev@chromium.org/11316171/5025

[commit-bot: I haz the power, 2012-11-28 23:58:36]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-29 00:45:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skaslev@chromium.org/11316171/12021

[commit-bot: I haz the power, 2012-11-29 18:25:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skaslev@chromium.org/11316171/5028

[commit-bot: I haz the power, 2012-11-29 23:52:00]

Retried try job too often on win_aura for step(s) content_browsertests

[commit-bot: I haz the power, 2012-11-30 17:50:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skaslev@chromium.org/11316171/5028

[commit-bot: I haz the power, 2012-11-30 18:24:58]

Change committed as 170487