cc: Make PictureLayerImpl use a better choice for animated raster scale.

cc: Make PictureLayerImpl use a better choice for animated raster scale.

Two changes here:
1. When finding the max scale of an animation, only consider the scales
at each of the animations keyframes excluding the animations origin.

This is done because the origin is not an interesting scale to use, we
are animating away from it. And in the case of a layer at a high scale,
we are unable to raster all of its content at that scale. It would be
better to use the scale at the end of the transform since that is what
we will want to raster eventually.

2. When PictureLayerImpl chooses to use the animation scale:
a) Always allow using it if the scale is less than the current raster
scale choice. It can only put us in a better position memory-wise.
b) Don't use the max() with the current raster scale choice, that means
we will never use the animation scale if it is zooming out, which is
not what we want. This would also mean we choose a new raster scale on
every frame, which is bad. Just use the scale of the animation straight
up, and if it's not known, use a source scale of 1.

R=ajuma, vmpstr
BUG=421812
Committed: https://crrev.com/e488f71403f5a3a9bd43629b4b9fbb7b9d3d6ff7
Cr-Commit-Position: refs/heads/master@{#299162}

[vmpstr, 2014-10-10 00:50:49]

https://codereview.chromium.org/642983003/diff/1/cc/animation/keyframed_anima...
File cc/animation/keyframed_animation_curve.cc (right):

https://codereview.chromium.org/642983003/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve.cc:383:
fmax(std::abs(target_scale_for_segment.x()),
Why fmax?

https://codereview.chromium.org/642983003/diff/1/cc/animation/keyframed_anima...
File cc/animation/keyframed_animation_curve_unittest.cc (right):

https://codereview.chromium.org/642983003/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve_unittest.cc:551:
operations4.AppendScale(0.5f, 0.3f, 0.8f);
Can you make that one -0.8f? (just to ensure that we're grabbing the abs scale)

https://codereview.chromium.org/642983003/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/642983003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:1175: if (maximum_scale <
raster_contents_scale_) {
Does the comment need updating above? Also, can you add a comment here for
what's the two cases. I'm not sure if it's just an optimization or something
else.

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

https://codereview.chromium.org/642983003/diff/220001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:1189: raster_contents_scale_ = maximum_scale;
If I understand this correctly, maximum_scale is a maximum scale at keyframes
not including the intial one. Is that correct? In that case what happens if my
animation is from some large zoom (when layer is 1000x1000) in to a 5x5 pixels
square that kicks off after a couple of seconds? Would the result basically
rerasterize at the scale needed for 5x5 even when it's at 999x999? (This is
assuming there's only two keyframes: the current one and the final one).

[danakj, 2014-10-10 01:00:19]

PTAL

https://codereview.chromium.org/642983003/diff/1/cc/animation/keyframed_anima...
File cc/animation/keyframed_animation_curve.cc (right):

https://codereview.chromium.org/642983003/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve.cc:383:
fmax(std::abs(target_scale_for_segment.x()),
On 2014/10/10 00:50:48, vmpstr wrote:
> Why fmax?

oops, I meant fmaxf

https://codereview.chromium.org/642983003/diff/1/cc/animation/keyframed_anima...
File cc/animation/keyframed_animation_curve_unittest.cc (right):

https://codereview.chromium.org/642983003/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve_unittest.cc:551:
operations4.AppendScale(0.5f, 0.3f, 0.8f);
On 2014/10/10 00:50:49, vmpstr wrote:
> Can you make that one -0.8f? (just to ensure that we're grabbing the abs
scale)

Sure, I think did this somewhere else too.. but done!

https://codereview.chromium.org/642983003/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/642983003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:1175: if (maximum_scale <
raster_contents_scale_) {
On 2014/10/10 00:50:49, vmpstr wrote:
> Does the comment need updating above? Also, can you add a comment here for
> what's the two cases. I'm not sure if it's just an optimization or something
> else.

Yar.

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

https://codereview.chromium.org/642983003/diff/220001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:1189: raster_contents_scale_ = maximum_scale;
On 2014/10/10 00:50:49, vmpstr wrote:
> If I understand this correctly, maximum_scale is a maximum scale at keyframes
> not including the intial one. Is that correct? In that case what happens if my
> animation is from some large zoom (when layer is 1000x1000) in to a 5x5 pixels
> square that kicks off after a couple of seconds? Would the result basically
> rerasterize at the scale needed for 5x5 even when it's at 999x999? (This is
> assuming there's only two keyframes: the current one and the final one).

We'd raster new content at the very small scale yes. That doesn't mean we'd
throw away the old tiles though, we'd consider those better for drawing purposes
and keep them around until we actually got (close?) to the 5x5 scale. That's
what my intention with this change is.

[vmpstr, 2014-10-10 03:31:23]

https://codereview.chromium.org/642983003/diff/400001/cc/animation/keyframed_...
File cc/animation/keyframed_animation_curve.cc (right):

https://codereview.chromium.org/642983003/diff/400001/cc/animation/keyframed_...
cc/animation/keyframed_animation_curve.cc:383:
fmaxf(std::abs(target_scale_for_segment.x()),
hehe, why fmaxf then? :P Also, should this be std::fmaxf?

[danakj, 2014-10-10 05:30:11]

https://codereview.chromium.org/642983003/diff/400001/cc/animation/keyframed_...
File cc/animation/keyframed_animation_curve.cc (right):

https://codereview.chromium.org/642983003/diff/400001/cc/animation/keyframed_...
cc/animation/keyframed_animation_curve.cc:383:
fmaxf(std::abs(target_scale_for_segment.x()),
On 2014/10/10 03:31:23, vmpstr wrote:
> hehe, why fmaxf then? :P Also, should this be std::fmaxf?

Its not in std::  it actually is a co.piler defined thing I think. Anyway I
noticed it was cheaper than std::max when I was playing with the geometry types
and in lining stuff. 

Oh and c++11 should be just fmax apparently
http://www.cplusplus.com/reference/cmath/fmax/

[vmpstr, 2014-10-10 07:26:24]

lgtm

https://codereview.chromium.org/642983003/diff/400001/cc/animation/keyframed_...
File cc/animation/keyframed_animation_curve.cc (right):

https://codereview.chromium.org/642983003/diff/400001/cc/animation/keyframed_...
cc/animation/keyframed_animation_curve.cc:383:
fmaxf(std::abs(target_scale_for_segment.x()),
On 2014/10/10 05:30:11, danakj wrote:
> On 2014/10/10 03:31:23, vmpstr wrote:
> > hehe, why fmaxf then? :P Also, should this be std::fmaxf?
> 
> Its not in std::  it actually is a co.piler defined thing I think. Anyway I
> noticed it was cheaper than std::max when I was playing with the geometry
types
> and in lining stuff. 
> 
> Oh and c++11 should be just fmax apparently
> http://www.cplusplus.com/reference/cmath/fmax/

If it's cheaper, then that's good enough. Right maybe I was confusing std::fmax
with c++11 stuff.

[ajuma, 2014-10-10 13:43:07]

The intuition behind this approach is that we already have the content rastered
at the current scale, so if we're scaling down we won't immediately blurrify.
But what if the thing being scaled down is newly-promoted? Wouldn't we then
immediately blurrify it, even if rastering it at maximum scale doesn't take up
too much memory?

https://codereview.chromium.org/642983003/diff/400001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller.cc (right):

https://codereview.chromium.org/642983003/diff/400001/cc/animation/layer_anim...
cc/animation/layer_animation_controller.cc:506:
animations_[i]->curve()->ToTransformAnimationCurve();
We also need to account for animations being run in reverse (which would be
those with Reverse or AlternateReverse direction and positive playback rate, and
those with Normal or Alternate direction and negative playback rate), since that
changes the targets to consider (the endpoint of the last keyframe is the real
starting point).

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

https://codereview.chromium.org/642983003/diff/400001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:1177: if (maximum_scale <
raster_contents_scale_) {
What if raster_contents_scale is huge, maximum_scale is huge-1, but the
animation also at some point has tiny scale? e.g. say the animation starts at
scale 100, then goes to scale 99, and then down to scale 1. One option would be
to still do the size check below, and end up using scale 1. The downside is that
if the animation is really just scaling down from 100 to 99, using scale 1 for
newly-rastered tiles isn't that great.

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

https://codereview.chromium.org/642983003/diff/400001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:2319: // high-res tiling should not be
created at a source scale of 1.
s/not// ?

[danakj, 2014-10-10 14:26:00]

On Fri, Oct 10, 2014 at 9:43 AM, <ajuma@chromium.org> wrote:

> The intuition behind this approach is that we already have the content
> rastered
> at the current scale, so if we're scaling down we won't immediately
> blurrify.
> But what if the thing being scaled down is newly-promoted? Wouldn't we then
> immediately blurrify it, even if rastering it at maximum scale doesn't
> take up
> too much memory?
>

Yes we would, but to do otherwise is to raster a bunch of things to throw
them away in a few seconds. I don't think it's worth it? Do you have a test
case in mind we could look at it with this patch?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ajuma, 2014-10-10 14:40:29]

On 2014/10/10 14:26:00, danakj wrote:
> On Fri, Oct 10, 2014 at 9:43 AM, <mailto:ajuma@chromium.org> wrote:
> 
> > The intuition behind this approach is that we already have the content
> > rastered
> > at the current scale, so if we're scaling down we won't immediately
> > blurrify.
> > But what if the thing being scaled down is newly-promoted? Wouldn't we then
> > immediately blurrify it, even if rastering it at maximum scale doesn't
> > take up
> > too much memory?
> >
> 
> Yes we would, but to do otherwise is to raster a bunch of things to throw
> them away in a few seconds. I don't think it's worth it? Do you have a test
> case in mind we could look at it with this patch?

Here's a test case where we promote right at the start of a scale-down:
http://jsfiddle.net/avSqL/17/show/

It's true that to keep this sharp, we need to raster tiles only to throw them
away a few seconds later, but it seems worth it (within reason memory-wise).

[danakj, 2014-10-10 15:07:10]

On Fri, Oct 10, 2014 at 10:40 AM, <ajuma@chromium.org> wrote:

> On 2014/10/10 14:26:00, danakj wrote:
>
>  On Fri, Oct 10, 2014 at 9:43 AM, <mailto:ajuma@chromium.org> wrote:
>>
>
>  > The intuition behind this approach is that we already have the content
>> > rastered
>> > at the current scale, so if we're scaling down we won't immediately
>> > blurrify.
>> > But what if the thing being scaled down is newly-promoted? Wouldn't we
>> then
>> > immediately blurrify it, even if rastering it at maximum scale doesn't
>> > take up
>> > too much memory?
>> >
>>
>
>  Yes we would, but to do otherwise is to raster a bunch of things to throw
>> them away in a few seconds. I don't think it's worth it? Do you have a
>> test
>> case in mind we could look at it with this patch?
>>
>
> Here's a test case where we promote right at the start of a scale-down:
> http://jsfiddle.net/avSqL/17/show/
>
> It's true that to keep this sharp, we need to raster tiles only to throw
> them
> away a few seconds later, but it seems worth it (within reason
> memory-wise).
>

OK I'm adding a TODO about a starting_animation_contents_scale as discussed.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-10-10 15:14:46]

PTAL

https://codereview.chromium.org/642983003/diff/400001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller.cc (right):

https://codereview.chromium.org/642983003/diff/400001/cc/animation/layer_anim...
cc/animation/layer_animation_controller.cc:506:
animations_[i]->curve()->ToTransformAnimationCurve();
On 2014/10/10 13:43:07, ajuma wrote:
> We also need to account for animations being run in reverse (which would be
> those with Reverse or AlternateReverse direction and positive playback rate,
and
> those with Normal or Alternate direction and negative playback rate), since
that
> changes the targets to consider (the endpoint of the last keyframe is the real
> starting point).

OK I think I have this now, thanks!

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

https://codereview.chromium.org/642983003/diff/400001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:1177: if (maximum_scale <
raster_contents_scale_) {
On 2014/10/10 13:43:07, ajuma wrote:
> What if raster_contents_scale is huge, maximum_scale is huge-1, but the
> animation also at some point has tiny scale? e.g. say the animation starts at
> scale 100, then goes to scale 99, and then down to scale 1. One option would
be
> to still do the size check below, and end up using scale 1. The downside is
that
> if the animation is really just scaling down from 100 to 99, using scale 1 for
> newly-rastered tiles isn't that great.

Ya.. that's a good point. Uhhh. This function really wants to know every scale
in the animation.. heh.

The upside to using 1 for new tiles is that from a scale of 100 to 99 there
won't be many (or any? prepainting?) new tiles really.. so I should probably do
that. Changed with tests.

[danakj, 2014-10-10 15:15:52]

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

https://codereview.chromium.org/642983003/diff/710001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:1194: raster_contents_scale_ = 1.f *
ideal_page_scale_ * ideal_device_scale_;
I do find this else case a little odd, and wonder if we still need it after
this, or can remove it and just use the default scale choice ignoring the
animation if the max is too large.

[ajuma, 2014-10-10 15:43:04]

Thanks, lgtm with a couple comments.

https://codereview.chromium.org/642983003/diff/710001/cc/resources/picture_pi...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/642983003/diff/710001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:169: TRACE_EVENT0("cc",
"PicturePile::UpdateAndExpandInvalidation");
Did you mean to add these trace events, or is this left-over debugging code?

https://codereview.chromium.org/642983003/diff/710001/ui/compositor/transform...
File ui/compositor/transform_animation_curve_adapter.h (right):

https://codereview.chromium.org/642983003/diff/710001/ui/compositor/transform...
ui/compositor/transform_animation_curve_adapter.h:35: virtual bool
MaximumTargetScale(float* max_scale) const override;
This needs to a |forward_direction| argument too. (Same below.)

[danakj, 2014-10-10 16:07:15]

https://codereview.chromium.org/642983003/diff/710001/cc/resources/picture_pi...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/642983003/diff/710001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:169: TRACE_EVENT0("cc",
"PicturePile::UpdateAndExpandInvalidation");
On 2014/10/10 15:43:03, ajuma wrote:
> Did you mean to add these trace events, or is this left-over debugging code?

Oh thanks that's from debugging the next bug, they slipped in overnight.

[danakj, 2014-10-10 16:09:59]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-10-10 16:10:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/642983003/820001

[commit-bot: I haz the power, 2014-10-10 17:18:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-10 17:18:19]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[danakj, 2014-10-10 18:34:58]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-10-10 18:35:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/642983003/1030001

[commit-bot: I haz the power, 2014-10-10 19:35:14]

Committed patchset #7 (id:1030001)

[commit-bot: I haz the power, 2014-10-10 19:35:55]

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