Tighter sync and faster ACK on SwapBuffers/PostSubBuffers

Tighter sync and faster ACK on SwapBuffers/PostSubBuffers

To prevent renderers from racing ahead of the browser's ability to display them, we deschedule the renderer in the GPU process until the UI has displayed the frame. Previously, we weren't checking that the UI actually displayed the renderer's contents before rescheduling. Now we do.

To gain some asynchronicity, we reschedule once the UI has issued the present (rather than wait for GPU process to process the UI present).

BUG=none
TEST=none

[jonathan.backer, 2012-06-01 15:49:31]

piman: This solves the issue of kicking an extra frame. It was r139160. I think
that we're OK for the synchronization on access to
texture_image_transport_surface front_buffer because the browser UI swap will
flush before we start sending ACKs back to reschedule.

[piman, 2012-06-04 18:20:39]

https://chromiumcodereview.appspot.com/10453117/diff/7003/content/browser/ren...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://chromiumcodereview.appspot.com/10453117/diff/7003/content/browser/ren...
content/browser/renderer_host/render_widget_host_view_aura.cc:579:
on_compositing_will_end_callbacks_.push_back(CallbackAndCompositeNumber(
A possible gotcha for the future, is that this relies on the fact that we're
never pipelining the composites - i.e. we don't start one until we got the
swapbuffers callback. We should call that out explicitly, because that may not
stay true if we switch to using the CCScheduler / the compositor thread.

On the GPU process side, as soon as we receive the swap buffers ack, we will
touch the ex-front-now-back buffer, possibly resizing it immediately, and
because the callback is sent on another channel than the compositor context, it
will happen out-of-order wrt the rendering commands that may be referring to
that buffer.
Sending the ack on the OnCompositingEnded ensured that the commands that refer
to the new front have been executed, so there is no pending command from the UI
compositor that will refer to the old front. This is now only ensured by the
fact that we don't start a compositing pass until the commands for the previous
pass have been fully executed. We should call that out.

https://chromiumcodereview.appspot.com/10453117/diff/7003/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

https://chromiumcodereview.appspot.com/10453117/diff/7003/ui/aura/root_window...
ui/aura/root_window.cc:592: defer_draw_scheduling_ = false;
A possible issue with this is that if other observers want to trigger a redraw
in their own OnCompositingWillEnd callback, then the ability to do so depends on
the observer order (leading to subtle bugs...).
I was thinking of moving some of the ScheduleDraw culling logic to Compositor -
in a way, that's where it belongs - where it'd be easier to do this.

https://chromiumcodereview.appspot.com/10453117/diff/7003/ui/compositor/compo...
File ui/compositor/compositor.h (right):

https://chromiumcodereview.appspot.com/10453117/diff/7003/ui/compositor/compo...
ui/compositor/compositor.h:252: int last_started_id_;
naming bikeshedding: mind calling these
last_started_frame_/last_will_end_frame_/last_ended_frame_ ? From outside the
class (e.g. in root_window.cc), it's very non-obvious what these "ids" refer to.

[jonathan.backer, 2012-06-05 13:35:57]

Just to be clear, I was trying to address two problems:
- eliminating a gap of 0.5 .. 1 ms on alex between renderer and browser
composites in the GPU process by ACKing early (seems like playing with fire ---
things may break if we pipeline browser composites)
- culling superfluous ScheduleDraws

The first isn't critical, but would be nice. The second is critical to landing
browser prioritization in the GPU process (but can be a one line change if we
are willing to break aura_bench).

The versioning of ACKs from the browser also isn't critical, but made it easier
for me to reason about the traces that I was seeing. It may help with some of
the pipelining concerns that you have if we trigger OnCompositingEnded.

http://codereview.chromium.org/10453117/diff/7003/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

http://codereview.chromium.org/10453117/diff/7003/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_aura.cc:579:
on_compositing_will_end_callbacks_.push_back(CallbackAndCompositeNumber(
On 2012/06/04 18:20:39, piman wrote:
> A possible gotcha for the future, is that this relies on the fact that we're
> never pipelining the composites - i.e. we don't start one until we got the
> swapbuffers callback. We should call that out explicitly, because that may not
> stay true if we switch to using the CCScheduler / the compositor thread.

Just to reiterate what I think you are saying: Currently, when the brwoser gets
a swapbuffers callback (triggered by an Echo and Echo_ACK?), we know that all
the GL draw commands referring to previous RWHVA front surface have been issued
by the GPU process (i.e. the order of various client command streams has been
serialized appropriately). All browser composites will refer to the new RWHVA
front surface.

If pipelining the composites, we cannot AcceleratedSurfaceMsg_BuffersSwappedACK
when we receive OnCompositingEnded (the swapbuffers callback) because there may
be another frame currently being issued that refers to the previous RWHVA front
surface (which we don't want the renderer corrupting).

If we can get something similar to the composite frame ID introduced in this CL,
we can handle this appropriately --- figure out what frame ID the commit for the
new RWHVA front buffer happened on and wait for it to clear (via a swapbuffers
callback) before we send AcceleratedSurfaceMsg_BuffersSwappedACK. I don't know
enough of the CC architecture to know how feasible this is.

But just to be 100% clear, we cannot pipeline as ToT stands? This problem is
somewhat independent of my change?

> 
> On the GPU process side, as soon as we receive the swap buffers ack, we will
> touch the ex-front-now-back buffer, possibly resizing it immediately, and
> because the callback is sent on another channel than the compositor context,
it
> will happen out-of-order wrt the rendering commands that may be referring to
> that buffer.

I presume that that callback being sent that you are referring to above is
AcceleratedSurfaceMsg_BuffersSwappedACK. If so, I'm tracking your arguments.
(One channel is owned by a GpuChannelHost and another by a GpuProcessHost and
there is no guaranteed ordering?)

> Sending the ack on the OnCompositingEnded ensured that the commands that refer
> to the new front have been executed, so there is no pending command from the
UI
> compositor that will refer to the old front. This is now only ensured by the
> fact that we don't start a compositing pass until the commands for the
previous
> pass have been fully executed. We should call that out.

On ToT, there could have been a browser composite in flight (draw commands
issued, just waiting for a swapbuffers callback) when an
GpuHostMsg_AcceleratedSurfaceBuffersSwapped comes in. In this case,
OnCompositingEnded could be called for a frame displaying an old front buffer
(not a new one). Without pipelining, this is sufficient synchronization. But it
is also relying on the fact that we don't start a compositing pass until the
commands for the previous pass have been fully executed.

How about ACKing on OnCompositingEnded, but using the composite ID framework
introduced in this CL? Then we get the desired synchronization (presuming that
pipelining could maintain composite ID's appropriately)?

http://codereview.chromium.org/10453117/diff/7003/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

http://codereview.chromium.org/10453117/diff/7003/ui/aura/root_window.cc#newc...
ui/aura/root_window.cc:592: defer_draw_scheduling_ = false;
On 2012/06/04 18:20:39, piman wrote:
> A possible issue with this is that if other observers want to trigger a redraw
> in their own OnCompositingWillEnd callback, then the ability to do so depends
on
> the observer order (leading to subtle bugs...).
> I was thinking of moving some of the ScheduleDraw culling logic to Compositor
-
> in a way, that's where it belongs - where it'd be easier to do this.

Your point is taken. It's a potential problem.

I need to cull the ScheduleDraw's for the browser prioritization in the GPU
process to land (otherwise the browser draws 2 frames for every 1 from a
renderer). I'm happy to keep this or temporarily break your aura_bench suite.

Given that kicking ACKs on OnCompositingWillEnd is inherently dangerous, I
prefer the latter (breaking aura_bench). WDYT?

[piman, 2012-06-05 18:28:50]

On Tue, Jun 5, 2012 at 6:35 AM, <backer@chromium.org> wrote:

> Just to be clear, I was trying to address two problems:
> - eliminating a gap of 0.5 .. 1 ms on alex between renderer and browser
> composites in the GPU process by ACKing early (seems like playing with
> fire ---
> things may break if we pipeline browser composites)
> - culling superfluous ScheduleDraws
>
> The first isn't critical, but would be nice. The second is critical to
> landing
> browser prioritization in the GPU process (but can be a one line change if
> we
> are willing to break aura_bench).
>
> The versioning of ACKs from the browser also isn't critical, but made it
> easier
> for me to reason about the traces that I was seeing. It may help with some
> of
> the pipelining concerns that you have if we trigger OnCompositingEnded.
>
>
> http://codereview.chromium.**org/10453117/diff/7003/**
>
content/browser/renderer_host/**render_widget_host_view_aura.**cc<http://codereview.chromium.org/10453117/diff/7003/content/browser/renderer_host/render_widget_host_view_aura.cc>
> File content/browser/renderer_host/**render_widget_host_view_aura.**cc
> (right):
>
> http://codereview.chromium.**org/10453117/diff/7003/**
> content/browser/renderer_host/**render_widget_host_view_aura.**
>
cc#newcode579<http://codereview.chromium.org/10453117/diff/7003/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode579>
> content/browser/renderer_host/**render_widget_host_view_aura.**cc:579:
> on_compositing_will_end_**callbacks_.push_back(**
> CallbackAndCompositeNumber(
>
> On 2012/06/04 18:20:39, piman wrote:
>
>> A possible gotcha for the future, is that this relies on the fact that
>>
> we're
>
>> never pipelining the composites - i.e. we don't start one until we got
>>
> the
>
>> swapbuffers callback. We should call that out explicitly, because that
>>
> may not
>
>> stay true if we switch to using the CCScheduler / the compositor
>>
> thread.
>
> Just to reiterate what I think you are saying: Currently, when the
> brwoser gets a swapbuffers callback (triggered by an Echo and
> Echo_ACK?), we know that all the GL draw commands referring to previous
> RWHVA front surface have been issued by the GPU process (i.e. the order
> of various client command streams has been serialized appropriately).
> All browser composites will refer to the new RWHVA front surface.
>
> If pipelining the composites, we cannot
> AcceleratedSurfaceMsg_**BuffersSwappedACK when we receive
> OnCompositingEnded (the swapbuffers callback) because there may be
> another frame currently being issued that refers to the previous RWHVA
> front surface (which we don't want the renderer corrupting).
>
> If we can get something similar to the composite frame ID introduced in
> this CL, we can handle this appropriately --- figure out what frame ID
> the commit for the new RWHVA front buffer happened on and wait for it to
> clear (via a swapbuffers callback) before we send
> AcceleratedSurfaceMsg_**BuffersSwappedACK. I don't know enough of the CC
> architecture to know how feasible this is.
>
> But just to be 100% clear, we cannot pipeline as ToT stands? This
> problem is somewhat independent of my change?


I think you're right, because we would potentially ack on the swapbuffers
callback for 2 frames ago instead of the last one.

In any case all I was saying is that we should make a note about this, not
that we can't land your change!


> On the GPU process side, as soon as we receive the swap buffers ack,
>>
> we will
>
>> touch the ex-front-now-back buffer, possibly resizing it immediately,
>>
> and
>
>> because the callback is sent on another channel than the compositor
>>
> context, it
>
>> will happen out-of-order wrt the rendering commands that may be
>>
> referring to
>
>> that buffer.
>>
>
> I presume that that callback being sent that you are referring to above
> is AcceleratedSurfaceMsg_**BuffersSwappedACK. If so, I'm tracking your
> arguments. (One channel is owned by a GpuChannelHost and another by a
> GpuProcessHost and there is no guaranteed ordering?)


Correct.
The syncpoints in https://chromiumcodereview.appspot.com/10510013/ could
help restore that ordering though.

Sending the ack on the OnCompositingEnded ensured that the commands
>>
> that refer
>
>> to the new front have been executed, so there is no pending command
>>
> from the UI
>
>> compositor that will refer to the old front. This is now only ensured
>>
> by the
>
>> fact that we don't start a compositing pass until the commands for the
>>
> previous
>
>> pass have been fully executed. We should call that out.
>>
>
> On ToT, there could have been a browser composite in flight (draw
> commands issued, just waiting for a swapbuffers callback) when an
> GpuHostMsg_**AcceleratedSurfaceBuffersSwapp**ed comes in. In this case,
> OnCompositingEnded could be called for a frame displaying an old front
> buffer (not a new one). Without pipelining, this is sufficient
> synchronization. But it is also relying on the fact that we don't start
>
> a compositing pass until the commands for the previous pass have been
> fully executed.
>
> How about ACKing on OnCompositingEnded, but using the composite ID
> framework introduced in this CL? Then we get the desired synchronization
> (presuming that pipelining could maintain composite ID's appropriately)?
>

Yes, that could solve the issue as well. But that part of this change is
fine for now (maybe just add a note).


> http://codereview.chromium.**org/10453117/diff/7003/ui/**
>
aura/root_window.cc<http://codereview.chromium.org/10453117/diff/7003/ui/aura/root_window.cc>
> File ui/aura/root_window.cc (right):
>
> http://codereview.chromium.**org/10453117/diff/7003/ui/**
>
aura/root_window.cc#newcode592<http://codereview.chromium.org/10453117/diff/7003/ui/aura/root_window.cc#newcode592>
>
> ui/aura/root_window.cc:592: defer_draw_scheduling_ = false;
> On 2012/06/04 18:20:39, piman wrote:
>
>> A possible issue with this is that if other observers want to trigger
>>
> a redraw
>
>> in their own OnCompositingWillEnd callback, then the ability to do so
>>
> depends on
>
>> the observer order (leading to subtle bugs...).
>> I was thinking of moving some of the ScheduleDraw culling logic to
>>
> Compositor -
>
>> in a way, that's where it belongs - where it'd be easier to do this.
>>
>
> Your point is taken. It's a potential problem.
>
> I need to cull the ScheduleDraw's for the browser prioritization in the
> GPU process to land (otherwise the browser draws 2 frames for every 1
> from a renderer). I'm happy to keep this or temporarily break your
> aura_bench suite.
>
> Given that kicking ACKs on OnCompositingWillEnd is inherently dangerous,
> I prefer the latter (breaking aura_bench). WDYT?
>

That's fine - I just figured it's fairly trivial to move the
 defer_draw_scheduling_ logic into compositor.cc where it belongs. If not,
you can break aura_bench, and I'll pick up the pieces later.


>
http://codereview.chromium.**org/10453117/<http://codereview.chromium.org/104...
>

[jonathan.backer, 2012-06-05 18:37:46]

On 2012/06/05 18:28:50, piman wrote:
> >
> > Given that kicking ACKs on OnCompositingWillEnd is inherently dangerous,
> > I prefer the latter (breaking aura_bench). WDYT?
> >
> 
> That's fine - I just figured it's fairly trivial to move the
>  defer_draw_scheduling_ logic into compositor.cc where it belongs. If not,
> you can break aura_bench, and I'll pick up the pieces later.

I'll take a stab at this (probably late today or early tomorrow). I'll
prioritize this given http://crbug.com/131059). I imagined making the changes up
in WK -- which I was going to leave up to you :D

Thanks for all of the clarifications. It's good to know that we're all on the
same page.