cc: Directly use property trees to calculate clip rect

cc: Directly use property trees to calculate clip rect

Clip tree at this point uses top down recursion with caching to
calculate clips. This matches the behavior old CDP. Now that CDP is
gone, clip rect can be calculated directly from property trees without
recursion.

This CL is only turned on for cc_unittests.

This CL is a proof of concept. There is no caching so this CL would
not be as performance. This CL is behind a flag, and only switched
to after caching and visible rect calculations are finished.

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

Committed: https://crrev.com/365a6fe2125dd821db7df698778fc05f3099bcff
Cr-Commit-Position: refs/heads/master@{#393331}

[weiliangc, 2016-03-15 00:55:29]

Description was changed from

==========
[WIP] cc: Directly use property trees to calculate clip rect

Clip tree at this point uses top down recursion with caching to
calculate clips. This matches the behavior old CDP. Now that CDP is
gone, clip rect can be calculated directly from property trees without
recursion.

This CL is a proof of concept. There is no caching so this CL would
not be as performance. This CL will be behind a flag, and only switched
to after caching and visible rect calculations are finished.

R=enne
BUG=594675
==========

to

==========
[WIP] cc: Directly use property trees to calculate clip rect

Clip tree at this point uses top down recursion with caching to
calculate clips. This matches the behavior old CDP. Now that CDP is
gone, clip rect can be calculated directly from property trees without
recursion.

This CL is a proof of concept. There is no caching so this CL would
not be as performance. This CL will be behind a flag, and only switched
to after caching and visible rect calculations are finished.

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

[weiliangc, 2016-03-15 01:31:06]

Patchset #1 (id:1) has been deleted

[weiliangc, 2016-03-15 01:31:52]

Description was changed from

==========
[WIP] cc: Directly use property trees to calculate clip rect

Clip tree at this point uses top down recursion with caching to
calculate clips. This matches the behavior old CDP. Now that CDP is
gone, clip rect can be calculated directly from property trees without
recursion.

This CL is a proof of concept. There is no caching so this CL would
not be as performance. This CL will be behind a flag, and only switched
to after caching and visible rect calculations are finished.

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

to

==========
[WIP] cc: Directly use property trees to calculate clip rect

Clip tree at this point uses top down recursion with caching to
calculate clips. This matches the behavior old CDP. Now that CDP is
gone, clip rect can be calculated directly from property trees without
recursion.

This CL is a proof of concept. There is no caching so this CL would
not be as performance. This CL will be behind a flag, and only switched
to after caching and visible rect calculations are finished.

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

[weiliangc, 2016-03-15 01:37:07]

Mostly send out as first draft. Right now this only added CHECKs trying to flush
out bugs. Will use a flag to guard actually calculation later since there is no
caching.

The logic is in draw_prop_utils file.

[ajuma, 2016-03-15 14:59:56]

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

[ajuma, 2016-03-15 14:59:56]

Nice work, the overall logic looks great.

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

https://codereview.chromium.org/1800923002/diff/20001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:692: if (local_clip_id ==
target_node->data.clip_id)
This confused me for a bit since it seemed like this was checking the common
case of something having the same clip node as its target, in which case we'd
also need to check if the target has unclipped descendants, but (given that we
currently always create a clip node for each target), I think this is actually
checking if the local clip is the same as the parent of the clip node created
for the target? So, e.g., this would happen for a layer with a clip parent? If
so, could there be other clips in between the clip parent and the next clipped
target that also need to be accumulated?

https://codereview.chromium.org/1800923002/diff/20001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:736: if (!clip_node->data.applies_local_clip &&
clip_node->id != 1) {
Do we still need this special handling for the viewport clip? (Are there cases
where the viewport clip needs to be included in a clip rect?)

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

https://codereview.chromium.org/1800923002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.h:398: bool
ComputeTransformWithDifferenceInSublayerScale(
Seems unused?

[weiliangc, 2016-03-15 15:29:36]

address review comments.

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

https://codereview.chromium.org/1800923002/diff/20001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:692: if (local_clip_id ==
target_node->data.clip_id)
On 2016/03/15 at 14:59:56, ajuma wrote:
> This confused me for a bit since it seemed like this was checking the common
case of something having the same clip node as its target, in which case we'd
also need to check if the target has unclipped descendants, but (given that we
currently always create a clip node for each target), I think this is actually
checking if the local clip is the same as the parent of the clip node created
for the target? So, e.g., this would happen for a layer with a clip parent? If
so, could there be other clips in between the clip parent and the next clipped
target that also need to be accumulated?

This is a special case I started with, but is no longer needed. The following
code handles this case naturally. So going to remove this.

Maybe this is only true when we have two render surfaces without no clip in
between?

https://codereview.chromium.org/1800923002/diff/20001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:736: if (!clip_node->data.applies_local_clip &&
clip_node->id != 1) {
On 2016/03/15 at 14:59:56, ajuma wrote:
> Do we still need this special handling for the viewport clip? (Are there cases
where the viewport clip needs to be included in a clip rect?)

No, I realized viewport clip always applies local clip. Removed.

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

https://codereview.chromium.org/1800923002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.h:398: bool
ComputeTransformWithDifferenceInSublayerScale(
On 2016/03/15 at 14:59:56, ajuma wrote:
> Seems unused?

Ah thanks for the catch. Removed.

[weiliangc, 2016-03-18 22:59:23]

Patchset #4 (id:80001) has been deleted

[weiliangc, 2016-03-18 23:05:08]

Description was changed from

==========
[WIP] cc: Directly use property trees to calculate clip rect

Clip tree at this point uses top down recursion with caching to
calculate clips. This matches the behavior old CDP. Now that CDP is
gone, clip rect can be calculated directly from property trees without
recursion.

This CL is a proof of concept. There is no caching so this CL would
not be as performance. This CL will be behind a flag, and only switched
to after caching and visible rect calculations are finished.

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

to

==========
cc: Directly use property trees to calculate clip rect

Clip tree at this point uses top down recursion with caching to
calculate clips. This matches the behavior old CDP. Now that CDP is
gone, clip rect can be calculated directly from property trees without
recursion.

This CL is only turned on for cc_unittests.

This CL is a proof of concept. There is no caching so this CL would
not be as performance. This CL is behind a flag, and only switched
to after caching and visible rect calculations are finished.

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

[weiliangc, 2016-03-18 23:05:36]

Only verify with cc_unittests. Please take a look.

[ajuma, 2016-03-19 00:29:41]

https://codereview.chromium.org/1800923002/diff/120001/cc/test/layer_tree_set...
File cc/test/layer_tree_settings_for_testing.cc (right):

https://codereview.chromium.org/1800923002/diff/120001/cc/test/layer_tree_set...
cc/test/layer_tree_settings_for_testing.cc:11:
this->verify_clip_tree_calculations = true;
Nit: Using 'this' isn't really needed here.

https://codereview.chromium.org/1800923002/diff/120001/cc/trees/draw_property...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/1800923002/diff/120001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:765: DCHECK(accumulated_clip ==
clip_node->data.clip_in_target_space);
The original patch also had logic to compare the effect node clip rect to the
surface's computed clip_rect for clipped surfaces. Did you mean to move that
logic here rather than deleting it? There doesn't seem to be a nice way to get
the surface here (to check is_clipped), but I think changing the 'if' condition
to ((clip->node->data.layers_are_clipped || effect_node->id == 0)  &&
!effect_node->data.has_unclipped_descendants &&
effect_node->data.has_render_surface) is equivalent to having a clipped surface.
(Please also include the two rects in the output of the DCHECK, like the DCHECK
for layer clip rects below.)

https://codereview.chromium.org/1800923002/diff/120001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:785: if
(!property_trees->non_root_surfaces_enabled ||
Since this is currently only called when non_root_surfaces_enabled is true, it
might be clearer to remove this condition for now.

https://codereview.chromium.org/1800923002/diff/120001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:885: if
(property_trees->non_root_surfaces_enabled) {
Just to make sure I understand, the reason this doesn't work yet with non-root
surfaces disabled is that the has_render_surface value on each node is currently
only updated on the main thread?

https://codereview.chromium.org/1800923002/diff/120001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:958: DCHECK(render_surface->OwningLayerId() ==
clip_node->owner_id)
Since we now have this DCHECK, the 'if' below can be removed.

[weiliangc, 2016-05-11 23:08:08]

addressed review comments.

https://codereview.chromium.org/1800923002/diff/120001/cc/test/layer_tree_set...
File cc/test/layer_tree_settings_for_testing.cc (right):

https://codereview.chromium.org/1800923002/diff/120001/cc/test/layer_tree_set...
cc/test/layer_tree_settings_for_testing.cc:11:
this->verify_clip_tree_calculations = true;
On 2016/03/19 00:29:41, ajuma wrote:
> Nit: Using 'this' isn't really needed here.

Done.

https://codereview.chromium.org/1800923002/diff/120001/cc/trees/draw_property...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/1800923002/diff/120001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:765: DCHECK(accumulated_clip ==
clip_node->data.clip_in_target_space);
On 2016/03/19 00:29:41, ajuma wrote:
> The original patch also had logic to compare the effect node clip rect to the
> surface's computed clip_rect for clipped surfaces. Did you mean to move that
> logic here rather than deleting it? There doesn't seem to be a nice way to get
> the surface here (to check is_clipped), but I think changing the 'if'
condition
> to ((clip->node->data.layers_are_clipped || effect_node->id == 0)  &&
> !effect_node->data.has_unclipped_descendants &&
> effect_node->data.has_render_surface) is equivalent to having a clipped
surface.
> (Please also include the two rects in the output of the DCHECK, like the
DCHECK
> for layer clip rects below.)

Added the logic back. Now the verification on render surface should run after
render surface calculated draw property behind the flag.

https://codereview.chromium.org/1800923002/diff/120001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:785: if
(!property_trees->non_root_surfaces_enabled ||
On 2016/03/19 00:29:41, ajuma wrote:
> Since this is currently only called when non_root_surfaces_enabled is true, it
> might be clearer to remove this condition for now.

Added computation and verification for resourceless software draw so edited this
condition.

https://codereview.chromium.org/1800923002/diff/120001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:885: if
(property_trees->non_root_surfaces_enabled) {
On 2016/03/19 00:29:41, ajuma wrote:
> Just to make sure I understand, the reason this doesn't work yet with non-root
> surfaces disabled is that the has_render_surface value on each node is
currently
> only updated on the main thread?

Yes. I have added a hack to the computation (aka everybody has root as target)
so we can verify for resourceless draw unitttests as well.

https://codereview.chromium.org/1800923002/diff/120001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:958: DCHECK(render_surface->OwningLayerId() ==
clip_node->owner_id)
On 2016/03/19 00:29:41, ajuma wrote:
> Since we now have this DCHECK, the 'if' below can be removed.

Done.

[ajuma, 2016-05-11 23:26:40]

Thanks, lgtm

[weiliangc, 2016-05-12 18:43:37]

The CQ bit was checked by weiliangc@chromium.org

[weiliangc, 2016-05-12 18:43:38]

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/1800923002/#ps200001
(title: "fix perftests compile by not verify clip tree calc there")

[commit-bot: I haz the power, 2016-05-12 18:44:03]

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

[commit-bot: I haz the power, 2016-05-12 19:32:43]

Committed patchset #9 (id:200001)

[commit-bot: I haz the power, 2016-05-12 19:33:48]

Description was changed from

==========
cc: Directly use property trees to calculate clip rect

Clip tree at this point uses top down recursion with caching to
calculate clips. This matches the behavior old CDP. Now that CDP is
gone, clip rect can be calculated directly from property trees without
recursion.

This CL is only turned on for cc_unittests.

This CL is a proof of concept. There is no caching so this CL would
not be as performance. This CL is behind a flag, and only switched
to after caching and visible rect calculations are finished.

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

to

==========
cc: Directly use property trees to calculate clip rect

Clip tree at this point uses top down recursion with caching to
calculate clips. This matches the behavior old CDP. Now that CDP is
gone, clip rect can be calculated directly from property trees without
recursion.

This CL is only turned on for cc_unittests.

This CL is a proof of concept. There is no caching so this CL would
not be as performance. This CL is behind a flag, and only switched
to after caching and visible rect calculations are finished.

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

Committed: https://crrev.com/365a6fe2125dd821db7df698778fc05f3099bcff
Cr-Commit-Position: refs/heads/master@{#393331}
==========

[commit-bot: I haz the power, 2016-05-12 19:33:49]

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