Introduce PushPullFIFO class and remove other FIFOs

third_party/WebKit/Source/platform/audio/PushPullFIFO.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 "wtf/PtrUtil.h"
+#include <memory>
+
+namespace blink {
+
+PushPullFIFO::PushPullFIFO(unsigned numberOfChannels, size_t fifoLength)
+    : m_fifoBus(AudioBus::create(numberOfChannels, fifoLength)),
+      m_fifoLength(static_cast<int>(fifoLength)),
+      m_framesAvailable(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|.
+void PushPullFIFO::push(const AudioBus* inputBus) {
+  DCHECK(inputBus);
+  if (!inputBus)
+    return;
+
+  int inputBusLength = static_cast<int>(inputBus->length());

[o1ka, 2017/01/13 11:23:44]

const?

[hongchan, 2017/01/13 23:29:22]

Done.

+  int remainder = m_fifoLength - m_indexWrite;

[o1ka, 2017/01/13 11:23:44]

const?

[hongchan, 2017/01/13 23:29:22]

Done.

+  SECURITY_CHECK(inputBusLength < m_fifoLength);
+  SECURITY_CHECK(m_indexWrite < m_fifoLength);
+
+  // This check catches the overflow; the write index will go over the read
+  // index. In the case of push overflow, move the read index to the updated
+  // write index.
+  bool fifoOverflow = m_fifoLength < inputBusLength + m_framesAvailable;
+  DCHECK(!fifoOverflow);

[o1ka, 2017/01/13 11:23:44]

DCHECK means further execution under this condition does not make sense. But
l.61 assumes that fifoOverflow can be true.
Remove DCHECK?

[hongchan, 2017/01/13 23:29:22]

I talked with rtoy@ about these conditions - what's the best practice to notify
when this actually happens? We certainly want to know when and why in the debug
build.

LOG? LOG_IF? DVLOG?

+
+  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));
+    }
+  }
+
+  // Accumulate the valid number of frames in FIFO.
+  m_framesAvailable += inputBusLength;
+  DCHECK_LE(m_framesAvailable, m_fifoLength);
+
+  // Update the write index; wrap it around if necessary.
+  m_indexWrite = (m_indexWrite + inputBusLength) % m_fifoLength;
+  if (fifoOverflow) {
+    m_indexRead = m_indexWrite;
+  }
+}
+
+// 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.
+void PushPullFIFO::pull(AudioBus* outputBus, size_t framesToPull) {

[o1ka, 2017/01/13 11:23:44]

Name it framesRequested? framesToPull, framesToFill and framesToProvide side by
side are very confusing.

[hongchan, 2017/01/13 23:29:23]

Done.

+  DCHECK(outputBus);
+  SECURITY_CHECK(framesToPull <= outputBus->length());
+  if (!outputBus)
+    return;
+
+  // TODO(hongchan): add a mutex here for this operation when two threads are
+  // introduced. Pulling operation has a higher priority to ensure the audio
+  // device callback to be uninterrupted.
+
+  int remainder = m_fifoLength - m_indexRead;
+  int framesToProvide = static_cast<int>(framesToPull);

[o1ka, 2017/01/13 11:23:44]

This is not safe cast. Switch interface to int?

[o1ka, 2017/01/13 11:23:44]

both const?

[hongchan, 2017/01/13 23:29:22]

Done.

[hongchan, 2017/01/13 23:29:23]

I have thought about it, but WebAudio has been using size_t for frame numbers.
So I wanted to keep size_t for public method signature. Also there is a
SECURITY_CHECK() right below to catch when framesToProvide goes crazy.

+  SECURITY_CHECK(framesToProvide < m_fifoLength);
+  SECURITY_CHECK(m_indexRead < m_fifoLength);
+
+  // This check catches the underflow; the read index will go over the write
+  // index. In the case of underflow, the rest will be filled up with zeros
+  // (thus glitches). Note that such case does not cause assertion failure
+  // unlike the pushing process. Pulling an empty FIFO is a valid operation,
+  // and the consumer will get a block of silence.

[Raymond Toy, 2017/01/12 19:29:40]

This is a good comment, but I think you really need to describe the behavior of
the fifo in the header as a general description of the class.  Include info on
how the pointers work and what happens if they cross.

[hongchan, 2017/01/13 23:29:23]

Done.

+  int framesToFill = std::min(m_framesAvailable, framesToProvide);
+  int silentFrames = std::max(0, framesToProvide - m_framesAvailable);

[o1ka, 2017/01/13 11:23:44]

both const?

[o1ka, 2017/01/13 11:23:44]

Just framesToProvide-framesToFill?

[hongchan, 2017/01/13 23:29:22]

Done.

+
+  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,

[Raymond Toy, 2017/01/12 19:29:40]

Should we add DCHECKs here to make sure that we don't write past the end of the
destination or read past the end of the source?  (Other parts of webaudio seem
to do that.)

Same comment for other uses of memcpy and memset.

[hongchan, 2017/01/13 23:29:22]

I believe this is done by line 81 and 82.

+             (framesToFill - remainder) * sizeof(*fifoBusChannel));
+    }
+
+    // Then handles the FIFO data shortage if necessary; The frames available
+    // was not enough to fulfill the requested frames. Fill the rest of the
+    // channel with silence.
+    if (silentFrames > 0) {
+      memset(outputBusChannel + framesToFill, 0,
+             silentFrames * sizeof(*outputBusChannel));
+    }
+  }
+
+  // Update the read index; wrap it around if necessary.
+  m_indexRead = (m_indexRead + framesToFill) % m_fifoLength;
+
+  // Decrement the valid number of frames in FIFO.
+  m_framesAvailable -= framesToFill;

[o1ka, 2017/01/13 11:23:44]

Something like DCHECK((m_indexRead + m_framesAvailable) % m_fifoLength ==
m_indexWrite) ?
BTW - that's why I do not quite like to have all of three: one can be calculated
from the other two. And if we forget to update one of 3, everything is broken.
It may be safer to have getFramesAvailable() calculating the value from the two
indexes.

[hongchan, 2017/01/13 23:29:22]

It seems like a neat idea, but not sure how to make it always correct. To handle
the case of index flipping, we still need something like m_framesAvailable.
Right? How do you calculate it only with two indexes?

For the time being, I minimized the usage of m_framesAvailable in the code. It
is fetched in the beginning and updated at the end, not in the middle.

+  DCHECK_GE(m_framesAvailable, 0);
+}
+
+}  // namespace blink