Thread ui transform animations

Thread ui transform animations

This threads ui transform animations, dispatching
them to cc as we now already do for ui opacity
animations.

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

[ajuma, 2013-02-15 18:03:24]

This depends on https://codereview.chromium.org/11896017

[Ian Vollick, 2013-02-16 04:35:35]

On 2013/02/15 18:03:24, ajuma wrote:
> This depends on https://codereview.chromium.org/11896017

ui/compositor lgtm! 

+sky for ui/views

It's a bit of a bummer that we're copying matrices so much more, but I can't see
a nice way around it. We have to be able to get roughly accurate transforms
mid-animation so we have to read them from the cc::layer and those transforms
are in a different space. I suppose we could cache both the 'ui' and 'cc'
transforms in ui::Layer::transform() and return the cached ui transform if the
cached cc transform matches the one we read from the cc::Layer, but this seems
pretty gross.

[danakj, 2013-02-16 04:49:51]

https://codereview.chromium.org/12226080/diff/10001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/12226080/diff/10001/ui/compositor/layer.cc#ne...
ui/compositor/layer.cc:419: scale_translate.matrix().set3x3(device_scale_factor,
0, 0,
Why do you use temporary gfx::Transforms for scaling and translating? Why not
just use Scale() and Translate()?

https://codereview.chromium.org/12226080/diff/10001/ui/compositor/layer.cc#ne...
ui/compositor/layer.cc:823: inverse_scale.matrix().set3x3(1.0f /
device_scale_factor_, 0, 0,
Here you can use Scale() and Translate() as well instead of temp matrices?

[ajuma, 2013-02-19 16:12:19]

https://chromiumcodereview.appspot.com/12226080/diff/10001/ui/compositor/laye...
File ui/compositor/layer.cc (right):

https://chromiumcodereview.appspot.com/12226080/diff/10001/ui/compositor/laye...
ui/compositor/layer.cc:419: scale_translate.matrix().set3x3(device_scale_factor,
0, 0,
On 2013/02/16 04:49:51, danakj wrote:
> Why do you use temporary gfx::Transforms for scaling and translating? Why not
> just use Scale() and Translate()?

Fixed now.

https://chromiumcodereview.appspot.com/12226080/diff/10001/ui/compositor/laye...
ui/compositor/layer.cc:823: inverse_scale.matrix().set3x3(1.0f /
device_scale_factor_, 0, 0,
On 2013/02/16 04:49:51, danakj wrote:
> Here you can use Scale() and Translate() as well instead of temp matrices?

Done.

[danakj, 2013-02-19 16:19:31]

Thanks, like this better :)

https://chromiumcodereview.appspot.com/12226080/diff/16001/ui/compositor/laye...
File ui/compositor/layer.cc (right):

https://chromiumcodereview.appspot.com/12226080/diff/16001/ui/compositor/laye...
ui/compositor/layer.cc:814: transform.Translate(-bounds_.x(), -bounds_.y());
This ordering is different from the inverse case. There the bounds() and
transform() are applied without the device scale. Here one is with and one
without. Is that intentional?

[ajuma, 2013-02-19 16:35:47]

On 2013/02/19 16:19:31, danakj wrote:
> Thanks, like this better :)
> 
>
https://chromiumcodereview.appspot.com/12226080/diff/16001/ui/compositor/laye...
> File ui/compositor/layer.cc (right):
> 
>
https://chromiumcodereview.appspot.com/12226080/diff/16001/ui/compositor/laye...
> ui/compositor/layer.cc:814: transform.Translate(-bounds_.x(), -bounds_.y());
> This ordering is different from the inverse case. There the bounds() and
> transform() are applied without the device scale. Here one is with and one
> without. Is that intentional?

Yes. It looked weird to me at first too, but here's how it all works out:

Let D be the device scale factor scaling matrix. Let B be the bounds translation
matrix. Let T be the ui transform. Let C be the cc transform.

In Layer::ConvertTransformToCCTransform, we compute:
C = D * B * T * D^{-1}

So solving for T, we get:
T = B^{-1} * D^{-1} * C * D

and this is what Layer::transform computes.

[sky, 2013-02-19 22:15:12]

What do you need me to review here? It also looks like you've pulled in a bunch
of your other patch into this one.

[ajuma, 2013-02-19 22:19:44]

On 2013/02/19 22:15:12, sky wrote:
> What do you need me to review here? 

ui/views

> It also looks like you've pulled in a bunch
> of your other patch into this one.

This is based on top of the other one; the other one has to land first.

[sky, 2013-02-19 23:13:36]

ui/views LGTM. git allows uploading relative to another patch.

[ajuma, 2013-02-20 00:09:20]

On 2013/02/19 23:13:36, sky wrote:
> git allows uploading relative to another patch.

That's how I've uploaded this patch, so unless something went wrong (did it?)
this should be relative to the other patch.

[sky, 2013-02-20 00:42:05]

My mistake, I thought I saw parts of the other patch in
layer_animation_element.

  -Scott

On Tue, Feb 19, 2013 at 4:09 PM,  <ajuma@chromium.org> wrote:
> On 2013/02/19 23:13:36, sky wrote:
>>
>> git allows uploading relative to another patch.
>
>
> That's how I've uploaded this patch, so unless something went wrong (did
> it?)
> this should be relative to the other patch.
>
> https://chromiumcodereview.appspot.com/12226080/

[ajuma, 2013-02-25 22:05:47]

Patch rebased onto trunk (now that the patch to thread opacity has landed) and
updated with the following changes:

1) ThreadedTransformTransition::OnAbort now takes tween_type into account (this
is analogous to https://codereview.chromium.org/12310115).

2) LayerAnimationTestController::StartThreadedAnimationsIfNeeded now also starts
transform animations.

[Ian Vollick, 2013-02-27 17:07:42]

On 2013/02/25 22:05:47, ajuma wrote:
> Patch rebased onto trunk (now that the patch to thread opacity has landed) and
> updated with the following changes:
> 
> 1) ThreadedTransformTransition::OnAbort now takes tween_type into account
(this
> is analogous to https://codereview.chromium.org/12310115).
> 
> 2) LayerAnimationTestController::StartThreadedAnimationsIfNeeded now also
starts
> transform animations.

The new changes lgtm.

[commit-bot: I haz the power, 2013-02-27 19:01:58]

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

[commit-bot: I haz the power, 2013-02-27 21:06:14]

Retried try job too often on linux_aura for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[ajuma, 2013-03-01 19:31:42]

WebContentsViewAuraTest.QuickOverscrollDirectionChange was failing because it
was taking too long to finish.

This test produces a sequence of touch events; after each such event, it runs
the MessageLoop until the MessageLoop is idle. Each touch event triggers a
transform animation that has duration 200ms.

With non-threaded transform animations, the MessageLoop becomes idle almost
immediately after the animation is scheduled; at this point, the test continues
on, triggering the next animation, and this cancels the previous animation. In
other words, almost none of the triggered animations ends up taking 200ms.

With animations dispatched to cc (the test doesn't use a compositor thread, so
the animations aren't actually "threaded"), the MessageLoop doesn't become idle
until each animation completes: while there are animations in progress,
cc::LayerTreeHostImpl::animateLayers calls
cc::SingleThreadProxy::setNeedsCommitOnImplThread, which ultimately results in a
call to aura::RootWindow::ScheduleDraw, which posts a task to the MessageLoop a
call to RootWindow::Draw. But RootWindow::Draw ultimately results in a call to
SingleThreadProxy::doComposite, which calls LayerTreeHostImpl::animateLayers.

Since each animation takes 200ms to complete, and there are over 200 such
animations, the test times out.

To allow the message loop to become idle (and hence allow the test to continue)
without having to wait for each animation to finish, I've added a method
DelayDrawsForTesting to RootWindow, which allows specifying a delay to be used
when posting a task in RootWindow::ScheduleDraw, and I've a call to this method
in WebContentsViewAuraTest.QuickOverscrollDirectionChange.

[Ian Vollick, 2013-03-04 13:50:50]

Your approach sounds reasonable to me, but I have no authority to r+ a patch in
aura/

sky@, WDYT?

https://codereview.chromium.org/12226080/diff/49001/content/browser/web_conte...
File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right):

https://codereview.chromium.org/12226080/diff/49001/content/browser/web_conte...
content/browser/web_contents/web_contents_view_aura_browsertest.cc:233:
root_window->DelayDrawsForTesting(base::TimeDelta::FromMilliseconds(10));
SetDrawDelayForTesting, perhaps? DelayDrawsForTesting sounds like it wants a
boolean parameter, if any; passing a time delta reads a little funny.

[sky, 2013-03-04 14:48:56]

I would rather see the test add an observe that tracks when the animation
completes and then moves to the next part of the test.

[ajuma, 2013-03-04 15:14:21]

On 2013/03/04 14:48:56, sky wrote:
> I would rather see the test add an observe that tracks when the animation
> completes and then moves to the next part of the test.

Waiting till the animation completes will still leave us with the problem of the
test timing out (since 200+ animations each taking 200ms will exceed the 30s
time limit for unit tests). One approach for overcoming this would be to reduce
the duration of the animations for this test. We can do this be generalizing the
current LayerAnimator::set_slow_animation_scale_factor (which lets us increase
the duration of animations for tests) to a method that lets us increase or
decrease the duration of animations for tests.

[sky, 2013-03-04 18:05:34]

We should make the animations instantaneous in tests too.

  -Scott

On Mon, Mar 4, 2013 at 7:14 AM,  <ajuma@chromium.org> wrote:
> On 2013/03/04 14:48:56, sky wrote:
>>
>> I would rather see the test add an observe that tracks when the animation
>> completes and then moves to the next part of the test.
>
>
> Waiting till the animation completes will still leave us with the problem of
> the
> test timing out (since 200+ animations each taking 200ms will exceed the 30s
> time limit for unit tests). One approach for overcoming this would be to
> reduce
> the duration of the animations for this test. We can do this be generalizing
> the
> current LayerAnimator::set_slow_animation_scale_factor (which lets us
> increase
> the duration of animations for tests) to a method that lets us increase or
> decrease the duration of animations for tests.
>
> https://codereview.chromium.org/12226080/

[ajuma, 2013-03-04 18:52:40]

On 2013/03/04 18:05:34, sky wrote:
> We should make the animations instantaneous in tests too.

Making them fast-but-not-instantaneous would make the test more realistic though
(in terms of the code paths that are exercised), particularly since the test in
question is testing code that depends on whether an animation is in progress (a
state that would never occur if animations are instantaneous).

[sky, 2013-03-04 22:08:04]

Yes, that is what I was hoping for.

On Mon, Mar 4, 2013 at 10:52 AM,  <ajuma@chromium.org> wrote:
> On 2013/03/04 18:05:34, sky wrote:
>>
>> We should make the animations instantaneous in tests too.
>
>
> Making them fast-but-not-instantaneous would make the test more realistic
> though
> (in terms of the code paths that are exercised), particularly since the test
> in
> question is testing code that depends on whether an animation is in progress
> (a
> state that would never occur if animations are instantaneous).
>
> https://codereview.chromium.org/12226080/

[ajuma, 2013-03-13 18:21:24]

sky, PTAL: WebContentsViewAuraTest.QuickOverscrollDirectionChange
now uses ScopedAnimatonDurationScaleMode to speed up the test.

[sky, 2013-03-13 20:51:58]

nice LGTM

[commit-bot: I haz the power, 2013-03-13 21:33:23]

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

[commit-bot: I haz the power, 2013-03-13 23:06:18]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2013-03-13 23:44:04]

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

[commit-bot: I haz the power, 2013-03-14 07:09:20]

Change committed as 188007