Prevent incorrect start time from being used during new range creation.

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 485 matching lines @@
   } else {
     base::TimeDelta new_range_start_time = std::min(
         media_segment_start_time_, buffers.front()->GetDecodeTimestamp());
     const BufferQueue* buffers_for_new_range = &buffers;
     BufferQueue trimmed_buffers;
 
     // If the new range is not being created because of a new media
     // segment, then we must make sure that we start with a keyframe.
     // This can happen if the GOP in the previous append gets destroyed
     // by a Remove() call.
-    if (!new_media_segment_ && !buffers.front()->IsKeyframe()) {
+    if (!new_media_segment_) {
       BufferQueue::const_iterator itr = buffers.begin();
 
       // Scan past all the non-keyframes.
       while (itr != buffers.end() && !(*itr)->IsKeyframe()) {
         ++itr;
       }
 
       // If we didn't find a keyframe, then update the last appended
       // buffer state and return.
       if (itr == buffers.end()) {
         last_appended_buffer_timestamp_ = buffers.back()->GetDecodeTimestamp();
         last_appended_buffer_is_keyframe_ = buffers.back()->IsKeyframe();
         return true;
+      } else if (itr != buffers.begin()) {
+        // Copy the first keyframe and everything after it into
+        // |trimmed_buffers|.
+        trimmed_buffers.assign(itr, buffers.end());
+        buffers_for_new_range = &trimmed_buffers;
       }
 
-      // Copy the first keyframe and everything after it into |trimmed_buffers|.
-      trimmed_buffers.assign(itr, buffers.end());
-
-      new_range_start_time = trimmed_buffers.front()->GetDecodeTimestamp();
-      buffers_for_new_range = &trimmed_buffers;
+      new_range_start_time =
+          buffers_for_new_range->front()->GetDecodeTimestamp();
     }
 
     range_for_next_append_ =
         AddToRanges(new SourceBufferRange(
             GetType(), *buffers_for_new_range, new_range_start_time,
             base::Bind(&SourceBufferStream::GetMaxInterbufferDistance,
                        base::Unretained(this))));
     last_appended_buffer_timestamp_ =
         buffers_for_new_range->back()->GetDecodeTimestamp();
     last_appended_buffer_is_keyframe_ =
@@ skipping 126 matching lines @@
     // Clear |range_for_next_append_| if we determine that the removal
     // operation makes it impossible for the next append to be added
     // to the current range.
     if (range_for_next_append_ != ranges_.end() &&
         *range_for_next_append_ == range &&
         last_appended_buffer_timestamp_ != kNoTimestamp()) {
       base::TimeDelta potential_next_append_timestamp =
           last_appended_buffer_timestamp_ +
           base::TimeDelta::FromInternalValue(1);
 
       if (!range->BelongsToRange(potential_next_append_timestamp)) {

[wolenetz, 2014/06/11 01:53:54]

I'm unclear how this path could be reached if range's buffers_ is empty().
Shouldn't the earlier range->TruncateAt have returned true in this case?

[acolwell GONE FROM CHROMIUM, 2014/06/11 16:47:51]

Yes. TruncateAt() should return true. I'm still investigating that for a
followup patch. I'll change the description to make it clear that this simply
prevents the wrong start time from being used when creating the new range.

         DVLOG(1) << "Resetting range_for_next_append_ since the next append"
                  <<  " can't add to the current range.";
         range_for_next_append_ =
             FindExistingRangeFor(potential_next_append_timestamp);
       }
     }
 
     // Move on to the next range.
     ++itr;
   }
@@ skipping 928 matching lines @@
 
   DCHECK(*itr != ranges_.end());
   if (**itr == selected_range_) {
     DVLOG(1) << __FUNCTION__ << " deleting selected range.";
     SetSelectedRange(NULL);
   }
 
   if (*itr == range_for_next_append_) {
     DVLOG(1) << __FUNCTION__ << " deleting range_for_next_append_.";
     range_for_next_append_ = ranges_.end();
+    last_appended_buffer_timestamp_ = kNoTimestamp();

[acolwell GONE FROM CHROMIUM, 2014/06/10 23:11:13]

This prevents the interbuffer distance calculation from going crazy when the
range we are appending to get destroyed.

+    last_appended_buffer_is_keyframe_ = false;
   }
 
   delete **itr;
   *itr = ranges_.erase(*itr);
 }
 
 void SourceBufferStream::GenerateSpliceFrame(const BufferQueue& new_buffers) {
   DCHECK(!new_buffers.empty());
 
   // Splice frames are only supported for audio.
@@ skipping 372 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;

[wolenetz, 2014/06/11 01:53:54]

Should this return true? (see my other comment)

[acolwell GONE FROM CHROMIUM, 2014/06/11 16:47:51]

I believe this should return buffers_.empty(). I was investigating this and was
planning on putting this in a separate CL. This is the true reason for the
crash, but I'm still investigating whether it is still possible to get into this
situation with the fix in this patch in place. I believe the answer is "yes",
but I haven't created a test to prove it yet.

 
   // 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 211 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