Fix gpu acceleration with --in-process-gpu

Fix gpu acceleration with --in-process-gpu and --single-process modes.

With recent changes that have moved gpu message handling in the browser to the IO thread (and moved the handling of messages between gpu and renderer, that are mediated by the browser, to GpuProcessHost), the routing for such messages was broken when running the gpu thread (rather than process).

The new approach is to always instantiate GpuProcessHost (even when running a gpu thread only) and have a real IPC channel between host and gpu thread. This makes the 'in-process' GPU code work similar to what the renderer does when running --single-process.

Note that --single-process mode is potentially still a bit fragile with this, since ChildProcess and ChildThread are currently written to only allow a single static instance in one process (it would be better to instantiate GpuProcess and RenderProcess simultaneously), so ambiguous calls to access e.g. the main thread are possible.

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

[Daniel Sievers (Google), 2011-05-20 17:08:43]

Maybe if I merge the code from GpuHostDelegate into GpuBrowserHost and then
rename that class to something more meaningful like GpuMessageHub, this could be
getting somewhere.

[apatrick_chromium, 2011-05-20 18:22:25]

On 2011/05/20 17:08:43, Daniel Sievers wrote:
> Maybe if I merge the code from GpuHostDelegate into GpuBrowserHost and then
> rename that class to something more meaningful like GpuMessageHub, this could
be
> getting somewhere.

Thanks for looking into fixing this. Looks good so far. A whiteboard session
would help. I think there is one class more than needed but I'm not sure which
one :) See comment about potential base interface. I will continue to ponder
this.

[apatrick_chromium, 2011-05-23 22:01:33]

LGTM

http://codereview.chromium.org/7054005/diff/5001/content/browser/gpu/gpu_thre...
File content/browser/gpu/gpu_thread_host.cc (right):

http://codereview.chromium.org/7054005/diff/5001/content/browser/gpu/gpu_thre...
content/browser/gpu/gpu_thread_host.cc:36: : message_hub_(browser_host) { }
drop } to next line.

http://codereview.chromium.org/7054005/diff/5001/content/browser/gpu/gpu_thre...
content/browser/gpu/gpu_thread_host.cc:102: // TODO(apatrick): Handle failure to
initialize (asynchronously).
redundant block

http://codereview.chromium.org/7054005/diff/5001/content/browser/renderer_hos...
File content/browser/renderer_host/gpu_message_filter.cc (left):

http://codereview.chromium.org/7054005/diff/5001/content/browser/renderer_hos...
content/browser/renderer_host/gpu_message_filter.cc:183: // TODO(apatrick):
Eventually, this IPC message will be routed to a
keep TODO

http://codereview.chromium.org/7054005/diff/5001/content/browser/renderer_hos...
content/browser/renderer_host/gpu_message_filter.cc:201: // TODO(apatrick):
Eventually, this IPC message will be routed to a
keep TODO

[jam, 2011-05-23 22:19:44]

I didn't look closely at this, but I had one question: as I understand it, when
the user of the GPU process is the browser process (as opposed to a renderer),
IPC messages are posted to threads directly.  Do you have sync messages?

It seems that this creates two different ways of talking to the GPU process,
whether the source is the renderer or browser.  Why not make the browser send
messages over a real IPC channel?  This is what the single-process mode does
with the renderer, and it keeps the code simple since we only have one code
path.

[jam, 2011-05-23 22:36:27]

also a small nit: there's no reason for 

BUG=
TEST=

only add either of these lines if there's a bug number or there are instructions
you want to pass to the manual test team.

[Daniel Sievers (Google), 2011-05-26 00:53:01]

New version uploaded according to John's idea.

This works for --in-process-gpu, but needs more work for --single-process (see
also discussions in http://codereview.chromium.org/6966030/), because we
currently cannot instantiate more than one ChildProcess object (i.e. GpuProcess
and RenderProcess) simultaneously in the same process.

[jam, 2011-05-26 01:15:24]

http://codereview.chromium.org/7054005/diff/12001/content/browser/gpu/gpu_pro...
File content/browser/gpu/gpu_process_host.cc (right):

http://codereview.chromium.org/7054005/diff/12001/content/browser/gpu/gpu_pro...
content/browser/gpu/gpu_process_host.cc:140: DCHECK(!GpuProcess::current());
what happens if someone uses --single-process mode now?  this will break right? 
we should at least keep single process mode rendering working, even if gpu stuff
doesn't work.

http://codereview.chromium.org/7054005/diff/12001/content/browser/gpu/gpu_pro...
content/browser/gpu/gpu_process_host.cc:178: bool in_process =
can you move the in_process bits to inside of GpuProcessHost's constructor,
instead of in the factory method?  It's always nicer to make the constructor
take less arguments

http://codereview.chromium.org/7054005/diff/12001/content/browser/gpu/gpu_pro...
File content/browser/gpu/gpu_process_host.h (right):

http://codereview.chromium.org/7054005/diff/12001/content/browser/gpu/gpu_pro...
content/browser/gpu/gpu_process_host.h:26: class GpuThreadWrapper;
nit: can you call this GpuMainThread for consistency with
BrowserRenderProcessHost's RendererMainThread

[Daniel Sievers (Google), 2011-05-26 01:35:09]

+avi

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/browser_thread.h,content/browser/browser_thread.cc

http://codereview.chromium.org/7054005/diff/12001/content/browser/gpu/gpu_pro...
File content/browser/gpu/gpu_process_host.cc (right):

http://codereview.chromium.org/7054005/diff/12001/content/browser/gpu/gpu_pro...
content/browser/gpu/gpu_process_host.cc:140: DCHECK(!GpuProcess::current());
On 2011/05/26 01:15:24, John Abd-El-Malek wrote:
> what happens if someone uses --single-process mode now?  this will break
right? 

Before this change, it would just not work (time out trying to establish a
channel). 

Ignore the DCHECK(), it would still not work after this change, because of
ChildThread::current() and ChildProcess::current() logic in the code.

I have verified that it would however work, if that issue was solved by allowing
GpuProcess to have it's own instance and patching a few other places.



> we should at least keep single process mode rendering working, even if gpu
stuff
> doesn't work.

It actually will not hit the DCHECK() as long as you do not try to enter
compositing, in which case it wouldn't work anyhow.

[jam, 2011-05-26 04:46:51]

On 2011/05/26 01:35:09, Daniel Sievers wrote:
> +avi
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for:
> content/browser/browser_thread.h,content/browser/browser_thread.cc
> 
>
http://codereview.chromium.org/7054005/diff/12001/content/browser/gpu/gpu_pro...
> File content/browser/gpu/gpu_process_host.cc (right):
> 
>
http://codereview.chromium.org/7054005/diff/12001/content/browser/gpu/gpu_pro...
> content/browser/gpu/gpu_process_host.cc:140: DCHECK(!GpuProcess::current());
> On 2011/05/26 01:15:24, John Abd-El-Malek wrote:
> > what happens if someone uses --single-process mode now?  this will break
> right? 
> 
> Before this change, it would just not work (time out trying to establish a
> channel). 
> 
> Ignore the DCHECK(), it would still not work after this change, because of
> ChildThread::current() and ChildProcess::current() logic in the code.
> 
> I have verified that it would however work, if that issue was solved by
allowing
> GpuProcess to have it's own instance and patching a few other places.

I mean that people who use single process mode might get a dcheck now where
before they wouldn't.  Sure in both cases the GPU code wouldn't work, but it
would be good to not get a dcheck now if before we wouldn't.

> 
> 
> 
> > we should at least keep single process mode rendering working, even if gpu
> stuff
> > doesn't work.
> 
> It actually will not hit the DCHECK() as long as you do not try to enter
> compositing, in which case it wouldn't work anyhow.

[jam, 2011-05-26 04:50:00]

lgtm after you address the comment re the dcheck (note i'm in the owners file
for content, so you don't need avi for this.  the presubmit check failed because
i hadn't LGTM'd it yet)

[Daniel Sievers (Google), 2011-05-26 19:09:08]

Ok, I changed the DCHECK() and actually was able to make --single-process mode
work even (not in the way it should eventually work, but better than nothing).

It just required the additional change to gpu_child_thread.cc because the log
message handler would end up retrieving the wrong thread from
ChildThread::current(), but no reason to pipe the log messages in single-process
mode anyways (logging still works in single-process mode).


On 2011/05/26 04:50:00, John Abd-El-Malek wrote:
> lgtm after you address the comment re the dcheck (note i'm in the owners file
> for content, so you don't need avi for this.  the presubmit check failed
because
> i hadn't LGTM'd it yet)

[jam, 2011-05-26 22:03:54]

lgtm

http://codereview.chromium.org/7054005/diff/18002/content/gpu/gpu_child_threa...
File content/gpu/gpu_child_thread.cc (right):

http://codereview.chromium.org/7054005/diff/18002/content/gpu/gpu_child_threa...
content/gpu/gpu_child_thread.cc:119: if
(!CommandLine::ForCurrentProcess()->HasSwitch(switches::kSingleProcess))
can you please add a comment about why you're doing this

[commit-bot: I haz the power, 2011-05-26 22:35:32]

Try job failure for 7054005-23002 on linux:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[commit-bot: I haz the power, 2011-05-27 03:18:38]

Change committed as 86958