Clean up and refactor platform/AudioDestination

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

Index: third_party/WebKit/Source/platform/audio/AudioDestination.h
diff --git a/third_party/WebKit/Source/platform/audio/AudioDestination.h b/third_party/WebKit/Source/platform/audio/AudioDestination.h
index f1d13c3d6ce715714496458f324d1c2cd7459813..f50c70017d0f21467cd7a56cda89c6802820fe1a 100644
--- a/third_party/WebKit/Source/platform/audio/AudioDestination.h
+++ b/third_party/WebKit/Source/platform/audio/AudioDestination.h
@@ -36,7 +36,6 @@
 #include "public/platform/WebVector.h"
 #include "wtf/Allocator.h"
 #include "wtf/Noncopyable.h"
-#include "wtf/PassRefPtr.h"
 #include "wtf/text/WTFString.h"
 #include <memory>
 
@@ -45,8 +44,10 @@ namespace blink {
 class AudioPullFIFO;
 class SecurityOrigin;
 
-// An AudioDestination using Chromium's audio system
-
+// The AudioDestination class is an audio sink interface between the media
+// renderer and the Blink's WebAudio module. It has a FIFO to adapt the

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

s/and the/and/

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

Acknowledged.

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

Done.

+// different frequency of WebAudio renderer and the actual hardware audio

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

"frequency" may not be the right word here.  Maybe "processing block sizes"?

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

Acknowledged.

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

Done.

+// callback.
 class PLATFORM_EXPORT AudioDestination : public WebAudioDevice::RenderCallback,
                                          public AudioSourceProvider {
   USING_FAST_MALLOC(AudioDestination);
@@ -54,66 +55,60 @@ class PLATFORM_EXPORT AudioDestination : public WebAudioDevice::RenderCallback,
 
  public:
   AudioDestination(AudioIOCallback&,
-                   const String& inputDeviceId,
-                   unsigned numberOfInputChannels,
                    unsigned numberOfOutputChannels,
                    float sampleRate,
                    PassRefPtr<SecurityOrigin>);
   ~AudioDestination() override;
 
-  // Pass in (numberOfInputChannels > 0) if live/local audio input is desired.
-  // Port-specific device identification information for live/local input
-  // streams can be passed in the inputDeviceId.
   static std::unique_ptr<AudioDestination> create(
       AudioIOCallback&,
-      const String& inputDeviceId,
-      unsigned numberOfInputChannels,
       unsigned numberOfOutputChannels,
       float sampleRate,
       PassRefPtr<SecurityOrigin>);
 
-  virtual void start();
-  virtual void stop();
-  bool isPlaying() { return m_isPlaying; }
-
-  float sampleRate() const { return m_sampleRate; }
-
-  // WebAudioDevice::RenderCallback
-  void render(const WebVector<float*>& audioData,
+  // The actual render function (WebAudioDevice::RenderCallback) isochronously
+  // invoked by the media renderer.
+  void render(const WebVector<float*>& destinationData,
               size_t numberOfFrames,
               double delay,
               double delayTimestamp,
               size_t priorFramesSkipped) override;
 
-  // AudioSourceProvider
-  void provideInput(AudioBus*, size_t framesToProcess) override;
+  // AudioSourceProvider (FIFO)
+  void provideInput(AudioBus* outputBus, size_t framesToProcess) override;
 
-  static float hardwareSampleRate();
+  virtual void start();

[Raymond Toy, 2016/12/22 17:00:00]

Why move these around?

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

render() and provideInput() are the core methods of this class. Then
start/stop(). Getters are at the end.

+  virtual void stop();
 
   size_t callbackBufferSize() const { return m_callbackBufferSize; }
+  float sampleRate() const { return m_sampleRate; }
+  bool isPlaying() { return m_isPlaying; }

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

Why are these no longer static?  And why change the order?

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

They have never been static. These are regular member functions.

[Raymond Toy, 2016/12/22 17:29:38]

Sorry. I misread this and the following functions.

 
-  // maxChannelCount() returns the total number of output channels of the audio
-  // hardware.  A value of 0 indicates that the number of channels cannot be
-  // configured and that only stereo (2-channel) destinations can be created.
-  // The numberOfOutputChannels parameter of AudioDestination::create() is
-  // allowed to be a value: 1 <= numberOfOutputChannels <= maxChannelCount(),
-  // or if maxChannelCount() equals 0, then numberOfOutputChannels must be 2.

[Raymond Toy, 2016/12/22 17:00:00]

Where did this comment go?

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

First, not sure what these comments mean here. None of logics explained here is
not in this class at all. I believe this is happening in the media renderer
side, and the comment is wrongly placed.

+  // The information from the actual audio hardware. (via Platform::current)
+  static size_t hardwareBufferSize();
+  static float hardwareSampleRate();

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

This is a new public API.  Who needs it?

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

This class does. Here's what I am thinking:

- AudioDestination represents the audio hardware within Blink-WebAudio.
- It proxies the hardware information such as buffer size, sample rate and max
channel count; so we don't have to call Platform::current()-> to query the
hardware information.
- We did not expose the sample rate, so I added it as a static getter.

[Raymond Toy, 2016/12/22 21:46:06]

Yes, but these could be private.
Unless there's already an external user for these, I'd prefer to make these
private methods.  When a user needs them we can make them public then.  Is there
a CL coming very soon that needs these?

[hongchan, 2017/01/03 18:50:26]

I'll make hardwareBufferSize private, but leave hardwareSampleRate as it is.

   static unsigned long maxChannelCount();
 
  private:
-  AudioIOCallback& m_callback;
+  std::unique_ptr<WebAudioDevice> m_webAudioDevice;
   unsigned m_numberOfOutputChannels;
-  RefPtr<AudioBus> m_renderBus;
+  size_t m_callbackBufferSize;
   float m_sampleRate;
   bool m_isPlaying;
-  std::unique_ptr<WebAudioDevice> m_audioDevice;
-  size_t m_callbackBufferSize;

[Raymond Toy, 2016/12/22 17:00:00]

Why move these things around?  The order isn't important is it?

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

The right/meaningful order is certainly important for the readability. I
separated them into 3 blocks: WebAudioDevice, WebAudio rendering, and
OutputPosition.

Like I said, this will be a messy CL but I think it is important to have the
good readability just like other parts of Blink.

 
+  // The render callback function of WebAudio engine. (i.e. DestinationNode)
+  AudioIOCallback& m_callback;
+
+  RefPtr<AudioBus> m_outputBus;
   std::unique_ptr<AudioPullFIFO> m_fifo;
 
   size_t m_framesElapsed;
   AudioIOPosition m_outputPosition;
   base::TimeTicks m_outputPositionReceivedTimestamp;
+
+  // Calculate the optimum buffer size for a given platform. Return false if the
+  // buffer size calculation fails.
+  bool calculateBufferSize();
 };
 
 }  // namespace blink