Commit Pepper backing texture immediately on swapbuffers.

Commit Pepper backing texture immediately on swapbuffers.

There's no need to wait for the GPU process to finish processing the commands before we inform the compositor and allow it to composite, so inform it immediately on swapbuffers.


BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111483

[piman, 2011-11-23 06:41:49]

http://codereview.chromium.org/8678001/diff/1/webkit/plugins/ppapi/ppb_graphi...
File webkit/plugins/ppapi/ppb_graphics_3d_impl.cc (right):

http://codereview.chromium.org/8678001/diff/1/webkit/plugins/ppapi/ppb_graphi...
webkit/plugins/ppapi/ppb_graphics_3d_impl.cc:176:
ResourceHelper::GetPluginInstance(this)->CommitBackingTexture();
This part is good, but you'll want to fix
ppapi::proxy::Graphics3D::DoSwapBuffers to send the command buffer swap *before*
the swap buffers message, otherwise you have a race.
(note, it looks like the current code already has a race, that was introduced by
the switch to Echo. Hmm...).

[jbauman, 2011-11-23 07:06:20]

On 2011/11/23 06:41:49, piman wrote:
>
http://codereview.chromium.org/8678001/diff/1/webkit/plugins/ppapi/ppb_graphi...
> File webkit/plugins/ppapi/ppb_graphics_3d_impl.cc (right):
> 
>
http://codereview.chromium.org/8678001/diff/1/webkit/plugins/ppapi/ppb_graphi...
> webkit/plugins/ppapi/ppb_graphics_3d_impl.cc:176:
> ResourceHelper::GetPluginInstance(this)->CommitBackingTexture();
> This part is good, but you'll want to fix
> ppapi::proxy::Graphics3D::DoSwapBuffers to send the command buffer swap
*before*
> the swap buffers message, otherwise you have a race.
> (note, it looks like the current code already has a race, that was introduced
by
> the switch to Echo. Hmm...).
Yeah, nfullagar is already working on that. I'm not sure if that race can
actually happen as I think the commit just queues up a composite which will
happen after the untrusted code does the command buffer swap.

[piman, 2011-11-23 07:35:28]

On 2011/11/23 07:06:20, jbauman wrote:
> On 2011/11/23 06:41:49, piman wrote:
> >
>
http://codereview.chromium.org/8678001/diff/1/webkit/plugins/ppapi/ppb_graphi...
> > File webkit/plugins/ppapi/ppb_graphics_3d_impl.cc (right):
> > 
> >
>
http://codereview.chromium.org/8678001/diff/1/webkit/plugins/ppapi/ppb_graphi...
> > webkit/plugins/ppapi/ppb_graphics_3d_impl.cc:176:
> > ResourceHelper::GetPluginInstance(this)->CommitBackingTexture();
> > This part is good, but you'll want to fix
> > ppapi::proxy::Graphics3D::DoSwapBuffers to send the command buffer swap
> *before*
> > the swap buffers message, otherwise you have a race.
> > (note, it looks like the current code already has a race, that was
introduced
> by
> > the switch to Echo. Hmm...).
> Yeah, nfullagar is already working on that. I'm not sure if that race can
> actually happen as I think the commit just queues up a composite which will
> happen after the untrusted code does the command buffer swap.

I'm pretty sure it could happen because you're running a race between the
compositing pass and the handling of the next Flush that will include the swap
in the command buffer (you want that Flush to happen before the compositing
pass).
BTW, just to be clear, what I meant by ppapi::proxy::Graphics3D::DoSwapBuffers
is the chrome IPC proxy (in ppapi/proxy/ppb_graphics3d_proxy.cc), which is used
by Pepper Flash. The NaCl proxy may or may not be subject to this too.

[jbauman, 2011-11-23 08:15:18]

On 2011/11/23 07:35:28, piman wrote:
> On 2011/11/23 07:06:20, jbauman wrote:
> > On 2011/11/23 06:41:49, piman wrote:
> > >
> >
>
http://codereview.chromium.org/8678001/diff/1/webkit/plugins/ppapi/ppb_graphi...
> > > File webkit/plugins/ppapi/ppb_graphics_3d_impl.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/8678001/diff/1/webkit/plugins/ppapi/ppb_graphi...
> > > webkit/plugins/ppapi/ppb_graphics_3d_impl.cc:176:
> > > ResourceHelper::GetPluginInstance(this)->CommitBackingTexture();
> > > This part is good, but you'll want to fix
> > > ppapi::proxy::Graphics3D::DoSwapBuffers to send the command buffer swap
> > *before*
> > > the swap buffers message, otherwise you have a race.
> > > (note, it looks like the current code already has a race, that was
> introduced
> > by
> > > the switch to Echo. Hmm...).
> > Yeah, nfullagar is already working on that. I'm not sure if that race can
> > actually happen as I think the commit just queues up a composite which will
> > happen after the untrusted code does the command buffer swap.
> 
> I'm pretty sure it could happen because you're running a race between the
> compositing pass and the handling of the next Flush that will include the swap
> in the command buffer (you want that Flush to happen before the compositing
> pass).
> BTW, just to be clear, what I meant by ppapi::proxy::Graphics3D::DoSwapBuffers
> is the chrome IPC proxy (in ppapi/proxy/ppb_graphics3d_proxy.cc), which is
used
> by Pepper Flash. The NaCl proxy may or may not be subject to this too.

At least with NaCl it seems that the renderer always calls into NaCl and is busy
when it's running, so if the commit just posts a task to the main loop to do a
composite (which it used to do, not sure about now) then that task will happen
after NaCl finishes running and sending the swapbuffers.

The ordering problem is pretty much identical in the normal pepper proxy and the
NaCl proxy, so this issue is the same.

[piman, 2011-11-23 08:45:21]

On Wed, Nov 23, 2011 at 12:15 AM, <jbauman@chromium.org> wrote:

> On 2011/11/23 07:35:28, piman wrote:
>
>> On 2011/11/23 07:06:20, jbauman wrote:
>> > On 2011/11/23 06:41:49, piman wrote:
>> > >
>> >
>>
>
> http://codereview.chromium.**org/8678001/diff/1/webkit/**
>
plugins/ppapi/ppb_graphics_3d_**impl.cc<http://codereview.chromium.org/8678001/diff/1/webkit/plugins/ppapi/ppb_graphics_3d_impl.cc>
>
>> > > File webkit/plugins/ppapi/ppb_**graphics_3d_impl.cc (right):
>> > >
>> > >
>> >
>>
>
> http://codereview.chromium.**org/8678001/diff/1/webkit/**
>
plugins/ppapi/ppb_graphics_3d_**impl.cc#newcode176<http://codereview.chromium.org/8678001/diff/1/webkit/plugins/ppapi/ppb_graphics_3d_impl.cc#newcode176>
>
>> > > webkit/plugins/ppapi/ppb_**graphics_3d_impl.cc:176:
>> > > ResourceHelper::**GetPluginInstance(this)->**CommitBackingTexture();
>> > > This part is good, but you'll want to fix
>> > > ppapi::proxy::Graphics3D::**DoSwapBuffers to send the command buffer
>> swap
>> > *before*
>> > > the swap buffers message, otherwise you have a race.
>> > > (note, it looks like the current code already has a race, that was
>> introduced
>> > by
>> > > the switch to Echo. Hmm...).
>> > Yeah, nfullagar is already working on that. I'm not sure if that race
>> can
>> > actually happen as I think the commit just queues up a composite which
>> will
>> > happen after the untrusted code does the command buffer swap.
>>
>
>  I'm pretty sure it could happen because you're running a race between the
>> compositing pass and the handling of the next Flush that will include the
>> swap
>> in the command buffer (you want that Flush to happen before the
>> compositing
>> pass).
>> BTW, just to be clear, what I meant by ppapi::proxy::Graphics3D::**
>> DoSwapBuffers
>> is the chrome IPC proxy (in ppapi/proxy/ppb_graphics3d_**proxy.cc),
>> which is
>>
> used
>
>> by Pepper Flash. The NaCl proxy may or may not be subject to this too.
>>
>
> At least with NaCl it seems that the renderer always calls into NaCl and
> is busy
> when it's running, so if the commit just posts a task to the main loop to
> do a
> composite (which it used to do, not sure about now) then that task will
> happen
> after NaCl finishes running and sending the swapbuffers.
>
> The ordering problem is pretty much identical in the normal pepper proxy
> and the
> NaCl proxy, so this issue is the same.
>

It is not, because the pepper proxy is asynchronous, so it is not
necessarily true that the compositor task will be delayed after the Flush
has been sent:

1- (pepper plugin) Graphics3D::DoSwapBuffers is
called, PpapiHostMsg_PPBGraphics3D_SwapBuffers message is sent
2- (renderer io thread) PpapiHostMsg_PPBGraphics3D_SwapBuffers message is
received, task is posted to renderer main thread
3- (renderer main thread) PPB_Graphics3D_Proxy::OnMsgSwapBuffers is called,
calls PPB_Graphics3D_Impl::DoSwapBuffers, compositor is triggered,
compositor task is posted to the main thread
4- (pepper plugin) still from Graphics3D::DoSwapBuffers, the SwapBuffers
command is added to the plugin command buffer, CommandBuffer::Flush is
called, PpapiHostMsg_PPBGraphics3D_AsyncFlush is sent
5- (renderer io thread) PpapiHostMsg_PPBGraphics3D_AsyncFlush is received,
task is posted to renderer main thread
6- (renderer main thread) compositor task, draws using current
(non-swapped) plugin texture, flushes the compositor command buffer to the
GPU process
7- (renderer main thread) PPB_Graphics3D_Proxy::OnMsgAsyncFlush is called,
calls PPB_Graphics3D_Impl::Flush, flushed the plugin command buffer
(containing the swap) to the GPU process

You have a race between (6) and (7), because you have a race between (3)
and (5).
If you change the code in Graphics3D::DoSwapBuffers to do the command
buffer swap *before* sending the PpapiHostMsg_PPBGraphics3D_SwapBuffers,
then you ensure that (5) hapens before (2), so (7) happens before (3), and
so before (6), which is what you want.

Antoine


>
http://codereview.chromium.**org/8678001/<http://codereview.chromium.org/8678...
>

[jbauman, 2011-11-24 01:16:42]

Ok, that race probably wasn't possible on NaCl, for various NaCl-specific
reasons. Anyway, it should hopefully be fixed now for both.

On 2011/11/23 08:45:21, piman wrote:
> On Wed, Nov 23, 2011 at 12:15 AM, <mailto:jbauman@chromium.org> wrote:
> 
> > On 2011/11/23 07:35:28, piman wrote:
> >
> >> On 2011/11/23 07:06:20, jbauman wrote:
> >> > On 2011/11/23 06:41:49, piman wrote:
> >> > >
> >> >
> >>
> >
> > http://codereview.chromium.**org/8678001/diff/1/webkit/**
> >
>
plugins/ppapi/ppb_graphics_3d_**impl.cc<http://codereview.chromium.org/8678001/diff/1/webkit/plugins/ppapi/ppb_graphics_3d_impl.cc>
> >
> >> > > File webkit/plugins/ppapi/ppb_**graphics_3d_impl.cc (right):
> >> > >
> >> > >
> >> >
> >>
> >
> > http://codereview.chromium.**org/8678001/diff/1/webkit/**
> >
>
plugins/ppapi/ppb_graphics_3d_**impl.cc#newcode176<http://codereview.chromium.org/8678001/diff/1/webkit/plugins/ppapi/ppb_graphics_3d_impl.cc#newcode176>
> >
> >> > > webkit/plugins/ppapi/ppb_**graphics_3d_impl.cc:176:
> >> > > ResourceHelper::**GetPluginInstance(this)->**CommitBackingTexture();
> >> > > This part is good, but you'll want to fix
> >> > > ppapi::proxy::Graphics3D::**DoSwapBuffers to send the command buffer
> >> swap
> >> > *before*
> >> > > the swap buffers message, otherwise you have a race.
> >> > > (note, it looks like the current code already has a race, that was
> >> introduced
> >> > by
> >> > > the switch to Echo. Hmm...).
> >> > Yeah, nfullagar is already working on that. I'm not sure if that race
> >> can
> >> > actually happen as I think the commit just queues up a composite which
> >> will
> >> > happen after the untrusted code does the command buffer swap.
> >>
> >
> >  I'm pretty sure it could happen because you're running a race between the
> >> compositing pass and the handling of the next Flush that will include the
> >> swap
> >> in the command buffer (you want that Flush to happen before the
> >> compositing
> >> pass).
> >> BTW, just to be clear, what I meant by ppapi::proxy::Graphics3D::**
> >> DoSwapBuffers
> >> is the chrome IPC proxy (in ppapi/proxy/ppb_graphics3d_**proxy.cc),
> >> which is
> >>
> > used
> >
> >> by Pepper Flash. The NaCl proxy may or may not be subject to this too.
> >>
> >
> > At least with NaCl it seems that the renderer always calls into NaCl and
> > is busy
> > when it's running, so if the commit just posts a task to the main loop to
> > do a
> > composite (which it used to do, not sure about now) then that task will
> > happen
> > after NaCl finishes running and sending the swapbuffers.
> >
> > The ordering problem is pretty much identical in the normal pepper proxy
> > and the
> > NaCl proxy, so this issue is the same.
> >
> 
> It is not, because the pepper proxy is asynchronous, so it is not
> necessarily true that the compositor task will be delayed after the Flush
> has been sent:
> 
> 1- (pepper plugin) Graphics3D::DoSwapBuffers is
> called, PpapiHostMsg_PPBGraphics3D_SwapBuffers message is sent
> 2- (renderer io thread) PpapiHostMsg_PPBGraphics3D_SwapBuffers message is
> received, task is posted to renderer main thread
> 3- (renderer main thread) PPB_Graphics3D_Proxy::OnMsgSwapBuffers is called,
> calls PPB_Graphics3D_Impl::DoSwapBuffers, compositor is triggered,
> compositor task is posted to the main thread
> 4- (pepper plugin) still from Graphics3D::DoSwapBuffers, the SwapBuffers
> command is added to the plugin command buffer, CommandBuffer::Flush is
> called, PpapiHostMsg_PPBGraphics3D_AsyncFlush is sent
> 5- (renderer io thread) PpapiHostMsg_PPBGraphics3D_AsyncFlush is received,
> task is posted to renderer main thread
> 6- (renderer main thread) compositor task, draws using current
> (non-swapped) plugin texture, flushes the compositor command buffer to the
> GPU process
> 7- (renderer main thread) PPB_Graphics3D_Proxy::OnMsgAsyncFlush is called,
> calls PPB_Graphics3D_Impl::Flush, flushed the plugin command buffer
> (containing the swap) to the GPU process
> 
> You have a race between (6) and (7), because you have a race between (3)
> and (5).
> If you change the code in Graphics3D::DoSwapBuffers to do the command
> buffer swap *before* sending the PpapiHostMsg_PPBGraphics3D_SwapBuffers,
> then you ensure that (5) hapens before (2), so (7) happens before (3), and
> so before (6), which is what you want.
> 
> Antoine
> 
> 
> >
>
http://codereview.chromium.**org/8678001/%3Chttp://codereview.chromium.org/86...>
> >

[piman, 2011-11-24 01:32:26]

On 2011/11/24 01:16:42, jbauman wrote:
> Ok, that race probably wasn't possible on NaCl, for various NaCl-specific
> reasons. Anyway, it should hopefully be fixed now for both.

LGTM

> 
> On 2011/11/23 08:45:21, piman wrote:
> > On Wed, Nov 23, 2011 at 12:15 AM, <mailto:jbauman@chromium.org> wrote:
> > 
> > > On 2011/11/23 07:35:28, piman wrote:
> > >
> > >> On 2011/11/23 07:06:20, jbauman wrote:
> > >> > On 2011/11/23 06:41:49, piman wrote:
> > >> > >
> > >> >
> > >>
> > >
> > > http://codereview.chromium.**org/8678001/diff/1/webkit/**
> > >
> >
>
plugins/ppapi/ppb_graphics_3d_**impl.cc<http://codereview.chromium.org/8678001/diff/1/webkit/plugins/ppapi/ppb_graphics_3d_impl.cc>
> > >
> > >> > > File webkit/plugins/ppapi/ppb_**graphics_3d_impl.cc (right):
> > >> > >
> > >> > >
> > >> >
> > >>
> > >
> > > http://codereview.chromium.**org/8678001/diff/1/webkit/**
> > >
> >
>
plugins/ppapi/ppb_graphics_3d_**impl.cc#newcode176<http://codereview.chromium.org/8678001/diff/1/webkit/plugins/ppapi/ppb_graphics_3d_impl.cc#newcode176>
> > >
> > >> > > webkit/plugins/ppapi/ppb_**graphics_3d_impl.cc:176:
> > >> > > ResourceHelper::**GetPluginInstance(this)->**CommitBackingTexture();
> > >> > > This part is good, but you'll want to fix
> > >> > > ppapi::proxy::Graphics3D::**DoSwapBuffers to send the command buffer
> > >> swap
> > >> > *before*
> > >> > > the swap buffers message, otherwise you have a race.
> > >> > > (note, it looks like the current code already has a race, that was
> > >> introduced
> > >> > by
> > >> > > the switch to Echo. Hmm...).
> > >> > Yeah, nfullagar is already working on that. I'm not sure if that race
> > >> can
> > >> > actually happen as I think the commit just queues up a composite which
> > >> will
> > >> > happen after the untrusted code does the command buffer swap.
> > >>
> > >
> > >  I'm pretty sure it could happen because you're running a race between the
> > >> compositing pass and the handling of the next Flush that will include the
> > >> swap
> > >> in the command buffer (you want that Flush to happen before the
> > >> compositing
> > >> pass).
> > >> BTW, just to be clear, what I meant by ppapi::proxy::Graphics3D::**
> > >> DoSwapBuffers
> > >> is the chrome IPC proxy (in ppapi/proxy/ppb_graphics3d_**proxy.cc),
> > >> which is
> > >>
> > > used
> > >
> > >> by Pepper Flash. The NaCl proxy may or may not be subject to this too.
> > >>
> > >
> > > At least with NaCl it seems that the renderer always calls into NaCl and
> > > is busy
> > > when it's running, so if the commit just posts a task to the main loop to
> > > do a
> > > composite (which it used to do, not sure about now) then that task will
> > > happen
> > > after NaCl finishes running and sending the swapbuffers.
> > >
> > > The ordering problem is pretty much identical in the normal pepper proxy
> > > and the
> > > NaCl proxy, so this issue is the same.
> > >
> > 
> > It is not, because the pepper proxy is asynchronous, so it is not
> > necessarily true that the compositor task will be delayed after the Flush
> > has been sent:
> > 
> > 1- (pepper plugin) Graphics3D::DoSwapBuffers is
> > called, PpapiHostMsg_PPBGraphics3D_SwapBuffers message is sent
> > 2- (renderer io thread) PpapiHostMsg_PPBGraphics3D_SwapBuffers message is
> > received, task is posted to renderer main thread
> > 3- (renderer main thread) PPB_Graphics3D_Proxy::OnMsgSwapBuffers is called,
> > calls PPB_Graphics3D_Impl::DoSwapBuffers, compositor is triggered,
> > compositor task is posted to the main thread
> > 4- (pepper plugin) still from Graphics3D::DoSwapBuffers, the SwapBuffers
> > command is added to the plugin command buffer, CommandBuffer::Flush is
> > called, PpapiHostMsg_PPBGraphics3D_AsyncFlush is sent
> > 5- (renderer io thread) PpapiHostMsg_PPBGraphics3D_AsyncFlush is received,
> > task is posted to renderer main thread
> > 6- (renderer main thread) compositor task, draws using current
> > (non-swapped) plugin texture, flushes the compositor command buffer to the
> > GPU process
> > 7- (renderer main thread) PPB_Graphics3D_Proxy::OnMsgAsyncFlush is called,
> > calls PPB_Graphics3D_Impl::Flush, flushed the plugin command buffer
> > (containing the swap) to the GPU process
> > 
> > You have a race between (6) and (7), because you have a race between (3)
> > and (5).
> > If you change the code in Graphics3D::DoSwapBuffers to do the command
> > buffer swap *before* sending the PpapiHostMsg_PPBGraphics3D_SwapBuffers,
> > then you ensure that (5) hapens before (2), so (7) happens before (3), and
> > so before (6), which is what you want.
> > 
> > Antoine
> > 
> > 
> > >
> >
>
http://codereview.chromium.**org/8678001/%253Chttp://codereview.chromium.org/...>
> > >

[commit-bot: I haz the power, 2011-11-24 01:36:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbauman@chromium.org/8678001/1

[commit-bot: I haz the power, 2011-11-24 02:39:14]

Change committed as 111483