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

media/filters/source_buffer_stream.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/filters/source_buffer_stream.h"
 
 #include <algorithm>
 #include <map>
 
 #include "base/bind.h"
@@ skipping 1756 matching lines @@
   DCHECK_LT(next_buffer_index_, static_cast<int>(buffers_.size()));
 }
 
 void SourceBufferRange::SeekToStart() {
   DCHECK(!buffers_.empty());
   next_buffer_index_ = 0;
 }
 
 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 =
       GetFirstKeyframeAt(timestamp, is_exclusive);
 
   // If there is no keyframe after |timestamp|, we can't split the range.
   if (new_beginning_keyframe == keyframe_map_.end())
     return NULL;
 
   // Remove the data beginning at |keyframe_index| from |buffers_| and save it
   // into |removed_buffers|.
   int keyframe_index =
       new_beginning_keyframe->second - keyframe_map_index_base_;
   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.
   if (next_buffer_index_ >= static_cast<int>(buffers_.size())) {
     split_range->next_buffer_index_ = next_buffer_index_ - keyframe_index;
     ResetNextBufferPosition();
   }
 
   return split_range;
 }
@@ skipping 198 matching lines @@
   }
   buffers_.erase(starting_point, ending_point);
 }
 
 bool SourceBufferRange::TruncateAt(
     const BufferQueue::iterator& starting_point, BufferQueue* removed_buffers) {
   DCHECK(!removed_buffers || removed_buffers->empty());
 
   // 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.
   if (HasNextBufferPosition()) {
     base::TimeDelta next_buffer_timestamp = GetNextTimestamp();
     if (next_buffer_timestamp == kNoTimestamp() ||
         next_buffer_timestamp >= (*starting_point)->GetDecodeTimestamp()) {
       if (HasNextBuffer() && removed_buffers) {
         int starting_offset = starting_point - buffers_.begin();
         int next_buffer_offset = next_buffer_index_ - starting_offset;
@@ skipping 128 matching lines @@
 base::TimeDelta SourceBufferRange::NextKeyframeTimestamp(
     base::TimeDelta timestamp) {
   DCHECK(!keyframe_map_.empty());
 
   if (timestamp < GetStartTimestamp() || timestamp >= GetBufferedEndTimestamp())
     return kNoTimestamp();
 
   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;
 }
 
 base::TimeDelta SourceBufferRange::KeyframeBeforeTimestamp(
     base::TimeDelta timestamp) {
   DCHECK(!keyframe_map_.empty());
 
   if (timestamp < GetStartTimestamp() || timestamp >= GetBufferedEndTimestamp())
     return kNoTimestamp();
 
@@ skipping 63 matching lines @@
     return false;
 
   DCHECK_NE(have_splice_buffers, have_preroll_buffer);
   splice_buffers_index_ = 0;
   pending_buffer_.swap(*out_buffer);
   pending_buffers_complete_ = false;
   return true;
 }
 
 }  // namespace media