Track buffered byte ranges correctly in media::Pipeline.

Track buffered byte ranges correctly in media::Pipeline.

Previously, the interaction was:
BufferedDataSource: hey Pipeline, I just read byte X
Pipeline: cool story bro!  I'll just pretend you've read every single byte from 0 to X.

Now the interaction is:
BufferedDataSource: hey Pipeline, I just read bytes X-Y
Pipeline: neato!  I'll just add that range to my list of buffered ranges.

The most noticeable outcome of this change is that seeking in a media format that requires reading a seek index from the end of the file (e.g. WebM w/ CUES at the end) no longer results in an almost-instant claim of having buffered the entire video just because a seek was completed (esp. dramatic when viewing a very large file, such as a multi-hour video).

BUG=103513, 127355
TEST=besides unittests, this allows a cleaned-up version of http/tests/media/video-buffered.html to be un-SKIPped!


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

[Ami GONE FROM CHROMIUM, 2012-05-26 05:18:25]

Note this needs to wait for http://trac.webkit.org/changeset/118577 to roll into
chromium DEPS.

[scherkus (not reviewing), 2012-05-29 18:13:29]

https://chromiumcodereview.appspot.com/10451049/diff/2001/media/base/data_sou...
File media/base/data_source.h (right):

https://chromiumcodereview.appspot.com/10451049/diff/2001/media/base/data_sou...
media/base/data_source.h:21: // Notify the host that byte range [start,end] has
been buffered.
I applaud your use of mathematical interval notation!

https://chromiumcodereview.appspot.com/10451049/diff/2001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://chromiumcodereview.appspot.com/10451049/diff/2001/media/base/pipeline...
media/base/pipeline.cc:206: return buffered_time_ranges_;
would it be simpler to compute buffered_time_ranges_ off of
buffered_byte_ranges_ here instead of handling it on the fly in SetDuration() et
al?

https://chromiumcodereview.appspot.com/10451049/diff/2001/media/base/pipeline...
media/base/pipeline.cc:242: void Pipeline::SetCurrentReadPosition(int64 offset)
{
AFAIK this is still called by FFDemuxer but I can't remember why or whether it's
still useful.

Do you happen to know?

https://chromiumcodereview.appspot.com/10451049/diff/2001/media/base/pipeline...
media/base/pipeline.cc:249: // buffered_byte_ranges_ and current_bytes_ to
always be legal values in
|var|

https://chromiumcodereview.appspot.com/10451049/diff/2001/media/base/pipeline.h
File media/base/pipeline.h (right):

https://chromiumcodereview.appspot.com/10451049/diff/2001/media/base/pipeline...
media/base/pipeline.h:482: // Amount of available buffered data.  Set by
filters.
s/filters/data source/?

technically it's the demuxer because DemuxerHost extends DataSourceHost :s

[Ami GONE FROM CHROMIUM, 2012-05-29 20:45:44]

https://chromiumcodereview.appspot.com/10451049/diff/2001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://chromiumcodereview.appspot.com/10451049/diff/2001/media/base/pipeline...
media/base/pipeline.cc:206: return buffered_time_ranges_;
On 2012/05/29 18:13:29, scherkus wrote:
> would it be simpler to compute buffered_time_ranges_ off of
> buffered_byte_ranges_ here instead of handling it on the fly in SetDuration()
et
> al?

Hmm; that does indeed simplify the code.  There is a small cost here in that
this method is called per UI refresh, whereas the others are called only for
network updates (much less frequent), but I don't think that's worth worrying
about.  Done.

https://chromiumcodereview.appspot.com/10451049/diff/2001/media/base/pipeline...
media/base/pipeline.cc:242: void Pipeline::SetCurrentReadPosition(int64 offset)
{
On 2012/05/29 18:13:29, scherkus wrote:
> AFAIK this is still called by FFDemuxer but I can't remember why or whether
it's
> still useful.
> 
> Do you happen to know?

Hahahahahahahahaha.
current_bytes_ is (was) a write-only variable!

https://chromiumcodereview.appspot.com/10451049/diff/2001/media/base/pipeline...
media/base/pipeline.cc:249: // buffered_byte_ranges_ and current_bytes_ to
always be legal values in
On 2012/05/29 18:13:29, scherkus wrote:
> |var|

Done.

https://chromiumcodereview.appspot.com/10451049/diff/2001/media/base/pipeline.h
File media/base/pipeline.h (right):

https://chromiumcodereview.appspot.com/10451049/diff/2001/media/base/pipeline...
media/base/pipeline.h:482: // Amount of available buffered data.  Set by
filters.
On 2012/05/29 18:13:29, scherkus wrote:
> s/filters/data source/?
> 
> technically it's the demuxer because DemuxerHost extends DataSourceHost :s

Just removed that part of the comment (pointers to {Demuxer,DataSource}Host
exist at the AddBufferedByteRange declaration above).

[scherkus (not reviewing), 2012-05-29 21:01:48]

LGTM w/ one nit

me likey!

https://chromiumcodereview.appspot.com/10451049/diff/3020/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://chromiumcodereview.appspot.com/10451049/diff/3020/media/base/pipeline...
media/base/pipeline.cc:205: Ranges<base::TimeDelta> time_ranges;
s/base:://

https://chromiumcodereview.appspot.com/10451049/diff/3020/media/base/pipeline...
media/base/pipeline.cc:476: void Pipeline::SetNetworkActivity(bool
is_downloading_data) {
hmm... how soon until this + network_cb_ can go away :)

[Ami GONE FROM CHROMIUM, 2012-05-29 21:08:37]

https://chromiumcodereview.appspot.com/10451049/diff/3020/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://chromiumcodereview.appspot.com/10451049/diff/3020/media/base/pipeline...
media/base/pipeline.cc:205: Ranges<base::TimeDelta> time_ranges;
On 2012/05/29 21:01:48, scherkus wrote:
> s/base:://

Done.

https://chromiumcodereview.appspot.com/10451049/diff/3020/media/base/pipeline...
media/base/pipeline.cc:476: void Pipeline::SetNetworkActivity(bool
is_downloading_data) {
On 2012/05/29 21:01:48, scherkus wrote:
> hmm... how soon until this + network_cb_ can go away :)

What is this I don't even.
Pretty soon, I'm guessing (from a bit of spelunking).
But I don't want to hold up the cascade of other CLs I have behind this :)

[commit-bot: I haz the power, 2012-05-29 21:09:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/10451049/12020

[scherkus (not reviewing), 2012-05-29 21:11:35]

On 2012/05/29 21:08:37, Ami Fischman wrote:
>
https://chromiumcodereview.appspot.com/10451049/diff/3020/media/base/pipeline.cc
> File media/base/pipeline.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10451049/diff/3020/media/base/pipeline...
> media/base/pipeline.cc:205: Ranges<base::TimeDelta> time_ranges;
> On 2012/05/29 21:01:48, scherkus wrote:
> > s/base:://
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10451049/diff/3020/media/base/pipeline...
> media/base/pipeline.cc:476: void Pipeline::SetNetworkActivity(bool
> is_downloading_data) {
> On 2012/05/29 21:01:48, scherkus wrote:
> > hmm... how soon until this + network_cb_ can go away :)
> 
> What is this I don't even.
> Pretty soon, I'm guessing (from a bit of spelunking).
> But I don't want to hold up the cascade of other CLs I have behind this :)

Yeah I didn't mean to include it in this CL but rather highlight it as another
area that could likely get flamethrowered

[commit-bot: I haz the power, 2012-05-30 02:17:58]

Change committed as 139452

[Ami GONE FROM CHROMIUM, 2012-06-10 21:05:27]

FYI

https://chromiumcodereview.appspot.com/10451049/diff/3020/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://chromiumcodereview.appspot.com/10451049/diff/3020/media/base/pipeline...
media/base/pipeline.cc:476: void Pipeline::SetNetworkActivity(bool
is_downloading_data) {
On 2012/05/29 21:08:37, Ami Fischman wrote:
> On 2012/05/29 21:01:48, scherkus wrote:
> > hmm... how soon until this + network_cb_ can go away :)
> 
> What is this I don't even.
> Pretty soon, I'm guessing (from a bit of spelunking).
> But I don't want to hold up the cascade of other CLs I have behind this :)

This is now https://chromiumcodereview.appspot.com/10535101/