Remove DemuxerFactory and URL parameter from Pipeline.

Remove DemuxerFactory and URL parameter from Pipeline.

Since Demuxers still require asynchronous initialization DemuxerFactory::Build() has been replaced with Demuxer::Initialize(). Since FFmpegDemuxer is the only Demuxer requiring a DataSource it is now passed in via FFmpegDemuxer's constructor.

Now that Demuxer::set_host() is guaranteed to be called prior to initialization we're able to tighten up some code inside ChunkDemuxer. We should still nuke set_host() (see bug 111585) but I'll leave that for a future CL.

BUG=110804, 110809
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=130165

[scherkus (not reviewing), 2012-03-27 16:51:11]

woooooooooooo

https://chromiumcodereview.appspot.com/9860027/diff/4001/media/filters/chunk_...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/9860027/diff/4001/media/filters/chunk_...
media/filters/chunk_demuxer.cc:366: // Initialize() is called?
NOTE: I did precisely that. It's crazy that you should have to write code to
handle a late set_host() call + remember to propagate any calls

the question is -- do you still need to override this method for a DCHECK? I say
no because we'll get null ptr exceptions instead

[scherkus (not reviewing), 2012-03-27 20:07:52]

as a note to myself this CL causes PrerenderHTML5VideoNetwork to fail --
investigating

[acolwell GONE FROM CHROMIUM, 2012-03-27 20:11:35]

Here are my initial comments

https://chromiumcodereview.appspot.com/9860027/diff/4001/media/base/demuxer.h
File media/base/demuxer.h (right):

https://chromiumcodereview.appspot.com/9860027/diff/4001/media/base/demuxer.h...
media/base/demuxer.h:43: virtual void Initialize(const PipelineStatusCB&
status_cb) = 0;
How about removing set_host() and adding DemuxerHost as a parameter to
Initialize()?

https://chromiumcodereview.appspot.com/9860027/diff/4001/media/base/filter_co...
File media/base/filter_collection.h (right):

https://chromiumcodereview.appspot.com/9860027/diff/4001/media/base/filter_co...
media/base/filter_collection.h:26: void SetDemuxer(Demuxer* demuxer);
nit: scoped_refptr<Demuxer>&?

https://chromiumcodereview.appspot.com/9860027/diff/4001/media/base/filter_co...
media/base/filter_collection.h:27: Demuxer* GetDemuxer();
nit: scoped_refptr<Demuxer>?

https://chromiumcodereview.appspot.com/9860027/diff/4001/media/filters/chunk_...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/9860027/diff/4001/media/filters/chunk_...
media/filters/chunk_demuxer.cc:366: // Initialize() is called?
I'm fine with you removing this method.

On 2012/03/27 16:51:11, scherkus wrote:
> NOTE: I did precisely that. It's crazy that you should have to write code to
> handle a late set_host() call + remember to propagate any calls
> 
> the question is -- do you still need to override this method for a DCHECK? I
say
> no because we'll get null ptr exceptions instead

https://chromiumcodereview.appspot.com/9860027/diff/4001/media/filters/ffmpeg...
File media/filters/ffmpeg_demuxer.cc (right):

https://chromiumcodereview.appspot.com/9860027/diff/4001/media/filters/ffmpeg...
media/filters/ffmpeg_demuxer.cc:346: if (max_duration_.InMicroseconds() >= 0)
I think all the deferred calls below shouldn't be necessary now. Right?

https://chromiumcodereview.appspot.com/9860027/diff/4001/media/filters/ffmpeg...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

https://chromiumcodereview.appspot.com/9860027/diff/4001/media/filters/ffmpeg...
media/filters/ffmpeg_demuxer_unittest.cc:700: InitializeDemuxer();
nit: How about making InitializeDemuxer() just take a filename since it looks
like almost all causes have the CreateDemuxer(filename) before them.

[scherkus (not reviewing), 2012-03-27 20:44:09]

https://chromiumcodereview.appspot.com/9860027/diff/4001/media/base/demuxer.h
File media/base/demuxer.h (right):

https://chromiumcodereview.appspot.com/9860027/diff/4001/media/base/demuxer.h...
media/base/demuxer.h:43: virtual void Initialize(const PipelineStatusCB&
status_cb) = 0;
On 2012/03/27 20:11:35, acolwell wrote:
> How about removing set_host() and adding DemuxerHost as a parameter to
> Initialize()?

added a TODO w/ bug as I was planning on doing that in a follow-up CL

it involves updating a bunch of tests :\

https://chromiumcodereview.appspot.com/9860027/diff/4001/media/base/filter_co...
File media/base/filter_collection.h (right):

https://chromiumcodereview.appspot.com/9860027/diff/4001/media/base/filter_co...
media/base/filter_collection.h:26: void SetDemuxer(Demuxer* demuxer);
On 2012/03/27 20:11:35, acolwell wrote:
> nit: scoped_refptr<Demuxer>&?

Done.

https://chromiumcodereview.appspot.com/9860027/diff/4001/media/base/filter_co...
media/base/filter_collection.h:27: Demuxer* GetDemuxer();
On 2012/03/27 20:11:35, acolwell wrote:
> nit: scoped_refptr<Demuxer>?

Done.

https://chromiumcodereview.appspot.com/9860027/diff/4001/media/filters/chunk_...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/9860027/diff/4001/media/filters/chunk_...
media/filters/chunk_demuxer.cc:366: // Initialize() is called?
On 2012/03/27 20:11:35, acolwell wrote:
> I'm fine with you removing this method.
> 
> On 2012/03/27 16:51:11, scherkus wrote:
> > NOTE: I did precisely that. It's crazy that you should have to write code to
> > handle a late set_host() call + remember to propagate any calls
> > 
> > the question is -- do you still need to override this method for a DCHECK? I
> say
> > no because we'll get null ptr exceptions instead
> 

Done.

https://chromiumcodereview.appspot.com/9860027/diff/4001/media/filters/ffmpeg...
File media/filters/ffmpeg_demuxer.cc (right):

https://chromiumcodereview.appspot.com/9860027/diff/4001/media/filters/ffmpeg...
media/filters/ffmpeg_demuxer.cc:346: if (max_duration_.InMicroseconds() >= 0)
On 2012/03/27 20:11:35, acolwell wrote:
> I think all the deferred calls below shouldn't be necessary now. Right?

woah!

ended up cleaning this code up a bit more by refactoring GetBitrate() into
CalculateBitrate(), etc...

[scherkus (not reviewing), 2012-03-28 00:24:34]

+shishir for prerender tests

http://codereview.chromium.org/9860027/diff/8001/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_html5_common.js (right):

http://codereview.chromium.org/9860027/diff/8001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_html5_common.js:53: assert(false);
shishir: can you take a look at this?

[Shishir, 2012-03-28 18:41:33]

http://codereview.chromium.org/9860027/diff/8001/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_html5_common.js (right):

http://codereview.chromium.org/9860027/diff/8001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_html5_common.js:53: assert(false);
On 2012/03/28 00:24:34, scherkus wrote:
> shishir: can you take a look at this?

From the webkit code
(third_party/WebKit/Source/WebCore/html/HTMLMediaElement.cpp), if there is no
progress events for 3 seconds, the stalled will fire. That seemed to be
happening when the test was written. Has something changed?

[scherkus (not reviewing), 2012-03-29 22:53:00]

On 2012/03/28 18:41:33, Shishir wrote:
>
http://codereview.chromium.org/9860027/diff/8001/chrome/test/data/prerender/p...
> File chrome/test/data/prerender/prerender_html5_common.js (right):
> 
>
http://codereview.chromium.org/9860027/diff/8001/chrome/test/data/prerender/p...
> chrome/test/data/prerender/prerender_html5_common.js:53: assert(false);
> On 2012/03/28 00:24:34, scherkus wrote:
> > shishir: can you take a look at this?
> 
> From the webkit code
> (third_party/WebKit/Source/WebCore/html/HTMLMediaElement.cpp), if there is no
> progress events for 3 seconds, the stalled will fire. That seemed to be
> happening when the test was written. Has something changed?

When I added logging it turned out that after prerendering is turned off we
immediately download the entire test clip but fail to change our network state
to NETWORK_IDLE, which causes us to fire a stalled event. That's a bug which
apparently this CL fixes, but causes the prerender test to fail.

[scherkus (not reviewing), 2012-03-30 20:11:17]

pinging acolwell in case he has more comments!

[acolwell GONE FROM CHROMIUM, 2012-03-30 21:16:39]

LGTM

http://codereview.chromium.org/9860027/diff/8001/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/9860027/diff/8001/media/base/pipeline.h#newcod...
media/base/pipeline.h:407: // Filter object from FilterCollection and initialize
it asynchronously.
nit: s/Filter object/object/ ?

[scherkus (not reviewing), 2012-03-30 21:40:45]

ping shishir

http://codereview.chromium.org/9860027/diff/8001/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_html5_common.js (right):

http://codereview.chromium.org/9860027/diff/8001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_html5_common.js:53: assert(false);
On 2012/03/28 18:41:33, Shishir wrote:
> On 2012/03/28 00:24:34, scherkus wrote:
> > shishir: can you take a look at this?
> 
> From the webkit code
> (third_party/WebKit/Source/WebCore/html/HTMLMediaElement.cpp), if there is no
> progress events for 3 seconds, the stalled will fire. That seemed to be
> happening when the test was written. Has something changed?

This CL apparently fixes that behaviour, which I would consider to be a bug. I'm
still digging in to see why that is.

http://codereview.chromium.org/9860027/diff/8001/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/9860027/diff/8001/media/base/pipeline.h#newcod...
media/base/pipeline.h:407: // Filter object from FilterCollection and initialize
it asynchronously.
On 2012/03/30 21:16:39, acolwell wrote:
> nit: s/Filter object/object/ ?

Done.

[Shishir, 2012-04-02 17:43:46]

LGTM for the prerender stuff.

[Shishir, 2012-04-02 17:46:08]

As discussed with Andrew, the network events were not being tested since DPP was
not blocking till the events actually fired. So for now the stalled network
event is being disabled and I will look to see how we can wait for them.

LGTM.