Separate DemuxerHost from DataSourceHost.

Separate DemuxerHost from DataSourceHost.

This is the first part of work to remove byte range computations out of media::Pipeline, and instead have the DataSource report those directly to Pipeline's parent, which will avoid crossing thread boundaries unnecessarily. This first CL just cleans up the interfaces required to do that, but already eliminates some of the stranger parts of the set-up code.

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

[sandersd (OOO until July 31), 2014-03-26 21:10:57]

Changes (read: hacks) to separate the two interfaces. It's now in a state where
all the DataSourceHost methods can be moved to WMPI without breaking Pipeline
(some DemuxerHost methods will move as well).

[scherkus (not reviewing), 2014-03-26 21:50:11]

nifty!

the next step would be to migrate the Pipeline implementation code of DSH into
either WMPI or some other helper class/impl that lives alongside WMPI under
content/renderer/media/

one other thought that popped into my mind is whether this code could get merged
into BufferedDataSource entirely, but that might be bloating that class a bit
too much...

https://codereview.chromium.org/212803004/diff/1/media/base/demuxer.h
File media/base/demuxer.h (left):

https://codereview.chromium.org/212803004/diff/1/media/base/demuxer.h#oldcode35
media/base/demuxer.h:35: virtual void RemoveTextStream(DemuxerStream*
text_stream) = 0;
what happened to this method?

https://codereview.chromium.org/212803004/diff/1/media/base/pipeline.cc
File media/base/pipeline.cc (left):

https://codereview.chromium.org/212803004/diff/1/media/base/pipeline.cc#oldco...
media/base/pipeline.cc:377: DCHECK(IsRunning());
I'm assuming these DCHECKs were getting hit, right?

https://codereview.chromium.org/212803004/diff/1/media/filters/ffmpeg_demuxer.cc
File media/filters/ffmpeg_demuxer.cc (left):

https://codereview.chromium.org/212803004/diff/1/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:449: data_source_->set_host(host);
\o/

[sandersd (OOO until July 31), 2014-03-26 22:10:54]

> one other thought that popped into my mind is whether this code could get
merged
> into BufferedDataSource entirely, but that might be bloating that class a bit
> too much...

I don't think so, the only real code involved is GetBufferedTimeRanges(), which
also uses data from the demuxer (time ranges and duration).

[sandersd (OOO until July 31), 2014-03-26 22:11:12]

https://codereview.chromium.org/212803004/diff/1/media/base/demuxer.h
File media/base/demuxer.h (left):

https://codereview.chromium.org/212803004/diff/1/media/base/demuxer.h#oldcode35
media/base/demuxer.h:35: virtual void RemoveTextStream(DemuxerStream*
text_stream) = 0;
On 2014/03/26 21:50:12, scherkus wrote:
> what happened to this method?

It's never called, but this is the right place for it. I'll put it back.

https://codereview.chromium.org/212803004/diff/1/media/base/pipeline.cc
File media/base/pipeline.cc (left):

https://codereview.chromium.org/212803004/diff/1/media/base/pipeline.cc#oldco...
media/base/pipeline.cc:377: DCHECK(IsRunning());
On 2014/03/26 21:50:12, scherkus wrote:
> I'm assuming these DCHECKs were getting hit, right?

I didn't check. Before these calls were delayed until after
Pipeline::InitializeDemuxer() (that's where the host gets passed), but now it
happens as soon as the BufferedDataSource's Loader starts (which I believe
happens strictly before the pipeline is started for FFmpeg, in parallel for
MSE).

[scherkus (not reviewing), 2014-03-26 22:22:37]

On 2014/03/26 22:10:54, Dan Sanders wrote:
> > one other thought that popped into my mind is whether this code could get
> merged
> > into BufferedDataSource entirely, but that might be bloating that class a
bit
> > too much...
> 
> I don't think so, the only real code involved is GetBufferedTimeRanges(),
which
> also uses data from the demuxer (time ranges and duration).

Yeah I wouldn't want to add the notion of media duration to BDS.

Anyway, the lower half of Pipeline::GetBufferedTimeRanges() where it loops over
buffered_byte_ranges_ is only ever used when BufferedDataSource+FFmpegDemuxer is
involved. We could extract that bit of code as well as
Pipeline::TimeForByteOffset_Locked() to WMPI / helper class since WMPI knows the
duration and could also be the one holding the buffered byte ranges.

The above might be a bit too much for the current CL, so alternatively we could
keep this CL roughly as-is but clean up DataSource{Host}:
  - Remove set_host()/host() from DataSource
  - Make DataSourceHost* a parameter to BufferedDataSource::Initialize() 
  - Remove the if-statements checking for the presence of a host() in
BufferedDataSource

WDYT?

[sandersd (OOO until July 31), 2014-03-26 22:51:22]

> Anyway, the lower half of Pipeline::GetBufferedTimeRanges() where it loops
over
> buffered_byte_ranges_ is only ever used when BufferedDataSource+FFmpegDemuxer
is
> involved. We could extract that bit of code as well as
> Pipeline::TimeForByteOffset_Locked() to WMPI / helper class since WMPI knows
the
> duration and could also be the one holding the buffered byte ranges.

That was exactly my plan. It's probably worth adding the actual duration to the
duration_change callback (maybe it should be part of PipelineMetadata?) to avoid
that accessor while we are at it.

> The above might be a bit too much for the current CL, so alternatively we
could
> keep this CL roughly as-is but clean up DataSource{Host}:
>   - Remove set_host()/host() from DataSource
>   - Make DataSourceHost* a parameter to BufferedDataSource::Initialize() 
>   - Remove the if-statements checking for the presence of a host() in
> BufferedDataSource
> 
> WDYT?

Probably a good idea to split it up. If I find I'm rewriting a lot of the tests
already then it will be easier to do it in one shot.

[scherkus (not reviewing), 2014-03-26 22:53:44]

On 2014/03/26 22:51:22, Dan Sanders wrote:
> > Anyway, the lower half of Pipeline::GetBufferedTimeRanges() where it loops
> over
> > buffered_byte_ranges_ is only ever used when
BufferedDataSource+FFmpegDemuxer
> is
> > involved. We could extract that bit of code as well as
> > Pipeline::TimeForByteOffset_Locked() to WMPI / helper class since WMPI knows
> the
> > duration and could also be the one holding the buffered byte ranges.
> 
> That was exactly my plan. It's probably worth adding the actual duration to
the
> duration_change callback (maybe it should be part of PipelineMetadata?) to
avoid
> that accessor while we are at it.

hah! yeah you could carve out the include-duration-inside-the-callback as a
separate tiny CL if you want or include it as part of this CL or defer it to a
future CL

> > The above might be a bit too much for the current CL, so alternatively we
> could
> > keep this CL roughly as-is but clean up DataSource{Host}:
> >   - Remove set_host()/host() from DataSource
> >   - Make DataSourceHost* a parameter to BufferedDataSource::Initialize() 
> >   - Remove the if-statements checking for the presence of a host() in
> > BufferedDataSource
> > 
> > WDYT?
> 
> Probably a good idea to split it up. If I find I'm rewriting a lot of the
tests
> already then it will be easier to do it in one shot.

SGTM. Let me know if things start looking hairy.

[sandersd (OOO until July 31), 2014-03-27 23:18:35]

> SGTM. Let me know if things start looking hairy.

It didn't end up too bad overall. I had to add a bad hack to pipeline_unittests,
but it will get fixed when we move the DataSourceHost implementation out of
Pipeline.

I may need some guidance adding tests for webmediaplayer_impl in that future CL,
as there are no tests at all right now.

[scherkus (not reviewing), 2014-03-28 20:54:09]

holy crap I love this CL

few tiny nitty things

https://codereview.chromium.org/212803004/diff/40001/content/renderer/media/a...
File content/renderer/media/android/media_source_delegate.cc (left):

https://codereview.chromium.org/212803004/diff/40001/content/renderer/media/a...
content/renderer/media/android/media_source_delegate.cc:303: NOTIMPLEMENTED();
case. in. point. :)

https://codereview.chromium.org/212803004/diff/40001/content/renderer/media/b...
File content/renderer/media/buffered_data_source.h (right):

https://codereview.chromium.org/212803004/diff/40001/content/renderer/media/b...
content/renderer/media/buffered_data_source.h:40: // will be called whenever the
downloading/paused state of the source changes.
want to a TODO(sandersd) saying how |host| should be moved to content?

https://codereview.chromium.org/212803004/diff/40001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/212803004/diff/40001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:309: // TODO(sandersd): Make WMPI
a DataSourceHost and pass &this instead of
nit: &this would be a WMPI** -- you probably mean |this|

https://codereview.chromium.org/212803004/diff/40001/media/base/data_source.h
File media/base/data_source.h (right):

https://codereview.chromium.org/212803004/diff/40001/media/base/data_source.h...
media/base/data_source.h:14: // TODO(sandersd): Rename to
BufferedDataSourceHost.
nit: ... and move to content/renderer/media/

https://codereview.chromium.org/212803004/diff/40001/media/base/pipeline_unit...
File media/base/pipeline_unittest.cc (right):

https://codereview.chromium.org/212803004/diff/40001/media/base/pipeline_unit...
media/base/pipeline_unittest.cc:220: // This is required to test the time range
code now that DemuxerHost does
nit: tell it like it and stick a big ol' "HACK: " in here -- right now it's a
bit of a subtle hack :)

I'd stick in a link to http://crbug.com/122071 as well

[sandersd (OOO until July 31), 2014-03-28 21:14:28]

https://codereview.chromium.org/212803004/diff/40001/content/renderer/media/b...
File content/renderer/media/buffered_data_source.h (right):

https://codereview.chromium.org/212803004/diff/40001/content/renderer/media/b...
content/renderer/media/buffered_data_source.h:40: // will be called whenever the
downloading/paused state of the source changes.
On 2014/03/28 20:54:09, scherkus wrote:
> want to a TODO(sandersd) saying how |host| should be moved to content?

Done.

https://codereview.chromium.org/212803004/diff/40001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/212803004/diff/40001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:309: // TODO(sandersd): Make WMPI
a DataSourceHost and pass &this instead of
On 2014/03/28 20:54:09, scherkus wrote:
> nit: &this would be a WMPI** -- you probably mean |this|

Done.

https://codereview.chromium.org/212803004/diff/40001/media/base/data_source.h
File media/base/data_source.h (right):

https://codereview.chromium.org/212803004/diff/40001/media/base/data_source.h...
media/base/data_source.h:14: // TODO(sandersd): Rename to
BufferedDataSourceHost.
On 2014/03/28 20:54:09, scherkus wrote:
> nit: ... and move to content/renderer/media/

Done.

https://codereview.chromium.org/212803004/diff/40001/media/base/pipeline_unit...
File media/base/pipeline_unittest.cc (right):

https://codereview.chromium.org/212803004/diff/40001/media/base/pipeline_unit...
media/base/pipeline_unittest.cc:220: // This is required to test the time range
code now that DemuxerHost does
On 2014/03/28 20:54:09, scherkus wrote:
> nit: tell it like it and stick a big ol' "HACK: " in here -- right now it's a
> bit of a subtle hack :)
> 
> I'd stick in a link to http://crbug.com/122071 as well

Done.

[scherkus (not reviewing), 2014-03-28 21:19:56]

two nits but this is lgtm!

can you also expand the CL description to answer "why?" (e.g., "DSH is only used
by BDS blah blah blah")

https://codereview.chromium.org/212803004/diff/60001/content/renderer/media/b...
File content/renderer/media/buffered_data_source.cc (right):

https://codereview.chromium.org/212803004/diff/60001/content/renderer/media/b...
content/renderer/media/buffered_data_source.cc:103:
DCHECK(!downloading_cb_.is_null());
DCHECK(host_) here isn't a bad thing as it's required by the API and we don't
immediately de-ref it in the ctor (which would help enforce the API by crashing)

https://codereview.chromium.org/212803004/diff/60001/content/renderer/media/b...
File content/renderer/media/buffered_data_source.h (right):

https://codereview.chromium.org/212803004/diff/60001/content/renderer/media/b...
content/renderer/media/buffered_data_source.h:41: // TODO(sandersd): Move
|media::DataSourceHost| to
nit: we |variables| but not namespace::FooBar

[sandersd (OOO until July 31), 2014-03-29 01:15:07]

https://codereview.chromium.org/212803004/diff/60001/content/renderer/media/b...
File content/renderer/media/buffered_data_source.cc (right):

https://codereview.chromium.org/212803004/diff/60001/content/renderer/media/b...
content/renderer/media/buffered_data_source.cc:103:
DCHECK(!downloading_cb_.is_null());
On 2014/03/28 21:19:56, scherkus wrote:
> DCHECK(host_) here isn't a bad thing as it's required by the API and we don't
> immediately de-ref it in the ctor (which would help enforce the API by
crashing)

Done.

https://codereview.chromium.org/212803004/diff/60001/content/renderer/media/b...
File content/renderer/media/buffered_data_source.h (right):

https://codereview.chromium.org/212803004/diff/60001/content/renderer/media/b...
content/renderer/media/buffered_data_source.h:41: // TODO(sandersd): Move
|media::DataSourceHost| to
On 2014/03/28 21:19:56, scherkus wrote:
> nit: we |variables| but not namespace::FooBar

Done.

[sandersd (OOO until July 31), 2014-03-29 01:17:41]

tommycli@chromium.org: Please review changes in chrome/utility/media_galleries/.

[tommycli, 2014-04-01 19:59:11]

lgtm

[sandersd (OOO until July 31), 2014-04-01 20:22:08]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2014-04-01 20:23:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sandersd@chromium.org/212803004/100001

[commit-bot: I haz the power, 2014-04-01 21:06:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-01 21:06:45]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[sandersd (OOO until July 31), 2014-04-01 21:21:32]

The CQ bit was checked by sandersd@chromium.org

[sandersd (OOO until July 31), 2014-04-01 22:37:59]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2014-04-01 22:48:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sandersd@chromium.org/212803004/100001

[commit-bot: I haz the power, 2014-04-02 02:04:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sandersd@chromium.org/212803004/120001

[commit-bot: I haz the power, 2014-04-02 10:17:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-02 10:17:56]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[sandersd (OOO until July 31), 2014-04-02 18:11:14]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2014-04-02 18:12:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sandersd@chromium.org/212803004/120001

[commit-bot: I haz the power, 2014-04-03 00:17:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-03 00:17:49]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[sandersd (OOO until July 31), 2014-04-03 00:31:25]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2014-04-03 00:32:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sandersd@chromium.org/212803004/140001

[commit-bot: I haz the power, 2014-04-03 02:43:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-03 02:44:00]

Retried try job too often on win_rel for step(s) telemetry_perf_unittests,
telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[sandersd (OOO until July 31), 2014-04-03 17:40:06]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2014-04-03 17:40:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sandersd@chromium.org/212803004/140001

[commit-bot: I haz the power, 2014-04-03 18:54:43]

Change committed as 261494