Reland: MSE: Relax the 'media segment must begin with keyframe' requirement

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 619ff47ab7d4c54a1dcc070384c3ab0bfcfe5c70..2a6f39d338f2de2e7182456939ba2d11649e0f28 100644
--- a/media/filters/source_buffer_stream.cc
+++ b/media/filters/source_buffer_stream.cc
@@ -37,6 +37,15 @@ const int kMaxTrackBufferGapWarningLogs = 20;
 // Limit the number of MEDIA_LOG() logs for MSE GC algorithm warnings.
 const int kMaxGarbageCollectAlgorithmWarningLogs = 20;
 
+// Limit the number of MEDIA_LOG() logs for same DTS for non-keyframe followed
+// by keyframe. Prior to relaxing the "media segments must begin with a
+// keyframe" requirement, we issued decode error for this situation. That was
+// likely too strict, and now that the keyframe requirement is relaxed, we have
+// no knowledge of media segment boundaries here. Now, we log but don't trigger
+// decode error, since we allow these sequences which may cause extra decoder
+// work or other side-effects.
+const int kMaxStrangeSameTimestampsLogs = 20;
+
 // Helper method that returns true if |ranges| is sorted in increasing order,
 // false otherwise.
 bool IsRangeListSorted(const std::list<media::SourceBufferRange*>& ranges) {
@@ -137,7 +146,7 @@ SourceBufferStream::SourceBufferStream(const AudioDecoderConfig& audio_config,
                                        bool splice_frames_enabled)
     : media_log_(media_log),
       seek_buffer_timestamp_(kNoTimestamp()),
-      media_segment_start_time_(kNoDecodeTimestamp()),
+      coded_frame_group_start_time_(kNoDecodeTimestamp()),
       range_for_next_append_(ranges_.end()),
       last_output_buffer_timestamp_(kNoDecodeTimestamp()),
       max_interbuffer_distance_(kNoTimestamp()),
@@ -152,7 +161,7 @@ SourceBufferStream::SourceBufferStream(const VideoDecoderConfig& video_config,
                                        bool splice_frames_enabled)
     : media_log_(media_log),
       seek_buffer_timestamp_(kNoTimestamp()),
-      media_segment_start_time_(kNoDecodeTimestamp()),
+      coded_frame_group_start_time_(kNoDecodeTimestamp()),
       range_for_next_append_(ranges_.end()),
       last_output_buffer_timestamp_(kNoDecodeTimestamp()),
       max_interbuffer_distance_(kNoTimestamp()),
@@ -168,7 +177,7 @@ SourceBufferStream::SourceBufferStream(const TextTrackConfig& text_config,
     : media_log_(media_log),
       text_track_config_(text_config),
       seek_buffer_timestamp_(kNoTimestamp()),
-      media_segment_start_time_(kNoDecodeTimestamp()),
+      coded_frame_group_start_time_(kNoDecodeTimestamp()),
       range_for_next_append_(ranges_.end()),
       last_output_buffer_timestamp_(kNoDecodeTimestamp()),
       max_interbuffer_distance_(kNoTimestamp()),
@@ -182,26 +191,29 @@ SourceBufferStream::~SourceBufferStream() {
   }
 }
 
-void SourceBufferStream::OnNewMediaSegment(
-    DecodeTimestamp media_segment_start_time) {
-  DVLOG(1) << __FUNCTION__ << " " << GetStreamTypeName()
-           << " (" << media_segment_start_time.InSecondsF() << ")";
+void SourceBufferStream::OnStartOfCodedFrameGroup(
+    DecodeTimestamp coded_frame_group_start_time) {
+  DVLOG(1) << __FUNCTION__ << " " << GetStreamTypeName() << " ("
+           << coded_frame_group_start_time.InSecondsF() << ")";
   DCHECK(!end_of_stream_);
-  media_segment_start_time_ = media_segment_start_time;
-  new_media_segment_ = true;
+  coded_frame_group_start_time_ = coded_frame_group_start_time;
+  new_coded_frame_group_ = true;
 
   RangeList::iterator last_range = range_for_next_append_;
-  range_for_next_append_ = FindExistingRangeFor(media_segment_start_time);
+  range_for_next_append_ = FindExistingRangeFor(coded_frame_group_start_time);
 
-  // Only reset |last_appended_buffer_timestamp_| if this new media segment is
-  // not adjacent to the previous media segment appended to the stream.
+  // Only reset |last_appended_buffer_timestamp_| if this new coded frame group
+  // is not adjacent to the previous coded frame group appended to the stream.
   if (range_for_next_append_ == ranges_.end() ||
       !AreAdjacentInSequence(last_appended_buffer_timestamp_,
-                             media_segment_start_time)) {
+                             coded_frame_group_start_time)) {
     last_appended_buffer_timestamp_ = kNoDecodeTimestamp();
     last_appended_buffer_duration_ = kNoTimestamp();
     last_appended_buffer_is_keyframe_ = false;
-    DVLOG(3) << __FUNCTION__ << " next appended buffers will be in a new range";
+    DVLOG(3) << __FUNCTION__ << " next appended buffers will "
+             << (range_for_next_append_ == ranges_.end()
+                     ? "be in a new range"
+                     : "overlap an existing range");
   } else if (last_range != ranges_.end()) {
     DCHECK(last_range == range_for_next_append_);
     DVLOG(3) << __FUNCTION__ << " next appended buffers will continue range "
@@ -215,41 +227,29 @@ bool SourceBufferStream::Append(const BufferQueue& buffers) {
                "buffers to append", buffers.size());
 
   DCHECK(!buffers.empty());
-  DCHECK(media_segment_start_time_ != kNoDecodeTimestamp());
-  DCHECK(media_segment_start_time_ <= buffers.front()->GetDecodeTimestamp());
+  DCHECK(coded_frame_group_start_time_ != kNoDecodeTimestamp());
+  DCHECK(coded_frame_group_start_time_ <=
+         buffers.front()->GetDecodeTimestamp());
   DCHECK(!end_of_stream_);
 
   DVLOG(1) << __FUNCTION__ << " " << GetStreamTypeName()
            << ": buffers " << BufferQueueToLogString(buffers);
 
-  // New media segments must begin with a keyframe.
-  // TODO(wolenetz): Relax this requirement. See http://crbug.com/229412.
-  if (new_media_segment_ && !buffers.front()->is_key_frame()) {
-    MEDIA_LOG(ERROR, media_log_) << "Media segment did not begin with key "
-                                    "frame. Support for such segments will be "
-                                    "available in a future version. Please see "
-                                    "https://crbug.com/229412.";
-    return false;
-  }
-
-  // Buffers within a media segment should be monotonically increasing.
-  if (!IsMonotonicallyIncreasing(buffers))
-    return false;
+  // New coded frame groups emitted by the coded frame processor must begin with
+  // a keyframe. TODO(wolenetz): Change this to [DCHECK + MEDIA_LOG(ERROR...) +
+  // return false] once the CHECK has baked in a stable release. See
+  // https://crbug.com/580621.
+  CHECK(!new_coded_frame_group_ || buffers.front()->is_key_frame());
 
-  if (media_segment_start_time_ < DecodeTimestamp() ||
-      buffers.front()->GetDecodeTimestamp() < DecodeTimestamp()) {
-    MEDIA_LOG(ERROR, media_log_)
-        << "Cannot append a media segment with negative timestamps.";
+  // Buffers within a coded frame group should be monotonically increasing.
+  if (!IsMonotonicallyIncreasing(buffers)) {
     return false;
   }
 
-  if (!IsNextTimestampValid(buffers.front()->GetDecodeTimestamp(),
-                            buffers.front()->is_key_frame())) {
-    const DecodeTimestamp& dts = buffers.front()->GetDecodeTimestamp();
+  if (coded_frame_group_start_time_ < DecodeTimestamp() ||
+      buffers.front()->GetDecodeTimestamp() < DecodeTimestamp()) {
     MEDIA_LOG(ERROR, media_log_)
-        << "Invalid same timestamp construct detected at"
-        << " time " << dts.InSecondsF();
-
+        << "Cannot append a coded frame group with negative timestamps.";
     return false;
   }
 
@@ -265,21 +265,40 @@ bool SourceBufferStream::Append(const BufferQueue& buffers) {
   // If there's a range for |buffers|, insert |buffers| accordingly. Otherwise,
   // create a new range with |buffers|.
   if (range_for_next_append_ != ranges_.end()) {
-    (*range_for_next_append_)->AppendBuffersToEnd(buffers);
+    if (new_coded_frame_group_ && (!splice_frames_enabled_ ||
+                                   buffers.front()->splice_buffers().empty())) {
+      // If the first append to this stream in a new coded frame group continues
+      // a previous range, use the new group's start time instead of the first
+      // new buffer's timestamp as the proof of adjacency to the existing range.
+      // A large gap (larger than our normal buffer adjacency test) can occur in
+      // a muxed set of streams (which share a common coded frame group start
+      // time) with a significantly jagged start across the streams.
+      // Don't do this logic if there was a splice frame generated for the first
+      // new buffer, since splices are guaranteed to be in the same range and
+      // adjacent, and since the splice frame's timestamp can be less than
+      // |coded_frame_group_start_time_| due to the splicing.
+      (*range_for_next_append_)
+          ->AppendBuffersToEnd(buffers, coded_frame_group_start_time_);
+    } else {
+      // Otherwise, use the first new buffer's timestamp as the proof of
+      // adjacency.
+      (*range_for_next_append_)
+          ->AppendBuffersToEnd(buffers, kNoDecodeTimestamp());
+    }
+
     last_appended_buffer_timestamp_ = buffers.back()->GetDecodeTimestamp();
     last_appended_buffer_duration_ = buffers.back()->duration();
     last_appended_buffer_is_keyframe_ = buffers.back()->is_key_frame();
   } else {
     DecodeTimestamp new_range_start_time = std::min(
-        media_segment_start_time_, buffers.front()->GetDecodeTimestamp());
+        coded_frame_group_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 key frame.
-    // This can happen if the GOP in the previous append gets destroyed
-    // by a Remove() call.
-    if (!new_media_segment_) {
+    // If the new range is not being created because of a new coded frame group,
+    // then we must make sure that we start with a key frame.  This can happen
+    // if the GOP in the previous append gets destroyed by a Remove() call.
+    if (!new_coded_frame_group_) {
       BufferQueue::const_iterator itr = buffers.begin();
 
       // Scan past all the non-key-frames.
@@ -294,7 +313,7 @@ bool SourceBufferStream::Append(const BufferQueue& buffers) {
         last_appended_buffer_duration_ = buffers.back()->duration();
         last_appended_buffer_is_keyframe_ = buffers.back()->is_key_frame();
         DVLOG(1) << __FUNCTION__ << " " << GetStreamTypeName()
-                 << ": new buffers in the middle of media segment depend on"
+                 << ": new buffers in the middle of coded frame group depend on"
                     "keyframe that has been removed, and contain no keyframes."
                     "Skipping further processing.";
         DVLOG(1) << __FUNCTION__ << " " << GetStreamTypeName()
@@ -324,7 +343,7 @@ bool SourceBufferStream::Append(const BufferQueue& buffers) {
         buffers_for_new_range->back()->is_key_frame();
   }
 
-  new_media_segment_ = false;
+  new_coded_frame_group_ = false;
 
   MergeWithAdjacentRangeIfNecessary(range_for_next_append_);
 
@@ -413,6 +432,26 @@ void SourceBufferStream::Remove(base::TimeDelta start, base::TimeDelta end,
   }
 }
 
+DecodeTimestamp SourceBufferStream::PotentialNextAppendTimestamp() const {
+  // The next potential append will either be just at or after
+  // |last_appended_buffer_timestamp_| (if known), or if unknown and we are
+  // still at the beginning of a new coded frame group, then will be into the
+  // range (if any) to which |coded_frame_group_start_time_| belongs.
+  if (last_appended_buffer_timestamp_ != kNoDecodeTimestamp()) {
+    return last_appended_buffer_timestamp_ +
+           base::TimeDelta::FromInternalValue(1);

[chcunningham, 2016/02/12 22:46:40]

What if the next timestamp is the same as the last timestamp? Should we not do
the +1 micro

[chcunningham, 2016/02/12 23:25:36]

We chatted. Please make a todo && crbug. This is at least no worse than it was
previously and doesn't seem to be impacted by key frame relaxation.

[wolenetz, 2016/02/24 00:34:46]

I think this +1us was useful when we previously had more strict disallowance of
Same Timestamp sequences of various kinds (now more lenient) along with
uncertainty about all buffers having nonzero duration (crbug 312836). Since
SourceBufferRange::BelongsToRange() is now more lenient w.r.t. same timestamp
sequences, I think we can relax this particular +1us. I'm on the fence about
doing that in this CL at this point, so I've gone with our previous decision to
TODO + crbug for this.

[chcunningham, 2016/02/24 19:06:26]

Acknowledged.

+  }
+
+  if (new_coded_frame_group_)
+    return coded_frame_group_start_time_;
+
+  // If we still don't know a potential next append timestamp, then we have
+  // removed the ranged to which it previously belonged and have not completed a
+  // subsequent append or received a subsequent OnStartOfCodedFrameGroup()
+  // signal.
+  return kNoDecodeTimestamp();
+}
+
 void SourceBufferStream::RemoveInternal(DecodeTimestamp start,
                                         DecodeTimestamp end,
                                         bool exclude_start,
@@ -440,6 +479,19 @@ void SourceBufferStream::RemoveInternal(DecodeTimestamp start,
     SourceBufferRange* new_range = range->SplitRange(end);
     if (new_range) {
       itr = ranges_.insert(++itr, new_range);
+
+      // Update |range_for_next_append_| if it was previously |range| and should
+      // be |new_range| now.
+      if (range_for_next_append_ != ranges_.end() &&
+          *range_for_next_append_ == range) {
+        DecodeTimestamp potential_next_append_timestamp =
+            PotentialNextAppendTimestamp();
+        if (potential_next_append_timestamp != kNoDecodeTimestamp() &&
+            new_range->BelongsToRange(potential_next_append_timestamp)) {
+          range_for_next_append_ = itr;
+        }
+      }
+
       --itr;
 
       // Update the selected range if the next buffer position was transferred
@@ -476,11 +528,9 @@ void SourceBufferStream::RemoveInternal(DecodeTimestamp start,
     // 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_ != kNoDecodeTimestamp()) {
+        *range_for_next_append_ == range) {
       DecodeTimestamp potential_next_append_timestamp =
-          last_appended_buffer_timestamp_ +
-          base::TimeDelta::FromInternalValue(1);
+          PotentialNextAppendTimestamp();
 
       if (!range->BelongsToRange(potential_next_append_timestamp)) {
         DVLOG(1) << "Resetting range_for_next_append_ since the next append"
@@ -522,8 +572,7 @@ bool SourceBufferStream::ShouldSeekToStartOfBuffered(
           beginning_of_buffered < kSeekToStartFudgeRoom());
 }
 
-bool SourceBufferStream::IsMonotonicallyIncreasing(
-    const BufferQueue& buffers) const {
+bool SourceBufferStream::IsMonotonicallyIncreasing(const BufferQueue& buffers) {
   DCHECK(!buffers.empty());
   DecodeTimestamp prev_timestamp = last_appended_buffer_timestamp_;
   bool prev_is_keyframe = last_appended_buffer_is_keyframe_;
@@ -546,12 +595,13 @@ bool SourceBufferStream::IsMonotonicallyIncreasing(
       }
 
       if (current_timestamp == prev_timestamp &&
-          !SourceBufferRange::AllowSameTimestamp(prev_is_keyframe,
-                                                 current_is_keyframe)) {
-        MEDIA_LOG(ERROR, media_log_) << "Unexpected combination of buffers with"
-                                     << " the same timestamp detected at "
-                                     << current_timestamp.InSecondsF();
-        return false;
+          SourceBufferRange::IsUncommonSameTimestampSequence(
+              prev_is_keyframe, current_is_keyframe)) {
+        LIMITED_MEDIA_LOG(DEBUG, media_log_, num_strange_same_timestamps_logs_,
+                          kMaxStrangeSameTimestampsLogs)
+            << "Detected an append sequence with keyframe following a "
+               "non-keyframe, both with the same decode timestamp of "
+            << current_timestamp.InSecondsF();
       }
     }
 
@@ -561,15 +611,6 @@ bool SourceBufferStream::IsMonotonicallyIncreasing(
   return true;
 }
 
-bool SourceBufferStream::IsNextTimestampValid(
-    DecodeTimestamp next_timestamp, bool next_is_keyframe) const {
-  return (last_appended_buffer_timestamp_ != next_timestamp) ||
-      new_media_segment_ ||
-      SourceBufferRange::AllowSameTimestamp(last_appended_buffer_is_keyframe_,
-                                            next_is_keyframe);
-}
-
-
 bool SourceBufferStream::OnlySelectedRangeIsSeeked() const {
   for (RangeList::const_iterator itr = ranges_.begin();
        itr != ranges_.end(); ++itr) {
@@ -927,9 +968,7 @@ void SourceBufferStream::PrepareRangesForNextAppend(
     GenerateSpliceFrame(new_buffers);
 
   DecodeTimestamp prev_timestamp = last_appended_buffer_timestamp_;
-  bool prev_is_keyframe = last_appended_buffer_is_keyframe_;
   DecodeTimestamp next_timestamp = new_buffers.front()->GetDecodeTimestamp();
-  bool next_is_keyframe = new_buffers.front()->is_key_frame();
 
   if (prev_timestamp != kNoDecodeTimestamp() &&
       prev_timestamp != next_timestamp) {
@@ -938,22 +977,25 @@ void SourceBufferStream::PrepareRangesForNextAppend(
     RemoveInternal(prev_timestamp, next_timestamp, true, deleted_buffers);
   }
 
-  // Make the delete range exclusive if we are dealing with an allowed same
-  // timestamp situation. This prevents the first buffer in the current append
-  // from deleting the last buffer in the previous append if both buffers
-  // have the same timestamp.
-  //
-  // The delete range should never be exclusive if a splice frame was generated
-  // because we don't generate splice frames for same timestamp situations.
+  // Always make the start of the delete range exclusive for same timestamp
+  // across the last buffer in the previous append and the first buffer in the
+  // current append. Never be exclusive if a splice frame was generated because
+  // we don't generate splice frames for same timestamp situations.
   DCHECK(new_buffers.front()->splice_timestamp() !=
          new_buffers.front()->timestamp());
-  const bool exclude_start =
-      new_buffers.front()->splice_buffers().empty() &&
-      prev_timestamp == next_timestamp &&
-      SourceBufferRange::AllowSameTimestamp(prev_is_keyframe, next_is_keyframe);
+  const bool exclude_start = new_buffers.front()->splice_buffers().empty() &&
+                             prev_timestamp == next_timestamp;
 
   // Delete the buffers that |new_buffers| overlaps.
   DecodeTimestamp start = new_buffers.front()->GetDecodeTimestamp();
+  if (new_coded_frame_group_) {
+    // Extend the deletion range earlier to the coded frame group start time if
+    // this is the first append in a new coded frame group. Note that |start|
+    // could already be less than |coded_frame_group_start_time_| if a splice
+    // was generated.
+    DCHECK(coded_frame_group_start_time_ != kNoDecodeTimestamp());
+    start = std::min(coded_frame_group_start_time_, start);
+  }
   DecodeTimestamp end = new_buffers.back()->GetDecodeTimestamp();
   base::TimeDelta duration = new_buffers.back()->duration();