Clean up and refactor platform/AudioDestination

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

Index: third_party/WebKit/Source/platform/audio/AudioDestination.cpp
diff --git a/third_party/WebKit/Source/platform/audio/AudioDestination.cpp b/third_party/WebKit/Source/platform/audio/AudioDestination.cpp
index 72444184e89d65df7c8a29bf9fb77efc2bc4af9b..80f24cc3f3539882b405600d0af5522942a2d574 100644
--- a/third_party/WebKit/Source/platform/audio/AudioDestination.cpp
+++ b/third_party/WebKit/Source/platform/audio/AudioDestination.cpp
@@ -29,7 +29,6 @@
 #include "platform/audio/AudioDestination.h"
 
 #include "platform/Histogram.h"
-#include "platform/audio/AudioFIFO.h"
 #include "platform/audio/AudioPullFIFO.h"
 #include "platform/audio/AudioUtilities.h"
 #include "platform/weborigin/SecurityOrigin.h"
@@ -40,138 +39,71 @@
 
 namespace blink {
 
-// Size of the FIFO
-const size_t fifoSize = 8192;
+// FIFO Size; determined arbitrarily.

[Raymond Toy, 2016/12/22 16:59:59]

That's not really true (arbitrarily).  It was an estimate of the largest
callback buffer size that we would ever need. And the FIFO size must be greater
than this.

The current UMA stats indicates that this is, in fact, probably too small. 
There are Android devices out there with a size of 8000 or so.  We might need to
make this larger.  In another CL.

[hongchan, 2016/12/22 17:19:49]

Acknowledged.

[hongchan, 2016/12/22 18:59:45]

Added comments as well. Done.

+const size_t kFIFOSize = 8192;
 
-// Factory method: Chromium-implementation
 std::unique_ptr<AudioDestination> AudioDestination::create(
     AudioIOCallback& callback,
-    const String& inputDeviceId,
-    unsigned numberOfInputChannels,
     unsigned numberOfOutputChannels,
     float sampleRate,
     PassRefPtr<SecurityOrigin> securityOrigin) {
   return WTF::wrapUnique(new AudioDestination(
-      callback, inputDeviceId, numberOfInputChannels, numberOfOutputChannels,
-      sampleRate, std::move(securityOrigin)));
+      callback, numberOfOutputChannels, sampleRate, std::move(securityOrigin)));
 }
 
 AudioDestination::AudioDestination(AudioIOCallback& callback,
-                                   const String& inputDeviceId,
-                                   unsigned numberOfInputChannels,
                                    unsigned numberOfOutputChannels,
                                    float sampleRate,
                                    PassRefPtr<SecurityOrigin> securityOrigin)
-    : m_callback(callback),
-      m_numberOfOutputChannels(numberOfOutputChannels),
-      m_renderBus(AudioBus::create(numberOfOutputChannels,
-                                   AudioUtilities::kRenderQuantumFrames,
-                                   false)),
+    : m_numberOfOutputChannels(numberOfOutputChannels),
       m_sampleRate(sampleRate),
       m_isPlaying(false),
-      m_framesElapsed(0),
-      m_outputPosition() {

[Raymond Toy, 2016/12/22 18:38:39]

What happened to this initializer?

[hongchan, 2016/12/22 18:59:45]

This is a non-pointer member variable. So we don't need to use the initializer -
it will be implicitly initialized at the construction.

-  // Histogram for audioHardwareBufferSize
-  DEFINE_STATIC_LOCAL(SparseHistogram, hardwareBufferSizeHistogram,
-                      ("WebAudio.AudioDestination.HardwareBufferSize"));
-  // Histogram for the actual callback size used.  Typically, this is the same
-  // as audioHardwareBufferSize, but can be adjusted depending on some
-  // heuristics below.
-  DEFINE_STATIC_LOCAL(SparseHistogram, callbackBufferSizeHistogram,
-                      ("WebAudio.AudioDestination.CallbackBufferSize"));
-
-  // Use the optimal buffer size recommended by the audio backend.
-  size_t recommendedHardwareBufferSize =
-      Platform::current()->audioHardwareBufferSize();
-  m_callbackBufferSize = recommendedHardwareBufferSize;
-
-#if OS(ANDROID)
-  // The optimum low-latency hardware buffer size is usually too small on
-  // Android for WebAudio to render without glitching. So, if it is small, use
-  // a larger size. If it was already large, use the requested size.
-  //
-  // Since WebAudio renders in 128-frame blocks, the small buffer sizes (144
-  // for a Galaxy Nexus), cause significant processing jitter. Sometimes
-  // multiple blocks will processed, but other times will not be since the FIFO
-  // can satisfy the request. By using a larger callbackBufferSize, we smooth
-  // out the jitter.
-  const size_t kSmallBufferSize = 1024;
-  const size_t kDefaultCallbackBufferSize = 2048;
-
-  if (m_callbackBufferSize <= kSmallBufferSize)
-    m_callbackBufferSize = kDefaultCallbackBufferSize;
-
-  LOG(INFO) << "audioHardwareBufferSize = " << recommendedHardwareBufferSize;
-  LOG(INFO) << "callbackBufferSize      = " << m_callbackBufferSize;
-#endif
-
-  // Quick exit if the requested size is too large.
-  DCHECK_LE(m_callbackBufferSize + AudioUtilities::kRenderQuantumFrames,
-            fifoSize);
-  if (m_callbackBufferSize + AudioUtilities::kRenderQuantumFrames > fifoSize)
-    return;
-
-  m_audioDevice = WTF::wrapUnique(Platform::current()->createAudioDevice(
-      m_callbackBufferSize, numberOfInputChannels, numberOfOutputChannels,
-      sampleRate, this, inputDeviceId, std::move(securityOrigin)));
-  ASSERT(m_audioDevice);
-
-  // Record the sizes if we successfully created an output device.
-  hardwareBufferSizeHistogram.sample(recommendedHardwareBufferSize);
-  callbackBufferSizeHistogram.sample(m_callbackBufferSize);
-
-  // Create a FIFO to handle the possibility of the callback size
-  // not being a multiple of the render size. If the FIFO already
-  // contains enough data, the data will be provided directly.
-  // Otherwise, the FIFO will call the provider enough times to
-  // satisfy the request for data.
-  m_fifo =
-      WTF::wrapUnique(new AudioPullFIFO(*this, numberOfOutputChannels, fifoSize,
-                                        AudioUtilities::kRenderQuantumFrames));
+      m_callback(callback),
+      m_outputBus(AudioBus::create(numberOfOutputChannels,
+                                   AudioUtilities::kRenderQuantumFrames,
+                                   false)),
+      m_framesElapsed(0) {
+  // Calculate the optimum buffer size first.
+  bool isBufferSizeValid = calculateBufferSize();
+  DCHECK(isBufferSizeValid);
+
+  if (isBufferSizeValid) {
+    // Create WebAudioDevice. blink::WebAudioDevice is designed to support the
+    // local input (e.g. loopback from OS audio system), but Chromium's media
+    // renderer does not support it currently. Thus, we use zero for the number
+    // of input channels.
+    const unsigned numberOfInputChannels = 0;
+    const String inputDeviceId;
+    m_webAudioDevice = WTF::wrapUnique(Platform::current()->createAudioDevice(
+        m_callbackBufferSize, numberOfInputChannels, numberOfOutputChannels,
+        sampleRate, this, inputDeviceId, std::move(securityOrigin)));

[Raymond Toy, 2016/12/22 18:38:39]

Why define these variables that are only used once?  Why not just pass them in
directly to the function?

[hongchan, 2016/12/22 18:59:45]

Sure. I'll do that.

+    DCHECK(m_webAudioDevice);
+
+    // Create a FIFO.
+    m_fifo = WTF::wrapUnique(
+        new AudioPullFIFO(*this, numberOfOutputChannels, kFIFOSize,
+                          AudioUtilities::kRenderQuantumFrames));
+  } else {
+    NOTREACHED();

[Raymond Toy, 2016/12/22 18:38:39]

Is this really necessary? You already have a DCHECK that the size is valid.

[hongchan, 2016/12/22 18:59:45]

I can remove it, however I am not sure what we should do if the buffer size
calculation fails.

+  }
 }
 
 AudioDestination::~AudioDestination() {
   stop();
 }
 
-void AudioDestination::start() {
-  if (!m_isPlaying && m_audioDevice) {
-    m_audioDevice->start();
-    m_isPlaying = true;
-  }
-}
-
-void AudioDestination::stop() {
-  if (m_isPlaying && m_audioDevice) {
-    m_audioDevice->stop();
-    m_isPlaying = false;
-  }
-}
-
-float AudioDestination::hardwareSampleRate() {
-  return static_cast<float>(Platform::current()->audioHardwareSampleRate());
-}
-
-unsigned long AudioDestination::maxChannelCount() {
-  return static_cast<float>(Platform::current()->audioHardwareOutputChannels());
-}
-
-void AudioDestination::render(const WebVector<float*>& audioData,
+void AudioDestination::render(const WebVector<float*>& destinationData,
                               size_t numberOfFrames,
                               double delay,
                               double delayTimestamp,
                               size_t priorFramesSkipped) {
-  bool isNumberOfChannelsGood = audioData.size() == m_numberOfOutputChannels;
-  if (!isNumberOfChannelsGood) {
-    ASSERT_NOT_REACHED();
+  DCHECK_EQ(destinationData.size(), m_numberOfOutputChannels);
+  if (destinationData.size() != m_numberOfOutputChannels)
     return;
-  }
 
-  bool isBufferSizeGood = numberOfFrames == m_callbackBufferSize;
-  if (!isBufferSizeGood) {
-    ASSERT_NOT_REACHED();
+  DCHECK_EQ(numberOfFrames, m_callbackBufferSize);
+  if (numberOfFrames != m_callbackBufferSize)
     return;
-  }
 
   m_framesElapsed -= std::min(m_framesElapsed, priorFramesSkipped);
   double outputPosition =
@@ -180,18 +112,20 @@ void AudioDestination::render(const WebVector<float*>& audioData,
   m_outputPosition.timestamp = delayTimestamp;
   m_outputPositionReceivedTimestamp = base::TimeTicks::Now();
 
+  // Associate the destination data array with the output bus then fill the
+  // FIFO.
   for (unsigned i = 0; i < m_numberOfOutputChannels; ++i)
-    m_renderBus->setChannelMemory(i, audioData[i], numberOfFrames);
-
-  m_fifo->consume(m_renderBus.get(), numberOfFrames);
+    m_outputBus->setChannelMemory(i, destinationData[i], numberOfFrames);
+  m_fifo->consume(m_outputBus.get(), numberOfFrames);
 
   m_framesElapsed += numberOfFrames;
 }
 
-void AudioDestination::provideInput(AudioBus* bus, size_t framesToProcess) {
+void AudioDestination::provideInput(AudioBus* outputBus,
+                                    size_t framesToProcess) {
   AudioIOPosition outputPosition = m_outputPosition;
 
-  // If platfrom buffer is more than two times longer than |framesToProcess|
+  // If platform buffer is more than two times longer than |framesToProcess|
   // we do not want output position to get stuck so we promote it
   // using the elapsed time from the moment it was initially obtained.
   if (m_callbackBufferSize > framesToProcess * 2) {
@@ -206,7 +140,83 @@ void AudioDestination::provideInput(AudioBus* bus, size_t framesToProcess) {
   if (outputPosition.position < 0.0)
     outputPosition.position = 0.0;
 
-  m_callback.render(nullptr, bus, framesToProcess, outputPosition);
+  // To fill the FIFO, start the render call chain of the destination node.
+  m_callback.render(nullptr, outputBus, framesToProcess, outputPosition);
+}
+
+void AudioDestination::start() {
+  if (m_webAudioDevice && !m_isPlaying) {
+    m_webAudioDevice->start();
+    m_isPlaying = true;
+  }
+}
+
+void AudioDestination::stop() {
+  if (m_webAudioDevice && m_isPlaying) {
+    m_webAudioDevice->stop();
+    m_isPlaying = false;
+  }
+}
+
+size_t AudioDestination::hardwareBufferSize() {
+  return Platform::current()->audioHardwareBufferSize();
+}
+
+float AudioDestination::hardwareSampleRate() {
+  return static_cast<float>(Platform::current()->audioHardwareSampleRate());
+}
+
+unsigned long AudioDestination::maxChannelCount() {
+  return static_cast<unsigned long>(
+      Platform::current()->audioHardwareOutputChannels());
+}
+
+bool AudioDestination::calculateBufferSize() {
+  // Use the optimal buffer size recommended by the audio backend.
+  size_t recommendedHardwareBufferSize = hardwareBufferSize();
+  m_callbackBufferSize = recommendedHardwareBufferSize;
+
+#if OS(ANDROID)
+  // The optimum low-latency hardware buffer size is usually too small on
+  // Android for WebAudio to render without glitching. So, if it is small, use a
+  // larger size. If it was already large, use the requested size.
+  //
+  // Since WebAudio renders in 128-frame blocks, the small buffer sizes (144 for
+  // a Galaxy Nexus), cause significant processing jitter. Sometimes multiple
+  // blocks will processed, but other times will not be since the FIFO can
+  // satisfy the request. By using a larger callbackBufferSize, we smooth out
+  // the jitter.
+  const size_t kSmallBufferSize = 1024;
+  const size_t kDefaultCallbackBufferSize = 2048;
+
+  if (m_callbackBufferSize <= kSmallBufferSize)
+    m_callbackBufferSize = kDefaultCallbackBufferSize;
+
+  LOG(INFO) << "audioHardwareBufferSize = " << recommendedHardwareBufferSize;
+  LOG(INFO) << "callbackBufferSize      = " << m_callbackBufferSize;
+#endif
+
+  // Histogram for audioHardwareBufferSize
+  DEFINE_STATIC_LOCAL(SparseHistogram, hardwareBufferSizeHistogram,
+                      ("WebAudio.AudioDestination.HardwareBufferSize"));
+
+  // Histogram for the actual callback size used.  Typically, this is the same
+  // as audioHardwareBufferSize, but can be adjusted depending on some
+  // heuristics below.
+  DEFINE_STATIC_LOCAL(SparseHistogram, callbackBufferSizeHistogram,
+                      ("WebAudio.AudioDestination.CallbackBufferSize"));
+
+  // Record the sizes if we successfully created an output device.
+  hardwareBufferSizeHistogram.sample(recommendedHardwareBufferSize);
+  callbackBufferSizeHistogram.sample(m_callbackBufferSize);
+
+  // Quick exit if the requested size is too large.
+  DCHECK_LE(m_callbackBufferSize + AudioUtilities::kRenderQuantumFrames,
+            kFIFOSize);
+  if (m_callbackBufferSize + AudioUtilities::kRenderQuantumFrames > kFIFOSize)
+    return false;
+
+  return true;
 }
 
 }  // namespace blink