Rasterize at maximum scale for scale animations on CPU-rasterized layers

Rasterize at maximum scale for scale animations on CPU-rasterized layers

This re-rasterizes CPU-rasterized layers animating scale at the
maximum scale that will be applied during the animation, if this
maximum scale can be computed.

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

[Ian Vollick, 2014-04-05 02:24:33]

The plumbing looks good (and heroic!), but I have a just a few comments on the
other stuff.

https://codereview.chromium.org/226283004/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/226283004/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1086: maximum_animation_scale_factor *
ideal_contents_scale_);
I'm totally new to this code, but I'm a little confused by the asymmetry with
the code below. Do we not need to consider the page and device scales in this
case?

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

https://codereview.chromium.org/226283004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1569: if (!globals.can_adjust_raster_scales
||
Can we avoid the HasOnlyTranslationTransforms work if we can't adjust raster
scales? (And all of it if data_from_ancestor.maximum_animation_scale_factor ==
0?)

I know I had bad luck with this previously, but is it hard on perf if this is
moved into a helper?

https://codereview.chromium.org/226283004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1573: if
(data_from_ancestor.ancestor_is_animating_scale ||
Might be worth mentioning why aren't we supporting scale accumulation. Is it
because the scales, when multiplied, could get enormous?

https://codereview.chromium.org/226283004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1581: layer_animation_scale_factor =
This could really use a comment. It took me a while to remember that what you're
doing here is compensating for the "current" value of an in-progress scale
animation.

https://codereview.chromium.org/226283004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1588:
data_from_ancestor.maximum_animation_scale_factor;
If I'm following along correctly, the point of this code is largely so that a
scale factor of 0 propagates all the way down to the leaves. Doing this with
multiplication works, but I think it's a bit too cute; it wasn't obvious to me
that this wouldn't result in an accumulation of scale factors. Can this be made
explicit or at least a comment added?

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

https://codereview.chromium.org/226283004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_unittest.cc:9865: EXPECT_EQ(0.f,
child->last_maximum_animation_scale_factor());
Might be worth mentioning that we don't support combining these scale factors
and that the 0 means "no max scale."

[ajuma, 2014-04-07 15:26:59]

https://codereview.chromium.org/226283004/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/226283004/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1086: maximum_animation_scale_factor *
ideal_contents_scale_);
On 2014/04/05 02:24:33, Ian Vollick wrote:
> I'm totally new to this code, but I'm a little confused by the asymmetry with
> the code below. Do we not need to consider the page and device scales in this
> case?

In the usual non-animated non-pinching case, we don't consider the page and
device scales when setting the contents scale (see above, where we just set
raster_contents_scale_ = ideal_contents_scale_). I *think* the reason it's
considered in the case below is simply for the sake of picking a reasonable
minimum when we know nothing about the animation.

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

https://codereview.chromium.org/226283004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1569: if (!globals.can_adjust_raster_scales
||
On 2014/04/05 02:24:33, Ian Vollick wrote:
> Can we avoid the HasOnlyTranslationTransforms work if we can't adjust raster
> scales? (And all of it if data_from_ancestor.maximum_animation_scale_factor ==
> 0?)

Done.

> I know I had bad luck with this previously, but is it hard on perf if this is
> moved into a helper?

Moved into a helper. Perftests run locally seemed fine with this.

https://codereview.chromium.org/226283004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1573: if
(data_from_ancestor.ancestor_is_animating_scale ||
On 2014/04/05 02:24:33, Ian Vollick wrote:
> Might be worth mentioning why aren't we supporting scale accumulation. Is it
> because the scales, when multiplied, could get enormous?

Done.

https://codereview.chromium.org/226283004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1581: layer_animation_scale_factor =
On 2014/04/05 02:24:33, Ian Vollick wrote:
> This could really use a comment. It took me a while to remember that what
you're
> doing here is compensating for the "current" value of an in-progress scale
> animation.

Done.

https://codereview.chromium.org/226283004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1588:
data_from_ancestor.maximum_animation_scale_factor;
On 2014/04/05 02:24:33, Ian Vollick wrote:
> If I'm following along correctly, the point of this code is largely so that a
> scale factor of 0 propagates all the way down to the leaves. Doing this with
> multiplication works, but I think it's a bit too cute; it wasn't obvious to me
> that this wouldn't result in an accumulation of scale factors. Can this be
made
> explicit or at least a comment added?

Removed the multiplication.

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

https://codereview.chromium.org/226283004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_unittest.cc:9865: EXPECT_EQ(0.f,
child->last_maximum_animation_scale_factor());
On 2014/04/05 02:24:33, Ian Vollick wrote:
> Might be worth mentioning that we don't support combining these scale factors
> and that the 0 means "no max scale."

Done.

[Ian Vollick, 2014-04-07 16:31:32]

lgtm

https://codereview.chromium.org/226283004/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/226283004/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1086: maximum_animation_scale_factor *
ideal_contents_scale_);
On 2014/04/07 15:27:00, ajuma wrote:
> On 2014/04/05 02:24:33, Ian Vollick wrote:
> > I'm totally new to this code, but I'm a little confused by the asymmetry
with
> > the code below. Do we not need to consider the page and device scales in
this
> > case?
> 
> In the usual non-animated non-pinching case, we don't consider the page and
> device scales when setting the contents scale (see above, where we just set
> raster_contents_scale_ = ideal_contents_scale_). I *think* the reason it's
> considered in the case below is simply for the sake of picking a reasonable
> minimum when we know nothing about the animation.
Ah, that makes sense. Thanks for the explanation.

[ajuma, 2014-04-07 16:53:07]

The CQ bit was checked by ajuma@chromium.org

[commit-bot: I haz the power, 2014-04-07 16:53:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/226283004/60001

[ajuma, 2014-04-07 17:01:43]

The CQ bit was unchecked by ajuma@chromium.org

[Ian Vollick, 2014-04-07 17:16:35]

On 2014/04/07 17:01:43, ajuma wrote:
> The CQ bit was unchecked by mailto:ajuma@chromium.org

+enne for picture_layer_impl

[enne (OOO), 2014-04-08 14:19:40]

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

https://codereview.chromium.org/226283004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1018: static inline void
CalculateAnimationScaleFactor(
Can you template this to only do work for LayerImpl and be a no-op for Layer?
This factor isn't used on the Layer side, so is wasted.

https://codereview.chromium.org/226283004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1065: DCHECK(layer_transform_scales !=
gfx::Vector2dF());
Can you convince me this is legit? It seems like a layer could have a
perspective transform here, causing this to use the fallback value and DCHECK?

https://codereview.chromium.org/226283004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1068: std::max(layer_transform_scales.x(),
layer_transform_scales.y());
This scares me.

How do you prevent multiple tilings at nearly-but-not-exactly the same scale
from being created during the animation?

My worry is that you have some complicated animation, and you stop at some messy
time, then dividing by the current scale may not give you exactly the same value
from frame to frame.  So, maybe one frame you create 4.00000001 and the next you
create 3.9999999998 and then you finally stop at 4 and you don't have that
tiling ready either.

Can you add a PictureLayerImpl unit test that animates through some arbitrary,
floating point unfriendly values of time and verifies that at all times the
final tiling is created at the exact right scale and that there's only one of
them?

I think the only way to make this work is to not use the layer transform scale
at all, and instead use the scales from the parent * maximum scale, which will
be consistent (if it's not animating too)?

[ajuma, 2014-04-08 18:59:16]

PTAL.

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

https://codereview.chromium.org/226283004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1018: static inline void
CalculateAnimationScaleFactor(
On 2014/04/08 14:19:41, enne wrote:
> Can you template this to only do work for LayerImpl and be a no-op for Layer?
> This factor isn't used on the Layer side, so is wasted.

Done.

https://codereview.chromium.org/226283004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1065: DCHECK(layer_transform_scales !=
gfx::Vector2dF());
On 2014/04/08 14:19:41, enne wrote:
> Can you convince me this is legit? It seems like a layer could have a
> perspective transform here, causing this to use the fallback value and DCHECK?

This code has now been removed, but the reason we could assert this was that we
already check above if the combined_transform is a scale or translation; a
perspective transform in layer->transform() would have caused combined_transform
to fail that check above.

https://codereview.chromium.org/226283004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1068: std::max(layer_transform_scales.x(),
layer_transform_scales.y());
On 2014/04/08 14:19:41, enne wrote:
> This scares me.
> 
> How do you prevent multiple tilings at nearly-but-not-exactly the same scale
> from being created during the animation?
> 
> My worry is that you have some complicated animation, and you stop at some
messy
> time, then dividing by the current scale may not give you exactly the same
value
> from frame to frame.  So, maybe one frame you create 4.00000001 and the next
you
> create 3.9999999998 and then you finally stop at 4 and you don't have that
> tiling ready either.
> 
> Can you add a PictureLayerImpl unit test that animates through some arbitrary,
> floating point unfriendly values of time and verifies that at all times the
> final tiling is created at the exact right scale and that there's only one of
> them?
> 
> I think the only way to make this work is to not use the layer transform scale
> at all, and instead use the scales from the parent * maximum scale, which will
> be consistent (if it's not animating too)?

I've removed the division-based approach. We no longer use the layer transform
scale at all from layers running scale animations, instead using only the
maximum scale for such layers.

[enne (OOO), 2014-04-08 19:18:47]

https://codereview.chromium.org/226283004/diff/130001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/226283004/diff/130001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:1085: std::max(raster_contents_scale_,
maximum_animation_scale_factor);
Maybe I'm missing something, but this seems incorrect with respect to device and
page scale.

maximum_animation_scale_factor in CalcDrawProperties looks like it includes
device and page scale because it takes scales from the parent_matrix.  However,
raster contents scale does not include device and page scale in this line of
code.

This is a bikeshed request, but can you name maximum_animation_scale_factor to
be what it is? It no longer seems to be a factor.  Is it
maximum_animation_raster_scale? Is it maximum_animation_contents_scale?

https://codereview.chromium.org/226283004/diff/130001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/226283004/diff/130001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1772: float device_scale = 1.f;
Can you use a non-1 device and page scale in this test?

https://codereview.chromium.org/226283004/diff/130001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/226283004/diff/130001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1055:
!layer->layer_animation_controller()->HasOnlyTranslationTransforms();
This is a weird bit of logic to read.  Couldn't a layer have rotation? That's
not a translation but it's also not a scale animation.

[ajuma, 2014-04-08 19:48:09]

https://codereview.chromium.org/226283004/diff/130001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/226283004/diff/130001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:1085: std::max(raster_contents_scale_,
maximum_animation_scale_factor);
On 2014/04/08 19:18:47, enne wrote:
> Maybe I'm missing something, but this seems incorrect with respect to device
and
> page scale.
> 
> maximum_animation_scale_factor in CalcDrawProperties looks like it includes
> device and page scale because it takes scales from the parent_matrix. 
However,
> raster contents scale does not include device and page scale in this line of
> code.

I think I'm confused. ideal_contents_scale is calculated in CalcDrawProperties
as the scale of combined_transform. For LayerImpls, UpdateLayerContentsScale
passes this value directly to CalculateContentsScale, where it is saved as
ideal_contents_scale_, and then set directly to raster_contents_scale_ above (in
the non-pinching case). So it seems like the behavior in the animated case
(taking a scale from parent_transform and from the layer's transform) is
consistent with the non-animated case (taking a scale from combined_transform).
Am I misunderstanding the logic in the non-animated case?

[enne (OOO), 2014-04-08 19:53:19]

https://codereview.chromium.org/226283004/diff/130001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/226283004/diff/130001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:1063: raster_contents_scale_ =
ideal_contents_scale_;
This looks wrong, to be honest.  It should be ideal_contents_scale_ /
ideal_device_scale_.

https://codereview.chromium.org/226283004/diff/130001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:1085: std::max(raster_contents_scale_,
maximum_animation_scale_factor);
On 2014/04/08 19:48:09, ajuma wrote:
> On 2014/04/08 19:18:47, enne wrote:
> > Maybe I'm missing something, but this seems incorrect with respect to device
> and
> > page scale.
> > 
> > maximum_animation_scale_factor in CalcDrawProperties looks like it includes
> > device and page scale because it takes scales from the parent_matrix. 
> However,
> > raster contents scale does not include device and page scale in this line of
> > code.
> 
> I think I'm confused. ideal_contents_scale is calculated in CalcDrawProperties
> as the scale of combined_transform. For LayerImpls, UpdateLayerContentsScale
> passes this value directly to CalculateContentsScale, where it is saved as
> ideal_contents_scale_, and then set directly to raster_contents_scale_ above
(in
> the non-pinching case). So it seems like the behavior in the animated case
> (taking a scale from parent_transform and from the layer's transform) is
> consistent with the non-animated case (taking a scale from
combined_transform).
> Am I misunderstanding the logic in the non-animated case?

I'm trying to assert that maximum_animation_scale_factor is really
maximum_animation_contents_scale based on the way you calculate it in
CalcDrawProps, but you are using it right here as if it were
maximum_animation_raster_scale.  I think if you adjust your test to use non-1
values for device and page scale, you'll see the issue.

[enne (OOO), 2014-04-08 19:57:05]

Nevermind, ignore me.  I think I am just confused by raster_source_scale vs.
raster_contents_scale.  Sorry for the noise.  I think
maximum_animation_scale_factor is really maximum_animation_contents_scale in all
cases and is correct (but maybe misnamed).

[ajuma, 2014-04-08 20:30:39]

PTAL.

https://codereview.chromium.org/226283004/diff/130001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/226283004/diff/130001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:1085: std::max(raster_contents_scale_,
maximum_animation_scale_factor);
On 2014/04/08 19:18:47, enne wrote:
> This is a bikeshed request, but can you name maximum_animation_scale_factor to
> be what it is? It no longer seems to be a factor.  Is it
> maximum_animation_raster_scale? Is it maximum_animation_contents_scale?

Renamed to maximum_animation_contents_scale.

https://codereview.chromium.org/226283004/diff/130001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/226283004/diff/130001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1055:
!layer->layer_animation_controller()->HasOnlyTranslationTransforms();
On 2014/04/08 19:18:47, enne wrote:
> This is a weird bit of logic to read.  Couldn't a layer have rotation? That's
> not a translation but it's also not a scale animation.

I've added a comment to clarify this.

[enne (OOO), 2014-04-08 20:37:00]

lgtm.  Thanks for the additional comment, and sorry about my confusion.

https://codereview.chromium.org/226283004/diff/150001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/226283004/diff/150001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1772: float device_scale = 1.f;
Can you change this to device and page scale to be non-1, just as a sanity
check?

[ajuma, 2014-04-08 20:54:55]

vollick, PTAL.

https://codereview.chromium.org/226283004/diff/150001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/226283004/diff/150001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1772: float device_scale = 1.f;
On 2014/04/08 20:37:01, enne wrote:
> Can you change this to device and page scale to be non-1, just as a sanity
> check?

Done.

[Ian Vollick, 2014-04-09 03:24:23]

On 2014/04/08 20:54:55, ajuma wrote:
> vollick, PTAL.
> 
>
https://codereview.chromium.org/226283004/diff/150001/cc/layers/picture_layer...
> File cc/layers/picture_layer_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/226283004/diff/150001/cc/layers/picture_layer...
> cc/layers/picture_layer_impl_unittest.cc:1772: float device_scale = 1.f;
> On 2014/04/08 20:37:01, enne wrote:
> > Can you change this to device and page scale to be non-1, just as a sanity
> > check?
> 
> Done.

lgtm!

[ajuma, 2014-04-09 13:04:07]

+skyostil for content/browser/android/edge_effect.cc

[Sami, 2014-04-09 16:31:31]

content/browser/android/edge_effect.cc lgtm.

[ajuma, 2014-04-09 16:40:04]

The CQ bit was checked by ajuma@chromium.org

[commit-bot: I haz the power, 2014-04-09 16:41:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/226283004/180001

[commit-bot: I haz the power, 2014-04-09 17:49:27]

Change committed as 262737