Android: Move scheduling logic to CompositorImpl

Android: Move scheduling logic to CompositorImpl

This adds two ways of scheduling:

- implicitly from LTHClient::ScheduleComposite():
This will post a task to composite immediately.

- explicitly (for Chrome) through SetNeedsComposite() where
there are no layer tree changes, but instead a Composite()
will be scheduled for vsync with the Layout() callback giving
the app a chance to update the tree.

TBR=skyostil@chromium.org
NOTRY=True

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

[no sievers, 2014-04-16 02:17:27]

Sami, I haven't properly tested this at all yet.

But I have naively simplified the scheduling logic as I moved it from
ContentViewRenderView to CompositorImpl.

Maybe you can skim over it and see what subtle things I have broken already :)

https://codereview.chromium.org/239963002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/239963002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:543:
base::MessageLoop::current()->PostTask(
It definitely loses the 'post to front of queue' behavior you had added here for
an issue. Is it still necessary?
If so we need to figure out how we handle this with the cc scheduler.

[Sami, 2014-04-16 15:16:30]

Much needed cleanup, thanks!

https://codereview.chromium.org/239963002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/239963002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:538: // We already
scheduled something.
The consumePendingRendererFrame() logic made sure this only took the fast path
if there was a new frame to composite. IIRC I had to add it because something in
the browser (loading spinner, tab switcher, dunno) was often scheduling
compositing and kicking the renderer into high latency mode. It would be good to
double check this doesn't happen with this patch.

https://codereview.chromium.org/239963002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:543:
base::MessageLoop::current()->PostTask(
On 2014/04/16 02:17:28, sievers wrote:
> It definitely loses the 'post to front of queue' behavior you had added here
for
> an issue. Is it still necessary?
> If so we need to figure out how we handle this with the cc scheduler.

I *think* it's still an issue (here's the original description for reference:
https://gerrit-int.chromium.org/43697). It should be fairly obvious from a trace
when it happens. Basically the browser compositor for frame N is delayed by
input for frame N+1. Maybe we could throttle the input to avoid that situation.

https://codereview.chromium.org/239963002/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(left):

https://codereview.chromium.org/239963002/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:251:
if (mContentViewCore == null || !mContentViewCore.isReady()) {
I think we still need this isReady() check in one form or another to avoid a
black flash when starting up.

[Sami, 2014-04-16 17:53:26]

Stating the obvious: would be really nice if we figured out a way to test at
least some of these corner cases :P

[no sievers, 2014-05-09 00:30:05]

Hey Sami, updated patch. PTAL.

I've made subtle changes to the throttling and swap limit logic, esp. since it's
more obvious now that there are potentially two mechanisms fighting with each
other (schedule on vsync or immediately).
In practice, I don't see any conflicts during most scrolls, or the other case,
tab stack animations.

I'm sure though that I'll be coming back to fine tune some of this :)  At the
same time, I don't want to spend too much effort depending on when we'll just be
able to switch to the cc scheduler instead.

https://codereview.chromium.org/239963002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/239963002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:543:
base::MessageLoop::current()->PostTask(
On 2014/04/16 15:16:30, Sami wrote:
> On 2014/04/16 02:17:28, sievers wrote:
> > It definitely loses the 'post to front of queue' behavior you had added here
> for
> > an issue. Is it still necessary?
> > If so we need to figure out how we handle this with the cc scheduler.
> 
> I *think* it's still an issue (here's the original description for reference:
> https://gerrit-int.chromium.org/43697). It should be fairly obvious from a
trace
> when it happens. Basically the browser compositor for frame N is delayed by
> input for frame N+1. Maybe we could throttle the input to avoid that
situation.

Do you mind if I follow up on this separately? Handling this the other way
around by deferring input or so sounds good.

https://codereview.chromium.org/239963002/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(left):

https://codereview.chromium.org/239963002/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:251:
if (mContentViewCore == null || !mContentViewCore.isReady()) {
On 2014/04/16 15:16:30, Sami wrote:
> I think we still need this isReady() check in one form or another to avoid a
> black flash when starting up.

Should be taken care of by crrev.com/268530 now.

[Sami, 2014-05-09 13:21:13]

On 2014/05/09 00:30:05, sievers wrote:
> Hey Sami, updated patch. PTAL.
>
>
> I've made subtle changes to the throttling and swap limit logic, esp. since
it's
> more obvious now that there are potentially two mechanisms fighting with each
> other (schedule on vsync or immediately).
> In practice, I don't see any conflicts during most scrolls, or the other case,
> tab stack animations.
> 
> I'm sure though that I'll be coming back to fine tune some of this :)  At the
> same time, I don't want to spend too much effort depending on when we'll just
be
> able to switch to the cc scheduler instead.

I'm also wondering whether we should just bite the bullet and use real deadline
scheduling here. Not really sure how much work that would be, but it still might
be easier than trying to come up with an alternative here. Then again, in my
mind the biggest risk of regression is somehow making the renderer run in high
latency mode in all cases, so if we make sure to cover that then we could
probably live with an intermediate step like this.

>
https://codereview.chromium.org/239963002/diff/1/content/browser/renderer_hos...
> File content/browser/renderer_host/compositor_impl_android.cc (right):
> 
>
https://codereview.chromium.org/239963002/diff/1/content/browser/renderer_hos...
> content/browser/renderer_host/compositor_impl_android.cc:543:
> base::MessageLoop::current()->PostTask(
> On 2014/04/16 15:16:30, Sami wrote:
> > On 2014/04/16 02:17:28, sievers wrote:
> > > It definitely loses the 'post to front of queue' behavior you had added
here
> > for
> > > an issue. Is it still necessary?
> > > If so we need to figure out how we handle this with the cc scheduler.
> > 
> > I *think* it's still an issue (here's the original description for
reference:
> > https://gerrit-int.chromium.org/43697). It should be fairly obvious from a
> trace
> > when it happens. Basically the browser compositor for frame N is delayed by
> > input for frame N+1. Maybe we could throttle the input to avoid that
> situation.
> 
> Do you mind if I follow up on this separately? Handling this the other way
> around by deferring input or so sounds good.

Sure, we can fix that as a follow up, as long as there's a bug to track :)

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

https://codereview.chromium.org/239963002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:263:
base::MessageLoop::current()->PostDelayedTask(
Is there a chance we could post this task several times?

https://codereview.chromium.org/239963002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:282: // Don't Composite
immediately more than once in between vsync ticks.
Maybe add something like this as a safeguard: DCHECK(!(is_vsync &&
did_composite_outside_vsync));

https://codereview.chromium.org/239963002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:285:
PostComposite(vsync_period_);
Just thinking out loud here: we generally composite immediately if there's a new
frame from the renderer. We can't tell in advance when that happens. If that
happens twice per frame, then it seems like there's something wrong with
BeginFrame scheduling and we should be hitting this case regularly.

https://codereview.chromium.org/239963002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:296:
PostComposite(vsync_period_);
Should we instead trigger the composite as soon as the swap limit clears?
Otherwise this can add some latency if the GPU is backed up.

https://codereview.chromium.org/239963002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:311:
host_->Composite(gfx::FrameTime::Now());
This should probably be derived from the most recent vsync time we have.

https://codereview.chromium.org/239963002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:448:
should_composite_on_vsync_ = true;
Doesn't this have the effect of re-scheduling any pending (immediate) composite
operation to the next vsync? Not sure if that happens in practice, but maybe we
should early out here if needs_composite_ == true.

https://codereview.chromium.org/239963002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:548: // to stop calling
it ourselves in Composite() below.
Composite() is above, not below :)

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

https://codereview.chromium.org/239963002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.h:107: void Composite(bool
is_vsync);
Mind making this an enum -- CompositingTrigger or something? I'm allergic to
opaque bool parameters :)

https://codereview.chromium.org/239963002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.h:141: // When
SetNeedsComposite() is getting called, we will try to schedule
This explanation should probably go next to SetNeedsComposite. ScheduleComposite
could also use a comment about what the difference between the two is.

https://codereview.chromium.org/239963002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.h:147: bool
did_composite_outside_vsync_;
Would something did_composite_during_this_frame_ be simpler? No matter why we
composite, we only want to do it once per frame.

https://codereview.chromium.org/239963002/diff/40001/content/public/browser/a...
File content/public/browser/android/compositor_client.h (right):

https://codereview.chromium.org/239963002/diff/40001/content/public/browser/a...
content/public/browser/android/compositor_client.h:18: virtual void
OnSwapBuffersCompleted(int pending_swap_buffers) {}
Does anyone need the parameter here?

[no sievers, 2014-05-09 22:53:44]

patch set updated. inline comments

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

https://codereview.chromium.org/239963002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:263:
base::MessageLoop::current()->PostDelayedTask(
On 2014/05/09 13:21:14, Sami wrote:
> Is there a chance we could post this task several times?

ScheduleComposite() has an early-out if needs_composite_ is already true.
So only in some more rare cases when we are throttling, we'd post multiple
tasks, but we should still not end up actually compositing too often.

https://codereview.chromium.org/239963002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:282: // Don't Composite
immediately more than once in between vsync ticks.
On 2014/05/09 13:21:14, Sami wrote:
> Maybe add something like this as a safeguard: DCHECK(!(is_vsync &&
> did_composite_outside_vsync));

Actually obsolete now with your other comment addressed
(did_composite_outside_vsync_ -> did_composite_this_frame_).
Now both can happen when we transition from one way of scheduling to the other.

https://codereview.chromium.org/239963002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:285:
PostComposite(vsync_period_);
On 2014/05/09 13:21:14, Sami wrote:
> Just thinking out loud here: we generally composite immediately if there's a
new
> frame from the renderer. We can't tell in advance when that happens. If that
> happens twice per frame, then it seems like there's something wrong with
> BeginFrame scheduling and we should be hitting this case regularly.

Yea I was wondering too and logged when it happens, but fortunately I don't see
it happen too often (probably glow effect and things can cause it).

https://codereview.chromium.org/239963002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:296:
PostComposite(vsync_period_);
On 2014/05/09 13:21:14, Sami wrote:
> Should we instead trigger the composite as soon as the swap limit clears?
> Otherwise this can add some latency if the GPU is backed up.

I was actually wondering if this is better for latency, i.e. to skip a frame
instead of keeping up as much backpressure as possible. wdyt?

Again, seems like with a deadline this would be way easier :/

https://codereview.chromium.org/239963002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:311:
host_->Composite(gfx::FrameTime::Now());
On 2014/05/09 13:21:14, Sami wrote:
> This should probably be derived from the most recent vsync time we have.

Hmm if we composite on vsync, now is the vsync time.
Otherwise would we really want to use vsync time? AURA seems to be using now()
also.

https://codereview.chromium.org/239963002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:448:
should_composite_on_vsync_ = true;
On 2014/05/09 13:21:14, Sami wrote:
> Doesn't this have the effect of re-scheduling any pending (immediate)
composite
> operation to the next vsync? Not sure if that happens in practice, but maybe
we
> should early out here if needs_composite_ == true.

Done.

https://codereview.chromium.org/239963002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:548: // to stop calling
it ourselves in Composite() below.
On 2014/05/09 13:21:14, Sami wrote:
> Composite() is above, not below :)

Done :)

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

https://codereview.chromium.org/239963002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.h:107: void Composite(bool
is_vsync);
On 2014/05/09 13:21:14, Sami wrote:
> Mind making this an enum -- CompositingTrigger or something? I'm allergic to
> opaque bool parameters :)

Done.

https://codereview.chromium.org/239963002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.h:141: // When
SetNeedsComposite() is getting called, we will try to schedule
On 2014/05/09 13:21:14, Sami wrote:
> This explanation should probably go next to SetNeedsComposite.
ScheduleComposite
> could also use a comment about what the difference between the two is.

ScheduleComposite is the LTHClient implementation. I've made the whole derived
implementation private and added comments inside both functions.

Compositor::SetNeedsComposite() has a general comment about when it should be
called in the public header.

https://codereview.chromium.org/239963002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.h:147: bool
did_composite_outside_vsync_;
On 2014/05/09 13:21:14, Sami wrote:
> Would something did_composite_during_this_frame_ be simpler? No matter why we
> composite, we only want to do it once per frame.

Done.

https://codereview.chromium.org/239963002/diff/40001/content/public/browser/a...
File content/public/browser/android/compositor_client.h (right):

https://codereview.chromium.org/239963002/diff/40001/content/public/browser/a...
content/public/browser/android/compositor_client.h:18: virtual void
OnSwapBuffersCompleted(int pending_swap_buffers) {}
On 2014/05/09 13:21:14, Sami wrote:
> Does anyone need the parameter here?

For some existing logic downstream that we will probably want to revisit
eventually.
Supposedly it has to do with rate limiting invalidations from native views while
the browser compositor is going.

[no sievers, 2014-05-09 23:25:56]

On 2014/05/09 13:21:13, Sami wrote:
> I'm also wondering whether we should just bite the bullet and use real
deadline
> scheduling here. Not really sure how much work that would be, but it still
might
> be easier than trying to come up with an alternative here. Then again, in my
> mind the biggest risk of regression is somehow making the renderer run in high
> latency mode in all cases, so if we make sure to cover that then we could
> probably live with an intermediate step like this.
> 

Ideally we can migrate to the cc scheduler + SingleThreadProxy + deadline
scheduling soon.
Also see https://codereview.chromium.org/134623005/

In the meantime we can probably get away with this. I'll also look again through
the downstream code and remove unneeded SetNeedsComposite() calls.
Ideally we'd be only scheduling one way or another depending on whether we are
in the tab stack or not. (with SetNeedsComposite() overriding the live tab to
vsync scheduling.)

[brianderson, 2014-05-10 00:24:39]

Sorry if my comments overlap with Sami's comments, I didn't get to look at the
review history in detail.

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

https://codereview.chromium.org/239963002/diff/80001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:290:
PostComposite(vsync_period_);
Why PostComposite a vsync period away here instead of just transitioning to
vsync scheduling?

https://codereview.chromium.org/239963002/diff/80001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:310: // Ignore
ScheduleComposite() from layer tree changes during Layout.
What happens if you don't ignore?

https://codereview.chromium.org/239963002/diff/80001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:389:
pending_swapbuffers_ = 0;
Do you not get pending acks when you go invisible?

https://codereview.chromium.org/239963002/diff/80001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:448: void
CompositorImpl::SetNeedsComposite() {
Is this the signal we get when the tab stack wants an update?

https://codereview.chromium.org/239963002/diff/80001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:609:
client_->OnSwapBuffersCompleted(pending_swapbuffers_--);
Can you decrement after the function call?

[no sievers, 2014-05-12 19:49:40]

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

https://codereview.chromium.org/239963002/diff/80001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:290:
PostComposite(vsync_period_);
On 2014/05/10 00:24:40, brianderson wrote:
> Why PostComposite a vsync period away here instead of just transitioning to
> vsync scheduling?

It would transition to vsync scheduling immediately if SetNeedsComposite() was
called (should_composite_on_vsync_).

The else is for when there was another tree modification, which should not
happen in normal operation. In that case I wanted to make sure that it does not
get lost, but we also don't unnecessarily change the phase.

https://codereview.chromium.org/239963002/diff/80001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:310: // Ignore
ScheduleComposite() from layer tree changes during Layout.
On 2014/05/10 00:24:40, brianderson wrote:
> What happens if you don't ignore?

It would post a task to Composite() unnecessarily.
It would also reset needs_composite_ to true.

I'm worried that it might composite unnecessarily for a while, as long as it
happens so that Chrome does end up touching one of the layers. On the other
hand, if the animation was really still going on it would force Composite()
through SetNeedsComposite().

https://codereview.chromium.org/239963002/diff/80001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:389:
pending_swapbuffers_ = 0;
On 2014/05/10 00:24:40, brianderson wrote:
> Do you not get pending acks when you go invisible?

Yes, I was running into accounting problems and it would stop updating after
becoming visible again.

I didn't fully investigate but am guessing that the callbacks can get dropped
when we synchronously tear down the LTH and context (and everything that belongs
to it like CommandBufferProxyImpl etc.) above.

We also had a reset logic in the previous code in ContentViewRenderView, but
interestingly only when the surface got recreated.

https://codereview.chromium.org/239963002/diff/80001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:448: void
CompositorImpl::SetNeedsComposite() {
On 2014/05/10 00:24:40, brianderson wrote:
> Is this the signal we get when the tab stack wants an update?

Yea exactly. It's needed because we don't get a call to ScheduleComposite()
because nothing gets changed in the layer tree.

It currently works the opposite way, more sort of like SetNeedsAnimate() with
the difference that it's not only changing properties on a layer but might
switch out entire layers (hence using Layout() callback before Composite()).

https://codereview.chromium.org/239963002/diff/80001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:609:
client_->OnSwapBuffersCompleted(pending_swapbuffers_--);
On 2014/05/10 00:24:40, brianderson wrote:
> Can you decrement after the function call?

Isn't that the same?

[brianderson, 2014-05-13 19:12:22]

lgtm

[Ted C, 2014-05-13 22:12:40]

lgtm - owners android

[Sami, 2014-05-13 22:31:19]

lgtm too, however:

> Hmm if we composite on vsync, now is the vsync time.
> Otherwise would we really want to use vsync time? AURA seems 
> to be using now()
> also.

Not exactly: now() will include random CPU jitter. Ideally we'd use the same
time we plug into BeginFrameArgs in RWHVA::OnVSync, i.e., the vsync time. But
let's fix this separately.

[no sievers, 2014-05-13 22:33:31]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2014-05-13 22:36:17]

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

[commit-bot: I haz the power, 2014-05-13 22:41:06]

Change committed as 270231

[no sievers, 2014-05-13 22:51:02]

On 2014/05/13 22:31:19, Sami wrote:
> lgtm too, however:
> 
> > Hmm if we composite on vsync, now is the vsync time.
> > Otherwise would we really want to use vsync time? AURA seems 
> > to be using now()
> > also.
> 
> Not exactly: now() will include random CPU jitter. Ideally we'd use the same
> time we plug into BeginFrameArgs in RWHVA::OnVSync, i.e., the vsync time. But
> let's fix this separately.

Thanks. I'll also file some other bugs for the things I know I broke, but Dana
says SingleThreadProxy with full-blown cc scheduler is likely to happen for M37,
so we can try to focus on that codepath.