Refactor AudioInputDevice to remove race conditions and allow more flexible calling sequences.

It also ensures that the AudioInputDevice can be destroyed during an active audio session
without crashing.

This CL is a summary of http://codereview.chromium.org/7497025, i.e., it removes potential
race conditions but in a more "lightweight" manner.

BUG=none
TEST=trybots
Review URL: http://codereview.chromium.org/7661017

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@98198 0039d316-1c4b-4281-b951-d872f2087c98

content/renderer/media/audio_input_device.cc

@@ -5,12 +5,19 @@
 #include "content/renderer/media/audio_input_device.h"
 
 #include "base/message_loop.h"
+#include "base/time.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 "media/audio/audio_util.h"
 
+// Max waiting time for Stop() to complete. If this time limit is passed,
+// we will stop waiting and return false. It ensures that Stop() can't block
+// the calling thread forever.
+static const base::TimeDelta kMaxTimeOut =
+    base::TimeDelta::FromMilliseconds(1000);
+
 AudioInputDevice::AudioInputDevice(size_t buffer_size,
                                    int channels,
                                    double sample_rate,
@@ -32,19 +39,15 @@ AudioInputDevice::AudioInputDevice(size_t buffer_size,
 }
 
 AudioInputDevice::~AudioInputDevice() {
-  // Make sure we have been shut down.
-  DCHECK_EQ(0, stream_id_);
-  Stop();
+  // TODO(henrika): The current design requires that the user calls
+  // Stop before deleting this class.
+  CHECK_EQ(0, stream_id_);
   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;
-
+void AudioInputDevice::Start() {
+  VLOG(1) << "Start()";
   AudioParameters params;
   // TODO(henrika): add support for low-latency mode?
   params.format = AudioParameters::AUDIO_PCM_LINEAR;
@@ -56,21 +59,32 @@ bool AudioInputDevice::Start() {
   ChildProcess::current()->io_message_loop()->PostTask(
       FROM_HERE,
       NewRunnableMethod(this, &AudioInputDevice::InitializeOnIOThread, params));
-
-  return true;
 }
 
 bool AudioInputDevice::Stop() {
-  if (!stream_id_)
-    return false;
+  VLOG(1) << "Stop()";
+  base::WaitableEvent completion(false, false);
 
   ChildProcess::current()->io_message_loop()->PostTask(
       FROM_HERE,
-      NewRunnableMethod(this, &AudioInputDevice::ShutDownOnIOThread));
-
-  if (audio_thread_.get()) {
-    socket_->Close();
-    audio_thread_->Join();
+      NewRunnableMethod(this, &AudioInputDevice::ShutDownOnIOThread,
+                        &completion));
+
+  // We wait here for the IO task to be completed to remove race conflicts
+  // with OnLowLatencyCreated() and to ensure that Stop() acts as a synchronous
+  // function call.
+  if (completion.TimedWait(kMaxTimeOut)) {
+    if (audio_thread_.get()) {
+      // Terminate the main thread function in the audio thread.
+      socket_->Close();
+      // Wait for the audio thread to exit.
+      audio_thread_->Join();
+      // Ensures that we can call Stop() multiple times.
+      audio_thread_.reset(NULL);
+    }
+  } else {
+    LOG(ERROR) << "Failed to shut down audio input on IO thread";
+    return false;
   }
 
   return true;
@@ -87,26 +101,39 @@ bool AudioInputDevice::GetVolume(double* volume) {
 }
 
 void AudioInputDevice::InitializeOnIOThread(const AudioParameters& params) {
+  DCHECK(MessageLoop::current() == ChildProcess::current()->io_message_loop());
+  // Make sure we don't call Start() more than once.
+  DCHECK_EQ(0, stream_id_);
+  if (stream_id_)
+    return;
+
   stream_id_ = filter_->AddDelegate(this);
   Send(new AudioInputHostMsg_CreateStream(stream_id_, params, true));
 }
 
 void AudioInputDevice::StartOnIOThread() {
+  DCHECK(MessageLoop::current() == ChildProcess::current()->io_message_loop());
   if (stream_id_)
     Send(new AudioInputHostMsg_RecordStream(stream_id_));
 }
 
-void AudioInputDevice::ShutDownOnIOThread() {
+void AudioInputDevice::ShutDownOnIOThread(base::WaitableEvent* completion) {
+  DCHECK(MessageLoop::current() == ChildProcess::current()->io_message_loop());
   // Make sure we don't call shutdown more than once.
-  if (!stream_id_)
+  if (!stream_id_) {
+    completion->Signal();
     return;
+  }
 
   filter_->RemoveDelegate(stream_id_);
   Send(new AudioInputHostMsg_CloseStream(stream_id_));
   stream_id_ = 0;
+
+  completion->Signal();
 }
 
 void AudioInputDevice::SetVolumeOnIOThread(double volume) {
+  DCHECK(MessageLoop::current() == ChildProcess::current()->io_message_loop());
   if (stream_id_)
     Send(new AudioInputHostMsg_SetVolume(stream_id_, volume));
 }
@@ -125,13 +152,21 @@ void AudioInputDevice::OnLowLatencyCreated(
 #endif
   DCHECK(length);
 
+  VLOG(1) << "OnLowLatencyCreated (stream_id=" << stream_id_ << ")";
+  // Takes care of the case when Stop() is called before OnLowLatencyCreated().
+  if (!stream_id_) {
+    base::SharedMemory::CloseHandle(handle);
+    base::SyncSocket socket(socket_handle);
+    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(
@@ -158,7 +193,7 @@ void AudioInputDevice::Run() {
 
   while (sizeof(pending_data) == socket_->Receive(&pending_data,
                                                   sizeof(pending_data)) &&
-         pending_data >= 0) {
+                                                  pending_data >= 0) {
     // TODO(henrika): investigate the provided |pending_data| value
     // and ensure that it is actually an accurate delay estimation.
 

content/renderer/media/audio_input_device.h

@@ -5,7 +5,7 @@
 // Low-latency audio capturing unit utilizing audio input stream provided
 // by browser process through IPC.
 //
-// Relationship of classes.
+// Relationship of classes:
 //
 //  AudioInputController                 AudioInputDevice
 //           ^                                  ^
@@ -19,7 +19,7 @@
 // an AudioInputDevice::CaptureCallback at construction and will be called
 // by the AudioInputDevice with recorded audio from the underlying audio layers.
 //
-// State sequences.
+// State sequences:
 //
 //            Task [IO thread]                  IPC [IO thread]
 //
@@ -32,6 +32,7 @@
 // Stop --> ShutDownOnIOThread ------>  AudioInputHostMsg_CloseStream -> Close
 //
 // This class utilizes three threads during its lifetime, namely:
+//
 // 1. Creating thread.
 //    Must be the main render thread. Start and Stop should be called on
 //    this thread.
@@ -42,6 +43,12 @@
 //    Responsible for calling the CaptrureCallback and feed audio samples from
 //    the audio layer in the browser process using sync sockets and shared
 //    memory.
+//
+// Implementation notes:
+//
+// - Start() is asynchronous/non-blocking.
+// - Stop() is synchronous/blocking.
+// - The user must call Stop() before deleting the class instance.
 
 #ifndef CONTENT_RENDERER_MEDIA_AUDIO_INPUT_DEVICE_H_
 #define CONTENT_RENDERER_MEDIA_AUDIO_INPUT_DEVICE_H_
@@ -59,6 +66,9 @@ struct AudioParameters;
 
 // TODO(henrika): This class is based on the AudioDevice class and it has
 // many components in common. Investigate potential for re-factoring.
+// TODO(henrika): Add support for event handling (e.g. OnStateChanged,
+// OnCaptureStopped etc.) and ensure that we can deliver these notifications
+// to any clients using this class.
 class AudioInputDevice
     : public AudioInputMessageFilter::Delegate,
       public base::DelegateSimpleThread::Delegate,
@@ -80,10 +90,13 @@ class AudioInputDevice
                    CaptureCallback* callback);
   virtual ~AudioInputDevice();
 
-  // Starts audio capturing. Returns |true| on success.
-  bool Start();
+  // Starts audio capturing. This method is asynchronous/non-blocking.
+  // TODO(henrika): add support for notification when recording has started.
+  void Start();
 
-  // Stops audio capturing. Returns |true| on success.
+  // Stops audio capturing. This method is synchronous/blocking.
+  // Returns |true| on success.
+  // TODO(henrika): add support for notification when recording has stopped.
   bool Stop();
 
   // Sets the capture volume scaling, with range [0.0, 1.0] inclusive.
@@ -111,7 +124,7 @@ class AudioInputDevice
   // sends IPC messages on that thread.
   void InitializeOnIOThread(const AudioParameters& params);
   void StartOnIOThread();
-  void ShutDownOnIOThread();
+  void ShutDownOnIOThread(base::WaitableEvent* completion);
   void SetVolumeOnIOThread(double volume);
 
   void Send(IPC::Message* message);

content/renderer/media/audio_input_message_filter.cc

@@ -71,15 +71,17 @@ void AudioInputMessageFilter::OnLowLatencyStreamCreated(
     base::FileDescriptor socket_descriptor,
 #endif
     uint32 length) {
+#if !defined(OS_WIN)
+  base::SyncSocket::Handle socket_handle = socket_descriptor.fd;
+#endif
   Delegate* delegate = delegates_.Lookup(stream_id);
   if (!delegate) {
     DLOG(WARNING) << "Got audio stream event for a non-existent or removed"
-        " audio capturer.";
+        " audio capturer (stream_id=" << stream_id << ").";
+    base::SharedMemory::CloseHandle(handle);
+    base::SyncSocket socket(socket_handle);
     return;
   }
-#if !defined(OS_WIN)
-  base::SyncSocket::Handle socket_handle = socket_descriptor.fd;
-#endif
   // Forward message to the stream delegate.
   delegate->OnLowLatencyCreated(handle, socket_handle, length);
 }
@@ -99,5 +101,6 @@ int32 AudioInputMessageFilter::AddDelegate(Delegate* delegate) {
 }
 
 void AudioInputMessageFilter::RemoveDelegate(int32 id) {
+  VLOG(1) << "AudioInputMessageFilter::RemoveDelegate(id=" << id << ")";
   delegates_.Remove(id);
 }

content/renderer/media/webrtc_audio_device_impl.cc

@@ -389,8 +389,9 @@ int32_t WebRtcAudioDeviceImpl::StartRecording() {
     LOG(WARNING) << "Recording is already active";
     return 0;
   }
-  recording_ = audio_input_device_->Start();
-  return (recording_ ? 0 : -1);
+  audio_input_device_->Start();
+  recording_ = true;
+  return 0;
 }
 
 int32_t WebRtcAudioDeviceImpl::StopRecording() {