Create video and audio decoder threads on demand.

Create video and audio decoder threads on demand.
Instead of creating these worker threads when the decoder objects
are instantiated, we now do it when Initialize is called.
Doing this avoids spinning a video decoder thread for an audio tag.

BUG=114699, 116873
TEST=Thread count in the renderer process goes up by 3 and not 4 when an audio element is created.

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

[Ami GONE FROM CHROMIUM, 2012-03-08 16:32:07]

Nice idea!

I think you can simplify the CL significantly though; see comments below. 
Obviously, they apply equally to FFmpegVideoDecoder.

https://chromiumcodereview.appspot.com/9632024/diff/1/media/filters/ffmpeg_au...
File media/filters/ffmpeg_audio_decoder.h (right):

https://chromiumcodereview.appspot.com/9632024/diff/1/media/filters/ffmpeg_au...
media/filters/ffmpeg_audio_decoder.h:22: FFmpegAudioDecoder(MessageLoopFactory*
loop_factory,
You could avoid the need for the MessageLoopFactory changes and the need for
MessageLoopFactoryForTest by instead passing a Callback<MessageLoop*()> to the
decoders, built with something like:
base::Bind(&MessageLoopFactory::GetMessageLoop, loop_factory,
"FooDecoderThread")
for the "real" callsite and 
template<class T> T Identity(T t) { return t; }
base::Bind(&Identity, MessageLoop::current())
for the test callsites.

https://chromiumcodereview.appspot.com/9632024/diff/1/media/filters/ffmpeg_au...
media/filters/ffmpeg_audio_decoder.h:60: MessageLoop* EnsureMessageLoop();
This idiom seems to me either racy or unnecessary.
If one is guaranteed that Initialize() is called before any other method, and
that that "before" carries memory-barrier implication, then every method other
than Initialize can simply trust & use message_loop_. 
If there is no such happens-before relationship then nothing prevents
EnsureMessageLoop() from running concurrently in different threads and returning
different values.

I believe the former is the case (and all methods other than Initialize() in the
.cc file can be reverted, and EML() removed).

[scherkus (not reviewing), 2012-03-08 19:12:44]

cool!

the much-more-awesome fix is to have a single thread and inject the msgloop via
ctor but I'll take this improvement :)

I +1 the base::Bind() suggestion to initialize a message_loop_ member variable
and also to just keep things simple by crashing on a NULL message_loop_ (you
must call Initialize() before operating on the object)

If you use base::Bind() you can also get rid of the virtual stuff on
MessageLoopFactory etc etc etc

[tommi (sloooow) - chröme, 2012-03-09 10:07:13]

Thanks for suggesting the callback solution.  The change is much simpler now.

https://chromiumcodereview.appspot.com/9632024/diff/1/media/filters/ffmpeg_au...
File media/filters/ffmpeg_audio_decoder.h (right):

https://chromiumcodereview.appspot.com/9632024/diff/1/media/filters/ffmpeg_au...
media/filters/ffmpeg_audio_decoder.h:22: FFmpegAudioDecoder(MessageLoopFactory*
loop_factory,
On 2012/03/08 16:32:07, Ami Fischman wrote:
> You could avoid the need for the MessageLoopFactory changes and the need for
> MessageLoopFactoryForTest by instead passing a Callback<MessageLoop*()> to the
> decoders, built with something like:
> base::Bind(&MessageLoopFactory::GetMessageLoop, loop_factory,
> "FooDecoderThread")
> for the "real" callsite and 
> template<class T> T Identity(T t) { return t; }
> base::Bind(&Identity, MessageLoop::current())
> for the test callsites.

That's so clever it can't possibly work! hehe, much better. Done!

https://chromiumcodereview.appspot.com/9632024/diff/1/media/filters/ffmpeg_au...
media/filters/ffmpeg_audio_decoder.h:60: MessageLoop* EnsureMessageLoop();
On 2012/03/08 16:32:07, Ami Fischman wrote:
> This idiom seems to me either racy or unnecessary.
> If one is guaranteed that Initialize() is called before any other method, and
> that that "before" carries memory-barrier implication, then every method other
> than Initialize can simply trust & use message_loop_. 
> If there is no such happens-before relationship then nothing prevents
> EnsureMessageLoop() from running concurrently in different threads and
returning
> different values.
> 
> I believe the former is the case (and all methods other than Initialize() in
the
> .cc file can be reverted, and EML() removed).

Removed EML and now only initialize message_loop_ inside Initialize.
EML wasn't racy though since MessageLoopFactory is thread safe and always
returns the same pointer given the same key.

[Ami GONE FROM CHROMIUM, 2012-03-09 16:15:05]

Basically LGTM; thanks for simplifying.
I made some comments on only video or audio side that apply equally to the
other.

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
File media/filters/ffmpeg_audio_decoder.h (right):

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
media/filters/ffmpeg_audio_decoder.h:19: class MessageLoopFactory;
Unnecessary

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
File media/filters/ffmpeg_audio_decoder_unittest.cc (right):

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
media/filters/ffmpeg_audio_decoder_unittest.cc:23: namespace {
s/anon namespace/static/ in media code

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
File media/filters/ffmpeg_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
media/filters/ffmpeg_video_decoder.cc:73: message_loop_ =
message_loop_cb_.Run();
add message_loop_cb_.Reset(); for clarity after this?

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
media/filters/ffmpeg_video_decoder.cc:76:
base::Bind(&FFmpegVideoDecoder::Initialize, this,
This indentation violates the google style guide.  Either revert or move the
preceding FROM_HERE to this style indent too.
(I prefer revert to match the rest of media code)

Ditto the rest of this CL.

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
File media/filters/ffmpeg_video_decoder.h (right):

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
media/filters/ffmpeg_video_decoder.h:23: class MessageLoopFactory;
unnecessary

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
media/filters/ffmpeg_video_decoder.h:74: base::Callback<MessageLoop*()>
message_loop_cb_;
s/message_loop_cb_/message_loop_factory_cb_/
and add a comment that this is !is_null() iff Initialize() hasn't been called
yet.

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
media/filters/ffmpeg_video_decoder_unittest.cc:32: namespace {
media code eschews anonymous namespaces in favor of statics.

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
media/filters/ffmpeg_video_decoder_unittest.cc:56: : decoder_(new
FFmpegVideoDecoder(base::Bind(&Identity<MessageLoop*>,
I'm surprised the <MessageLoop*> part is necessary.

[scherkus (not reviewing), 2012-03-09 20:01:16]

LGTM mod similar style-y bits

thanks!

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
media/filters/ffmpeg_audio_decoder.cc:11: #include
"media/base/message_loop_factory.h"
not needed

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
media/filters/ffmpeg_audio_decoder.cc:78: message_loop_->PostTask(FROM_HERE,
style -- I'd do the following:

message_loop_->PostTask(FROM_HERE, base::Bind(
____&FFmpegAudioDecoder::DoInitialize, ....

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
media/filters/ffmpeg_audio_decoder.cc:86: message_loop_->PostTask(FROM_HERE,
ditto here + below (i.e., revert these edits)

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
File media/filters/ffmpeg_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
media/filters/ffmpeg_video_decoder.cc:15: #include
"media/base/message_loop_factory.h"
not needed

[tommi (sloooow) - chröme, 2012-03-11 18:49:36]

all done.

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
media/filters/ffmpeg_audio_decoder.cc:11: #include
"media/base/message_loop_factory.h"
On 2012/03/09 20:01:16, scherkus wrote:
> not needed

Done.

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
media/filters/ffmpeg_audio_decoder.cc:78: message_loop_->PostTask(FROM_HERE,
On 2012/03/09 20:01:16, scherkus wrote:
> style -- I'd do the following:
> 
> message_loop_->PostTask(FROM_HERE, base::Bind(
> ____&FFmpegAudioDecoder::DoInitialize, ....

Done.

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
media/filters/ffmpeg_audio_decoder.cc:86: message_loop_->PostTask(FROM_HERE,
On 2012/03/09 20:01:16, scherkus wrote:
> ditto here + below (i.e., revert these edits)

Done.

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
File media/filters/ffmpeg_audio_decoder.h (right):

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
media/filters/ffmpeg_audio_decoder.h:19: class MessageLoopFactory;
On 2012/03/09 16:15:06, Ami Fischman wrote:
> Unnecessary

Done.

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
File media/filters/ffmpeg_audio_decoder_unittest.cc (right):

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
media/filters/ffmpeg_audio_decoder_unittest.cc:23: namespace {
On 2012/03/09 16:15:06, Ami Fischman wrote:
> s/anon namespace/static/ in media code

Done.

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
File media/filters/ffmpeg_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
media/filters/ffmpeg_video_decoder.cc:15: #include
"media/base/message_loop_factory.h"
On 2012/03/09 20:01:16, scherkus wrote:
> not needed

Done.

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
media/filters/ffmpeg_video_decoder.cc:73: message_loop_ =
message_loop_cb_.Run();
On 2012/03/09 16:15:06, Ami Fischman wrote:
> add message_loop_cb_.Reset(); for clarity after this?

Done.

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
media/filters/ffmpeg_video_decoder.cc:76:
base::Bind(&FFmpegVideoDecoder::Initialize, this,
On 2012/03/09 16:15:06, Ami Fischman wrote:
> This indentation violates the google style guide.  Either revert or move the
> preceding FROM_HERE to this style indent too.
> (I prefer revert to match the rest of media code)
> 
> Ditto the rest of this CL.

Done.

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
File media/filters/ffmpeg_video_decoder.h (right):

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
media/filters/ffmpeg_video_decoder.h:23: class MessageLoopFactory;
On 2012/03/09 16:15:06, Ami Fischman wrote:
> unnecessary

Done.

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
media/filters/ffmpeg_video_decoder.h:74: base::Callback<MessageLoop*()>
message_loop_cb_;
On 2012/03/09 16:15:06, Ami Fischman wrote:
> s/message_loop_cb_/message_loop_factory_cb_/
> and add a comment that this is !is_null() iff Initialize() hasn't been called
> yet.

Done.

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

https://chromiumcodereview.appspot.com/9632024/diff/7001/media/filters/ffmpeg...
media/filters/ffmpeg_video_decoder_unittest.cc:32: namespace {
On 2012/03/09 16:15:06, Ami Fischman wrote:
> media code eschews anonymous namespaces in favor of statics.

Done.

[Ami GONE FROM CHROMIUM, 2012-03-11 23:43:12]

https://chromiumcodereview.appspot.com/9632024/diff/18002/media/filters/ffmpe...
File media/filters/ffmpeg_decoder_unittest.h (right):

https://chromiumcodereview.appspot.com/9632024/diff/18002/media/filters/ffmpe...
media/filters/ffmpeg_decoder_unittest.h:10: template<class T> T Identity(T t) {
return t; }
I think you misunderstood my comment about namespace/static.
My point was that instead of using an anonymous namespace:

namespace {
template<class T> T Identity(T t) { return t; }
}

media code makes such things file-static:
template<class T> static T Identity(T t) { return t; }

, not that you should move it into the media namespace (or need a new .h).
This function is simple enough that duplicating its code in two test.cc files
seems fine to me.

[tommi (sloooow) - chröme, 2012-03-12 10:07:57]

On 2012/03/11 23:43:12, Ami Fischman wrote:
>
https://chromiumcodereview.appspot.com/9632024/diff/18002/media/filters/ffmpe...
> File media/filters/ffmpeg_decoder_unittest.h (right):
> 
>
https://chromiumcodereview.appspot.com/9632024/diff/18002/media/filters/ffmpe...
> media/filters/ffmpeg_decoder_unittest.h:10: template<class T> T Identity(T t)
{
> return t; }
> I think you misunderstood my comment about namespace/static.
> My point was that instead of using an anonymous namespace:
> 
> namespace {
> template<class T> T Identity(T t) { return t; }
> }
> 
> media code makes such things file-static:
> template<class T> static T Identity(T t) { return t; }
> 
> , not that you should move it into the media namespace (or need a new .h).
> This function is simple enough that duplicating its code in two test.cc files
> seems fine to me.

Ah, sorry about that Ami.  I'll fix it in a subsequent cl.