refactor AudioInputDevice to remove race condition.

content/renderer/media/webrtc_audio_device_impl.cc

Index: content/renderer/media/webrtc_audio_device_impl.cc
===================================================================
--- content/renderer/media/webrtc_audio_device_impl.cc	(revision 95453)
+++ content/renderer/media/webrtc_audio_device_impl.cc	(working copy)
@@ -16,9 +16,9 @@
 static const char kVersion[] = "WebRTC AudioDevice 1.0.0.Chrome";
 
 WebRtcAudioDeviceImpl::WebRtcAudioDeviceImpl(
-  size_t input_buffer_size, size_t output_buffer_size,
-  int input_channels, int output_channels,
-  double input_sample_rate, double output_sample_rate)
+    size_t input_buffer_size, size_t output_buffer_size,
+    int input_channels, int output_channels,
+    double input_sample_rate, double output_sample_rate)
     : audio_transport_callback_(NULL),
       last_error_(AudioDeviceModule::kAdmErrNone),
       input_buffer_size_(input_buffer_size),
@@ -27,14 +27,19 @@
       output_channels_(output_channels),
       input_sample_rate_(input_sample_rate),
       output_sample_rate_(output_sample_rate),
+      adm_thread_("WebRtcAudioDeviceImpl"),
+      recording_stop_event_(false, false),
       initialized_(false),
       playing_(false),
-      recording_(false),
+      recording_state_(kStopped),
       input_delay_ms_(0),
       output_delay_ms_(0),
       last_process_time_(base::TimeTicks::Now()) {
     VLOG(1) << "WebRtcAudioDeviceImpl::WebRtcAudioDeviceImpl()";
 
+    adm_thread_.Start();
+    adm_message_loop_ = adm_thread_.message_loop_proxy();
+
     // Create an AudioInputDevice client if the requested buffer size
     // is an even multiple of 10 milliseconds.
     if (BufferSizeIsValid(input_buffer_size, input_sample_rate)) {
@@ -42,6 +47,8 @@
           input_buffer_size,
           input_channels,
           input_sample_rate,
+          adm_message_loop_.get(),

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

Perhaps add a comment on what we want to achieve by feeding this thread as input
to the AudioInputDevice. 

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

Added some comments in AudioInputDevice constructor.

+          this,
           this);
     }
 
@@ -67,10 +74,10 @@
   VLOG(1) << "WebRtcAudioDeviceImpl::~WebRtcAudioDeviceImpl()";
   if (playing_)
     StopPlayout();
-  if (recording_)
-    StopRecording();
+  StopRecording();
   if (initialized_)
     Terminate();
+  adm_thread_.Stop();
 }
 
 void WebRtcAudioDeviceImpl::Render(
@@ -80,7 +87,9 @@
   DCHECK_LE(number_of_frames, kMaxBufferSize);
 
   // Store the reported audio delay locally.
+  lock_.Acquire();
   output_delay_ms_ = audio_delay_milliseconds;
+  lock_.Release();
 
   const int channels = audio_data.size();
   DCHECK_LE(channels, kMaxChannels);
@@ -154,6 +163,9 @@
   // webrtc::AudioTransport sink. Keep writing until our internal byte
   // buffer is empty.
   while (accumulated_audio_samples < number_of_frames) {
+    lock_.Acquire();
+    int output_delay_ms = output_delay_ms_;
+    lock_.Release();
     // Deliver 10ms of recorded PCM audio.
     // TODO(henrika): add support for analog AGC?
     audio_transport_callback_->RecordedDataIsAvailable(
@@ -162,7 +174,7 @@
         bytes_per_sample_,
         channels,
         samples_per_sec,
-        input_delay_ms_ + output_delay_ms_,
+        input_delay_ms_ + output_delay_ms,
         0,  // clock_drift
         0,  // current_mic_level
         new_mic_level);  // not used
@@ -232,12 +244,30 @@
 int32_t WebRtcAudioDeviceImpl::RegisterAudioCallback(
     webrtc::AudioTransport* audio_callback) {
   VLOG(1) << "RegisterAudioCallback()";
-  if (playing_ || recording_)  {
+  int32_t error = 0;
+  base::WaitableEvent event(false, false);
+  adm_message_loop_->PostTask(FROM_HERE,
+      NewRunnableMethod(
+          this,
+          &WebRtcAudioDeviceImpl::RegisterAudioCallbackOnAdmThread,
+          audio_callback, &error, &event));
+  event.Wait();
+  return error;
+}
+
+void WebRtcAudioDeviceImpl::RegisterAudioCallbackOnAdmThread(
+    webrtc::AudioTransport* audio_callback,
+    int32_t* error,
+    base::WaitableEvent* event) {
+  VLOG(1) << "RegisterAudioCallbackOnAdmThread()";
+  if (playing_ || recording_state_ != kStopped)  {
     LOG(ERROR) << "Unable to (de)register transport during active media";
-    return -1;
+    *error = -1;
+    return;
   }
   audio_transport_callback_ = audio_callback;
-  return 0;
+  *error = 0;
+  event->Signal();
 }
 
 int32_t WebRtcAudioDeviceImpl::Init() {
@@ -378,37 +408,96 @@
 
 int32_t WebRtcAudioDeviceImpl::StartRecording() {
   VLOG(1) << "StartRecording()";
-  LOG_IF(ERROR, !audio_transport_callback_) << "Audio transport is missing";
-  if (!audio_transport_callback_) {
-    LOG(ERROR) << "Audio transport is missing";
-    return -1;
-  }
-  if (recording_) {
+  adm_message_loop_->PostTask(FROM_HERE,
+      NewRunnableMethod(this,
+                        &WebRtcAudioDeviceImpl::StartRecordingOnAdmThread));
+  return 0;
+}
+
+void WebRtcAudioDeviceImpl::StartRecordingOnAdmThread() {
+  VLOG(1) << "StartRecordingOnAdmThread()";
+  // Required to set audio_transport_callback_ before starting recording.
+  DCHECK(audio_transport_callback_);
+  if (recording_state_ != kStopped) {
     // webrtc::VoiceEngine assumes that it is OK to call Start() twice and
     // that the call is ignored the second time.
     LOG(WARNING) << "Recording is already active";
-    return 0;
+    return;
   }
-  recording_ = audio_input_device_->Start();
-  return (recording_ ? 0 : -1);
+  audio_input_device_->Start();
+  recording_state_ = kStarting;
 }
 
 int32_t WebRtcAudioDeviceImpl::StopRecording() {
   VLOG(1) << "StopRecording()";
-  DCHECK(audio_input_device_);
-  if (!recording_) {
+  adm_message_loop_->PostTask(FROM_HERE,
+      NewRunnableMethod(this,
+                        &WebRtcAudioDeviceImpl::StopRecordingOnAdmThread));
+  recording_stop_event_.Wait();
+  return 0;
+}
+
+void WebRtcAudioDeviceImpl::StopRecordingOnAdmThread() {
+  VLOG(1) << "StopRecordingOnAdmThread()";
+  // Client never sees kStopping state since it's always blocked on
+  // StopRecording() call.
+  DCHECK_NE(recording_state_, kStopping);
+  if (recording_state_ == kStopped) {
     // webrtc::VoiceEngine assumes that it is OK to call Stop() just in case.
     LOG(WARNING) << "Recording was already stopped";
-    return 0;
+    recording_stop_event_.Signal();
+    return;
   }
-  recording_ = !audio_input_device_->Stop();
-  return (!recording_ ? 0 : -1);
+  audio_input_device_->Stop();
+  recording_state_ = kStopping;
 }
 
+void WebRtcAudioDeviceImpl::OnRecordingStarted() {
+  VLOG(1) << "OnRecordingStarted()";
+  adm_message_loop_->PostTask(FROM_HERE,

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

Can't we set state in this method directly? It is called from the ADM thread,
right?

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

Setting state here put some assumption on AudioInputDevice implementation.

+      NewRunnableMethod(this,
+                        &WebRtcAudioDeviceImpl::OnRecordingStartedOnAdmThread));
+}
+
+void WebRtcAudioDeviceImpl::OnRecordingStartedOnAdmThread() {
+  VLOG(1) << "OnRecordingStartedOnAdmThread()";
+  recording_state_ = kStarted;
+}
+
+void WebRtcAudioDeviceImpl::OnRecordingStopped() {
+  VLOG(1) << "OnRecordingStopped()";
+  adm_message_loop_->PostTask(FROM_HERE,

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

Same comment as for Start.

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

See comments above.

+      NewRunnableMethod(this,
+                        &WebRtcAudioDeviceImpl::OnRecordingStoppedOnAdmThread));
+}
+
+void WebRtcAudioDeviceImpl::OnRecordingStoppedOnAdmThread() {
+  VLOG(1) << "OnRecordingStoppedOnAdmThread()";
+  recording_state_ = kStopped;
+  // Always signal "stopped" since client must be waiting for it.
+  recording_stop_event_.Signal();
+}
+
 bool WebRtcAudioDeviceImpl::Recording() const {
-  return recording_;
+  bool recording = false;
+  base::WaitableEvent event(false, false);
+  adm_message_loop_->PostTask(FROM_HERE,
+      NewRunnableMethod(this,
+                        &WebRtcAudioDeviceImpl::GetRecordingOnAdmThread,
+                        &recording, &event));
+  event.Wait();
+  return recording;
 }
 
+void WebRtcAudioDeviceImpl::GetRecordingOnAdmThread(
+    bool* recording,
+    base::WaitableEvent* event) const {
+  *recording = recording_state_ == kStarting ||
+               recording_state_ == kStarted ||
+               recording_state_ == kStopping;
+  event->Signal();
+}
+
 int32_t WebRtcAudioDeviceImpl::SetAGC(bool enable) {
   NOTIMPLEMENTED();
   return -1;
@@ -644,9 +733,21 @@
 }
 
 int32_t WebRtcAudioDeviceImpl::RecordingDelay(uint16_t* delay_ms) const {
+  base::WaitableEvent event(false, false);
+  adm_message_loop_->PostTask(FROM_HERE,
+      NewRunnableMethod(this,
+                        &WebRtcAudioDeviceImpl::GetRecordingDelayOnAdmThread,
+                        delay_ms, &event));
+  event.Wait();
+  return 0;
+}
+
+void WebRtcAudioDeviceImpl::GetRecordingDelayOnAdmThread(
+    uint16_t* delay_ms,
+    base::WaitableEvent* event) const {
   // Report the cached output delay value.
   *delay_ms = static_cast<uint16_t>(input_delay_ms_);
-  return 0;
+  event->Signal();
 }
 
 int32_t WebRtcAudioDeviceImpl::CPULoad(uint16_t* load) const {