Fix SourceBufferStream alt-ref frame handling.

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 105 matching lines @@
   // Returns the timestamp of the next buffer that will be returned from
   // GetNextBuffer(), or kNoTimestamp() if the timestamp is unknown.
   base::TimeDelta GetNextTimestamp() const;
 
   // Returns the start timestamp of the range.
   base::TimeDelta GetStartTimestamp() const;
 
   // 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.
   base::TimeDelta GetBufferedEndTimestamp() const;
 
   // Gets the timestamp for the keyframe that is after |timestamp|. If
   // there isn't a keyframe in the range after |timestamp| then kNoTimestamp()
   // is returned.
   base::TimeDelta NextKeyframeTimestamp(base::TimeDelta timestamp);
 
@@ skipping 232 matching lines @@
   DCHECK(!buffers.empty());
   DCHECK(media_segment_start_time_ != kNoTimestamp());
 
   // New media segments must begin with a keyframe.
   if (new_media_segment_ && !buffers.front()->IsKeyframe()) {
     MEDIA_LOG(log_cb_) << "Media segment did not begin with keyframe.";
     return false;
   }
 
   // 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()) {
     MEDIA_LOG(log_cb_)
         << "Cannot append a media segment with negative timestamps.";
     return false;
   }
 
   UpdateMaxInterbufferDistance(buffers);
   SetConfigIds(buffers);
 
   // Save a snapshot of stream state before range modifications are made.
   base::TimeDelta next_buffer_timestamp = GetNextBufferTimestamp();
   BufferQueue deleted_buffers;
 
   RangeList::iterator range_for_new_buffers = range_for_next_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 =
         AddToRanges(new SourceBufferRange(
             buffers, media_segment_start_time_,
             base::Bind(&SourceBufferStream::GetMaxInterbufferDistance,
                        base::Unretained(this))));
   }
 
   range_for_next_append_ = range_for_new_buffers;
@@ skipping 58 matching lines @@
   base::TimeDelta beginning_of_buffered =
       ranges_.front()->GetStartTimestamp();
   return (seek_timestamp <= beginning_of_buffered &&
           beginning_of_buffered < kSeekToStartFudgeRoom());
 }
 
 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;
 }
 
 bool SourceBufferStream::OnlySelectedRangeIsSeeked() const {
   for (RangeList::const_iterator itr = ranges_.begin();
        itr != ranges_.end(); ++itr) {
     if ((*itr)->HasNextBufferPosition() && (*itr) != selected_range_)
       return false;
   }
@@ skipping 122 matching lines @@
     if (range_for_next_append_ != ranges_.begin()) {
       RangeList::iterator range_before_next = range_for_next_append_;
       --range_before_next;
       MergeWithAdjacentRangeIfNecessary(range_before_next);
     }
     MergeWithAdjacentRangeIfNecessary(range_for_next_append_);
   }
   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);
 
   SourceBufferRange* range_for_new_buffers = *range_for_new_buffers_itr;
 
   bool temporarily_select_range = false;
   if (!track_buffer_.empty()) {
     base::TimeDelta tb_timestamp = track_buffer_.back()->GetDecodeTimestamp();
     base::TimeDelta seek_timestamp = FindKeyframeAfterTimestamp(tb_timestamp);
@@ skipping 17 matching lines @@
 
   if (last_appended_buffer_timestamp_ != kNoTimestamp()) {
     // 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,
         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,
         deleted_buffers);
   }
 
   // Restore the range seek state if necessary.
   if (temporarily_select_range)
     SetSelectedRange(NULL);
 
   range_for_new_buffers->AppendBuffersToEnd(new_buffers);
+  return true;
 }
 
 void SourceBufferStream::DeleteBetween(
     const RangeList::iterator& range_itr, base::TimeDelta start_timestamp,
     base::TimeDelta end_timestamp, bool is_range_exclusive,
     BufferQueue* deleted_buffers) {
   SourceBufferRange* new_next_range =
       (*range_itr)->SplitRange(end_timestamp, is_range_exclusive);
 
   // Insert the |new_next_range| into |ranges_| after |range|.
@@ skipping 942 matching lines @@
   if (start_timestamp == kNoTimestamp())
     start_timestamp = buffers_.front()->GetDecodeTimestamp();
   return start_timestamp;
 }
 
 base::TimeDelta SourceBufferRange::GetEndTimestamp() const {
   DCHECK(!buffers_.empty());
   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();
   if (duration == kNoTimestamp() || duration == base::TimeDelta())
     duration = GetApproximateDuration();
   return GetEndTimestamp() + duration;
 }
 
 base::TimeDelta SourceBufferRange::NextKeyframeTimestamp(
     base::TimeDelta timestamp) {
@@ skipping 29 matching lines @@
   return ComputeFudgeRoom(GetApproximateDuration());
 }
 
 base::TimeDelta SourceBufferRange::GetApproximateDuration() const {
   base::TimeDelta max_interbuffer_distance = interbuffer_distance_cb_.Run();
   DCHECK(max_interbuffer_distance != kNoTimestamp());
   return max_interbuffer_distance;
 }
 
 }  // namespace media