HW video decode support for --enable-threaded-compositing

Add support for --enable-threaded-compositing by making
RendererGpuVideoDecoderFactories compositor-thread-aware.

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

[Ami GONE FROM CHROMIUM, 2012-03-08 03:18:57]

James: please review.  
Daniel: FYI.

Note this requires commenting out CCLayerTreeHost::context()'s:
    ASSERT(!CCProxy::hasImplThread());
to actually work w/ threaded compositing.  I'll send a separate (webkit) CL to
remove that.

I still need to do more testing but wanted to get your eyes on it early in case
something's crazy.

[jamesr, 2012-03-08 05:34:06]

looks fairly sane

https://chromiumcodereview.appspot.com/9639005/diff/1/content/renderer/media/...
File content/renderer/media/renderer_gpu_video_decoder_factories.cc (right):

https://chromiumcodereview.appspot.com/9639005/diff/1/content/renderer/media/...
content/renderer/media/renderer_gpu_video_decoder_factories.cc:33:
waiter.Wait();
wait, it's async but we block on it? that looks more sync to me

https://chromiumcodereview.appspot.com/9639005/diff/1/content/renderer/media/...
content/renderer/media/renderer_gpu_video_decoder_factories.cc:42: if
(wgc3dcbi_->makeContextCurrent()) {
in chromium land is it typical practice to DCHECK() on the message loop in
functions like this? in webkit in the compositor we tag most every function with
a particular thread requirement with an ASSERT() - it's useful to check people
breaking thread assumptions, but even more useful (IMO) when reading the code to
know what thread you are on.

[Ami GONE FROM CHROMIUM, 2012-03-08 17:39:50]

https://chromiumcodereview.appspot.com/9639005/diff/1/content/renderer/media/...
File content/renderer/media/renderer_gpu_video_decoder_factories.cc (right):

https://chromiumcodereview.appspot.com/9639005/diff/1/content/renderer/media/...
content/renderer/media/renderer_gpu_video_decoder_factories.cc:33:
waiter.Wait();
On 2012/03/08 05:34:06, jamesr wrote:
> wait, it's async but we block on it? that looks more sync to me

*this* method is sync; the called method is async.

https://chromiumcodereview.appspot.com/9639005/diff/1/content/renderer/media/...
content/renderer/media/renderer_gpu_video_decoder_factories.cc:42: if
(wgc3dcbi_->makeContextCurrent()) {
On 2012/03/08 05:34:06, jamesr wrote:
> in chromium land is it typical practice to DCHECK() on the message loop in
> functions like this? in webkit in the compositor we tag most every function
with
> a particular thread requirement with an ASSERT() - it's useful to check people
> breaking thread assumptions, but even more useful (IMO) when reading the code
to
> know what thread you are on.

Done.

[no sievers, 2012-03-08 20:09:27]

https://chromiumcodereview.appspot.com/9639005/diff/8001/content/renderer/med...
File content/renderer/media/renderer_gpu_video_decoder_factories.h (right):

https://chromiumcodereview.appspot.com/9639005/diff/8001/content/renderer/med...
content/renderer/media/renderer_gpu_video_decoder_factories.h:78:
WebGraphicsContext3DCommandBufferImpl* wgc3dcbi_;
What about the WGC3D going away?
Not only during context lost, but what about races with shutdown and such?

[Ami GONE FROM CHROMIUM, 2012-03-08 20:19:51]

https://chromiumcodereview.appspot.com/9639005/diff/8001/content/renderer/med...
File content/renderer/media/renderer_gpu_video_decoder_factories.h (right):

https://chromiumcodereview.appspot.com/9639005/diff/8001/content/renderer/med...
content/renderer/media/renderer_gpu_video_decoder_factories.h:78:
WebGraphicsContext3DCommandBufferImpl* wgc3dcbi_;
On 2012/03/08 20:09:27, Daniel Sievers wrote:
> What about the WGC3D going away?
> Not only during context lost, but what about races with shutdown and such?

Dunno.  James?
(in practice these races will be rare as this class is mostly involved in
setup/teardown, not steady-state playback)

[no sievers, 2012-03-08 21:16:16]

https://chromiumcodereview.appspot.com/9639005/diff/8001/content/renderer/med...
File content/renderer/media/renderer_gpu_video_decoder_factories.h (right):

https://chromiumcodereview.appspot.com/9639005/diff/8001/content/renderer/med...
content/renderer/media/renderer_gpu_video_decoder_factories.h:78:
WebGraphicsContext3DCommandBufferImpl* wgc3dcbi_;
On 2012/03/08 20:19:51, Ami Fischman wrote:
> On 2012/03/08 20:09:27, Daniel Sievers wrote:
> > What about the WGC3D going away?
> > Not only during context lost, but what about races with shutdown and such?
> 
> Dunno.  James?
> (in practice these races will be rare as this class is mostly involved in
> setup/teardown, not steady-state playback)

If you still use the WeakPtr to the ContentGLContext, I guess you'd avoid the
problem (as long as you check it for being valid any time you dereference it
obviously).

[Ami GONE FROM CHROMIUM, 2012-03-08 21:22:27]

https://chromiumcodereview.appspot.com/9639005/diff/8001/content/renderer/med...
File content/renderer/media/renderer_gpu_video_decoder_factories.h (right):

https://chromiumcodereview.appspot.com/9639005/diff/8001/content/renderer/med...
content/renderer/media/renderer_gpu_video_decoder_factories.h:78:
WebGraphicsContext3DCommandBufferImpl* wgc3dcbi_;
On 2012/03/08 21:16:16, Daniel Sievers wrote:
> On 2012/03/08 20:19:51, Ami Fischman wrote:
> > On 2012/03/08 20:09:27, Daniel Sievers wrote:
> > > What about the WGC3D going away?
> > > Not only during context lost, but what about races with shutdown and such?
> > 
> > Dunno.  James?
> > (in practice these races will be rare as this class is mostly involved in
> > setup/teardown, not steady-state playback)
> 
> If you still use the WeakPtr to the ContentGLContext, I guess you'd avoid the
> problem (as long as you check it for being valid any time you dereference it
> obviously).
> 

Actually isn't it enough to know that this member is only dereferenced on the
compositor thread's loop?  I.e. is it possible the WGC3D goes away while the
compositor's loop is still processing tasks?

[jamesr, 2012-03-08 21:33:11]

On 2012/03/08 21:22:27, Ami Fischman wrote:
>
https://chromiumcodereview.appspot.com/9639005/diff/8001/content/renderer/med...
> File content/renderer/media/renderer_gpu_video_decoder_factories.h (right):
> 
>
https://chromiumcodereview.appspot.com/9639005/diff/8001/content/renderer/med...
> content/renderer/media/renderer_gpu_video_decoder_factories.h:78:
> WebGraphicsContext3DCommandBufferImpl* wgc3dcbi_;
> On 2012/03/08 21:16:16, Daniel Sievers wrote:
> > On 2012/03/08 20:19:51, Ami Fischman wrote:
> > > On 2012/03/08 20:09:27, Daniel Sievers wrote:
> > > > What about the WGC3D going away?
> > > > Not only during context lost, but what about races with shutdown and
such?
> > > 
> > > Dunno.  James?
> > > (in practice these races will be rare as this class is mostly involved in
> > > setup/teardown, not steady-state playback)
> > 
> > If you still use the WeakPtr to the ContentGLContext, I guess you'd avoid
the
> > problem (as long as you check it for being valid any time you dereference it
> > obviously).
> > 
> 
> Actually isn't it enough to know that this member is only dereferenced on the
> compositor thread's loop?  I.e. is it possible the WGC3D goes away while the
> compositor's loop is still processing tasks?

Oh yeah, there are N compositor instances for a given tab that all share the
same compositor thread. They can all go away.

I think that if you bounce over to the compositor thread synchronously with the
WGC3DCBI*, and then once you're on the correct thread do a makeContextCurrent()
call and then grab the WeakPtr<> to the ContentGLContext, then you should be in
the clear.  The WGC3DCBI* can't go away while you are holding the callstack
open.

[Ami GONE FROM CHROMIUM, 2012-03-08 21:59:16]

>
> I think that if you bounce over to the compositor thread synchronously
> with the
> WGC3DCBI*, and then once you're on the correct thread do a
> makeContextCurrent()
> call and then grab the WeakPtr<> to the ContentGLContext, then you should
> be in
> the clear.  The WGC3DCBI* can't go away while you are holding the callstack
> open.
>

I don't follow.  Are you saying what I have now is OK (b/c I always mCC()
before using WGC3DCBI*, and always on the compositor thread) or are you
saying that I need to make a change?

[jamesr1, 2012-03-08 22:01:34]

On Thu, Mar 8, 2012 at 1:59 PM, Ami Fischman <fischman@chromium.org> wrote:

> I think that if you bounce over to the compositor thread synchronously
>> with the
>>  WGC3DCBI*, and then once you're on the correct thread do a
>> makeContextCurrent()
>> call and then grab the WeakPtr<> to the ContentGLContext, then you should
>> be in
>> the clear.  The WGC3DCBI* can't go away while you are holding the
>> callstack
>> open.
>>
>
> I don't follow.  Are you saying what I have now is OK (b/c I always mCC()
> before using WGC3DCBI*, and always on the compositor thread) or are you
> saying that I need to make a change?
>

I think you need to make a change if you're retaining a WGC3DBI* past the
initial callstack - a compositor context could go away before the
mediaplayer (or whatever's owning the factory) goes away.

- James

[Ami GONE FROM CHROMIUM, 2012-03-09 00:02:46]

PTAL - implemented what we chatted about.

[jamesr, 2012-03-09 05:02:16]

FYI, I'm handling pepper in a different way - I don't know if it'll work for
video or not.  Pepper requires that we be able to make a texture in some context
that's in a share group with the compositor context, but that's about it.  So,
I'm just providing access to a main-thread owned offscreen context that's in the
same share group as the compositor context to make the texture.

I don't think this will work for video decode - it looks like the video decode
pipeline ties into the command buffer's IPC routing, and I think you definitely
want any video decode related messages to come in to the compositor's thread and
not the main thread.

One other nice thing about using an offscreen context for pepper is that we can
always instantiate an offscreen context, but there are times in the
RenderViewImpl's lifetime where we can't construct a compositor context because
the HWND/GtkWindow doesn't exist yet, and we can't block the renderer main
thread until it comes online because it could cause deadlocks with NPAPI
plugins.  Hopefully this is never a concern for hardware decode because it's
only enabled on platforms where we don't support NPAPI (like Aura), but FYI

With all that said LGTM

http://codereview.chromium.org/9639005/diff/12001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/9639005/diff/12001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2149:
RenderThreadImpl::current()->compositor_thread()->GetWebThread()
should we just have a message_loop() getter on compositor_thread() ?

[Ami GONE FROM CHROMIUM, 2012-03-09 16:22:33]

On 2012/03/09 05:02:16, jamesr wrote:
> FYI, I'm handling pepper in a different way - I don't know if it'll work for
> video or not.  Pepper requires that we be able to make a texture in some
context
> that's in a share group with the compositor context, but that's about it.  So,
> I'm just providing access to a main-thread owned offscreen context that's in
the
> same share group as the compositor context to make the texture.

Thanks; for my education what's a "share group"?

> I don't think this will work for video decode - it looks like the video decode
> pipeline ties into the command buffer's IPC routing, and I think you
definitely
> want any video decode related messages to come in to the compositor's thread
and
> not the main thread.

Right.

> Hopefully this is never a concern for hardware decode because it's
> only enabled on platforms where we don't support NPAPI (like Aura), but FYI

Hopefully in m20 we'll have HW decode on non-cros platforms as well.
But the current code will simply fallback to SW decode when RVI can't have a
context at mediaplayer construction time, so let's worry about this then.

> With all that said LGTM

Thanks for the review!

>
http://codereview.chromium.org/9639005/diff/12001/content/renderer/render_vie...
> File content/renderer/render_view_impl.cc (right):
> 
>
http://codereview.chromium.org/9639005/diff/12001/content/renderer/render_vie...
> content/renderer/render_view_impl.cc:2149:
> RenderThreadImpl::current()->compositor_thread()->GetWebThread()
> should we just have a message_loop() getter on compositor_thread() ?

Not until there are more callsites that go through this contortion?  (IMO it
takes a number of callsites being made cleaner to warrant widening APIs)

[commit-bot: I haz the power, 2012-03-09 16:22:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/9639005/12001

[commit-bot: I haz the power, 2012-03-09 18:43:42]

Change committed as 125859