Removing DataSource from Filter hierarchy

Removing DataSource from Filter hierarchy and refactoring FilterHost into DemuxerHost & DataSourceHost.

Over the last year, several refactorings have caused DataSource to have almost nothing in common with the Filter interface. This change removes it from the Filter class hierarchy and splits up the FilterHost interface so that DataSource, Demuxer, and Filter have their own host interfaces. Splitting FilterHost improves encapsulation and makes it easier to reason about which host methods are required by different parts of the code.

BUG=
TEST=media_unittests


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

[acolwell GONE FROM CHROMIUM, 2011-12-14 17:57:38]

I'm intentionally not making any changes to the callers. I just wanted to make
the methods only accessible to the objects that actually call them. Follow-on
CLs will improve the actual  interactions between objects.

[Ami GONE FROM CHROMIUM, 2011-12-15 18:39:07]

Exciting.

http://codereview.chromium.org/8936014/diff/7001/media/base/data_source.cc
File media/base/data_source.cc (right):

http://codereview.chromium.org/8936014/diff/7001/media/base/data_source.cc#ne...
media/base/data_source.cc:16: void DataSource::set_host(DataSourceHost* host) {
host_ = host; }
Filter::set_host() is more DCHECK-happy.  Wanna copy that here?

http://codereview.chromium.org/8936014/diff/7001/media/base/data_source.cc#ne...
media/base/data_source.cc:18: void DataSource::SetPlaybackRate(float
playback_rate) {}
I wonder if this wouldn't be better off pure-virtual.
(or at least impl'd as a NOTIMPLEMENTED if there are many derived classes, most
of which don't implement this but also expect to never have it called on them).

http://codereview.chromium.org/8936014/diff/7001/media/base/data_source.h
File media/base/data_source.h (right):

http://codereview.chromium.org/8936014/diff/7001/media/base/data_source.h#new...
media/base/data_source.h:14: class DataSourceHost {
No MEDIA_EXPORT on this and the enum below?

http://codereview.chromium.org/8936014/diff/7001/media/base/data_source.h#new...
media/base/data_source.h:27: virtual ~DataSourceHost() {}
Comment about why?

http://codereview.chromium.org/8936014/diff/7001/media/base/data_source.h#new...
media/base/data_source.h:37: enum Preload {
I wonder whether this wants to live inside DataSource.

http://codereview.chromium.org/8936014/diff/7001/media/base/data_source.h#new...
media/base/data_source.h:83: friend class
base::RefCountedThreadSafe<DataSource>;
This is necessary?

http://codereview.chromium.org/8936014/diff/7001/media/base/data_source.h#new...
media/base/data_source.h:85: DataSource();
doco why these are protected?

(generally, "protected" without a comment explaining the expected contract with
subclassers is raises my suspicions).

http://codereview.chromium.org/8936014/diff/7001/media/base/demuxer.h
File media/base/demuxer.h (right):

http://codereview.chromium.org/8936014/diff/7001/media/base/demuxer.h#newcode38
media/base/demuxer.h:38: virtual ~DemuxerHost() {}
Out-of-line please.
(even though here the clang plugin doesn't ask you to).

http://codereview.chromium.org/8936014/diff/7001/media/base/demuxer.h#newcode91
media/base/demuxer.h:91: protected:
Ditto q about documenting protected.

http://codereview.chromium.org/8936014/diff/7001/media/base/filter_host.h
File media/base/filter_host.h (left):

http://codereview.chromium.org/8936014/diff/7001/media/base/filter_host.h#old...
media/base/filter_host.h:31: // Gets the current time in microseconds.
"in microseconds" here and below is BS.

http://codereview.chromium.org/8936014/diff/7001/media/base/filters.h
File media/base/filters.h (left):

http://codereview.chromium.org/8936014/diff/7001/media/base/filters.h#oldcode136
media/base/filters.h:136: class MEDIA_EXPORT DataSource : public Filter {
This is actually the more notable part of this CL to me (DS is no longer a
Filter).  I'd call that out in the CL description (as well as a general
explanation for why this is a good idea)

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

http://codereview.chromium.org/8936014/diff/7001/media/base/pipeline.h#newcode14
media/base/pipeline.h:14: #include "media/base/data_source.h"
Why?

http://codereview.chromium.org/8936014/diff/7001/media/base/pipeline_impl.h
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/8936014/diff/7001/media/base/pipeline_impl.h#n...
media/base/pipeline_impl.h:196: // DataSourceHost implementation.
s/DataSourceHost/DataSourceHost (by way of DemuxerHost)/

http://codereview.chromium.org/8936014/diff/7001/webkit/media/buffered_data_s...
File webkit/media/buffered_data_source.cc (right):

http://codereview.chromium.org/8936014/diff/7001/webkit/media/buffered_data_s...
webkit/media/buffered_data_source.cc:652: media::DataSourceHost*
data_source_host = host();
I realize the diff here is mechanical, but you might take the opp to drop the
local variable.

http://codereview.chromium.org/8936014/diff/7001/webkit/media/webmediaplayer_...
File webkit/media/webmediaplayer_proxy.h (right):

http://codereview.chromium.org/8936014/diff/7001/webkit/media/webmediaplayer_...
webkit/media/webmediaplayer_proxy.h:13: #include "media/base/video_frame.h"
Replace w/ a fwd decl if necessary?

[acolwell GONE FROM CHROMIUM, 2011-12-15 20:57:59]

http://codereview.chromium.org/8936014/diff/7001/media/base/data_source.cc
File media/base/data_source.cc (right):

http://codereview.chromium.org/8936014/diff/7001/media/base/data_source.cc#ne...
media/base/data_source.cc:16: void DataSource::set_host(DataSourceHost* host) {
host_ = host; }
On 2011/12/15 18:39:08, Ami Fischman wrote:
> Filter::set_host() is more DCHECK-happy.  Wanna copy that here?

Done.

http://codereview.chromium.org/8936014/diff/7001/media/base/data_source.cc#ne...
media/base/data_source.cc:18: void DataSource::SetPlaybackRate(float
playback_rate) {}
On 2011/12/15 18:39:08, Ami Fischman wrote:
> I wonder if this wouldn't be better off pure-virtual.
> (or at least impl'd as a NOTIMPLEMENTED if there are many derived classes,
most
> of which don't implement this but also expect to never have it called on
them).

So this is another artifact of how we handle different Preroll behaviors. Really
only BufferedDataSource needs this call, but FFmpegDemuxer calls this method
independent of what DataSource we are using. I believe this method should go
away and we should implement something a little more straight forward. That is a
future CL though.

I can't change this to NOTIMPLEMENTED since I know it will get called even on
DataSources that don't care about it. Should I leave this as is or make it
pure-virtual and add empty implementations to FileDataSource & SimpleDataSource?

http://codereview.chromium.org/8936014/diff/7001/media/base/data_source.h
File media/base/data_source.h (right):

http://codereview.chromium.org/8936014/diff/7001/media/base/data_source.h#new...
media/base/data_source.h:14: class DataSourceHost {
On 2011/12/15 18:39:08, Ami Fischman wrote:
> No MEDIA_EXPORT on this and the enum below?
Didn't appear to be needed for my shared build, but I'll add it to
DataSourceHost just in case.

PipelineStatus doesn't have a MEDIA_EXPORT so I think Preroll should be ok.

http://codereview.chromium.org/8936014/diff/7001/media/base/data_source.h#new...
media/base/data_source.h:27: virtual ~DataSourceHost() {}
On 2011/12/15 18:39:08, Ami Fischman wrote:
> Comment about why?
I don't have a good reason why so I'm just going to make it public.

http://codereview.chromium.org/8936014/diff/7001/media/base/data_source.h#new...
media/base/data_source.h:37: enum Preload {
On 2011/12/15 18:39:08, Ami Fischman wrote:
> I wonder whether this wants to live inside DataSource.
Actually after further thought, I don't think DataSource is a good home for
this. DataSource only needs to know about it because of how FFmpegDemuxer
implemented the preload behaviors. To me the preload concept is really a demuxer
level or higher concept. For example ChunkDemuxer could implement it's preroll
behaviors but doesn't use DataSource.

Perhaps move this into it's own file? demuxer.h? pipeline.h?

http://codereview.chromium.org/8936014/diff/7001/media/base/data_source.h#new...
media/base/data_source.h:83: friend class
base::RefCountedThreadSafe<DataSource>;
On 2011/12/15 18:39:08, Ami Fischman wrote:
> This is necessary?
Yes. This follows the pattern, in other parts of the media code, of only
allowing the scoped_refptr to delete the object. Added the comment indicating
this that I forgot to copy over.

http://codereview.chromium.org/8936014/diff/7001/media/base/data_source.h#new...
media/base/data_source.h:85: DataSource();
On 2011/12/15 18:39:08, Ami Fischman wrote:
> doco why these are protected?
> 
> (generally, "protected" without a comment explaining the expected contract
with
> subclassers is raises my suspicions).
Made this public since a DataSource can't be created on its own.

http://codereview.chromium.org/8936014/diff/7001/media/base/demuxer.h
File media/base/demuxer.h (right):

http://codereview.chromium.org/8936014/diff/7001/media/base/demuxer.h#newcode38
media/base/demuxer.h:38: virtual ~DemuxerHost() {}
On 2011/12/15 18:39:08, Ami Fischman wrote:
> Out-of-line please.
> (even though here the clang plugin doesn't ask you to).

Done and made public since I don't have a good reason for leaving it protected.

http://codereview.chromium.org/8936014/diff/7001/media/base/demuxer.h#newcode91
media/base/demuxer.h:91: protected:
On 2011/12/15 18:39:08, Ami Fischman wrote:
> Ditto q about documenting protected.

Done.

http://codereview.chromium.org/8936014/diff/7001/media/base/filter_host.h
File media/base/filter_host.h (left):

http://codereview.chromium.org/8936014/diff/7001/media/base/filter_host.h#old...
media/base/filter_host.h:31: // Gets the current time in microseconds.
On 2011/12/15 18:39:08, Ami Fischman wrote:
> "in microseconds" here and below is BS.

Done.

http://codereview.chromium.org/8936014/diff/7001/media/base/filters.h
File media/base/filters.h (left):

http://codereview.chromium.org/8936014/diff/7001/media/base/filters.h#oldcode136
media/base/filters.h:136: class MEDIA_EXPORT DataSource : public Filter {
On 2011/12/15 18:39:08, Ami Fischman wrote:
> This is actually the more notable part of this CL to me (DS is no longer a
> Filter).  I'd call that out in the CL description (as well as a general
> explanation for why this is a good idea)

Done.

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

http://codereview.chromium.org/8936014/diff/7001/media/base/pipeline.h#newcode14
media/base/pipeline.h:14: #include "media/base/data_source.h"
On 2011/12/15 18:39:08, Ami Fischman wrote:
> Why?
Needed for SetPreroll(). :/  more fuel for Preroll in its own file?

http://codereview.chromium.org/8936014/diff/7001/media/base/pipeline_impl.h
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/8936014/diff/7001/media/base/pipeline_impl.h#n...
media/base/pipeline_impl.h:196: // DataSourceHost implementation.
On 2011/12/15 18:39:08, Ami Fischman wrote:
> s/DataSourceHost/DataSourceHost (by way of DemuxerHost)/

Done.

http://codereview.chromium.org/8936014/diff/7001/webkit/media/buffered_data_s...
File webkit/media/buffered_data_source.cc (right):

http://codereview.chromium.org/8936014/diff/7001/webkit/media/buffered_data_s...
webkit/media/buffered_data_source.cc:652: media::DataSourceHost*
data_source_host = host();
On 2011/12/15 18:39:08, Ami Fischman wrote:
> I realize the diff here is mechanical, but you might take the opp to drop the
> local variable.

Done.

http://codereview.chromium.org/8936014/diff/7001/webkit/media/webmediaplayer_...
File webkit/media/webmediaplayer_proxy.h (right):

http://codereview.chromium.org/8936014/diff/7001/webkit/media/webmediaplayer_...
webkit/media/webmediaplayer_proxy.h:13: #include "media/base/video_frame.h"
On 2011/12/15 18:39:08, Ami Fischman wrote:
> Replace w/ a fwd decl if necessary?

Done.

[Ami GONE FROM CHROMIUM, 2011-12-15 22:39:16]

lgtm

http://codereview.chromium.org/8936014/diff/7001/media/base/data_source.cc
File media/base/data_source.cc (right):

http://codereview.chromium.org/8936014/diff/7001/media/base/data_source.cc#ne...
media/base/data_source.cc:18: void DataSource::SetPlaybackRate(float
playback_rate) {}
On 2011/12/15 20:57:59, acolwell wrote:
> On 2011/12/15 18:39:08, Ami Fischman wrote:
> > I wonder if this wouldn't be better off pure-virtual.
> > (or at least impl'd as a NOTIMPLEMENTED if there are many derived classes,
> most
> > of which don't implement this but also expect to never have it called on
> them).
> 
> So this is another artifact of how we handle different Preroll behaviors.
Really
> only BufferedDataSource needs this call, but FFmpegDemuxer calls this method
> independent of what DataSource we are using. I believe this method should go
> away and we should implement something a little more straight forward. That is
a
> future CL though.
> 
> I can't change this to NOTIMPLEMENTED since I know it will get called even on
> DataSources that don't care about it. Should I leave this as is or make it
> pure-virtual and add empty implementations to FileDataSource &
SimpleDataSource?

I don't care.

http://codereview.chromium.org/8936014/diff/7001/media/base/data_source.h
File media/base/data_source.h (right):

http://codereview.chromium.org/8936014/diff/7001/media/base/data_source.h#new...
media/base/data_source.h:37: enum Preload {
On 2011/12/15 20:57:59, acolwell wrote:
> On 2011/12/15 18:39:08, Ami Fischman wrote:
> > I wonder whether this wants to live inside DataSource.
> Actually after further thought, I don't think DataSource is a good home for
> this. DataSource only needs to know about it because of how FFmpegDemuxer
> implemented the preload behaviors. To me the preload concept is really a
demuxer
> level or higher concept. For example ChunkDemuxer could implement it's preroll
> behaviors but doesn't use DataSource.
> 
> Perhaps move this into it's own file? demuxer.h? pipeline.h?

Either of the latter 2 would cause a circular dependency, so bad.
ISTM the _right_ solution here is to have different enums for different points
in the hierarchy, but I guess I don't care enough about it to force any change
here.

[commit-bot: I haz the power, 2011-12-16 15:16:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/8936014/25001

[commit-bot: I haz the power, 2011-12-16 17:14:27]

Change committed as 114819

[jennb, 2011-12-16 18:09:04]

http://codereview.chromium.org/8936014/diff/25001/media/media.gyp
File media/media.gyp (right):

http://codereview.chromium.org/8936014/diff/25001/media/media.gyp#newcode682
media/media.gyp:682: 'base/mock_ddata_source_host.h',
typo: ddata