Allow cmdbuffer creation from compositor thread.

Allow cmdbuffer creation from compositor thread.

This adds a message filter on the IO thread which routes messages to
the corresponding cmdbuffer proxy on the correct thread (main or compositor).
The proxies become reference counted so that the filter can track them.
When a context gets deleted, a flag gets set on the context to indicate
they have become orphaned so we do not handle any incoming messages anymore
(the callbacks etc. have become invalid when the context goes away).


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

[no sievers, 2011-08-15 18:32:58]

TODO: Use MessageLoopProxy instead of MessageLoop when
http://codereview.chromium.org/7583053/ lands.

[apatrick_chromium, 2011-08-15 19:42:02]

lgtm

[nduca, 2011-08-15 21:37:52]

http://codereview.chromium.org/7634019/diff/5010/content/renderer/gpu/gpu_cha...
File content/renderer/gpu/gpu_channel_host.h (right):

http://codereview.chromium.org/7634019/diff/5010/content/renderer/gpu/gpu_cha...
content/renderer/gpu/gpu_channel_host.h:130: class Listener : public
base::RefCountedThreadSafe<Listener> {
Promote this class out of MessageFilter and as either GpuMessageHost::Listener,
or maybe even make it toplevel and call it GpuMessageListener?

http://codereview.chromium.org/7634019/diff/5010/content/renderer/gpu/gpu_cha...
content/renderer/gpu/gpu_channel_host.h:151: class ListenerInfo {
scoped_refptr<MessageLoopProxy> not MessageLoopProxy? Afact, the only point of
having a proxy is so that you can hold a reference to it and thus keep it alive.

Can we loop_ as a member of Listener? Set it during Listener::Listener(), that
way you dont have to have a second structure?

[nduca, 2011-08-15 22:28:36]

LGTM with fragility comment fixed

http://codereview.chromium.org/7634019/diff/10001/content/renderer/gpu/comman...
File content/renderer/gpu/command_buffer_proxy.cc (right):

http://codereview.chromium.org/7634019/diff/10001/content/renderer/gpu/comman...
content/renderer/gpu/command_buffer_proxy.cc:43: if (IsOrphaned())
How about doing this in the MessageFilter side before calling OnMessageRecieved
on the listener? That would reduce fragility...

http://codereview.chromium.org/7634019/diff/10001/content/renderer/gpu/gpu_ch...
File content/renderer/gpu/gpu_channel_host.cc (right):

http://codereview.chromium.org/7634019/diff/10001/content/renderer/gpu/gpu_ch...
content/renderer/gpu/gpu_channel_host.cc:73:
&GpuChannelListener::OnMessageReceived,
i.e. post to DispatchMessage and check this->orphaned() before calling the
virtual OnMessageReceived.

[commit-bot: I haz the power, 2011-08-16 23:32:36]

Can't apply patch for file content/renderer/gpu/gpu_channel_host.h.
While running patch -p1 --forward --force;
patching file content/renderer/gpu/gpu_channel_host.h
Hunk #3 FAILED at 95.
1 out of 6 hunks FAILED -- saving rejects to file
content/renderer/gpu/gpu_channel_host.h.rej

[commit-bot: I haz the power, 2011-08-17 03:25:51]

Try job failure for 7634019-15001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[no sievers, 2011-08-18 20:31:26]

A new conflict came up with http://codereview.chromium.org/7659001.

Since GpuVideoDecodeAcceleratorHost also wants to be a directly-routed -to
listener on the channel now (but is already refcounted, and so is
GpuChannelListener), I thought instead of adding another shim, using weak
pointers would actually be nicer (rather than changing proxies to be refcounted
and using the orphan flag). 

However, note that calling WeakPtr::~WeakPtr() is currently not thread-safe, so
this needs either http://codereview.chromium.org/7677028/ or a custom
RefCountedThreadSafeTraits to delete GpuChannelListeners on the right thread.

[nduca, 2011-08-18 20:55:55]

I'm not sure I understand what's wrong? You're saying we can't land this because
it breaks a just-landed video patch?

[no sievers, 2011-08-18 22:28:46]

On 2011/08/18 20:55:55, nduca wrote:
> I'm not sure I understand what's wrong? You're saying we can't land this
because
> it breaks a just-landed video patch?

Correct, see new version. The problem is having a common base class that is
refcounted, while GpuVideoDecodeAcceleratorHost already is refcounted itself
through another base class.

I could of course add another layer for the video decoder listeners, but on
second thought I was wondering if it's nicer this way, i.e. just requiring the
listeners to be able to provide a weak pointer that we can then check (just like
the orphan flag) on the target thread.
It also allows for hiding the GpuChannelListener class in the .cc file.

[Ami GONE FROM CHROMIUM, 2011-08-19 15:28:03]

LGTM for OMX/cros/ARM purposes - I tested gles2 in Debug & Release and didn't
see any problems.
(leaving actual CR to nduca@).

[nduca, 2011-08-19 15:56:13]

LGTM with corrections.

http://codereview.chromium.org/7634019/diff/18001/content/renderer/gpu/gpu_ch...
File content/renderer/gpu/gpu_channel_host.cc (right):

http://codereview.chromium.org/7634019/diff/18001/content/renderer/gpu/gpu_ch...
content/renderer/gpu/gpu_channel_host.cc:39: };
Does the style guide say to put the implementations inline for classes like
these? I forget...

http://codereview.chromium.org/7634019/diff/18001/content/renderer/gpu/gpu_ch...
content/renderer/gpu/gpu_channel_host.cc:318: delete command_buffer;
Deletion as part of removing from the host is a scary side effect.. can it be
reworked so that CommandBufferProxy::~CommandBufferProxy() calls this function,
and we make the proxy a scoped_ptr of GLContext? And instead of destroy, we
could say something like "RemoveCommandBuffer"

[no sievers, 2011-08-22 22:45:17]

http://codereview.chromium.org/7634019/diff/18001/content/renderer/gpu/gpu_ch...
File content/renderer/gpu/gpu_channel_host.cc (right):

http://codereview.chromium.org/7634019/diff/18001/content/renderer/gpu/gpu_ch...
content/renderer/gpu/gpu_channel_host.cc:39: };
Good point. It would have to be put inside an anonymous namespace, which doesn't
work because the MessageFilter class in the header file references it.

I made it a non-public nested class, which is allowed too.

On 2011/08/19 15:56:13, nduca wrote:
> Does the style guide say to put the implementations inline for classes like
> these? I forget...

http://codereview.chromium.org/7634019/diff/18001/content/renderer/gpu/gpu_ch...
content/renderer/gpu/gpu_channel_host.cc:318: delete command_buffer;
We discussed this and will approach this separately.

On 2011/08/19 15:56:13, nduca wrote:
> Deletion as part of removing from the host is a scary side effect.. can it be
> reworked so that CommandBufferProxy::~CommandBufferProxy() calls this
function,
> and we make the proxy a scoped_ptr of GLContext? And instead of destroy, we
> could say something like "RemoveCommandBuffer"

[commit-bot: I haz the power, 2011-08-23 06:34:05]

Change committed as 97824