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..258a09b3b3c35d340caeb2805bb9e40b826f4014
--- /dev/null
+++ b/third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp
@@ -0,0 +1,109 @@
+// 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 "wtf/PtrUtil.h"
+#include <memory>
+
+namespace blink {
+
+PushPullFIFO::PushPullFIFO(unsigned numberOfChannels, size_t length)
+    : m_fifoDataBus(AudioBus::create(numberOfChannels, length)),
+      m_fifoLength(length),
+      m_framesInFIFO(0),
+      m_indexRead(0),
+      m_indexWrite(0) {}
+
+PushPullFIFO::~PushPullFIFO() {}
+
+// Push the data from |inputBus| to FIFO. The size of push is determined by
+// the length of |inputBus|.
+//
+// TODO(hongchan): consider the following cases.
+//   - What if the write index wraps around and passes the read index again?

[o1ka, 2017/01/10 10:07:19]

This is the most interesting part.

+//   - What if the pulling is invoked while the pushing is in progress?

[o1ka, 2017/01/10 10:07:19]

Grab the lock :)

+void PushPullFIFO::push(const AudioBus* inputBus) {
+  DCHECK(inputBus);
+  DCHECK_LE(inputBus->length() + m_framesInFIFO, m_fifoLength);
+  DCHECK_LE(m_indexWrite, m_fifoLength);
+
+  if (!inputBus)
+    return;

[o1ka, 2017/01/10 10:07:19]

Why these silent returns are less preferable than crashes:
If we have a crash, it will be reported and we can debug it and fix it. If we
return silently, there will be "no audio" bugs files those impossible to debug,
because there is no any data.

[Raymond Toy, 2017/01/10 16:18:15]

This is how most of WebAudio works, from the original author.  This is one
aspect of the code that I really hate, exactly for this reason.  If something
truly bad happens (out-of-bounds access, invariants aren't), I think we should
just crash instead of continuing silently.

+
+  size_t remainder = m_fifoLength - m_indexWrite;

[o1ka, 2017/01/10 10:07:19]

Here and in other places where you work with memory indexes: It should be
CHECK(m_fifoLengthm_indexWrite <= m_fifoLength), not DCHECK: otherwise we can
have size_t overflow and will be writing into random memory - it's a security
issue.

+  size_t inputBusLength = inputBus->length();
+
+  for (unsigned i = 0; i < m_fifoDataBus->numberOfChannels(); ++i) {
+    float* fifoBusChannel = m_fifoDataBus->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));
+    }
+  }
+
+  // Accumulate the valid number of frames in FIFO.
+  m_framesInFIFO += inputBusLength;
+  DCHECK_LE(m_framesInFIFO, m_fifoLength);
+
+  // Update the write index; wrap it around if necessary.
+  m_indexWrite = (m_indexWrite + 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
+// silence.

[o1ka, 2017/01/10 10:07:20]

This is not true as of now. You have a DCHECK on l.73.

+//
+// TODO(hongchan): consider the following cases.
+//    - What if the frames to pull is bigger than the frames in FIFO?
+//    - What should the lock be placed?

[o1ka, 2017/01/10 10:07:19]

Not sure what it means.

+void PushPullFIFO::pull(AudioBus* outputBus, size_t framesToPull) {
+  DCHECK(outputBus);
+  DCHECK_LE(framesToPull, outputBus->length());
+  DCHECK_LE(framesToPull, m_fifoLength);
+  DCHECK_LE(framesToPull, m_framesInFIFO);
+  DCHECK_LE(m_indexRead, m_fifoLength);

[o1ka, 2017/01/10 10:07:19]

See my other comment about memory indexes: these should be CHECKs

+
+  if (!outputBus)
+    return;
+
+  // Pulling operation has a higher priority to ensure the audio device callback
+  // to be uninterrupted.
+  MutexLocker locker(m_fifoDataBusLock);

[o1ka, 2017/01/10 10:07:20]

I'm not sure how "priority" is addressed here.
However, you took the lock in pull(), and did not take it in push(). 
This is not quite how locks work.
"locks are advisory locks, where each thread cooperates by acquiring the lock
before accessing the corresponding data", see
[https://en.wikipedia.org/wiki/Lock_(computer_science)]

+
+  size_t remainder = m_fifoLength - m_indexRead;
+
+  for (unsigned i = 0; i < m_fifoDataBus->numberOfChannels(); ++i) {
+    const float* fifoBusChannel = m_fifoDataBus->channel(i)->data();
+    float* outputBusChannel = outputBus->channel(i)->mutableData();
+    if (remainder >= framesToPull) {
+      // The remainder is big enough for the frames to pull.
+      memcpy(outputBusChannel, fifoBusChannel + m_indexRead,
+             framesToPull * 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,
+             (framesToPull - remainder) * sizeof(*fifoBusChannel));
+    }
+  }
+
+  // Update the read index; wrap it around if necessary.
+  m_indexRead = (m_indexRead + framesToPull) % m_fifoLength;
+
+  // Decrement the valid number of frames in FIFO.
+  m_framesInFIFO -= framesToPull;
+}
+
+}  // namespace blink