refactor AudioInputDevice to remove race condition.

content/renderer/media/audio_input_device.cc

Index: content/renderer/media/audio_input_device.cc
===================================================================
--- content/renderer/media/audio_input_device.cc	(revision 95453)
+++ content/renderer/media/audio_input_device.cc	(working copy)
@@ -4,25 +4,31 @@
 
 #include "content/renderer/media/audio_input_device.h"
 
-#include "base/message_loop.h"
 #include "content/common/child_process.h"
 #include "content/common/media/audio_messages.h"
 #include "content/common/view_messages.h"
 #include "content/renderer/render_thread.h"
+#include "content/renderer/media/audio_input_device_event_handler.h"
 #include "media/audio/audio_util.h"
 
 AudioInputDevice::AudioInputDevice(size_t buffer_size,
                                    int channels,
                                    double sample_rate,
+                                   base::MessageLoopProxy* message_loop_proxy,
+                                   AudioInputDeviceEventHandler* handler,

[henrika_dont_use, 2011/08/07 16:52:27]

Just an idea; could it be useful to allow NULL here? Gives user the option to
enable/disable event handling.

[wjia(left Chromium), 2011/08/09 01:40:36]

The client is required to wait for "stopped" signal before it can destruct
AudioInputDevice. Therefore, non-NULL handler is required.

                                    CaptureCallback* callback)
     : buffer_size_(buffer_size),
       channels_(channels),
       bits_per_sample_(16),
       sample_rate_(sample_rate),
       callback_(callback),
+      event_handler_(handler),
       audio_delay_milliseconds_(0),
       volume_(1.0),
+      capture_message_loop_proxy_(message_loop_proxy),
+      state_(kStopped),
       stream_id_(0) {
+  DCHECK(handler);
   filter_ = RenderThread::current()->audio_input_message_filter();
   audio_data_.reserve(channels);
   for (int i = 0; i < channels; ++i) {
@@ -32,47 +38,19 @@
 }
 
 AudioInputDevice::~AudioInputDevice() {
-  // Make sure we have been shut down.
-  DCHECK_EQ(0, stream_id_);
-  Stop();
   for (int i = 0; i < channels_; ++i)
     delete [] audio_data_[i];
 }
 
 bool AudioInputDevice::Start() {
-  // Make sure we don't call Start() more than once.
-  DCHECK_EQ(0, stream_id_);
-  if (stream_id_)
-    return false;
-
-  AudioParameters params;
-  // TODO(henrika): add support for low-latency mode?
-  params.format = AudioParameters::AUDIO_PCM_LINEAR;
-  params.channels = channels_;
-  params.sample_rate = static_cast<int>(sample_rate_);
-  params.bits_per_sample = bits_per_sample_;
-  params.samples_per_packet = buffer_size_;
-
-  ChildProcess::current()->io_message_loop()->PostTask(
-    FROM_HERE,
-    NewRunnableMethod(this, &AudioInputDevice::InitializeOnIOThread, params));
-
+  capture_message_loop_proxy_->PostTask(FROM_HERE,
+      NewRunnableMethod(this, &AudioInputDevice::StartOnCaptureThread));
   return true;
 }
 
 bool AudioInputDevice::Stop() {
-  if (!stream_id_)
-    return false;
-
-  ChildProcess::current()->io_message_loop()->PostTask(
-      FROM_HERE,
-      NewRunnableMethod(this, &AudioInputDevice::ShutDownOnIOThread));
-
-  if (audio_thread_.get()) {
-    socket_->Close();
-    audio_thread_->Join();
-  }
-
+  capture_message_loop_proxy_->PostTask(FROM_HERE,
+      NewRunnableMethod(this, &AudioInputDevice::StopOnCaptureThread));
   return true;
 }
 
@@ -86,36 +64,46 @@
   return false;
 }
 
-void AudioInputDevice::InitializeOnIOThread(const AudioParameters& params) {
-  stream_id_ = filter_->AddDelegate(this);
-  Send(new AudioInputHostMsg_CreateStream(stream_id_, params, true));
-}
+void AudioInputDevice::StartOnCaptureThread() {
+  // Client should call Start() after fully stopped.
+  DCHECK_NE(state_, kStopping);
+  // Make sure we don't call Start() more than once.
+  if (state_ == kStarting || state_ == kStarted)
+    return;
 
-void AudioInputDevice::StartOnIOThread() {
-  if (stream_id_)
-    Send(new AudioInputHostMsg_RecordStream(stream_id_));
+  state_ = kStarting;
+  ChildProcess::current()->io_message_loop_proxy()->PostTask(FROM_HERE,
+      NewRunnableMethod(this, &AudioInputDevice::AddDelegateOnIOThread));
 }
 
-void AudioInputDevice::ShutDownOnIOThread() {
-  // Make sure we don't call shutdown more than once.
-  if (!stream_id_)
+void AudioInputDevice::OnDelegateAddedOnCaptureThread(int32 stream_id) {
+  stream_id_ = stream_id;
+
+  // Got Stop() during starting.
+  if (state_ == kStopping) {
+    event_handler_->OnRecordingStarted();

[henrika_dont_use, 2011/08/07 16:52:27]

Why do we want to signal RecordingStarted here?

[wjia(left Chromium), 2011/08/09 01:40:36]

The interface is: client calls Start() and expects "started", calls Stop() and
expects "stopped".

+    ChildProcess::current()->io_message_loop_proxy()->PostTask(FROM_HERE,
+        NewRunnableMethod(this, &AudioInputDevice::RemoveDelegateOnIOThread,
+                          stream_id_));
+    stream_id_ = 0;
     return;
+  }
 
-  filter_->RemoveDelegate(stream_id_);
-  Send(new AudioInputHostMsg_CloseStream(stream_id_));
-  stream_id_ = 0;
-}
+  AudioParameters params;
+  // TODO(henrika): add support for low-latency mode?
+  params.format = AudioParameters::AUDIO_PCM_LINEAR;
+  params.channels = channels_;
+  params.sample_rate = static_cast<int>(sample_rate_);
+  params.bits_per_sample = bits_per_sample_;
+  params.samples_per_packet = buffer_size_;
 
-void AudioInputDevice::SetVolumeOnIOThread(double volume) {
-  if (stream_id_)
-    Send(new AudioInputHostMsg_SetVolume(stream_id_, volume));
+  Send(new AudioInputHostMsg_CreateStream(stream_id_, params, true));
 }
 
-void AudioInputDevice::OnLowLatencyCreated(
+void AudioInputDevice::StartRecordingOnCaptureThread(
     base::SharedMemoryHandle handle,
     base::SyncSocket::Handle socket_handle,
     uint32 length) {
-  DCHECK(MessageLoop::current() == ChildProcess::current()->io_message_loop());
 #if defined(OS_WIN)
   DCHECK(handle);
   DCHECK(socket_handle);
@@ -125,26 +113,88 @@
 #endif
   DCHECK(length);
 
+  // Got Stop() during starting.
+  if (state_ == kStopping) {
+    base::SharedMemory::CloseHandle(handle);
+    base::SyncSocket socket(socket_handle);
+    event_handler_->OnRecordingStarted();

[henrika_dont_use, 2011/08/07 16:52:27]

Why?

[wjia(left Chromium), 2011/08/09 01:40:36]

Same as above.

+    ChildProcess::current()->io_message_loop_proxy()->PostTask(FROM_HERE,
+        NewRunnableMethod(this, &AudioInputDevice::RemoveDelegateOnIOThread,
+                          stream_id_));
+    return;
+  }
+
   shared_memory_.reset(new base::SharedMemory(handle, false));
   shared_memory_->Map(length);
 
   socket_.reset(new base::SyncSocket(socket_handle));
 
   audio_thread_.reset(
-      new base::DelegateSimpleThread(this, "renderer_audio_input_thread"));
+      new base::DelegateSimpleThread(this, "RendererAudioInputThread"));
   audio_thread_->Start();
 
-  MessageLoop::current()->PostTask(
-      FROM_HERE,
-      NewRunnableMethod(this, &AudioInputDevice::StartOnIOThread));
+  Send(new AudioInputHostMsg_RecordStream(stream_id_));
+  state_ = kStarted;
+  event_handler_->OnRecordingStarted();
 }
 
+void AudioInputDevice::StopOnCaptureThread() {
+  // Make sure we don't call shutdown more than once.
+  if (state_ == kStopping || state_ == kStopped)
+    return;
+
+  // Got Stop() during starting.
+  if (state_ == kStarting) {
+    state_ = kStopping;
+    return;
+  }
+
+  ChildProcess::current()->io_message_loop_proxy()->PostTask(FROM_HERE,
+      NewRunnableMethod(this, &AudioInputDevice::RemoveDelegateOnIOThread,
+                        stream_id_));
+}
+
+void AudioInputDevice::OnDelegateRemovedOnCaptureThread() {
+  if (stream_id_) {
+    Send(new AudioInputHostMsg_CloseStream(stream_id_));
+    stream_id_ = 0;
+  }
+
+  if (audio_thread_.get()) {
+    socket_->Close();
+    audio_thread_->Join();
+    audio_thread_.reset(NULL);
+  }
+
+  state_ = kStopped;
+  event_handler_->OnRecordingStopped();
+}
+
+
+void AudioInputDevice::SetVolumeOnCaptureThread(double volume) {
+  if (stream_id_)
+    Send(new AudioInputHostMsg_SetVolume(stream_id_, volume));
+}
+
+void AudioInputDevice::OnLowLatencyCreated(
+    base::SharedMemoryHandle handle,
+    base::SyncSocket::Handle socket_handle,
+    uint32 length) {
+  DCHECK(ChildProcess::current()->io_message_loop_proxy()->

[henrika_dont_use, 2011/08/07 16:52:27]

Why is this test needed?

[wjia(left Chromium), 2011/08/09 01:40:36]

This is a public function.

+         BelongsToCurrentThread());
+  capture_message_loop_proxy_->PostTask(FROM_HERE,
+      NewRunnableMethod(this, &AudioInputDevice::StartRecordingOnCaptureThread,
+                        handle, socket_handle, length));
+}
+
 void AudioInputDevice::OnVolume(double volume) {
   NOTIMPLEMENTED();
 }
 
 void AudioInputDevice::Send(IPC::Message* message) {
-  filter_->Send(message);
+  ChildProcess::current()->io_message_loop_proxy()->PostTask(FROM_HERE,
+      NewRunnableMethod(filter_.get(),
+                        &AudioInputMessageFilter::Send, message));
 }
 
 // Our audio thread runs here. We receive captured audio samples on
@@ -197,3 +247,19 @@
                      number_of_frames,
                      audio_delay_milliseconds_);
 }
+
+void AudioInputDevice::AddDelegateOnIOThread() {
+  int32 stream_id = filter_->AddDelegate(this);
+  VLOG(1) << "add stream_id = " << stream_id;
+  capture_message_loop_proxy_->PostTask(FROM_HERE,
+      NewRunnableMethod(this, &AudioInputDevice::OnDelegateAddedOnCaptureThread,
+                        stream_id));
+}
+
+void AudioInputDevice::RemoveDelegateOnIOThread(int32 stream_id) {
+  filter_->RemoveDelegate(stream_id);
+  VLOG(1) << "remove stream_id = " << stream_id;
+  capture_message_loop_proxy_->PostTask(FROM_HERE,
+      NewRunnableMethod(this,
+                        &AudioInputDevice::OnDelegateRemovedOnCaptureThread));
+}