Fold media::MessageLoopFactoryImpl into media::MessageLoopFactory.

Fold media::MessageLoopFactoryImpl into media::MessageLoopFactory.

We haven't had a second implementation of the interface since MessageLoopFactory was added in r71548.

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

[scherkus (not reviewing), 2012-03-05 18:05:14]

whipped this up on the plane ride

[acolwell GONE FROM CHROMIUM, 2012-03-05 18:15:04]

On 2012/03/05 18:05:14, scherkus wrote:
> whipped this up on the plane ride

LGTM

[scherkus (not reviewing), 2012-03-05 23:05:43]

tony: OWNERS for ./webkit/*
jam: OWNERS for ./content/renderer/*

[tony, 2012-03-05 23:07:09]

webkit LGTM

[Ami GONE FROM CHROMIUM, 2012-03-05 23:20:15]

drive-by for posterity; apparently this class was originally born of a notion
that the same MLFactory would be passed to multiple pipelines and threads
re-used, but that has never come to pass, and there's no reason to think this is
the right API for that.  Noting some things I'd like to see done differently if
this is touched again.

https://chromiumcodereview.appspot.com/9597016/diff/4001/media/base/message_l...
File media/base/message_loop_factory.cc (right):

https://chromiumcodereview.appspot.com/9597016/diff/4001/media/base/message_l...
media/base/message_loop_factory.cc:14: base::AutoLock auto_lock(lock_);
Locking in the dtor is a pretty good indicator that things are borked; if
another method invocation races with this, then merely holding it off until the
dtor completes will result in that method blowing up (attempting to use a
just-deleted |*this|).

https://chromiumcodereview.appspot.com/9597016/diff/4001/media/base/message_l...
media/base/message_loop_factory.cc:42: return NULL;
This isn't part of the doco'd interface of the class; DCHECK_NE(name, "") looks
better to me (and reduces the error checking you need to do in
GetMessageLoop{,Proxy}() above).

https://chromiumcodereview.appspot.com/9597016/diff/4001/media/base/message_l...
media/base/message_loop_factory.cc:46: ThreadMap::iterator it =
thread_map_.find(name);
We don't actually make use of this functionality anywhere, so IMO this whole
class should be dropped in favor of creating threads at GetThreads() call-sites
and passing their ownership to the objects that live on them.

[Ami GONE FROM CHROMIUM, 2012-03-05 23:22:39]

(filed previous comment as http://crbug.com/116873)

[scherkus (not reviewing), 2012-03-05 23:29:35]

https://chromiumcodereview.appspot.com/9597016/diff/4001/media/base/message_l...
File media/base/message_loop_factory.cc (right):

https://chromiumcodereview.appspot.com/9597016/diff/4001/media/base/message_l...
media/base/message_loop_factory.cc:14: base::AutoLock auto_lock(lock_);
On 2012/03/05 23:20:15, Ami Fischman wrote:
> Locking in the dtor is a pretty good indicator that things are borked; if
> another method invocation races with this, then merely holding it off until
the
> dtor completes will result in that method blowing up (attempting to use a
> just-deleted |*this|).

I don't think things are borked rather coded defensively (but acolwell can chime
in seeing as he wrote the initial CL!)

https://chromiumcodereview.appspot.com/9597016/diff/4001/media/base/message_l...
media/base/message_loop_factory.cc:42: return NULL;
On 2012/03/05 23:20:15, Ami Fischman wrote:
> This isn't part of the doco'd interface of the class; DCHECK_NE(name, "")
looks
> better to me (and reduces the error checking you need to do in
> GetMessageLoop{,Proxy}() above).


"NULL is returned if |name| is an empty string" ??

although I'm not sure when/why we'd do that

https://chromiumcodereview.appspot.com/9597016/diff/4001/media/base/message_l...
media/base/message_loop_factory.cc:46: ThreadMap::iterator it =
thread_map_.find(name);
On 2012/03/05 23:20:15, Ami Fischman wrote:
> We don't actually make use of this functionality anywhere, so IMO this whole
> class should be dropped in favor of creating threads at GetThreads()
call-sites
> and passing their ownership to the objects that live on them.

yup but ideally going further by not creating additional threads per object
(that was the original intent of MLF)

[acolwell GONE FROM CHROMIUM, 2012-03-05 23:53:19]

https://chromiumcodereview.appspot.com/9597016/diff/4001/media/base/message_l...
File media/base/message_loop_factory.cc (right):

https://chromiumcodereview.appspot.com/9597016/diff/4001/media/base/message_l...
media/base/message_loop_factory.cc:14: base::AutoLock auto_lock(lock_);
This was an attempt at defensive coding. Nothing should be calling GetThread()
when the destructor gets called. I agree that things will go BOOM if that does
happen though. The original intent was to just protect all thread_map_ accesses
with the lock.

On 2012/03/05 23:29:35, scherkus wrote:
> On 2012/03/05 23:20:15, Ami Fischman wrote:
> > Locking in the dtor is a pretty good indicator that things are borked; if
> > another method invocation races with this, then merely holding it off until
> the
> > dtor completes will result in that method blowing up (attempting to use a
> > just-deleted |*this|).
> 
> I don't think things are borked rather coded defensively (but acolwell can
chime
> in seeing as he wrote the initial CL!)

https://chromiumcodereview.appspot.com/9597016/diff/4001/media/base/message_l...
media/base/message_loop_factory.cc:42: return NULL;
DCHECK_NE() seems good to me. I think this was one of my earliest CLs so I was
much less DCHECK happy.

On 2012/03/05 23:29:35, scherkus wrote:
> On 2012/03/05 23:20:15, Ami Fischman wrote:
> > This isn't part of the doco'd interface of the class; DCHECK_NE(name, "")
> looks
> > better to me (and reduces the error checking you need to do in
> > GetMessageLoop{,Proxy}() above).
> 
> 
> "NULL is returned if |name| is an empty string" ??
> 
> although I'm not sure when/why we'd do that

https://chromiumcodereview.appspot.com/9597016/diff/4001/media/base/message_l...
media/base/message_loop_factory.cc:46: ThreadMap::iterator it =
thread_map_.find(name);
So the original intent was to share threads across multiple audio & video
decoders. Obviously that functionality hasn't been implemented over the last
year. I'm open to replacing this with something better. 

On 2012/03/05 23:29:35, scherkus wrote:
> On 2012/03/05 23:20:15, Ami Fischman wrote:
> > We don't actually make use of this functionality anywhere, so IMO this whole
> > class should be dropped in favor of creating threads at GetThreads()
> call-sites
> > and passing their ownership to the objects that live on them.
> 
> yup but ideally going further by not creating additional threads per object
> (that was the original intent of MLF)

[jam, 2012-03-06 01:05:59]

lgtm

[scherkus (not reviewing), 2012-03-06 01:25:01]

to close the loop: I added a TODO w/ the bug number and replaced the "" string
check w/ a DCHECK + updated docs

thanks for the reviews y'all

[xhwang, 2012-07-12 22:22:20]

On 2012/03/05 23:53:19, acolwell wrote:
>
https://chromiumcodereview.appspot.com/9597016/diff/4001/media/base/message_l...
> File media/base/message_loop_factory.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9597016/diff/4001/media/base/message_l...
> media/base/message_loop_factory.cc:14: base::AutoLock auto_lock(lock_);
> This was an attempt at defensive coding. Nothing should be calling GetThread()
> when the destructor gets called. I agree that things will go BOOM if that does
> happen though. The original intent was to just protect all thread_map_
accesses
> with the lock.
> 
> On 2012/03/05 23:29:35, scherkus wrote:
> > On 2012/03/05 23:20:15, Ami Fischman wrote:
> > > Locking in the dtor is a pretty good indicator that things are borked; if
> > > another method invocation races with this, then merely holding it off
until
> > the
> > > dtor completes will result in that method blowing up (attempting to use a
> > > just-deleted |*this|).
> > 
> > I don't think things are borked rather coded defensively (but acolwell can
> chime
> > in seeing as he wrote the initial CL!)
> 
>
https://chromiumcodereview.appspot.com/9597016/diff/4001/media/base/message_l...
> media/base/message_loop_factory.cc:42: return NULL;
> DCHECK_NE() seems good to me. I think this was one of my earliest CLs so I was
> much less DCHECK happy.
> 
> On 2012/03/05 23:29:35, scherkus wrote:
> > On 2012/03/05 23:20:15, Ami Fischman wrote:
> > > This isn't part of the doco'd interface of the class; DCHECK_NE(name, "")
> > looks
> > > better to me (and reduces the error checking you need to do in
> > > GetMessageLoop{,Proxy}() above).
> > 
> > 
> > "NULL is returned if |name| is an empty string" ??
> > 
> > although I'm not sure when/why we'd do that
> 
>
https://chromiumcodereview.appspot.com/9597016/diff/4001/media/base/message_l...
> media/base/message_loop_factory.cc:46: ThreadMap::iterator it =
> thread_map_.find(name);
> So the original intent was to share threads across multiple audio & video
> decoders. Obviously that functionality hasn't been implemented over the last
> year. I'm open to replacing this with something better. 

Now I need to share threads between the decryptor and decoder, so this
functionality isn't all useless. The trouble I am having is that all decoders
choose their own names, e.g. "VideoDecoderThread" for FFVD, "GpuVideoDecoder"
for GVD, "CaptureVideoDecoderThread" for CpatureVideoDecoder and
"CaptureVideoDecoder" for VideoFrameGenerator! So there's no way for the
decryptor or whoever wants to use the real "video decoder thread" to get access
of it. I chatted with scherkus@ offline and he has some plan to replace this
code. Before that's implemented and make me unblocked, I'am thinking of making
GetThread()/GetMessageLoop() enum based instead of string based:

enum ThreadId {
  AudioDecoderThread,
  VideoDecoderThread,
  ...
}

base::Thread* GetThread(ThreadId id);

So we can only create/get certain threads and any object can get certain thread
it wants through the message_loop_factory object.

Any comments?

> 
> On 2012/03/05 23:29:35, scherkus wrote:
> > On 2012/03/05 23:20:15, Ami Fischman wrote:
> > > We don't actually make use of this functionality anywhere, so IMO this
whole
> > > class should be dropped in favor of creating threads at GetThreads()
> > call-sites
> > > and passing their ownership to the objects that live on them.
> > 
> > yup but ideally going further by not creating additional threads per object
> > (that was the original intent of MLF)

[Ami GONE FROM CHROMIUM, 2012-07-12 22:25:26]

On Thu, Jul 12, 2012 at 3:22 PM, <xhwang@chromium.org> wrote:

> Now I need to share threads between the decryptor and decoder, so this
> functionality isn't all useless. The trouble I am having is that all
> decoders
> choose their own names, e.g. "VideoDecoderThread" for FFVD,
> "GpuVideoDecoder"
> for GVD, "CaptureVideoDecoderThread" for CpatureVideoDecoder and
> "CaptureVideoDecoder" for VideoFrameGenerator! So there's no way for the
> decryptor or whoever wants to use the real "video decoder thread" to get
> access
> of it. I chatted with scherkus@ offline and he has some plan to replace
> this
> code. Before that's implemented and make me unblocked, I'am thinking of
> making
> GetThread()/GetMessageLoop() enum based instead of string based:
>
> enum ThreadId {
>   AudioDecoderThread,
>   VideoDecoderThread,
>   ...
> }
>
> base::Thread* GetThread(ThreadId id);
>
> So we can only create/get certain threads and any object can get certain
> thread
> it wants through the message_loop_factory object.
>
> Any comments?
>

My comment is that I hate this plan ;)
Can whatever Initialize()'s the decryptor query the decoder for its
loop/proxy and hand that to the decryptor?

-a

[xhwang1, 2012-07-12 22:37:46]

On Thu, Jul 12, 2012 at 3:25 PM, Ami Fischman <fischman@chromium.org> wrote:

> On Thu, Jul 12, 2012 at 3:22 PM, <xhwang@chromium.org> wrote:
>
>> Now I need to share threads between the decryptor and decoder, so this
>> functionality isn't all useless. The trouble I am having is that all
>> decoders
>> choose their own names, e.g. "VideoDecoderThread" for FFVD,
>> "GpuVideoDecoder"
>> for GVD, "CaptureVideoDecoderThread" for CpatureVideoDecoder and
>> "CaptureVideoDecoder" for VideoFrameGenerator! So there's no way for the
>> decryptor or whoever wants to use the real "video decoder thread" to get
>> access
>> of it. I chatted with scherkus@ offline and he has some plan to replace
>> this
>> code. Before that's implemented and make me unblocked, I'am thinking of
>> making
>> GetThread()/GetMessageLoop() enum based instead of string based:
>>
>> enum ThreadId {
>>   AudioDecoderThread,
>>   VideoDecoderThread,
>>   ...
>> }
>>
>> base::Thread* GetThread(ThreadId id);
>>
>> So we can only create/get certain threads and any object can get certain
>> thread
>> it wants through the message_loop_factory object.
>>
>> Any comments?
>>
>
> My comment is that I hate this plan ;)
> Can whatever Initialize()'s the decryptor query the decoder for its
> loop/proxy and hand that to the decryptor?
>

I know you hate it :)  By "query the decoder" do you mean to add something
like "GetMessageLoop()" in media::VideoDecoder? That'll work too.

>
> -a
>

[Ami GONE FROM CHROMIUM, 2012-07-12 22:40:00]

On Thu, Jul 12, 2012 at 3:37 PM, Xiaohan Wang (王消寒) <xhwang@google.com>wrote:

> I know you hate it :)  By "query the decoder" do you mean to add something
> like "GetMessageLoop()" in media::VideoDecoder? That'll work too.


Yes, that's what I meant.
Of course that's terrible too (it hardcodes the expectation that the
videodecoder always has a ML), but that's less terrible than asking for
threads by name (IMO).

What about using the render thread for this stuff?  You just need the ML as
a trampoline site, right?
I.e. ChildThread::current()

-a

[Ami GONE FROM CHROMIUM, 2012-07-13 22:55:10]

To close out the reviewlog thread, xhwang@, acolwell@, scherkus@, and I are
discussing the future of threads in media code.  If anyone else on the reviewlog
here is interested in following along, drop one of us a line.