Remove root layer specialness in calculateDrawTransforms

Remove root layer specialness in calculateDrawTransforms

This patch is step 2 in removing root layer specialness. The previous patch removed root
layer specialness from code outside of CCLayerTreeHostCommon, this patch removes it from
within CCLayerTreeHostCommon.

One subtle semantics change occurs with this patch, that only affects tests and not
real-world code: The root renderSurface now re-parents the root layer's drawTransform. In
practice, the root layer's drawTransform is always an identity matrix, so it does not
matter.

BUG=154442
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=164723

[shawnsingh, 2012-10-09 22:16:02]

Current status:  this first patch is the proposed complete change, but unit
tests are not yet updated.

After this patch, there is a subtle semantics difference where the root layer's
drawTransform is re-parented by it's renderSurface.  In practice that doesn't
matter.  However, about 12 unit tests would need to be updated to account for
this.

Personally I think it's appropriate to do the following:
(1) continue with this refactor
(2) fix the 12 unit tests
(3) add some assertions in an appropriate place, that the root layer should have
identity transform.  (they might even already exist and I don't realize it yet)

[danakj, 2012-10-09 22:21:53]

I think this is really cool Shawn.

In practice, I don't think anyone even uses the root surface's drawtransform,
cuz it never draws *to* anything. But I agree with assertions on the root's
transform still.

https://codereview.chromium.org/11017044/diff/1/cc/CCLayerTreeHostCommon.cpp
File cc/CCLayerTreeHostCommon.cpp (right):

https://codereview.chromium.org/11017044/diff/1/cc/CCLayerTreeHostCommon.cpp#...
cc/CCLayerTreeHostCommon.cpp:226: if (!layer->parent())
<3 this

https://codereview.chromium.org/11017044/diff/1/cc/CCLayerTreeHostCommon.cpp#...
cc/CCLayerTreeHostCommon.cpp:778: deviceViewportRect, true, 0,
renderSurfaceLayerList,
Can we remove the magic true bool here and give it a variable while we're here?

[enne (OOO), 2012-10-15 18:29:45]

https://codereview.chromium.org/11017044/diff/1/cc/CCLayerTreeHostCommon.cpp
File cc/CCLayerTreeHostCommon.cpp (right):

https://codereview.chromium.org/11017044/diff/1/cc/CCLayerTreeHostCommon.cpp#...
cc/CCLayerTreeHostCommon.cpp:226: if (!layer->parent())
On 2012/10/09 22:21:54, danakj wrote:
> <3 this

Can you remove the inconsistent use of layer == rootLayer vs !layer->parent()?
Do we even need to pass the root layer down this function recursively?

[shawnsingh, 2012-10-15 18:43:37]

On 2012/10/15 18:29:45, enne wrote:
> https://codereview.chromium.org/11017044/diff/1/cc/CCLayerTreeHostCommon.cpp
> File cc/CCLayerTreeHostCommon.cpp (right):
> 
>
https://codereview.chromium.org/11017044/diff/1/cc/CCLayerTreeHostCommon.cpp#...
> cc/CCLayerTreeHostCommon.cpp:226: if (!layer->parent())
> On 2012/10/09 22:21:54, danakj wrote:
> > <3 this
> 
> Can you remove the inconsistent use of layer == rootLayer vs !layer->parent()?
> Do we even need to pass the root layer down this function recursively?

That's a perfect idea, should make things even nicer.   Would it be worth adding
a layer->isRoot() helper, so that the code reads a bit more naturally?

Anyway, at the moment, this patch had gotten stuck on some scroll-compensation
problem.  I will have to either fix something in this patch or in a preliminary
patch before this one.

[danakj, 2012-10-15 18:50:14]

On Mon, Oct 15, 2012 at 2:43 PM,  <shawnsingh@chromium.org> wrote:
> On 2012/10/15 18:29:45, enne wrote:
>>
>>
>> https://codereview.chromium.org/11017044/diff/1/cc/CCLayerTreeHostCommon.cpp
>> File cc/CCLayerTreeHostCommon.cpp (right):
>
>
>
>
https://codereview.chromium.org/11017044/diff/1/cc/CCLayerTreeHostCommon.cpp#...
>>
>> cc/CCLayerTreeHostCommon.cpp:226: if (!layer->parent())
>> On 2012/10/09 22:21:54, danakj wrote:
>> > <3 this
>
>
>> Can you remove the inconsistent use of layer == rootLayer vs
>> !layer->parent()?
>> Do we even need to pass the root layer down this function recursively?
>
>
> That's a perfect idea, should make things even nicer.   Would it be worth
> adding
> a layer->isRoot() helper, so that the code reads a bit more naturally?

static inline layerIsRoot(layer) inside CCLTHCommon? It doesnt seem
like code other places care about?

> Anyway, at the moment, this patch had gotten stuck on some
> scroll-compensation
> problem.  I will have to either fix something in this patch or in a
> preliminary
> patch before this one.
>
> https://codereview.chromium.org/11017044/

[shawnsingh, 2012-10-25 23:38:26]

Next patch coming in a moment, has the following changes compared to the
previous version:

(1) added a isRootLayer helper private to layer_tree_host_common

(2) a few more DCHECKs that seemed appropriate

(3) this patch revealed a lot of issues in unit tests that are now fixed:

(3a) fixes to many verifyTransformsXXX tests that assumed root layer's surface
does not reparent transforms of root layer

(3b) fixed a bug in gutter quads unit test, where it was still using half-width,
half-height stuff, hard-coded in, without applying quadTransform when verifying
quads.  Earlier the transforms were lined up by fluke, now they are working
correctly.

(3c) fixed a scroll compensation unit test, where the expected results were
incorrect with the new root layer semantics

(3d) fixed an animation unit test that was too fragile in how it tested whether
layers were actually updated.  As part of this fix, I needed to add an arg that
allowed to set the opacity animation on an arbitrary layer in the test
infrastructure.

[shawnsingh, 2012-10-26 17:51:38]

I'm verifying locally, but I really don't think those layout/memory failures
have anything to do with this patch.  Reviewers can you still take a look?
thanks in advance

[enne (OOO), 2012-10-26 19:33:30]

http://codereview.chromium.org/11017044/diff/8001/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

http://codereview.chromium.org/11017044/diff/8001/cc/layer_tree_host_common.c...
cc/layer_tree_host_common.cc:316: //    (i.e. it would still compensate for root
layer's scrollDelta, even though that translation gets adopted by the root
renderSurface).
What? I'm pretty sure the root render surface is located at the origin and has
the size of the device viewport.

I don't understand this comment or what you mean by "reparenting".

[shawnsingh, 2012-10-26 19:51:29]

On 2012/10/26 19:33:30, enne wrote:
> http://codereview.chromium.org/11017044/diff/8001/cc/layer_tree_host_common.cc
> File cc/layer_tree_host_common.cc (right):
> 
>
http://codereview.chromium.org/11017044/diff/8001/cc/layer_tree_host_common.c...
> cc/layer_tree_host_common.cc:316: //    (i.e. it would still compensate for
root
> layer's scrollDelta, even though that translation gets adopted by the root
> renderSurface).
> What? I'm pretty sure the root render surface is located at the origin and has
> the size of the device viewport.
> 
> I don't understand this comment or what you mean by "reparenting".

Thanks for catching - the comment is wrong. Instead, we are assuming that the
root layer never scrolls. If we think this is a bad assumption, it should be
fixed in a separate patch anyway, just let me know your opinion.

[enne (OOO), 2012-10-26 20:33:20]

http://codereview.chromium.org/11017044/diff/22001/cc/test/layer_test_common.cc
File cc/test/layer_test_common.cc (right):

http://codereview.chromium.org/11017044/diff/22001/cc/test/layer_test_common....
cc/test/layer_test_common.cc:25: cc::IntRect quadRect =
cc::MathUtil::mapClippedRect(quad->sharedQuadState()->quadTransform,
cc::IntRect(quad->quadRect()));
This is wrong.  The rect is in the content space of the layer, so the quads do
not need to be transformed.

[shawnsingh, 2012-10-26 21:27:25]

On 2012/10/26 20:33:20, enne wrote:
>
http://codereview.chromium.org/11017044/diff/22001/cc/test/layer_test_common.cc
> File cc/test/layer_test_common.cc (right):
> 
>
http://codereview.chromium.org/11017044/diff/22001/cc/test/layer_test_common....
> cc/test/layer_test_common.cc:25: cc::IntRect quadRect =
> cc::MathUtil::mapClippedRect(quad->sharedQuadState()->quadTransform,
> cc::IntRect(quad->quadRect()));
> This is wrong.  The rect is in the content space of the layer, so the quads do
> not need to be transformed.


I think what you're referring to is the usage from tiled_layer_impl unit tests. 
 But in that code, it creates a dummy layer with identity transform, so mapping
is a no-op and things still test correctly.

However, in layer_tree_host_impl unit tests, the quad list being used in
viewportCovered test is actually in target surface space.

the untransformed quads are:
  layer quad:
         (0, 0) 200x200
  4 gutter quads:
         (0, 0)        1000x500   // top gutter
         (0, 500)    500x200     // left gutter
         (700,500) 300x200     // right gutter
         (0, 700)    1000x300   // bottom gutter

these 5 quads are not in the same space... the layer quad needs to apply it's
quadTransform so that it is actually located at (500, 500), only then will the
test pass correctly.

[enne (OOO), 2012-10-26 22:15:25]

In that case, the math here needs to be fixed.  mapClippedRect does an implicit
enclosingIntRect, which doesn't really satisfy "verifyQuadsExactlyCoverRect".

[shawnsingh, 2012-10-29 17:57:59]

On 2012/10/26 22:15:25, enne wrote:
> In that case, the math here needs to be fixed.  mapClippedRect does an
implicit
> enclosingIntRect, which doesn't really satisfy "verifyQuadsExactlyCoverRect".

OK, as we discussed offline, I added a helper function to verify that rounding
to integer will not inflate/change the values that are being tested.   This
should address our concerns about rounding.

[enne (OOO), 2012-10-29 18:02:11]

lgtm

[commit-bot: I haz the power, 2012-10-29 18:03:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shawnsingh@chromium.org/11017044/30001

[commit-bot: I haz the power, 2012-10-29 19:57:17]

Change committed as 164723