Fix crash caused by removing the gap at the beginning of a media segment.

Fix crash caused by removing the gap at the beginning of a media segment.

Some media segments have a start time that is slightly 
earlier than the timestamp of the first buffer in that
segment. If SourceBufferStream::Remove() is called with a
range that only covered part of this gap, then the
SourceBufferStream code could end up crashing when it was
trying to handle the remove. This change fixes the crash by
properly removing the empty range that gets created when
such a remove occurs.

BUG=382815
TEST=SourceBufferStreamTest.Remove_GapAtBeginningOfMediaSegment

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

[wolenetz, 2014-06-12 00:00:16]

https://codereview.chromium.org/328223002/diff/20001/media/filters/source_buf...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/328223002/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream.cc:1798: media_segment_start_time_ <
buffers_.front()->GetDecodeTimestamp() &&
Can |media_segment_start_time_| be kNoTimestamp() here (such as from GC or end
overlap causing split range as described around line 271)?

https://codereview.chromium.org/328223002/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream.cc:1810: // Create a new range with
|removed_buffers|.
If |removed_buffers| is empty(), why create an empty split_range with
kNoTimestamp() |media_segment_start_time_|? Actually, since
|new_beginning_keyframe| was not keyframe_map_.end() earlier in this method,
doesn't that mean that CHECK(!removed_buffers.empty()) should never fail here
and split range should never produce an empty() new range?

https://codereview.chromium.org/328223002/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream.cc:2183: // If the timestamp is inside the
gap between the start of the media
nit: Update the method declaration's comment, too?

https://codereview.chromium.org/328223002/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream.cc:2185: // keyframe at the specified
timestamp.
I'm not totally clear why this pretending is necessary.

https://codereview.chromium.org/328223002/diff/20001/media/filters/source_buf...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/328223002/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream_unittest.cc:3581: // removed but didn't
happen to remove any buffers.
Is the only source of gaps at beginning of a range due to demuxed multitrack
appends where some other track actually has a keyframe buffer at the (same)
beginning of the range. (If so, this test only simulates this for one track, but
that's fine at this scope.) If not, I'm confused.

IIUC, I agree: it seems that an explicit seek to a still-buffered-after-remove
time should be necessary to not stall on reading from the track, which differs
from the behavior expected in this change.

[acolwell GONE FROM CHROMIUM, 2014-06-12 01:19:28]

https://codereview.chromium.org/328223002/diff/20001/media/filters/source_buf...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/328223002/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream.cc:1798: media_segment_start_time_ <
buffers_.front()->GetDecodeTimestamp() &&
On 2014/06/12 00:00:16, wolenetz wrote:
> Can |media_segment_start_time_| be kNoTimestamp() here (such as from GC or end
> overlap causing split range as described around line 271)?
Oops. Changed code to use GetStartTimestamp() so the code is clearer and this
issue is taken care for me. :)

https://codereview.chromium.org/328223002/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream.cc:1810: // Create a new range with
|removed_buffers|.
On 2014/06/12 00:00:16, wolenetz wrote:
> If |removed_buffers| is empty(), why create an empty split_range with
> kNoTimestamp() |media_segment_start_time_|? Actually, since
> |new_beginning_keyframe| was not keyframe_map_.end() earlier in this method,
> doesn't that mean that CHECK(!removed_buffers.empty()) should never fail here
> and split range should never produce an empty() new range?

Removed the check above. There is a DCHECK() in the SourceRange() constructor
that makes sure a range isn't created w/ an empty BufferQueue.

https://codereview.chromium.org/328223002/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream.cc:2183: // If the timestamp is inside the
gap between the start of the media
On 2014/06/12 00:00:16, wolenetz wrote:
> nit: Update the method declaration's comment, too?

Done.

https://codereview.chromium.org/328223002/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream.cc:2185: // keyframe at the specified
timestamp.
On 2014/06/12 00:00:16, wolenetz wrote:
> I'm not totally clear why this pretending is necessary.
It makes the buffered ranges behave in a way that people expect.
If I have [0,180) and remove [0,10), I think people would expect to see [10,180)
and not [30,180). I think the remaining part of the "gap" should still be
considered part of the buffered range. That is basically what this change is
enabling.

https://codereview.chromium.org/328223002/diff/20001/media/filters/source_buf...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/328223002/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream_unittest.cc:3581: // removed but didn't
happen to remove any buffers.
On 2014/06/12 00:00:16, wolenetz wrote:
> Is the only source of gaps at beginning of a range due to demuxed multitrack
> appends where some other track actually has a keyframe buffer at the (same)
> beginning of the range. (If so, this test only simulates this for one track,
but
> that's fine at this scope.) If not, I'm confused.

Yes. This simulates the sitation where the first audio frame is at 0 and the
first video frame is at 30 in a muxed stream.

> 
> IIUC, I agree: it seems that an explicit seek to a still-buffered-after-remove
> time should be necessary to not stall on reading from the track, which differs
> from the behavior expected in this change.

Yes. It seems to me if you are "seeked to time 0", but haven't handed out any
buffers yet and then time 0 gets removed, the seek should stall and no buffers
should be handed out after the remove. I'm defering this change in behavior to a
different CL because I'm pretty sure it will have effects elsewhere and I didn't
want to muddy the purpose of this CL.

[wolenetz, 2014-06-12 19:20:50]

lgtm % nits

https://codereview.chromium.org/328223002/diff/20001/media/filters/source_buf...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/328223002/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream.cc:2187: timestamp >=
media_segment_start_time_ &&
nit: If timestamp == media_segment_start_time_, do we still want to pretend
there's a keyframe at the media segment start time?

https://codereview.chromium.org/328223002/diff/20001/media/filters/source_buf...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/328223002/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream_unittest.cc:3581: // removed but didn't
happen to remove any buffers.
On 2014/06/12 01:19:27, acolwell wrote:
> On 2014/06/12 00:00:16, wolenetz wrote:
> > Is the only source of gaps at beginning of a range due to demuxed multitrack
> > appends where some other track actually has a keyframe buffer at the (same)
> > beginning of the range. (If so, this test only simulates this for one track,
> but
> > that's fine at this scope.) If not, I'm confused.
> 
> Yes. This simulates the sitation where the first audio frame is at 0 and the
> first video frame is at 30 in a muxed stream.
> 
> > 
> > IIUC, I agree: it seems that an explicit seek to a
still-buffered-after-remove
> > time should be necessary to not stall on reading from the track, which
differs
> > from the behavior expected in this change.
> 
> Yes. It seems to me if you are "seeked to time 0", but haven't handed out any
> buffers yet and then time 0 gets removed, the seek should stall and no buffers
> should be handed out after the remove. I'm defering this change in behavior to
a
> different CL because I'm pretty sure it will have effects elsewhere and I
didn't
> want to muddy the purpose of this CL.

nit: please file and reference crbug

https://codereview.chromium.org/328223002/diff/40001/media/filters/source_buf...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/328223002/diff/40001/media/filters/source_buf...
media/filters/source_buffer_stream.cc:171: // is returned. If |timestamp| is in
the "gap" between the value
nit: these new pieces seem wrapped at less than 80 chars max. If vim'ing, select
comment lines and use 'gq' command sequence to re-wrap.

https://codereview.chromium.org/328223002/diff/40001/media/filters/source_buf...
media/filters/source_buffer_stream.cc:1712: DCHECK(!new_buffers.empty());
nit: to better protect against undefined behavior on possible re-introduction
later of a path creating empty ranges that are subsequently operated on blindly
assuming non-emptiness, change this to a CHECK?

[acolwell GONE FROM CHROMIUM, 2014-06-12 20:17:59]

Thanks for the review.

https://codereview.chromium.org/328223002/diff/20001/media/filters/source_buf...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/328223002/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream.cc:2187: timestamp >=
media_segment_start_time_ &&
On 2014/06/12 19:20:49, wolenetz wrote:
> nit: If timestamp == media_segment_start_time_, do we still want to pretend
> there's a keyframe at the media segment start time?
Probably not. Made this >.

https://codereview.chromium.org/328223002/diff/20001/media/filters/source_buf...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/328223002/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream_unittest.cc:3581: // removed but didn't
happen to remove any buffers.
On 2014/06/12 19:20:49, wolenetz wrote:
> On 2014/06/12 01:19:27, acolwell wrote:
> > On 2014/06/12 00:00:16, wolenetz wrote:
> > > Is the only source of gaps at beginning of a range due to demuxed
multitrack
> > > appends where some other track actually has a keyframe buffer at the
(same)
> > > beginning of the range. (If so, this test only simulates this for one
track,
> > but
> > > that's fine at this scope.) If not, I'm confused.
> > 
> > Yes. This simulates the sitation where the first audio frame is at 0 and the
> > first video frame is at 30 in a muxed stream.
> > 
> > > 
> > > IIUC, I agree: it seems that an explicit seek to a
> still-buffered-after-remove
> > > time should be necessary to not stall on reading from the track, which
> differs
> > > from the behavior expected in this change.
> > 
> > Yes. It seems to me if you are "seeked to time 0", but haven't handed out
any
> > buffers yet and then time 0 gets removed, the seek should stall and no
buffers
> > should be handed out after the remove. I'm defering this change in behavior
to
> a
> > different CL because I'm pretty sure it will have effects elsewhere and I
> didn't
> > want to muddy the purpose of this CL.
> 
> nit: please file and reference crbug

Done.

https://codereview.chromium.org/328223002/diff/40001/media/filters/source_buf...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/328223002/diff/40001/media/filters/source_buf...
media/filters/source_buffer_stream.cc:171: // is returned. If |timestamp| is in
the "gap" between the value
On 2014/06/12 19:20:49, wolenetz wrote:
> nit: these new pieces seem wrapped at less than 80 chars max. If vim'ing,
select
> comment lines and use 'gq' command sequence to re-wrap.

Done.

https://codereview.chromium.org/328223002/diff/40001/media/filters/source_buf...
media/filters/source_buffer_stream.cc:1712: DCHECK(!new_buffers.empty());
On 2014/06/12 19:20:49, wolenetz wrote:
> nit: to better protect against undefined behavior on possible re-introduction
> later of a path creating empty ranges that are subsequently operated on
blindly
> assuming non-emptiness, change this to a CHECK?

Done.

[acolwell GONE FROM CHROMIUM, 2014-06-12 20:18:02]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-06-12 20:19:21]

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

[commit-bot: I haz the power, 2014-06-13 02:40:37]

Change committed as 276897