Create RenderSurface on Effect Tree

Create RenderSurface on Effect Tree

Move RenderSurface creation reason to effect tree. Update LayerImpl's
API to include SetForceRenderSurface. Update unittests.

R=enne
BUG=557160
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/4374c2cc231a7e5b6ae65d0e0814b5dccf445200
Cr-Commit-Position: refs/heads/master@{#363544}

Committed: https://crrev.com/c154ce2efa59dbaecb3cb038adb247426f7c6988
Cr-Commit-Position: refs/heads/master@{#363967}

[weiliangc, 2015-12-02 00:37:56]

Description was changed from

==========
Create RenderSurface on Effect Tree

Move RenderSurface creation reason to effect tree. Update LayerImpl's
API to include SetForceRenderSurface. Update unittests.

R=enne
BUG=557160
==========

to

==========
Create RenderSurface on Effect Tree

Move RenderSurface creation reason to effect tree. Update LayerImpl's
API to include SetForceRenderSurface. Update unittests.

R=enne
BUG=557160
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[weiliangc, 2015-12-02 01:07:28]

Spirit child of my previous patch on move RS creation to impl thread
(crrev.com/1449193002). Cleaned up the ShouldRenderToSeparateSurface on LTHC,
and rm can_render_to_separate_surface on main thread.

[ajuma, 2015-12-02 14:54:21]

ajuma@chromium.org changed reviewers:
+ ajuma@chromium.org

[ajuma, 2015-12-02 14:54:21]

https://codereview.chromium.org/1491033002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/1491033002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:8785: EXPECT_EQ(gfx::Rect(22, 21),
test_layer->visible_rect_from_property_trees());
Is this change because layer_clips_subtree has a surface now but didn't before?
(And do we need to change whether layer_clips_subtree has a surface?)

https://codereview.chromium.org/1491033002/diff/20001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/1491033002/diff/20001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.h:238: property_trees_.needs_rebuild = true;
This will only actually trigger a rebuild in tests right? (Since
CalculateDrawPropertiesAndVerify will be called with DONT_BUILD_PROPERTY_TREES
when called by LayerTreeImpl.) Please add a comment about this. Also, are there
tests that really require this? Since (I think) this code will only get reached
during a commit, won't a freshly built set of property trees get set on the
LayerTreeImpl during the commit?

https://codereview.chromium.org/1491033002/diff/20001/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/1491033002/diff/20001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:600:
data_for_children->axis_align_since_render_target = true;
Should the two lines above be conditional on should_create_render_surface?

[weiliangc, 2015-12-02 18:16:39]

Updated according to review comments.

https://codereview.chromium.org/1491033002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/1491033002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:8785: EXPECT_EQ(gfx::Rect(22, 21),
test_layer->visible_rect_from_property_trees());
On 2015/12/02 at 14:54:21, ajuma wrote:
> Is this change because layer_clips_subtree has a surface now but didn't
before? (And do we need to change whether layer_clips_subtree has a surface?)

Mostly before ptree is built using LayerImpl's has_render_surface information as
is, now ExecuteCalcDrawProps determines whether layer has RS. Since
layer_clips_subtree's parent significant_transform is not axis-aligned, it now
has a RenderSurface. I can rm all the SetForceRenderSurface calls if that helps?

Since render_surface layer here still clips the test_layer I just changed the
test expectation to match.

Adding a special code path for LayerImpl unittests to build effect tree
differently (based on pre-set has_rs) seems like adding too much complexity and
not testing real behavior.

https://codereview.chromium.org/1491033002/diff/20001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/1491033002/diff/20001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.h:238: property_trees_.needs_rebuild = true;
On 2015/12/02 at 14:54:21, ajuma wrote:
> This will only actually trigger a rebuild in tests right? (Since
CalculateDrawPropertiesAndVerify will be called with DONT_BUILD_PROPERTY_TREES
when called by LayerTreeImpl.) Please add a comment about this. Also, are there
tests that really require this? Since (I think) this code will only get reached
during a commit, won't a freshly built set of property trees get set on the
LayerTreeImpl during the commit?

Sorry I think I left this in during one of my attempts to make some unittests
pass. Unused and removed.

https://codereview.chromium.org/1491033002/diff/20001/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/1491033002/diff/20001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:600:
data_for_children->axis_align_since_render_target = true;
On 2015/12/02 at 14:54:21, ajuma wrote:
> Should the two lines above be conditional on should_create_render_surface?

Nice catch. Thanks!

Done.

[ajuma, 2015-12-02 18:25:08]

Thanks, lgtm % enne.

https://codereview.chromium.org/1491033002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/1491033002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:8785: EXPECT_EQ(gfx::Rect(22, 21),
test_layer->visible_rect_from_property_trees());
On 2015/12/02 18:16:38, weiliangc wrote:
> On 2015/12/02 at 14:54:21, ajuma wrote:
> > Is this change because layer_clips_subtree has a surface now but didn't
> before? (And do we need to change whether layer_clips_subtree has a surface?)
> 
> Mostly before ptree is built using LayerImpl's has_render_surface information
as
> is, now ExecuteCalcDrawProps determines whether layer has RS. Since
> layer_clips_subtree's parent significant_transform is not axis-aligned, it now
> has a RenderSurface.

Ah, that makes sense, thanks.

> I can rm all the SetForceRenderSurface calls if that helps?

I think it's fine to leave them. If you decide to remove them, please add a
comment above (where you check that layer_clips_subtree has a surface) that the
layer has a surface because of non-axis-aligned clipping.

> Since render_surface layer here still clips the test_layer I just changed the
> test expectation to match.
> 
> Adding a special code path for LayerImpl unittests to build effect tree
> differently (based on pre-set has_rs) seems like adding too much complexity
and
> not testing real behavior.

Agreed.

[enne (OOO), 2015-12-02 21:59:51]

Looking great in general.  A few general comments.

https://codereview.chromium.org/1491033002/diff/40001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1491033002/diff/40001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1813: void LayerImpl::SetForceRenderSurface(bool
force_render_surface) {
I guess you need this to force the creation of a node that has a surface?

https://codereview.chromium.org/1491033002/diff/40001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/1491033002/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:465: void
UpdateRenderSurfacesWithEffectTree(EffectTree* effect_tree,
I'm a little confused why there needs to be an update function.  Can there be a
Layer(Impl)::HasRenderSurface function that just says "is the owner_id of this
effect node me? if so, I have a render surface if the effect node does".

https://codereview.chromium.org/1491033002/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:713: #if DCHECK_IS_ON()
I kind of hate DCHECK_IS_ON ifdef proliferation.  I suspect that function will
be optimized out, so just remove all of these?

https://codereview.chromium.org/1491033002/diff/40001/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/1491033002/diff/40001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:438: *child_axis_alignment =
This doesn't seem like it fits in this function.  Can it just get calculated by
the caller, since it doesn't get used here?

https://codereview.chromium.org/1491033002/diff/40001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:448: // TODO(weiliangc): After slimming paint
there won't be replica layers.
Such hope!

https://codereview.chromium.org/1491033002/diff/40001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:566:
data_for_children->compound_transform_since_render_target *=
It's kind of sad to me we have to drag along this transform recursively.  Can
you ask the transform tree about it?  Alternatively, can we just simplify and
consider axis_align_since_render_target to just be if every transform along the
way preserves axis alignment independently?  Does the math work out on that?

[weiliangc, 2015-12-04 00:54:54]

https://codereview.chromium.org/1491033002/diff/40001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/1491033002/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:465: void
UpdateRenderSurfacesWithEffectTree(EffectTree* effect_tree,
On 2015/12/02 at 21:59:51, enne wrote:
> I'm a little confused why there needs to be an update function.  Can there be
a Layer(Impl)::HasRenderSurface function that just says "is the owner_id of this
effect node me? if so, I have a render surface if the effect node does".

For not remembering convenient thing such as owner_id.

Updated to a simpler version.

https://codereview.chromium.org/1491033002/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:713: #if DCHECK_IS_ON()
On 2015/12/02 at 21:59:51, enne wrote:
> I kind of hate DCHECK_IS_ON ifdef proliferation.  I suspect that function will
be optimized out, so just remove all of these?

Done.

https://codereview.chromium.org/1491033002/diff/40001/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/1491033002/diff/40001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:438: *child_axis_alignment =
On 2015/12/02 at 21:59:51, enne wrote:
> This doesn't seem like it fits in this function.  Can it just get calculated
by the caller, since it doesn't get used here?

Moved out.

https://codereview.chromium.org/1491033002/diff/40001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:448: // TODO(weiliangc): After slimming paint
there won't be replica layers.
On 2015/12/02 at 21:59:51, enne wrote:
> Such hope!

It's a TODO now. TODOs are never wrong!

https://codereview.chromium.org/1491033002/diff/40001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:566:
data_for_children->compound_transform_since_render_target *=
On 2015/12/02 at 21:59:51, enne wrote:
> It's kind of sad to me we have to drag along this transform recursively.  Can
you ask the transform tree about it?  Alternatively, can we just simplify and
consider axis_align_since_render_target to just be if every transform along the
way preserves axis alignment independently?  Does the math work out on that?

Unfortunately Preserves2dAxisAlignment() is distributive, since A * B might ends
up have x or y perspective making it non-axis-aligned, when both A and B are
axis aligned.
(https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/transform.c...)

Since it's building the transform tree at the same time, I'm not sure there is
nice value to ask directly. 

:(

[ajuma, 2015-12-04 14:38:38]

https://codereview.chromium.org/1491033002/diff/40001/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/1491033002/diff/40001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:566:
data_for_children->compound_transform_since_render_target *=
On 2015/12/04 00:54:54, weiliangc wrote:
> On 2015/12/02 at 21:59:51, enne wrote:
> > It's kind of sad to me we have to drag along this transform recursively. 
Can
> you ask the transform tree about it?  Alternatively, can we just simplify and
> consider axis_align_since_render_target to just be if every transform along
the
> way preserves axis alignment independently?  Does the math work out on that?
> 
> Unfortunately Preserves2dAxisAlignment() is distributive, since A * B might
ends
> up have x or y perspective making it non-axis-aligned, when both A and B are
> axis aligned.
>
(https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/transform.c...)
> 
> Since it's building the transform tree at the same time, I'm not sure there is
> nice value to ask directly. 
> 
> :(

I wonder if we could get away with pessimistically treating any matrix along the
way that isn't a scale or translation as non-axis-aligned, just like
TransformOperations::PreservesAxisAlignment does. In some situations that would
cause additional render surfaces, but probably those are rare in practice? On
the other hand, since it's a behavior change, it might best be left as an
optimization to be done in a separate CL.

[ajuma, 2015-12-04 15:17:01]

https://codereview.chromium.org/1491033002/diff/40001/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/1491033002/diff/40001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:566:
data_for_children->compound_transform_since_render_target *=
On 2015/12/04 14:38:38, ajuma wrote:
> On 2015/12/04 00:54:54, weiliangc wrote:
> > On 2015/12/02 at 21:59:51, enne wrote:
> > > It's kind of sad to me we have to drag along this transform recursively. 
> Can
> > you ask the transform tree about it?  Alternatively, can we just simplify
and
> > consider axis_align_since_render_target to just be if every transform along
> the
> > way preserves axis alignment independently?  Does the math work out on that?
> > 
> > Unfortunately Preserves2dAxisAlignment() is distributive, since A * B might
> ends
> > up have x or y perspective making it non-axis-aligned, when both A and B are
> > axis aligned.
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/transform.c...)
> > 
> > Since it's building the transform tree at the same time, I'm not sure there
is
> > nice value to ask directly. 
> > 
> > :(
> 
> I wonder if we could get away with pessimistically treating any matrix along
the
> way that isn't a scale or translation as non-axis-aligned, just like
> TransformOperations::PreservesAxisAlignment does. In some situations that
would
> cause additional render surfaces, but probably those are rare in practice? On
> the other hand, since it's a behavior change, it might best be left as an
> optimization to be done in a separate CL.

Hmm, it turns out we used to do something pessimistic before (checking if the
combined transform was a scale or translation rather than the more complicated
logic in gfx::Transform::Preserves2dAxisAlignment), and the additional surfaces
did in fact cause problems -- see http://crbug.com/258760 and
http://crrev.com/11576036. So I think we're stuck with needing to know the
combined transform.

[enne (OOO), 2015-12-04 17:39:30]

lgtm

> Hmm, it turns out we used to do something pessimistic before (checking if the
combined transform was a scale or translation rather than the more complicated
logic in gfx::Transform::Preserves2dAxisAlignment), and the additional surfaces
did in fact cause problems -- see http://crbug.com/258760 and
http://crrev.com/11576036. So I think we're stuck with needing to know the
combined transform.

Thanks for digging up that info.  :)

https://codereview.chromium.org/1491033002/diff/60001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/1491033002/diff/60001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:487: } else if (effect_node->owner_id ==
layer->id() &&
This bottom half of the function looks like a duplicate of the above.  Can you
just call UpdateRenderSurfaesWithEffectTree(effect_tree, layer) instead of this?

[weiliangc, 2015-12-04 20:28:12]

https://codereview.chromium.org/1491033002/diff/60001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/1491033002/diff/60001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:487: } else if (effect_node->owner_id ==
layer->id() &&
On 2015/12/04 at 17:39:30, enne wrote:
> This bottom half of the function looks like a duplicate of the above.  Can you
just call UpdateRenderSurfaesWithEffectTree(effect_tree, layer) instead of this?

Done.

[weiliangc, 2015-12-04 21:49:06]

The CQ bit was checked by weiliangc@chromium.org

[weiliangc, 2015-12-04 21:49:07]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
enne@chromium.org
Link to the patchset: https://codereview.chromium.org/1491033002/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2015-12-04 21:52:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1491033002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1491033002/100001

[commit-bot: I haz the power, 2015-12-04 22:01:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-04 22:01:18]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[weiliangc, 2015-12-07 16:40:59]

The CQ bit was checked by weiliangc@chromium.org

[weiliangc, 2015-12-07 16:40:59]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
enne@chromium.org
Link to the patchset: https://codereview.chromium.org/1491033002/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2015-12-07 16:42:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1491033002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1491033002/120001

[commit-bot: I haz the power, 2015-12-07 19:26:11]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-12-07 19:27:31]

Description was changed from

==========
Create RenderSurface on Effect Tree

Move RenderSurface creation reason to effect tree. Update LayerImpl's
API to include SetForceRenderSurface. Update unittests.

R=enne
BUG=557160
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Create RenderSurface on Effect Tree

Move RenderSurface creation reason to effect tree. Update LayerImpl's
API to include SetForceRenderSurface. Update unittests.

R=enne
BUG=557160
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/4374c2cc231a7e5b6ae65d0e0814b5dccf445200
Cr-Commit-Position: refs/heads/master@{#363544}
==========

[commit-bot: I haz the power, 2015-12-07 19:27:32]

Patchset 7 (id:??) landed as
https://crrev.com/4374c2cc231a7e5b6ae65d0e0814b5dccf445200
Cr-Commit-Position: refs/heads/master@{#363544}

[weiliangc, 2015-12-08 21:53:46]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1505243003/ by weiliangc@chromium.org.

The reason for reverting is: crbug.com/567529. Top crash..

[weiliangc, 2015-12-09 00:50:28]

Quick fix for the crash.

Test:
1. Go to http://www.google.com/intl/en/chromecast/
2. Click on ‘BUY NOW’ button present on top right corner of the page
3. Does not crash
4. No matter how many times you try to buy a chromecast, does not crash

[enne (OOO), 2015-12-09 00:53:04]

Fix lgtm, thanks.

[weiliangc, 2015-12-09 00:54:09]

The CQ bit was checked by weiliangc@chromium.org

[weiliangc, 2015-12-09 00:54:10]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/1491033002/#ps160001
(title: "fix for crash")

[commit-bot: I haz the power, 2015-12-09 01:01:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1491033002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1491033002/160001

[commit-bot: I haz the power, 2015-12-09 03:39:36]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-12-09 03:40:36]

Description was changed from

==========
Create RenderSurface on Effect Tree

Move RenderSurface creation reason to effect tree. Update LayerImpl's
API to include SetForceRenderSurface. Update unittests.

R=enne
BUG=557160
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/4374c2cc231a7e5b6ae65d0e0814b5dccf445200
Cr-Commit-Position: refs/heads/master@{#363544}
==========

to

==========
Create RenderSurface on Effect Tree

Move RenderSurface creation reason to effect tree. Update LayerImpl's
API to include SetForceRenderSurface. Update unittests.

R=enne
BUG=557160
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/4374c2cc231a7e5b6ae65d0e0814b5dccf445200
Cr-Commit-Position: refs/heads/master@{#363544}

Committed: https://crrev.com/c154ce2efa59dbaecb3cb038adb247426f7c6988
Cr-Commit-Position: refs/heads/master@{#363967}
==========

[commit-bot: I haz the power, 2015-12-09 03:40:37]

Patchset 9 (id:??) landed as
https://crrev.com/c154ce2efa59dbaecb3cb038adb247426f7c6988
Cr-Commit-Position: refs/heads/master@{#363967}