Bug 904784: use a separate critical section for the recording callback r=mwu a=bajaj
authorRandell Jesup <rjesup@jesup.org>
Sat, 07 Sep 2013 23:42:01 -0400
changeset 149290 57b49da2d5a85f5cf7037cfdf1b75e70950cf7c1
parent 149289 f67ee08f57de48c5152356095d586c614ef21e43
child 149291 408704f1c8842d7628af35281332b5aa91aeeb06
push id4227
push userrjesup@wgate.com
push dateMon, 09 Sep 2013 08:18:45 +0000
treeherdermozilla-aurora@57b49da2d5a8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmwu, bajaj
bugs904784
milestone25.0a2
Bug 904784: use a separate critical section for the recording callback r=mwu a=bajaj
media/webrtc/trunk/webrtc/modules/audio_device/audio_device_opensles.cc
media/webrtc/trunk/webrtc/modules/audio_device/audio_device_opensles.h
--- a/media/webrtc/trunk/webrtc/modules/audio_device/audio_device_opensles.cc
+++ b/media/webrtc/trunk/webrtc/modules/audio_device/audio_device_opensles.cc
@@ -31,16 +31,17 @@
 #define WEBRTC_OPENSL_TRACE WEBRTC_TRACE
 #endif
 
 namespace webrtc {
 
 AudioDeviceAndroidOpenSLES::AudioDeviceAndroidOpenSLES(const int32_t id)
     : voe_audio_buffer_(NULL),
       crit_sect_(*CriticalSectionWrapper::CreateCriticalSection()),
+      callback_crit_sect_(*CriticalSectionWrapper::CreateCriticalSection()),
       id_(id),
       sles_engine_(NULL),
       sles_player_(NULL),
       sles_engine_itf_(NULL),
       sles_player_itf_(NULL),
       sles_player_sbq_itf_(NULL),
       sles_output_mixer_(NULL),
       sles_speaker_volume_(NULL),
@@ -270,16 +271,17 @@ int32_t AudioDeviceAndroidOpenSLES::Micr
     bool& available) {
   // We always assume it's available.
   available = true;
   return 0;
 }
 
 int32_t AudioDeviceAndroidOpenSLES::InitMicrophone() {
   CriticalSectionScoped lock(&crit_sect_);
+  CriticalSectionScoped callback_lock(&callback_crit_sect_);
   if (is_recording_) {
     WEBRTC_OPENSL_TRACE(kTraceWarning, kTraceAudioDevice, id_,
                         "  Recording already started");
     return -1;
   }
   if (!is_recording_dev_specified_) {
     WEBRTC_OPENSL_TRACE(kTraceError, kTraceAudioDevice, id_,
                         "  Recording device is not specified");
@@ -934,16 +936,17 @@ int32_t AudioDeviceAndroidOpenSLES::Init
   }
 
   is_rec_initialized_ = true;
   return 0;
 }
 
 int32_t AudioDeviceAndroidOpenSLES::StartRecording() {
   CriticalSectionScoped lock(&crit_sect_);
+  CriticalSectionScoped callback_lock(&callback_crit_sect_);
 
   if (!is_rec_initialized_) {
     WEBRTC_OPENSL_TRACE(kTraceError, kTraceAudioDevice, id_,
                         "  Recording not initialized");
     return -1;
   }
 
   if (is_recording_) {
@@ -1073,29 +1076,31 @@ int32_t AudioDeviceAndroidOpenSLES::Stop
     } else {
       WEBRTC_TRACE(kTraceError, kTraceAudioDevice, id_,
                    "Failed to stop recording thread ");
       return -1;
     }
   }
 
   CriticalSectionScoped lock(&crit_sect_);
+  CriticalSectionScoped callback_lock(&callback_crit_sect_);
   is_rec_initialized_ = false;
   is_recording_ = false;
   rec_warning_ = 0;
   rec_error_ = 0;
 
   return 0;
 }
 
 bool AudioDeviceAndroidOpenSLES::RecordingIsInitialized() const {
   return is_rec_initialized_;
 }
 
 bool AudioDeviceAndroidOpenSLES::Recording() const {
+  CriticalSectionScoped callback_lock(&callback_crit_sect_);
   return is_recording_;
 }
 
 bool AudioDeviceAndroidOpenSLES::PlayoutIsInitialized() const {
   return is_play_initialized_;
 }
 
 int32_t AudioDeviceAndroidOpenSLES::StartPlayout() {
@@ -1344,89 +1349,97 @@ void AudioDeviceAndroidOpenSLES::Recorde
     SLAndroidSimpleBufferQueueItf queue_itf,
     void* p_context) {
   AudioDeviceAndroidOpenSLES* audio_device =
       static_cast<AudioDeviceAndroidOpenSLES*>(p_context);
   audio_device->RecorderSimpleBufferQueueCallbackHandler(queue_itf);
 }
 
 bool AudioDeviceAndroidOpenSLES::RecThreadFuncImpl() {
-  if (is_recording_) {
-    // TODO(leozwang): Add seting correct scheduling and thread priority.
+  // TODO(leozwang): Add seting correct scheduling and thread priority.
 
-    const unsigned int num_samples = mic_sampling_rate_ / 100;
-    const unsigned int num_bytes =
-        N_REC_CHANNELS * num_samples * sizeof(int16_t);
-    const unsigned int total_bytes = num_bytes;
-    int8_t buf[REC_MAX_TEMP_BUF_SIZE_PER_10ms];
+  const unsigned int num_samples = mic_sampling_rate_ / 100;
+  const unsigned int num_bytes =
+    N_REC_CHANNELS * num_samples * sizeof(int16_t);
+  const unsigned int total_bytes = num_bytes;
+  int8_t buf[REC_MAX_TEMP_BUF_SIZE_PER_10ms];
 
-    {
-      CriticalSectionScoped lock(&crit_sect_);
-      if (rec_voe_audio_queue_.size() <= 0) {
-        rec_timer_.Wait(1);
-        return true;
-      }
-
-      int8_t* audio = rec_voe_audio_queue_.front();
-      rec_voe_audio_queue_.pop();
-      memcpy(buf, audio, total_bytes);
-      memset(audio, 0, total_bytes);
-      rec_voe_ready_queue_.push(audio);
+  {
+    // Always grab crit_sect_ first, then callback_crit_sect_
+    // And vice-versa for releasing
+    CriticalSectionScoped lock(&crit_sect_);
+    CriticalSectionScoped callback_lock(&callback_crit_sect_);
+    if (!is_recording_) {
+      return true;
+    }
+    if (rec_voe_audio_queue_.size() <= 0) {
+      rec_timer_.Wait(1);
+      return true;
     }
 
-    UpdateRecordingDelay();
-    voe_audio_buffer_->SetRecordedBuffer(buf, num_samples);
-    voe_audio_buffer_->SetVQEData(playout_delay_, recording_delay_, 0);
-    voe_audio_buffer_->DeliverRecordedData();
+    int8_t* audio = rec_voe_audio_queue_.front();
+    rec_voe_audio_queue_.pop();
+    memcpy(buf, audio, total_bytes);
+    memset(audio, 0, total_bytes);
+    rec_voe_ready_queue_.push(audio);
   }
 
+  UpdateRecordingDelay();
+  voe_audio_buffer_->SetRecordedBuffer(buf, num_samples);
+  voe_audio_buffer_->SetVQEData(playout_delay_, recording_delay_, 0);
+  voe_audio_buffer_->DeliverRecordedData();
+
   return true;
 }
 
 void AudioDeviceAndroidOpenSLES::RecorderSimpleBufferQueueCallbackHandler(
-    SLAndroidSimpleBufferQueueItf queue_itf) {
-  if (is_recording_) {
-    const unsigned int num_samples = mic_sampling_rate_ / 100;
-    const unsigned int num_bytes =
-        N_REC_CHANNELS * num_samples * sizeof(int16_t);
-    const unsigned int total_bytes = num_bytes;
-    int8_t* audio;
+  SLAndroidSimpleBufferQueueItf queue_itf) {
+  const unsigned int num_samples = mic_sampling_rate_ / 100;
+  const unsigned int num_bytes =
+    N_REC_CHANNELS * num_samples * sizeof(int16_t);
+  const unsigned int total_bytes = num_bytes;
+  int8_t* audio;
 
-    {
-      CriticalSectionScoped lock(&crit_sect_);
-      audio = rec_queue_.front();
-      rec_queue_.pop();
-      rec_voe_audio_queue_.push(audio);
+  {
+    // use this instead of crit_sect_ to avoid race against StopRecording()
+    // (which holds crit_sect, and then waits for the thread that calls
+    // this to exit, leading to possible deadlock (bug 904784)
+    CriticalSectionScoped lock(&callback_crit_sect_);
+    if (!is_recording_) {
+      return;
+    }
+    audio = rec_queue_.front();
+    rec_queue_.pop();
+    rec_voe_audio_queue_.push(audio);
 
-      if (rec_voe_ready_queue_.size() <= 0) {
-        // Log Error.
-        rec_error_ = 1;
-        WEBRTC_OPENSL_TRACE(kTraceError, kTraceAudioDevice, id_,
-                            "  Audio Rec thread buffers underrun");
-      } else {
-        audio = rec_voe_ready_queue_.front();
-        rec_voe_ready_queue_.pop();
-      }
+    if (rec_voe_ready_queue_.size() <= 0) {
+      // Log Error.
+      rec_error_ = 1;
+      WEBRTC_OPENSL_TRACE(kTraceError, kTraceAudioDevice, id_,
+                          "  Audio Rec thread buffers underrun");
+    } else {
+      audio = rec_voe_ready_queue_.front();
+      rec_voe_ready_queue_.pop();
     }
+  }
 
-    int32_t res = (*queue_itf)->Enqueue(queue_itf,
-                                              audio,
-                                              total_bytes);
-    if (res != SL_RESULT_SUCCESS) {
-      WEBRTC_OPENSL_TRACE(kTraceWarning, kTraceAudioDevice, id_,
-                          "  recorder callback Enqueue failed, %d", res);
-      rec_warning_ = 1;
-      return;
-    } else {
-      rec_queue_.push(audio);
-    }
+  int32_t res = (*queue_itf)->Enqueue(queue_itf,
+                                      audio,
+                                      total_bytes);
+  if (res != SL_RESULT_SUCCESS) {
+    WEBRTC_OPENSL_TRACE(kTraceWarning, kTraceAudioDevice, id_,
+                        "  recorder callback Enqueue failed, %d", res);
+    rec_warning_ = 1;
+    return;
+  } else {
+    rec_queue_.push(audio);
+  }
 
-    // TODO(leozwang): OpenSL ES doesn't support AudioRecorder
-    // volume control now, add it when it's ready.
-  }
+  // TODO(leozwang): OpenSL ES doesn't support AudioRecorder
+  // volume control now, add it when it's ready.
 }
 
 void AudioDeviceAndroidOpenSLES::CheckErr(SLresult res) {
   if (res != SL_RESULT_SUCCESS) {
     WEBRTC_OPENSL_TRACE(kTraceError, kTraceAudioDevice, id_,
                         "  AudioDeviceAndroidOpenSLES::CheckErr(%lu)", res);
     exit(-1);
   }
--- a/media/webrtc/trunk/webrtc/modules/audio_device/audio_device_opensles.h
+++ b/media/webrtc/trunk/webrtc/modules/audio_device/audio_device_opensles.h
@@ -236,16 +236,20 @@ class AudioDeviceAndroidOpenSLES: public
   void UpdatePlayoutDelay(uint32_t nSamplePlayed);
 
   // Init
   int32_t InitSampleRate();
 
   // Misc
   AudioDeviceBuffer* voe_audio_buffer_;
   CriticalSectionWrapper& crit_sect_;
+  // callback_crit_sect_ is used to lock rec_queue and rec_voe_ready_queue
+  // and also for changing is_recording to false.  If you hold this and
+  // crit_sect_, you must grab crit_sect_ first
+  CriticalSectionWrapper& callback_crit_sect_;
   int32_t id_;
 
   // audio unit
   SLObjectItf sles_engine_;
 
   // playout device
   SLObjectItf sles_player_;
   SLEngineItf sles_engine_itf_;