Introduce PushPullFIFO class and remove other FIFOs

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

Index: third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp
diff --git a/third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp b/third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..5d666cc8b8df1a98594f2b1c5205e6d89ab7e6ed
--- /dev/null
+++ b/third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp
@@ -0,0 +1,142 @@
+// Copyright 2017 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 "wtf/PtrUtil.h"
+#include <memory>
+
+namespace blink {
+
+// Maximum FIFO length.
+const size_t kMaxFIFOLength = 32768;

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

rtoy@ and I believe 32K is good enough based on the data we collected.

[Raymond Toy, 2017/02/01 19:02:37]

I checked the metrics again.  There is one case of 46080 in the last 28 days; 7
with 31744(!?!)

Perhaps we fundamentally don't care about such huge latencies.  But also
consider that we support a max sample rate of 384 kHz now, so the fifo may need
to be scaled up to accommodate that?

[hongchan, 2017/02/01 23:09:06]

Should FIFO aware of sample rate? Is that what we want?

[Raymond Toy, 2017/02/01 23:38:55]

Well, I was just thinking it needs to be large enough to support 384 kHz. 32k
size is just 40ms of buffering.  Is that enough?  Perhaps.  We can always easily
increase the limit later.  Just want to be sure we have something fairly large.

[hongchan, 2017/02/02 19:55:23]

Acknowledged.

+
+// Suppress the warning log if over/underflow happens more than 100 times.
+const unsigned kLogCapOverUnderflow = 100;

[Raymond Toy, 2017/01/30 20:59:28]

Maybe kMaxMessagesToLog?

[hongchan, 2017/02/01 18:07:27]

Done.

+
+PushPullFIFO::PushPullFIFO(unsigned numberOfChannels, size_t fifoLength)
+    : m_fifoLength(std::min(fifoLength, kMaxFIFOLength)),
+      m_fifoBus(AudioBus::create(numberOfChannels, m_fifoLength)),
+      m_framesAvailable(0),
+      m_indexRead(0),
+      m_indexWrite(0),
+      m_overflowCount(0),
+      m_underflowCount(0) {}
+
+PushPullFIFO::~PushPullFIFO() {}
+
+// Push the data from |inputBus| to FIFO. The size of push is determined by
+// the length of |inputBus|.
+void PushPullFIFO::push(const AudioBus* inputBus) {
+  CHECK(inputBus);
+  SECURITY_CHECK(inputBus->length() <= m_fifoLength);
+  SECURITY_CHECK(m_indexWrite < m_fifoLength);

[Raymond Toy, 2017/01/30 20:59:28]

You said the push size is 128.  Shouldn't there be a check here for that? 
Otherwise, there's no enforcement of this requirement.

[hongchan, 2017/02/01 18:07:27]

Done.

+
+  const size_t inputBusLength = inputBus->length();
+  const size_t remainder = m_fifoLength - m_indexWrite;
+
+  for (unsigned i = 0; i < m_fifoBus->numberOfChannels(); ++i) {
+    float* fifoBusChannel = m_fifoBus->channel(i)->mutableData();
+    const float* inputBusChannel = inputBus->channel(i)->data();
+    if (remainder >= inputBusLength) {
+      // The remainder is big enough for the input data.
+      memcpy(fifoBusChannel + m_indexWrite, inputBusChannel,
+             inputBusLength * sizeof(*fifoBusChannel));
+    } else {
+      // The input data overflows the remainder size. Wrap around the index.
+      memcpy(fifoBusChannel + m_indexWrite, inputBusChannel,
+             remainder * sizeof(*fifoBusChannel));
+      memcpy(fifoBusChannel, inputBusChannel + remainder,
+             (inputBusLength - remainder) * sizeof(*fifoBusChannel));
+    }
+  }
+
+  // Update the write index; wrap it around if necessary.
+  m_indexWrite = (m_indexWrite + inputBusLength) % m_fifoLength;
+
+  // In case of overflow, move the read index to the updated write index.
+  if (inputBusLength > m_fifoLength - m_framesAvailable) {
+    m_indexRead = m_indexWrite;
+    if (++m_overflowCount < kLogCapOverUnderflow) {
+      DVLOG(1) << "PushPullFIFO: overflow while pushing ("
+               << "overflowCount=" << m_overflowCount
+               << ", availableFrames=" << m_framesAvailable
+               << ", inputFrames=" << inputBusLength
+               << ", fifoLength=" << m_fifoLength << ")";
+    }
+  }
+
+  // Update the number of frames available in FIFO.
+  m_framesAvailable =
+      std::min(m_framesAvailable + inputBusLength, m_fifoLength);
+}
+
+// Pull the data out of FIFO to |outputBus|. If the remaining frames in the FIFO
+// is less than the frames to pull, provides the remaining frames plus the

[Raymond Toy, 2017/01/30 20:59:28]

s/the//

[hongchan, 2017/02/01 18:07:27]

Done.

+// silence.
+void PushPullFIFO::pull(AudioBus* outputBus, size_t framesRequested) {
+  CHECK(outputBus);
+  SECURITY_CHECK(framesRequested <= outputBus->length());

[Raymond Toy, 2017/01/30 20:59:28]

You need to add this to your contract.

[hongchan, 2017/02/01 18:07:27]

I can certainly add, but this is more of sanity check rather than the contract.
You cannot request frames that is bigger than the output bucket.

[Raymond Toy, 2017/02/01 19:02:38]

Well, you could have defined pull to use the min of outputBus->length() and
framesRequested.  So it's not clear what the constraints are for pull from
reading the header file.

[hongchan, 2017/02/01 23:09:06]

//  - If |framesRequested| is bigger than the length of |outputBus|, it
//    violates SECURITY_CHECK().

This comment is in the header.

+  SECURITY_CHECK(framesRequested <= m_fifoLength);

[Raymond Toy, 2017/01/30 20:59:28]

This too.

[Raymond Toy, 2017/02/01 19:02:38]

I can imagine that if you request more than the fifo length, you could treat it
as an underflow and fill in zeroes for the remaining data.  You're not doing
that, so it's part of the contract.

[hongchan, 2017/02/01 23:09:06]

//  - If |framesRequested| is bigger than FIFO length, it violates
//    SECURITY_CHECK().

I have this in the header. So it's in the contract.

+  SECURITY_CHECK(m_indexRead < m_fifoLength);
+
+  // TODO(hongchan): add a tryLock here for this operation when two threads are
+  // introduced. Pulling operation has a higher priority to ensure the audio
+  // device callback to be uninterrupted.
+
+  const size_t remainder = m_fifoLength - m_indexRead;
+  const size_t framesToFill = std::min(m_framesAvailable, framesRequested);
+
+  for (unsigned i = 0; i < m_fifoBus->numberOfChannels(); ++i) {
+    const float* fifoBusChannel = m_fifoBus->channel(i)->data();
+    float* outputBusChannel = outputBus->channel(i)->mutableData();
+
+    // Fill up the output bus with the available frames first.
+    if (remainder >= framesToFill) {
+      // The remainder is big enough for the frames to pull.
+      memcpy(outputBusChannel, fifoBusChannel + m_indexRead,
+             framesToFill * sizeof(*fifoBusChannel));
+    } else {
+      // The frames to pull is bigger than the remainder size.
+      // Wrap around the index.
+      memcpy(outputBusChannel, fifoBusChannel + m_indexRead,
+             remainder * sizeof(*fifoBusChannel));
+      memcpy(outputBusChannel + remainder, fifoBusChannel,
+             (framesToFill - remainder) * sizeof(*fifoBusChannel));
+    }
+
+    // The frames available was not enough to fulfill the requested frames. Fill
+    // the rest of the channel with silence.
+    if (framesRequested > framesToFill) {
+      memset(outputBusChannel + framesToFill, 0,
+             (framesRequested - framesToFill) * sizeof(*outputBusChannel));
+    }
+  }
+
+  // Update the read index; wrap it around if necessary.
+  m_indexRead = (m_indexRead + framesToFill) % m_fifoLength;
+
+  // In case of underflow, move the write index to the updated read index.
+  if (framesRequested > framesToFill) {
+    m_indexWrite = m_indexRead;
+    if (m_underflowCount++ < kLogCapOverUnderflow) {
+      DVLOG(1) << "PushPullFIFO: underflow while pulling ("
+               << "underflowCount=" << m_underflowCount
+               << ", availableFrames=" << m_framesAvailable
+               << ", requestedFrames=" << framesRequested
+               << ", fifoLength=" << m_fifoLength << ")";
+    }
+  }
+
+  // Update the number of frames in FIFO.
+  m_framesAvailable =
+      m_framesAvailable > framesToFill ? m_framesAvailable - framesToFill : 0;

[Raymond Toy, 2017/01/30 20:59:28]

Doesn't line 89 guarantees the m_framesAvailable >= framesToFill.  Thus, no
check needed.

[hongchan, 2017/02/01 18:07:27]

Done.

+}
+
+const PushPullFIFOStateForTest PushPullFIFO::getStateForTest() const {
+  return {length(),     numberOfChannels(), framesAvailable(), m_indexRead,
+          m_indexWrite, m_overflowCount,    m_underflowCount};
+}
+
+}  // namespace blink