MSE: Signal new coded frame group more aggressively

For 'sequence' mode coded frame processing, we may need to signal a new
coded frame group because application may override timestampOffset and
cause the processed frame sequence to go backwards in time. Jumps
forward are allowed and kept within the same coded frame group. Jumps
backward are now detected and trigger new coded frame group signalling
so the overlap-append logic in SourceBufferStream handles this situation
more correctly.

TEST=*FrameProcessorTest.AudioVideo_Discontinuity_TimestampOffset*, http/tests/media/media-source/mediasource-crbug-616565.html
BUG=616565,620523

Review-Url: https://codereview.chromium.org/2035003002
Cr-Commit-Position: refs/heads/master@{#400249}

media/filters/frame_processor.cc

@@ -185,6 +185,10 @@ void FrameProcessor::SetSequenceMode(bool sequence_mode) {
   if (sequence_mode) {
     DCHECK(kNoTimestamp() != group_end_timestamp_);
     group_start_timestamp_ = group_end_timestamp_;
+  } else if (sequence_mode_) {
+    // We're switching from 'sequence' to 'segments' mode. Be safe and signal a
+    // new coded frame group on the next frame emitted.
+    coded_frame_group_last_dts_ = kNoDecodeTimestamp();
   }
 
   // Step 8: Update the attribute to new mode.
@@ -292,10 +296,13 @@ void FrameProcessor::Reset() {
     itr->second->Reset();
   }
 
-  // Maintain current |in_coded_frame_group_| state for Reset() during
-  // sequence mode. Reset it here only if in segments mode.
+  // Maintain current |coded_frame_group_last_dts_| state for Reset() during
+  // sequence mode. Reset it here only if in segments mode. In sequence mode,
+  // the current coded frame group may be continued across Reset() operations to
+  // allow the stream to coaelesce what might otherwise be gaps in the buffered
+  // ranges. See also the declaration for |coded_frame_group_last_dts_|.
   if (!sequence_mode_) {
-    in_coded_frame_group_ = false;
+    coded_frame_group_last_dts_ = kNoDecodeTimestamp();
     return;
   }
 
@@ -601,7 +608,7 @@ bool FrameProcessor::ProcessFrame(
           decode_timestamp - track_last_decode_timestamp;
       if (track_dts_delta < base::TimeDelta() ||
           track_dts_delta > 2 * track_buffer->last_frame_duration()) {
-        DCHECK(in_coded_frame_group_);
+        DCHECK(coded_frame_group_last_dts_ != kNoDecodeTimestamp());
         // 6.1. If mode equals "segments": Set group end timestamp to
         //      presentation timestamp.
         //      If mode equals "sequence": Set group start timestamp equal to
@@ -610,13 +617,13 @@ bool FrameProcessor::ProcessFrame(
           group_end_timestamp_ = presentation_timestamp;
           // This triggers a discontinuity so we need to treat the next frames
           // appended within the append window as if they were the beginning of
-          // a new coded frame group.
-          in_coded_frame_group_ = false;
+          // a new coded frame group. |coded_frame_group_last_dts_| is reset in
+          // Reset(), below, for "segments" mode.
         } else {
           DVLOG(3) << __FUNCTION__ << " : Sequence mode discontinuity, GETS: "
                    << group_end_timestamp_.InSecondsF();
-          DCHECK(kNoTimestamp() != group_end_timestamp_);
-          group_start_timestamp_ = group_end_timestamp_;
+          // Reset(), below, performs the "Set group start timestamp equal to
+          // the group end timestamp" operation for "sequence" mode.
         }
 
         // 6.2. - 6.5.:
@@ -704,18 +711,24 @@ bool FrameProcessor::ProcessFrame(
     // If it is the first in a new coded frame group (such as following a
     // discontinuity), notify all the track buffers' streams that a coded frame
     // group is starting.
-    if (!in_coded_frame_group_) {
+    // If in 'sequence' appendMode, also check to make sure we don't need to
+    // signal the start of a new coded frame group in the case where
+    // timestampOffset adjustments by the app may cause this coded frame to be
+    // in the timeline prior to the last frame processed.
+    if (coded_frame_group_last_dts_ == kNoDecodeTimestamp() ||
+        (sequence_mode_ && coded_frame_group_last_dts_ > decode_timestamp)) {
       // First, complete the append to track buffer streams of the previous
       // coded frame group's frames, if any.
       if (!FlushProcessedFrames())
         return false;
 
-      // TODO(acolwell/wolenetz): This should be changed to a presentation
-      // timestamp. See http://crbug.com/402502
+      // TODO(wolenetz): This should be changed to a presentation timestamp. See
+      // http://crbug.com/402502
       NotifyStartOfCodedFrameGroup(decode_timestamp);
-      in_coded_frame_group_ = true;
     }
 
+    coded_frame_group_last_dts_ = decode_timestamp;
+
     DVLOG(3) << __FUNCTION__ << ": Sending processed frame to stream, "
              << "PTS=" << presentation_timestamp.InSecondsF()
              << ", DTS=" << decode_timestamp.InSecondsF();

media/filters/frame_processor.h

@@ -152,11 +152,17 @@ class MEDIA_EXPORT FrameProcessor {
   // set to false ("segments").
   bool sequence_mode_ = false;
 
-  // Flag to track whether or not the next processed frame is a continuation of
-  // a coded frame group. This flag resets to false upon detection of
-  // discontinuity, and becomes true once a processed coded frame for the
-  // current coded frame group is sent to its track buffer.
-  bool in_coded_frame_group_ = false;
+  // Tracks whether or not the next processed frame is a continuation of a coded
+  // frame group (see https://w3c.github.io/media-source/#coded-frame-group).
+  // Resets to kNoDecodeTimestamp() upon detection of 'segments' mode
+  // discontinuity, parser reset during 'segments' mode, or switching from
+  // 'sequence' to 'segments' mode.
+  // Once a processed coded frame is emitted for the current coded frame group,
+  // tracks the decode timestamp of the last frame emitted.
+  // Explicit setting of timestampOffset will trigger subsequent notification of
+  // a new coded frame start to the tracks' streams, even in 'sequence' mode, if
+  // the resulting frame has a DTS less than this.
+  DecodeTimestamp coded_frame_group_last_dts_ = kNoDecodeTimestamp();
 
   // Tracks the MSE coded frame processing variable of same name.
   // Initially kNoTimestamp(), meaning "unset".

media/filters/frame_processor_unittest.cc

@@ -248,7 +248,8 @@ class FrameProcessorTest : public testing::TestWithParam<bool> {
   // thereof of new coded frame group by the FrameProcessor. See
   // https://crbug.com/580613.
   bool in_coded_frame_group() {
-    return frame_processor_->in_coded_frame_group_;
+    return frame_processor_->coded_frame_group_last_dts_ !=
+           kNoDecodeTimestamp();
   }
 
   base::MessageLoop message_loop_;
@@ -582,6 +583,113 @@ TEST_P(FrameProcessorTest, AudioVideo_Discontinuity) {
   }
 }
 
+TEST_P(FrameProcessorTest, AudioVideo_Discontinuity_TimestampOffset) {
+  // If in 'sequence' mode, a new coded frame group is *only* started if the
+  // processed frame sequence outputs something that goes backwards in DTS
+  // order. This helps retain the intent of 'sequence' mode: it both collapses
+  // gaps as well as allows app to override the timeline placement and so needs
+  // to handle overlap-appends, too.
+  InSequence s;
+  AddTestTracks(HAS_AUDIO | HAS_VIDEO);
+  bool using_sequence_mode = GetParam();
+  frame_processor_->SetSequenceMode(using_sequence_mode);
+
+  // Start a coded frame group at time 100ms. Note the jagged start still uses
+  // the coded frame group's start time as the range start for both streams.
+  EXPECT_CALL(callbacks_, PossibleDurationIncrease(frame_duration_ * 14));
+  SetTimestampOffset(frame_duration_ * 10);
+  ProcessFrames("0K 10K 20K", "10K 20K 30K");
+  EXPECT_EQ(frame_duration_ * 10, timestamp_offset_);
+  EXPECT_TRUE(in_coded_frame_group());
+  CheckExpectedRangesByTimestamp(audio_.get(), "{ [100,130) }");
+  CheckExpectedRangesByTimestamp(video_.get(), "{ [100,140) }");
+
+  // Test the differentiation between 'sequence' and 'segments' mode results if
+  // the coded frame sequence jumps forward beyond the normal discontinuity
+  // threshold.
+  EXPECT_CALL(callbacks_, PossibleDurationIncrease(frame_duration_ * 24));
+  SetTimestampOffset(frame_duration_ * 20);
+  ProcessFrames("0K 10K 20K", "10K 20K 30K");
+  EXPECT_EQ(frame_duration_ * 20, timestamp_offset_);
+  EXPECT_TRUE(in_coded_frame_group());
+  if (using_sequence_mode) {
+    CheckExpectedRangesByTimestamp(audio_.get(), "{ [100,230) }");
+    CheckExpectedRangesByTimestamp(video_.get(), "{ [100,240) }");
+  } else {
+    CheckExpectedRangesByTimestamp(audio_.get(), "{ [100,130) [200,230) }");
+    CheckExpectedRangesByTimestamp(video_.get(), "{ [100,140) [200,240) }");
+  }
+
+  // Test the behavior when timestampOffset adjustment causes next frames to be
+  // in the past relative to the previously processed frame and triggers a new
+  // coded frame group, even in 'sequence' mode.
+  base::TimeDelta fifty_five_ms = base::TimeDelta::FromMilliseconds(55);
+  EXPECT_CALL(callbacks_,
+              PossibleDurationIncrease(fifty_five_ms + frame_duration_ * 4));
+  SetTimestampOffset(fifty_five_ms);
+  ProcessFrames("0K 10K 20K", "10K 20K 30K");
+  EXPECT_EQ(fifty_five_ms, timestamp_offset_);
+  EXPECT_TRUE(in_coded_frame_group());
+  // The new audio range is not within SourceBufferStream's coalescing threshold
+  // relative to the next range, but the new video range is within the
+  // threshold.
+  if (using_sequence_mode) {
+    // TODO(wolenetz/chcunningham): The large explicit-timestampOffset-induced
+    // jump forward (from timestamp 130 to 200) while in a sequence mode coded
+    // frame group makes our adjacency threshold in SourceBuffer, based on
+    // max-interbuffer-distance-within-coded-frame-group, very lenient.
+    // This causes [55,85) to merge with [100,230) here for audio, and similar
+    // for video. See also https://crbug.com/620523.
+    CheckExpectedRangesByTimestamp(audio_.get(), "{ [55,230) }");
+    CheckExpectedRangesByTimestamp(video_.get(), "{ [55,240) }");
+  } else {
+    CheckExpectedRangesByTimestamp(audio_.get(),
+                                   "{ [55,85) [100,130) [200,230) }");
+    // Note that the range adjacency logic used in this case is doesn't consider
+    // DTS 85 to be close enough to [100,140), since the first DTS in video
+    // range [100,140) is actually 110. The muxed data started a coded frame
+    // group at time 100, but actual DTS is used for adjacency checks while
+    // appending.
+    CheckExpectedRangesByTimestamp(video_.get(),
+                                   "{ [55,95) [100,140) [200,240) }");
+  }
+
+  // Verify the buffers.
+  audio_->AbortReads();
+  audio_->Seek(fifty_five_ms);
+  audio_->StartReturningData();
+  video_->AbortReads();
+  video_->Seek(fifty_five_ms);
+  video_->StartReturningData();
+  if (using_sequence_mode) {
+    CheckReadsThenReadStalls(
+        audio_.get(),
+        "55:0 65:10 75:20 100:0 110:10 120:20 200:0 210:10 220:20");
+    CheckReadsThenReadStalls(
+        video_.get(),
+        "65:10 75:20 85:30 110:10 120:20 130:30 210:10 220:20 230:30");
+  } else {
+    CheckReadsThenReadStalls(audio_.get(), "55:0 65:10 75:20");
+    CheckReadsThenReadStalls(video_.get(), "65:10 75:20 85:30");
+    audio_->AbortReads();
+    audio_->Seek(frame_duration_ * 10);
+    audio_->StartReturningData();
+    video_->AbortReads();
+    video_->Seek(frame_duration_ * 10);
+    video_->StartReturningData();
+    CheckReadsThenReadStalls(audio_.get(), "100:0 110:10 120:20");
+    CheckReadsThenReadStalls(video_.get(), "110:10 120:20 130:30");
+    audio_->AbortReads();
+    audio_->Seek(frame_duration_ * 20);
+    audio_->StartReturningData();
+    video_->AbortReads();
+    video_->Seek(frame_duration_ * 20);
+    video_->StartReturningData();
+    CheckReadsThenReadStalls(audio_.get(), "200:0 210:10 220:20");
+    CheckReadsThenReadStalls(video_.get(), "210:10 220:20 230:30");
+  }
+}
+
 TEST_P(FrameProcessorTest,
        AppendWindowFilterOfNegativeBufferTimestampsWithPrerollDiscard) {
   InSequence s;

third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-sequencemode-crbug-616565.html

@@ -0,0 +1,71 @@
+<!DOCTYPE html>
+<html>
+    <head>
+        <script src="/w3c/resources/testharness.js"></script>
+        <script src="/w3c/resources/testharnessreport.js"></script>
+        <script src="mediasource-util.js"></script>
+    </head>
+    <body>
+        <script>
+            mediasource_testafterdataloaded(function(test, mediaElement, mediaSource, segmentInfo, sourceBuffer, mediaData)
+            {
+                assert_greater_than(segmentInfo.media.length, 3, "at least 3 media segments for supported type");
+                test.failOnEvent(mediaElement, "error");
+                sourceBuffer.mode = "sequence";
+                assert_equals(sourceBuffer.mode, "sequence", "mode after setting it to 'sequence'");
+
+                // Append all media segments, preceded by a (now redundant) initialization segment.
+                test.expectEvent(sourceBuffer, "updatestart", "media segment append started.");
+                test.expectEvent(sourceBuffer, "update", "media segment append success.");
+                test.expectEvent(sourceBuffer, "updateend", "media segment append ended.");
+                sourceBuffer.appendBuffer(mediaData);
+
+                test.waitForExpectedEvents(function()
+                {
+                    // Remove much of what we just appended.
+                    test.expectEvent(sourceBuffer, "updatestart", "remove started.");
+                    test.expectEvent(sourceBuffer, "update", "remove success.");
+                    test.expectEvent(sourceBuffer, "updateend", "remove ended.");
+                    sourceBuffer.remove(segmentInfo.media[3].timecode, mediaSource.duration);
+                });
+
+                test.waitForExpectedEvents(function()
+                {
+                    // Set the timestampOffset to the beginning of the region we just removed.
+                    sourceBuffer.timestampOffset = segmentInfo.media[3].timecode;
+
+                    // Remove everything.
+                    test.expectEvent(sourceBuffer, "updatestart", "remove started.");
+                    test.expectEvent(sourceBuffer, "update", "remove success.");
+                    test.expectEvent(sourceBuffer, "updateend", "remove ended.");
+                    sourceBuffer.remove(0, mediaSource.duration);
+                });
+
+                test.waitForExpectedEvents(function()
+                {
+                    // Set the timestampOffset back to the beginning.
+                    sourceBuffer.timestampOffset = 0;
+
+                    // Re-append everything.
+                    test.expectEvent(sourceBuffer, "updatestart", "media segment append started.");
+                    test.expectEvent(sourceBuffer, "update", "media segment append success.");
+                    test.expectEvent(sourceBuffer, "updateend", "media segment append ended.");
+                    sourceBuffer.appendBuffer(mediaData);
+                });
+
+                test.waitForExpectedEvents(function()
+                {
+                    // Use EOS to get a more precisely verifiable buffered range given our segmentInfo.
+                    test.expectEvent(mediaSource, "sourceended", "mediaSource endOfStream");
+                    mediaSource.endOfStream();
+                });
+
+                test.waitForExpectedEvents(function()
+                {
+                    assertBufferedEquals(sourceBuffer, "{ [0.000, " + segmentInfo.duration.toFixed(3) + ") }");
+                    test.done();
+                });
+            }, "Test no error for sequence mode remove and timestampOffset scenario in bug 616565");
+        </script>
+    </body>
+</html>