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

media/filters/source_buffer_stream.cc

Index: media/filters/source_buffer_stream.cc
diff --git a/media/filters/source_buffer_stream.cc b/media/filters/source_buffer_stream.cc
index e7258867308ec74756befd089917b83988984780..0a71014def225ad19852989dc112a42be966f058 100644
--- a/media/filters/source_buffer_stream.cc
+++ b/media/filters/source_buffer_stream.cc
@@ -1774,6 +1774,8 @@ void SourceBufferRange::SeekToStart() {
 
 SourceBufferRange* SourceBufferRange::SplitRange(
     base::TimeDelta timestamp, bool is_exclusive) {
+  CHECK(!buffers_.empty());
+
   // Find the first keyframe after |timestamp|. If |is_exclusive|, do not
   // include keyframes at |timestamp|.
   KeyframeMap::iterator new_beginning_keyframe =
@@ -1790,13 +1792,26 @@ SourceBufferRange* SourceBufferRange::SplitRange(
   DCHECK_LT(keyframe_index, static_cast<int>(buffers_.size()));
   BufferQueue::iterator starting_point = buffers_.begin() + keyframe_index;
   BufferQueue removed_buffers(starting_point, buffers_.end());
+
+  base::TimeDelta new_range_start_timestamp = kNoTimestamp();
+  if (!removed_buffers.empty() &&
+      media_segment_start_time_ < buffers_.front()->GetDecodeTimestamp() &&

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

Can |media_segment_start_time_| be kNoTimestamp() here (such as from GC or end
overlap causing split range as described around line 271)?

[acolwell GONE FROM CHROMIUM, 2014/06/12 01:19:27]

Oops. Changed code to use GetStartTimestamp() so the code is clearer and this
issue is taken care for me. :)

+      timestamp < removed_buffers.front()->GetDecodeTimestamp()) {
+    // The split is in the gap between |media_segment_start_time_| and
+    // the first buffer of the new range so we should set the start
+    // time of the new range to |timestamp| so we preserve part of the
+    // gap in the new range.
+    new_range_start_timestamp = timestamp;
+  }
+
   keyframe_map_.erase(new_beginning_keyframe, keyframe_map_.end());
   FreeBufferRange(starting_point, buffers_.end());
 
   // Create a new range with |removed_buffers|.

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

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?

[acolwell GONE FROM CHROMIUM, 2014/06/12 01:19:27]

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

   SourceBufferRange* split_range =
       new SourceBufferRange(
-          type_, removed_buffers, kNoTimestamp(), interbuffer_distance_cb_);
+          type_, removed_buffers, new_range_start_timestamp,
+          interbuffer_distance_cb_);
 
   // If the next buffer position is now in |split_range|, update the state of
   // this range and |split_range| accordingly.
@@ -2015,7 +2030,7 @@ bool SourceBufferRange::TruncateAt(
 
   // Return if we're not deleting anything.
   if (starting_point == buffers_.end())
-    return false;
+    return buffers_.empty();
 
   // Reset the next buffer index if we will be deleting the buffer that's next
   // in sequence.
@@ -2164,6 +2179,16 @@ base::TimeDelta SourceBufferRange::NextKeyframeTimestamp(
   KeyframeMap::iterator itr = GetFirstKeyframeAt(timestamp, false);
   if (itr == keyframe_map_.end())
     return kNoTimestamp();
+
+  // If the timestamp is inside the gap between the start of the media

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

nit: Update the method declaration's comment, too?

[acolwell GONE FROM CHROMIUM, 2014/06/12 01:19:27]

Done.

+  // segment and the first buffer, then just pretend there is a
+  // keyframe at the specified timestamp.

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

I'm not totally clear why this pretending is necessary.

[acolwell GONE FROM CHROMIUM, 2014/06/12 01:19:27]

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.

+  if (itr == keyframe_map_.begin() &&
+      timestamp >= media_segment_start_time_ &&

[wolenetz, 2014/06/12 19:20:49]

nit: If timestamp == media_segment_start_time_, do we still want to pretend
there's a keyframe at the media segment start time?

[acolwell GONE FROM CHROMIUM, 2014/06/12 20:17:58]

Probably not. Made this >.

+      timestamp < itr->first) {
+    return timestamp;
+  }
+
   return itr->first;
 }