cc:: Add transform nodes at boundaries of 3d rendering contexts

cc:: Add transform nodes at boundaries of 3d rendering contexts

We then store the sorting context id in transform nodes and
use it to determine layer and surface backface visibility.
This patch also fixes a bug in backface visibility
computation when we have nested 3d rendering contexts.

BUG=568746, 581570
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[jaydasika, 2016-01-23 00:29:42]

Description was changed from

==========
cc:: Add transform nodes at boundaries of 3d rendering contexts

BUG=
==========

to

==========
cc:: Add transform nodes at boundaries of 3d rendering contexts

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jaydasika, 2016-01-23 00:38:49]

jaydasika@chromium.org changed reviewers:
+ ajuma@chromium.org, enne@chromium.org, vollick@chromium.org,
weiliangc@chromium.org

[jaydasika, 2016-01-23 00:38:49]

Implemented Ali's suggestion of adding a transform node at 3d rendering context
boundaries. Sending it out to get opinions on the changes.

Tests are not yet added.

[ajuma, 2016-01-25 14:35:22]

https://codereview.chromium.org/1624693004/diff/1/cc/trees/draw_property_util...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/1624693004/diff/1/cc/trees/draw_property_util...
cc/trees/draw_property_utils.cc:233: if (node->data.is_3d_sorted && parent_node
&& parent_node->data.is_3d_sorted)
Is this equivalent to the existing logic? Say we have: ...L->A->B->C->M....
somewhere in the tree, where A,B,C are all 3d sorted, and L,M are not. In the
existing code, B and C are treated as being in an existing 3d context (and A
isn't). In the new code, A and M will have transform nodes, but B and C won't.
But this means neither B nor C will be treated as being in an existing 3d
context.

https://codereview.chromium.org/1624693004/diff/1/cc/trees/draw_property_util...
cc/trees/draw_property_utils.cc:265: return
layer->transform().IsBackFaceVisible();
If we reach here, it it necessarily the case that the layer is the root of a new
rendering context? (Consider the example from the comment above, where multiple
3d sorted layers have the same transform id.)

[weiliangc, 2016-01-25 21:29:26]

https://codereview.chromium.org/1624693004/diff/1/cc/trees/draw_property_util...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/1624693004/diff/1/cc/trees/draw_property_util...
cc/trees/draw_property_utils.cc:233: if (node->data.is_3d_sorted && parent_node
&& parent_node->data.is_3d_sorted)
On 2016/01/25 at 14:35:22, ajuma wrote:
> Is this equivalent to the existing logic? Say we have: ...L->A->B->C->M....
somewhere in the tree, where A,B,C are all 3d sorted, and L,M are not. In the
existing code, B and C are treated as being in an existing 3d context (and A
isn't). In the new code, A and M will have transform nodes, but B and C won't.
But this means neither B nor C will be treated as being in an existing 3d
context.

Would it make sense for A, B/C, and M to create 3 transform nodes?
  Ta
 / \
Tb  Tm
Where Ta would indicate root, Tb would be existing, and Tm would be endOf?

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

https://codereview.chromium.org/1624693004/diff/1/cc/trees/property_tree.h#ne...
cc/trees/property_tree.h:121: bool is_3d_sorted : 1;
Can we have 3d rendering context within 3d rendering context? Is bool sufficient
or do we need an id?

[ajuma, 2016-01-25 21:53:00]

https://codereview.chromium.org/1624693004/diff/1/cc/trees/draw_property_util...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/1624693004/diff/1/cc/trees/draw_property_util...
cc/trees/draw_property_utils.cc:233: if (node->data.is_3d_sorted && parent_node
&& parent_node->data.is_3d_sorted)
On 2016/01/25 21:29:26, weiliangc wrote:
> On 2016/01/25 at 14:35:22, ajuma wrote:
> > Is this equivalent to the existing logic? Say we have: ...L->A->B->C->M....
> somewhere in the tree, where A,B,C are all 3d sorted, and L,M are not. In the
> existing code, B and C are treated as being in an existing 3d context (and A
> isn't). In the new code, A and M will have transform nodes, but B and C won't.
> But this means neither B nor C will be treated as being in an existing 3d
> context.
> 
> Would it make sense for A, B/C, and M to create 3 transform nodes?
>   Ta
>  / \
> Tb  Tm
> Where Ta would indicate root, Tb would be existing, and Tm would be endOf?

That makes sense (if adding 3 nodes isn't too expensive).

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

https://codereview.chromium.org/1624693004/diff/1/cc/trees/property_tree.h#ne...
cc/trees/property_tree.h:121: bool is_3d_sorted : 1;
On 2016/01/25 21:29:26, weiliangc wrote:
> Can we have 3d rendering context within 3d rendering context? Is bool
sufficient
> or do we need an id?

Looking at PaintLayer::renderingContextRoot (the id is just a hash of this
value), it seems possible to have:
A->B->C where A and C preserve-3d but B doesn't, resulting in all layers being
3d sorted but A and B having a different id than C. So I think we need an id
here.

[jaydasika, 2016-01-25 22:05:36]

On 2016/01/25 21:53:00, ajuma wrote:
>
https://codereview.chromium.org/1624693004/diff/1/cc/trees/draw_property_util...
> File cc/trees/draw_property_utils.cc (right):
> 
>
https://codereview.chromium.org/1624693004/diff/1/cc/trees/draw_property_util...
> cc/trees/draw_property_utils.cc:233: if (node->data.is_3d_sorted &&
parent_node
> && parent_node->data.is_3d_sorted)
> On 2016/01/25 21:29:26, weiliangc wrote:
> > On 2016/01/25 at 14:35:22, ajuma wrote:
> > > Is this equivalent to the existing logic? Say we have:
...L->A->B->C->M....
> > somewhere in the tree, where A,B,C are all 3d sorted, and L,M are not. In
the
> > existing code, B and C are treated as being in an existing 3d context (and A
> > isn't). In the new code, A and M will have transform nodes, but B and C
won't.
> > But this means neither B nor C will be treated as being in an existing 3d
> > context.
> > 
> > Would it make sense for A, B/C, and M to create 3 transform nodes?
> >   Ta
> >  / \
> > Tb  Tm
> > Where Ta would indicate root, Tb would be existing, and Tm would be endOf?
> 
> That makes sense (if adding 3 nodes isn't too expensive).
> 
> https://codereview.chromium.org/1624693004/diff/1/cc/trees/property_tree.h
> File cc/trees/property_tree.h (right):
> 
>
https://codereview.chromium.org/1624693004/diff/1/cc/trees/property_tree.h#ne...
> cc/trees/property_tree.h:121: bool is_3d_sorted : 1;
> On 2016/01/25 21:29:26, weiliangc wrote:
> > Can we have 3d rendering context within 3d rendering context? Is bool
> sufficient
> > or do we need an id?
> 
> Looking at PaintLayer::renderingContextRoot (the id is just a hash of this
> value), it seems possible to have:
> A->B->C where A and C preserve-3d but B doesn't, resulting in all layers being
> 3d sorted but A and B having a different id than C. So I think we need an id
> here.

The current implementation (without this patch) would treat C as neither the
root
of a new rendering context nor as in an existing 3d context. So,we don't seem to
be
handling this case.

[jaydasika, 2016-01-26 00:10:01]

https://codereview.chromium.org/1624693004/diff/1/cc/trees/draw_property_util...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/1624693004/diff/1/cc/trees/draw_property_util...
cc/trees/draw_property_utils.cc:233: if (node->data.is_3d_sorted && parent_node
&& parent_node->data.is_3d_sorted)
On 2016/01/25 21:53:00, ajuma wrote:
> On 2016/01/25 21:29:26, weiliangc wrote:
> > On 2016/01/25 at 14:35:22, ajuma wrote:
> > > Is this equivalent to the existing logic? Say we have:
...L->A->B->C->M....
> > somewhere in the tree, where A,B,C are all 3d sorted, and L,M are not. In
the
> > existing code, B and C are treated as being in an existing 3d context (and A
> > isn't). In the new code, A and M will have transform nodes, but B and C
won't.
> > But this means neither B nor C will be treated as being in an existing 3d
> > context.
> > 
> > Would it make sense for A, B/C, and M to create 3 transform nodes?
> >   Ta
> >  / \
> > Tb  Tm
> > Where Ta would indicate root, Tb would be existing, and Tm would be endOf?
> 
> That makes sense (if adding 3 nodes isn't too expensive).

I think we can do with just 2 nodes. A and M will create transform
nodes.

IsInExistingRenderingContext =
if (node owner = layer)
  node->is_3d_sorted && parent->is_3d_sorted
else
  node->is_3d_sorted

[jaydasika, 2016-01-27 20:47:35]

Description was changed from

==========
cc:: Add transform nodes at boundaries of 3d rendering contexts

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc:: Add transform nodes at boundaries of 3d rendering contexts

We then store the sorting context id in transform nodes and use it to determine
layer and surface backface visibility. This patch also fixes a bug in backface
visibility computation when we have nested 3d rendering contexts.
 
BUG=568746,581570
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jaydasika, 2016-01-27 20:48:04]

Description was changed from

==========
cc:: Add transform nodes at boundaries of 3d rendering contexts

We then store the sorting context id in transform nodes and use it to determine
layer and surface backface visibility. This patch also fixes a bug in backface
visibility computation when we have nested 3d rendering contexts.
 
BUG=568746,581570
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc:: Add transform nodes at boundaries of 3d rendering contexts

We then store the sorting context id in transform nodes and
use it to determine layer and surface backface visibility.
This patch also fixes a bug in backface visibility
computation when we have nested 3d rendering contexts.
 
BUG=568746,581570
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jaydasika, 2016-01-27 20:52:37]

Fixed the backface visibility bug when we have nested 3d rendering contexts.
Added test.

https://codereview.chromium.org/1624693004/diff/1/cc/trees/draw_property_util...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/1624693004/diff/1/cc/trees/draw_property_util...
cc/trees/draw_property_utils.cc:265: return
layer->transform().IsBackFaceVisible();
On 2016/01/25 14:35:22, ajuma wrote:
> If we reach here, it it necessarily the case that the layer is the root of a
new
> rendering context? (Consider the example from the comment above, where
multiple
> 3d sorted layers have the same transform id.)

We always have a transform node for surfaces. So we won't have that issue here.

[jaydasika, 2016-01-27 21:49:04]

After Ali suggested that we should check if addition of transform nodes effects
ComputeTransforms performance, I measured the time for computing visible rects
on the compositor on transform heavy pages (poster circle and
https://www.keithclark.co.uk/labs/css-fps/nojs/) with and without this patch and
did not find any significant regression.
In fact, the number of transform nodes created with and without this patch was
almost the same for these pages. I think the reason for this is, we create a
render surface when we get out of 3d rendering context and so form a transform
node anyway. When we enter a new rendering context, it is highly likely that we
also have a perspective and so create a transform node.

[enne (OOO), 2016-01-27 21:55:32]

I previously measured the expense of transform nodes on an N4 at roughly
0.0015ms per node per ComputeTransforms call.  It was consistent across a
variety of sizes of trees.  I think we should still try to not make nodes
unnecessarily (as something like only 1000 starts taking 1.5ms), but 3d
rendering contexts are rare enough that I agree it shouldn't be an issue.

https://docs.google.com/spreadsheets/d/1TLx5iqAUO5ZAEleNtJYmc-7XSwNpZhZqE5NtK...

[ajuma, 2016-01-28 00:08:36]

Thanks, lgtm.

https://codereview.chromium.org/1624693004/diff/1/cc/trees/draw_property_util...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/1624693004/diff/1/cc/trees/draw_property_util...
cc/trees/draw_property_utils.cc:265: return
layer->transform().IsBackFaceVisible();
On 2016/01/27 20:52:37, jaydasika wrote:
> On 2016/01/25 14:35:22, ajuma wrote:
> > If we reach here, it it necessarily the case that the layer is the root of a
> new
> > rendering context? (Consider the example from the comment above, where
> multiple
> > 3d sorted layers have the same transform id.)
> 
> We always have a transform node for surfaces. So we won't have that issue
here.

Oh, right, I missed that this layer owns a surface.

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

https://codereview.chromium.org/1624693004/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:271: // We use layer's transform to determine
back face visibility when its the
typo: "it's"

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

https://codereview.chromium.org/1624693004/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:6455: // double-sided, but will
still be in RSLL as its in a differnt 3d rendering
typos: "it's" and "different"

[jaydasika, 2016-01-28 19:11:37]

The CQ bit was checked by jaydasika@chromium.org

[jaydasika, 2016-01-28 19:11: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/1624693004/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-01-28 19:12:55]

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

[commit-bot: I haz the power, 2016-01-28 20:18:42]

Description was changed from

==========
cc:: Add transform nodes at boundaries of 3d rendering contexts

We then store the sorting context id in transform nodes and
use it to determine layer and surface backface visibility.
This patch also fixes a bug in backface visibility
computation when we have nested 3d rendering contexts.
 
BUG=568746,581570
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc:: Add transform nodes at boundaries of 3d rendering contexts

We then store the sorting context id in transform nodes and
use it to determine layer and surface backface visibility.
This patch also fixes a bug in backface visibility
computation when we have nested 3d rendering contexts.
 
BUG=568746,581570
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-01-28 20:18:43]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-28 20:19:50]

Description was changed from

==========
cc:: Add transform nodes at boundaries of 3d rendering contexts

We then store the sorting context id in transform nodes and
use it to determine layer and surface backface visibility.
This patch also fixes a bug in backface visibility
computation when we have nested 3d rendering contexts.
 
BUG=568746,581570
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc:: Add transform nodes at boundaries of 3d rendering contexts

We then store the sorting context id in transform nodes and
use it to determine layer and surface backface visibility.
This patch also fixes a bug in backface visibility
computation when we have nested 3d rendering contexts.

BUG=568746,581570
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[commit-bot: I haz the power, 2016-01-28 20:19:51]

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