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

media/test/pipeline_integration_test.cc

Index: media/test/pipeline_integration_test.cc
diff --git a/media/test/pipeline_integration_test.cc b/media/test/pipeline_integration_test.cc
index b81a19cba477d6bf0151ec6170855c95c49dd580..a06b08db23bff8fc1810940e1e2819a56021ad91 100644
--- a/media/test/pipeline_integration_test.cc
+++ b/media/test/pipeline_integration_test.cc
@@ -105,6 +105,16 @@ 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;
@@ -487,6 +497,13 @@ class MockMediaSource {
     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());
@@ -677,6 +694,12 @@ class PipelineIntegrationTest : public PipelineIntegrationTestHost {
     StartPipelineWithMediaSource(source);
   }
 
+  void StartHashedClocklessPipelineWithMediaSource(MockMediaSource* source) {
+    hashing_enabled_ = true;
+    clockless_playback_ = true;
+    StartPipelineWithMediaSource(source);
+  }
+
   void StartPipelineWithEncryptedMedia(
       MockMediaSource* source,
       FakeEncryptedMedia* encrypted_media) {
@@ -783,6 +806,86 @@ TEST_F(PipelineIntegrationTest, BasicPlaybackHashed) {
   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));