Improved error handling in iOS ADM to avoid race during init

webrtc/modules/audio_device/ios/audio_device_ios.mm

Index: webrtc/modules/audio_device/ios/audio_device_ios.mm
diff --git a/webrtc/modules/audio_device/ios/audio_device_ios.mm b/webrtc/modules/audio_device/ios/audio_device_ios.mm
index 1466d6abca4ee58ce4aa8449adaf0f4a71815e3d..e8dd46d4dedf598bd6266e93a1d8ab8f66f1ac88 100644
--- a/webrtc/modules/audio_device/ios/audio_device_ios.mm
+++ b/webrtc/modules/audio_device/ios/audio_device_ios.mm
@@ -25,6 +25,8 @@
 
 namespace webrtc {
 
+volatile int AudioDeviceIOS::object_count_ = 0;
+
 #define LOGI() LOG(LS_INFO) << "AudioDeviceIOS::"
 
 #define LOG_AND_RETURN_IF_ERROR(error, message) \
@@ -80,7 +82,7 @@ using ios::CheckAndLogError;
 // Activates an audio session suitable for full duplex VoIP sessions when
 // |activate| is true. Also sets the preferred sample rate and IO buffer
 // duration. Deactivates an active audio session if |activate| is set to false.
-static void ActivateAudioSession(AVAudioSession* session, bool activate) {
+static bool ActivateAudioSession(AVAudioSession* session, bool activate) {

[tkchin_webrtc, 2015/11/16 21:30:08]

Would this be suitable for the new rtc::Optional thing? Just a question, not
really related to this CL.

[henrika_webrtc, 2015/11/17 14:46:11]

Not sure if I understand. How would it improve the existing implementation? It
is a three-state bool, right?

   LOG(LS_INFO) << "ActivateAudioSession(" << activate << ")";
   @autoreleasepool {
     NSError* error = nil;
@@ -89,8 +91,7 @@ static void ActivateAudioSession(AVAudioSession* session, bool activate) {
     // Deactivate the audio session and return if |activate| is false.
     if (!activate) {
       success = [session setActive:NO error:&error];
-      RTC_DCHECK(CheckAndLogError(success, error));
-      return;
+      return CheckAndLogError(success, error);
     }
 
     // Use a category which supports simultaneous recording and playback.
@@ -121,7 +122,6 @@ static void ActivateAudioSession(AVAudioSession* session, bool activate) {
     RTC_DCHECK(CheckAndLogError(success, error));
 
     // Set the preferred audio I/O buffer duration, in seconds.
-    // TODO(henrika): add more comments here.
     error = nil;
     success = [session setPreferredIOBufferDuration:kPreferredIOBufferDuration
                                               error:&error];
@@ -131,13 +131,27 @@ static void ActivateAudioSession(AVAudioSession* session, bool activate) {
     // session (e.g. phone call) has higher priority than ours.
     error = nil;
     success = [session setActive:YES error:&error];
-    RTC_DCHECK(CheckAndLogError(success, error));
-    RTC_CHECK(session.isInputAvailable) << "No input path is available!";
+    if (!CheckAndLogError(success, error)) {
+      return false;
+    }
 
-    // Ensure that category and mode are actually activated.
-    RTC_DCHECK(
-        [session.category isEqualToString:AVAudioSessionCategoryPlayAndRecord]);
-    RTC_DCHECK([session.mode isEqualToString:AVAudioSessionModeVoiceChat]);
+    // Ensure that the device currently supports audio input.
+    if (!session.isInputAvailable) {
+      LOG(LS_ERROR) << "No audio input path is available!";
+      return false;
+    }
+
+    // Ensure that the required category and mode are actually activated.
+    if (![session.category
+            isEqualToString:AVAudioSessionCategoryPlayAndRecord]) {
+      LOG(LS_ERROR)
+          << "Failed to set category to AVAudioSessionCategoryPlayAndRecord";
+      return false;
+    }
+    if (![session.mode isEqualToString:AVAudioSessionModeVoiceChat]) {
+      LOG(LS_ERROR) << "Failed to set mode to AVAudioSessionModeVoiceChat";
+      return false;
+    }
 
     // Try to set the preferred number of hardware audio channels. These calls
     // must be done after setting the audio session’s category and mode and
@@ -156,6 +170,7 @@ static void ActivateAudioSession(AVAudioSession* session, bool activate) {
         [session setPreferredOutputNumberOfChannels:kPreferredNumberOfChannels
                                               error:&error];
     RTC_DCHECK(CheckAndLogError(success, error));
+    return true;
   }
 }
 
@@ -200,11 +215,14 @@ AudioDeviceIOS::AudioDeviceIOS()
       play_is_initialized_(false),
       audio_interruption_observer_(nullptr),
       route_change_observer_(nullptr) {
-  LOGI() << "ctor" << ios::GetCurrentThreadDescription();
+  const int count = rtc::AtomicOps::Increment(&object_count_);

[tkchin_webrtc, 2015/11/16 21:30:08]

Is AudioDeviceIOS always created from the same thread? Otherwise wouldn't you
need to lock?

[henrika_webrtc, 2015/11/17 14:46:11]

If I make an AppRTC call, we see:

(audio_device_ios.mm:227): AudioDeviceIOS::ctor[1]<NSThread: 0x124d5d570>{number
= 3, name = (null)}
[009:727] [26115] (audio_device_ios.mm:232): AudioDeviceIOS::~dtor[1]
[042:415] [25375] (audio_device_ios.mm:227): AudioDeviceIOS::ctor[1]<NSThread:
0x124d38dd0>{number = 6, name = (null)}
[043:831] [61191] (audio_device_ios.mm:227): AudioDeviceIOS::ctor[2]<NSThread:
0x128b2ebc0>{number = 7, name = (null)}
[128:947] [61191] (audio_device_ios.mm:232): AudioDeviceIOS::~dtor[2]
[129:053] [25375] (audio_device_ios.mm:232): AudioDeviceIOS::~dtor[1]

hence, it can be created on more than one thread but it is always destructed on
same as created.

Anyow, it does not feel OK with a lock in ctor/dtor since the only usage of this
counter is to disable the audio session when last object dies.

Let's open up a separate discussion if there are potential improvements in this
area?

[tkchin_webrtc, 2015/11/17 21:08:46]

So this count can be potentially inaccurate then. Because it's possible to read
stale value. Unless I'm misunderstanding something?

You can potentially use main queue to guarantee correct order. Do
dispatch_async(dispatch_get_main_queue(), ^{
  <incr/decr>
  <log>
});

Since it's a serial execution queue you won't even need to atomic or volatile
bits.

[henrika_webrtc, 2015/11/18 09:26:04]

Thanks, I am learning something new every day and dispatch queues was news to me
;-)

I was actually very close to an alternative solution (after discussions with
pbos@) using a combo of global lock and avoid counting objetcs but instead keep
track of the actual number of activations. Can most likely use your idea as well
but the pattern is new to me. 

Let me see if I can come up with two alternative solutions. 

+  LOGI() << "ctor[" << count << "]" << ios::GetCurrentThreadDescription();
 }
 
 AudioDeviceIOS::~AudioDeviceIOS() {
-  LOGI() << "~dtor";
+  const int count = rtc::AtomicOps::AcquireLoad(&object_count_);
+  LOGI() << "~dtor[" << count << "]";
+  rtc::AtomicOps::Decrement(&object_count_);
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   Terminate();
 }
@@ -259,7 +277,7 @@ int32_t AudioDeviceIOS::InitPlayout() {
   RTC_DCHECK(!playing_);
   if (!rec_is_initialized_) {
     if (!InitPlayOrRecord()) {
-      LOG_F(LS_ERROR) << "InitPlayOrRecord failed!";
+      LOG_F(LS_ERROR) << "InitPlayOrRecord failed for InitPlayout!";
       return -1;
     }
   }
@@ -275,7 +293,7 @@ int32_t AudioDeviceIOS::InitRecording() {
   RTC_DCHECK(!recording_);
   if (!play_is_initialized_) {
     if (!InitPlayOrRecord()) {
-      LOG_F(LS_ERROR) << "InitPlayOrRecord failed!";
+      LOG_F(LS_ERROR) << "InitPlayOrRecord failed for InitRecording!";
       return -1;
     }
   }
@@ -631,7 +649,7 @@ void AudioDeviceIOS::SetupAudioBuffersForActiveAudioSession() {
 
 bool AudioDeviceIOS::SetupAndInitializeVoiceProcessingAudioUnit() {
   LOGI() << "SetupAndInitializeVoiceProcessingAudioUnit";
-  RTC_DCHECK(!vpio_unit_);
+  RTC_DCHECK(!vpio_unit_) << "VoiceProcessingIO audio unit already exists";
   // Create an audio component description to identify the Voice-Processing
   // I/O audio unit.
   AudioComponentDescription vpio_unit_description;
@@ -645,29 +663,42 @@ bool AudioDeviceIOS::SetupAndInitializeVoiceProcessingAudioUnit() {
       AudioComponentFindNext(nullptr, &vpio_unit_description);
 
   // Create a Voice-Processing IO audio unit.
-  LOG_AND_RETURN_IF_ERROR(
-      AudioComponentInstanceNew(found_vpio_unit_ref, &vpio_unit_),
-      "Failed to create a VoiceProcessingIO audio unit");
+  OSStatus result = noErr;
+  result = AudioComponentInstanceNew(found_vpio_unit_ref, &vpio_unit_);
+  if (result != noErr) {
+    vpio_unit_ = nullptr;
+    LOG(LS_ERROR) << "AudioComponentInstanceNew failed: " << result;
+    return false;
+  }
 
   // A VP I/O unit's bus 1 connects to input hardware (microphone). Enable
   // input on the input scope of the input element.
   AudioUnitElement input_bus = 1;
   UInt32 enable_input = 1;
-  LOG_AND_RETURN_IF_ERROR(
-      AudioUnitSetProperty(vpio_unit_, kAudioOutputUnitProperty_EnableIO,
-                           kAudioUnitScope_Input, input_bus, &enable_input,
-                           sizeof(enable_input)),
-      "Failed to enable input on input scope of input element");
+  result = AudioUnitSetProperty(vpio_unit_, kAudioOutputUnitProperty_EnableIO,
+                                kAudioUnitScope_Input, input_bus, &enable_input,
+                                sizeof(enable_input));
+  if (result != noErr) {
+    DisposeAudioUnit();
+    LOG(LS_ERROR) << "Failed to enable input on input scope of input element: "
+                  << result;
+    return false;
+  }
 
   // A VP I/O unit's bus 0 connects to output hardware (speaker). Enable
   // output on the output scope of the output element.
   AudioUnitElement output_bus = 0;
   UInt32 enable_output = 1;
-  LOG_AND_RETURN_IF_ERROR(
-      AudioUnitSetProperty(vpio_unit_, kAudioOutputUnitProperty_EnableIO,
-                           kAudioUnitScope_Output, output_bus, &enable_output,
-                           sizeof(enable_output)),
-      "Failed to enable output on output scope of output element");
+  result = AudioUnitSetProperty(vpio_unit_, kAudioOutputUnitProperty_EnableIO,
+                                kAudioUnitScope_Output, output_bus,
+                                &enable_output, sizeof(enable_output));
+  if (result != noErr) {
+    DisposeAudioUnit();
+    LOG(LS_ERROR)
+        << "Failed to enable output on output scope of output element: "
+        << result;
+    return false;
+  }
 
   // Set the application formats for input and output:
   // - use same format in both directions
@@ -695,38 +726,55 @@ bool AudioDeviceIOS::SetupAndInitializeVoiceProcessingAudioUnit() {
 #endif
 
   // Set the application format on the output scope of the input element/bus.
-  LOG_AND_RETURN_IF_ERROR(
-      AudioUnitSetProperty(vpio_unit_, kAudioUnitProperty_StreamFormat,
-                           kAudioUnitScope_Output, input_bus,
-                           &application_format, size),
-      "Failed to set application format on output scope of input element");
+  result = AudioUnitSetProperty(vpio_unit_, kAudioUnitProperty_StreamFormat,
+                                kAudioUnitScope_Output, input_bus,
+                                &application_format, size);
+  if (result != noErr) {
+    DisposeAudioUnit();
+    LOG(LS_ERROR)
+        << "Failed to set application format on output scope of input bus: "
+        << result;
+    return false;
+  }
 
   // Set the application format on the input scope of the output element/bus.
-  LOG_AND_RETURN_IF_ERROR(
-      AudioUnitSetProperty(vpio_unit_, kAudioUnitProperty_StreamFormat,
-                           kAudioUnitScope_Input, output_bus,
-                           &application_format, size),
-      "Failed to set application format on input scope of output element");
+  result = AudioUnitSetProperty(vpio_unit_, kAudioUnitProperty_StreamFormat,
+                                kAudioUnitScope_Input, output_bus,
+                                &application_format, size);
+  if (result != noErr) {
+    DisposeAudioUnit();
+    LOG(LS_ERROR)
+        << "Failed to set application format on input scope of output bus: "
+        << result;
+    return false;
+  }
 
   // Specify the callback function that provides audio samples to the audio
   // unit.
   AURenderCallbackStruct render_callback;
   render_callback.inputProc = GetPlayoutData;
   render_callback.inputProcRefCon = this;
-  LOG_AND_RETURN_IF_ERROR(
-      AudioUnitSetProperty(vpio_unit_, kAudioUnitProperty_SetRenderCallback,
-                           kAudioUnitScope_Input, output_bus, &render_callback,
-                           sizeof(render_callback)),
-      "Failed to specify the render callback on the output element");
+  result = AudioUnitSetProperty(
+      vpio_unit_, kAudioUnitProperty_SetRenderCallback, kAudioUnitScope_Input,
+      output_bus, &render_callback, sizeof(render_callback));
+  if (result != noErr) {
+    DisposeAudioUnit();
+    LOG(LS_ERROR) << "Failed to specify the render callback on the output bus: "
+                  << result;
+    return false;
+  }
 
   // Disable AU buffer allocation for the recorder, we allocate our own.
   // TODO(henrika): not sure that it actually saves resource to make this call.
   UInt32 flag = 0;
-  LOG_AND_RETURN_IF_ERROR(
-      AudioUnitSetProperty(vpio_unit_, kAudioUnitProperty_ShouldAllocateBuffer,
-                           kAudioUnitScope_Output, input_bus, &flag,
-                           sizeof(flag)),
-      "Failed to disable buffer allocation on the input element");
+  result = AudioUnitSetProperty(
+      vpio_unit_, kAudioUnitProperty_ShouldAllocateBuffer,
+      kAudioUnitScope_Output, input_bus, &flag, sizeof(flag));
+  if (result != noErr) {
+    DisposeAudioUnit();
+    LOG(LS_ERROR) << "Failed to disable buffer allocation on the input bus: "
+                  << result;
+  }
 
   // Specify the callback to be called by the I/O thread to us when input audio
   // is available. The recorded samples can then be obtained by calling the
@@ -734,16 +782,24 @@ bool AudioDeviceIOS::SetupAndInitializeVoiceProcessingAudioUnit() {
   AURenderCallbackStruct input_callback;
   input_callback.inputProc = RecordedDataIsAvailable;
   input_callback.inputProcRefCon = this;
-  LOG_AND_RETURN_IF_ERROR(
-      AudioUnitSetProperty(vpio_unit_,
-                           kAudioOutputUnitProperty_SetInputCallback,
-                           kAudioUnitScope_Global, input_bus, &input_callback,
-                           sizeof(input_callback)),
-      "Failed to specify the input callback on the input element");
+  result = AudioUnitSetProperty(vpio_unit_,
+                                kAudioOutputUnitProperty_SetInputCallback,
+                                kAudioUnitScope_Global, input_bus,
+                                &input_callback, sizeof(input_callback));
+  if (result != noErr) {
+    DisposeAudioUnit();
+    LOG(LS_ERROR) << "Failed to specify the input callback on the input bus: "
+                  << result;
+  }
 
   // Initialize the Voice-Processing I/O unit instance.
-  LOG_AND_RETURN_IF_ERROR(AudioUnitInitialize(vpio_unit_),
-                          "Failed to initialize the Voice-Processing I/O unit");
+  result = AudioUnitInitialize(vpio_unit_);
+  if (result != noErr) {
+    DisposeAudioUnit();
+    LOG(LS_ERROR) << "Failed to initialize the Voice-Processing I/O unit: "
+                  << result;
+    return false;
+  }
   return true;
 }
 
@@ -782,9 +838,16 @@ bool AudioDeviceIOS::RestartAudioUnitWithNewFormat(float sample_rate) {
 
 bool AudioDeviceIOS::InitPlayOrRecord() {
   LOGI() << "InitPlayOrRecord";
+  // The system provides an audio session object upon launch of an application.
+  // However, we must initialize the session in order to handle interruptions.
+  // Implicit initialization occurs when obtaining a reference to the
+  // AVAudioSession object.
   AVAudioSession* session = [AVAudioSession sharedInstance];
+
   // Activate the audio session and ask for a set of preferred audio parameters.
-  ActivateAudioSession(session, true);
+  if (!ActivateAudioSession(session, true)) {
+    return false;
+  }
 
   // Start observing audio session interruptions and route changes.
   RegisterNotificationObservers();
@@ -794,6 +857,7 @@ bool AudioDeviceIOS::InitPlayOrRecord() {
 
   // Create, setup and initialize a new Voice-Processing I/O unit.
   if (!SetupAndInitializeVoiceProcessingAudioUnit()) {
+    LOG(LS_ERROR) << "SetupAndInitializeVoiceProcessingAudioUnit failed";
     return false;
   }
   return true;
@@ -815,20 +879,31 @@ bool AudioDeviceIOS::ShutdownPlayOrRecord() {
     if (result != noErr) {
       LOG_F(LS_ERROR) << "AudioUnitUninitialize failed: " << result;
     }
-    result = AudioComponentInstanceDispose(vpio_unit_);
-    if (result != noErr) {
-      LOG_F(LS_ERROR) << "AudioComponentInstanceDispose failed: " << result;
-    }
-    vpio_unit_ = nullptr;
+    DisposeAudioUnit();
   }
 
   // All I/O should be stopped or paused prior to deactivating the audio
-  // session, hence we deactivate as last action.
-  AVAudioSession* session = [AVAudioSession sharedInstance];
-  ActivateAudioSession(session, false);
+  // session, hence we deactivate as last action. However, if more than one
+  // object is using the audio session, ensure that only the last object
+  // deactivates.
+  const int count = rtc::AtomicOps::AcquireLoad(&object_count_);
+  if (count == 1) {
+    AVAudioSession* session = [AVAudioSession sharedInstance];
+    ActivateAudioSession(session, false);
+  }
   return true;
 }
 
+void AudioDeviceIOS::DisposeAudioUnit() {
+  if (nullptr == vpio_unit_)
+    return;
+  OSStatus result = noErr;
+  if ((result = AudioComponentInstanceDispose(vpio_unit_)) != noErr) {

[tkchin_webrtc, 2015/11/16 21:30:08]

Why not just:
OSStatus result = udioComponentInstanceDispose(vpio_unit_);
if (result != noError) {
}

[henrika_webrtc, 2015/11/17 14:46:11]

Done.

+    LOG(LS_ERROR) << "AudioComponentInstanceDispose failed:" << result;
+  }
+  vpio_unit_ = nullptr;
+}
+
 OSStatus AudioDeviceIOS::RecordedDataIsAvailable(
     void* in_ref_con,
     AudioUnitRenderActionFlags* io_action_flags,