Fix SourceBufferStream alt-ref frame handling.

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 fd20802cfe8aeff4ebc530bd2ed581743a1f0309..79cf4397390fd91b401ceef5e62e6ced7b7d2bc2 100644
--- a/media/filters/source_buffer_stream.cc
+++ b/media/filters/source_buffer_stream.cc
@@ -300,6 +300,7 @@ SourceBufferStream::SourceBufferStream(const AudioDecoderConfig& audio_config,
       range_for_next_append_(ranges_.end()),
       new_media_segment_(false),
       last_appended_buffer_timestamp_(kNoTimestamp()),
+      last_appended_buffer_is_keyframe_(false),
       last_output_buffer_timestamp_(kNoTimestamp()),
       max_interbuffer_distance_(kNoTimestamp()),
       memory_limit_(kDefaultAudioMemoryLimit),
@@ -321,6 +322,7 @@ SourceBufferStream::SourceBufferStream(const VideoDecoderConfig& video_config,
       range_for_next_append_(ranges_.end()),
       new_media_segment_(false),
       last_appended_buffer_timestamp_(kNoTimestamp()),
+      last_appended_buffer_is_keyframe_(false),
       last_output_buffer_timestamp_(kNoTimestamp()),
       max_interbuffer_distance_(kNoTimestamp()),
       memory_limit_(kDefaultVideoMemoryLimit),
@@ -354,6 +356,7 @@ void SourceBufferStream::OnNewMediaSegment(
       !AreAdjacentInSequence(last_appended_buffer_timestamp_,
                              media_segment_start_time)) {
     last_appended_buffer_timestamp_ = kNoTimestamp();
+    last_appended_buffer_is_keyframe_ = false;
   } else {
     DCHECK(last_range == range_for_next_append_);
   }
@@ -375,10 +378,8 @@ bool SourceBufferStream::Append(
   }
 
   // Buffers within a media segment should be monotonically increasing.
-  if (!IsMonotonicallyIncreasing(buffers)) {
-    MEDIA_LOG(log_cb_) << "Buffers were not monotonically increasing.";
+  if (!IsMonotonicallyIncreasing(buffers))
     return false;
-  }
 
   if (media_segment_start_time_ < base::TimeDelta() ||
       buffers.front()->GetDecodeTimestamp() < base::TimeDelta()) {
@@ -398,7 +399,10 @@ bool SourceBufferStream::Append(
   // If there's a range for |buffers|, insert |buffers| accordingly. Otherwise,
   // create a new range with |buffers|.
   if (range_for_new_buffers != ranges_.end()) {
-    InsertIntoExistingRange(range_for_new_buffers, buffers, &deleted_buffers);
+    if (!InsertIntoExistingRange(range_for_new_buffers, buffers,
+                                 &deleted_buffers)) {
+      return false;
+    }
   } else {
     DCHECK(new_media_segment_);
     range_for_new_buffers =
@@ -411,6 +415,7 @@ bool SourceBufferStream::Append(
   range_for_next_append_ = range_for_new_buffers;
   new_media_segment_ = false;
   last_appended_buffer_timestamp_ = buffers.back()->GetDecodeTimestamp();
+  last_appended_buffer_is_keyframe_ = buffers.back()->IsKeyframe();
 
   // Resolve overlaps.
   ResolveCompleteOverlaps(range_for_new_buffers, &deleted_buffers);
@@ -477,15 +482,29 @@ bool SourceBufferStream::IsMonotonicallyIncreasing(
     const BufferQueue& buffers) const {
   DCHECK(!buffers.empty());
   base::TimeDelta prev_timestamp = last_appended_buffer_timestamp_;
+  bool prev_is_keyframe = last_appended_buffer_is_keyframe_;
   for (BufferQueue::const_iterator itr = buffers.begin();
        itr != buffers.end(); ++itr) {
     base::TimeDelta current_timestamp = (*itr)->GetDecodeTimestamp();
+    bool current_is_keyframe = (*itr)->IsKeyframe();
     DCHECK(current_timestamp != kNoTimestamp());
 
-    if (prev_timestamp != kNoTimestamp() && current_timestamp < prev_timestamp)
-      return false;
+    if (prev_timestamp != kNoTimestamp()) {
+      if (current_timestamp < prev_timestamp) {
+        MEDIA_LOG(log_cb_) << "Buffers were not monotonically increasing.";
+        return false;
+      }
+
+      if (current_timestamp == prev_timestamp &&
+          (current_is_keyframe || prev_is_keyframe)) {
+        MEDIA_LOG(log_cb_) << "Invalid alt-ref frame construct detected at "
+                           << current_timestamp.InSecondsF();
+        return false;
+      }
+    }
 
     prev_timestamp = current_timestamp;
+    prev_is_keyframe = current_is_keyframe;
   }
   return true;
 }
@@ -628,7 +647,7 @@ int SourceBufferStream::FreeBuffers(int total_bytes_to_free,
   return bytes_freed;
 }
 
-void SourceBufferStream::InsertIntoExistingRange(
+bool SourceBufferStream::InsertIntoExistingRange(
     const RangeList::iterator& range_for_new_buffers_itr,
     const BufferQueue& new_buffers, BufferQueue* deleted_buffers) {
   DCHECK(deleted_buffers);
@@ -657,22 +676,44 @@ void SourceBufferStream::InsertIntoExistingRange(
     }
   }
 
-  if (last_appended_buffer_timestamp_ != kNoTimestamp()) {
+  base::TimeDelta prev_timestamp = last_appended_buffer_timestamp_;
+  bool prev_is_keyframe = last_appended_buffer_is_keyframe_;
+  base::TimeDelta next_timestamp = new_buffers.front()->GetDecodeTimestamp();
+  bool next_is_keyframe = new_buffers.front()->IsKeyframe();
+
+  if (prev_timestamp != kNoTimestamp() && prev_timestamp != next_timestamp) {
     // Clean up the old buffers between the last appended buffer and the
     // beginning of |new_buffers|.
     DeleteBetween(
-        range_for_new_buffers_itr, last_appended_buffer_timestamp_,
-        new_buffers.front()->GetDecodeTimestamp(), true,
+        range_for_new_buffers_itr, prev_timestamp, next_timestamp, true,
         deleted_buffers);
   }
 
+  // Check for invalid alt-ref frame constructs.

[scherkus (not reviewing), 2013/03/11 23:21:50]

want to format this to use a bullet list / indent for the two cases we're
checking for?

Check for invalid alt-ref frame constructs:
  * a keyframe followed by a non-keyframe or
  * a non-keyframe blah blah ...

also might be nice to link / describe what an alt-ref frame is -- the expected
results for AltRefFrame_Overlap_3 looks a bit funny at first glance due to the
duplicated "90" timestamps (I'm guessing one of those is considered the alt-ref
frame?)

[acolwell GONE FROM CHROMIUM, 2013/03/12 03:22:50]

Done.

+  // A keyframe followed by a non-keyframe or
+  // a non-keyframe followed by a keyframe that is not
+  // the first frame of a media segment are errors.
+  if (prev_timestamp == next_timestamp &&
+      ((prev_is_keyframe && !next_is_keyframe) ||
+       (!new_media_segment_ && next_is_keyframe))) {
+    MEDIA_LOG(log_cb_) << "Invalid alt-ref frame construct detected at time "
+                       << prev_timestamp.InSecondsF();
+    return false;
+  }
+
   // If we cannot append the |new_buffers| to the end of the existing range,
   // this is either a start overlap or an middle overlap. Delete the buffers

[scherkus (not reviewing), 2013/03/11 23:21:50]

s/or an middle/or a middle/

[acolwell GONE FROM CHROMIUM, 2013/03/12 03:22:50]

Done.

   // that |new_buffers| overlaps.
   if (!range_for_new_buffers->CanAppendBuffersToEnd(new_buffers)) {
+    // Make the delete range exclusive if we are dealing with an alt-ref
+    // situation so that the buffer with the same timestamp that is already
+    // stored in |*range_for_new_buffers_itr| doesn't get deleted.
+    bool is_exclusive = prev_timestamp == next_timestamp &&
+        !prev_is_keyframe && !next_is_keyframe;
+
     DeleteBetween(
         range_for_new_buffers_itr, new_buffers.front()->GetDecodeTimestamp(),
-        new_buffers.back()->GetDecodeTimestamp(), false,
+        new_buffers.back()->GetDecodeTimestamp(), is_exclusive,
         deleted_buffers);
   }
 
@@ -681,6 +722,7 @@ void SourceBufferStream::InsertIntoExistingRange(
     SetSelectedRange(NULL);
 
   range_for_new_buffers->AppendBuffersToEnd(new_buffers);
+  return true;
 }
 
 void SourceBufferStream::DeleteBetween(
@@ -1257,6 +1299,10 @@ SourceBufferRange::SourceBufferRange(
 }
 
 void SourceBufferRange::AppendBuffersToEnd(const BufferQueue& new_buffers) {
+  DCHECK(buffers_.empty() ||
+         buffers_.back()->GetDecodeTimestamp() <=
+         new_buffers.front()->GetDecodeTimestamp());
+
   for (BufferQueue::const_iterator itr = new_buffers.begin();
        itr != new_buffers.end(); ++itr) {
     DCHECK((*itr)->GetDecodeTimestamp() != kNoTimestamp());