Implement start, end, and middle overlaps for SourceBufferStream

Implement start, end, and middle overlaps for SourceBufferStream

Handles cases where the selected range is overlapped.
Does not handle buffer sizes of differing durations.

BUG=126560, 125072
TEST=media_unittests

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

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

Whew...done. This is quite a CL. Your unit tests are wonderful. They are really
good at explaining the various scenarios they are testing. Good job.

http://codereview.chromium.org/10389185/diff/2001/media/filters/source_buffer...
File media/filters/source_buffer_stream.cc (right):

http://codereview.chromium.org/10389185/diff/2001/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:23: SourceBufferRange(const BufferQueue&
new_buffers);
add explicit

http://codereview.chromium.org/10389185/diff/2001/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:34: // If |transfer_current_position| is
true, |range|'s |next_buffer_position_|
next_buffer_position_ -> next_buffer_index_?

http://codereview.chromium.org/10389185/diff/2001/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:61: // this range, and |next_buffer|
points to the deleted next buffer or to
nit: Perhaps reword "deleted next buffer" to something like "buffer in
|deleted_buffers| that |next_buffer_index_| pointed to."

http://codereview.chromium.org/10389185/diff/2001/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:62: // |deleted_buffers.end()| if the next
buffer was not deleted.
nit: "the next buffer was not deleted" -> "|next_buffer_index_| did not point to
any buffer added to |deleted_buffers|"

http://codereview.chromium.org/10389185/diff/2001/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:63: void
DeleteAfter(scoped_refptr<StreamParserBuffer> buffer,
nit: It might be good to mention that keyframe_map_ entries for the deleted
buffers are also removed.

http://codereview.chromium.org/10389185/diff/2001/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:88: // the last buffer's timestamp + its
duration.) Returns kNoTimestamp if the
nit: Can we have an empty range? If not remove that part of the comment please.

http://codereview.chromium.org/10389185/diff/2001/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:307: selected_range_ =
range_with_new_buffers;
This might not be necessary. It looks like UpdateTrackBuffer() does this.

http://codereview.chromium.org/10389185/diff/2001/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:337: next_range_itr =
ranges_.erase(next_range_itr);
I think it would be better to put this above the previous line so that it is
closer to the ownership transfer.

http://codereview.chromium.org/10389185/diff/2001/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:362:
UpdateTrackBuffer(range_with_new_buffers_itr, deleted_buffers, next_buffer);
You might want to add a comment here indicating that selected_range_ gets
overridden in this call so it doesn't look like we are keeping a pointer to
overlapped_range around.

http://codereview.chromium.org/10389185/diff/2001/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:400: if (next_range_itr != ranges_.end())
{
I don't think I really understand what is going on here. Is this where the track
buffer goes off the end of the old range, there is a gap, but overlaps with the
next range?

http://codereview.chromium.org/10389185/diff/2001/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:647: return BufferedEnd();
nit: One of these two methods should be removed since they are doing the same
thing.

http://codereview.chromium.org/10389185/diff/2001/media/filters/source_buffer...
File media/filters/source_buffer_stream.h (right):

http://codereview.chromium.org/10389185/diff/2001/media/filters/source_buffer...
media/filters/source_buffer_stream.h:77: // points to the next buffer in
|deleted_buffers| that was deleted. Assumes
The "that was deleted" here confuses me. I don't really understand what you are
trying to say here. Is this the next buffer that should be returned from
GetNextBuffer()?

http://codereview.chromium.org/10389185/diff/2001/media/filters/source_buffer...
File media/filters/source_buffer_stream_unittest.cc (right):

http://codereview.chromium.org/10389185/diff/2001/media/filters/source_buffer...
media/filters/source_buffer_stream_unittest.cc:328: Seek(19);
nit: Perhaps check to make sure we don't have 13 before the seek?

http://codereview.chromium.org/10389185/diff/2001/media/filters/source_buffer...
media/filters/source_buffer_stream_unittest.cc:544:
CheckExpectedTimespans(expected);
nit: Consider adding a helper TimespanList CheckExpectedTimespans(int, int)
method so you don't have to repeat this 3 line construct so often. 

If you wanted to get super fancy you could also add a varargs one too for the
multirange case. (ie. TimespanList CheckExpectedTimespans(int range_count, ...))
Then you could do something like CheckExpectedTimespans(2, CreateTimespan(0, 9),
CreateTimespan(15, 19))

http://codereview.chromium.org/10389185/diff/2001/media/filters/source_buffer...
media/filters/source_buffer_stream_unittest.cc:693: Seek(10);
nit: Perhaps verify 8 is really gone?

http://codereview.chromium.org/10389185/diff/2001/media/filters/source_buffer...
media/filters/source_buffer_stream_unittest.cc:851:
EXPECT_FALSE(stream_.GetNextBuffer(&buffer));
nit: Perhaps create a CheckNoNextBuffer() helper and use everywhere instead?

http://codereview.chromium.org/10389185/diff/2001/media/filters/source_buffer...
media/filters/source_buffer_stream_unittest.cc:994: // after: |A a a a a B b b| 
 |A a a a a|
You should add the track buffer line here like in the examples above.

[vrk (LEFT CHROMIUM), 2012-05-24 20:06:30]

Thanks for getting through all that, Aaron! :)

https://chromiumcodereview.appspot.com/10389185/diff/2001/media/filters/sourc...
File media/filters/source_buffer_stream.cc (right):

https://chromiumcodereview.appspot.com/10389185/diff/2001/media/filters/sourc...
media/filters/source_buffer_stream.cc:23: SourceBufferRange(const BufferQueue&
new_buffers);
On 2012/05/22 19:35:23, acolwell wrote:
> add explicit

Good catch, done.

https://chromiumcodereview.appspot.com/10389185/diff/2001/media/filters/sourc...
media/filters/source_buffer_stream.cc:34: // If |transfer_current_position| is
true, |range|'s |next_buffer_position_|
On 2012/05/22 19:35:23, acolwell wrote:
> next_buffer_position_ -> next_buffer_index_?

Done.

https://chromiumcodereview.appspot.com/10389185/diff/2001/media/filters/sourc...
media/filters/source_buffer_stream.cc:61: // this range, and |next_buffer|
points to the deleted next buffer or to
On 2012/05/22 19:35:23, acolwell wrote:
> nit: Perhaps reword "deleted next buffer" to something like "buffer in
> |deleted_buffers| that |next_buffer_index_| pointed to."

Did some rewording, and also deleted the other comments for DeleteAll and the
private version of DeleteAfter to reduce mistakes from copy/paste.

https://chromiumcodereview.appspot.com/10389185/diff/2001/media/filters/sourc...
media/filters/source_buffer_stream.cc:62: // |deleted_buffers.end()| if the next
buffer was not deleted.
On 2012/05/22 19:35:23, acolwell wrote:
> nit: "the next buffer was not deleted" -> "|next_buffer_index_| did not point
to
> any buffer added to |deleted_buffers|"

Done.

https://chromiumcodereview.appspot.com/10389185/diff/2001/media/filters/sourc...
media/filters/source_buffer_stream.cc:63: void
DeleteAfter(scoped_refptr<StreamParserBuffer> buffer,
On 2012/05/22 19:35:23, acolwell wrote:
> nit: It might be good to mention that keyframe_map_ entries for the deleted
> buffers are also removed.

Done.

https://chromiumcodereview.appspot.com/10389185/diff/2001/media/filters/sourc...
media/filters/source_buffer_stream.cc:88: // the last buffer's timestamp + its
duration.) Returns kNoTimestamp if the
On 2012/05/22 19:35:23, acolwell wrote:
> nit: Can we have an empty range? If not remove that part of the comment
please.

It is possible (e.g. after completely overlapping a Range and DeleteAll() is
called), but the expectation is that an empty Range will be immediately deleted,
and there should be no empty Ranges in |ranges_|.

Anyway, you're right that the comment should be deleted and the method should
DCHECK instead of doing the if statement. Done.

https://chromiumcodereview.appspot.com/10389185/diff/2001/media/filters/sourc...
media/filters/source_buffer_stream.cc:307: selected_range_ =
range_with_new_buffers;
On 2012/05/22 19:35:23, acolwell wrote:
> This might not be necessary. It looks like UpdateTrackBuffer() does this.

Yes, you're right, removed and added DCHECK_NE(selected_range_, *next_range_itr)

https://chromiumcodereview.appspot.com/10389185/diff/2001/media/filters/sourc...
media/filters/source_buffer_stream.cc:337: next_range_itr =
ranges_.erase(next_range_itr);
On 2012/05/22 19:35:23, acolwell wrote:
> I think it would be better to put this above the previous line so that it is
> closer to the ownership transfer.

Good idea, done.

https://chromiumcodereview.appspot.com/10389185/diff/2001/media/filters/sourc...
media/filters/source_buffer_stream.cc:362:
UpdateTrackBuffer(range_with_new_buffers_itr, deleted_buffers, next_buffer);
On 2012/05/22 19:35:23, acolwell wrote:
> You might want to add a comment here indicating that selected_range_ gets
> overridden in this call so it doesn't look like we are keeping a pointer to
> overlapped_range around.

Added a DCHECK and comment.

https://chromiumcodereview.appspot.com/10389185/diff/2001/media/filters/sourc...
media/filters/source_buffer_stream.cc:400: if (next_range_itr != ranges_.end())
{
On 2012/05/22 19:35:23, acolwell wrote:
> I don't think I really understand what is going on here. Is this where the
track
> buffer goes off the end of the old range, there is a gap, but overlaps with
the
> next range?

Close: the track buffer should never overlap with the next range, but it might
be adjacent to it. This logic is used in between
End_Overlap_Selected_OverlappedByNew_2. Let me copy/paste that example:
 
 index:  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0
 old  :           |A*a*a a a A a a a a|
 new  :  B b b b b B b
 after: |B b b b b B b|     |A a a a a|
 track:             |a a a a|

So in this beginning of this method, we set |selected_range_| to
|range_with_new_buffers|, which is the left-most range with the B's in it. The
while loop adds the A's in the track buffer.  

Now this weird logic from lines 396-407 is checking to see if the track buffer
ends where the next range begins. In the EOS_OBN_2 example, this block is all
true, and we set the |selected_range_| to the right-most range with the A's in
it, so that the track buffer seamlessly transitions to the existing data.

If lines 396-407 were omitted, then the |selected_range_| is the left-most
range. Because it doesn't have a keyframe after the end of the track buffer,  it
is in the "waiting for next keyframe" mode. After the track buffer has been
depleted, you'd stall until you get the next keyframe in the left-most range...
which will be overlapping the right-most range.

Hope that makes sense... btw, it could make sense to delete lines 396-407 if we
did some conveniently placed garbage collecting. We should nail down the
simplifications, possibly in a follow-up CL.

https://chromiumcodereview.appspot.com/10389185/diff/2001/media/filters/sourc...
media/filters/source_buffer_stream.cc:647: return BufferedEnd();
On 2012/05/22 19:35:23, acolwell wrote:
> nit: One of these two methods should be removed since they are doing the same
> thing.

Done.

https://chromiumcodereview.appspot.com/10389185/diff/2001/media/filters/sourc...
File media/filters/source_buffer_stream.h (right):

https://chromiumcodereview.appspot.com/10389185/diff/2001/media/filters/sourc...
media/filters/source_buffer_stream.h:77: // points to the next buffer in
|deleted_buffers| that was deleted. Assumes
On 2012/05/22 19:35:23, acolwell wrote:
> The "that was deleted" here confuses me. I don't really understand what you
are
> trying to say here. Is this the next buffer that should be returned from
> GetNextBuffer()?

Yeah sorry, I didn't know how to describe this without using
|next_buffer_index_|! But "next buffer to be returned from GetNextBuffer()"
sounds good. Reworded comment!

https://chromiumcodereview.appspot.com/10389185/diff/2001/media/filters/sourc...
File media/filters/source_buffer_stream_unittest.cc (right):

https://chromiumcodereview.appspot.com/10389185/diff/2001/media/filters/sourc...
media/filters/source_buffer_stream_unittest.cc:328: Seek(19);
On 2012/05/22 19:35:23, acolwell wrote:
> nit: Perhaps check to make sure we don't have 13 before the seek?

Done.

https://chromiumcodereview.appspot.com/10389185/diff/2001/media/filters/sourc...
media/filters/source_buffer_stream_unittest.cc:544:
CheckExpectedTimespans(expected);
On 2012/05/22 19:35:23, acolwell wrote:
> nit: Consider adding a helper TimespanList CheckExpectedTimespans(int, int)
> method so you don't have to repeat this 3 line construct so often. 
Done.

> If you wanted to get super fancy you could also add a varargs one too for the
> multirange case. (ie. TimespanList CheckExpectedTimespans(int range_count,
...))
> Then you could do something like CheckExpectedTimespans(2, CreateTimespan(0,
9),
> CreateTimespan(15, 19))

Tried to be fancy! Unfortunately, looks like C++ varargs only work with POD
types, and Timespan is not a POD. Even if I made Timespan a struct instead of
std::pair, TimeDelta is not a POD so its container can't be a POD. I decided a
list of ints like CheckExpectedTimespans(0, 5, 10, 14) was more confusing to
read, and could have made a struct Range { int start; int end }; specifically
for this unit test, but I decided it wasn't worth it!

tl;dr: didn't change this!

https://chromiumcodereview.appspot.com/10389185/diff/2001/media/filters/sourc...
media/filters/source_buffer_stream_unittest.cc:693: Seek(10);
On 2012/05/22 19:35:23, acolwell wrote:
> nit: Perhaps verify 8 is really gone?

Done.

https://chromiumcodereview.appspot.com/10389185/diff/2001/media/filters/sourc...
media/filters/source_buffer_stream_unittest.cc:851:
EXPECT_FALSE(stream_.GetNextBuffer(&buffer));
On 2012/05/22 19:35:23, acolwell wrote:
> nit: Perhaps create a CheckNoNextBuffer() helper and use everywhere instead?

Yeah, that's a good idea -- easier to read, too. Done.

https://chromiumcodereview.appspot.com/10389185/diff/2001/media/filters/sourc...
media/filters/source_buffer_stream_unittest.cc:994: // after: |A a a a a B b b| 
 |A a a a a|
On 2012/05/22 19:35:23, acolwell wrote:
> You should add the track buffer line here like in the examples above.

Oops, yes! Done.

[vrk (LEFT CHROMIUM), 2012-05-24 21:09:32]

BTW, I just created crbug.com/129623 documenting my discontent with the code in
this CL along with the ideas in my head to simplify the code. (Would like to
implement those ideas in follow-up CLs!)

[acolwell GONE FROM CHROMIUM, 2012-05-24 22:20:59]

LGTM. Coordinate with annacc@ on when to checkin so merge pain can be minimized.
:)

https://chromiumcodereview.appspot.com/10389185/diff/9001/media/filters/sourc...
File media/filters/source_buffer_stream.cc (right):

https://chromiumcodereview.appspot.com/10389185/diff/9001/media/filters/sourc...
media/filters/source_buffer_stream.cc:211: if (range_value < 0)
nit: Keep the "|start_timestamp| is before the current range in this loop." just
to remind people what this means?

[vrk (LEFT CHROMIUM), 2012-05-25 15:48:08]

Thanks Aaron!!

annacc: I'll wait to land this until after you land your CL!

https://chromiumcodereview.appspot.com/10389185/diff/9001/media/filters/sourc...
File media/filters/source_buffer_stream.cc (right):

https://chromiumcodereview.appspot.com/10389185/diff/9001/media/filters/sourc...
media/filters/source_buffer_stream.cc:211: if (range_value < 0)
On 2012/05/24 22:21:00, acolwell wrote:
> nit: Keep the "|start_timestamp| is before the current range in this loop."
just
> to remind people what this means?

Done.