Android: Throttle render compositor if browser (compositor) is behind.

Android: Throttle render compositor if browser (compositor) is behind.

This delays the Swap ACK to the renderer if the browser has not consumed
the current frontbuffer yet.

Note: This might break embedders that don't use the browser compositor. The texture needs to be taken from RenderWidgetHostVIew through the TextureLayerClient::PrepareTexture() method.

BUG=224602, 220907
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195340

[no sievers, 2013-04-19 17:57:17]

Antoine, mind taking a quick look if this makes sense to you? I'm trying to do
something minimal and with subtle impact for now (and for M27).

dtrainor: Can you think of any unwanted frontend interactions? Related to
- specifying a client for the layer overwrites it to be drawable even if the
texture id is zero (and you might look at isDrawable) - I tried to ensure the
old behavior
- what about pending acks and the layer being removed from the tree and such?
Seems like not accumulating acks when we are hidden should be good enough, since
it should not get remove while the RWHV is visible.

https://codereview.chromium.org/14348029/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/14348029/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_android.cc:797: unsigned
RenderWidgetHostViewAndroid::PrepareTexture(
Well, it's update resources, not really post commit. But doesn't matter for what
we have now (single-threaded browser compositor).

Soon I should be able to change this to share the frontbuffer texture and pass
the mailbox to the texture layer.

About delegated layers: Do you think it would be straightforward to also have a
callback on the layer for when commit happened and unused resources are
returned? I really like not having the path back from the compositor to all the
views for didCommit on Android (since we don't have the whole aura ui layer on
top of things to abstract this nicely, and already have too many pointers
between native and Java objects in this area).

[piman, 2013-04-19 18:09:11]

https://codereview.chromium.org/14348029/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/14348029/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_android.cc:627:
ack_callback.Run();
Note: this can add a frame of latency when GPU-bound, because the renderer will
ready a frame early, but it'll be queued until after this frame was pushed.
Furthermore, the renderer may submit GPU work (for the next frame) before the
composite for this frame, delaying even more.
I don't know if it's a concern. It'll do the trick to limit renderer throughput.

[no sievers, 2013-04-19 18:21:15]

On 2013/04/19 18:09:11, piman wrote:
>
https://codereview.chromium.org/14348029/diff/1/content/browser/renderer_host...
> File content/browser/renderer_host/render_widget_host_view_android.cc (right):
> 
>
https://codereview.chromium.org/14348029/diff/1/content/browser/renderer_host...
> content/browser/renderer_host/render_widget_host_view_android.cc:627:
> ack_callback.Run();
> Note: this can add a frame of latency when GPU-bound, because the renderer
will
> ready a frame early, but it'll be queued until after this frame was pushed.
> Furthermore, the renderer may submit GPU work (for the next frame) before the
> composite for this frame, delaying even more.
> I don't know if it's a concern. It'll do the trick to limit renderer
throughput.

Yes, I somewhat wanted that for now until we understand the overall perf impact
of the low-latency pipe better, and also until our renderer is better aligned
with vsync. I'm afraid that we've been hiding some problems by pretty much
always falling into the backlogged mode (with allowing maxFramesPending == 2 in
the renderer).

I'm afraid that otherwise taking both changes (this one and limiting the
renderer to 1 frame pending) might regress throughput in flings and other
animations.

[piman, 2013-04-19 18:34:32]

On Fri, Apr 19, 2013 at 11:21 AM, <sievers@chromium.org> wrote:

> On 2013/04/19 18:09:11, piman wrote:
>
> https://codereview.chromium.**org/14348029/diff/1/content/**
>
browser/renderer_host/render_**widget_host_view_android.cc<https://codereview.chromium.org/14348029/diff/1/content/browser/renderer_host/render_widget_host_view_android.cc>
>
>> File content/browser/renderer_host/**render_widget_host_view_**android.cc
>> (right):
>>
>
>
> https://codereview.chromium.**org/14348029/diff/1/content/**
>
browser/renderer_host/render_**widget_host_view_android.cc#**newcode627<https://codereview.chromium.org/14348029/diff/1/content/browser/renderer_host/render_widget_host_view_android.cc#newcode627>
>
>> content/browser/renderer_host/**render_widget_host_view_**android.cc:627:
>> ack_callback.Run();
>> Note: this can add a frame of latency when GPU-bound, because the renderer
>>
> will
>
>> ready a frame early, but it'll be queued until after this frame was
>> pushed.
>> Furthermore, the renderer may submit GPU work (for the next frame) before
>> the
>> composite for this frame, delaying even more.
>> I don't know if it's a concern. It'll do the trick to limit renderer
>>
> throughput.
>
> Yes, I somewhat wanted that for now until we understand the overall perf
> impact
> of the low-latency pipe better, and also until our renderer is better
> aligned
> with vsync. I'm afraid that we've been hiding some problems by pretty much
> always falling into the backlogged mode (with allowing maxFramesPending ==
> 2 in
> the renderer).
>
> I'm afraid that otherwise taking both changes (this one and limiting the
> renderer to 1 frame pending) might regress throughput in flings and other
> animations.
>

Got it, this change + going from 2 frames to 1 would be overall a noop wrt
pipelining.


>
>
https://codereview.chromium.**org/14348029/<https://codereview.chromium.org/1...
>

[brianderson, 2013-04-19 18:49:19]

https://codereview.chromium.org/14348029/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/14348029/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_android.cc:627:
ack_callback.Run();
On 2013/04/19 18:09:12, piman wrote:
> Note: this can add a frame of latency when GPU-bound, because the renderer
will
> ready a frame early, but it'll be queued until after this frame was pushed.
> Furthermore, the renderer may submit GPU work (for the next frame) before the
> composite for this frame, delaying even more.
> I don't know if it's a concern. It'll do the trick to limit renderer
throughput.

I'm not sure if this is still true, but I tried to do something similar to this
patch a few months ago and running the callback after the swap kept the renderer
and browser properly ordered on the gpu thread. i.e. even when the renderer
submitted gpu work ahead of time it would always alternate with the browser on
the gpu thread. At the time, the browser was inserting a sync point after the
swap that would unblock the next renderer frame on the gpu thread. The mailbox
path likely behaves differently though.

I agree with the increased latency part if we keep maxFramesPending==2. However,
we should only get increased latency if the renderer is producing faster than
the later pipeline stages (including the gpu hardware) can consume. Ideally we
could avoid setting maxFramesPending==1 since that would reduce concurrency, but
still keep latency low.

Keeping the latency low with maxFramesPending==2 would require us detecting that
we are in a high latency mode and then recovering from it. Unfortunately, we
currently don't have enough feedback to detect that we are in a high latency
mode.

[no sievers, 2013-04-19 18:57:06]

On 2013/04/19 18:49:19, Brian Anderson wrote:
>
https://codereview.chromium.org/14348029/diff/1/content/browser/renderer_host...
> File content/browser/renderer_host/render_widget_host_view_android.cc (right):
> 
>
https://codereview.chromium.org/14348029/diff/1/content/browser/renderer_host...
> content/browser/renderer_host/render_widget_host_view_android.cc:627:
> ack_callback.Run();
> On 2013/04/19 18:09:12, piman wrote:
> > Note: this can add a frame of latency when GPU-bound, because the renderer
> will
> > ready a frame early, but it'll be queued until after this frame was pushed.
> > Furthermore, the renderer may submit GPU work (for the next frame) before
the
> > composite for this frame, delaying even more.
> > I don't know if it's a concern. It'll do the trick to limit renderer
> throughput.
> 
> I'm not sure if this is still true, but I tried to do something similar to
this
> patch a few months ago and running the callback after the swap kept the
renderer
> and browser properly ordered on the gpu thread. i.e. even when the renderer
> submitted gpu work ahead of time it would always alternate with the browser on
> the gpu thread. At the time, the browser was inserting a sync point after the
> swap that would unblock the next renderer frame on the gpu thread. The mailbox
> path likely behaves differently though.
> 
> I agree with the increased latency part if we keep maxFramesPending==2.
However,
> we should only get increased latency if the renderer is producing faster than
> the later pipeline stages (including the gpu hardware) can consume. Ideally we
> could avoid setting maxFramesPending==1 since that would reduce concurrency,
but
> still keep latency low.
> 
> Keeping the latency low with maxFramesPending==2 would require us detecting
that
> we are in a high latency mode and then recovering from it. Unfortunately, we
> currently don't have enough feedback to detect that we are in a high latency
> mode.

Yes, I was actually thinking about something like this. Getting the best of both
worlds, i.e. pipelining extra frames when we know the future (fling and
animations), but not for scroll.

Since Sami and you have thought about this a lot we should sync up some time
soon.

Btw, this patch attempts to only fix the instability (see the bug), where the
browser does not get frames on the screen anymore. 
I also think we should stick with maxFramesPending == 1 in the renderer for now,
and investigate underrun cases that we encounter and try to optimize them
separately.

[no sievers, 2013-04-19 19:04:16]

On 2013/04/19 18:57:06, Daniel Sievers wrote:
> On 2013/04/19 18:49:19, Brian Anderson wrote:
> >
>
https://codereview.chromium.org/14348029/diff/1/content/browser/renderer_host...
> > File content/browser/renderer_host/render_widget_host_view_android.cc
(right):
> > 
> >
>
https://codereview.chromium.org/14348029/diff/1/content/browser/renderer_host...
> > content/browser/renderer_host/render_widget_host_view_android.cc:627:
> > ack_callback.Run();
> > On 2013/04/19 18:09:12, piman wrote:
> > > Note: this can add a frame of latency when GPU-bound, because the renderer
> > will
> > > ready a frame early, but it'll be queued until after this frame was
pushed.
> > > Furthermore, the renderer may submit GPU work (for the next frame) before
> the
> > > composite for this frame, delaying even more.
> > > I don't know if it's a concern. It'll do the trick to limit renderer
> > throughput.
> > 
> > I'm not sure if this is still true, but I tried to do something similar to
> this
> > patch a few months ago and running the callback after the swap kept the
> renderer
> > and browser properly ordered on the gpu thread. i.e. even when the renderer
> > submitted gpu work ahead of time it would always alternate with the browser
on
> > the gpu thread. At the time, the browser was inserting a sync point after
the
> > swap that would unblock the next renderer frame on the gpu thread. The
mailbox
> > path likely behaves differently though.
> > 
> > I agree with the increased latency part if we keep maxFramesPending==2.
> However,
> > we should only get increased latency if the renderer is producing faster
than
> > the later pipeline stages (including the gpu hardware) can consume. Ideally
we
> > could avoid setting maxFramesPending==1 since that would reduce concurrency,
> but
> > still keep latency low.
> > 
> > Keeping the latency low with maxFramesPending==2 would require us detecting
> that
> > we are in a high latency mode and then recovering from it. Unfortunately, we
> > currently don't have enough feedback to detect that we are in a high latency
> > mode.
> 
> Yes, I was actually thinking about something like this. Getting the best of
both
> worlds, i.e. pipelining extra frames when we know the future (fling and
> animations), but not for scroll.
> 
> Since Sami and you have thought about this a lot we should sync up some time
> soon.
> 
> Btw, this patch attempts to only fix the instability (see the bug), where the
> browser does not get frames on the screen anymore. 
> I also think we should stick with maxFramesPending == 1 in the renderer for
now,
> and investigate underrun cases that we encounter and try to optimize them
> separately.

Oh and that being said, also aim for removing the pipelining I allow in the
browser in this patch, like Antoine pointed out. 
When we switch to a threaded compositor, the immediate ACK will be mostly
impossible anyways, because we'd have to wait for commit to be able to return
the previous texture.

[no sievers, 2013-04-19 19:21:46]

On 2013/04/19 19:04:16, Daniel Sievers wrote:
> On 2013/04/19 18:57:06, Daniel Sievers wrote:
> > On 2013/04/19 18:49:19, Brian Anderson wrote:
> > >
> >
>
https://codereview.chromium.org/14348029/diff/1/content/browser/renderer_host...
> > > File content/browser/renderer_host/render_widget_host_view_android.cc
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/14348029/diff/1/content/browser/renderer_host...
> > > content/browser/renderer_host/render_widget_host_view_android.cc:627:
> > > ack_callback.Run();
> > > On 2013/04/19 18:09:12, piman wrote:
> > > > Note: this can add a frame of latency when GPU-bound, because the
renderer
> > > will
> > > > ready a frame early, but it'll be queued until after this frame was
> pushed.
> > > > Furthermore, the renderer may submit GPU work (for the next frame)
before
> > the
> > > > composite for this frame, delaying even more.
> > > > I don't know if it's a concern. It'll do the trick to limit renderer
> > > throughput.
> > > 
> > > I'm not sure if this is still true, but I tried to do something similar to
> > this
> > > patch a few months ago and running the callback after the swap kept the
> > renderer
> > > and browser properly ordered on the gpu thread. i.e. even when the
renderer
> > > submitted gpu work ahead of time it would always alternate with the
browser
> on
> > > the gpu thread. At the time, the browser was inserting a sync point after
> the
> > > swap that would unblock the next renderer frame on the gpu thread. The
> mailbox
> > > path likely behaves differently though.
> > > 
> > > I agree with the increased latency part if we keep maxFramesPending==2.
> > However,
> > > we should only get increased latency if the renderer is producing faster
> than
> > > the later pipeline stages (including the gpu hardware) can consume.
Ideally
> we
> > > could avoid setting maxFramesPending==1 since that would reduce
concurrency,
> > but
> > > still keep latency low.
> > > 
> > > Keeping the latency low with maxFramesPending==2 would require us
detecting
> > that
> > > we are in a high latency mode and then recovering from it. Unfortunately,
we
> > > currently don't have enough feedback to detect that we are in a high
latency
> > > mode.
> > 
> > Yes, I was actually thinking about something like this. Getting the best of
> both
> > worlds, i.e. pipelining extra frames when we know the future (fling and
> > animations), but not for scroll.
> > 
> > Since Sami and you have thought about this a lot we should sync up some time
> > soon.
> > 
> > Btw, this patch attempts to only fix the instability (see the bug), where
the
> > browser does not get frames on the screen anymore. 
> > I also think we should stick with maxFramesPending == 1 in the renderer for
> now,
> > and investigate underrun cases that we encounter and try to optimize them
> > separately.
> 
> Oh and that being said, also aim for removing the pipelining I allow in the
> browser in this patch, like Antoine pointed out. 
> When we switch to a threaded compositor, the immediate ACK will be mostly
> impossible anyways, because we'd have to wait for commit to be able to return
> the previous texture.

Also, there is a subtle difference in the interaction of scheduling and
pipelining in whether you throttle at the source (renderer, i.e.
maxFramesPending = 1), or throttle later on the GPU thread (which is what we
additionally have without composite-to-mailbox), and throttling  based on
whether the browser is behind. 

But I just wanted to mention it, because I can imagine the same effective
pipelining (2 buffers in use) having different outcome. For example, imagine the
case of the browser being poorly aligned with the system vsync, and furthermore
on a bad pattern (bad interval).

We should probably discuss somewhere else, not in this patch, my head already
hurts :)

[no sievers, 2013-04-19 19:33:06]

So based on the quasi-lgtm feedback, I guess I'll basically wait on lgtm from
dtrainor wrt frontend.

And we might want to try removing the consumed_current_texture_ optimization
sooner, rather than later.

[David Trainor- moved to gerrit, 2013-04-19 20:19:03]

lgtm

[commit-bot: I haz the power, 2013-04-19 22:43:13]

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

[no sievers, 2013-04-19 23:44:21]

Committed patchset #3 manually as r195340 (presubmit successful).

[no sievers, 2013-04-19 23:53:03]

On 2013/04/19 19:33:06, Daniel Sievers wrote:
> And we might want to try removing the consumed_current_texture_ optimization
> sooner, rather than later.

Actually, no reason not to do it right rightaway on trunk:
https://codereview.chromium.org/14254004/