Don't call unconfigureWebRTCSession if setConfiguration fails. webrtc:7471

webrtc/modules/audio_device/ios/audio_device_ios.mm

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 699 matching lines @@
     case VoiceProcessingAudioUnit::kStarted:
       RTCLog(@"VPAU state: Started");
       RTC_DCHECK(playing_ || recording_);
       should_stop_audio_unit = !can_play_or_record;
       should_uninitialize_audio_unit = should_stop_audio_unit;
       break;
   }
 
   if (should_initialize_audio_unit) {
     RTCLog(@"Initializing audio unit for UpdateAudioUnit");
-    ConfigureAudioSession();
-    SetupAudioBuffersForActiveAudioSession();
-    if (!audio_unit_->Initialize(playout_parameters_.sample_rate())) {
-      RTCLogError(@"Failed to initialize audio unit.");
+    if (ConfigureAudioSession()) {
+      SetupAudioBuffersForActiveAudioSession();
+      if (!audio_unit_->Initialize(playout_parameters_.sample_rate())) {
+        RTCLogError(@"Failed to initialize audio unit.");
+        return;
+      }
+    } else {
+      RTCLogError(@"Failed to configure audio unit.");

[tkchin_webrtc, 2017/04/18 21:06:33]

misleading - the audio unit is never configured here. Failed to configure audio
session?

will this cause problems though? What if the error is transient? When would
audio unit initialization be attempted again?

Is it better to attempt to setup the audio unit anyway?

[jtt_webrtc, 2017/04/18 21:28:05]

Callers may assume that the audio session has been configured. Should it be
better to fail?
There may be unseen consequences of having the audio unit initialized without
the audio session activated.

[tkchin_webrtc, 2017/04/21 19:52:21]

I'm worried that this will break existing situations where we attempt to
initialize anyway and it works.
If we don't try to start here - when will we actually retry? We can try it if
you think the risk is low.

Please update the text to "Failed to configure audio session" rather than
"Failed to configure audio unit". Maybe do it in ConfigureAudioSession.

[jtt_webrtc, 2017/04/21 20:12:15]

I'll change it so that SetupAudioBuffersForActiveAudioSession(); is called
anyway. 

       return;
     }
   }
 
   if (should_start_audio_unit) {
     RTCLog(@"Starting audio unit for UpdateAudioUnit");
     // Log session settings before trying to start audio streaming.
     RTCAudioSession* session = [RTCAudioSession sharedInstance];
     RTCLog(@"%@", session);
     if (!audio_unit_->Start()) {
@@ skipping 10 matching lines @@
     }
   }
 
   if (should_uninitialize_audio_unit) {
     RTCLog(@"Uninitializing audio unit for UpdateAudioUnit");
     audio_unit_->Uninitialize();
     UnconfigureAudioSession();
   }
 }
 
-void AudioDeviceIOS::ConfigureAudioSession() {
+bool AudioDeviceIOS::ConfigureAudioSession() {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   RTCLog(@"Configuring audio session.");
   if (has_configured_session_) {
     RTCLogWarning(@"Audio session already configured.");
-    return;
+    return false;
   }
   RTCAudioSession* session = [RTCAudioSession sharedInstance];
   [session lockForConfiguration];
-  [session configureWebRTCSession:nil];
+  has_configured_session_ = [session configureWebRTCSession:nil];
   [session unlockForConfiguration];
-  has_configured_session_ = true;
   RTCLog(@"Configured audio session.");

[tkchin_webrtc, 2017/04/21 19:52:21]

This isn't true if the session configuration failed. Use bool.

[jtt_webrtc, 2017/04/21 20:12:16]

Done.

+  return has_configured_session_;
 }
 
 void AudioDeviceIOS::UnconfigureAudioSession() {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   RTCLog(@"Unconfiguring audio session.");
   if (!has_configured_session_) {
     RTCLogWarning(@"Audio session already unconfigured.");
     return;
   }
   RTCAudioSession* session = [RTCAudioSession sharedInstance];
@@ skipping 21 matching lines @@
   NSError* error = nil;
   if (![session beginWebRTCSession:&error]) {
     [session unlockForConfiguration];
     RTCLogError(@"Failed to begin WebRTC session: %@",
                 error.localizedDescription);
     return false;
   }
 
   // If we are ready to play or record, initialize the audio unit.
   if (session.canPlayOrRecord) {
-    ConfigureAudioSession();
-    SetupAudioBuffersForActiveAudioSession();
-    audio_unit_->Initialize(playout_parameters_.sample_rate());
+    if (ConfigureAudioSession()) {
+      SetupAudioBuffersForActiveAudioSession();
+      audio_unit_->Initialize(playout_parameters_.sample_rate());
+    }
   }
 
   // Release the lock.
   [session unlockForConfiguration];
 
-  return true;
+  return has_configured_session_;
 }
 
 void AudioDeviceIOS::ShutdownPlayOrRecord() {
   LOGI() << "ShutdownPlayOrRecord";
 
   // Stop the audio unit to prevent any additional audio callbacks.
   audio_unit_->Stop();
 
   // Close and delete the voice-processing I/O unit.
   audio_unit_.reset();
 
   // Remove audio session notification observers.
   RTCAudioSession* session = [RTCAudioSession sharedInstance];
   [session removeDelegate:audio_session_observer_];
 
   // All I/O should be stopped or paused prior to deactivating the audio
   // session, hence we deactivate as last action.
   [session lockForConfiguration];
   UnconfigureAudioSession();
   [session endWebRTCSession:nil];
   [session unlockForConfiguration];
 }
 
 }  // namespace webrtc