Roll ffmpeg DEPS for MP3 seek bug, add tests.

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/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
@@ skipping 1211 matching lines @@
   ASSERT_EQ(PIPELINE_OK, Start("sfx.mp3", kHashed));
 
   Play();
 
   ASSERT_TRUE(WaitUntilOnEnded());
 
   // Verify codec delay and preroll are stripped.
   EXPECT_HASH_EQ("1.30,2.72,4.56,5.08,3.74,2.03,", GetAudioHash());
 }
 
+class Mp3SeekTestParams {
+ public:
+  Mp3SeekTestParams(const char* filename, const char* hash)
+      : filename(filename), hash(hash) {}
+  const char* filename;
+  const char* hash;
+};
+class Mp3SeekPipelineIntegrationTest

[ddorwin, 2015/12/03 23:07:54]

nit: Why now empty lines around this class?

[chcunningham, 2015/12/04 02:29:55]

Fixed

+    : public PipelineIntegrationTest,
+      public testing::WithParamInterface<Mp3SeekTestParams> {};
+TEST_P(Mp3SeekPipelineIntegrationTest, FastSeekAccuracy_MP3) {

[ddorwin, 2015/12/03 23:07:54]

Should we have such tests for other types?

[chcunningham, 2015/12/04 02:29:55]

No, this fastseek thing is mp3 specific. See
https://docs.google.com/document/d/1jNzeyaHz3_Hkdm7Z1rS6mVG4pV4Fpq5IxLOGJS7cc...

+  Mp3SeekTestParams config = GetParam();
+  ASSERT_EQ(PIPELINE_OK, Start(config.filename, kHashed));

[ddorwin, 2015/12/03 23:07:54]

None of the ASSERTed items will cause a crash. Generally prefer EXPECT_.

[chcunningham, 2015/12/04 02:29:55]

Leaving to match the other tests in this file

+
+  base::TimeDelta duration(pipeline_->GetMediaDuration());
+
+  // NOTE this seek time of (.3 * duration) is deliberately chosen to expose

[ddorwin, 2015/12/03 23:07:53]

nit: 0.3? Also, below.

[chcunningham, 2015/12/04 02:29:55]

Done.

+  // loss of precision in the MP3 TOC for CBR. Quick TOC design (not pretty!):
+  // - All MP3 TOCs are 100 bytes
+  // - Each byte is read as a uint8, value between 0 - 255.

[ddorwin, 2015/12/03 23:07:54]

nit: remove the comma or replace it with something else

[chcunningham, 2015/12/04 02:29:55]

Done.

+  // - The index into this array is the numerator in the ratio: index / 100.
+  //   This fraction represents a playback time as a percentage of duration.
+  // - The value at the given index is the numerator in the ratio: value / 256.
+  //   This fraction represents a byte offset as a percentage of the file size.
+  //
+  // For CBR files, each frame is the same size, so the offset for time of
+  // (.3 * duration) should be around (.3 * file size). This is 78.8 / 256,

[ddorwin, 2015/12/03 23:07:54]

7_6_.8?

[chcunningham, 2015/12/04 02:29:55]

Done.

+  // but the numerator will be truncated in the TOC as 78, loosing precision.

[ddorwin, 2015/12/03 23:07:53]

76?

[chcunningham, 2015/12/04 02:29:55]

Done.

+  //
+  // TLDR:

[ddorwin, 2015/12/03 23:07:53]

This comment block should probably be external to the test code. Also, this
seems like the introduction followed by the Quick TOC design followed by
discussion of .3. WDYT?

[chcunningham, 2015/12/04 02:29:55]

Reordered and condensed

+  // The TOC is inaccurate. We don't use it for CBR, we tolerate it for VBR (for
+  // faster seeking, see Mp3SeekFFmpegDemuxerTest). The chosen seek time

[ddorwin, 2015/12/03 23:07:54]

nit: semicolon instead of colon? This isn't intended to be a preposition is it?

[chcunningham, 2015/12/04 02:29:55]

Done.

+  // exposes inaccuracy in TOC such that the hash will change if seek logic is
+  // regressed. See https://crbug.com/545914.
+  base::TimeDelta seek_time(.3 * duration);
+  base::TimeDelta end_time = seek_time + base::TimeDelta::FromMilliseconds(500);
+
+  ASSERT_TRUE(Seek(seek_time));
+  Play();
+  ASSERT_TRUE(WaitUntilCurrentTimeIsAfter(end_time));

[ddorwin, 2015/12/03 23:07:54]

Is this call somehow frame-accurate? Otherwise, the hash would vary, right? How
does it work?

[chcunningham, 2015/12/04 02:29:55]

Great catch! reworked the test to play from near-end -> end. 

+
+  EXPECT_HASH_EQ(config.hash, GetAudioHash());
+}
+
+INSTANTIATE_TEST_CASE_P(
+    ,

[ddorwin, 2015/12/03 23:07:54]

?

[chcunningham, 2015/12/04 02:29:55]

Wasn't using this, but I broke up the tests now into the "good" accurate and
"bad" inaccurate seeks

+    Mp3SeekPipelineIntegrationTest,
+    ::testing::Values(Mp3SeekTestParams("bear-audio-10s-CBR-has-TOC.mp3",
+                                        "0.06,0.10,-0.32,0.08,0.52,0.78,"),
+                      Mp3SeekTestParams("bear-audio-10s-CBR-no-TOC.mp3",
+                                        "-0.08,0.06,0.34,0.65,1.01,0.57,"),
+                      Mp3SeekTestParams("bear-audio-10s-VBR-has-TOC.mp3",
+                                        "-0.02,-0.11,0.02,0.49,1.16,0.88,"),
+                      Mp3SeekTestParams("bear-audio-10s-VBR-no-TOC.mp3",
+                                        "0.40,0.22,0.36,0.40,1.21,0.91,")));
+
 TEST_F(PipelineIntegrationTest, MediaSource_MP3) {
   MockMediaSource source("sfx.mp3", kMP3, kAppendWholeFile);
   StartHashedPipelineWithMediaSource(&source);
   source.EndOfStream();
 
   EXPECT_EQ(1u, pipeline_->GetBufferedTimeRanges().size());
   EXPECT_EQ(0, pipeline_->GetBufferedTimeRanges().start(0).InMilliseconds());
   EXPECT_EQ(313, pipeline_->GetBufferedTimeRanges().end(0).InMilliseconds());
 
   Play();
@@ skipping 598 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