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 id33459
push userrjesup@wgate.com
push dateSun, 08 Sep 2013 03:45:13 +0000
treeherdermozilla-inbound@69e78915fc92 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmwu
bugs904784
milestone26.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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_;