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..103aaf6e1409aa4b42b5d385787e059ec37c7f11 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,22 +191,22 @@ 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;
@@ -215,41 +224,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.

[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;
   }
 
-  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;
   }
 
@@ -271,15 +268,14 @@ bool SourceBufferStream::Append(const BufferQueue& buffers) {
     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 +290,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 +320,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_);
 
@@ -440,6 +436,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

[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
@@ -522,8 +531,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 +554,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(

[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();
       }
     }
 
@@ -561,15 +570,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 +927,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,19 +936,14 @@ 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

[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();