Fire CanPlayThrough immediately for local and streaming media files

Fire CanPlayThrough immediately for local and streaming media files

Changes DownloadRateMonitor logic to know to immediately fire CanPlayThrough
for media files with local sources, including local webcam streams. Renamed
the concept of "loaded" source to "local" source in the media pipeline for
clarity. 

Also updated the DownloadRateMonitor logic slightly to lean toward optimism 
in firing CanPlayThrough, including firing the event immediately for streaming
media.

BUG=105163
TEST=media_unittests,test_shell_tests, wjia's media stream test page works

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

[vrk (LEFT CHROMIUM), 2011-11-22 22:21:13]

Fixes the bug uncovered by wjia in http://codereview.chromium.org/8625002.

[wjia(left Chromium), 2011-11-22 23:01:17]

vrk, thanks for the fix!

would it good to let PipelineImpl decide only two cases?
1. need download_rate_monitor;
2. don't need download_rate_monitor;

It seems a bit extra workload for download_rate_monitor to take care of all
kinds of data sources. In the future, there could be more data sources, such as
screen capture, etc.

http://codereview.chromium.org/8661002/diff/1/media/base/download_rate_monito...
File media/base/download_rate_monitor.cc (right):

http://codereview.chromium.org/8661002/diff/1/media/base/download_rate_monito...
media/base/download_rate_monitor.cc:210: if (loaded_ || streaming_ ||
media_stream_)
should it be: if (loaded_ || !streaming_ || media_stream_) ?

For streaming except media_stream, download control is needed, right?

[scherkus (not reviewing), 2011-11-23 01:59:43]

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

http://codereview.chromium.org/8661002/diff/1/media/base/download_rate_monito...
media/base/download_rate_monitor.h:159: bool streaming_;
wouldn't setting streaming=true for web cams also work w/o introducing the
concept of a media stream?

[vrk (LEFT CHROMIUM), 2011-11-23 21:29:45]

> would it good to let PipelineImpl decide only two cases?
> 1. need download_rate_monitor;
> 2. don't need download_rate_monitor;
> 
> It seems a bit extra workload for download_rate_monitor to take care of all
> kinds of data sources. In the future, there could be more data sources, such
as
> screen capture, etc.

That's a good point! But I think I'd like to cross that road when we get there.
For now, I'd like to keep all the "When should we fire CanPlayThrough?"-related
logic in DownloadRateMonitor.

http://codereview.chromium.org/8661002/diff/1/media/base/download_rate_monito...
File media/base/download_rate_monitor.cc (right):

http://codereview.chromium.org/8661002/diff/1/media/base/download_rate_monito...
media/base/download_rate_monitor.cc:210: if (loaded_ || streaming_ ||
media_stream_)
On 2011/11/22 23:01:17, wjia wrote:
> should it be: if (loaded_ || !streaming_ || media_stream_) ?
> 
> For streaming except media_stream, download control is needed, right?

So to be clear, the DownloadRateMonitor does not do any download "controlling."
Its purpose is to decide when CanPlayThrough should be fired. To make this
decision, it needs a variety of input, such as some information about the media
itself (loaded_, total_bytes_, etc) and it does some estimation of the rate of
download by watching calls to SetBufferedBytes.

With that: For streaming outside of media_stream, I intentionally changed the
code to fire CanPlayThrough immediately as a design decision. With a stream,
you're either going to have a fast enough connection to keep up, or you won't.
Instead of trying to estimate whether you can keep up (which can be inaccurate
and has time overhead), might as well just fire the event and try playback. (As
I mention in the comment, this may need tweaking.)

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

http://codereview.chromium.org/8661002/diff/1/media/base/download_rate_monito...
media/base/download_rate_monitor.h:159: bool streaming_;
On 2011/11/23 01:59:44, scherkus wrote:
> wouldn't setting streaming=true for web cams also work w/o introducing the
> concept of a media stream?

There were 2 reasons why I introduced media stream:

1) Technically there is a slight difference in their definitions, the most
important difference being that a local webcam stream does not download data,
whereas every other stream type does. In terms of DownloadRateMonitor, this
means a local webcam stream *must* immediately fire CanPlayThrough, but firing
CanPlayThrough immediately for other types of streams is a design decision.

2) The SetStreaming method in PipelineImpl is part of the FilterHost interface,
meaning I had to add a differently named method to the Pipeline interface anyway
to get the WebMediaPlayerImpl -> PipelineImpl communication. So I figured I may
as well be specific.

I agree it's not ideal :/ so I'm willing to change.

Btw, in the currently checked-in code, can MediaStream be anything but a local
webcam feed? (I know the spec says it can be a remote source, but is that
implemented and checked in?) If it's always a local source, I could at least
rename things to make things clearer.

[wjia(left Chromium), 2011-11-23 21:43:10]

lgtm

[wjia(left Chromium), 2011-11-23 21:50:21]

On Wed, Nov 23, 2011 at 1:29 PM, <vrk@chromium.org> wrote:

> would it good to let PipelineImpl decide only two cases?
>> 1. need download_rate_monitor;
>> 2. don't need download_rate_monitor;
>>
>
>  It seems a bit extra workload for download_rate_monitor to take care of
>> all
>> kinds of data sources. In the future, there could be more data sources,
>> such
>>
> as
>
>> screen capture, etc.
>>
>
> That's a good point! But I think I'd like to cross that road when we get
> there.
> For now, I'd like to keep all the "When should we fire
> CanPlayThrough?"-related
> logic in DownloadRateMonitor.
>
>
>
> http://codereview.chromium.**org/8661002/diff/1/media/base/**
>
download_rate_monitor.cc<http://codereview.chromium.org/8661002/diff/1/media/base/download_rate_monitor.cc>
> File media/base/download_rate_**monitor.cc (right):
>
> http://codereview.chromium.**org/8661002/diff/1/media/base/**
>
download_rate_monitor.cc#**newcode210<http://codereview.chromium.org/8661002/diff/1/media/base/download_rate_monitor.cc#newcode210>
> media/base/download_rate_**monitor.cc:210: if (loaded_ || streaming_ ||
> media_stream_)
> On 2011/11/22 23:01:17, wjia wrote:
>
>> should it be: if (loaded_ || !streaming_ || media_stream_) ?
>>
>
>  For streaming except media_stream, download control is needed, right?
>>
>
> So to be clear, the DownloadRateMonitor does not do any download
> "controlling." Its purpose is to decide when CanPlayThrough should be
> fired. To make this decision, it needs a variety of input, such as some
> information about the media itself (loaded_, total_bytes_, etc) and it
> does some estimation of the rate of download by watching calls to
> SetBufferedBytes.
>
> With that: For streaming outside of media_stream, I intentionally
> changed the code to fire CanPlayThrough immediately as a design
> decision. With a stream, you're either going to have a fast enough
> connection to keep up, or you won't. Instead of trying to estimate
> whether you can keep up (which can be inaccurate and has time overhead),
> might as well just fire the event and try playback. (As I mention in the
> comment, this may need tweaking.)
>
>
> http://codereview.chromium.**org/8661002/diff/1/media/base/**
>
download_rate_monitor.h<http://codereview.chromium.org/8661002/diff/1/media/base/download_rate_monitor.h>
> File media/base/download_rate_**monitor.h (right):
>
> http://codereview.chromium.**org/8661002/diff/1/media/base/**
>
download_rate_monitor.h#**newcode159<http://codereview.chromium.org/8661002/diff/1/media/base/download_rate_monitor.h#newcode159>
> media/base/download_rate_**monitor.h:159: bool streaming_;
> On 2011/11/23 01:59:44, scherkus wrote:
>
>> wouldn't setting streaming=true for web cams also work w/o introducing
>>
> the
>
>> concept of a media stream?
>>
>
> There were 2 reasons why I introduced media stream:
>
> 1) Technically there is a slight difference in their definitions, the
> most important difference being that a local webcam stream does not
> download data, whereas every other stream type does. In terms of
> DownloadRateMonitor, this means a local webcam stream *must* immediately
> fire CanPlayThrough, but firing CanPlayThrough immediately for other
> types of streams is a design decision.
>
> 2) The SetStreaming method in PipelineImpl is part of the FilterHost
> interface, meaning I had to add a differently named method to the
> Pipeline interface anyway to get the WebMediaPlayerImpl -> PipelineImpl
> communication. So I figured I may as well be specific.
>
> I agree it's not ideal :/ so I'm willing to change.
>
> Btw, in the currently checked-in code, can MediaStream be anything but a
> local webcam feed? (I know the spec says it can be a remote source, but
> is that implemented and checked in?) If it's always a local source, I
> could at least rename things to make things clearer.
>
> Current media stream is just a partial check-in. It could be both local
(for preview) and remote (for display remote stream, but stream decoding
happens in webrtc video engine).


>
http://codereview.chromium.**org/8661002/<http://codereview.chromium.org/8661...
>

[vrk (LEFT CHROMIUM), 2011-11-23 23:24:51]

scherkus: PTAL. Changed loaded -> local + fallout and updated the description.

wjia: FYI the TODO in webmediaplayer_impl.cc.

[scherkus (not reviewing), 2011-11-28 21:35:03]

http://codereview.chromium.org/8661002/diff/9003/webkit/media/webmediaplayer_...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8661002/diff/9003/webkit/media/webmediaplayer_...
webkit/media/webmediaplayer_impl.cc:296: pipeline_->SetLocalSource(true);
we should try to have a single way to set this attribute

could we have DummyDemuxer or the video decoder returned from
media_stream_client_ call SetLocalSource()?

[acolwell GONE FROM CHROMIUM, 2011-11-28 21:49:55]

I'm a little late to this party, but I've got a question.

Why can't we have an IsLocal() or UsesNetwork() method on the Demuxer interface
and simply remove the FilterHost business? 

On 2011/11/28 21:35:03, scherkus wrote:
>
http://codereview.chromium.org/8661002/diff/9003/webkit/media/webmediaplayer_...
> File webkit/media/webmediaplayer_impl.cc (right):
> 
>
http://codereview.chromium.org/8661002/diff/9003/webkit/media/webmediaplayer_...
> webkit/media/webmediaplayer_impl.cc:296: pipeline_->SetLocalSource(true);
> we should try to have a single way to set this attribute
> 
> could we have DummyDemuxer or the video decoder returned from
> media_stream_client_ call SetLocalSource()?

[wjia(left Chromium), 2011-11-28 22:06:06]

On 2011/11/28 21:49:55, acolwell wrote:
> I'm a little late to this party, but I've got a question.
> 
> Why can't we have an IsLocal() or UsesNetwork() method on the Demuxer
interface
> and simply remove the FilterHost business?

http://codereview.chromium.org/8625002/ gets info from demuxer. It use bitrate
as an indicator.

> 
> On 2011/11/28 21:35:03, scherkus wrote:
> >
>
http://codereview.chromium.org/8661002/diff/9003/webkit/media/webmediaplayer_...
> > File webkit/media/webmediaplayer_impl.cc (right):
> > 
> >
>
http://codereview.chromium.org/8661002/diff/9003/webkit/media/webmediaplayer_...
> > webkit/media/webmediaplayer_impl.cc:296: pipeline_->SetLocalSource(true);
> > we should try to have a single way to set this attribute
> > 
> > could we have DummyDemuxer or the video decoder returned from
> > media_stream_client_ call SetLocalSource()?

[acolwell GONE FROM CHROMIUM, 2011-11-28 22:27:17]

On 2011/11/28 22:06:06, wjia wrote:
> On 2011/11/28 21:49:55, acolwell wrote:
> > I'm a little late to this party, but I've got a question.
> > 
> > Why can't we have an IsLocal() or UsesNetwork() method on the Demuxer
> interface
> > and simply remove the FilterHost business?
> 
> http://codereview.chromium.org/8625002/ gets info from demuxer. It use bitrate
> as an indicator.
> 
Something similar to this might work, but using bitrate is not appropriate since
lack of bitrate information at the beginning of playback does not necessarily
indicate we should fire canplaythrough immediately. We should have something
more explicit and DummyDemuxer can be modified to signal that canplaythrough
should fire immediately. 
 FFmpegDemuxer could base it's return value on information received from its
DataSource.

[scherkus (not reviewing), 2011-11-29 00:45:10]

acolwell and I chatted -- I agree with him so I'll let him take on the review
(he reviewed vrk's initial patch anyway) and remove myself

[vrk (LEFT CHROMIUM), 2011-11-29 23:54:05]

acolwell: PTAL. I moved the "IsLocal" decision to the demuxers, and here's what
it looks like... 

This feels a *bit* weird to me because isn't local/remote determined entirely by
the URL scheme? And shouldn't the local/remote criteria be identical for all/any
demuxer? (But perhaps this could be fixed in another CL.)

BTW, this CL now breaks the media/event-attributes.html layout test because
there are no progress events fired between the start of the test and the
canplaythrough event... will look into it.

[vrk (LEFT CHROMIUM), 2011-11-30 01:23:49]

> BTW, this CL now breaks the media/event-attributes.html layout test because
> there are no progress events fired between the start of the test and the
> canplaythrough event... will look into it.

Fixed the test! I had accidentally deleted a crucial line from PipelineImpl...

[acolwell GONE FROM CHROMIUM, 2011-11-30 19:26:11]

Looking good. Just a few suggestions to cleanup more of the code.

http://codereview.chromium.org/8661002/diff/21002/media/base/filter_host.h
File media/base/filter_host.h (right):

http://codereview.chromium.org/8661002/diff/21002/media/base/filter_host.h#ne...
media/base/filter_host.h:60: virtual void SetStreaming(bool streaming) = 0;
While we are in here, lets get rid of SetStreaming() too.

http://codereview.chromium.org/8661002/diff/21002/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/8661002/diff/21002/media/base/filters.h#newcod...
media/base/filters.h:163: virtual bool IsLocalSource() = 0;
I wonder if adding this method is needed since the implementations for the 2
DataSource implementations are very similar and are both scheme based. 

What do you think about adding the URL scheme checks to FFmpegDemuxerFactory and
just pass the local_source value to FFmpegDemuxer's constructor? I believe this
should work since I don't think it is valid for HTTP to redirect to file: or
data: URLs

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

http://codereview.chromium.org/8661002/diff/21002/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:1040:
download_rate_monitor_.set_local_source(local_source_);
Consider making local_source a parameter of the Start() call. streaming could
also be added as a parameter once you've added IsStreaming() to Demuxer as
mentioned earlier.

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

http://codereview.chromium.org/8661002/diff/21002/media/base/pipeline_impl.h#...
media/base/pipeline_impl.h:387: // Set by the filters to indicate whether the
data source is a streaming
Update comment to reflect that this comes from the Demuxer and not filters.

[vrk (LEFT CHROMIUM), 2011-12-01 02:11:35]

http://codereview.chromium.org/8661002/diff/21002/media/base/filter_host.h
File media/base/filter_host.h (right):

http://codereview.chromium.org/8661002/diff/21002/media/base/filter_host.h#ne...
media/base/filter_host.h:60: virtual void SetStreaming(bool streaming) = 0;
On 2011/11/30 19:26:11, acolwell wrote:
> While we are in here, lets get rid of SetStreaming() too.

Done.

http://codereview.chromium.org/8661002/diff/21002/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/8661002/diff/21002/media/base/filters.h#newcod...
media/base/filters.h:163: virtual bool IsLocalSource() = 0;
On 2011/11/30 19:26:11, acolwell wrote:
> I wonder if adding this method is needed since the implementations for the 2
> DataSource implementations are very similar and are both scheme based. 
> 
> What do you think about adding the URL scheme checks to FFmpegDemuxerFactory
and
> just pass the local_source value to FFmpegDemuxer's constructor? I believe
this
> should work since I don't think it is valid for HTTP to redirect to file: or
> data: URLs

Yeah, that's what I was thinking too. Done.

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

http://codereview.chromium.org/8661002/diff/21002/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:1040:
download_rate_monitor_.set_local_source(local_source_);
On 2011/11/30 19:26:11, acolwell wrote:
> Consider making local_source a parameter of the Start() call. streaming could
> also be added as a parameter once you've added IsStreaming() to Demuxer as
> mentioned earlier.

Done.

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

http://codereview.chromium.org/8661002/diff/21002/media/base/pipeline_impl.h#...
media/base/pipeline_impl.h:387: // Set by the filters to indicate whether the
data source is a streaming
On 2011/11/30 19:26:11, acolwell wrote:
> Update comment to reflect that this comes from the Demuxer and not filters.

Done.

[acolwell GONE FROM CHROMIUM, 2011-12-01 04:25:00]

LGTM

http://codereview.chromium.org/8661002/diff/38001/media/filters/dummy_demuxer.cc
File media/filters/dummy_demuxer.cc (right):

http://codereview.chromium.org/8661002/diff/38001/media/filters/dummy_demuxer...
media/filters/dummy_demuxer.cc:69: return false;
nit: Might want to add a brief comment here mentioning that this is always false
because the only user of this demuxer is WebRTC and such streams are not
seekable.

[scherkus (not reviewing), 2011-12-01 22:25:19]

LGTM