MSE: Replace crossfade splicing overlap trimming.

media/filters/chunk_demuxer_unittest.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/chunk_demuxer.h"
 
 #include <stddef.h>
 #include <stdint.h>
 #include <algorithm>
 #include <utility>
@@ skipping 147 matching lines @@
       arg, "Initialization segment " +
                std::string(mime_missing_track_type_bool ? "has "
                                                         : "does not have ") +
                std::string(track_type_string_with_article) +
                " track, but the mimetype " +
                std::string(mime_missing_track_type_bool ? "does not specify "
                                                         : "specifies ") +
                std::string(track_type_string_with_article) + " codec.");
 }
 
-MATCHER_P2(GeneratedSplice, duration_microseconds, time_microseconds, "") {
-  return CONTAINS_STRING(arg, "Generated splice of overlap duration " +
-                                  base::IntToString(duration_microseconds) +
-                                  "us into new buffer at " +
-                                  base::IntToString(time_microseconds) + "us.");
+MATCHER_P3(TrimmedSpliceOverlap,
+           splice_time_us,
+           overlapped_start_us,
+           trim_duration_us,
+           "") {
+  return CONTAINS_STRING(
+      arg, "Audio buffer splice at PTS=" + base::IntToString(splice_time_us) +
+               "us. Trimmed tail of overlapped buffer (PTS=" +
+               base::IntToString(overlapped_start_us) + "us) by " +
+               base::IntToString(trim_duration_us));
 }
 
-MATCHER_P2(SkippingSpliceAtOrBefore,
-           new_microseconds,
-           existing_microseconds,
-           "") {
-  return CONTAINS_STRING(
-      arg, "Skipping splice frame generation: first new buffer at " +
-               base::IntToString(new_microseconds) +
-               "us begins at or before existing buffer at " +
-               base::IntToString(existing_microseconds) + "us.");
-}
-
-MATCHER_P(SkippingSpliceAlreadySpliced, time_microseconds, "") {
-  return CONTAINS_STRING(
-      arg, "Skipping splice frame generation: overlapped buffers at " +
-               base::IntToString(time_microseconds) +
-               "us are in a previously buffered splice.");
-}
 
 MATCHER_P(WebMSimpleBlockDurationEstimated, estimated_duration_ms, "") {
   return CONTAINS_STRING(arg, "Estimating WebM block duration to be " +
                                   base::IntToString(estimated_duration_ms) +
                                   "ms for the last (Simple)Block in the "
                                   "Cluster for this Track. Use BlockGroups "
                                   "with BlockDurations at the end of each "
                                   "Track in a Cluster to avoid estimation.");
 }
 
@@ skipping 69 matching lines @@
     init_segment_received_cb_ = base::Bind(
         &ChunkDemuxerTest::InitSegmentReceived, base::Unretained(this));
     CreateNewDemuxer();
   }
 
   void CreateNewDemuxer() {
     base::Closure open_cb =
         base::Bind(&ChunkDemuxerTest::DemuxerOpened, base::Unretained(this));
     Demuxer::EncryptedMediaInitDataCB encrypted_media_init_data_cb = base::Bind(
         &ChunkDemuxerTest::OnEncryptedMediaInitData, base::Unretained(this));
-    demuxer_.reset(new ChunkDemuxer(open_cb, encrypted_media_init_data_cb,
-                                    media_log_, true));
+    demuxer_.reset(
+        new ChunkDemuxer(open_cb, encrypted_media_init_data_cb, media_log_));
   }
 
   virtual ~ChunkDemuxerTest() {
     ShutdownDemuxer();
   }
 
   void CreateInitSegment(int stream_flags,
                          bool is_audio_encrypted,
                          bool is_video_encrypted,
                          std::unique_ptr<uint8_t[]>* buffer,
@@ skipping 672 matching lines @@
     // Note: Offsets here and below are derived from
     // media/test/data/bear-640x360-manifest.js and
     // media/test/data/bear-320x240-manifest.js which were
     // generated from media/test/data/bear-640x360.webm and
     // media/test/data/bear-320x240.webm respectively.
     EXPECT_CALL(*this, InitSegmentReceivedMock(_));
     EXPECT_TRUE(AppendData(bear2->data(), 4340));
 
     // Append a media segment that goes from [0.527000, 1.014000).
     EXPECT_MEDIA_LOG(WebMSimpleBlockDurationEstimated(2));
-    EXPECT_MEDIA_LOG(GeneratedSplice(20000, 527000));
+    EXPECT_MEDIA_LOG(TrimmedSpliceOverlap(527000, 524000, 20000));
     EXPECT_TRUE(AppendData(bear2->data() + 55290, 18785));
     CheckExpectedRanges("{ [0,1027) [1201,2736) }");
 
     // Append initialization segment for bear1 & fill gap with [779-1197)
     // segment.
     EXPECT_CALL(*this, InitSegmentReceivedMock(_));
     EXPECT_TRUE(AppendData(bear1->data(), 4370));
     EXPECT_MEDIA_LOG(WebMSimpleBlockDurationEstimated(23));
-    EXPECT_MEDIA_LOG(GeneratedSplice(26000, 779000));
+    EXPECT_MEDIA_LOG(TrimmedSpliceOverlap(779000, 759000, 3000));

[wolenetz, 2016/09/22 23:48:37]

Why is this 3000 not 26000?

[chcunningham, 2016/10/27 23:36:06]

First, let me list some numbers...

A) splice time: 779000
B) start time of pre_splice_buffers->back() used in original calculation: 782000
C) start time of splice overlapped buffer: 759000
D) buffer durations (same for all buffers): 23000

Now to explain. The original impl fetched more than one pre_splice_buffer and
the back of that list of buffers might have a PTS > than the splice PTS (true in
this case). Said another way, the PTS for back of the pre_splice_buffers
actually lands in the middle of the new splice buffer. The splice duration was
then calculated as B + D - A = 26000. I think this may have been a bug. The
buffers do overlap, but this does not actually correctly compute the duration of
their overlap.

Today's impl only grabs a single pre-splice buffer. The pre-splice buffer have a
PTS < the splice buffer, and its tail is overlapped by the splice buffer. Here
we compute the duration as C + D - A = 3000

In todays impl, the buffer B, with PTS > the splice buffer is still overlapped,
but we have no use for it, so it gets completely removed.

[wolenetz, 2016/10/28 23:08:18]

Acknowledged. The change in expectations makes sense. I believe we previously
had to have a > 3ms overlap (kCrossfadeDurationInMilliseconds = 5ms) to do the
splice, hence we had previously kept B in pre-splice buffers. The net overlap of
pre-splice and post-splice previously was indeed 26ms in this case, so on the
surface, that doesn't look like a previous bug. Regardless, new expectation in
this CL of *no crossfade* SGTM :)

     EXPECT_TRUE(AppendData(bear1->data() + 72737, 28183));
     CheckExpectedRanges("{ [0,2736) }");
 
     MarkEndOfStream(PIPELINE_OK);
     return true;
   }
 
   void ShutdownDemuxer() {
     if (demuxer_) {
       demuxer_->Shutdown();
@@ skipping 838 matching lines @@
                        base::TimeDelta::FromMilliseconds(0),
                        &video_read_done));
 
   EXPECT_TRUE(audio_read_done);
   EXPECT_TRUE(video_read_done);
 }
 
 TEST_F(ChunkDemuxerTest, OutOfOrderClusters) {
   ASSERT_TRUE(InitDemuxer(HAS_AUDIO | HAS_VIDEO));
   ASSERT_TRUE(AppendCluster(kDefaultFirstCluster()));
-  EXPECT_MEDIA_LOG(GeneratedSplice(13000, 10000));
+  EXPECT_MEDIA_LOG(TrimmedSpliceOverlap(10000, 0, 13000));
   ASSERT_TRUE(AppendCluster(GenerateCluster(10, 4)));
 
   // Make sure that AppendCluster() does not fail with a cluster that has
   // overlaps with the previously appended cluster.
-  EXPECT_MEDIA_LOG(SkippingSpliceAlreadySpliced(0));
+  EXPECT_MEDIA_LOG(TrimmedSpliceOverlap(5000, 0, 5000));

[wolenetz, 2016/09/22 23:48:37]

To be clear, this trims an *additional* 5ms from the original 23ms frame which
previously had 10ms trimmed from it, right? Or does this change to just
discarding 5ms from the original 23ms (which would be incorrect)?

To assist review/maintainability, can you read back the resulting buffers and
verify their durations/discard amounts after the next line or at end of this
unit test?

e.g, at end of this test, I would expect the buffers to be:
Audio: [0,5)[5,28)[28,45)[45,68)
Video: [0,5)[5,38)[38,45)[45,78) ** Note that the video case is underspecified
in the spec. I've filed https://github.com/w3c/media-source/issues/166 to track
clarifying that (and Chrome only adjusts overlapped video frame durations if the
overlapped frame had an estimated duration. Not very intuitive, and probably
leading to other problems.)

If we have this verification already in SourceBufferStreamTest, then maybe point
to such test(s) from this one? (or add it over there and point to it from this
test).

[chcunningham, 2016/10/27 23:36:06]

Added buffer times for readability. Did not add duration verification - instead
referenced SourceBufferStream tests that already do this.

[wolenetz, 2016/10/28 23:08:18]

Ack. Thanks for the additional verification and reference to the SBSTest that
verifies trimmed audio durations on overlap splice.

   ASSERT_TRUE(AppendCluster(GenerateCluster(5, 4)));
 
   // Verify that AppendData() can still accept more data.
   std::unique_ptr<Cluster> cluster_c(GenerateCluster(45, 2));
-  EXPECT_MEDIA_LOG(GeneratedSplice(6000, 45000));
+  EXPECT_MEDIA_LOG(TrimmedSpliceOverlap(45000, 28000, 6000));
   ASSERT_TRUE(demuxer_->AppendData(
       kSourceId, cluster_c->data(), cluster_c->size(),
       append_window_start_for_next_append_, append_window_end_for_next_append_,
       &timestamp_offset_map_[kSourceId]));
 }
 
 TEST_F(ChunkDemuxerTest, NonMonotonicButAboveClusterTimecode) {
   ASSERT_TRUE(InitDemuxer(HAS_AUDIO | HAS_VIDEO));
   ASSERT_TRUE(AppendCluster(kDefaultFirstCluster()));
 
@@ skipping 312 matching lines @@
   // affect the expected ranges.
   CheckExpectedRanges("{ [0,66) }");
 
   // Unmark end of stream state and verify that the ranges return to
   // their pre-"end of stream" values.
   demuxer_->UnmarkEndOfStream();
   CheckExpectedRanges("{ [0,46) }");
 
   // Add text track data and verify that the buffered ranges don't change
   // since the intersection of all the tracks doesn't change.
-  EXPECT_MEDIA_LOG(SkippingSpliceAtOrBefore(0, 0));
   EXPECT_CALL(host_, SetDuration(base::TimeDelta::FromMilliseconds(200)));
   AppendMuxedCluster(MuxedStreamInfo(kVideoTrackNum, "0K 33", 33),
                      MuxedStreamInfo(kAudioTrackNum, "0K 23K", 23),
                      MuxedStreamInfo(kTextTrackNum, "0K 100K"));
   CheckExpectedRanges("{ [0,46) }");
 
   // Mark end of stream and verify that text track data is reflected in
   // the new range.
   MarkEndOfStream(PIPELINE_OK);
   CheckExpectedRanges("{ [0,200) }");
@@ skipping 689 matching lines @@
       audio_id, GenerateSingleStreamCluster(900, 970, kAudioTrackNum, 70)));
   ASSERT_TRUE(AppendCluster(
       video_id, GenerateSingleStreamCluster(920, 950, kVideoTrackNum, 30)));
   CheckExpectedRanges(DemuxerStream::AUDIO,
                       "{ [0,23) [300,400) [520,590) [720,750) [900,970) }");
   CheckExpectedRanges(DemuxerStream::VIDEO,
                       "{ [0,33) [320,420) [500,570) [700,770) [920,950) }");
   CheckExpectedRangesForMediaSource(
       "{ [0,23) [320,400) [520,570) [720,750) [920,950) }");
 
-  // Appending within buffered range should not affect buffered ranges.
-  EXPECT_MEDIA_LOG(GeneratedSplice(40000, 930000));
+  // Audio buffered ranges are trimmed from 970 to 950 due to splicing the
+  // previously buffered audio frame
+  // - existing frame trimmed from [900, 970) to [900-930),

[wolenetz, 2016/09/26 23:52:37]

nit: consistent [900,930)

[chcunningham, 2016/10/27 23:36:06]

Done.

[wolenetz, 2016/10/28 23:08:18]

Not done here actually :) (though done in at least one other place).
This is a really small nit though.

[chcunningham, 2016/11/02 01:28:42]

Done.

+  // - newly appended audio from [930, 950).
+  EXPECT_MEDIA_LOG(TrimmedSpliceOverlap(930000, 900000, 40000));
   ASSERT_TRUE(AppendCluster(
       audio_id, GenerateSingleStreamCluster(930, 950, kAudioTrackNum, 20)));
+  CheckExpectedRanges(DemuxerStream::AUDIO,
+                      "{ [0,23) [300,400) [520,590) [720,750) [900,950) }");
+
+  // Appending within buffered range should not affect video buffered ranges.

[wolenetz, 2016/09/26 23:52:37]

nit: This isn't always true. But it is for the particular append done here.
Rephrase?

[chcunningham, 2016/10/27 23:36:06]

Done.

   ASSERT_TRUE(AppendCluster(
       video_id, GenerateSingleStreamCluster(930, 950, kVideoTrackNum, 20)));
-  CheckExpectedRanges(DemuxerStream::AUDIO,
-                      "{ [0,23) [300,400) [520,590) [720,750) [900,970) }");

[wolenetz, 2016/09/26 23:52:37]

Wow, this appears to have been broken before. Why wasn't the last range
previously also [900,950) before this CL?

[wolenetz, 2016/09/27 00:35:35]

Offline, yes looks like an artifact before (crossfade duration kept buffered
range duration longer?)

[chcunningham, 2016/10/27 23:36:06]

Right, not sure if this is a bug originally (much of the orignal splicing code
is difficult to grok), but when converting the new appended buffer to a splice
buffer (passing in the pre-splice buffers), we used the duration of the
pre-splice buffers if it exceeds the overlap duration (which happens in this
case since overlap = 20, pre-splice-duration=70). 

https://cs.chromium.org/chromium/src/media/base/stream_parser_buffer.cc?rcl=1...

I look forward to deleting this code

[wolenetz, 2016/10/28 23:08:18]

Acknowledged.

   CheckExpectedRanges(DemuxerStream::VIDEO,
                       "{ [0,33) [320,420) [500,570) [700,770) [920,950) }");
+
   CheckExpectedRangesForMediaSource(
       "{ [0,23) [320,400) [520,570) [720,750) [920,950) }");
 }
 
 TEST_F(ChunkDemuxerTest, GetBufferedRanges_AudioVideo) {
   ASSERT_TRUE(InitDemuxer(HAS_AUDIO | HAS_VIDEO));
 
   // Audio block: 0 -> 23
   // Video block: 0 -> 33
   // Buffered Range: 0 -> 23
@@ skipping 51 matching lines @@
   // 900, so the added buffered range is 900 -> 950
   AppendMuxedCluster(MuxedStreamInfo(kAudioTrackNum, "900D70K"),
                      MuxedStreamInfo(kVideoTrackNum, "920D30K"));
 
   CheckExpectedRanges(DemuxerStream::AUDIO,
                       "{ [0,23) [300,400) [500,590) [700,750) [900,970) }");
   CheckExpectedRanges(DemuxerStream::VIDEO,
                       "{ [0,33) [300,420) [500,570) [700,770) [900,950) }");
   CheckExpectedRanges("{ [0,23) [300,400) [500,570) [700,750) [900,950) }");
 
-  // Appending within buffered range should not affect buffered ranges.
-  EXPECT_MEDIA_LOG(GeneratedSplice(40000, 930000));
+  // Appending within buffered range should not affect video buffered ranges.

[wolenetz, 2016/09/26 23:52:37]

nit ditto: rephrase?

[chcunningham, 2016/10/27 23:36:06]

Done.

+  EXPECT_MEDIA_LOG(TrimmedSpliceOverlap(930000, 900000, 40000));
   AppendMuxedCluster(MuxedStreamInfo(kAudioTrackNum, "930D20K"),
                      MuxedStreamInfo(kVideoTrackNum, "930D20K"));
-  CheckExpectedRanges(DemuxerStream::AUDIO,
-                      "{ [0,23) [300,400) [500,590) [700,750) [900,970) }");

[wolenetz, 2016/09/26 23:52:37]

ditto: why broken before?

[wolenetz, 2016/09/27 00:35:35]

Ditto Ack of offline.

   CheckExpectedRanges(DemuxerStream::VIDEO,
                       "{ [0,33) [300,420) [500,570) [700,770) [900,950) }");
+
+  // Audio buffered ranges are trimmed from 970 to 950 due to splicing the
+  // previously buffered audio frame.
+  // - existing frame trimmed from [900, 970) to [900-930),
+  // - newly appended audio from [930, 950).
+  CheckExpectedRanges(DemuxerStream::AUDIO,
+                      "{ [0,23) [300,400) [500,590) [700,750) [900,950) }");
+
   CheckExpectedRanges("{ [0,23) [300,400) [500,570) [700,750) [900,950) }");
 }
 
 TEST_F(ChunkDemuxerTest, GetBufferedRanges_AudioVideoText) {
   EXPECT_CALL(host_, AddTextStream(_, _));
   ASSERT_TRUE(InitDemuxer(HAS_AUDIO | HAS_VIDEO | HAS_TEXT));
 
   // Append audio & video data
   AppendMuxedCluster(MuxedStreamInfo(kAudioTrackNum, "0K 23K", 23),
                      MuxedStreamInfo(kVideoTrackNum, "0K 33", 33));
@@ skipping 319 matching lines @@
   DemuxerStream* audio = demuxer_->GetStream(DemuxerStream::AUDIO);
 
   // Fetch initial audio config and verify it matches what we expect.
   const AudioDecoderConfig& audio_config_1 = audio->audio_decoder_config();
   ASSERT_TRUE(audio_config_1.IsValidConfig());
   EXPECT_EQ(audio_config_1.samples_per_second(), 44100);
   EXPECT_EQ(audio_config_1.extra_data().size(), 3863u);
 
   ExpectRead(DemuxerStream::AUDIO, 0);
 
-  // The first config change seen is from a splice frame representing an overlap
-  // of buffer from config 1 by buffers from config 2.
+  // Read until we encounter config 2.
   ReadUntilNotOkOrEndOfStream(DemuxerStream::AUDIO, &status, &last_timestamp);
   ASSERT_EQ(status, DemuxerStream::kConfigChanged);
   EXPECT_EQ(last_timestamp.InMilliseconds(), 524);
 
   // Fetch the new decoder config.
   const AudioDecoderConfig& audio_config_2 = audio->audio_decoder_config();
   ASSERT_TRUE(audio_config_2.IsValidConfig());
   EXPECT_EQ(audio_config_2.samples_per_second(), 44100);
   EXPECT_EQ(audio_config_2.extra_data().size(), 3935u);
 
-  // The next config change is from a splice frame representing an overlap of
-  // buffers from config 2 by buffers from config 1.
+  // Read until we encounter config 1 again.
   ReadUntilNotOkOrEndOfStream(DemuxerStream::AUDIO, &status, &last_timestamp);
   ASSERT_EQ(status, DemuxerStream::kConfigChanged);
-  EXPECT_EQ(last_timestamp.InMilliseconds(), 782);
+  EXPECT_EQ(last_timestamp.InMilliseconds(), 759);

[wolenetz, 2016/09/26 23:52:37]

Is this 782 -> 759 change because the 782 buffer was needed for crossfade, but
instead we now just discard tail of 759 buffer?

[chcunningham, 2016/10/27 23:36:06]

Yes. See detailed explanation in my comments on InitDemuxerWithConfigChangeData

[wolenetz, 2016/10/28 23:08:18]

Acknowledged.

   ASSERT_TRUE(audio_config_1.Matches(audio->audio_decoder_config()));
 
   // Read until the end of the stream just to make sure there aren't any other
   // config changes.
   ReadUntilNotOkOrEndOfStream(DemuxerStream::AUDIO, &status, &last_timestamp);
   ASSERT_EQ(status, DemuxerStream::kOk);
   EXPECT_EQ(last_timestamp.InMilliseconds(), 2744);
 }
 
 TEST_F(ChunkDemuxerTest, ConfigChange_Seek) {
@@ skipping 280 matching lines @@
       true, true, true, true, false,
   };
 
   static_assert(arraysize(kBuffer) == arraysize(kExpectedReturnValues),
       "test arrays out of sync");
   static_assert(arraysize(kBuffer) == sizeof(kBuffer),
       "there should be one byte per index");
 
   ASSERT_TRUE(InitDemuxer(HAS_AUDIO));
   EXPECT_MEDIA_LOG(WebMSimpleBlockDurationEstimated(23)).Times(2);
-  EXPECT_MEDIA_LOG(GeneratedSplice(22000, 2000));
+  EXPECT_MEDIA_LOG(TrimmedSpliceOverlap(2000, 1000, 22000));
   for (size_t i = 0; i < sizeof(kBuffer); i++) {
     DVLOG(3) << "Appending and testing index " << i;
     ASSERT_TRUE(AppendData(kBuffer + i, 1));
     bool expected_return_value = kExpectedReturnValues[i];
     EXPECT_EQ(expected_return_value,
               demuxer_->IsParsingMediaSegment(kSourceId));
   }
 }
 
 TEST_F(ChunkDemuxerTest, DurationChange) {
@@ skipping 1086 matching lines @@
   ASSERT_TRUE(InitDemuxer(HAS_AUDIO | HAS_VIDEO));
   DemuxerStream* audio_stream = demuxer_->GetStream(DemuxerStream::AUDIO);
   EXPECT_NE(nullptr, audio_stream);
   CheckStreamStatusNotifications(audio_stream);
   DemuxerStream* video_stream = demuxer_->GetStream(DemuxerStream::VIDEO);
   EXPECT_NE(nullptr, video_stream);
   CheckStreamStatusNotifications(video_stream);
 }
 
 }  // namespace media