Bug 904784: use a separate critical section for the recording callback r=mwu
authorRandell Jesup <rjesup@jesup.org>
Sat, 07 Sep 2013 23:42:01 -0400
changeset 146075 69e78915fc92cb08b22f29eeaaa87971787a291f
parent 146074 6f837a184a9066f43f7bb962d97236cc9d5614dd
child 146076 9e839e0432ebed639ed1af98601d70e4913e8eaa
push id1
push userroot
push dateMon, 20 Oct 2014 17:29:22 +0000
reviewersmwu
bugs904784
milestone26.0a1
Bug 904784: use a separate critical section for the recording callback r=mwu
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),
@@ -271,16 +272,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");
@@ -935,16 +937,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_) {
@@ -1074,29 +1077,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() {
@@ -1361,50 +1366,59 @@ bool AudioDeviceAndroidOpenSLES::RecThre
   // 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];
 
+  // Always grab crit_sect_ first, then callback_crit_sect_
+  // And vice-versa for releasing
   crit_sect_.Enter();
+  callback_crit_sect_.Enter();
   while (is_recording_) {
     if (rec_voe_audio_queue_.size() <= 0) {
+      callback_crit_sect_.Leave();
       crit_sect_.Leave();
       // Wait for max 40ms for incoming audio data before looping the
       // poll and checking for ::Stop() being called (which waits for us
       // to return).  Actual time between audio callbacks will vary, but
       // is based on AudioRecord min buffer sizes, which may be 15-80ms
       // or more, depending on sampling frequency, but typically will be
       // 25ish ms at 44100Hz.  We also don't want to take "too long" to
       // exit after ::Stop().  This value of 40ms is arbitrary.
       rec_timer_.Wait(40);
       crit_sect_.Enter();
+      callback_crit_sect_.Enter();
       if (rec_voe_audio_queue_.size() <= 0) {
         // still no audio data; check for ::Stop()
+        callback_crit_sect_.Leave();
         crit_sect_.Leave();
         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);
 
     voe_audio_buffer_->SetRecordedBuffer(buf, num_samples);
     voe_audio_buffer_->SetVQEData(playout_delay_, recording_delay_, 0);
 
     // All other implementations UnLock around DeliverRecordedData() only
+    callback_crit_sect_.Leave();
     crit_sect_.Leave();
     voe_audio_buffer_->DeliverRecordedData();
     crit_sect_.Enter();
+    callback_crit_sect_.Enter();
   }
+  callback_crit_sect_.Leave();
   crit_sect_.Leave();
 
   // if is_recording is false, we either *just* were started, or for some reason
   // Stop() failed to get us to return for 10 seconds - and when we return here,
   // Stop will kill us anyways.
   rec_timer_.Wait(1);
 
   return true;
@@ -1415,17 +1429,20 @@ void AudioDeviceAndroidOpenSLES::Recorde
 
   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_);
+    // 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);
     rec_timer_.Set(); // wake up record thread to process it
 
--- a/media/webrtc/trunk/webrtc/modules/audio_device/audio_device_opensles.h
+++ b/media/webrtc/trunk/webrtc/modules/audio_device/audio_device_opensles.h
@@ -235,16 +235,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_;