Make RendererGpuVideoDecoderFactories live on arbitrary threads.

Make RendererGpuVideoDecoderFactories live on arbitrary threads.

Mailbox support for video frames has landed. So context3d can live on
on arbitrary threads. GVDAH and its context are moved to media thread in
GpuVideoDecoder case and new vda thread in RtcVideoDecoder case.

The benefits include avoiding spurious thread hopping and not relying on
compositor/render thread always being spinning.

BUG=249594
TEST=Try http://apprtc.appspot.com/?debug=loopback on Chromebook Daisy.
     Play videos that are hardware accelerated.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213412

[wuchengli, 2013-07-17 15:16:04]

CL is ready for review. I'm not sure if GVDAH needs any change . I don't
understand how to use CommandBufferContextProvider to deal with context loss. 

Now RtcVideoDecoder and RGVDF uses a new thread. I did not use
Chrome_libJingle_WorkerThread because I remember the thread was pegged and
performance would be worse. I'll check again. Since RtcVideoDecoder is not using
Chrome_libJingle_WorkerThread, the async paths in RGVDF cannot be removed.

[Ami GONE FROM CHROMIUM, 2013-07-17 18:06:33]

Added scherkus@ to R line per his request.

[danakj, 2013-07-17 22:19:24]

On 2013/07/17 15:16:04, wuchengli wrote:
> CL is ready for review. I'm not sure if GVDAH needs any change . I don't
> understand how to use CommandBufferContextProvider to deal with context loss. 

When the context is lost, you would just need to grab the current
ContextProvider from the main thread again. Since they are ref-counted, you may
continue to use the provider with the lost context in it as long as you want
(not useful to do though, but won't crash).

> Now RtcVideoDecoder and RGVDF uses a new thread. I did not use
> Chrome_libJingle_WorkerThread because I remember the thread was pegged and
> performance would be worse. I'll check again. Since RtcVideoDecoder is not
using
> Chrome_libJingle_WorkerThread, the async paths in RGVDF cannot be removed.

[scherkus (not reviewing), 2013-07-18 00:54:43]

https://codereview.chromium.org/19534002/diff/2002/content/renderer/media/med...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/19534002/diff/2002/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:500:
scoped_ptr<base::Thread> vda_thread(new base::Thread("vda_thread"));
OOC what warrants using a separate thread? Is it your comment about the
libjingle worker thread being pegged?

Is it possible to use some other existing thread? (e.g., render thread, IO,
media thread, some other process-wide thread...)

If not, can we make this a lazily-allocated process-wide thread?

https://codereview.chromium.org/19534002/diff/2002/content/renderer/media/rtc...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/19534002/diff/2002/content/renderer/media/rtc...
content/renderer/media/rtc_video_decoder.cc:71: const
scoped_refptr<media::GpuVideoDecoder::Factories>& factories)
if GVD::Factories is being used outside of GVD, it seems like it's time to move
it to a separate interface

can you add a TODO to GVD::F?

https://codereview.chromium.org/19534002/diff/2002/content/renderer/media/rtc...
File content/renderer/media/rtc_video_decoder_factory.cc (right):

https://codereview.chromium.org/19534002/diff/2002/content/renderer/media/rtc...
content/renderer/media/rtc_video_decoder_factory.cc:29:
RTCVideoDecoder::Create(type, gpu_factories_->Clone());
can we call
RenderThreadImpl::current()->GetGpuFactories(vda_thread_->message_loop_proxy())
instead of Clone(), or is the issue that CVD() isn't called on the main thread?

https://codereview.chromium.org/19534002/diff/2002/media/filters/gpu_video_de...
File media/filters/gpu_video_decoder.h (left):

https://codereview.chromium.org/19534002/diff/2002/media/filters/gpu_video_de...
media/filters/gpu_video_decoder.h:80: GpuVideoDecoder(const
scoped_refptr<base::MessageLoopProxy>& message_loop,
this is a bit backwards ... for media::VideoDecoder implementations we pass in
the MLP of the thread that the media::VideoDecoder API is expected to run on

I'd revert this change and add a DCHECK() that message_loop ==
factories->GetMessageLoop() inside the ctor + update comment here

at least that way we'll require that |factories| must have been created to work
on the media message loop, and not the other way around (i.e., that
|factories|'s message loop just happens to be the right message loop for
media::VideoDecoder calls)


ideally GetMessageLoop() should go away entirely as I introduced as a temporary
measure since webkit/ code couldn't access content/, but that's no longer the
case, but it seems it has grown new use cases :(

https://codereview.chromium.org/11468033/diff/2001/media/filters/gpu_video_de...

[wuchengli, 2013-07-18 09:42:00]

All done. Please take another look.

https://codereview.chromium.org/19534002/diff/2002/content/renderer/media/med...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/19534002/diff/2002/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:500:
scoped_ptr<base::Thread> vda_thread(new base::Thread("vda_thread"));
On 2013/07/18 00:54:43, scherkus wrote:
> OOC what warrants using a separate thread? Is it your comment about the
> libjingle worker thread being pegged?
> 
> Is it possible to use some other existing thread? (e.g., render thread, IO,
> media thread, some other process-wide thread...)
> 
> If not, can we make this a lazily-allocated process-wide thread?
libjingle worker thread being pegged only means I cannot use it in
RtcVideoDecoder and RGVDF.

Using media thread sounds like a good idea. What RtcVideoDecoder does is similar
to GpuVideoDecoder. It makes them more consistent. I've changed the code to use
media thread.

By the way, I verified that the performance of using libjingle worker thread is
worse than using media thread or a new thread.

https://codereview.chromium.org/19534002/diff/2002/content/renderer/media/rtc...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/19534002/diff/2002/content/renderer/media/rtc...
content/renderer/media/rtc_video_decoder.cc:71: const
scoped_refptr<media::GpuVideoDecoder::Factories>& factories)
On 2013/07/18 00:54:43, scherkus wrote:
> if GVD::Factories is being used outside of GVD, it seems like it's time to
move
> it to a separate interface
> 
> can you add a TODO to GVD::F?

Done.

https://codereview.chromium.org/19534002/diff/2002/content/renderer/media/rtc...
File content/renderer/media/rtc_video_decoder_factory.cc (right):

https://codereview.chromium.org/19534002/diff/2002/content/renderer/media/rtc...
content/renderer/media/rtc_video_decoder_factory.cc:29:
RTCVideoDecoder::Create(type, gpu_factories_->Clone());
On 2013/07/18 00:54:43, scherkus wrote:
> can we call
>
RenderThreadImpl::current()->GetGpuFactories(vda_thread_->message_loop_proxy())
> instead of Clone(), or is the issue that CVD() isn't called on the main
thread?
CVD() is called on Chrome_libJingle_WorkerThread and the main thread is blocked
while this runs.

https://codereview.chromium.org/19534002/diff/2002/media/filters/gpu_video_de...
File media/filters/gpu_video_decoder.h (left):

https://codereview.chromium.org/19534002/diff/2002/media/filters/gpu_video_de...
media/filters/gpu_video_decoder.h:80: GpuVideoDecoder(const
scoped_refptr<base::MessageLoopProxy>& message_loop,
On 2013/07/18 00:54:43, scherkus wrote:
> this is a bit backwards ... for media::VideoDecoder implementations we pass in
> the MLP of the thread that the media::VideoDecoder API is expected to run on
> 
> I'd revert this change and add a DCHECK() that message_loop ==
> factories->GetMessageLoop() inside the ctor + update comment here
> 
> at least that way we'll require that |factories| must have been created to
work
> on the media message loop, and not the other way around (i.e., that
> |factories|'s message loop just happens to be the right message loop for
> media::VideoDecoder calls)
> 
> 
> ideally GetMessageLoop() should go away entirely as I introduced as a
temporary
> measure since webkit/ code couldn't access content/, but that's no longer the
> case, but it seems it has grown new use cases :(
> 
>
https://codereview.chromium.org/11468033/diff/2001/media/filters/gpu_video_de...

Done. I added DCHECK. GetMessageLoop() are only used by DCHECK now. So it's not
too bad.

[wuchengli, 2013-07-18 15:40:05]

https://codereview.chromium.org/19534002/diff/2002/content/renderer/media/rtc...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/19534002/diff/2002/content/renderer/media/rtc...
content/renderer/media/rtc_video_decoder.cc:71: const
scoped_refptr<media::GpuVideoDecoder::Factories>& factories)
On 2013/07/18 09:42:00, wuchengli wrote:
> On 2013/07/18 00:54:43, scherkus wrote:
> > if GVD::Factories is being used outside of GVD, it seems like it's time to
> move
> > it to a separate interface
> > 
> > can you add a TODO to GVD::F?
> 
> Done.
I'll upload a CL tomorrow to refactor GpuVideoDecoder::Factories out of
GpuVideoDecoder.

[wuchengli, 2013-07-19 07:47:13]

https://codereview.chromium.org/19534002/diff/2002/content/renderer/media/rtc...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/19534002/diff/2002/content/renderer/media/rtc...
content/renderer/media/rtc_video_decoder.cc:71: const
scoped_refptr<media::GpuVideoDecoder::Factories>& factories)
On 2013/07/18 15:40:05, wuchengli wrote:
> On 2013/07/18 09:42:00, wuchengli wrote:
> > On 2013/07/18 00:54:43, scherkus wrote:
> > > if GVD::Factories is being used outside of GVD, it seems like it's time to
> > move
> > > it to a separate interface
> > > 
> > > can you add a TODO to GVD::F?
> > 
> > Done.
> I'll upload a CL tomorrow to refactor GpuVideoDecoder::Factories out of
> GpuVideoDecoder.
The CL to move GpuVideoDecoder::Factories to a spparate interface is at
https://codereview.chromium.org/19638008/

[scherkus (not reviewing), 2013-07-19 18:13:58]

https://codereview.chromium.org/19534002/diff/36001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/19534002/diff/36001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:503:
RenderThreadImpl::current()->GetGpuFactories(media_loop_proxy_);
how about replacing media_loop_proxy_ with a call to
RenderThreadImpl::current()->GetMediaThreadMessageLoopProxy() ?

https://codereview.chromium.org/19534002/diff/36001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.h (right):

https://codereview.chromium.org/19534002/diff/36001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.h:12: #include
"base/message_loop/message_loop_proxy.h"
AFAIK you can fwd decl vs include

you may also be able to get rid of |media_loop_proxy_| -- see my comment in .cc

https://codereview.chromium.org/19534002/diff/36001/content/renderer/media/rt...
File content/renderer/media/rtc_video_decoder_factory.cc (right):

https://codereview.chromium.org/19534002/diff/36001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder_factory.cc:15: const
scoped_refptr<content::RendererGpuVideoDecoderFactories>&
don't need content namespace

https://codereview.chromium.org/19534002/diff/36001/content/renderer/media/rt...
File content/renderer/media/rtc_video_decoder_factory.h (right):

https://codereview.chromium.org/19534002/diff/36001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder_factory.h:26: const
scoped_refptr<content::RendererGpuVideoDecoderFactories>&
don't need content namespace

https://codereview.chromium.org/19534002/diff/36001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder_factory.h:41:
scoped_refptr<content::RendererGpuVideoDecoderFactories> gpu_factories_;
don't need content namespace

https://codereview.chromium.org/19534002/diff/36001/content/renderer/render_t...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/19534002/diff/36001/content/renderer/render_t...
content/renderer/render_thread_impl.cc:888:
scoped_refptr<content::RendererGpuVideoDecoderFactories>
don't need content namespace

https://codereview.chromium.org/19534002/diff/36001/content/renderer/render_t...
content/renderer/render_thread_impl.cc:894:
scoped_refptr<content::RendererGpuVideoDecoderFactories> gpu_factories;
you don't need content namespace

https://codereview.chromium.org/19534002/diff/36001/content/renderer/render_t...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/19534002/diff/36001/content/renderer/render_t...
content/renderer/render_thread_impl.h:259:
scoped_refptr<content::RendererGpuVideoDecoderFactories> GetGpuFactories(
don't need content::

but... why are we returning the impl vs the interface?

can we get away with using just the interface?

https://codereview.chromium.org/19534002/diff/36001/content/renderer/render_t...
content/renderer/render_thread_impl.h:260: scoped_refptr<base::MessageLoopProxy>
factories_loop);
update docs about |factories_loop|

even though with your latest PS |factories_loop| is always
GetMediaThreadMessageLoopProxy(), I'm OK leaving this as is otherwise it isn't
obvious at the call sites which thread this thing is running on

https://codereview.chromium.org/19534002/diff/36001/content/renderer/render_t...
content/renderer/render_thread_impl.h:260: scoped_refptr<base::MessageLoopProxy>
factories_loop);
this can be const-ref

https://codereview.chromium.org/19534002/diff/36001/media/filters/gpu_video_d...
File media/filters/gpu_video_decoder.h (right):

https://codereview.chromium.org/19534002/diff/36001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.h:66: virtual
scoped_refptr<base::MessageLoopProxy> GetMessageLoop() = 0;
can we remove this?

it looks like it's only used for DCHECK()'ing inside the ctor but the DCHECK()s
inside of RendererGVDF should catch the same bug

[wuchengli, 2013-07-20 06:04:13]

All done. PTAL.

https://codereview.chromium.org/19534002/diff/36001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/19534002/diff/36001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:503:
RenderThreadImpl::current()->GetGpuFactories(media_loop_proxy_);
On 2013/07/19 18:13:58, scherkus wrote:
> how about replacing media_loop_proxy_ with a call to
> RenderThreadImpl::current()->GetMediaThreadMessageLoopProxy() ?
Good idea. Done.

https://codereview.chromium.org/19534002/diff/36001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.h (right):

https://codereview.chromium.org/19534002/diff/36001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.h:12: #include
"base/message_loop/message_loop_proxy.h"
On 2013/07/19 18:13:58, scherkus wrote:
> AFAIK you can fwd decl vs include
> 
> you may also be able to get rid of |media_loop_proxy_| -- see my comment in
.cc

Done.

https://codereview.chromium.org/19534002/diff/36001/content/renderer/media/rt...
File content/renderer/media/rtc_video_decoder_factory.cc (right):

https://codereview.chromium.org/19534002/diff/36001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder_factory.cc:15: const
scoped_refptr<content::RendererGpuVideoDecoderFactories>&
On 2013/07/19 18:13:58, scherkus wrote:
> don't need content namespace
Changed to media::GpuVideoDecoder::Factories.

https://codereview.chromium.org/19534002/diff/36001/content/renderer/media/rt...
File content/renderer/media/rtc_video_decoder_factory.h (right):

https://codereview.chromium.org/19534002/diff/36001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder_factory.h:26: const
scoped_refptr<content::RendererGpuVideoDecoderFactories>&
On 2013/07/19 18:13:58, scherkus wrote:
> don't need content namespace
Changed back to media::GpuVideoDecoder::Factories.

https://codereview.chromium.org/19534002/diff/36001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder_factory.h:41:
scoped_refptr<content::RendererGpuVideoDecoderFactories> gpu_factories_;
On 2013/07/19 18:13:58, scherkus wrote:
> don't need content namespace
Changed back to media::GpuVideoDecoder::Factories.

https://codereview.chromium.org/19534002/diff/36001/content/renderer/render_t...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/19534002/diff/36001/content/renderer/render_t...
content/renderer/render_thread_impl.cc:888:
scoped_refptr<content::RendererGpuVideoDecoderFactories>
On 2013/07/19 18:13:58, scherkus wrote:
> don't need content namespace
Changed back to media::GpuVideoDecoder::Factories.

https://codereview.chromium.org/19534002/diff/36001/content/renderer/render_t...
content/renderer/render_thread_impl.cc:894:
scoped_refptr<content::RendererGpuVideoDecoderFactories> gpu_factories;
On 2013/07/19 18:13:58, scherkus wrote:
> you don't need content namespace
Changed back to media::GpuVideoDecoder::Factories.

https://codereview.chromium.org/19534002/diff/36001/content/renderer/render_t...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/19534002/diff/36001/content/renderer/render_t...
content/renderer/render_thread_impl.h:259:
scoped_refptr<content::RendererGpuVideoDecoderFactories> GetGpuFactories(
On 2013/07/19 18:13:58, scherkus wrote:
> don't need content::
> 
> but... why are we returning the impl vs the interface?
> 
> can we get away with using just the interface?
Yes. We can. I wasn't sure if it's good to put Clone in the interface because
RtcVideoDecoderFactory uses it but GpuVideoDecoder doesn't. I've added Clone to
the interface and change the return type here to the interface.

https://codereview.chromium.org/19534002/diff/36001/content/renderer/render_t...
content/renderer/render_thread_impl.h:260: scoped_refptr<base::MessageLoopProxy>
factories_loop);
On 2013/07/19 18:13:58, scherkus wrote:
> this can be const-ref

Done.

https://codereview.chromium.org/19534002/diff/36001/content/renderer/render_t...
content/renderer/render_thread_impl.h:260: scoped_refptr<base::MessageLoopProxy>
factories_loop);
On 2013/07/19 18:13:58, scherkus wrote:
> update docs about |factories_loop|
> 
> even though with your latest PS |factories_loop| is always
> GetMediaThreadMessageLoopProxy(), I'm OK leaving this as is otherwise it isn't
> obvious at the call sites which thread this thing is running on

Done.

https://codereview.chromium.org/19534002/diff/36001/media/filters/gpu_video_d...
File media/filters/gpu_video_decoder.h (right):

https://codereview.chromium.org/19534002/diff/36001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.h:66: virtual
scoped_refptr<base::MessageLoopProxy> GetMessageLoop() = 0;
On 2013/07/19 18:13:58, scherkus wrote:
> can we remove this?
> 
> it looks like it's only used for DCHECK()'ing inside the ctor but the
DCHECK()s
> inside of RendererGVDF should catch the same bug
It's only used by the DCHECK() inside the ctor. But the DCHECK()s in RGVDF do
not check the thread. RGVDF offers both sync and async paths for GpuVideoDecoder
and RtcVideoDecoder, respectively.

This can still be removed if we think some comments are enough to replace the
DCHECK() inside the ctor. I prefer keeping it to prevent bugs.

[scherkus (not reviewing), 2013-07-22 17:56:39]

lgtm w/ one nit

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/rt...
File content/renderer/media/rtc_video_decoder_factory.cc (right):

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder_factory.cc:32:
RTCVideoDecoder::Create(type, vda_loop_proxy_, gpu_factories_->Clone());
Considering this is a problem unique to content/ and RGVDF, I feel like Clone()
should live on RendererGpuVideoDecoderFactories as opposed to on the Factories
interface itself

This class can then hold onto a RGVDF and call its Clone() method.

This way the scope of Clone() is restricted to the places where it needs to be
called (i.e., here) instead of being accessible to the end users (i.e., RTCVD
and GVD)

[piman, 2013-07-22 19:05:03]

lgtm

[Ami GONE FROM CHROMIUM, 2013-07-22 19:46:21]

I love this CL!

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/re...
File content/renderer/media/renderer_gpu_video_decoder_factories.cc (right):

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/re...
content/renderer/media/renderer_gpu_video_decoder_factories.cc:200: return;
message_loop_async_waiter_.Reset();

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/re...
content/renderer/media/renderer_gpu_video_decoder_factories.cc:242:
AsyncReadPixels(texture_id, texture_target, size);
message_loop_async_waiter_.Reset();

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/re...
content/renderer/media/renderer_gpu_video_decoder_factories.cc:322: new
RendererGpuVideoDecoderFactories();
AFAICT you can reuse the non-trivial ctor and then just set main_message_loop_
explicitly.
(if you do this then you can revert the IMPLICIT->COPY_ASSIGN diff in the
header)

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/rt...
File content/renderer/media/rtc_video_decoder.h (right):

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.h:153: void ReleaseInternal();
Any reason for this, given it's no different from DestroyVDA()?

(if you keep it, you should doco its purpose)

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/rt...
File content/renderer/media/rtc_video_decoder_factory.cc (left):

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder_factory.cc:21: // TODO(wuchengli):
remove this restriction.
Yay!

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/rt...
File content/renderer/media/rtc_video_decoder_factory.cc (right):

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder_factory.cc:39: if
(!vda_loop_proxy_->DeleteSoon(FROM_HERE, decoder))
You might enjoy
https://plus.sandbox.google.com/104229030418342490750/posts/ezkMjb2k5b8

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/rt...
File content/renderer/media/rtc_video_decoder_unittest.cc (right):

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder_unittest.cc:160: 
Where's the test that multiple decoders can be created? :)

https://codereview.chromium.org/19534002/diff/60001/media/filters/gpu_video_d...
File media/filters/gpu_video_decoder.cc (left):

https://codereview.chromium.org/19534002/diff/60001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:23: // Proxies calls to a
VideoDecodeAccelerator::Client from the calling thread to
This makes me ever so happy!

https://codereview.chromium.org/19534002/diff/60001/media/filters/gpu_video_d...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/19534002/diff/60001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:156:
factories_->CreateVideoDecodeAccelerator(config.profile(), this);
nit: you don't need this intermediate variable; you can stick the result of
CreateVideoDecodeAccelerator straight into vda_.

https://codereview.chromium.org/19534002/diff/60001/media/filters/gpu_video_d...
File media/filters/gpu_video_decoder.h (right):

https://codereview.chromium.org/19534002/diff/60001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.h:40: // TODO(wuchengli): move this out of
GpuVideoDecoder because RTCVideoDecoder
Rebase makes this unnecessary?

https://codereview.chromium.org/19534002/diff/60001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.h:83: // |message_loop| should be the same as
the one running |factories|.
Why not drop this param and just use factories->GetMessageLoop().get() instead?

[wuchengli, 2013-07-23 16:29:28]

All done. PTAL.

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/re...
File content/renderer/media/renderer_gpu_video_decoder_factories.cc (right):

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/re...
content/renderer/media/renderer_gpu_video_decoder_factories.cc:200: return;
On 2013/07/22 19:46:21, Ami Fischman wrote:
> message_loop_async_waiter_.Reset();

Done.

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/re...
content/renderer/media/renderer_gpu_video_decoder_factories.cc:242:
AsyncReadPixels(texture_id, texture_target, size);
On 2013/07/22 19:46:21, Ami Fischman wrote:
> message_loop_async_waiter_.Reset();

Done.

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/re...
content/renderer/media/renderer_gpu_video_decoder_factories.cc:322: new
RendererGpuVideoDecoderFactories();
On 2013/07/22 19:46:21, Ami Fischman wrote:
> AFAICT you can reuse the non-trivial ctor and then just set main_message_loop_
> explicitly.
> (if you do this then you can revert the IMPLICIT->COPY_ASSIGN diff in the
> header)
Non-trial ctor takes raw pointer of WebGraphicsContext3DCommandBufferImpl. But
Clone() is called on libjingle thread, not |message_loop_| (media thread). So we
cannot dereference the weak pointer |context_| here.

In my understanding, make a copy of WeakPtr on a different thread is OK.

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/rt...
File content/renderer/media/rtc_video_decoder.h (right):

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.h:153: void ReleaseInternal();
On 2013/07/22 19:46:21, Ami Fischman wrote:
> Any reason for this, given it's no different from DestroyVDA()?
> 
> (if you keep it, you should doco its purpose)
You are right. ReleaseInternal is not required.

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/rt...
File content/renderer/media/rtc_video_decoder_factory.cc (left):

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder_factory.cc:21: // TODO(wuchengli):
remove this restriction.
On 2013/07/22 19:46:21, Ami Fischman wrote:
> Yay!
:)

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/rt...
File content/renderer/media/rtc_video_decoder_factory.cc (right):

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder_factory.cc:32:
RTCVideoDecoder::Create(type, vda_loop_proxy_, gpu_factories_->Clone());
On 2013/07/22 17:56:40, scherkus wrote:
> Considering this is a problem unique to content/ and RGVDF, I feel like
Clone()
> should live on RendererGpuVideoDecoderFactories as opposed to on the Factories
> interface itself
> 
> This class can then hold onto a RGVDF and call its Clone() method.
> 
> This way the scope of Clone() is restricted to the places where it needs to be
> called (i.e., here) instead of being accessible to the end users (i.e., RTCVD
> and GVD)
Done. That's what I wasn't sure about in
https://codereview.chromium.org/19534002/diff/36001/content/renderer/render_t....
I've moved Clone() out of the interface and made
RenderThreadImpl::GetGpuFactories return RendererGpuVideoDecoderFactories.

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder_factory.cc:39: if
(!vda_loop_proxy_->DeleteSoon(FROM_HERE, decoder))
On 2013/07/22 19:46:21, Ami Fischman wrote:
> You might enjoy
> https://plus.sandbox.google.com/104229030418342490750/posts/ezkMjb2k5b8
Haha. I copied the anti-pattern from other places.

I've removed "delete decoder" because media thread won't stop until this
returns.

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/rt...
File content/renderer/media/rtc_video_decoder_unittest.cc (right):

https://codereview.chromium.org/19534002/diff/60001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder_unittest.cc:160: 
On 2013/07/22 19:46:21, Ami Fischman wrote:
> Where's the test that multiple decoders can be created? :)
Good point. That should be a new test file because decoders are created in
rtc_video_decoder_factory.cc. I tried and found lots of code had to change. It
can be another CL. I've added TODO in rtc_video_decoder_factory.h.

https://codereview.chromium.org/19534002/diff/60001/media/filters/gpu_video_d...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/19534002/diff/60001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:156:
factories_->CreateVideoDecodeAccelerator(config.profile(), this);
On 2013/07/22 19:46:21, Ami Fischman wrote:
> nit: you don't need this intermediate variable; you can stick the result of
> CreateVideoDecodeAccelerator straight into vda_.

Done.

https://codereview.chromium.org/19534002/diff/60001/media/filters/gpu_video_d...
File media/filters/gpu_video_decoder.h (right):

https://codereview.chromium.org/19534002/diff/60001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.h:40: // TODO(wuchengli): move this out of
GpuVideoDecoder because RTCVideoDecoder
On 2013/07/22 19:46:21, Ami Fischman wrote:
> Rebase makes this unnecessary?
I couldn't submit that CL today because of submit try bots failures. Some other
people were blocked by the same failure. I'll rebase my CL as soon as it is
submitted.

https://codereview.chromium.org/19534002/diff/60001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.h:83: // |message_loop| should be the same as
the one running |factories|.
On 2013/07/22 19:46:21, Ami Fischman wrote:
> Why not drop this param and just use factories->GetMessageLoop().get()
instead?
That was recommended by Andrew.
https://codereview.chromium.org/19534002/diff/2002/media/filters/gpu_video_de...

Andrew. I've changed this back. First I feel using the message loop of
|factories| is natural because it is the "factory" of the video decoder. Also we
can remove DCHECKs and the lines of code are less. Second those
DCHECK(gvd_loop_proxy_->BelongsToCurrentThread()) in GpuVideoDecoder methods
will still require |factories| must have been created to work on the media
message loop.

[Ami GONE FROM CHROMIUM, 2013-07-23 18:13:27]

LGTM

https://codereview.chromium.org/19534002/diff/94001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/19534002/diff/94001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
In future please rebase *after* addressing review comments.

https://codereview.chromium.org/19534002/diff/94001/media/filters/gpu_video_d...
File media/filters/gpu_video_decoder.h (right):

https://codereview.chromium.org/19534002/diff/94001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.h:81: GpuVideoDecoder(const
scoped_refptr<Factories>& factories);
explicit

[wuchengli, 2013-07-24 08:54:14]

Done. Committing.

https://codereview.chromium.org/19534002/diff/94001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/19534002/diff/94001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2013/07/23 18:13:28, Ami Fischman wrote:
> In future please rebase *after* addressing review comments.
Got it.

What happened was that I synced the code to rebase another CL in another branch.
When I switched back to this branch, I couldn't pass build/gyp_chromium without
rebase. I'll be more careful in the future.

https://codereview.chromium.org/19534002/diff/94001/media/filters/gpu_video_d...
File media/filters/gpu_video_decoder.h (right):

https://codereview.chromium.org/19534002/diff/94001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.h:81: GpuVideoDecoder(const
scoped_refptr<Factories>& factories);
On 2013/07/23 18:13:28, Ami Fischman wrote:
> explicit

Done.

[commit-bot: I haz the power, 2013-07-24 09:02:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wuchengli@chromium.org/19534002/109001

[commit-bot: I haz the power, 2013-07-24 12:22:56]

Change committed as 213412