Improve thread creation in plaform/audio/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 e71d5ec1d0382f3196048e39545da9686cd543b4..4507be20150045bb30ecef0493ff3a94718f5936 100644
--- a/third_party/WebKit/Source/platform/audio/AudioDestination.cpp
+++ b/third_party/WebKit/Source/platform/audio/AudioDestination.cpp
@@ -67,8 +67,6 @@ AudioDestination::AudioDestination(AudioIOCallback& callback,
                                    PassRefPtr<SecurityOrigin> security_origin)
     : number_of_output_channels_(number_of_output_channels),
       is_playing_(false),
-      rendering_thread_(WTF::WrapUnique(
-          Platform::Current()->CreateThread("WebAudio Rendering Thread"))),
       fifo_(WTF::WrapUnique(
           new PushPullFIFO(number_of_output_channels, kFIFOSize))),
       output_bus_(AudioBus::Create(number_of_output_channels,
@@ -121,12 +119,15 @@ void AudioDestination::Render(const WebVector<float*>& destination_data,
 
   size_t frames_to_render = fifo_->Pull(output_bus_.Get(), number_of_frames);
 
-  rendering_thread_->GetWebTaskRunner()->PostTask(
-      BLINK_FROM_HERE,
-      CrossThreadBind(&AudioDestination::RequestRenderOnWebThread,
-                      CrossThreadUnretained(this),
-                      number_of_frames, frames_to_render,
-                      delay, delay_timestamp, prior_frames_skipped));
+  // TODO(hongchan): this check might be redundant, so consider removing later.
+  if (frames_to_render != 0 && rendering_thread_) {
+    rendering_thread_->GetWebTaskRunner()->PostTask(
+        BLINK_FROM_HERE,
+        CrossThreadBind(&AudioDestination::RequestRenderOnWebThread,
+                        CrossThreadUnretained(this), number_of_frames,
+                        frames_to_render, delay, delay_timestamp,
+                        prior_frames_skipped));
+  }
 }
 
 void AudioDestination::RequestRenderOnWebThread(size_t frames_requested,
@@ -171,19 +172,44 @@ void AudioDestination::RequestRenderOnWebThread(size_t frames_requested,
 }
 
 void AudioDestination::Start() {
+  DCHECK(WTF::IsMainThread());

[haraken, 2017/05/02 05:02:07]

WTF:: would not be needed.

[hongchan, 2017/05/02 16:49:41]

Done.

+
+  // Start the "audio device" after the rendering thread is ready.
   if (web_audio_device_ && !is_playing_) {

[nhiroki, 2017/05/02 02:08:06]

When is |web_audio_device_| null? It looks like it's created at the ctor and
never nullified.

[hongchan, 2017/05/02 16:49:41]

Hmm. That part I haven't questioned so far. Even if so, I don't think we should
fix it in this CL.

+    rendering_thread_ = WTF::WrapUnique(
+        Platform::Current()->CreateThread("WebAudio Rendering Thread"));
     web_audio_device_->Start();
     is_playing_ = true;
   }
 }
 
 void AudioDestination::Stop() {
+  DCHECK(WTF::IsMainThread());
+
+  // This assumes stopping the "audio device" is synchronous and dumping the
+  // rendering thread is safe after that.
   if (web_audio_device_ && is_playing_) {
     web_audio_device_->Stop();
+    rendering_thread_.reset();
     is_playing_ = false;
   }
 }
 
+size_t AudioDestination::CallbackBufferSize() const {
+  DCHECK(WTF::IsMainThread());
+  return callback_buffer_size_;
+}
+
+bool AudioDestination::IsPlaying() {
+  DCHECK(WTF::IsMainThread());
+  return is_playing_;

[nhiroki, 2017/05/02 02:08:06]

To simplify state management, how about replacing |is_playing| with
|rendering_thread_| and removing |is_playing_| from AudioDestination?

[hongchan, 2017/05/02 16:49:41]

I agree that they are related, but I rather want to change the method name to be
more precise/descriptive. Having an active thread is a bit different from "the
destination is playing."

I believe this CL should just focus on fixing data race and use-after-free
issue.

However, with the feedback in this CL, I should revisit this code and refactor
it in the future.

[Raymond Toy, 2017/05/02 18:11:19]

I agree. But please also file a bug that we should clean up this.  Otherwise,
I'm going to forget.

+}
+
+int AudioDestination::FramesPerBuffer() const {
+  DCHECK(WTF::IsMainThread());
+  return web_audio_device_->FramesPerBuffer();
+}
+
 size_t AudioDestination::HardwareBufferSize() {
   return Platform::Current()->AudioHardwareBufferSize();
 }