Fix ChunkDemuxer so it properly outputs buffered ranges.

Fix ChunkDemuxer so it properly outputs buffered ranges.


BUG=133042
TEST=None. Verified manually by a simple test page that appends some data
     and then checks the buffered attribute. I will convert this to a 
     LayoutTest soon.


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

[acolwell GONE FROM CHROMIUM, 2012-06-18 15:38:44]

On 2012/06/15 20:10:07, acolwell wrote:

ping. fischman@

[Ami GONE FROM CHROMIUM, 2012-06-18 16:18:31]

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:78: // The fake total size we use for converting
times to bytes
Yowza!  Why not instead teach Pipeline to track buffered times in addition to
buffered bytes?
(i.e. make it keep a Ranges<TimeDelta> in addition to its Ranges<int64> and make
GetBufferedTimeRanges() use whichever is present)

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:80: static int kFakeTotalBytes = 1000000;
If you need to keep this, prefer enum to static int
(and, if you keep static int, make it const)

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:657: bool ChunkDemuxer::CopyIntoRanges(
I realize this is just a move of an existing function, but it highlights a
question: why does ChunkDemuxer::Ranges exist, instead of re-using
SourceBufferStream::TimespanList ?

And, either way, can both types be replaced with a
media::Ranges<base::TimeDelta> ?  ISTM we probably want a new 
Ranges<T> IntersectionWith(const Ranges<T>& other);
method on Ranges, but otherwise it seems silly to have these various
range-maintaining one-offs.

(of course, it's quite possible Ranges didn't exist when this code was first
written; I'm just saying now it does exist can it be used here?)

Maybe/probably belongs in a different CL.

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:668: lock_.AssertAcquired();
ranges_out->clear() ?

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:670: if (!audio_ && !video_)
s/&&/||/ since you unconditionally deref them a few lines down (and, also, by
definition a missing stream intersected w/ anything else will be the empty set).

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:795: ComputeIntersection(&ranges);
Note ignored return value; a hint that CI shouldn't be returning bool in the
first place.

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:803: kFakeTotalBytes * itr->first.InSecondsF() /
duration_.InSecondsF();
at l.789 you needed to test for duration_ being TimeDelta(), but here you divide
by it unconditionally?  Divide-by-0?

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:977: host_->SetTotalBytes(kFakeTotalBytes);
Hopefully this can go away.

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer.h
File media/filters/chunk_demuxer.h (right):

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.h:133: bool ComputeIntersection(Ranges* ranges_out)
const;
I don't get why make the return value bool instead of void.
The condition currently returned as "false" is equivalent to
ranges_out->empty().
In fact, the same question arises for GetBufferedRanges() above.  Chasing its
(non-test) callers down, I think the only use is in WMPP, which in turn is only
used by WebMediaPlayerImpl::sourceBuffered(), where the return value could just
as well be ignored (and the for loop turns into a no-op).

Ditto for CopyIntoRanges().

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer...
File media/filters/chunk_demuxer_unittest.cc (right):

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer_unittest.cc:258: EXPECT_GE(buffered_bytes_,
old_buffered_bytes);
Why the change?  Rounding?

[acolwell GONE FROM CHROMIUM, 2012-06-19 06:12:34]

PTAL. Life is so much nicer w/ media::Ranges and a little cleanup.

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:78: // The fake total size we use for converting
times to bytes
On 2012/06/18 16:18:31, Ami Fischman wrote:
> Yowza!  Why not instead teach Pipeline to track buffered times in addition to
> buffered bytes?
> (i.e. make it keep a Ranges<TimeDelta> in addition to its Ranges<int64> and
make
> GetBufferedTimeRanges() use whichever is present)
Based on our offline discussion, I'd like to defer the pipeline changes until
after branch cut.

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:80: static int kFakeTotalBytes = 1000000;
On 2012/06/18 16:18:31, Ami Fischman wrote:
> If you need to keep this, prefer enum to static int
> (and, if you keep static int, make it const)

Done.

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:657: bool ChunkDemuxer::CopyIntoRanges(
On 2012/06/18 16:18:31, Ami Fischman wrote:
> I realize this is just a move of an existing function, but it highlights a
> question: why does ChunkDemuxer::Ranges exist, instead of re-using
> SourceBufferStream::TimespanList ?
> 
> And, either way, can both types be replaced with a
> media::Ranges<base::TimeDelta> ?  ISTM we probably want a new 
> Ranges<T> IntersectionWith(const Ranges<T>& other);
> method on Ranges, but otherwise it seems silly to have these various
> range-maintaining one-offs.
> 
> (of course, it's quite possible Ranges didn't exist when this code was first
> written; I'm just saying now it does exist can it be used here?)
> 
> Maybe/probably belongs in a different CL.
I've updated the CL to use media::Ranges and eliminated the other types. I
believe the code that introduce Timespans existed before Ranges existed and that
just propagated over the last few weeks instead of taking advantage of your
handy Ranges class.

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:668: lock_.AssertAcquired();
On 2012/06/18 16:18:31, Ami Fischman wrote:
> ranges_out->clear() ?

Done.

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:670: if (!audio_ && !video_)
On 2012/06/18 16:18:31, Ami Fischman wrote:
> s/&&/||/ since you unconditionally deref them a few lines down (and, also, by
> definition a missing stream intersected w/ anything else will be the empty
set).

Done.

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:795: ComputeIntersection(&ranges);
On 2012/06/18 16:18:31, Ami Fischman wrote:
> Note ignored return value; a hint that CI shouldn't be returning bool in the
> first place.

Done.

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:803: kFakeTotalBytes * itr->first.InSecondsF() /
duration_.InSecondsF();
On 2012/06/18 16:18:31, Ami Fischman wrote:
> at l.789 you needed to test for duration_ being TimeDelta(), but here you
divide
> by it unconditionally?  Divide-by-0?

ranges won't get populated if duration_ is 0 so this loop body won't run. Added
a DCHECK to make sure this assumption holds.

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer.h
File media/filters/chunk_demuxer.h (right):

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.h:133: bool ComputeIntersection(Ranges* ranges_out)
const;
On 2012/06/18 16:18:31, Ami Fischman wrote:
> I don't get why make the return value bool instead of void.
> The condition currently returned as "false" is equivalent to
> ranges_out->empty().
> In fact, the same question arises for GetBufferedRanges() above.  Chasing its
> (non-test) callers down, I think the only use is in WMPP, which in turn is
only
> used by WebMediaPlayerImpl::sourceBuffered(), where the return value could
just
> as well be ignored (and the for loop turns into a no-op).
> 
> Ditto for CopyIntoRanges().

Done.

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer...
File media/filters/chunk_demuxer_unittest.cc (right):

http://codereview.chromium.org/10558011/diff/2001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer_unittest.cc:258: EXPECT_GE(buffered_bytes_,
old_buffered_bytes);
On 2012/06/18 16:18:31, Ami Fischman wrote:
> Why the change?  Rounding?
Some of the appends are smaller than an actual frame so buffered_bytes_ doesn't
change because the time ranges don't change.

[Ami GONE FROM CHROMIUM, 2012-06-19 17:40:37]

LGTM % nits

http://codereview.chromium.org/10558011/diff/21001/media/base/ranges.h
File media/base/ranges.h (right):

http://codereview.chromium.org/10558011/diff/21001/media/base/ranges.h#newcod...
media/base/ranges.h:126: if (!size() || !other.size())
Unnecessary?

http://codereview.chromium.org/10558011/diff/21001/media/base/ranges.h#newcod...
media/base/ranges.h:140: if (end(i) < other.end(j)) {
l.140-146 would be shorter and clearer, IMO, as:
if (end(i) < other.end(j))
  ++i;
else
  ++j;

http://codereview.chromium.org/10558011/diff/21001/media/base/ranges_unittest.cc
File media/base/ranges_unittest.cc (right):

http://codereview.chromium.org/10558011/diff/21001/media/base/ranges_unittest...
media/base/ranges_unittest.cc:132: // Test intersections with partially
overlapping ranges.
Since the incrementing works based on comparing the .end()'s, IWBN to have a
test that says the right things happen even if they are equal. I.e.
{[0,1), [2,3)} \intersect {[0,3)}
and the reverse order both produce {[0,1), [2,3)}

http://codereview.chromium.org/10558011/diff/21001/media/filters/chunk_demuxe...
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/10558011/diff/21001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:79: enum { kFakeTotalBytes = 1000000 };
TODO to drop this in favor of teaching Pipeline to accept buffered times instead
of buffered bytes?

http://codereview.chromium.org/10558011/diff/21001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:155: void Seek(base::TimeDelta time);
At 21 occurrences in this file, I'd say base::TimeDelta has earned a using
statement (to be able to drop base:: from the use sites).  Your choice.

http://codereview.chromium.org/10558011/diff/21001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:680: result.Add(std::max(audio_start,
video_start),
should this be a "min" instead of a max?  Otherwise l.667-668 is a lie - not all
the data for from the last ranges is added.

But if the comment reflects what you really want to happen, why not replace
l.673-681 with simply adding the last range from each stream to result and let
range coallescing do its thing?

http://codereview.chromium.org/10558011/diff/21001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:750: DCHECK(duration_ > base::TimeDelta());
silly to do this inside the for-loop; move up as:
DCHECK(ranges.empty() || duration_ > base::TimeDelta());
?

http://codereview.chromium.org/10558011/diff/21001/media/filters/chunk_demuxe...
File media/filters/chunk_demuxer_unittest.cc (right):

http://codereview.chromium.org/10558011/diff/21001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer_unittest.cc:479: EXPECT_EQ(expected, ss.str());
reverse args?

http://codereview.chromium.org/10558011/diff/21001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer_unittest.cc:1426: CheckExpectedRanges("{ [0,92) }");
Is it easy to see that these numbers are right, here and below? (until the
ranges become more explicitly in terms of time)

http://codereview.chromium.org/10558011/diff/21001/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10558011/diff/21001/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_impl.cc:659: media::Ranges<base::TimeDelta>
buffered_ranges =
dedup against buffered()?

[acolwell GONE FROM CHROMIUM, 2012-06-19 19:50:15]

https://chromiumcodereview.appspot.com/10558011/diff/21001/media/base/ranges.h
File media/base/ranges.h (right):

https://chromiumcodereview.appspot.com/10558011/diff/21001/media/base/ranges....
media/base/ranges.h:126: if (!size() || !other.size())
On 2012/06/19 17:40:37, Ami Fischman wrote:
> Unnecessary?

Done.

https://chromiumcodereview.appspot.com/10558011/diff/21001/media/base/ranges....
media/base/ranges.h:140: if (end(i) < other.end(j)) {
On 2012/06/19 17:40:37, Ami Fischman wrote:
> l.140-146 would be shorter and clearer, IMO, as:
> if (end(i) < other.end(j))
>   ++i;
> else
>   ++j;

Done.

https://chromiumcodereview.appspot.com/10558011/diff/21001/media/base/ranges_...
File media/base/ranges_unittest.cc (right):

https://chromiumcodereview.appspot.com/10558011/diff/21001/media/base/ranges_...
media/base/ranges_unittest.cc:132: // Test intersections with partially
overlapping ranges.
On 2012/06/19 17:40:37, Ami Fischman wrote:
> Since the incrementing works based on comparing the .end()'s, IWBN to have a
> test that says the right things happen even if they are equal. I.e.
> {[0,1), [2,3)} \intersect {[0,3)}
> and the reverse order both produce {[0,1), [2,3)}

Done.

https://chromiumcodereview.appspot.com/10558011/diff/21001/media/filters/chun...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/10558011/diff/21001/media/filters/chun...
media/filters/chunk_demuxer.cc:79: enum { kFakeTotalBytes = 1000000 };
On 2012/06/19 17:40:37, Ami Fischman wrote:
> TODO to drop this in favor of teaching Pipeline to accept buffered times
instead
> of buffered bytes?

Done.

https://chromiumcodereview.appspot.com/10558011/diff/21001/media/filters/chun...
media/filters/chunk_demuxer.cc:155: void Seek(base::TimeDelta time);
On 2012/06/19 17:40:37, Ami Fischman wrote:
> At 21 occurrences in this file, I'd say base::TimeDelta has earned a using
> statement (to be able to drop base:: from the use sites).  Your choice.

Done.

https://chromiumcodereview.appspot.com/10558011/diff/21001/media/filters/chun...
media/filters/chunk_demuxer.cc:680: result.Add(std::max(audio_start,
video_start),
On 2012/06/19 17:40:37, Ami Fischman wrote:
> should this be a "min" instead of a max?  Otherwise l.667-668 is a lie - not
all
> the data for from the last ranges is added.
> 
> But if the comment reflects what you really want to happen, why not replace
> l.673-681 with simply adding the last range from each stream to result and let
> range coallescing do its thing?
I don't want min here. I only want the intersection plus whatever extra data
exists past the end of the intersection. 
The intersection represents a region where I can actually play both streams, but
in the end of stream case the data beyond the intersection will also get played
if the streams happen to be different lengths. If I used min here buffered would
lie about what ranges are actually playable because It would reflect starting
positions that only have data for one of the streams.

https://chromiumcodereview.appspot.com/10558011/diff/21001/media/filters/chun...
media/filters/chunk_demuxer.cc:750: DCHECK(duration_ > base::TimeDelta());
On 2012/06/19 17:40:37, Ami Fischman wrote:
> silly to do this inside the for-loop; move up as:
> DCHECK(ranges.empty() || duration_ > base::TimeDelta());
> ?

Done. Used !ranges.size() since empty() doesn't exist.

https://chromiumcodereview.appspot.com/10558011/diff/21001/media/filters/chun...
File media/filters/chunk_demuxer_unittest.cc (right):

https://chromiumcodereview.appspot.com/10558011/diff/21001/media/filters/chun...
media/filters/chunk_demuxer_unittest.cc:479: EXPECT_EQ(expected, ss.str());
On 2012/06/19 17:40:37, Ami Fischman wrote:
> reverse args?
This order makes |expected| appear with the error message's "Expected:" text.

https://chromiumcodereview.appspot.com/10558011/diff/21001/media/filters/chun...
media/filters/chunk_demuxer_unittest.cc:1426: CheckExpectedRanges("{ [0,92) }");
On 2012/06/19 17:40:37, Ami Fischman wrote:
> Is it easy to see that these numbers are right, here and below? (until the
> ranges become more explicitly in terms of time)
I've changed GenerateSingleStreamCluster() so that it uses timestamps instead of
a block count. This makes it clearer where the values in the ranges are coming
from.

https://chromiumcodereview.appspot.com/10558011/diff/21001/webkit/media/webme...
File webkit/media/webmediaplayer_impl.cc (right):

https://chromiumcodereview.appspot.com/10558011/diff/21001/webkit/media/webme...
webkit/media/webmediaplayer_impl.cc:659: media::Ranges<base::TimeDelta>
buffered_ranges =
On 2012/06/19 17:40:37, Ami Fischman wrote:
> dedup against buffered()?

Done. Created a ConvertToWebTimeRanges() helper function to remove the duplicate
logic.

[Ami GONE FROM CHROMIUM, 2012-06-19 20:23:38]

https://chromiumcodereview.appspot.com/10558011/diff/21001/media/filters/chun...
File media/filters/chunk_demuxer_unittest.cc (right):

https://chromiumcodereview.appspot.com/10558011/diff/21001/media/filters/chun...
media/filters/chunk_demuxer_unittest.cc:479: EXPECT_EQ(expected, ss.str());
On 2012/06/19 19:50:15, acolwell wrote:
> On 2012/06/19 17:40:37, Ami Fischman wrote:
> > reverse args?
> This order makes |expected| appear with the error message's "Expected:" text. 

True, but it runs counter to the order used in most media code, as well as
hundreds of years of mathematical notation...  Whatever.

https://chromiumcodereview.appspot.com/10558011/diff/21003/media/filters/chun...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/10558011/diff/21003/media/filters/chun...
media/filters/chunk_demuxer.cc:671: // data from the last ranges for each
stream. This allows the buffered
So then this is not completely correct, as you say in your last response.  This
should say what it actually does (extend the last intersection buffer to the max
of the last audio/video buffers).

https://chromiumcodereview.appspot.com/10558011/diff/21003/media/filters/chun...
media/filters/chunk_demuxer.cc:674: // final ranges overlap because this
indicates there is actually a location
The final ranges must overlap b/c you don't allow the transition to ENDED
otherwise.  So this comment needs to be reworded...

https://chromiumcodereview.appspot.com/10558011/diff/21003/media/filters/chun...
media/filters/chunk_demuxer.cc:681: if ((audio_start <= video_start &&
video_start <= audio_end) ||
...and this if needs to turn into a DCHECK.

https://chromiumcodereview.appspot.com/10558011/diff/21003/media/filters/chun...
media/filters/chunk_demuxer.cc:683: result.Add(std::max(audio_start,
video_start),
Once you DCHECK, this Add can be
result.Add(result.end(result.size()-1), std::max(audio_end, video_end))
to emph the "extending" nature of the operation.

[acolwell GONE FROM CHROMIUM, 2012-06-19 22:39:35]

https://chromiumcodereview.appspot.com/10558011/diff/21001/media/filters/chun...
File media/filters/chunk_demuxer_unittest.cc (right):

https://chromiumcodereview.appspot.com/10558011/diff/21001/media/filters/chun...
media/filters/chunk_demuxer_unittest.cc:479: EXPECT_EQ(expected, ss.str());
On 2012/06/19 20:23:38, Ami Fischman wrote:
> On 2012/06/19 19:50:15, acolwell wrote:
> > On 2012/06/19 17:40:37, Ami Fischman wrote:
> > > reverse args?
> > This order makes |expected| appear with the error message's "Expected:"
text. 
> 
> True, but it runs counter to the order used in most media code, as well as
> hundreds of years of mathematical notation...  Whatever.
Switched here and in source_buffer_stream_unittest.

https://chromiumcodereview.appspot.com/10558011/diff/21003/media/filters/chun...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/10558011/diff/21003/media/filters/chun...
media/filters/chunk_demuxer.cc:671: // data from the last ranges for each
stream. This allows the buffered
On 2012/06/19 20:23:38, Ami Fischman wrote:
> So then this is not completely correct, as you say in your last response. 
This
> should say what it actually does (extend the last intersection buffer to the
max
> of the last audio/video buffers).

Done.

https://chromiumcodereview.appspot.com/10558011/diff/21003/media/filters/chun...
media/filters/chunk_demuxer.cc:674: // final ranges overlap because this
indicates there is actually a location
On 2012/06/19 20:23:38, Ami Fischman wrote:
> The final ranges must overlap b/c you don't allow the transition to ENDED
> otherwise.  So this comment needs to be reworded...

Done.

https://chromiumcodereview.appspot.com/10558011/diff/21003/media/filters/chun...
media/filters/chunk_demuxer.cc:681: if ((audio_start <= video_start &&
video_start <= audio_end) ||
On 2012/06/19 20:23:38, Ami Fischman wrote:
> ...and this if needs to turn into a DCHECK.

Done.

https://chromiumcodereview.appspot.com/10558011/diff/21003/media/filters/chun...
media/filters/chunk_demuxer.cc:683: result.Add(std::max(audio_start,
video_start),
On 2012/06/19 20:23:38, Ami Fischman wrote:
> Once you DCHECK, this Add can be
> result.Add(result.end(result.size()-1), std::max(audio_end, video_end))
> to emph the "extending" nature of the operation.

Done.

[commit-bot: I haz the power, 2012-06-19 23:29:45]

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

[commit-bot: I haz the power, 2012-06-20 00:51:57]

Change committed as 143106