cc: Don't DCHECK for layers with non-invertible transforms.

cc: Don't DCHECK for layers with weird transforms.

A layer with a non-invertible transform should have an empty
visible content rect, so that we do not try to raster it.

A layer that has a transform that makes it non-visible in the
viewport would also inversely make the viewport non-visible
(empty) in its own space. The prioritization rect for this layer
should be empty, making all tiles not live.

Tests:
PictureLayerTilingTest.EmptyStartingRect
PictureLayerTilingIteratorTest.TilesExist
LayerTreeHostCommonTest.DrawableAndVisibleContentRectsForLayersWithUninvertibleTransform
LayerTreeHostTestUninvertibleTransformDoesNotBlockActivation.RunMultiThread

R=enne
BUG=229409
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193329

[danakj, 2013-04-08 22:40:52]

On youtube (chromos build/impl side painting on) I'm seeing a layer with this
transform which causes a DCHECK for me.

[ +0.0000 +0.0000 +0.0000 +0.0000  
  +0.0000 +0.0000 +0.0000 +0.0000  
  +0.0000 +0.0000 +1.0000 +0.0000  
  +0.0000 +0.0000 +0.0000 +1.0000 ]

[enne (OOO), 2013-04-09 02:08:38]

https://codereview.chromium.org/13820002/diff/1/cc/resources/picture_layer_ti...
File cc/resources/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/13820002/diff/1/cc/resources/picture_layer_ti...
cc/resources/picture_layer_tiling_unittest.cc:361: TEST(PictureLayerTilingTest,
EmptyStartingRect) {
WTB unit test that makes sure this layer also has visible resources ready.

[danakj, 2013-04-09 02:42:17]

https://codereview.chromium.org/13820002/diff/1/cc/resources/picture_layer_ti...
File cc/resources/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/13820002/diff/1/cc/resources/picture_layer_ti...
cc/resources/picture_layer_tiling_unittest.cc:361: TEST(PictureLayerTilingTest,
EmptyStartingRect) {
On 2013/04/09 02:08:38, enne wrote:
> WTB unit test that makes sure this layer also has visible resources ready.

OK will do!

[danakj, 2013-04-09 19:51:34]

3 tests now:

1) If the layer's rect is not invertible, then we should not compute its visible
content rect with an identity transform. We should just make its visible rect
empty.
- Tests that this is so.
2) If the viewport rect is empty in the layer's space for any other reason, we
should not make any tiles live.
- Tests that we generate an empty priority rect, and that this results in no
live tiles.

Last, in AreVRR() we dcheck that all tiles are live - they are in the visible
content rect - but don't block activation if they are not.

[danakj, 2013-04-09 19:52:29]

+shawnsingh

[enne (OOO), 2013-04-09 21:09:47]

Sorry to ask for a test again, but I've been burned by too many "accidentally
never activate" bugs in the past.  Can you make a PictureLayerImpl test that has
an empty visible content rect and check that AreVRR is true for this layer?

Your code looks reasonable to my eyes, but if we change AreVRR in the future
then I want to make sure this path won't cause an activation bug.

[shawnsingh, 2013-04-09 21:30:31]

I'm worried about the blanket assumption that "not invertible" implying "not
visible".   For example, if this uninvertibleness comes only from a projection
of the z value, then the x-y values would still be valid.

I wonder if this would be better:
(1) In this patch, instead of identifying the case we want to fix as
uninvertible, we want to identify whether it projects on x or y values.  If it
does, that implies the content really will not be visible, and we can apply the
fix made here.
(2) leave the "projection of z values" case handled by using identity
transforms, just to keep existing behavior.  I think neither the new way or the
old way are correct in this case, but at least the old way might draw something
correct, as opposed to not drawing anything at all.
(3) create a bug that we should use to follow-up and try to address
uninvertible-yet-visible transforms.

[danakj, 2013-04-09 22:46:18]

On 2013/04/09 21:30:31, shawnsingh wrote:
> I'm worried about the blanket assumption that "not invertible" implying "not
> visible".   For example, if this uninvertibleness comes only from a projection
> of the z value, then the x-y values would still be valid.
> 
> I wonder if this would be better:
> (1) In this patch, instead of identifying the case we want to fix as
> uninvertible, we want to identify whether it projects on x or y values.  If it
> does, that implies the content really will not be visible, and we can apply
the
> fix made here.
> (2) leave the "projection of z values" case handled by using identity
> transforms, just to keep existing behavior.  I think neither the new way or
the
> old way are correct in this case, but at least the old way might draw
something
> correct, as opposed to not drawing anything at all.

This would only work in super special cases though, no? It's using the identity
transform to decide what's visible, it could easily decide something completely
wrong if the transform isn't super trivial?

> (3) create a bug that we should use to follow-up and try to address
> uninvertible-yet-visible transforms.

[danakj, 2013-04-10 00:14:24]

Added
LayerTreeHostTestUninvertibleTransformDoesNotBlockActivation.RunMultiThread

[shawnsingh, 2013-04-10 00:23:28]

> This would only work in super special cases though, no? It's using the
identity
> transform to decide what's visible, it could easily decide something
completely
> wrong if the transform isn't super trivial?
> 

The way I see it, it's better to think that something is visible, and let it
draw/paint/possiblyjank with the correct uninvertible transform, rather than to
assume none of it is visible.  Am I missing something that might make it draw
more incorrectly?

But I still feel that "uninvertible" may have been the way we discovered this
bug, but it may not be the criteria we want to use to apply a fix.   I think we
should only fix transforms that are projecting x or y components, because those
are the uninvertible cases that will NOT be visible.   projecting the z
component will make an uninvertible matrix but its layer might still be visible.

[danakj, 2013-04-10 00:49:33]

Added TODO for shawn saying that we want a non-empty visible content rect
sometimes, but the behaviour is basically undefined right now when the
matrix is uninvertible.


On Tue, Apr 9, 2013 at 5:23 PM, <shawnsingh@chromium.org> wrote:

>  This would only work in super special cases though, no? It's using the
>>
> identity
>
>> transform to decide what's visible, it could easily decide something
>>
> completely
>
>> wrong if the transform isn't super trivial?
>>
>
>
> The way I see it, it's better to think that something is visible, and let
> it
> draw/paint/possiblyjank with the correct uninvertible transform, rather
> than to
> assume none of it is visible.  Am I missing something that might make it
> draw
> more incorrectly?
>
> But I still feel that "uninvertible" may have been the way we discovered
> this
> bug, but it may not be the criteria we want to use to apply a fix.   I
> think we
> should only fix transforms that are projecting x or y components, because
> those
> are the uninvertible cases that will NOT be visible.   projecting the z
> component will make an uninvertible matrix but its layer might still be
> visible.
>
>
>
https://codereview.chromium.**org/13820002/<https://codereview.chromium.org/1...
>

[enne (OOO), 2013-04-10 00:56:32]

Thanks for all the tests, lgtm

[commit-bot: I haz the power, 2013-04-10 01:01:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/13820002/19001

[commit-bot: I haz the power, 2013-04-10 05:12:48]

Change committed as 193329