Calculate the buffered ranges in ChunkDemuxer::GetBufferedRanges()

Calculate the buffered ranges in ChunkDemuxer::GetBufferedRanges()

BUG=129852
TEST=media_unittests


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

[annacc, 2012-06-12 20:23:44]

Ready for review!

[acolwell GONE FROM CHROMIUM, 2012-06-12 22:48:13]

The approach looks good. Just a few comments to make things a little easier to
read.

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer.cc:598: // Only include ranges that have been
buffered in |audio_|
Move this to a helper method so we don't have duplicate code below. Something
along the lines of bool CopyIntoRanges(const SourceBufferStream::TimespanList&
timespans, Ranges* ranges_out);

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer.cc:629: 
Precompute the "last ranges" condition into a bool here instead of in
AddIntersectionRange() & use (itr + 1) instead of next(). Pass this bool into
the AddIntersectionRange() instead of the iterators. I think that will make
things easier to read.

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer.cc:633: AddIntersectionRange(audio_ranges_itr,
video_ranges_itr,
How about passing in *xxx_itr instead of the iterator itself and moving the
iterator increment outside of   AddIntersectionRange()? I think it would make
the body of  AddIntersectionRange() easier to read.

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer.cc:650: // No overlap was found.  Increment the
smallest one and keep looking.
nit: smallest -> earliest?

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer.cc:672: next(itr_a) == itr_a_end &&
nit: Use (itr_a + 1) instead.

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer.cc:684:
SourceBufferStream::TimespanList::const_iterator ChunkDemuxer::next(
nit: I'd prefer you use (itr + 1) instead of defining this method.

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
File media/filters/chunk_demuxer_unittest.cc (right):

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:459: void
CheckExpectedRanges(ChunkDemuxer::Ranges expected_times) {
nit: const&

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:1415: kAudioTrackNum,
kAudioBlockDuration));
nit: fix indent

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:1425: kAudioTrackNum,
kAudioBlockDuration));
nit: fix indent

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:1429:
expected.push_back(CreateRange(150, 3, kAudioBlockDuration));
nit: perhaps move this up by the cluster_2 decl so that it's easy to see that
the parameters here match the cluster parameters.

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:1444: kVideoTrackNum,
kVideoBlockDuration));
nit: fix indent.

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:1449: expected.push_back(CreateRange(0,
4, kVideoBlockDuration));
nit: move this near cluster creation here and all similar instances below just
to make it easier to see that the cluster and range parameters match.

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:1454: kVideoTrackNum,
kVideoBlockDuration));
nit: fix indent.

[vrk (LEFT CHROMIUM), 2012-06-12 22:54:01]

driveby: I think acolwell@ suggests using "itr + 1", which you can't use on an
std::list (the underlying data structure for TimespanList), but you can use on
std::vector and I think std::deque. Feel free to change the type of TimespanList
accordingly!

[acolwell GONE FROM CHROMIUM, 2012-06-12 23:08:02]

On 2012/06/12 22:54:01, Victoria Kirst wrote:
> driveby: I think acolwell@ suggests using "itr + 1", which you can't use on an
> std::list (the underlying data structure for TimespanList), but you can use on
> std::vector and I think std::deque. Feel free to change the type of
TimespanList
> accordingly!

oh.. right. You could also use --xx_ranges.end() instead. It might be good to
put these in xxx_last_range temporary variables so it is easy to see what you
are trying to do here.

[annacc, 2012-06-13 00:00:01]

Thanks all for the suggestions!  I've taken vrk's suggestion to change
TimespanList to a deque.

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer.cc:598: // Only include ranges that have been
buffered in |audio_|
On 2012/06/12 22:48:13, acolwell wrote:
> Move this to a helper method so we don't have duplicate code below. Something
> along the lines of bool CopyIntoRanges(const SourceBufferStream::TimespanList&
> timespans, Ranges* ranges_out);

Done.

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer.cc:629: 
On 2012/06/12 22:48:13, acolwell wrote:
> Precompute the "last ranges" condition into a bool here instead of in
> AddIntersectionRange() & use (itr + 1) instead of next(). Pass this bool into
> the AddIntersectionRange() instead of the iterators. I think that will make
> things easier to read.

Done.

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer.cc:633: AddIntersectionRange(audio_ranges_itr,
video_ranges_itr,
On 2012/06/12 22:48:13, acolwell wrote:
> How about passing in *xxx_itr instead of the iterator itself and moving the
> iterator increment outside of   AddIntersectionRange()? I think it would make
> the body of  AddIntersectionRange() easier to read.

I think I've realized your suggestion here, but you should check :)

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer.cc:650: // No overlap was found.  Increment the
smallest one and keep looking.
On 2012/06/12 22:48:13, acolwell wrote:
> nit: smallest -> earliest?

Done.

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer.cc:672: next(itr_a) == itr_a_end &&
On 2012/06/12 22:48:13, acolwell wrote:
> nit: Use (itr_a + 1) instead.

Done.

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer.cc:684:
SourceBufferStream::TimespanList::const_iterator ChunkDemuxer::next(
On 2012/06/12 22:48:13, acolwell wrote:
> nit: I'd prefer you use (itr + 1) instead of defining this method.

Done.

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
File media/filters/chunk_demuxer_unittest.cc (right):

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:459: void
CheckExpectedRanges(ChunkDemuxer::Ranges expected_times) {
On 2012/06/12 22:48:13, acolwell wrote:
> nit: const&

I can make it a ref, but not const.

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:1415: kAudioTrackNum,
kAudioBlockDuration));
On 2012/06/12 22:48:13, acolwell wrote:
> nit: fix indent

Done.

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:1425: kAudioTrackNum,
kAudioBlockDuration));
On 2012/06/12 22:48:13, acolwell wrote:
> nit: fix indent

Done.

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:1429:
expected.push_back(CreateRange(150, 3, kAudioBlockDuration));
On 2012/06/12 22:48:13, acolwell wrote:
> nit: perhaps move this up by the cluster_2 decl so that it's easy to see that
> the parameters here match the cluster parameters.

Done.

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:1444: kVideoTrackNum,
kVideoBlockDuration));
On 2012/06/12 22:48:13, acolwell wrote:
> nit: fix indent.

Done.

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:1449: expected.push_back(CreateRange(0,
4, kVideoBlockDuration));
On 2012/06/12 22:48:13, acolwell wrote:
> nit: move this near cluster creation here and all similar instances below just
> to make it easier to see that the cluster and range parameters match.

Done.

https://chromiumcodereview.appspot.com/10539115/diff/1005/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:1454: kVideoTrackNum,
kVideoBlockDuration));
On 2012/06/12 22:48:13, acolwell wrote:
> nit: fix indent.

Done.

[acolwell GONE FROM CHROMIUM, 2012-06-13 00:33:40]

LGTM % nits

https://chromiumcodereview.appspot.com/10539115/diff/4004/media/filters/chunk...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/10539115/diff/4004/media/filters/chunk...
media/filters/chunk_demuxer.cc:617: 
nit: remove blank line

https://chromiumcodereview.appspot.com/10539115/diff/4004/media/filters/chunk...
media/filters/chunk_demuxer.cc:664: return success;
nit: You can use !timestamps.empty() here instead.

https://chromiumcodereview.appspot.com/10539115/diff/4004/media/filters/chunk...
File media/filters/chunk_demuxer_unittest.cc (right):

https://chromiumcodereview.appspot.com/10539115/diff/4004/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:464: for (ChunkDemuxer::Ranges::iterator
actual_itr =
nit: if you use a const_iterator here I'm pretty sure you can make
expected_times const.

[annacc, 2012-06-13 03:35:43]

Thanks so much!

https://chromiumcodereview.appspot.com/10539115/diff/4004/media/filters/chunk...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/10539115/diff/4004/media/filters/chunk...
media/filters/chunk_demuxer.cc:617: 
On 2012/06/13 00:33:40, acolwell wrote:
> nit: remove blank line

Done.

https://chromiumcodereview.appspot.com/10539115/diff/4004/media/filters/chunk...
media/filters/chunk_demuxer.cc:664: return success;
On 2012/06/13 00:33:40, acolwell wrote:
> nit: You can use !timestamps.empty() here instead.

Done.

https://chromiumcodereview.appspot.com/10539115/diff/4004/media/filters/chunk...
File media/filters/chunk_demuxer_unittest.cc (right):

https://chromiumcodereview.appspot.com/10539115/diff/4004/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:464: for (ChunkDemuxer::Ranges::iterator
actual_itr =
On 2012/06/13 00:33:40, acolwell wrote:
> nit: if you use a const_iterator here I'm pretty sure you can make
> expected_times const.

Ah, thanks.

[commit-bot: I haz the power, 2012-06-13 04:02:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/annacc@chromium.org/10539115/7005

[commit-bot: I haz the power, 2012-06-13 05:09:40]

Change committed as 141859