Fix unit test failures with estimated durations and splice frames.

When an append results in a splice frame where the overlapping buffer is
the start of the media segment, the new range start time should be the
start of the first pre splice buffer.

WebM duration estimation should always use the minimum duration, not
the maximum to ensure frames are not overestimated -- otherwise large
amounts of incorrect splice frames are generated.

Enables splice frame generation for all test code.

BUG=356805
TEST=media_unittests is fixed.  New unittest added for segment start.

Review URL: https://codereview.chromium.org/220103002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@264708 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 77 insertions, 49 deletions

diff --git a/media/base/audio_splicer.cc b/media/base/audio_splicer.cc
index 29649bc..5ce45f3 100644
--- a/media/base/audio_splicer.cc
+++ b/media/base/audio_splicer.cc
@@ -44,7 +44,8 @@ static void AccurateTrimStart(int frames_to_trim,
 static void AccurateTrimEnd(int frames_to_trim,
                             const scoped_refptr<AudioBuffer> buffer,
                             const AudioTimestampHelper& timestamp_helper) {
-  DCHECK(buffer->timestamp() == timestamp_helper.GetTimestamp());
+  DCHECK_LT(std::abs(timestamp_helper.GetFramesToTarget(buffer->timestamp())),
+            kMinGapSize);
   buffer->TrimEnd(frames_to_trim);
   buffer->set_duration(
       timestamp_helper.GetFrameDuration(buffer->frame_count()));
diff --git a/media/filters/chunk_demuxer_unittest.cc b/media/filters/chunk_demuxer_unittest.cc
index 9df5108..e477eee 100644
--- a/media/filters/chunk_demuxer_unittest.cc
+++ b/media/filters/chunk_demuxer_unittest.cc
@@ -172,7 +172,7 @@ class ChunkDemuxerTest : public ::testing::TestWithParam<bool> {
     Demuxer::NeedKeyCB need_key_cb =
         base::Bind(&ChunkDemuxerTest::DemuxerNeedKey, base::Unretained(this));
     demuxer_.reset(
-        new ChunkDemuxer(open_cb, need_key_cb, base::Bind(&LogFunc), false));
+        new ChunkDemuxer(open_cb, need_key_cb, base::Bind(&LogFunc), true));
   }
 
   virtual ~ChunkDemuxerTest() {
@@ -577,7 +577,7 @@ class ChunkDemuxerTest : public ::testing::TestWithParam<bool> {
     // the files are fixed to have the correct duration in their init segments,
     // and the CreateInitDoneCB() call, above, is fixed to used that duration.
     // See http://crbug.com/354284.
-    EXPECT_CALL(host_, SetDuration(base::TimeDelta::FromMilliseconds(2768)));
+    EXPECT_CALL(host_, SetDuration(base::TimeDelta::FromMilliseconds(2746)));
     AppendData(bear1->data(), bear1->data_size());
     // Last audio frame has timestamp 2721 and duration 24 (estimated from max
     // seen so far for audio track).
@@ -1671,7 +1671,7 @@ TEST_P(ChunkDemuxerTest, WebMFile_AudioAndVideo) {
   // TODO(wolenetz/acolwell): Remove this SetDuration expectation and update the
   // ParseWebMFile() call's expected duration, below, once the file is fixed to
   // have the correct duration in the init segment. See http://crbug.com/354284.
-  EXPECT_CALL(host_, SetDuration(base::TimeDelta::FromMilliseconds(2768)));
+  EXPECT_CALL(host_, SetDuration(base::TimeDelta::FromMilliseconds(2746)));
 
   ASSERT_TRUE(ParseWebMFile("bear-320x240.webm", buffer_timestamps,
                             base::TimeDelta::FromMilliseconds(2744)));
@@ -1704,7 +1704,7 @@ TEST_P(ChunkDemuxerTest, WebMFile_AudioOnly) {
   // TODO(wolenetz/acolwell): Remove this SetDuration expectation and update the
   // ParseWebMFile() call's expected duration, below, once the file is fixed to
   // have the correct duration in the init segment. See http://crbug.com/354284.
-  EXPECT_CALL(host_, SetDuration(base::TimeDelta::FromMilliseconds(2768)));
+  EXPECT_CALL(host_, SetDuration(base::TimeDelta::FromMilliseconds(2746)));
 
   ASSERT_TRUE(ParseWebMFile("bear-320x240-audio-only.webm", buffer_timestamps,
                             base::TimeDelta::FromMilliseconds(2744),
@@ -1741,11 +1741,6 @@ TEST_P(ChunkDemuxerTest, WebMFile_AltRefFrames) {
     {kSkip, kSkip},
   };
 
-  // TODO(wolenetz/acolwell): Remove this SetDuration expectation and update the
-  // ParseWebMFile() call's expected duration, below, once the file is fixed to
-  // have the correct duration in the init segment. See http://crbug.com/354284.
-  EXPECT_CALL(host_, SetDuration(base::TimeDelta::FromMilliseconds(2768)));
-
   ASSERT_TRUE(ParseWebMFile("bear-320x240-altref.webm", buffer_timestamps,
                             base::TimeDelta::FromMilliseconds(2767)));
 }
@@ -2294,14 +2289,14 @@ TEST_P(ChunkDemuxerTest, GetBufferedRanges_EndOfStream) {
   // Append and remove data so that the 2 streams' end ranges do not overlap.
 
   EXPECT_CALL(host_, SetDuration(base::TimeDelta::FromMilliseconds(246)));
-  EXPECT_CALL(host_, SetDuration(base::TimeDelta::FromMilliseconds(366)));
+  EXPECT_CALL(host_, SetDuration(base::TimeDelta::FromMilliseconds(398)));
   AppendSingleStreamCluster(kSourceId, kAudioTrackNum, "200K 223K");
   AppendSingleStreamCluster(kSourceId, kVideoTrackNum,
-                            "200K 233 266 299 300K 333");
+                            "200K 233 266 299 332K 365");
 
   // At this point, the per-stream ranges are as follows:
   // Audio: [0,46) [200,246)
-  // Video: [0,66) [200,366)
+  // Video: [0,66) [200,398)
   CheckExpectedRanges("{ [0,46) [200,246) }");
 
   demuxer_->Remove(kSourceId, base::TimeDelta::FromMilliseconds(200),
@@ -2309,7 +2304,7 @@ TEST_P(ChunkDemuxerTest, GetBufferedRanges_EndOfStream) {
 
   // At this point, the per-stream ranges are as follows:
   // Audio: [0,46)
-  // Video: [0,66) [300,366)
+  // Video: [0,66) [332,398)
   CheckExpectedRanges("{ [0,46) }");
 
   AppendSingleStreamCluster(kSourceId, kAudioTrackNum, "200K 223K");
@@ -2317,7 +2312,7 @@ TEST_P(ChunkDemuxerTest, GetBufferedRanges_EndOfStream) {
 
   // At this point, the per-stream ranges are as follows:
   // Audio: [0,46) [200,246)
-  // Video: [0,66) [200,266) [300,366)
+  // Video: [0,66) [200,266) [332,398)
   // NOTE: The last range on each stream do not overlap in time.
   CheckExpectedRanges("{ [0,46) [200,246) }");
 
@@ -2325,9 +2320,9 @@ TEST_P(ChunkDemuxerTest, GetBufferedRanges_EndOfStream) {
 
   // NOTE: The last range on each stream gets extended to the highest
   // end timestamp according to the spec. The last audio range gets extended
-  // from [200,246) to [200,366) which is why the intersection results in the
+  // from [200,246) to [200,398) which is why the intersection results in the
   // middle range getting larger AND the new range appearing.
-  CheckExpectedRanges("{ [0,46) [200,266) [300,366) }");
+  CheckExpectedRanges("{ [0,46) [200,266) [332,398) }");
 }
 
 TEST_P(ChunkDemuxerTest, DifferentStreamTimecodes) {
@@ -2583,10 +2578,17 @@ TEST_P(ChunkDemuxerTest, ConfigChange_Audio) {
 
   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.
   ReadUntilNotOkOrEndOfStream(DemuxerStream::AUDIO, &status, &last_timestamp);
-
   ASSERT_EQ(status, DemuxerStream::kConfigChanged);
   EXPECT_EQ(last_timestamp.InMilliseconds(), 524);
+  ASSERT_TRUE(audio_config_1.Matches(audio->audio_decoder_config()));
+
+  // The next is due to a typical config difference.
+  ReadUntilNotOkOrEndOfStream(DemuxerStream::AUDIO, &status, &last_timestamp);
+  ASSERT_EQ(status, DemuxerStream::kConfigChanged);
+  EXPECT_EQ(last_timestamp.InMilliseconds(), 527);
 
   // Fetch the new decoder config.
   const AudioDecoderConfig& audio_config_2 = audio->audio_decoder_config();
@@ -2594,22 +2596,26 @@ TEST_P(ChunkDemuxerTest, ConfigChange_Audio) {
   EXPECT_EQ(audio_config_2.samples_per_second(), 44100);
   EXPECT_EQ(audio_config_2.extra_data_size(), 3935u);
 
-  ExpectRead(DemuxerStream::AUDIO, 527);
+  // The next config change is from a splice frame representing an overlap of
+  // buffers from config 2 by buffers from config 1.
+  ReadUntilNotOkOrEndOfStream(DemuxerStream::AUDIO, &status, &last_timestamp);
+  ASSERT_EQ(status, DemuxerStream::kConfigChanged);
+  EXPECT_EQ(last_timestamp.InMilliseconds(), 782);
+  ASSERT_TRUE(audio_config_2.Matches(audio->audio_decoder_config()));
 
-  // Read until the next config change.
   ReadUntilNotOkOrEndOfStream(DemuxerStream::AUDIO, &status, &last_timestamp);
+
   ASSERT_EQ(status, DemuxerStream::kConfigChanged);
-  EXPECT_EQ(last_timestamp.InMilliseconds(), 759);
+  EXPECT_EQ(last_timestamp.InMilliseconds(), 779);
 
   // Get the new config and verify that it matches the first one.
   ASSERT_TRUE(audio_config_1.Matches(audio->audio_decoder_config()));
 
-  ExpectRead(DemuxerStream::AUDIO, 779);
-
   // 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_P(ChunkDemuxerTest, ConfigChange_Seek) {
diff --git a/media/filters/pipeline_integration_test.cc b/media/filters/pipeline_integration_test.cc
index 133c497..c3a3c44b 100644
--- a/media/filters/pipeline_integration_test.cc
+++ b/media/filters/pipeline_integration_test.cc
@@ -62,10 +62,10 @@ const int kAppendWholeFile = -1;
 const int kAppendTimeSec = 1;
 const int kAppendTimeMs = kAppendTimeSec * 1000;
 const int k320WebMFileDurationMs = 2736;
-const int k640WebMFileDurationMs = 2762;
+const int k640WebMFileDurationMs = 2749;
 const int kOpusEndTrimmingWebMFileDurationMs = 2771;
 const int kVP9WebMFileDurationMs = 2736;
-const int kVP8AWebMFileDurationMs = 2734;
+const int kVP8AWebMFileDurationMs = 2733;
 
 #if defined(USE_PROPRIETARY_CODECS)
 const int k640IsoFileDurationMs = 2737;
@@ -298,7 +298,7 @@ class MockMediaSource {
             base::Bind(&MockMediaSource::DemuxerNeedKey,
                        base::Unretained(this)),
             LogCB(),
-            false)),
+            true)),
         owned_chunk_demuxer_(chunk_demuxer_),
         use_legacy_frame_processor_(use_legacy_frame_processor) {
 
@@ -663,11 +663,7 @@ TEST_P(PipelineIntegrationTest, BasicPlayback_MediaSource_Opus_WebM) {
 
   EXPECT_EQ(1u, pipeline_->GetBufferedTimeRanges().size());
   EXPECT_EQ(0, pipeline_->GetBufferedTimeRanges().start(0).InMilliseconds());
-  // TODO(acolwell/wolenetz): Drop the "+ 1" once WebM stream parser always
-  // emits frames with valid durations (see http://crbug.com/351166) and
-  // compliant coded frame processor's "highest presentation end timestamp" is
-  // used to update duration (see http://crbug.com/249422).
-  EXPECT_EQ(kOpusEndTrimmingWebMFileDurationMs + 1,
+  EXPECT_EQ(kOpusEndTrimmingWebMFileDurationMs,
             pipeline_->GetBufferedTimeRanges().end(0).InMilliseconds());
   Play();
 
@@ -684,11 +680,7 @@ TEST_P(PipelineIntegrationTest, DISABLED_MediaSource_Opus_Seeking_WebM) {
 
   EXPECT_EQ(1u, pipeline_->GetBufferedTimeRanges().size());
   EXPECT_EQ(0, pipeline_->GetBufferedTimeRanges().start(0).InMilliseconds());
-  // TODO(acolwell/wolenetz): Drop the "+ 1" once WebM stream parser always
-  // emits frames with valid durations (see http://crbug.com/351166) and
-  // compliant coded frame processor's "highest presentation end timestamp" is
-  // used to update duration (see http://crbug.com/249422).
-  EXPECT_EQ(kOpusEndTrimmingWebMFileDurationMs + 1,
+  EXPECT_EQ(kOpusEndTrimmingWebMFileDurationMs,
             pipeline_->GetBufferedTimeRanges().end(0).InMilliseconds());
 
   base::TimeDelta start_seek_time = base::TimeDelta::FromMilliseconds(1000);
@@ -749,10 +741,7 @@ TEST_P(PipelineIntegrationTest, MediaSource_ConfigChange_Encrypted_WebM) {
 
   EXPECT_EQ(1u, pipeline_->GetBufferedTimeRanges().size());
   EXPECT_EQ(0, pipeline_->GetBufferedTimeRanges().start(0).InMilliseconds());
-  // The "+ 1" is due to estimated audio and video frame durations on the last
-  // frames appended. The unencrypted file has a TrackEntry DefaultDuration
-  // field for the video track, but the encrypted file does not.
-  EXPECT_EQ(kAppendTimeMs + k640WebMFileDurationMs + 1,
+  EXPECT_EQ(kAppendTimeMs + k640WebMFileDurationMs,
             pipeline_->GetBufferedTimeRanges().end(0).InMilliseconds());
 
   Play();
@@ -812,10 +801,7 @@ TEST_P(PipelineIntegrationTest,
   EXPECT_EQ(1u, pipeline_->GetBufferedTimeRanges().size());
   EXPECT_EQ(0, pipeline_->GetBufferedTimeRanges().start(0).InMilliseconds());
   // The second video was not added, so its time has not been added.
-  // The "+ 1" is due to estimated audio and video frame durations on the last
-  // frames appended. The unencrypted file has a TrackEntry DefaultDuration
-  // field for the video track, but the encrypted file does not.
-  EXPECT_EQ(k320WebMFileDurationMs + 1,
+  EXPECT_EQ(k320WebMFileDurationMs,
             pipeline_->GetBufferedTimeRanges().end(0).InMilliseconds());
 
   Play();
diff --git a/media/filters/source_buffer_stream.cc b/media/filters/source_buffer_stream.cc
index 4b892d7..09a3145 100644
--- a/media/filters/source_buffer_stream.cc
+++ b/media/filters/source_buffer_stream.cc
@@ -490,7 +490,8 @@ bool SourceBufferStream::Append(const BufferQueue& buffers) {
     last_appended_buffer_timestamp_ = buffers.back()->GetDecodeTimestamp();
     last_appended_buffer_is_keyframe_ = buffers.back()->IsKeyframe();
   } else {
-    base::TimeDelta new_range_start_time = media_segment_start_time_;
+    base::TimeDelta new_range_start_time = std::min(
+        media_segment_start_time_, buffers.front()->GetDecodeTimestamp());
     const BufferQueue* buffers_for_new_range = &buffers;
     BufferQueue trimmed_buffers;
 
@@ -988,7 +989,14 @@ void SourceBufferStream::PrepareRangesForNextAppend(
   // 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.
-  bool is_exclusive = (prev_timestamp == next_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.
+  DCHECK(new_buffers.front()->splice_timestamp() !=
+         new_buffers.front()->timestamp());
+  const bool is_exclusive =
+      new_buffers.front()->get_splice_buffers().empty() &&
+      prev_timestamp == next_timestamp &&
       AllowSameTimestamp(prev_is_keyframe, next_is_keyframe, GetType());
 
   // Delete the buffers that |new_buffers| overlaps.
@@ -1677,8 +1685,12 @@ SourceBufferRange::SourceBufferRange(
 
 void SourceBufferRange::AppendBuffersToEnd(const BufferQueue& new_buffers) {
   DCHECK(buffers_.empty() || CanAppendBuffersToEnd(new_buffers));
+  DCHECK(media_segment_start_time_ == kNoTimestamp() ||
+         media_segment_start_time_ <=
+             new_buffers.front()->GetDecodeTimestamp());
   for (BufferQueue::const_iterator itr = new_buffers.begin();
-       itr != new_buffers.end(); ++itr) {
+       itr != new_buffers.end();
+       ++itr) {
     DCHECK((*itr)->GetDecodeTimestamp() != kNoTimestamp());
     buffers_.push_back(*itr);
     size_in_bytes_ += (*itr)->data_size();
diff --git a/media/filters/source_buffer_stream_unittest.cc b/media/filters/source_buffer_stream_unittest.cc
index db54a41..ccd4f58 100644
--- a/media/filters/source_buffer_stream_unittest.cc
+++ b/media/filters/source_buffer_stream_unittest.cc
@@ -3691,6 +3691,19 @@ TEST_F(SourceBufferStreamTest, Audio_SpliceFrame_NoSplice) {
   CheckNoNextBuffer();
 }
 
+TEST_F(SourceBufferStreamTest, Audio_SpliceFrame_CorrectMediaSegmentStartTime) {
+  SetAudioStream();
+  Seek(0);
+  NewSegmentAppend("0K 2K 4K");
+  CheckExpectedRangesByTimestamp("{ [0,6) }");
+  NewSegmentAppend("6K 8K 10K");
+  CheckExpectedRangesByTimestamp("{ [0,12) }");
+  NewSegmentAppend("1K 4K");
+  CheckExpectedRangesByTimestamp("{ [0,12) }");
+  CheckExpectedBuffers("0K 2K 4K C 1K 4K 6K 8K 10K");
+  CheckNoNextBuffer();
+}
+
 // TODO(vrk): Add unit tests where keyframes are unaligned between streams.
 // (crbug.com/133557)
 
diff --git a/media/formats/webm/webm_cluster_parser.cc b/media/formats/webm/webm_cluster_parser.cc
index 5530815..5a3adc3 100644
--- a/media/formats/webm/webm_cluster_parser.cc
+++ b/media/formats/webm/webm_cluster_parser.cc
@@ -532,8 +532,18 @@ bool WebMClusterParser::Track::QueueBuffer(
     return false;
   }
 
-  estimated_next_frame_duration_ = std::max(duration,
-                                            estimated_next_frame_duration_);
+  // The estimated frame duration is the minimum non-zero duration since the
+  // last initialization segment.  The minimum is used to ensure frame durations
+  // aren't overestimated.
+  if (duration > base::TimeDelta()) {
+    if (estimated_next_frame_duration_ == kNoTimestamp()) {
+      estimated_next_frame_duration_ = duration;
+    } else {
+      estimated_next_frame_duration_ =
+          std::min(duration, estimated_next_frame_duration_);
+    }
+  }
+
   buffers_.push_back(buffer);
   return true;
 }