Fix muted audio when playback rate != 1.0 or 0.0

media/filters/audio_renderer_algorithm_base_unittest.cc

Index: media/filters/audio_renderer_algorithm_base_unittest.cc
diff --git a/media/filters/audio_renderer_algorithm_base_unittest.cc b/media/filters/audio_renderer_algorithm_base_unittest.cc
index 75d8c52cd3be9819fe6d76d269eb9eec81ac5bfc..917bcca2e2f86ad858c85cfb8584edbfcae2a6af 100644
--- a/media/filters/audio_renderer_algorithm_base_unittest.cc
+++ b/media/filters/audio_renderer_algorithm_base_unittest.cc
@@ -12,132 +12,209 @@
 #include "base/callback.h"
 #include "media/base/data_buffer.h"
 #include "media/filters/audio_renderer_algorithm_base.h"
-#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-using ::testing::AnyNumber;
+static const size_t kRawDataSize = 1024;
+static const int kSamplesPerSecond = 1234;
 
 namespace media {
 
-static const int kChannels = 1;
-static const int kSampleRate = 1000;
-static const int kSampleBits = 8;
-
-TEST(AudioRendererAlgorithmBaseTest, FillBuffer_NormalRate) {
-  // When playback rate == 1.0f: straight copy of whatever is in |queue_|.
-  AudioRendererAlgorithmBase algorithm;
-  algorithm.Initialize(kChannels, kSampleRate, kSampleBits, 1.0f,
-                       base::Bind(&base::DoNothing));
-
-  // Enqueue a buffer of any size since it doesn't matter.
-  const size_t kDataSize = 1024;
-  algorithm.EnqueueBuffer(new DataBuffer(
-      scoped_array<uint8>(new uint8[kDataSize]), kDataSize));
-  EXPECT_EQ(kDataSize, algorithm.bytes_buffered());
-
-  // Read the same sized amount.
-  scoped_array<uint8> data(new uint8[kDataSize]);
-  EXPECT_EQ(kDataSize, algorithm.FillBuffer(data.get(), kDataSize));
-  EXPECT_EQ(0u, algorithm.bytes_buffered());
-}
+class AudioRendererAlgorithmBaseTest : public testing::Test {
+ public:
+  AudioRendererAlgorithmBaseTest()
+      : bytes_enqueued_(0),
+        bytes_per_frame_(0) {
+  }
+
+  ~AudioRendererAlgorithmBaseTest() {}
 
-TEST(AudioRendererAlgorithmBaseTest, FillBuffer_DoubleRate) {
-  // When playback rate > 1.0f: input is read faster than output is written.
-  AudioRendererAlgorithmBase algorithm;
-  algorithm.Initialize(kChannels, kSampleRate, kSampleBits, 2.0f,
-                       base::Bind(&base::DoNothing));
-
-  // First parameter is the input buffer size, second parameter is how much data
-  // we expect to consume in order to have no data left in the |algorithm|.
-  //
-  // For rate == 0.5f, reading half the input size should consume all enqueued
-  // data.
-  const size_t kBufferSize = 16 * 1024;
-  scoped_array<uint8> data(new uint8[kBufferSize]);
-  const size_t kTestData[][2] = {
-    { algorithm.window_size(), algorithm.window_size() / 2},
-    { algorithm.window_size() / 2, algorithm.window_size() / 4},
-    { 4u, 2u },
-    { 0u, 0u },
-  };
-
-  for (size_t i = 0u; i < arraysize(kTestData); ++i) {
-    const size_t kDataSize = kTestData[i][0];
-    algorithm.EnqueueBuffer(new DataBuffer(
-        scoped_array<uint8>(new uint8[kDataSize]), kDataSize));
-    EXPECT_EQ(kDataSize, algorithm.bytes_buffered());
-
-    const size_t kExpectedSize = kTestData[i][1];
-    ASSERT_LE(kExpectedSize, kBufferSize);
-    EXPECT_EQ(kExpectedSize, algorithm.FillBuffer(data.get(), kBufferSize));
-    EXPECT_EQ(0u, algorithm.bytes_buffered());
+  void Initialize() {
+    static const int kDefaultChannels = 2;

[acolwell GONE FROM CHROMIUM, 2012/02/27 21:00:08]

nit: Any reason not to put these up with the constants above?

[vrk (LEFT CHROMIUM), 2012/02/28 23:40:32]

Nah, I just figured I'd put them close to where they're used. I can move them up
top with the rest of them. Done.

+    static const int kDefaultSampleBits = 16;
+
+    Initialize(kDefaultChannels, kDefaultSampleBits);
   }
-}
 
-TEST(AudioRendererAlgorithmBaseTest, FillBuffer_HalfRate) {
-  // When playback rate < 1.0f: input is read slower than output is written.
-  AudioRendererAlgorithmBase algorithm;
-  algorithm.Initialize(kChannels, kSampleRate, kSampleBits, 0.5f,
-                       base::Bind(&base::DoNothing));
-
-  // First parameter is the input buffer size, second parameter is how much data
-  // we expect to consume in order to have no data left in the |algorithm|.
-  //
-  // For rate == 0.5f, reading double the input size should consume all enqueued
-  // data.
-  const size_t kBufferSize = 16 * 1024;
-  scoped_array<uint8> data(new uint8[kBufferSize]);
-  const size_t kTestData[][2] = {
-    { algorithm.window_size(), algorithm.window_size() * 2 },
-    { algorithm.window_size() / 2, algorithm.window_size() },
-    { 2u, 4u },
-    { 0u, 0u },
-  };
-
-  for (size_t i = 0u; i < arraysize(kTestData); ++i) {
-    const size_t kDataSize = kTestData[i][0];
-    algorithm.EnqueueBuffer(new DataBuffer(
-        scoped_array<uint8>(new uint8[kDataSize]), kDataSize));
-    EXPECT_EQ(kDataSize, algorithm.bytes_buffered());
-
-    const size_t kExpectedSize = kTestData[i][1];
-    ASSERT_LE(kExpectedSize, kBufferSize);
-    EXPECT_EQ(kExpectedSize, algorithm.FillBuffer(data.get(), kBufferSize));
-    EXPECT_EQ(0u, algorithm.bytes_buffered());
+  void Initialize(int channels, int bits_per_sample) {
+    bytes_per_frame_ = channels * (bits_per_sample / 8);

[acolwell GONE FROM CHROMIUM, 2012/02/27 21:00:08]

nit: Perhaps add a protected accessor method to algorithm so we don't have to
recompute this here? That way the test and the implementation can't get out of
sync.

[vrk (LEFT CHROMIUM), 2012/02/28 23:40:32]

Done.

+    algorithm_.Initialize(
+        channels, kSamplesPerSecond, bits_per_sample, 1.0f,
+        base::Bind(&AudioRendererAlgorithmBaseTest::EnqueueData,
+                   base::Unretained(this)));
+    EnqueueData();
   }
-}
 
-TEST(AudioRendererAlgorithmBaseTest, FillBuffer_QuarterRate) {
-  // When playback rate is very low the audio is simply muted.
-  AudioRendererAlgorithmBase algorithm;
-  algorithm.Initialize(kChannels, kSampleRate, kSampleBits, 0.25f,
-                       base::Bind(&base::DoNothing));
-
-  // First parameter is the input buffer size, second parameter is how much data
-  // we expect to consume in order to have no data left in the |algorithm|.
-  //
-  // For rate == 0.25f, reading four times the input size should consume all
-  // enqueued data but without executing OLA.
-  const size_t kBufferSize = 16 * 1024;
-  scoped_array<uint8> data(new uint8[kBufferSize]);
-  const size_t kTestData[][2] = {
-    { algorithm.window_size(), algorithm.window_size() * 4},
-    { algorithm.window_size() / 2, algorithm.window_size() * 2},
-    { 1u, 4u },
-    { 0u, 0u },
-  };
-
-  for (size_t i = 0u; i < arraysize(kTestData); ++i) {
-    const size_t kDataSize = kTestData[i][0];
-    algorithm.EnqueueBuffer(new DataBuffer(scoped_array<uint8>(
-        new uint8[kDataSize]), kDataSize));
-    EXPECT_EQ(kDataSize, algorithm.bytes_buffered());
-
-    const size_t kExpectedSize = kTestData[i][1];
-    ASSERT_LE(kExpectedSize, kBufferSize);
-    EXPECT_EQ(kExpectedSize, algorithm.FillBuffer(data.get(), kBufferSize));
-    EXPECT_EQ(0u, algorithm.bytes_buffered());
+  void EnqueueData() {
+    algorithm_.EnqueueBuffer(new DataBuffer(
+        scoped_array<uint8>(new uint8[kRawDataSize]), kRawDataSize));
+    bytes_enqueued_ += kRawDataSize;
+  }
+
+  int ComputeConsumedBytes(int initial_bytes_enqueued,
+                           int initial_bytes_buffered) {
+    int byte_delta = bytes_enqueued_ - initial_bytes_enqueued;
+    int buffered_delta = algorithm_.bytes_buffered() - initial_bytes_buffered;
+    int consumed = byte_delta - buffered_delta;
+    CHECK_GE(consumed, 0);
+    return consumed;
+  }
+
+  void TestPlaybackRate(float playback_rate) {
+    algorithm_.SetPlaybackRate(playback_rate);
+    static const int kDefaultBufferSize = kSamplesPerSecond / 2;
+    static const int kDefaultFramesRequested = 10 * kSamplesPerSecond;
+
+    TestPlaybackRate(
+        playback_rate, kDefaultBufferSize, kDefaultFramesRequested);
   }
+
+  void TestPlaybackRate(float playback_rate,
+                        int buffer_size_in_frames,
+                        int total_frames_requested) {
+    int initial_bytes_enqueued = bytes_enqueued_;
+    int initial_bytes_buffered = algorithm_.bytes_buffered();
+
+    algorithm_.SetPlaybackRate(playback_rate);
+
+    scoped_array<uint8> buffer(
+        new uint8[buffer_size_in_frames * bytes_per_frame_]);
+    int frames_remaining = total_frames_requested;
+
+    while (frames_remaining > 0) {
+      int frames_requested = std::min(buffer_size_in_frames, frames_remaining);
+      int frames_written =
+          algorithm_.FillBuffer(buffer.get(), frames_requested);
+      if (playback_rate == 0.0) {
+        EXPECT_EQ(0, frames_written);

[acolwell GONE FROM CHROMIUM, 2012/02/27 21:00:08]

nit: I think it might be clearer to just move this case out before the loop.

[vrk (LEFT CHROMIUM), 2012/02/28 23:40:32]

Done.

+        return;
+      }
+      frames_remaining -= frames_written;
+    }
+
+    int bytes_requested = total_frames_requested * bytes_per_frame_;
+    int bytes_consumed = ComputeConsumedBytes(initial_bytes_enqueued,
+                                              initial_bytes_buffered);
+
+    // If playing back at normal speed, we should always get back the same
+    // number of bytes requested.
+    if (playback_rate == 1.0) {
+      EXPECT_EQ(bytes_requested, bytes_consumed);
+      return;
+    }
+
+    // Otherwise, allow |kMaxAcceptableDelta| difference between the target and
+    // actual playback rate.
+    static const float kMaxAcceptableDelta = 0.05;

[acolwell GONE FROM CHROMIUM, 2012/02/27 21:00:08]

nit: Is the error bound really constant? I thought it would change w/ the window
sizes. It might be good to base this value off of window values in algorithm_.
An alternative would be to add comment text describing where this value came
from and what assumptions around window size and playback speed are being made.

[vrk (LEFT CHROMIUM), 2012/02/28 23:40:32]

Done.

+    float actual_playback_rate = 1.0 * bytes_consumed / bytes_requested;
+
+    // Calculate the percentage difference from the target |playback_rate| as a
+    // fraction from 0.0 to 1.0.
+    float delta = 1.0 - (actual_playback_rate / playback_rate);

[acolwell GONE FROM CHROMIUM, 2012/02/27 21:00:08]

nit: std::abs() instead of if below?

[vrk (LEFT CHROMIUM), 2012/02/28 23:40:32]

Ooh d'oh! Originally I tried this and it converted everything to int, but now I
realize I was using C abs() instead of std::abs(). Fixed!

+    if (delta < 0.0)
+      delta *= -1.0;
+
+    EXPECT_LE(delta, kMaxAcceptableDelta);
+  }
+
+ protected:
+  AudioRendererAlgorithmBase algorithm_;
+  int bytes_enqueued_;
+  int bytes_per_frame_;
+};
+
+TEST_F(AudioRendererAlgorithmBaseTest, FillBuffer_NormalRate) {
+  Initialize();
+  TestPlaybackRate(1.0);
+}
+
+TEST_F(AudioRendererAlgorithmBaseTest, FillBuffer_OneAndAQuarterRate) {
+  Initialize();
+  TestPlaybackRate(1.25);
+}
+
+TEST_F(AudioRendererAlgorithmBaseTest, FillBuffer_OneAndAHalfRate) {
+  Initialize();
+  TestPlaybackRate(1.5);
+}
+
+TEST_F(AudioRendererAlgorithmBaseTest, FillBuffer_DoubleRate) {
+  Initialize();
+  TestPlaybackRate(2.0);
+}
+
+TEST_F(AudioRendererAlgorithmBaseTest, FillBuffer_ThreeQuartersRate) {
+  Initialize();
+  TestPlaybackRate(0.75);
+}
+
+TEST_F(AudioRendererAlgorithmBaseTest, FillBuffer_HalfRate) {
+  Initialize();
+  TestPlaybackRate(0.5);
+}
+
+TEST_F(AudioRendererAlgorithmBaseTest, FillBuffer_QuarterRate) {
+  Initialize();
+  TestPlaybackRate(0.25);
+}
+
+TEST_F(AudioRendererAlgorithmBaseTest, FillBuffer_Pause) {
+  Initialize();
+  TestPlaybackRate(0.0);
+}
+
+TEST_F(AudioRendererAlgorithmBaseTest, FillBuffer_SlowDown) {
+  Initialize();
+  TestPlaybackRate(3.0);

[acolwell GONE FROM CHROMIUM, 2012/02/27 21:00:08]

nit: Should add a case above kMaxPlaybackRate (ie 4x) so we test that code path.

[vrk (LEFT CHROMIUM), 2012/02/28 23:40:32]

Done.

+  TestPlaybackRate(2.0);
+  TestPlaybackRate(1.0);
+  TestPlaybackRate(0.5);
+  TestPlaybackRate(0.25);
+}
+
+TEST_F(AudioRendererAlgorithmBaseTest, FillBuffer_SpeedUp) {
+  Initialize();
+  TestPlaybackRate(0.25);
+  TestPlaybackRate(0.5);
+  TestPlaybackRate(1.0);
+  TestPlaybackRate(2.0);
+  TestPlaybackRate(3.0);

[acolwell GONE FROM CHROMIUM, 2012/02/27 21:00:08]

nit: Should add a case above kMaxPlaybackRate (ie 4x) so we test that code path.

[vrk (LEFT CHROMIUM), 2012/02/28 23:40:32]

Done.

+}
+
+TEST_F(AudioRendererAlgorithmBaseTest, FillBuffer_JumpAroundSpeeds) {
+  Initialize();
+  TestPlaybackRate(2.1);
+  TestPlaybackRate(0.9);
+  TestPlaybackRate(0.6);
+  TestPlaybackRate(1.4);
+  TestPlaybackRate(0.3);
+}
+
+TEST_F(AudioRendererAlgorithmBaseTest, FillBuffer_SmallBufferSize) {
+  Initialize();
+  static const int kBufferSizeInFrames = 1;
+  static const int kFramesRequested= kSamplesPerSecond;
+
+  TestPlaybackRate(1.0, kBufferSizeInFrames, kFramesRequested);
+  TestPlaybackRate(0.5, kBufferSizeInFrames, kFramesRequested);
+  TestPlaybackRate(1.5, kBufferSizeInFrames, kFramesRequested);
+}
+
+TEST_F(AudioRendererAlgorithmBaseTest, FillBuffer_LowerQualityAudio) {
+  static const int kChannels = 1;
+  static const int kSampleBits = 8;
+  Initialize(kChannels, kSampleBits);
+  TestPlaybackRate(1.0);
+  TestPlaybackRate(0.5);
+  TestPlaybackRate(1.5);
+}
+
+TEST_F(AudioRendererAlgorithmBaseTest, FillBuffer_HigherQualityAudio) {
+  static const int kChannels = 2;
+  static const int kSampleBits = 32;
+  Initialize(kChannels, kSampleBits);
+  TestPlaybackRate(1.0);
+  TestPlaybackRate(0.5);
+  TestPlaybackRate(1.5);
 }
 

[acolwell GONE FROM CHROMIUM, 2012/02/27 21:00:08]

Should we add tests to verify sound is output between kMinPlaybackRate <
playbackRate < kMaxPlaybackRate and muted otherwise? We could simply enqueue sin
or square waves and then check to see if we get all zeros or buffers that
contain non-zero data.

[vrk (LEFT CHROMIUM), 2012/02/28 23:40:32]

Done - but it took more code than I thought! I had to use templated functions
again to deal with the different audio sizes. This helped me find a bug though
-- see comment in AudioRendererAlgorithmBase. Let me know what you think!

 }  // namespace media