Introduce PushPullFIFO class and remove other FIFOs

third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp

Index: third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp
diff --git a/third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp b/third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..9530d82a54339aa253590b532d33b07f2593cd55
--- /dev/null
+++ b/third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp
@@ -0,0 +1,134 @@
+// Copyright 2016 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 "platform/audio/PushPullFIFO.h"
+
+#include "platform/audio/AudioUtilities.h"
+#include "platform/testing/TestingPlatformSupport.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "wtf/PtrUtil.h"
+#include <memory>
+
+namespace blink {
+
+namespace {
+
+void fillBusWithValue(AudioBus* targetBus, float value) {
+  for (unsigned c = 0; c < targetBus->numberOfChannels(); ++c) {
+    float* busChannel = targetBus->channel(c)->mutableData();
+    std::fill(busChannel, busChannel + targetBus->channel(c)->length(), value);
+  }
+}
+
+void fillBusWithIncrementalIndex(AudioBus* targetBus) {

[Raymond Toy, 2017/01/23 17:52:25]

What does "IncrementalIndex" mean here?

[hongchan, 2017/01/23 19:37:36]

[0, 1, 2, 3, 4, 5, 6, .... ]

You can call it a ramp, but this is literally an array filled with indexes.

[Raymond Toy, 2017/01/23 21:53:53]

The name is not very informative because there's no context on what
"Incremental" is or what "Index" you're referring to.  At least
fillBusWithLinearRamp kind of tells you whats in the bus.

[hongchan, 2017/01/26 22:33:18]

Done.

+  for (unsigned c = 0; c < targetBus->numberOfChannels(); ++c) {
+    float* busChannel = targetBus->channel(c)->mutableData();
+    for (unsigned i = 0; i < targetBus->channel(c)->length(); ++i)
+      busChannel[i] = i;
+  }
+}
+
+bool verifyBusValueAtIndex(AudioBus* targetBus, int index, float value) {
+  for (unsigned c = 0; c < targetBus->numberOfChannels(); ++c) {
+    float* busChannel = targetBus->channel(c)->mutableData();
+    if (busChannel[index] != value) {
+      LOG(ERROR) << "[FAIL] index = " << index
+                 << " value = " << busChannel[index];
+      return false;
+    }
+  }
+  return true;
+}
+
+struct PushPullFIFOTestParam {
+  PushPullFIFOTestParam(size_t fifoSize,
+                        unsigned numberOfChannels,
+                        size_t framesAvailable,
+                        size_t framesToPush,
+                        size_t framesToPull,
+                        unsigned expectedReadIndex,
+                        float expectedValueAtReadIndex,
+                        unsigned expectedWriteIndex,
+                        float expectedValueAtWriteIndex)
+      : fifoSize(fifoSize),
+        numberOfChannels(numberOfChannels),
+        framesAvailable(framesAvailable),
+        framesToPush(framesToPush),
+        framesToPull(framesToPull),
+        expectedReadIndex(expectedReadIndex),
+        expectedValueAtReadIndex(expectedValueAtReadIndex),
+        expectedWriteIndex(expectedWriteIndex),
+        expectedValueAtWriteIndex(expectedValueAtWriteIndex) {}
+  ~PushPullFIFOTestParam() {}
+
+  const size_t fifoSize;
+  const unsigned numberOfChannels;
+  const size_t framesAvailable;
+  const size_t framesToPush;
+  const size_t framesToPull;
+  const unsigned expectedReadIndex;
+  const float expectedValueAtReadIndex;
+  const unsigned expectedWriteIndex;
+  const float expectedValueAtWriteIndex;
+};
+
+std::ostream& operator<<(std::ostream& out,
+                         const PushPullFIFOTestParam& param) {
+  out << "fifoSze=" << param.fifoSize
+      << " numberOfChannels=" << param.numberOfChannels
+      << " framesAvailable=" << param.framesAvailable
+      << " framesToPush=" << param.framesToPush
+      << " framesToPull=" << param.framesToPull
+      << " expectedReadIndex=" << param.expectedReadIndex
+      << " expectedValueAtReadIndex=" << param.expectedValueAtReadIndex
+      << " expectedWriteIndex=" << param.expectedWriteIndex
+      << " expectedValueAtWriteIndex=" << param.expectedValueAtWriteIndex;
+  return out;
+}
+
+class PushPullFIFOTest
+    : public ::testing::TestWithParam<PushPullFIFOTestParam> {};
+
+TEST_P(PushPullFIFOTest, BasicOperation) {
+  std::unique_ptr<PushPullFIFO> fifo = WTF::wrapUnique(
+      new PushPullFIFO(GetParam().numberOfChannels, GetParam().fifoSize));
+
+  // Simulates existing frames in the FIFO. These frames are initialized with
+  // -1.
+  RefPtr<AudioBus> existingBus =
+      AudioBus::create(GetParam().numberOfChannels, GetParam().framesAvailable);
+  fillBusWithValue(existingBus.get(), -1);
+  fifo->push(existingBus.get());
+
+  // Then push data with given length. The content of input bus is a series of
+  // incremental index.

[Raymond Toy, 2017/01/23 17:52:25]

"incremental index"?  This is just basically a linear ramp?

[hongchan, 2017/01/23 19:37:35]

See the comment above.

+  RefPtr<AudioBus> inputBus =
+      AudioBus::create(GetParam().numberOfChannels, GetParam().framesToPush);
+  fillBusWithIncrementalIndex(inputBus.get());
+  fifo->push(inputBus.get());
+
+  // Lastly, pull the data from FIFO to reach the state required by the test
+  // parameter.

[Raymond Toy, 2017/01/23 17:52:25]

"test parameter" -> |framesToPull| argument.  Or something because "test
parameter" is too since there are many parameters to PushPullFIFOTestParam.

[hongchan, 2017/01/23 19:37:36]

Done.

[hongchan, 2017/01/26 22:33:18]

This is sort of a combination of push and pull. So technically |the test
parameter| is a correct term here.

+  RefPtr<AudioBus> outputBus =
+      AudioBus::create(GetParam().numberOfChannels, GetParam().framesToPull);
+  fifo->pull(outputBus.get(), GetParam().framesToPull);
+
+  // Verify the position of current indexes and their values.
+  EXPECT_EQ(true,
+            verifyBusValueAtIndex(fifo->bus(), GetParam().expectedReadIndex,
+                                  GetParam().expectedValueAtReadIndex));
+  EXPECT_EQ(true,
+            verifyBusValueAtIndex(fifo->bus(), GetParam().expectedWriteIndex,
+                                  GetParam().expectedValueAtWriteIndex));
+}

[Raymond Toy, 2017/01/23 17:52:25]

Should there be some checks on the values of the read/write indices?  Or is
testing the output good enough?

Should there be multiple push/pulls tested too to verify that the indices are
updated correctly?  Need tests to verify wrap around too?  Maybe that should be
in a different test?

[hongchan, 2017/01/23 19:37:36]

For that, I will have to add some methods to the class to extract the value.
Yes, that's the idea. If we like the pattern in this patchset then I will start
expanding the unit test with different cases.

[Raymond Toy, 2017/01/23 21:53:53]

Oh. I was kind of expecting this to already be available since the Param already
includes expectedReadIndex and expectedWriteIndex.  So if you're not going to
test the indices in this CL, I'd remove these (if that is what they really
mean). If you are going to test, then we need the new methods.

[hongchan, 2017/01/23 22:28:36]

Based on quick offline discussion, the test looks fine. I am starting to expand
with more test cases.

+
+INSTANTIATE_TEST_CASE_P(
+    PushPullFIFOTest,
+    PushPullFIFOTest,
+    ::testing::Values(
+        PushPullFIFOTestParam(8192, 2, 128, 256, 128, 128, 0, 384, 0)));
+
+}  // namespace
+
+}  // namespace blink