Stop displaying layers with non-invertible transforms.

Stop displaying layers with non-invertible transforms.

According to the CSS spec, if the transform of an object is
non-invertible the object and its content do not get displayed.

+ Added transform_is_invertible to Layer and LayerImpl which gets
  calculated once when the transform is set. When pusing to another layer
  the transform invertibility isn't recalculated.
+ Added invertibility check to LTHC::SubtreeShouldBeSkipped.

BUG=173726
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264341

[avallee, 2014-03-28 18:35:13]

ajuma@chromium.org: Please review changes in 

vollick@chromium.org: Please review changes in

[ajuma, 2014-03-28 18:49:07]

Please add tests.

https://codereview.chromium.org/217313003/diff/1/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/217313003/diff/1/cc/layers/layer_impl.cc#newc...
cc/layers/layer_impl.cc:1007: DLOG(ERROR) << "Can't change invertibility if
transform is unchanged";
How about a DCHECK instead of a DLOG?

https://codereview.chromium.org/217313003/diff/1/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/217313003/diff/1/cc/layers/layer_impl.h#newco...
cc/layers/layer_impl.h:604: // Prevent calculating transform_'s invertibility
too frequently.
"Cache transform_'s invertibility to avoid needing to recompute it"?

https://codereview.chromium.org/217313003/diff/1/cc/trees/layer_tree_host_com...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/217313003/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common.cc:482: if (!layer->transform_is_invertible())
Can this early-out interact negatively with readback requests (see the next
check below)? Should we also check if transform is animating here (just as we do
below before checking for opacity 0)?

https://codereview.chromium.org/217313003/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common.cc:509: if (!layer->transform_is_invertible())
Same questions as above about readback and animating transforms.

https://codereview.chromium.org/217313003/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common.cc:1081: if (!layer->transform_is_invertible())
{
Should we check if transform is animating? (e.g. Suppose we're animating scale
from 0 to 1. Do we really want to skip this layer when this is run on the main
thread layer tree?)

[avallee, 2014-04-15 18:53:25]

Added tests.

Let me know if you think there are more tests that make sense here.

https://codereview.chromium.org/217313003/diff/1/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/217313003/diff/1/cc/layers/layer_impl.cc#newc...
cc/layers/layer_impl.cc:1007: DLOG(ERROR) << "Can't change invertibility if
transform is unchanged";
On 2014/03/28 18:49:07, ajuma wrote:
> How about a DCHECK instead of a DLOG?

Done.

https://codereview.chromium.org/217313003/diff/1/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/217313003/diff/1/cc/layers/layer_impl.h#newco...
cc/layers/layer_impl.h:604: // Prevent calculating transform_'s invertibility
too frequently.
On 2014/03/28 18:49:07, ajuma wrote:
> "Cache transform_'s invertibility to avoid needing to recompute it"?

Done.

https://codereview.chromium.org/217313003/diff/1/cc/trees/layer_tree_host_com...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/217313003/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common.cc:482: if (!layer->transform_is_invertible())
On 2014/03/28 18:49:07, ajuma wrote:
> Can this early-out interact negatively with readback requests (see the next
> check below)? Should we also check if transform is animating here (just as we
do
> below before checking for opacity 0)?

Yes, we do. The spec is all about requiring that it's not rendered. It shouldn't
show up in readback or at any singular in-between transform.

https://codereview.chromium.org/217313003/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common.cc:509: if (!layer->transform_is_invertible())
On 2014/03/28 18:49:07, ajuma wrote:
> Same questions as above about readback and animating transforms.

See above.

https://codereview.chromium.org/217313003/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common.cc:1081: if (!layer->transform_is_invertible())
{
On 2014/03/28 18:49:07, ajuma wrote:
> Should we check if transform is animating? (e.g. Suppose we're animating scale
> from 0 to 1. Do we really want to skip this layer when this is run on the main
> thread layer tree?)

Yes we do.

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

https://codereview.chromium.org/217313003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_unittest.cc:2945: // TODO(avallee): Cases 2, 3
comments no longer accurate. CSS spec days to
What do you think of deleting these cases entirely?

https://codereview.chromium.org/217313003/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/217313003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:816: DCHECK(it->transform_is_invertible()) <<
"Layer should not have been added"
I didn't see any place where it made sense to add a test to
layer_tree_host_impl_unittests, do you think there's one that makes sense?

[ajuma, 2014-04-15 19:45:10]

https://codereview.chromium.org/217313003/diff/1/cc/trees/layer_tree_host_com...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/217313003/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common.cc:482: if (!layer->transform_is_invertible())
On 2014/04/15 18:53:25, avallee wrote:
> On 2014/03/28 18:49:07, ajuma wrote:
> > Can this early-out interact negatively with readback requests (see the next
> > check below)? Should we also check if transform is animating here (just as
we
> do
> > below before checking for opacity 0)?
> 
> Yes, we do. The spec is all about requiring that it's not rendered. It
shouldn't
> show up in readback or at any singular in-between transform.
 
I agree with not drawing it, but I'm worried that if we just skip the layer here
altogether even when we're animating it, this will make it impossible to solve
issues like crbug.com/354880, where we want to pre-rasterize layers that are
animating scale from 0 (since we'll end up skipping the subtree that we want to
pre-rasterize). Can we prevent drawing such an animated layer without skipping
it here?

https://codereview.chromium.org/217313003/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common.cc:1081: if (!layer->transform_is_invertible())
{
On 2014/04/15 18:53:25, avallee wrote:
> On 2014/03/28 18:49:07, ajuma wrote:
> > Should we check if transform is animating? (e.g. Suppose we're animating
scale
> > from 0 to 1. Do we really want to skip this layer when this is run on the
main
> > thread layer tree?)
> 
> Yes we do.

We do want to skip, or we do check? (I don't see this checked in the latest
patch.)

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

https://codereview.chromium.org/217313003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_unittest.cc:2945: // TODO(avallee): Cases 2, 3
comments no longer accurate. CSS spec days to
On 2014/04/15 18:53:26, avallee wrote:
> What do you think of deleting these cases entirely?

They're probably still worth keeping (with corrected comments).

https://codereview.chromium.org/217313003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_unittest.cc:9198: // Ensure that the entire
subtree under an layer with singular transform does
s/an/a

[avallee, 2014-04-16 18:20:56]

https://codereview.chromium.org/217313003/diff/1/cc/trees/layer_tree_host_com...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/217313003/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common.cc:482: if (!layer->transform_is_invertible())
On 2014/04/15 19:45:10, ajuma wrote:
> On 2014/04/15 18:53:25, avallee wrote:
> > On 2014/03/28 18:49:07, ajuma wrote:
> > > Can this early-out interact negatively with readback requests (see the
next
> > > check below)? Should we also check if transform is animating here (just as
> we
> > do
> > > below before checking for opacity 0)?
> > 
> > Yes, we do. The spec is all about requiring that it's not rendered. It
> shouldn't
> > show up in readback or at any singular in-between transform.
>  
> I agree with not drawing it, but I'm worried that if we just skip the layer
here
> altogether even when we're animating it, this will make it impossible to solve
> issues like crbug.com/354880, where we want to pre-rasterize layers that are
> animating scale from 0 (since we'll end up skipping the subtree that we want
to
> pre-rasterize). Can we prevent drawing such an animated layer without skipping
> it here?
Done. Won't skip on animated transform. Fix for second CL.

https://codereview.chromium.org/217313003/diff/1/cc/trees/layer_tree_host_com...
cc/trees/layer_tree_host_common.cc:1081: if (!layer->transform_is_invertible())
{
On 2014/04/15 19:45:10, ajuma wrote:
> On 2014/04/15 18:53:25, avallee wrote:
> > On 2014/03/28 18:49:07, ajuma wrote:
> > > Should we check if transform is animating? (e.g. Suppose we're animating
> scale
> > > from 0 to 1. Do we really want to skip this layer when this is run on the
> main
> > > thread layer tree?)
> > 
> > Yes we do.
> 
> We do want to skip, or we do check? (I don't see this checked in the latest
> patch.)

Let's check for now and this will need to be changed for pre-rastering.

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

https://codereview.chromium.org/217313003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_unittest.cc:9198: // Ensure that the entire
subtree under an layer with singular transform does
On 2014/04/15 19:45:11, ajuma wrote:
> s/an/a

Done.

https://codereview.chromium.org/217313003/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/217313003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:816: DCHECK(it->transform_is_invertible()) <<
"Layer should not have been added"
On 2014/04/15 18:53:26, avallee wrote:
> I didn't see any place where it made sense to add a test to
> layer_tree_host_impl_unittests, do you think there's one that makes sense?

Deleted this pending part 2.

[ajuma, 2014-04-16 19:24:49]

lgtm, but vollick should have a look too.

https://codereview.chromium.org/217313003/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/217313003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:511: // pre-raster.
This TODO should move to the previous function (the LayerImpl version).

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

https://codereview.chromium.org/217313003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_unittest.cc:2945: // TODO(avallee): Cases 2, 3
comments no longer accurate. CSS spec days to
Please fix the comments rather than having this TODO.

[avallee, 2014-04-16 19:33:24]

https://codereview.chromium.org/217313003/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/217313003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:511: // pre-raster.
On 2014/04/16 19:24:49, ajuma wrote:
> This TODO should move to the previous function (the LayerImpl version).

Done.

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

https://codereview.chromium.org/217313003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_unittest.cc:2945: // TODO(avallee): Cases 2, 3
comments no longer accurate. CSS spec days to
On 2014/04/16 19:24:49, ajuma wrote:
> Please fix the comments rather than having this TODO.

Done.

[Ian Vollick, 2014-04-16 19:37:14]

On 2014/04/16 19:33:24, avallee wrote:
>
https://codereview.chromium.org/217313003/diff/40001/cc/trees/layer_tree_host...
> File cc/trees/layer_tree_host_common.cc (right):
> 
>
https://codereview.chromium.org/217313003/diff/40001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host_common.cc:511: // pre-raster.
> On 2014/04/16 19:24:49, ajuma wrote:
> > This TODO should move to the previous function (the LayerImpl version).
> 
> Done.
> 
>
https://codereview.chromium.org/217313003/diff/40001/cc/trees/layer_tree_host...
> File cc/trees/layer_tree_host_common_unittest.cc (right):
> 
>
https://codereview.chromium.org/217313003/diff/40001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host_common_unittest.cc:2945: // TODO(avallee): Cases 2, 3
> comments no longer accurate. CSS spec days to
> On 2014/04/16 19:24:49, ajuma wrote:
> > Please fix the comments rather than having this TODO.
> 
> Done.

lgtm2

[avallee, 2014-04-16 19:43:28]

The CQ bit was checked by avallee@chromium.org

[commit-bot: I haz the power, 2014-04-16 19:43:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avallee@chromium.org/217313003/80001

[commit-bot: I haz the power, 2014-04-16 23:23:30]

Change committed as 264341