Add CommandBuffer::SetGetBuffer

Add CommandBuffer::SetGetBuffer

As well as remove CommandBuffer::GetRingBuffer and
change CommandBuffer::Initialize

Before this change the service allocated and managed the command buffer.

After this change the client uses CreateTransferBuffer, GetTransferBuffer,
end potentially DeleteTransferBufffer to manage the command buffer.

Another CL will actually make the client delete the command buffer
on demand.

TEST=unit tests and run some samples and a NaCl 3D game
BUG=103989
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113250

[greggman, 2011-11-19 00:40:31]

Hey Piman, Ken, Al, Nicholas, John

I think you 5 cover all the areas I touched.  Feel free to review as much as you
want or just your area.

Vangelis asked me to make this possible to free the command buffer. Arguably
it's cleaner this way but it touched a lot of places, especially in PPAPI land
which I have no experience with. 

I'm hoping none of the stuff I changed would break anything. I tried running
Sixty Seconds Shooter from the Chrome Web Store which is a NaCl Pepper 3D app
and it ran.

[greggman, 2011-11-19 00:51:44]

PS: I get this message from git cl upload

Missing matching PPAPI definition:

***************
  ppapi/api/dev/ppb_context_3d_trusted_dev.idl
  ppapi/api/trusted/ppb_graphics_3d_trusted.idl
***************


I'm assuming I can ignore that?

[jam, 2011-11-19 01:16:29]

can you please specify which reviewers should look at which files? the files in
content already have Ken/Al as owners, so you don't need me for these. the files
in plugins/ppapi already have Antoine. since i'm very overloaded with reviews,
if other can review the files, better that they do it (they're also much more
familiar with this code)

[greggman, 2011-11-19 01:35:11]

Sorry John, I don't think you need to review anything. My bad.

piman: webkit/plugins, ppapi (except native_client)
kbr: webkit/gpu
nfullgar: ppapi/native_client
al: gpu, content

[piman, 2011-11-20 01:19:15]

+ddorwin/alokp about Context3D: what do you feel about ripping it out?

http://codereview.chromium.org/8566059/diff/8001/ppapi/c/trusted/ppb_graphics...
File ppapi/c/trusted/ppb_graphics_3d_trusted.h (right):

http://codereview.chromium.org/8566059/diff/8001/ppapi/c/trusted/ppb_graphics...
ppapi/c/trusted/ppb_graphics_3d_trusted.h:61: // Initializes the command buffer
with the given size.
nit: Remove "with the given size"

http://codereview.chromium.org/8566059/diff/8001/ppapi/proxy/ppb_context_3d_p...
File ppapi/proxy/ppb_context_3d_proxy.cc (right):

http://codereview.chromium.org/8566059/diff/8001/ppapi/proxy/ppb_context_3d_p...
ppapi/proxy/ppb_context_3d_proxy.cc:189: Send(new
PpapiHostMsg_PPBContext3D_SetGetBuffer(
Note: I can see that this message is not handled, meaning, this is not
functional (you'd need to add SetGetBuffer to PPB_Context3DTrusted_Dev too).
However, Context3D/Surface3D has been superseded by Graphics3D, so I think we
can probably just rip them out. I think they're not called anymore.

[ddorwin, 2011-11-21 16:36:43]

We are no longer using the old Context and Surface interfaces, so we are fine
with removing them.

[alokp, 2011-11-21 16:41:42]

On 2011/11/21 16:36:43, ddorwin wrote:
> We are no longer using the old Context and Surface interfaces, so we are fine
> with removing them.

I have already ported all demos to use the new API. So there should not be a
problem removing them unless NaCl still has a few references left. Nicholas?

[Ken Russell (switch to Gerrit), 2011-11-21 18:04:10]

LGTM overall. Couple of minor comments.

http://codereview.chromium.org/8566059/diff/8001/gpu/command_buffer/client/gl...
File gpu/command_buffer/client/gles2_implementation_unittest.cc (right):

http://codereview.chromium.org/8566059/diff/8001/gpu/command_buffer/client/gl...
gpu/command_buffer/client/gles2_implementation_unittest.cc:123: Buffer
transfer_buffer_buffers_[10];
Could you #define kMaxTransferBuffers at the top of the file instead, and use it
instead of the "10" here?

http://codereview.chromium.org/8566059/diff/8001/gpu/command_buffer/common/co...
File gpu/command_buffer/common/command_buffer.h (right):

http://codereview.chromium.org/8566059/diff/8001/gpu/command_buffer/common/co...
gpu/command_buffer/common/command_buffer.h:89: // Sets the buffer commands are
read from.
Could you document that the passed ID is one previously returned from
CreateTransferBuffer? shm_id is a slightly confusing name -- transfer_buffer_id
might be better and avoid the need for the additional comment.

http://codereview.chromium.org/8566059/diff/8001/gpu/command_buffer/service/c...
File gpu/command_buffer/service/common_decoder_unittest.cc (right):

http://codereview.chromium.org/8566059/diff/8001/gpu/command_buffer/service/c...
gpu/command_buffer/service/common_decoder_unittest.cc:120: // TODO(gman): Fill
in before check-in.
Looks like this is still to be done.

http://codereview.chromium.org/8566059/diff/8001/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gpu_scheduler_unittest.cc (right):

http://codereview.chromium.org/8566059/diff/8001/gpu/command_buffer/service/g...
gpu/command_buffer/service/gpu_scheduler_unittest.cc:34: // FIX before check-in!
Still to be done?

[nfullagar, 2011-11-21 19:23:24]

I'll send a link to another test to try.  Otherwise lgtm.  To address Alok's
question, all of NaCl has moved to graphics3d, NaCl no longer proxies surface3d
or context3d.

[apatrick_chromium, 2011-11-22 00:27:31]

lgtm

[commit-bot: I haz the power, 2011-11-28 23:32:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gman@chromium.org/8566059/18001

[commit-bot: I haz the power, 2011-11-28 23:33:06]

Presubmit check for 8566059-18001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_server.cc, line 2357, 103
chars \
  ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_server.cc, line 2358, 93
chars

Missing matching PPAPI definition:

***************
  ppapi/api/dev/ppb_context_3d_trusted_dev.idl
  ppapi/api/trusted/ppb_graphics_3d_trusted.idl
***************

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
ppapi/proxy/ppb_context_3d_proxy.cc,ppapi/proxy/ppapi_messages.h,ppapi/proxy/ppb_graphics_3d_proxy.cc,ppapi/proxy/ppb_graphics_3d_proxy.h,webkit/plugins/ppapi/ppb_graphics_3d_impl.h,ppapi/proxy/ppb_context_3d_proxy.h,webkit/plugins/ppapi/ppb_graphics_3d_impl.cc,webkit/plugins/ppapi/ppb_context_3d_impl.cc,ppapi/thunk/ppb_context_3d_api.h,webkit/plugins/ppapi/ppb_context_3d_impl.h,ppapi/thunk/ppb_context_3d_trusted_thunk.cc,ppapi/thunk/ppb_graphics_3d_trusted_thunk.cc,ppapi/c/trusted/ppb_graphics_3d_trusted.h,ppapi/thunk/ppb_graphics_3d_api.h,ppapi/c/dev/ppb_context_3d_trusted_dev.h

Presubmit checks took 10.2s to calculate.

[piman, 2011-11-28 23:51:19]

On 2011/11/20 01:19:15, piman wrote:
> +ddorwin/alokp about Context3D: what do you feel about ripping it out?
> 
>
http://codereview.chromium.org/8566059/diff/8001/ppapi/c/trusted/ppb_graphics...
> File ppapi/c/trusted/ppb_graphics_3d_trusted.h (right):
> 
>
http://codereview.chromium.org/8566059/diff/8001/ppapi/c/trusted/ppb_graphics...
> ppapi/c/trusted/ppb_graphics_3d_trusted.h:61: // Initializes the command
buffer
> with the given size.
> nit: Remove "with the given size"
> 
>
http://codereview.chromium.org/8566059/diff/8001/ppapi/proxy/ppb_context_3d_p...
> File ppapi/proxy/ppb_context_3d_proxy.cc (right):
> 
>
http://codereview.chromium.org/8566059/diff/8001/ppapi/proxy/ppb_context_3d_p...
> ppapi/proxy/ppb_context_3d_proxy.cc:189: Send(new
> PpapiHostMsg_PPBContext3D_SetGetBuffer(
> Note: I can see that this message is not handled, meaning, this is not
> functional (you'd need to add SetGetBuffer to PPB_Context3DTrusted_Dev too).
> However, Context3D/Surface3D has been superseded by Graphics3D, so I think we
> can probably just rip them out. I think they're not called anymore.

I meant this here. Please either fix Context3D or remove it, but please do not
leave it in a limbo state.

[greggman, 2011-11-29 00:03:16]

On 2011/11/28 23:51:19, piman wrote:
> On 2011/11/20 01:19:15, piman wrote:
> > +ddorwin/alokp about Context3D: what do you feel about ripping it out?
> > 
> >
>
http://codereview.chromium.org/8566059/diff/8001/ppapi/c/trusted/ppb_graphics...
> > File ppapi/c/trusted/ppb_graphics_3d_trusted.h (right):
> > 
> >
>
http://codereview.chromium.org/8566059/diff/8001/ppapi/c/trusted/ppb_graphics...
> > ppapi/c/trusted/ppb_graphics_3d_trusted.h:61: // Initializes the command
> buffer
> > with the given size.
> > nit: Remove "with the given size"
> > 
> >
>
http://codereview.chromium.org/8566059/diff/8001/ppapi/proxy/ppb_context_3d_p...
> > File ppapi/proxy/ppb_context_3d_proxy.cc (right):
> > 
> >
>
http://codereview.chromium.org/8566059/diff/8001/ppapi/proxy/ppb_context_3d_p...
> > ppapi/proxy/ppb_context_3d_proxy.cc:189: Send(new
> > PpapiHostMsg_PPBContext3D_SetGetBuffer(
> > Note: I can see that this message is not handled, meaning, this is not
> > functional (you'd need to add SetGetBuffer to PPB_Context3DTrusted_Dev too).
> > However, Context3D/Surface3D has been superseded by Graphics3D, so I think
we
> > can probably just rip them out. I think they're not called anymore.
> 
> I meant this here. Please either fix Context3D or remove it, but please do not
> leave it in a limbo state.

I don't know what "remove it" means. There is no idl file checked in for those
so that I can fix them.

Missing matching PPAPI definition:

***************
 ppapi/api/dev/ppb_context_3d_trusted_dev.idl
 ppapi/api/trusted/ppb_graphics_3d_trusted.idl
***************

Please tell me exactly what you need me to do and I'll do it. I know absolutely
nothing about anything in src/PPAPI

[greggman, 2011-11-29 18:37:57]

On 2011/11/29 00:03:16, greggman wrote:
> On 2011/11/28 23:51:19, piman wrote:
> > On 2011/11/20 01:19:15, piman wrote:
> > > +ddorwin/alokp about Context3D: what do you feel about ripping it out?
> > > 
> > >
> >
>
http://codereview.chromium.org/8566059/diff/8001/ppapi/c/trusted/ppb_graphics...
> > > File ppapi/c/trusted/ppb_graphics_3d_trusted.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/8566059/diff/8001/ppapi/c/trusted/ppb_graphics...
> > > ppapi/c/trusted/ppb_graphics_3d_trusted.h:61: // Initializes the command
> > buffer
> > > with the given size.
> > > nit: Remove "with the given size"
> > > 
> > >
> >
>
http://codereview.chromium.org/8566059/diff/8001/ppapi/proxy/ppb_context_3d_p...
> > > File ppapi/proxy/ppb_context_3d_proxy.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/8566059/diff/8001/ppapi/proxy/ppb_context_3d_p...
> > > ppapi/proxy/ppb_context_3d_proxy.cc:189: Send(new
> > > PpapiHostMsg_PPBContext3D_SetGetBuffer(
> > > Note: I can see that this message is not handled, meaning, this is not
> > > functional (you'd need to add SetGetBuffer to PPB_Context3DTrusted_Dev
too).
> > > However, Context3D/Surface3D has been superseded by Graphics3D, so I think
> we
> > > can probably just rip them out. I think they're not called anymore.
> > 
> > I meant this here. Please either fix Context3D or remove it, but please do
not
> > leave it in a limbo state.
> 
> I don't know what "remove it" means. There is no idl file checked in for those
> so that I can fix them.
> 
> Missing matching PPAPI definition:
> 
> ***************
>  ppapi/api/dev/ppb_context_3d_trusted_dev.idl
>  ppapi/api/trusted/ppb_graphics_3d_trusted.idl
> ***************
> 
> Please tell me exactly what you need me to do and I'll do it. I know
absolutely
> nothing about anything in src/PPAPI

PING!!!

Can you please explain what you need me to do? All I did was change
src/gpu/common/command_buffer.h and follow the errors and that lead to 24
changes in PPAPI to get the errors so go away. Outside of that I have no
experience with PPAPI so if you want something different I need some direction.

PPB_Context3DTrusted_Dev doesn't have the functions that were removed (eg.
GetRingBuffer) so it doesn't seem like it needs updating.

If you want something removed what specifically? You want me to delete some
files? You want me to remove some classes from files? You want me to remove some
functions from classes? Which files, which functions, which classes?

You say fix Context3D? Which files? What do you want me to do in those files?

[greggman, 2011-12-02 19:17:11]

Hey Piman,

Can need your approval when you get a chance.
Thanks

[piman, 2011-12-02 20:32:11]

lgtm