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

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 <sstream>
 
@@ skipping 19 matching lines @@
 // important for debugging splice generation.
 const int kMaxSpliceGenerationWarningLogs = 50;
 const int kMaxSpliceGenerationSuccessLogs = 20;
 
 // Limit the number of MEDIA_LOG() logs for track buffer time gaps.
 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) {
   DecodeTimestamp prev = kNoDecodeTimestamp();
   for (std::list<SourceBufferRange*>::const_iterator itr =
        ranges.begin(); itr != ranges.end(); ++itr) {
     if (prev != kNoDecodeTimestamp() && prev >= (*itr)->GetStartTimestamp())
       return false;
     prev = (*itr)->GetEndTimestamp();
   }
@@ skipping 80 matching lines @@
   return SourceBufferRange::NO_GAPS_ALLOWED;
 }
 
 }  // namespace
 
 SourceBufferStream::SourceBufferStream(const AudioDecoderConfig& audio_config,
                                        const scoped_refptr<MediaLog>& media_log,
                                        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()),
       memory_limit_(kSourceBufferAudioMemoryLimit),
       splice_frames_enabled_(splice_frames_enabled) {
   DCHECK(audio_config.IsValidConfig());
   audio_configs_.push_back(audio_config);
 }
 
 SourceBufferStream::SourceBufferStream(const VideoDecoderConfig& video_config,
                                        const scoped_refptr<MediaLog>& media_log,
                                        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()),
       memory_limit_(kSourceBufferVideoMemoryLimit),
       splice_frames_enabled_(splice_frames_enabled) {
   DCHECK(video_config.IsValidConfig());
   video_configs_.push_back(video_config);
 }
 
 SourceBufferStream::SourceBufferStream(const TextTrackConfig& text_config,
                                        const scoped_refptr<MediaLog>& media_log,
                                        bool splice_frames_enabled)
     : 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()),
       memory_limit_(kSourceBufferAudioMemoryLimit),
       splice_frames_enabled_(splice_frames_enabled) {}
 
 SourceBufferStream::~SourceBufferStream() {
   while (!ranges_.empty()) {
     delete ranges_.front();
     ranges_.pop_front();
   }
 }
 
-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";
   } else if (last_range != ranges_.end()) {
     DCHECK(last_range == range_for_next_append_);
     DVLOG(3) << __FUNCTION__ << " next appended buffers will continue range "
              << "unless intervening remove makes discontinuity";
   }
 }
 
 bool SourceBufferStream::Append(const BufferQueue& buffers) {
   TRACE_EVENT2("media", "SourceBufferStream::Append",
                "stream type", GetStreamTypeName(),
                "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.";
+  // 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());
+
+  // Buffers within a coded frame group should be monotonically increasing.

[wolenetz, 2016/02/05 23:27:24]

Note that IsMonotonicallyIncreasing() does the same work as the now-removed call
to and method "IsNextTimestampValid".

[chcunningham, 2016/02/09 00:21:08]

Acknowledged.

+  if (!IsMonotonicallyIncreasing(buffers)) {
     return false;
   }
 
-  // Buffers within a media segment should be monotonically increasing.
-  if (!IsMonotonicallyIncreasing(buffers))
-    return false;
-
-  if (media_segment_start_time_ < DecodeTimestamp() ||
+  if (coded_frame_group_start_time_ < DecodeTimestamp() ||
       buffers.front()->GetDecodeTimestamp() < DecodeTimestamp()) {
     MEDIA_LOG(ERROR, media_log_)
-        << "Cannot append a media segment with negative timestamps.";
+        << "Cannot append a coded frame group with negative timestamps.";
     return false;
   }
 
-  if (!IsNextTimestampValid(buffers.front()->GetDecodeTimestamp(),
-                            buffers.front()->is_key_frame())) {
-    const DecodeTimestamp& dts = buffers.front()->GetDecodeTimestamp();
-    MEDIA_LOG(ERROR, media_log_)
-        << "Invalid same timestamp construct detected at"
-        << " time " << dts.InSecondsF();
-
-    return false;
-  }
-
   UpdateMaxInterbufferDistance(buffers);
   SetConfigIds(buffers);
 
   // Save a snapshot of stream state before range modifications are made.
   DecodeTimestamp next_buffer_timestamp = GetNextBufferTimestamp();
   BufferQueue deleted_buffers;
 
   PrepareRangesForNextAppend(buffers, &deleted_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);
     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.
       while (itr != buffers.end() && !(*itr)->is_key_frame()) {
         ++itr;
       }
 
       // If we didn't find a key frame, then update the last appended
       // buffer state and return.
       if (itr == buffers.end()) {
         last_appended_buffer_timestamp_ = buffers.back()->GetDecodeTimestamp();
         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()
                  << ": done. ranges_=" << RangesToString(ranges_);
         return true;
       } else if (itr != buffers.begin()) {
         // Copy the first key frame and everything after it into
         // |trimmed_buffers|.
         trimmed_buffers.assign(itr, buffers.end());
         buffers_for_new_range = &trimmed_buffers;
       }
 
       new_range_start_time =
           buffers_for_new_range->front()->GetDecodeTimestamp();
     }
 
     range_for_next_append_ =
         AddToRanges(new SourceBufferRange(
             TypeToGapPolicy(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_duration_ = buffers_for_new_range->back()->duration();
     last_appended_buffer_is_keyframe_ =
         buffers_for_new_range->back()->is_key_frame();
   }
 
-  new_media_segment_ = false;
+  new_coded_frame_group_ = false;
 
   MergeWithAdjacentRangeIfNecessary(range_for_next_append_);
 
   // Seek to try to fulfill a previous call to Seek().
   if (seek_pending_) {
     DCHECK(!selected_range_);
     DCHECK(deleted_buffers.empty());
     Seek(seek_buffer_timestamp_);
   }
 
@@ skipping 95 matching lines @@
   while (itr != ranges_.end()) {
     SourceBufferRange* range = *itr;
     if (range->GetStartTimestamp() >= end)
       break;
 
     // Split off any remaining GOPs starting at or after |end| and add it to
     // |ranges_|.
     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

[wolenetz, 2016/02/05 23:27:24]

Note: this is the core of the fix for crbug 581458. Even tiny gaps *within* a
media segment, prior to keyframe relaxation, would cause a buffered range gap if
there were a removal of the first part of the media segment prior to the buffers
following the tiny gap being appended. This fixes the same situation at the
coded-frame-group level of abstraction.

[chcunningham, 2016/02/09 00:21:08]

Acknowledged.

+      // be |new_range| now.
+      if (range_for_next_append_ != ranges_.end() &&
+          *range_for_next_append_ == range &&
+          last_appended_buffer_timestamp_ != kNoDecodeTimestamp()) {

[chcunningham, 2016/02/09 00:21:08]

I'm wondering about the case where we've just gotten a OnStartOfCodedFrameGroup,
such that new_range should be the range_for_next_append_, but we don't pick it
up here because last_appeneded_buffer_timestamp_ == kNoDecodeTimestamp()? I
*think* what would happen is we would end up creating a *new* range on the next
append... probably not what we want. I though MergeWithAjacentRange would save
us, but then it seems that only merges with the range ahead in the range list,
not the range behind. WDYT? 

It might be more correct to remove this guard on line 444 and change the lines
below to read:

DecodeTimestamp potential_next_append_timestamp = 
  last_appended_buffer_timestamp_ == kNoTimestamp() ?   
    coded_frame_group_start_time_ :
    last_appended_buffer_timestamp_ +           
base::TimeDelta::FromInternalValue(1);


This probably needs a unit tests to be fully fleshed out.

[wolenetz, 2016/02/09 00:47:19]

I'll need to grok this a little more. I'll get an answer for this tomorrow
morning :)

[wolenetz, 2016/02/09 01:36:41]

Great comment, Chris!

We've never tested any case like "starting a new {media segment or coded frame
group}, but don't append anything yet. Instead remove something and then maybe
append stuff later". Indeed this causes problems (as I've found while just now
adding an initial test like this locally).

I'll proceed with adding more tests and fixing the various issues (currently
DCHECK on line 50 of source_buffer_range.cc occurs for the one test I've tried
so far...)

[wolenetz, 2016/02/12 01:23:47]

Done. (as of patch set #5)

+        DecodeTimestamp potential_next_append_timestamp =
+            last_appended_buffer_timestamp_ +
+            base::TimeDelta::FromInternalValue(1);
+        if (new_range->BelongsToRange(potential_next_append_timestamp))
+          range_for_next_append_ = itr;
+      }
+
       --itr;
 
       // Update the selected range if the next buffer position was transferred
       // to |new_range|.
       if (new_range->HasNextBufferPosition())
         SetSelectedRange(new_range);
     }
 
     // Truncate the current range so that it only contains data before
     // the removal range.
@@ skipping 62 matching lines @@
 bool SourceBufferStream::ShouldSeekToStartOfBuffered(
     base::TimeDelta seek_timestamp) const {
   if (ranges_.empty())
     return false;
   base::TimeDelta beginning_of_buffered =
       ranges_.front()->GetStartTimestamp().ToPresentationTime();
   return (seek_timestamp <= beginning_of_buffered &&
           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_;
   for (BufferQueue::const_iterator itr = buffers.begin();
        itr != buffers.end(); ++itr) {
     DecodeTimestamp current_timestamp = (*itr)->GetDecodeTimestamp();
     bool current_is_keyframe = (*itr)->is_key_frame();
     DCHECK(current_timestamp != kNoDecodeTimestamp());
     DCHECK((*itr)->duration() >= base::TimeDelta())
         << "Packet with invalid duration."
         << " pts " << (*itr)->timestamp().InSecondsF()
         << " dts " << (*itr)->GetDecodeTimestamp().InSecondsF()
         << " dur " << (*itr)->duration().InSecondsF();
 
     if (prev_timestamp != kNoDecodeTimestamp()) {
       if (current_timestamp < prev_timestamp) {
         MEDIA_LOG(ERROR, media_log_)
             << "Buffers did not monotonically increase.";
         return false;
       }
 
       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(

[wolenetz, 2016/02/05 23:27:24]

Note, this is the core of the fix for crbug 581125. We now allow these uncommon
sequences (which can occur across media segments even), but medialog (up to
capped number) when they occur. This corresponds to the "don't drop b" version
from our internal thread, chcunningham@.

[chcunningham, 2016/02/09 00:21:08]

Acknowledged.

+              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();
       }
     }
 
     prev_timestamp = current_timestamp;
     prev_is_keyframe = current_is_keyframe;
   }
   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) {
     if ((*itr)->HasNextBufferPosition() && (*itr) != selected_range_)
       return false;
   }
   return !selected_range_ || selected_range_->HasNextBufferPosition();
 }
 
 void SourceBufferStream::UpdateMaxInterbufferDistance(
@@ skipping 337 matching lines @@
     const BufferQueue& new_buffers, BufferQueue* deleted_buffers) {
   DCHECK(deleted_buffers);
 
   // Handle splices between the existing buffers and the new buffers.  If a
   // splice is generated the timestamp and duration of the first buffer in
   // |new_buffers| will be modified.
   if (splice_frames_enabled_)
     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) {
     // Clean up the old buffers between the last appended buffer and the
     // beginning of |new_buffers|.
     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

[chcunningham, 2016/02/09 00:21:08]

Does the splice buffers check really make sense here? If we never generate a
splice in the event of same timestamp (not enough duration to make a good
splice), then it should really be more of a DCHECK, and probably closer to the
code where splices are generated instead of down here in
PrepareRangesForNextAppend. Perhaps something for a follow up CL? 

[wolenetz, 2016/02/09 00:47:19]

Hmm. PrepareRangesForNextAppend() actually calls GenerateSpliceFrame(), above
(line 927). This DCHECK is part of protecting the borders to prevent a possibly
divergent GenerateSpliceFrame() impl from breaking bigger things. Regardless, I
think a change to the splice checking is out of scope of this CL. WDYT?

[wolenetz, 2016/02/12 01:23:47]

From chat, this is fodder for a low pri later CL.

+  // 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();
   DecodeTimestamp end = new_buffers.back()->GetDecodeTimestamp();
   base::TimeDelta duration = new_buffers.back()->duration();
 
   // Set end time for remove to include the duration of last buffer. If the
   // duration is estimated, use 1 microsecond instead to ensure frames are not
   // accidentally removed due to over-estimation.
   if (duration != kNoTimestamp() && duration > base::TimeDelta() &&
@@ skipping 818 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