Remove lingering frames of old data when appending a new segment

Remove lingering frames of old data when appending a new segment

This CL adds a check in SourceBufferStream to delete old frames whose
timestamps are in between the timestamps of the new frames overlapping it.

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

[vrk (LEFT CHROMIUM), 2012-09-19 21:05:55]

Reworked into 3 diffs as discussed!

[acolwell GONE FROM CHROMIUM, 2012-09-19 23:15:51]

LGTM % minor nits

https://codereview.chromium.org/10952034/diff/1001/media/filters/source_buffe...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/10952034/diff/1001/media/filters/source_buffe...
media/filters/source_buffer_stream.cc:335:
!(*range_for_next_append_)->AreAdjacentInSequence(
This is a little awkward because it calls into the range just so the range can
call back into this class for the approximate duration value. I think making
this a helper method on SourceBufferStream would be better and just keep
SourceBufferRange::IsNextInSequence() as is for now.

You could create a static TimeDelta GetFudge(TimeDelta approximate_duration)
helper to avoid SourceBufferStream & SourceBufferRange getting out of sync.

https://codereview.chromium.org/10952034/diff/2002/media/filters/source_buffe...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/10952034/diff/2002/media/filters/source_buffe...
media/filters/source_buffer_stream.cc:602: *deleted_buffers = saved_buffers;
DCHECK(deleted_buffers->empty()) right before this?

https://codereview.chromium.org/10952034/diff/2002/media/filters/source_buffe...
media/filters/source_buffer_stream.cc:1140: } else {
nit: drop else and deindent

https://codereview.chromium.org/10952034/diff/2002/media/filters/source_buffe...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/10952034/diff/2002/media/filters/source_buffe...
media/filters/source_buffer_stream_unittest.cc:107: EXPECT_EQ(ss.str(),
expected);
nit: flip

https://codereview.chromium.org/10952034/diff/2002/media/filters/source_buffe...
media/filters/source_buffer_stream_unittest.cc:121: EXPECT_EQ(ss.str(),
expected);
nit: flip

https://codereview.chromium.org/10952034/diff/2002/media/filters/source_buffe...
media/filters/source_buffer_stream_unittest.cc:186: EXPECT_EQ(status,
SourceBufferStream::kSuccess);
nit:flip order.

https://codereview.chromium.org/10952034/diff/2002/media/filters/source_buffe...
media/filters/source_buffer_stream_unittest.cc:194: EXPECT_EQ(ss.str(),
timestamps[i]);
nit:ditto

https://codereview.chromium.org/10952034/diff/2002/media/filters/source_buffe...
media/filters/source_buffer_stream_unittest.cc:285:
StreamParserBuffer::CopyFrom(NULL, 0, is_keyframe);
nit: use kDataA & sizeof(kDataA) instead of NULL,0 so these don't get mistaken
for EOS buffers.

[vrk (LEFT CHROMIUM), 2012-09-20 00:00:59]

Thanks for reviewing, Aaron!

https://codereview.chromium.org/10952034/diff/1001/media/filters/source_buffe...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/10952034/diff/1001/media/filters/source_buffe...
media/filters/source_buffer_stream.cc:335:
!(*range_for_next_append_)->AreAdjacentInSequence(
On 2012/09/19 23:15:51, acolwell wrote:
> This is a little awkward because it calls into the range just so the range can
> call back into this class for the approximate duration value. I think making
> this a helper method on SourceBufferStream would be better and just keep
> SourceBufferRange::IsNextInSequence() as is for now.
> 
> You could create a static TimeDelta GetFudge(TimeDelta approximate_duration)
> helper to avoid SourceBufferStream & SourceBufferRange getting out of sync.

Done.

https://codereview.chromium.org/10952034/diff/2002/media/filters/source_buffe...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/10952034/diff/2002/media/filters/source_buffe...
media/filters/source_buffer_stream.cc:602: *deleted_buffers = saved_buffers;
On 2012/09/19 23:15:51, acolwell wrote:
> DCHECK(deleted_buffers->empty()) right before this?

Good idea, done.

https://codereview.chromium.org/10952034/diff/2002/media/filters/source_buffe...
media/filters/source_buffer_stream.cc:1140: } else {
On 2012/09/19 23:15:51, acolwell wrote:
> nit: drop else and deindent

Done.

https://codereview.chromium.org/10952034/diff/2002/media/filters/source_buffe...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/10952034/diff/2002/media/filters/source_buffe...
media/filters/source_buffer_stream_unittest.cc:107: EXPECT_EQ(ss.str(),
expected);
On 2012/09/19 23:15:51, acolwell wrote:
> nit: flip

Done.

https://codereview.chromium.org/10952034/diff/2002/media/filters/source_buffe...
media/filters/source_buffer_stream_unittest.cc:121: EXPECT_EQ(ss.str(),
expected);
On 2012/09/19 23:15:51, acolwell wrote:
> nit: flip

Done.

https://codereview.chromium.org/10952034/diff/2002/media/filters/source_buffe...
media/filters/source_buffer_stream_unittest.cc:186: EXPECT_EQ(status,
SourceBufferStream::kSuccess);
On 2012/09/19 23:15:51, acolwell wrote:
> nit:flip order.

Done.

https://codereview.chromium.org/10952034/diff/2002/media/filters/source_buffe...
media/filters/source_buffer_stream_unittest.cc:194: EXPECT_EQ(ss.str(),
timestamps[i]);
On 2012/09/19 23:15:51, acolwell wrote:
> nit:ditto

Done.

https://codereview.chromium.org/10952034/diff/2002/media/filters/source_buffe...
media/filters/source_buffer_stream_unittest.cc:285:
StreamParserBuffer::CopyFrom(NULL, 0, is_keyframe);
On 2012/09/19 23:15:51, acolwell wrote:
> nit: use kDataA & sizeof(kDataA) instead of NULL,0 so these don't get mistaken
> for EOS buffers.

Done.

[commit-bot: I haz the power, 2012-09-20 00:05:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vrk@chromium.org/10952034/15001

[commit-bot: I haz the power, 2012-09-20 02:27:36]

Change committed as 157688