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..8b63f8997b83c7f479fcb37a5434c71306965108 100644
--- a/media/filters/source_buffer_stream.cc
+++ b/media/filters/source_buffer_stream.cc
@@ -123,6 +123,9 @@ class SourceBufferRange {
   // Returns the timestamp of the last buffer in the range.
   base::TimeDelta GetEndTimestamp() const;
 
+  // Returns true if the last buffer in the range is a keyframe.
+  bool IsEndKeyframe() const;

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

EndsWithKeyframe?

[acolwell GONE FROM CHROMIUM, 2013/03/11 22:31:38]

Removed. Method no longer needed.

+
   // Returns the timestamp for the end of the buffered region in this range.
   // This is an approximation if the duration for the last buffer in the range
   // is unset.
@@ -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 =
@@ -477,15 +481,29 @@ bool SourceBufferStream::IsMonotonicallyIncreasing(
     const BufferQueue& buffers) const {
   DCHECK(!buffers.empty());
   base::TimeDelta prev_timestamp = last_appended_buffer_timestamp_;
+  bool prev_is_keyframe = false;
   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 +646,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);
@@ -666,10 +684,31 @@ void SourceBufferStream::InsertIntoExistingRange(
         deleted_buffers);
   }
 
-  // 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
-  // that |new_buffers| overlaps.
-  if (!range_for_new_buffers->CanAppendBuffersToEnd(new_buffers)) {
+  base::TimeDelta prev_timestamp = range_for_new_buffers->GetEndTimestamp();
+  bool prev_is_keyframe = range_for_new_buffers->IsEndKeyframe();
+  base::TimeDelta next_timestamp = new_buffers.front()->GetDecodeTimestamp();
+  bool next_is_keyframe = new_buffers.front()->IsKeyframe();
+  bool is_valid_alt_ref = prev_timestamp == next_timestamp &&

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

how does this compare with the if block below that checks for invalid alt-ref
frame constructs?

[acolwell GONE FROM CHROMIUM, 2013/03/11 22:31:38]

This identifies a valid alt-ref construct that we want to allow & handle in a
special way. The checks below exclude duplicate timestamp situations that we
should never allow. There is a case though where is_valid_alt_ref is false, but
we don't want to return an error. This is when we have an overlap situation
where the last buffer appended gets overlapped by the next append. vrk found a
bug in this code so the new patch, should make this a little clearer.

+      !prev_is_keyframe && !next_is_keyframe;
+
+  // Check for invalid alt-ref frame constructs.
+  // 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 a valid alt-ref frame sequence was not detected and 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 that |new_buffers|
+  // overlaps.
+  if (!is_valid_alt_ref &&
+      !range_for_new_buffers->CanAppendBuffersToEnd(new_buffers)) {
     DeleteBetween(
         range_for_new_buffers_itr, new_buffers.front()->GetDecodeTimestamp(),
         new_buffers.back()->GetDecodeTimestamp(), false,
@@ -681,6 +720,7 @@ void SourceBufferStream::InsertIntoExistingRange(
     SetSelectedRange(NULL);
 
   range_for_new_buffers->AppendBuffersToEnd(new_buffers);
+  return true;
 }
 
 void SourceBufferStream::DeleteBetween(
@@ -1643,6 +1683,11 @@ base::TimeDelta SourceBufferRange::GetEndTimestamp() const {
   return buffers_.back()->GetDecodeTimestamp();
 }
 
+bool SourceBufferRange::IsEndKeyframe() const {
+  DCHECK(!buffers_.empty());
+  return buffers_.back()->IsKeyframe();
+}
+
 base::TimeDelta SourceBufferRange::GetBufferedEndTimestamp() const {
   DCHECK(!buffers_.empty());
   base::TimeDelta duration = buffers_.back()->GetDuration();