DemuxerFactory is born!

DemuxerFactory is born!

PipelineImpl no longer knows about DataSources, with DemuxerFactory taking a
DataSourceFactory when appropriate (i.e. for FFmpegDemuxerFactory).

This diff is relative to patchset 8 of issue 6480050, which must be submitted
first.

BUG=none
TEST=media_unittests pass; chrome plays videos; trybots passed: mac,linux,linux_clang,win_layout; trybots with only unrelated-looking failures: win,{mac,linux}_layout

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

[hclam, 2011-03-08 20:46:32]

AWSOME!!!! YOU ARE HERE!!!! :)

Alpha

2011年3月8日12:44 <fischman@chromium.org>:

> Reviewers: acolwell, scherkus,
>
> Description:
> DemuxerFactory is born!
>
> PipelineImpl no longer knows about DataSources, with DemuxerFactory taking
> a
> DataSourceFactory when appropriate (i.e. for FFmpegDemuxerFactory).
>
> This diff is relative to patchset 8 of issue 6480050, which must be
> submitted
> first.
>
> BUG=none
> TEST=media_unittests pass, chrome plays videos, trybots passed:
> win,mac,linux,linux_clang,linux_rel
>
> Please review this at http://codereview.chromium.org/6648004/
>
> Affected files:
>  M media/base/filter_collection.h
>  M media/base/filter_collection.cc
>  M media/base/filter_factories.h
>  M media/base/filter_factories.cc
>  M media/base/filters.h
>  M media/base/mock_filters.h
>  M media/base/mock_filters.cc
>  M media/base/pipeline_impl.h
>  M media/base/pipeline_impl.cc
>  M media/base/pipeline_impl_unittest.cc
>  M media/filters/ffmpeg_demuxer.h
>  M media/filters/ffmpeg_demuxer.cc
>  A media/filters/ffmpeg_demuxer_factory.h
>  A media/filters/ffmpeg_demuxer_factory.cc
>  M media/filters/ffmpeg_demuxer_unittest.cc
>  M media/media.gyp
>  M media/tools/player_wtl/movie.cc
>  M media/tools/player_x11/player_x11.cc
>  M webkit/glue/media/buffered_data_source.cc
>  M webkit/glue/webmediaplayer_impl.h
>  M webkit/glue/webmediaplayer_impl.cc
>
>
>

[acolwell GONE FROM CHROMIUM, 2011-03-08 21:48:08]

Looks pretty good. Just a few minor comments.

http://codereview.chromium.org/6648004/diff/1/media/base/mock_filters.h
File media/base/mock_filters.h (right):

http://codereview.chromium.org/6648004/diff/1/media/base/mock_filters.h#newco...
media/base/mock_filters.h:67: class MockDataSource : public DataSource {
Can we remove MockDataSource?

http://codereview.chromium.org/6648004/diff/1/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/6648004/diff/1/media/base/pipeline_impl.cc#new...
media/base/pipeline_impl.cc:585: void PipelineImpl::InitializeTask() {
Indentation

http://codereview.chromium.org/6648004/diff/1/media/base/pipeline_impl.cc#new...
media/base/pipeline_impl.cc:1039: pipeline_init_state_->demuxer_ =
make_scoped_refptr(demuxer);
I don't think you need make_scoped_refptr() here.

http://codereview.chromium.org/6648004/diff/1/media/base/pipeline_impl.cc#new...
media/base/pipeline_impl.cc:1040: OnFilterInitialize();
I think you can call InitializeTask() from here instead since the check at the
top makes sure you are on the right thread.

http://codereview.chromium.org/6648004/diff/1/media/base/pipeline_impl_unitte...
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/6648004/diff/1/media/base/pipeline_impl_unitte...
media/base/pipeline_impl_unittest.cc:21: using ::testing::InvokeArgument;
Is this still needed?

http://codereview.chromium.org/6648004/diff/1/media/filters/ffmpeg_demuxer.h
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/6648004/diff/1/media/filters/ffmpeg_demuxer.h#...
media/filters/ffmpeg_demuxer.h:137: virtual void Initialize(
Consider moving this before the Demuxer & Filter implementation comments since
Initialize() isn't part of the Demuxer interface anymore.

Does Initialize() still need to be virtual?

http://codereview.chromium.org/6648004/diff/1/media/filters/ffmpeg_demuxer_fa...
File media/filters/ffmpeg_demuxer_factory.cc (right):

http://codereview.chromium.org/6648004/diff/1/media/filters/ffmpeg_demuxer_fa...
media/filters/ffmpeg_demuxer_factory.cc:45: DemuxerFactory::BuildCallback* cb_;
Shouldn't this be a scoped_ptr?

http://codereview.chromium.org/6648004/diff/1/media/filters/ffmpeg_demuxer_fa...
media/filters/ffmpeg_demuxer_factory.cc:83: FilterHost* host_;
Is this needed? It doesn't look like any of the code uses FilterHost.

http://codereview.chromium.org/6648004/diff/1/media/filters/ffmpeg_demuxer_fa...
File media/filters/ffmpeg_demuxer_factory.h (right):

http://codereview.chromium.org/6648004/diff/1/media/filters/ffmpeg_demuxer_fa...
media/filters/ffmpeg_demuxer_factory.h:33: FilterHost* host_;  // Unowned.
Remove? I don't see it being used.

http://codereview.chromium.org/6648004/diff/1/media/tools/player_x11/player_x...
File media/tools/player_x11/player_x11.cc (right):

http://codereview.chromium.org/6648004/diff/1/media/tools/player_x11/player_x...
media/tools/player_x11/player_x11.cc:104: *pipeline = new
media::PipelineImpl(message_loop);
Move back if FFmpegDemuxerFactory doesn't need FilterHost.

http://codereview.chromium.org/6648004/diff/1/media/tools/player_x11/player_x...
media/tools/player_x11/player_x11.cc:142: std::cout << "InitPipeline: " <<
(*pipeline)->GetError() << std::endl;
remove

http://codereview.chromium.org/6648004/diff/1/webkit/glue/webmediaplayer_impl.h
File webkit/glue/webmediaplayer_impl.h (right):

http://codereview.chromium.org/6648004/diff/1/webkit/glue/webmediaplayer_impl...
webkit/glue/webmediaplayer_impl.h:305: scoped_refptr<media::PipelineImpl>
pipeline_;
I don't think this will be needed once the demuxer factory doesn't require a
FilterHost.

[Ami GONE FROM CHROMIUM, 2011-03-08 22:44:48]

Thanks for the quick review!

http://codereview.chromium.org/6648004/diff/1/media/base/mock_filters.h
File media/base/mock_filters.h (right):

http://codereview.chromium.org/6648004/diff/1/media/base/mock_filters.h#newco...
media/base/mock_filters.h:67: class MockDataSource : public DataSource {
On 2011/03/08 21:48:09, acolwell wrote:
> Can we remove MockDataSource?

No, b/c ffmpeg_demuxer_unittest.cc uses it.

http://codereview.chromium.org/6648004/diff/1/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/6648004/diff/1/media/base/pipeline_impl.cc#new...
media/base/pipeline_impl.cc:585: void PipelineImpl::InitializeTask() {
On 2011/03/08 21:48:09, acolwell wrote:
> Indentation

Done.

http://codereview.chromium.org/6648004/diff/1/media/base/pipeline_impl.cc#new...
media/base/pipeline_impl.cc:1039: pipeline_init_state_->demuxer_ =
make_scoped_refptr(demuxer);
On 2011/03/08 21:48:09, acolwell wrote:
> I don't think you need make_scoped_refptr() here.

Done.

http://codereview.chromium.org/6648004/diff/1/media/base/pipeline_impl.cc#new...
media/base/pipeline_impl.cc:1040: OnFilterInitialize();
On 2011/03/08 21:48:09, acolwell wrote:
> I think you can call InitializeTask() from here instead since the check at the
> top makes sure you are on the right thread.

But calling OFI() has the additional benefit of decoupling the stacks.  I chose
this way b/c it is more consistent with the rest of the initialization
transitions in this file (continuation-style instead of call-style).

http://codereview.chromium.org/6648004/diff/1/media/base/pipeline_impl_unitte...
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/6648004/diff/1/media/base/pipeline_impl_unitte...
media/base/pipeline_impl_unittest.cc:21: using ::testing::InvokeArgument;
On 2011/03/08 21:48:09, acolwell wrote:
> Is this still needed?

Nope, deleted.

http://codereview.chromium.org/6648004/diff/1/media/filters/ffmpeg_demuxer.h
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/6648004/diff/1/media/filters/ffmpeg_demuxer.h#...
media/filters/ffmpeg_demuxer.h:137: virtual void Initialize(
On 2011/03/08 21:48:09, acolwell wrote:
> Consider moving this before the Demuxer & Filter implementation comments since
> Initialize() isn't part of the Demuxer interface anymore.

Done.

> Does Initialize() still need to be virtual?

Nope.  Done.

http://codereview.chromium.org/6648004/diff/1/media/filters/ffmpeg_demuxer_fa...
File media/filters/ffmpeg_demuxer_factory.cc (right):

http://codereview.chromium.org/6648004/diff/1/media/filters/ffmpeg_demuxer_fa...
media/filters/ffmpeg_demuxer_factory.cc:45: DemuxerFactory::BuildCallback* cb_;
On 2011/03/08 21:48:09, acolwell wrote:
> Shouldn't this be a scoped_ptr?

Done.

http://codereview.chromium.org/6648004/diff/1/media/filters/ffmpeg_demuxer_fa...
media/filters/ffmpeg_demuxer_factory.cc:83: FilterHost* host_;
On 2011/03/08 21:48:09, acolwell wrote:
> Is this needed? It doesn't look like any of the code uses FilterHost.

Not anymore.

http://codereview.chromium.org/6648004/diff/1/media/filters/ffmpeg_demuxer_fa...
File media/filters/ffmpeg_demuxer_factory.h (right):

http://codereview.chromium.org/6648004/diff/1/media/filters/ffmpeg_demuxer_fa...
media/filters/ffmpeg_demuxer_factory.h:33: FilterHost* host_;  // Unowned.
On 2011/03/08 21:48:09, acolwell wrote:
> Remove? I don't see it being used.

It used to be (before the set_host() shenaniganery) but isn't anymore.  Yay!

http://codereview.chromium.org/6648004/diff/1/media/tools/player_x11/player_x...
File media/tools/player_x11/player_x11.cc (right):

http://codereview.chromium.org/6648004/diff/1/media/tools/player_x11/player_x...
media/tools/player_x11/player_x11.cc:104: *pipeline = new
media::PipelineImpl(message_loop);
On 2011/03/08 21:48:09, acolwell wrote:
> Move back if FFmpegDemuxerFactory doesn't need FilterHost.

Done.

http://codereview.chromium.org/6648004/diff/1/media/tools/player_x11/player_x...
media/tools/player_x11/player_x11.cc:142: std::cout << "InitPipeline: " <<
(*pipeline)->GetError() << std::endl;
On 2011/03/08 21:48:09, acolwell wrote:
> remove

Keeping; it's useful for figuring out what went wrong (otherwise all you get is
the "Pipeline initialization failed" from line 279 and nothing else).

http://codereview.chromium.org/6648004/diff/1/webkit/glue/webmediaplayer_impl.h
File webkit/glue/webmediaplayer_impl.h (right):

http://codereview.chromium.org/6648004/diff/1/webkit/glue/webmediaplayer_impl...
webkit/glue/webmediaplayer_impl.h:305: scoped_refptr<media::PipelineImpl>
pipeline_;
On 2011/03/08 21:48:09, acolwell wrote:
> I don't think this will be needed once the demuxer factory doesn't require a
> FilterHost.

Done.

[acolwell GONE FROM CHROMIUM, 2011-03-08 23:19:00]

LGTM

http://codereview.chromium.org/6648004/diff/1/media/base/mock_filters.h
File media/base/mock_filters.h (right):

http://codereview.chromium.org/6648004/diff/1/media/base/mock_filters.h#newco...
media/base/mock_filters.h:67: class MockDataSource : public DataSource {
ok. Just checking.

On 2011/03/08 22:44:48, fischman wrote:
> On 2011/03/08 21:48:09, acolwell wrote:
> > Can we remove MockDataSource?
> 
> No, b/c ffmpeg_demuxer_unittest.cc uses it.

http://codereview.chromium.org/6648004/diff/1/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/6648004/diff/1/media/base/pipeline_impl.cc#new...
media/base/pipeline_impl.cc:1040: OnFilterInitialize();
On 2011/03/08 22:44:48, fischman wrote:
> On 2011/03/08 21:48:09, acolwell wrote:
> > I think you can call InitializeTask() from here instead since the check at
the
> > top makes sure you are on the right thread.
> 
> But calling OFI() has the additional benefit of decoupling the stacks.  I
chose
> this way b/c it is more consistent with the rest of the initialization
> transitions in this file (continuation-style instead of call-style).
Ok. Sounds reasonable to me.

http://codereview.chromium.org/6648004/diff/1/media/tools/player_x11/player_x...
File media/tools/player_x11/player_x11.cc (right):

http://codereview.chromium.org/6648004/diff/1/media/tools/player_x11/player_x...
media/tools/player_x11/player_x11.cc:142: std::cout << "InitPipeline: " <<
(*pipeline)->GetError() << std::endl;
On 2011/03/08 22:44:48, fischman wrote:
> On 2011/03/08 21:48:09, acolwell wrote:
> > remove
> 
> Keeping; it's useful for figuring out what went wrong (otherwise all you get
is
> the "Pipeline initialization failed" from line 279 and nothing else).

Sounds good to me.

[scherkus (not reviewing), 2011-03-11 21:46:58]

since this builds on top of acolwell's work (and he's in the process of fixing
that up), there's mostly style nits

http://codereview.chromium.org/6648004/diff/11001/media/base/mock_filters.h
File media/base/mock_filters.h (right):

http://codereview.chromium.org/6648004/diff/11001/media/base/mock_filters.h#n...
media/base/mock_filters.h:134: MockDemuxerFactory(MockDemuxer* demuxer);
explicit

http://codereview.chromium.org/6648004/diff/11001/media/base/mock_filters.h#n...
media/base/mock_filters.h:148: };
DISALLOW_etc..

http://codereview.chromium.org/6648004/diff/11001/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer_factory.cc (right):

http://codereview.chromium.org/6648004/diff/11001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_factory.cc:14: :
data_source_factory_(data_source_factory), loop_(loop) {}
indent by two more spaces

http://codereview.chromium.org/6648004/diff/11001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_factory.cc:22: // replaced with currying calls to
base::Bind.
nit: we like to annotate function names in comments with () (helps separate
classes from functions)

http://codereview.chromium.org/6648004/diff/11001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_factory.cc:28: : cb_(cb), demuxer_(demuxer) {
indent by two more spaces

http://codereview.chromium.org/6648004/diff/11001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_factory.cc:47: public
DataSourceFactory::BuildCallback {
de-indent by 2 spaces

http://codereview.chromium.org/6648004/diff/11001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_factory.cc:51: : cb_(cb), loop_(loop) {
indent by two more spaces

http://codereview.chromium.org/6648004/diff/11001/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/6648004/diff/11001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_unittest.cc:197:
NewExpectedStatusCallback(DEMUXER_ERROR_COULD_NOT_PARSE));
>80 chars

[scherkus (not reviewing), 2011-03-11 21:47:43]

LGTM, too (pending acolwell lands his stuff, of course!)

[Ami GONE FROM CHROMIUM, 2011-03-11 22:18:46]

http://codereview.chromium.org/6648004/diff/11001/media/base/mock_filters.h
File media/base/mock_filters.h (right):

http://codereview.chromium.org/6648004/diff/11001/media/base/mock_filters.h#n...
media/base/mock_filters.h:134: MockDemuxerFactory(MockDemuxer* demuxer);
On 2011/03/11 21:46:58, scherkus wrote:
> explicit

Done.

http://codereview.chromium.org/6648004/diff/11001/media/base/mock_filters.h#n...
media/base/mock_filters.h:148: };
On 2011/03/11 21:46:58, scherkus wrote:
> DISALLOW_etc..

Done.

http://codereview.chromium.org/6648004/diff/11001/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer_factory.cc (right):

http://codereview.chromium.org/6648004/diff/11001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_factory.cc:14: :
data_source_factory_(data_source_factory), loop_(loop) {}
On 2011/03/11 21:46:58, scherkus wrote:
> indent by two more spaces

Done.

http://codereview.chromium.org/6648004/diff/11001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_factory.cc:22: // replaced with currying calls to
base::Bind.
On 2011/03/11 21:46:58, scherkus wrote:
> nit: we like to annotate function names in comments with () (helps separate
> classes from functions)

Done.

http://codereview.chromium.org/6648004/diff/11001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_factory.cc:28: : cb_(cb), demuxer_(demuxer) {
On 2011/03/11 21:46:58, scherkus wrote:
> indent by two more spaces

Done.

http://codereview.chromium.org/6648004/diff/11001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_factory.cc:47: public
DataSourceFactory::BuildCallback {
On 2011/03/11 21:46:58, scherkus wrote:
> de-indent by 2 spaces

Done.

http://codereview.chromium.org/6648004/diff/11001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_factory.cc:51: : cb_(cb), loop_(loop) {
On 2011/03/11 21:46:58, scherkus wrote:
> indent by two more spaces

Done.

http://codereview.chromium.org/6648004/diff/11001/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/6648004/diff/11001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_unittest.cc:197:
NewExpectedStatusCallback(DEMUXER_ERROR_COULD_NOT_PARSE));
On 2011/03/11 21:46:58, scherkus wrote:
> >80 chars

Done.