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 length)
+    : m_dataBus(AudioBus::create(numberOfChannels, length)),
+      m_length(length),
+      m_numberOfFrames(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) {
+  if (!inputBus) {
+    NOTREACHED();

[o1ka, 2017/01/09 14:09:03]

Why not just DCHECK(inputBus)?

[hongchan, 2017/01/09 21:53:16]

Done. The early return is for the release build.

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

See my other comment about early returns

+    return;
+  }
+
+  size_t remainder = m_length - m_indexWrite;
+  size_t inputBusLength = inputBus->length();
+
+  if (remainder > m_length || m_indexWrite > m_length ||
+      inputBusLength > m_length ||
+      inputBusLength + m_numberOfFrames > m_length) {

[Raymond Toy, 2017/01/06 22:22:03]

Need a check if number of channels for the bus matches the number of channels in
the FIFO?  What should happen if they don't match?

[o1ka, 2017/01/09 14:09:03]

Same comment about naming: m_length, inputBusLength and m_numberOfFrames look
very confusing side by side (inputBusLength looks good by itself).

[hongchan, 2017/01/09 21:53:16]

@rtoy

Hmm. My code is based on the current FIFO design, but maybe we need a fresh look
on its heuristic. However, is it really possible? The size of AudioBus is fixed
after the creation, which is based on the info from the audio device. I don't
think that the audio device channel config can be changed on the fly.

I think it's up to how our audio backend behaves.

@o1ka

Should I separate these checks into multiple assertions? What do you mean by
"inputBusLength looking good by itself"?

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

Sorry for being unclear, my comment was about variable names.
Yes, it's better to have separate DCHECKs

[hongchan, 2017/01/10 21:15:58]

Done.

+    NOTREACHED();

[o1ka, 2017/01/09 14:09:03]

How will we debug silent failures like this?
Should we have a CHECK instead?

[hongchan, 2017/01/09 21:53:16]

This is a more of WebAudio's practice from the old days. The original author
preferred to have a silent failure rather than a crash. We can certainly change
that if there's a better way to handle this kind of failure.

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

See my other comment on this.

+    return;
+  }

[Raymond Toy, 2017/01/06 22:22:03]

Can we add DCHECKs for these too?  While it's nice to have NOTREACHED, when it
fails, there are 4 conditions that could have caused the failure and we won't
know what caused it.

[hongchan, 2017/01/09 21:53:16]

I think these checks are being overly careful about the conditions. Here's my
thoughts on 4 checks:

1) remainder > m_length: this is redundant because the remainder cannot be
greater than m_length after the line 29, but size_t can go crazy when it wraps
around.

2) m_indexWrite > m_length: this cannot happen after the line 97, but I think
this check is to handle the size_t wrap-around that might happen. I can't think
of such case though.

3) inputBusLength > m_length: is this needed? it depends on the behavior that we
want from this FIFO. Even if the condition is met at the current iteration, the
next iteration easily overflows and wraps around the FIFO if the consumer does
not pull the frames out of it.

4) inputBusLength + m_numberOfFrames: almost same with 3). We can hope for the
best, but the bad thing always can happen when the consumer does not behave
nicely.

I blindly used these checks from the old code, but we might have to reevaluate
them again.

[Raymond Toy, 2017/01/09 22:46:02]

Ok. I'll leave it up to you to decide which checks to remove and which to keep. 
But if we're going to keep some, a DCHECK for the individual check would be
really nice for debugging.

[hongchan, 2017/01/10 21:15:58]

Done.

+
+  for (unsigned i = 0; i < m_dataBus->numberOfChannels(); ++i) {

[o1ka, 2017/01/09 14:09:03]

Can we name it something like "m_fifoBus" to go along with inputBus?

[hongchan, 2017/01/09 21:53:16]

Done. Changed it to fifoDataBus.

+    float* fifoChannel = m_dataBus->channel(i)->mutableData();

[o1ka, 2017/01/09 14:09:03]

Can we name int fifoBusChannel to match inputBusChannel?

[hongchan, 2017/01/09 21:53:16]

Done.

+    const float* inputBusChannel = inputBus->channel(i)->data();
+    if (remainder >= inputBusLength) {
+      // The remainder is big enough for the input data.
+      memcpy(fifoChannel + m_indexWrite, inputBusChannel,
+             inputBusLength * sizeof(*fifoChannel));
+    } else {
+      // The input data overflows the remainder size. Wrap around the index.
+      memcpy(fifoChannel + m_indexWrite, inputBusChannel,
+             remainder * sizeof(*fifoChannel));
+      memcpy(fifoChannel, inputBusChannel + remainder,

[o1ka, 2017/01/09 14:09:03]

What if you overwrite the beginning of data written above?
What if you overwrite data m_indexRead is pointing to?
How is it ensured that m_indexRead always makes sense after wrap-around?

[hongchan, 2017/01/09 21:53:16]

Yes, this is something I wanted to discuss in this CL.
For this to happen, the incoming data must be bigger than m_fifoLength. I don't
think this can be happen.
This means pulling has not happened even after the multiple pushing occurred. We
can assume that the audio device is failing and stop the WebAudio rendering.
The previous FIFO has the assumption of 'a single render call chain' in the same
thread. So push/pull always happens sequentially/synchronously. Now we are about
to have two threads to do the push/pull, so I need to think about this
carefully.

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

You added a check for that, right?
It is possible that pulling has not happen - we call it "audio glitch". Have you
considered rendering with glitches?
Right.

+             (inputBusLength - remainder) * sizeof(*fifoChannel));
+    }
+  }
+
+  // Accumulate the valid number of frames in FIFO.
+  m_numberOfFrames += inputBusLength;

[o1ka, 2017/01/09 14:09:03]

What does |m_numberOfFrames| represent? Can it be more than FIFO length? What
should it be in case of overflow/wrap around above?

[hongchan, 2017/01/09 21:53:16]

It represents 'meaningful frames' that have not been played by the device.
|framesToBePlayed| might be better? Please note that 'inputBusLength +
m_numberOfFrames < m_length' check exists above.

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

|framesAvailable| maybe?

[hongchan, 2017/01/10 21:15:58]

Done.

+  DCHECK_LE(m_numberOfFrames, m_length);
+
+  // Update the write index; wrap it around if necessary.
+  m_indexWrite = (m_indexWrite + inputBusLength) % m_length;

[Raymond Toy, 2017/01/06 22:22:03]

It would be nice if m_length were a power of two.  Should we make that a
requirement?  Or do you think we need to be more flexible?

[hongchan, 2017/01/09 21:53:16]

What's the benefit of the FIFO length being power of two?

[Raymond Toy, 2017/01/09 22:46:02]

Modulo would just be a mask instead of a division.  But I guess it doesn't
really matter here.  We only do it once per call.

+}
+
+// Pull the data out of FIFO to |outputBus|.

[Raymond Toy, 2017/01/06 22:22:03]

Specify what happens if outputBus is longer than framesToPull.

[hongchan, 2017/01/09 21:53:16]

Done.

+void PushPullFIFO::pull(AudioBus* outputBus, size_t framesToPull) {
+  if (!outputBus) {
+    NOTREACHED();

[o1ka, 2017/01/09 14:09:03]

Same question as above for all NOTREACHED().

[hongchan, 2017/01/09 21:53:16]

Done.

+    return;
+  }
+
+  size_t remainder = m_length - m_indexRead;
+

[o1ka, 2017/01/09 14:09:03]

DCHECK(remainder >= 0)?

[hongchan, 2017/01/09 21:53:15]

Done.

[Raymond Toy, 2017/01/09 22:46:02]

That seems wrong.  remainder is a size_t so it's always >= 0.

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

Right, we should have CHECKs before subtraction instead.

+  if (remainder > m_length || m_indexRead > m_length ||
+      framesToPull > m_length || framesToPull > m_numberOfFrames ||
+      outputBus->length() > framesToPull) {
+    NOTREACHED();
+    return;
+  }
+
+  for (unsigned i = 0; i < m_dataBus->numberOfChannels(); ++i) {
+    const float* fifoChannel = m_dataBus->channel(i)->data();
+    float* outputBusChannel = outputBus->channel(i)->mutableData();
+    if (remainder >= framesToPull) {
+      // The remainder is big enough for the frames to pull.
+      memcpy(outputBusChannel, fifoChannel + m_indexRead,
+             framesToPull * sizeof(*fifoChannel));
+    } else {
+      // The frames to pull is bigger than the remainder size.
+      // Wrap around the index.
+      memcpy(outputBusChannel, fifoChannel + m_indexWrite,

[Raymond Toy, 2017/01/06 22:22:03]

You mean indexRead, not indexWrite?  It would be great if we could have a test
that would have caught this.

[hongchan, 2017/01/09 21:53:16]

Done. I am planning for a unit test for the FIFO. That should be a part of this
CL.

+             remainder * sizeof(*fifoChannel));
+      memcpy(outputBusChannel + remainder, fifoChannel,
+             (framesToPull - remainder) * sizeof(*fifoChannel));
+    }
+  }
+
+  // Update the read index; wrap it around if necessary.
+  m_indexRead = (m_indexRead + framesToPull) % m_length;
+
+  // Subtract the valid number of frames in FIFO.
+  DCHECK_GE(m_numberOfFrames, framesToPull);
+  m_numberOfFrames -= framesToPull;
+}
+
+}  // namespace blink