Android: Decouple animate from vsync

Android: Decouple animate from vsync

Add animation callback that happens before Composite.
SetNeedsAnimate() schedules animations with a deadline.

New frames arriving from the renderer will still composite immediately,
in which case they also take care of the animation update.

Remove |should_composite_on_vsync_| logic that was used for tab stack
animations and also use the same deadline logic instead.

BUG=373075
NOTRY=True

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

[no sievers, 2014-05-16 01:59:55]

ptal

[brianderson, 2014-05-21 17:22:59]

https://codereview.chromium.org/285373008/diff/40001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/285373008/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:275: if
(will_composite_eventually_)
Can we also return early here if will_composite_immediately_ is true?

https://codereview.chromium.org/285373008/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:299:
base::MessageLoop::current()->PostDelayedTask(
If will_composite_immediately_ just became true and will_composite_eventually_
was previously true, do we need to cancel the previously posted composite?

https://codereview.chromium.org/285373008/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:311:
will_composite_eventually_ &= trigger != COMPOSITE_EVENTUALLY;
It is hard to decipher the intention here. Is this equivalent?

if (trigger == COMPOSITE_IMMEDIATELY)
  will_composite_immediately_ = false;
if (trigger == COMPOSITE_EVENTUALLY)
  will_composite_eventually_ = false;

https://codereview.chromium.org/285373008/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:326:
PostComposite(COMPOSITE_EVENTUALLY);
Instead of posting a composite, would it be better to set a flag that
automatically triggers a composite when we get the swap ack?

https://codereview.chromium.org/285373008/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:667:
did_composite_this_frame_ = false;
If did_composite_this_frame_ is true at this point due to an "eventually
composite" and there is an "immediate composite" pending, can we trigger the
immediate composite right here?

[no sievers, 2014-05-23 02:49:44]

Comments addressed. I redid it a bit to only ever have one pending composite
task at any given time and also not try to schedule across vsyncs but instead
reschedule on vsync should it be necessary.

https://codereview.chromium.org/285373008/diff/40001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/285373008/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:275: if
(will_composite_eventually_)
On 2014/05/21 17:23:00, brianderson wrote:
> Can we also return early here if will_composite_immediately_ is true?

Good catch. Done.

https://codereview.chromium.org/285373008/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:299:
base::MessageLoop::current()->PostDelayedTask(
On 2014/05/21 17:23:00, brianderson wrote:
> If will_composite_immediately_ just became true and will_composite_eventually_
> was previously true, do we need to cancel the previously posted composite?

I think the early-out in Composite() (!needs_composite_) mostly takes care of
that. Also we should never composite twice per frame. 

It's actually pretty bad and noticeable in traces when we over-composite,
because we still have to do a lot of work in the compositor until we notice
there is no visible damage. I saw it initially with my patch when I had the
skip_frame logic and timing off in the code above.

https://codereview.chromium.org/285373008/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:311:
will_composite_eventually_ &= trigger != COMPOSITE_EVENTUALLY;
On 2014/05/21 17:23:00, brianderson wrote:
> It is hard to decipher the intention here. Is this equivalent?
> 
> if (trigger == COMPOSITE_IMMEDIATELY)
>   will_composite_immediately_ = false;
> if (trigger == COMPOSITE_EVENTUALLY)
>   will_composite_eventually_ = false;

Done. You're right, it's equivalent and more readable.

https://codereview.chromium.org/285373008/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:326:
PostComposite(COMPOSITE_EVENTUALLY);
On 2014/05/21 17:23:00, brianderson wrote:
> Instead of posting a composite, would it be better to set a flag that
> automatically triggers a composite when we get the swap ack?

Ok since Sami suggested this earlier too, will do :)

https://codereview.chromium.org/285373008/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:667:
did_composite_this_frame_ = false;
On 2014/05/21 17:23:00, brianderson wrote:
> If did_composite_this_frame_ is true at this point due to an "eventually
> composite" and there is an "immediate composite" pending, can we trigger the
> immediate composite right here?

You mean for when there is a ScheduleComposite(), let's say late frame from the
renderer, happening after a previous deadline where we composited? Originally I
thought it's maybe fine to wait until the next deadline since we cannot
composite twice per vsync anyways. But I guess it might be better to unblock the
renderer sooner, rather than later.

[no sievers, 2014-05-23 18:27:21]

https://codereview.chromium.org/285373008/diff/60001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/285373008/diff/60001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:300: // vsync.
You think we should do this for COMPOSITE_IMMEDIATELY also until we can throttle
the renderer better?

[brianderson, 2014-05-23 22:34:25]

https://codereview.chromium.org/285373008/diff/60001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/285373008/diff/60001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:282:
DCHECK(WillComposite());
The value of WillComposite() will only change if composite_on_vsync_trigger !=
COMPOSITE_IMMEDIATELY. Should the if statement above be a DCHECK instead?

https://codereview.chromium.org/285373008/diff/60001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:300: // vsync.
On 2014/05/23 18:27:22, sievers wrote:
> You think we should do this for COMPOSITE_IMMEDIATELY also until we can
throttle
> the renderer better?

No, I think we should just composite the Renderer swap ASAP  on Android if the
Browser hasn't already composited this frame. If there are perf issues, we
should fix them with better throttling or gpu scheduling.

[no sievers, 2014-05-27 18:20:44]

https://codereview.chromium.org/285373008/diff/60001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/285373008/diff/60001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:282:
DCHECK(WillComposite());
On 2014/05/23 22:34:25, brianderson wrote:
> The value of WillComposite() will only change if composite_on_vsync_trigger !=
> COMPOSITE_IMMEDIATELY. Should the if statement above be a DCHECK instead?

Thanks, I think line 277 is actually wrong because what it's supposed to do is
schedule on vsync with a request to schedule immediately taking priority.
However 'if (!WillComposite)' breaks that, so I removed that check.

[Sami, 2014-05-27 19:54:04]

https://codereview.chromium.org/285373008/diff/120001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/285373008/diff/120001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:355:
root_window_->Animate(gfx::FrameTime::Now());
Should we use the same gfx::FrameTime::Now() for animating and compositing
below?

https://codereview.chromium.org/285373008/diff/120001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.h (right):

https://codereview.chromium.org/285373008/diff/120001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.h:117: bool
WillCompositeThisFrame() {
nit: These three methods could be const.

https://codereview.chromium.org/285373008/diff/120001/ui/base/android/window_...
File ui/base/android/window_android.h (right):

https://codereview.chromium.org/285373008/diff/120001/ui/base/android/window_...
ui/base/android/window_android.h:56: void Animate(base::TimeTicks
frame_begin_time);
nit: begin_frame_time would be more consistent with existing naming.

[no sievers, 2014-05-27 20:05:16]

https://codereview.chromium.org/285373008/diff/120001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/285373008/diff/120001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:355:
root_window_->Animate(gfx::FrameTime::Now());
On 2014/05/27 19:54:05, Sami wrote:
> Should we use the same gfx::FrameTime::Now() for animating and compositing
> below?

Done.

https://codereview.chromium.org/285373008/diff/120001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.h (right):

https://codereview.chromium.org/285373008/diff/120001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.h:117: bool
WillCompositeThisFrame() {
On 2014/05/27 19:54:05, Sami wrote:
> nit: These three methods could be const.

Done.

https://codereview.chromium.org/285373008/diff/120001/ui/base/android/window_...
File ui/base/android/window_android.h (right):

https://codereview.chromium.org/285373008/diff/120001/ui/base/android/window_...
ui/base/android/window_android.h:56: void Animate(base::TimeTicks
frame_begin_time);
On 2014/05/27 19:54:05, Sami wrote:
> nit: begin_frame_time would be more consistent with existing naming.

Done.

[brianderson, 2014-05-27 21:00:45]

lgtm

[no sievers, 2014-05-27 21:08:16]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2014-05-27 21:09:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sievers@chromium.org/285373008/110009

[no sievers, 2014-05-27 21:19:56]

+Ted for ui/base/android OWNERS

[Ted C, 2014-05-27 21:33:19]

lgtm - owners ui/base/android

https://codereview.chromium.org/285373008/diff/110009/ui/base/android/window_...
File ui/base/android/window_android_observer.h (right):

https://codereview.chromium.org/285373008/diff/110009/ui/base/android/window_...
ui/base/android/window_android_observer.h:20: virtual void
OnAnimate(base::TimeTicks frame_begin_time) {}
do you ever want to make this pure virtual?  Just wondering if you want a TODO
to make sure you do that after the downstream roll.

[no sievers, 2014-05-27 21:35:04]

On 2014/05/27 21:33:19, Ted C wrote:
> lgtm - owners ui/base/android
> 
>
https://codereview.chromium.org/285373008/diff/110009/ui/base/android/window_...
> File ui/base/android/window_android_observer.h (right):
> 
>
https://codereview.chromium.org/285373008/diff/110009/ui/base/android/window_...
> ui/base/android/window_android_observer.h:20: virtual void
> OnAnimate(base::TimeTicks frame_begin_time) {}
> do you ever want to make this pure virtual?  Just wondering if you want a TODO
> to make sure you do that after the downstream roll.

I'm not sure, since it's kind of optional to be doing animations. Then again,
RWHVAndroid is really the only WindowAndroidObserver and I'm currently not sure
what else would ever implement it.

[no sievers, 2014-05-27 22:41:16]

The CQ bit was unchecked by sievers@chromium.org

[no sievers, 2014-05-27 22:41:17]

The CQ bit was checked by sievers@chromium.org

[no sievers, 2014-05-27 22:41:20]

The CQ bit was unchecked by sievers@chromium.org

[no sievers, 2014-05-27 22:41:29]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2014-05-27 22:43:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sievers@chromium.org/285373008/110009

[commit-bot: I haz the power, 2014-05-27 22:45:42]

Change committed as 273062