Route IPC through browser when creating a viewable command buffer.

Route IPC through browser when creating a viewable command buffer.

The communications path for creating a viewable command buffer used to be directly from the renderer (gpu_channel.cc) to the gpu process (gpu_channel.cc).  This patch makes the browser an intermediary:

- renderer (gpu_channel.cc) makes a synchronous request to the browser (picked up in renderer_message_filter.cc and forwarded to gpu_process_host.cc)

- browser (gpu_process_host.cc) makes an asynchronous request to the gpu process (picked up in gpu_thread.cc and forwarded to gpu_channel.cc) for the command buffer

- gpu process (gpu_thread.cc) sends an ACK with the route_id for the command buffer back to the browser (gpu_process_host.cc)

- browser (gpu_process_host.cc) sends a delayed reply back to the renderer 
(gpu_channel_host.cc), which had blocked

There are several motivations for this patch:

- creating an onscreen command buffer requires a window to draw into (which is acquired/locked in the browser); by routing through the browser, we can acquire the get the window beforehand (thereby preventing a deadlock in some other work that I'm doing)

- we can eliminate several separate synchronous IPC messages for obtaining and releasing the window associated with drawing (I've tried to unify the different code paths for Linux, Windows, and Mac)

- in the future, we can have the browser allocate SHM for the command buffer and transfer buffers, allowing us to sandbox the gpu process

BUG=none
TEST=by hand on all 3 platforms, trybots

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

[jam, 2011-01-25 17:30:21]

http://codereview.chromium.org/6343006/diff/9001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_message_filter.cc (right):

http://codereview.chromium.org/6343006/diff/9001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_message_filter.cc:1429: void
RenderMessageFilter::OnCreateViewCommandBuffer(
There are 3 GPU related messages now being filtered, it may be time to create a
separate BrowserMessageFilter to filter them, so that RenderMessageFilter
doesn't have to know about GPU related code.  Do you expect more messages in the
future?

If you do this, the renderer<>browser GPU related messages can also move to
gpu_messages

[apatrick, 2011-01-26 19:50:05]

This is a great improvement. A few thoughts about the organization of things in
GpuChannelHost. Also, could you put asserts in the GPU process Send functions to
check no synchronous messages are sent?

http://codereview.chromium.org/6343006/diff/9001/chrome/browser/gpu_process_h...
File chrome/browser/gpu_process_host.cc (right):

http://codereview.chromium.org/6343006/diff/9001/chrome/browser/gpu_process_h...
chrome/browser/gpu_process_host.cc:215: static void DispatchIPCAndQueueReply(
Can't these just be free functions?

http://codereview.chromium.org/6343006/diff/9001/chrome/browser/gpu_process_h...
chrome/browser/gpu_process_host.cc:260:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
As a general rule, all the code in this file should be running on the IO thread.
I understand this is not a member of GpuProcessHost and will not accidentally
access its members from the wrong thread but we have tried to organize code that
runs on the UI thread into GpuProcessHostUIShim.

http://codereview.chromium.org/6343006/diff/9001/chrome/gpu/gpu_thread.cc
File chrome/gpu/gpu_thread.cc (right):

http://codereview.chromium.org/6343006/diff/9001/chrome/gpu/gpu_thread.cc#new...
chrome/gpu/gpu_thread.cc:222: iter->second->CreateViewCommandBuffer(
More than one line so surround with braces.

[jonathan.backer, 2011-01-26 20:42:17]

Responds to comments, fixed a nit, and rebased off of ToT.

http://codereview.chromium.org/6343006/diff/9001/chrome/browser/gpu_process_h...
File chrome/browser/gpu_process_host.cc (right):

http://codereview.chromium.org/6343006/diff/9001/chrome/browser/gpu_process_h...
chrome/browser/gpu_process_host.cc:215: static void DispatchIPCAndQueueReply(
On 2011/01/26 19:50:05, apatrick wrote:
> Can't these just be free functions?

I need to access GpuProcessHost internals. I opted to make CVCBThreadHopping a
friend class to do this. That's why they aren't free functions.

I could have declared a static method of GpuProcessHost instead, but the method
would have to be public. This seemed like a good way to hide the implementation
details.

http://codereview.chromium.org/6343006/diff/9001/chrome/browser/gpu_process_h...
chrome/browser/gpu_process_host.cc:260:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2011/01/26 19:50:05, apatrick wrote:
> As a general rule, all the code in this file should be running on the IO
thread.
> I understand this is not a member of GpuProcessHost and will not accidentally
> access its members from the wrong thread but we have tried to organize code
that
> runs on the UI thread into GpuProcessHostUIShim.

I understand. I did it this way because I didn't want to clutter the
gpu_process_host_ui_shim.h and I thought the thread hoppping code was easier to
read all in one place.

http://codereview.chromium.org/6343006/diff/9001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_message_filter.cc (right):

http://codereview.chromium.org/6343006/diff/9001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_message_filter.cc:1429: void
RenderMessageFilter::OnCreateViewCommandBuffer(
On 2011/01/25 17:30:21, John Abd-El-Malek wrote:
> There are 3 GPU related messages now being filtered, it may be time to create
a
> separate BrowserMessageFilter to filter them, so that RenderMessageFilter
> doesn't have to know about GPU related code.  Do you expect more messages in
the
> future?
> 
> If you do this, the renderer<>browser GPU related messages can also move to
> gpu_messages

I think that this is a good idea. We probably will want more such messages for
sandboxing the gpu process (e.g. have the creation of offscreen command buffers
go through the browser so that we can allocate SHM for them as well).

However, this patch it getting long already. Could I do that in a separate
patch?

http://codereview.chromium.org/6343006/diff/9001/chrome/gpu/gpu_thread.cc
File chrome/gpu/gpu_thread.cc (right):

http://codereview.chromium.org/6343006/diff/9001/chrome/gpu/gpu_thread.cc#new...
chrome/gpu/gpu_thread.cc:222: iter->second->CreateViewCommandBuffer(
On 2011/01/26 19:50:05, apatrick wrote:
> More than one line so surround with braces.

Done.

[Ken Russell (switch to Gerrit), 2011-01-26 21:52:34]

This looks good to me. I don't have any substantive comments beyond those from
the other reviewers.

[jam, 2011-01-26 22:08:05]

On Wed, Jan 26, 2011 at 12:42 PM, <backer@chromium.org> wrote:

> Responds to comments, fixed a nit, and rebased off of ToT.
>
>
>
>
>
http://codereview.chromium.org/6343006/diff/9001/chrome/browser/gpu_process_h...
> File chrome/browser/gpu_process_host.cc (right):
>
>
>
http://codereview.chromium.org/6343006/diff/9001/chrome/browser/gpu_process_h...
> chrome/browser/gpu_process_host.cc:215: static void
> DispatchIPCAndQueueReply(
> On 2011/01/26 19:50:05, apatrick wrote:
>
>> Can't these just be free functions?
>>
>
> I need to access GpuProcessHost internals. I opted to make
> CVCBThreadHopping a friend class to do this. That's why they aren't free
> functions.
>
> I could have declared a static method of GpuProcessHost instead, but the
> method would have to be public. This seemed like a good way to hide the
> implementation details.
>
>
>
>
http://codereview.chromium.org/6343006/diff/9001/chrome/browser/gpu_process_h...
> chrome/browser/gpu_process_host.cc:260:
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
> On 2011/01/26 19:50:05, apatrick wrote:
>
>> As a general rule, all the code in this file should be running on the
>>
> IO thread.
>
>> I understand this is not a member of GpuProcessHost and will not
>>
> accidentally
>
>> access its members from the wrong thread but we have tried to organize
>>
> code that
>
>> runs on the UI thread into GpuProcessHostUIShim.
>>
>
> I understand. I did it this way because I didn't want to clutter the
> gpu_process_host_ui_shim.h and I thought the thread hoppping code was
> easier to read all in one place.
>
>
>
>
http://codereview.chromium.org/6343006/diff/9001/chrome/browser/renderer_host...
> File chrome/browser/renderer_host/render_message_filter.cc (right):
>
>
>
http://codereview.chromium.org/6343006/diff/9001/chrome/browser/renderer_host...
> chrome/browser/renderer_host/render_message_filter.cc:1429: void
> RenderMessageFilter::OnCreateViewCommandBuffer(
> On 2011/01/25 17:30:21, John Abd-El-Malek wrote:
>
>> There are 3 GPU related messages now being filtered, it may be time to
>>
> create a
>
>> separate BrowserMessageFilter to filter them, so that
>>
> RenderMessageFilter
>
>> doesn't have to know about GPU related code.  Do you expect more
>>
> messages in the
>
>> future?
>>
>
>  If you do this, the renderer<>browser GPU related messages can also
>>
> move to
>
>> gpu_messages
>>
>
> I think that this is a good idea. We probably will want more such
> messages for sandboxing the gpu process (e.g. have the creation of
> offscreen command buffers go through the browser so that we can allocate
> SHM for them as well).
>
> However, this patch it getting long already. Could I do that in a
> separate patch?


These are usually famous last words of a patch :)  lgtm if you do it this
week.


>
>
> http://codereview.chromium.org/6343006/diff/9001/chrome/gpu/gpu_thread.cc
> File chrome/gpu/gpu_thread.cc (right):
>
>
>
http://codereview.chromium.org/6343006/diff/9001/chrome/gpu/gpu_thread.cc#new...
> chrome/gpu/gpu_thread.cc:222: iter->second->CreateViewCommandBuffer(
> On 2011/01/26 19:50:05, apatrick wrote:
>
>> More than one line so surround with braces.
>>
>
> Done.
>
>
> http://codereview.chromium.org/6343006/
>

[apatrick_chromium, 2011-01-26 22:22:58]

LGTM