Introduce PushPullFIFO class and remove other FIFOs

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

+// 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.
+
+#ifndef PushPullFIFO_h
+#define PushPullFIFO_h
+
+#include "platform/audio/AudioBus.h"
+#include "wtf/Allocator.h"
+#include "wtf/ThreadingPrimitives.h"
+
+namespace blink {
+
+class PushPullFIFO {
+  USING_FAST_MALLOC(PushPullFIFO);
+  WTF_MAKE_NONCOPYABLE(PushPullFIFO);
+
+ public:
+  PushPullFIFO(unsigned numberOfChannels, size_t length);
+  ~PushPullFIFO();
+
+  // Pushing audio data is done by the WebAudio render-backing thread. The
+  // priority of this thread is lower than the actual audio device thread,
+  // thus the thread can be blocked when the audio device thread is pulling
+  // data from the FIFO.
+  void push(const AudioBus* inputBus);
+
+  // Pulling audio data is done by the audio device thread. This process must
+  // be non-blocking to ensure the glitch-less audio playback.
+  void pull(AudioBus* outputBus, size_t framesToPull);
+
+  // Returns the number of valid frames in FIFO.

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

This comment is probably better placed as a description of m_framesInFIFO.

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

Done.

+  size_t framesInFIFO() const { return m_framesInFIFO; }

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

This one can be inline.

[o1ka, 2017/01/10 16:41:39]

Ignore this comment, I thought I was in cpp :)

+
+ private:
+  RefPtr<AudioBus> m_fifoDataBus;
+  Mutex m_fifoDataBusLock;
+
+  size_t m_fifoLength;

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

const?

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

Also, do all of these really need to be size_t?  I think int would be a
perfectly fine type.

This is another nit I have with the current code base: size_t everywhere, even
for things we know will never be even close to 64-bits long.  And it's also been
a source of bugs when we take the difference of two size_t thing and save it in
a size_t object and suddenly get a huge number because we didn't realize the
difference could be negative.

[o1ka, 2017/01/10 16:41:39]

Agree. And as I mentioned somewhere we really need to crash (even in release
build) if memory indexes are wrong.

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

I'll investigate corner cases in the next patch set with a unit test.

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

Done.

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

Are you suggesting we change our convention starting from this CL? I am happy to
do that.

+  size_t m_framesInFIFO;

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

I can guess the difference between m_fifoLength and m_framesInFIFO, we should
probably make it clear with comments.  fifoLength is the size of the FIFO, and
framesInFIFO is how many frames of data are actually in the FIFO.  Presumably
fifoLength is also measured in frames?

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

Done. Yes, that's correct.

+  size_t m_indexRead;
+  size_t m_indexWrite;
+};
+
+}  // namespace blink
+
+#endif  // PushPullFIFO_h