Introduce PushPullFIFO class and remove other FIFOs

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

+// 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>
+#include <vector>
+
+namespace blink {
+
+namespace {
+
+// Fills each AudioChannel in an AudioBus with a series of indexes starting from
+// |frameCounter|. Then returns the increased |frameCounter|.
+// (e.g. [c + 0, c + 1, c + 2, c + 3, c + 4, ...])

[Raymond Toy, 2017/01/27 17:39:44]

Without looking at the code, what is c?  How is frameCounter used?

[hongchan, 2017/01/27 22:39:14]

Not sure how to rephrase this better. I think it is quiet clear - can you make
some suggestions?

+size_t fillBusWithFrameCounter(AudioBus* targetBus, size_t frameCounter) {
+  for (unsigned c = 0; c < targetBus->numberOfChannels(); ++c) {
+    float* busChannel = targetBus->channel(c)->mutableData();
+    for (size_t i = 0; i < targetBus->channel(c)->length(); ++i) {
+      busChannel[i] = static_cast<float>(frameCounter + i);
+    }
+  }
+  return frameCounter + targetBus->length();
+}
+
+// Inspect the content of AudioBus with a given set of index and value across
+// channels.
+bool verifyBusValueAtIndex(AudioBus* targetBus,
+                           int index,
+                           float expectedValue) {
+  for (unsigned c = 0; c < targetBus->numberOfChannels(); ++c) {
+    float* busChannel = targetBus->channel(c)->mutableData();
+    if (busChannel[index] != expectedValue) {
+      LOG(ERROR) << ">> [FAIL] expected " << expectedValue << " at index "
+                 << index << " but got " << busChannel[index] << ".";
+      return false;
+    }
+  }
+  return true;
+}
+
+// Data type for FIFO actions {"PUSH", "PULL"} with the frame count.

[Raymond Toy, 2017/01/27 17:39:44]

Put these comments with the variables below.  |type| and |frames| are pretty 
meaningless with comments.  Or better yet, choose better names.

[hongchan, 2017/01/27 22:39:14]

Done.

+struct FIFOAction {
+  const char* type;
+  const size_t frames;
+};
+
+// Test setup data container. The |actions| vector contains a list of FIFO
+// actions that needs to be performed.
+struct FIFOTestSetup {
+  const size_t fifoLength;
+  const unsigned numberOfChannels;
+  const std::vector<FIFOAction> actions;
+};
+
+// Data type of AudioBus verification; an index and the associated value.

[Raymond Toy, 2017/01/27 17:39:44]

This doesn't help me to understand what |index| and |value| mean.

[hongchan, 2017/01/27 22:39:14]

Done.

+struct AudioBusSample {
+  const size_t index;
+  const float value;
+};
+
+// Data container for the expected result. Note that |fifoSamples| and
+// |outputSamples| contain a list of |AudioBusSample| entries for multiple
+// verification points.
+struct FIFOTestExpectedState {
+  const size_t indexRead;
+  const size_t indexWrite;

[Raymond Toy, 2017/01/27 17:39:44]

Document what these two really mean.

[hongchan, 2017/01/27 22:39:14]

Done.

+  const unsigned overflowCount;
+  const unsigned underflowCount;
+  const std::vector<AudioBusSample> fifoSamples;
+  const std::vector<AudioBusSample> outputSamples;
+};
+
+// The data structure for the parameterized test cases.
+struct FIFOTestParam {
+  FIFOTestSetup setup;
+  FIFOTestExpectedState expectedState;
+};
+
+std::ostream& operator<<(std::ostream& out, const FIFOTestParam& param) {
+  out << "fifoLength=" << param.setup.fifoLength
+      << "numberOfChannels=" << param.setup.numberOfChannels;
+  return out;
+}
+
+FIFOTestParam basicFIFOTestParams[] = {
+
+    // 1. Push 1024 frames first then pull 1024 frames.

[Raymond Toy, 2017/01/27 17:39:44]

It would help if there were a contract/documentation on what this FIFO is
intended to support.  For example, will the audio thread ever push anything
other than 128 frames?  If not, we don't need to test anything other than this. 
(Except maybe to make sure we crash if something other than 128 is used.)

[hongchan, 2017/01/27 22:39:14]

Added several comments in the FIFO header file.

+    {{8192, 2, {{"PUSH", 1024}, {"PULL", 1024}}},
+     {1024, 1024, 0, 0, {{1024, 0}}, {{0, 0}, {1023, 1023}}}},
+
+    // 2. Push 3 x 768 frames, then pull 1024 frames. 1 overflow should be
+    //    recorded and the wrap-around should happen correctly. Use odd channel
+    //    count.
+    {{2048, 3, {{"PUSH", 768}, {"PUSH", 768}, {"PUSH", 768}, {"PULL", 1024}}},
+     {1280, 256, 1, 0, {{255, 2303}, {256, 256}}, {{0, 256}, {1023, 1279}}}},
+
+    // 3. Push 512 frames and then pull 1024. Check if underflow occurs
+    //    correctly. Inspect the output bus content; the underflow will fill the
+    //    second half of the output bus silence. Use odd FIFO size.
+    {{4096, 1, {{"PUSH", 512}, {"PULL", 1024}}},
+     {512, 512, 0, 1, {{512, 0}}, {{0, 0}, {511, 511}, {512, 0}, {1023, 0}}}},
+
+    // 4. Push 11, 324 frames. Then pull 11, 324 frames. The most recent output
+    //    must start from 11 and end with 323. Use odd FIFO size and channel
+    //    count.
+    {{1234, 5, {{"PUSH", 11}, {"PUSH", 324}, {"PULL", 11}, {"PULL", 324}}},
+     {335, 335, 0, 0, {}, {{0, 11}, {323, 334}}}}};

[o1ka, 2017/01/27 13:47:54]

This is nice, but what I was suggesting was a bit different.
Something like this:
Fifo size: 12345
push: buffer size 11, frequency 121 pushes/100 cycles
pull: buffer size 17, frequency 113 pulls/100 cycles
test duration: 15000 cycles.

Please make sure to use all primes for a portion of test cases, to catch various
corner cases.

[hongchan, 2017/01/27 22:39:14]

Before modifying the test structure, I think we all should agree with the
premise/contract of the FIFO design.

+
+class PushPullFIFOTest : public ::testing::TestWithParam<FIFOTestParam> {};
+
+TEST_P(PushPullFIFOTest, BasicTests) {
+  const FIFOTestSetup setup = GetParam().setup;
+  const FIFOTestExpectedState expectedState = GetParam().expectedState;
+
+  // Create a FIFO with a specified configuration.
+  std::unique_ptr<PushPullFIFO> fifo = WTF::wrapUnique(
+      new PushPullFIFO(setup.numberOfChannels, setup.fifoLength));
+
+  RefPtr<AudioBus> outputBus;
+
+  // Iterate all the scheduled push/pull actions.
+  size_t frameCounter = 0;
+  for (const auto& action : setup.actions) {
+    if (strcmp(action.type, "PUSH") == 0) {
+      RefPtr<AudioBus> inputBus =
+          AudioBus::create(setup.numberOfChannels, action.frames);
+      frameCounter = fillBusWithFrameCounter(inputBus.get(), frameCounter);
+      fifo->push(inputBus.get());
+      LOG(INFO) << "PUSH " << action.frames
+                << " frames (frameCounter=" << frameCounter << ")";
+    } else {
+      outputBus = AudioBus::create(setup.numberOfChannels, action.frames);
+      fifo->pull(outputBus.get(), action.frames);
+      LOG(INFO) << "PULL " << action.frames << " frames";
+    }
+  }
+
+  // Get FIFO config data.
+  const PushPullFIFOStateForTest actualState = fifo->getStateForTest();
+
+  // Verify the read/write indexes.
+  EXPECT_EQ(actualState.indexRead, expectedState.indexRead);
+  EXPECT_EQ(actualState.indexWrite, expectedState.indexWrite);
+  EXPECT_EQ(actualState.overflowCount, expectedState.overflowCount);
+  EXPECT_EQ(actualState.underflowCount, expectedState.underflowCount);
+
+  // Verify in-FIFO samples.
+  for (const auto& sample : expectedState.fifoSamples) {
+    EXPECT_TRUE(verifyBusValueAtIndex(fifo->bus(), sample.index, sample.value));
+  }
+
+  // Verify samples from the most recent output bus.
+  for (const auto& sample : expectedState.outputSamples) {
+    EXPECT_TRUE(
+        verifyBusValueAtIndex(outputBus.get(), sample.index, sample.value));
+  }
+}
+
+INSTANTIATE_TEST_CASE_P(PushPullFIFOTest,
+                        PushPullFIFOTest,
+                        ::testing::ValuesIn(basicFIFOTestParams));
+
+// TODO(hongchan): Do death tests for SECURITY_CHECK().
+
+}  // namespace
+
+}  // namespace blink