Fix incorrect opus seek preroll and flaky pre-skip removal.

media/test/pipeline_integration_test.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 "base/bind.h"
 #include "base/command_line.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/stl_util.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
@@ skipping 87 matching lines @@
 const int kAppendTimeMs = kAppendTimeSec * 1000;
 const int k320WebMFileDurationMs = 2736;
 #if !defined(DISABLE_EME_TESTS)
 const int k320EncWebMFileDurationMs = 2737;
 #endif  // !defined(DISABLE_EME_TESTS)
 const int k640WebMFileDurationMs = 2749;
 const int kOpusEndTrimmingWebMFileDurationMs = 2741;
 const int kVP9WebMFileDurationMs = 2736;
 const int kVP8AWebMFileDurationMs = 2734;
 
+// Hash for a full playthrough of "opus-trimming-test.(webm|ogg)".
+static const char kOpusEndTrimmingHash_1[] =
+    "-4.56,-5.65,-6.51,-6.29,-4.36,-3.59,";
+// The above hash, plus an additional playthrough starting from T=1s.
+static const char kOpusEndTrimmingHash_2[] =
+    "-11.89,-11.09,-8.25,-7.11,-7.84,-9.97,";
+// The above hash, plus an additional playthrough starting from T=6.36s.

[chcunningham, 2015/07/31 19:48:45]

Which "above" do you mean here? Could make this more clear if you ref the
variable name of the "above"

[DaleCurtis, 2015/08/03 22:49:53]

The one immediately above :) I dislike putting variable names in comments when
possible because they may change and it feels a bit redundant. Unless you
strongly prefer, I'm going to leave it as is.

+static const char kOpusEndTrimmingHash_3[] =
+    "-13.28,-14.35,-13.67,-11.68,-10.18,-10.46,";
+
 #if defined(USE_PROPRIETARY_CODECS)
 #if !defined(DISABLE_EME_TESTS)
 const int k640IsoFileDurationMs = 2737;
 const int k640IsoCencFileDurationMs = 2736;
 #endif  // !defined(DISABLE_EME_TESTS)
 const int k1280IsoFileDurationMs = 2736;
 const int k1280IsoAVC3FileDurationMs = 2736;
 #endif  // defined(USE_PROPRIETARY_CODECS)
 
 // Return a timeline offset for bear-320x240-live.webm.
@@ skipping 362 matching lines @@
         kSourceId,
         base::TimeDelta(), kInfiniteDuration(), &last_timestamp_offset_);
 
     DCHECK_GE(new_position, 0);
     DCHECK_LT(new_position, file_data_->data_size());
     current_position_ = new_position;
 
     AppendData(seek_append_size);
   }
 
+  void Seek(base::TimeDelta seek_time) {
+    chunk_demuxer_->StartWaitingForSeek(seek_time);
+    chunk_demuxer_->Abort(

[chcunningham, 2015/07/31 19:48:45]

Noob questions: I expected to see

chunk_demuxer->StartWaitingForSeek(...)
chunk_demuxer->Seek(...)

I don't follow why we abort, nor why we don't actually call seek?

[DaleCurtis, 2015/08/03 22:49:53]

I don't know either, I just copied the Seek(a,b,c) method on l.486. If you have
suggestions which would work better let me know.

[chcunningham, 2015/08/04 20:38:09]

wolenetz@, do you know more on this?

[wolenetz, 2015/08/06 22:21:27]

Good catch! tl;dr, drop the Abort() from this new Seek() helper method.

The l.486 Seek helper actually appends some data, too. This explains why the
Abort() is done there. Perhaps that helper should be renamed to something like
SeekDemuxerAndAppend, or otherwise refactored to extract the abort+append out
into some other abstraction.

Actual Demuxer::Seek() is done by pipeline_->Seek() in these tests. The helper
in l.486 just prepares the demuxer for an upcoming seek (and has crufty
side-effect of aborting any previous append/remove algorithm and appending some
more data). This new helper just prepares the demuxer for the upcoming seek and
includes no abort+append side-effects.

[DaleCurtis, 2015/08/06 23:45:27]

Hmm, didn't work when I tried this originally, but does now! Done.

+        kSourceId,
+        base::TimeDelta(), kInfiniteDuration(), &last_timestamp_offset_);
+  }
+
   void AppendData(int size) {
     DCHECK(chunk_demuxer_);
     DCHECK_LT(current_position_, file_data_->data_size());
     DCHECK_LE(current_position_ + size, file_data_->data_size());
 
     chunk_demuxer_->AppendData(
         kSourceId, file_data_->data() + current_position_, size,
         base::TimeDelta(), kInfiniteDuration(), &last_timestamp_offset_,
         base::Bind(&MockMediaSource::InitSegmentReceived,
                    base::Unretained(this)));
@@ skipping 170 matching lines @@
                    base::Unretained(this)));
     message_loop_.Run();
     EXPECT_EQ(PIPELINE_OK, pipeline_status_);
   }
 
   void StartHashedPipelineWithMediaSource(MockMediaSource* source) {
     hashing_enabled_ = true;
     StartPipelineWithMediaSource(source);
   }
 
+  void StartHashedClocklessPipelineWithMediaSource(MockMediaSource* source) {
+    hashing_enabled_ = true;
+    clockless_playback_ = true;
+    StartPipelineWithMediaSource(source);
+  }
+
   void StartPipelineWithEncryptedMedia(
       MockMediaSource* source,
       FakeEncryptedMedia* encrypted_media) {
     EXPECT_CALL(*source, InitSegmentReceived()).Times(AtLeast(1));
     EXPECT_CALL(*this, OnMetadata(_))
         .Times(AtMost(1))
         .WillRepeatedly(SaveArg<0>(&metadata_));
     EXPECT_CALL(*this, OnBufferingStateChanged(BUFFERING_HAVE_ENOUGH))
         .Times(AnyNumber());
     EXPECT_CALL(*this, OnBufferingStateChanged(BUFFERING_HAVE_NOTHING))
@@ skipping 86 matching lines @@
 
   Play();
 
   ASSERT_TRUE(WaitUntilOnEnded());
 
   EXPECT_HASH_EQ("f0be120a90a811506777c99a2cdf7cc1", GetVideoHash());
   EXPECT_HASH_EQ("-3.59,-2.06,-0.43,2.15,0.77,-0.95,", GetAudioHash());
   EXPECT_TRUE(demuxer_->GetTimelineOffset().is_null());
 }
 
+TEST_F(PipelineIntegrationTest, BasicPlaybackOpusOggTrimmingHashed) {
+  ASSERT_EQ(PIPELINE_OK,
+            Start("opus-trimming-test.webm", kHashed | kClockless));
+
+  Play();
+
+  ASSERT_TRUE(WaitUntilOnEnded());
+  EXPECT_HASH_EQ(kOpusEndTrimmingHash_1, GetAudioHash());
+
+  // Seek within the pre-skip section, this should not cause a beep.
+  ASSERT_TRUE(Seek(base::TimeDelta::FromSeconds(1)));
+  Play();
+  ASSERT_TRUE(WaitUntilOnEnded());
+  EXPECT_HASH_EQ(kOpusEndTrimmingHash_2, GetAudioHash());
+
+  // Seek somewhere outside of the pre-skip / end-trim section, demxuer should
+  // correctly preroll enough to accurately decode this segment.
+  ASSERT_TRUE(Seek(base::TimeDelta::FromMilliseconds(6360)));
+  Play();
+  ASSERT_TRUE(WaitUntilOnEnded());
+  EXPECT_HASH_EQ(kOpusEndTrimmingHash_3, GetAudioHash());
+}
+
+TEST_F(PipelineIntegrationTest, BasicPlaybackOpusWebmTrimmingHashed) {
+  ASSERT_EQ(PIPELINE_OK,
+            Start("opus-trimming-test.webm", kHashed | kClockless));
+
+  Play();
+
+  ASSERT_TRUE(WaitUntilOnEnded());
+  EXPECT_HASH_EQ(kOpusEndTrimmingHash_1, GetAudioHash());
+
+  // Seek within the pre-skip section, this should not cause a beep.
+  ASSERT_TRUE(Seek(base::TimeDelta::FromSeconds(1)));
+  Play();
+  ASSERT_TRUE(WaitUntilOnEnded());
+  EXPECT_HASH_EQ(kOpusEndTrimmingHash_2, GetAudioHash());
+
+  // Seek somewhere outside of the pre-skip / end-trim section, demxuer should
+  // correctly preroll enough to accurately decode this segment.
+  ASSERT_TRUE(Seek(base::TimeDelta::FromMilliseconds(6360)));
+  Play();
+  ASSERT_TRUE(WaitUntilOnEnded());
+  EXPECT_HASH_EQ(kOpusEndTrimmingHash_3, GetAudioHash());
+}
+
+TEST_F(PipelineIntegrationTest,
+       BasicPlaybackOpusWebmTrimmingHashed_MediaSource) {
+  MockMediaSource source("opus-trimming-test.webm", kOpusAudioOnlyWebM,
+                         kAppendWholeFile);
+  StartHashedClocklessPipelineWithMediaSource(&source);
+  source.EndOfStream();
+
+  Play();
+
+  ASSERT_TRUE(WaitUntilOnEnded());
+  EXPECT_HASH_EQ(kOpusEndTrimmingHash_1, GetAudioHash());
+
+  // Seek within the pre-skip section, this should not cause a beep.
+  base::TimeDelta seek_time = base::TimeDelta::FromSeconds(1);
+  source.Seek(seek_time);
+  ASSERT_TRUE(Seek(seek_time));
+  Play();
+  ASSERT_TRUE(WaitUntilOnEnded());
+  EXPECT_HASH_EQ(kOpusEndTrimmingHash_2, GetAudioHash());
+
+  // Seek somewhere outside of the pre-skip / end-trim section, demxuer should

[wolenetz, 2015/08/06 22:21:27]

nit: demuxer spellcheck

[DaleCurtis, 2015/08/06 23:45:27]

Done.

+  // correctly preroll enough to accurately decode this segment.
+  seek_time = base::TimeDelta::FromMilliseconds(6360);
+  source.Seek(seek_time);
+  ASSERT_TRUE(Seek(seek_time));
+  Play();
+  ASSERT_TRUE(WaitUntilOnEnded());
+  EXPECT_HASH_EQ(kOpusEndTrimmingHash_3, GetAudioHash());
+}
+
+// TODO(dalecurtis): Add a test file which ffmpeg and chunkdemuxer will both

[chcunningham, 2015/07/31 19:48:45]

Don't forget to file a bug if you don't handle this here.

[DaleCurtis, 2015/08/03 22:49:53]

Attached the bug to this.

+// seek in.  FFmpeg just always seeks to zero in bear-opus.webm while MSE
+// more correctly seeks to ~1s, resulting in some discrepencies.
+
 TEST_F(PipelineIntegrationTest, BasicPlaybackLive) {
   ASSERT_EQ(PIPELINE_OK, Start("bear-320x240-live.webm", kHashed));
 
   Play();
 
   ASSERT_TRUE(WaitUntilOnEnded());
 
   EXPECT_HASH_EQ("f0be120a90a811506777c99a2cdf7cc1", GetVideoHash());
   EXPECT_HASH_EQ("-3.59,-2.06,-0.43,2.15,0.77,-0.95,", GetAudioHash());
 
@@ skipping 112 matching lines @@
   EXPECT_EQ(kOpusEndTrimmingWebMFileDurationMs,
             pipeline_->GetBufferedTimeRanges().end(0).InMilliseconds());
   Play();
 
   ASSERT_TRUE(WaitUntilOnEnded());
   source.Abort();
   Stop();
 }
 
 // Flaky. http://crbug.com/304776
 TEST_F(PipelineIntegrationTest, DISABLED_MediaSource_Opus_Seeking_WebM) {

[chcunningham, 2015/07/31 19:48:44]

Still flaky? Perhaps the hashes are different now?

[DaleCurtis, 2015/08/03 22:49:53]

I haven't checked, will need to build a valgrind build and see. That's something
we can solve outside of this change though.

   MockMediaSource source("bear-opus-end-trimming.webm", kOpusAudioOnlyWebM,
                          kAppendWholeFile);
   StartHashedPipelineWithMediaSource(&source);
 
   EXPECT_EQ(1u, pipeline_->GetBufferedTimeRanges().size());
   EXPECT_EQ(0, pipeline_->GetBufferedTimeRanges().start(0).InMilliseconds());
   EXPECT_EQ(kOpusEndTrimmingWebMFileDurationMs,
             pipeline_->GetBufferedTimeRanges().end(0).InMilliseconds());
 
   base::TimeDelta start_seek_time = base::TimeDelta::FromMilliseconds(1000);
@@ skipping 795 matching lines @@
 
 TEST_F(PipelineIntegrationTest, BasicPlaybackPositiveStartTime) {
   ASSERT_EQ(PIPELINE_OK, Start("nonzero-start-time.webm"));
   Play();
   ASSERT_TRUE(WaitUntilOnEnded());
   ASSERT_EQ(base::TimeDelta::FromMicroseconds(396000),
             demuxer_->GetStartTime());
 }
 
 }  // namespace media