Reland "Replace RecursiveCriticalSection with Mutex in RTCAudioSession."

This is a reland of f8da43d179043f1df2e1c3e2c49494bc23f4ec28

Original change's description:
> Replace RecursiveCriticalSection with Mutex in RTCAudioSession.
>
> Bug: webrtc:11567
> Change-Id: I2a2ddbce57d070d6cbad5a64defb4c27be77a665
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/206472
> Reviewed-by: Harald Alvestrand <hta@webrtc.org>
> Reviewed-by: Kári Helgason <kthelgason@webrtc.org>
> Commit-Queue: Niels Moller <nisse@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#33259}

Bug: webrtc:11567
Change-Id: I4f7235dd164d8f698fe0bedea8c5dca50849f6d3
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/207432
Reviewed-by: Markus Handell <handellm@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Henrik Andreassson <henrika@webrtc.org>
Commit-Queue: Niels Moller <nisse@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33302}

sdk/BUILD.gn

@@ -334,6 +334,7 @@ if (is_ios || is_mac) {
           "../rtc_base",
           "../rtc_base:checks",
           "../rtc_base:rtc_base_approved",
+          "../rtc_base/synchronization:mutex",
         ]
       }
 

sdk/objc/components/audio/RTCAudioSession+Configuration.mm

@@ -43,9 +43,6 @@
   if (outError) {
     *outError = nil;
   }
-  if (![self checkLock:outError]) {
-    return NO;
-  }
 
   // Provide an error even if there isn't one so we can log it. We will not
   // return immediately on error in this function and instead try to set

sdk/objc/components/audio/RTCAudioSession+Private.h

@@ -35,8 +35,6 @@ NS_ASSUME_NONNULL_BEGIN
  */
 @property(nonatomic, assign) BOOL isInterrupted;
 
-- (BOOL)checkLock:(NSError **)outError;
-
 /** Adds the delegate to the list of delegates, and places it at the front of
  *  the list. This delegate will be notified before other delegates of
  *  audio events.

sdk/objc/components/audio/RTCAudioSession.mm

@@ -16,7 +16,7 @@
 
 #include "rtc_base/atomic_ops.h"
 #include "rtc_base/checks.h"
-#include "rtc_base/deprecated/recursive_critical_section.h"
+#include "rtc_base/synchronization/mutex.h"
 
 #import "RTCAudioSessionConfiguration.h"
 #import "base/RTCLogging.h"
@@ -35,10 +35,9 @@ NSString * const kRTCAudioSessionOutputVolumeSelector = @"outputVolume";
 // TODO(tkchin): Consider more granular locking. We're not expecting a lot of
 // lock contention so coarse locks should be fine for now.
 @implementation RTC_OBJC_TYPE (RTCAudioSession) {
-  rtc::RecursiveCriticalSection _crit;
+  webrtc::Mutex _mutex;
   AVAudioSession *_session;
   volatile int _activationCount;
-  volatile int _lockRecursionCount;
   volatile int _webRTCSessionCount;
   BOOL _isActive;
   BOOL _useManualAudio;
@@ -151,10 +150,6 @@ NSString * const kRTCAudioSessionOutputVolumeSelector = @"outputVolume";
   }
 }
 
-- (BOOL)isLocked {
-  return _lockRecursionCount > 0;
-}
-
 - (void)setUseManualAudio:(BOOL)useManualAudio {
   @synchronized(self) {
     if (_useManualAudio == useManualAudio) {
@@ -234,20 +229,11 @@ NSString * const kRTCAudioSessionOutputVolumeSelector = @"outputVolume";
 #pragma clang diagnostic ignored "-Wthread-safety-analysis"
 
 - (void)lockForConfiguration {
-  _crit.Enter();
+  _mutex.Lock();
-  rtc::AtomicOps::Increment(&_lockRecursionCount);
 }
 
 - (void)unlockForConfiguration {
-  // Don't let threads other than the one that called lockForConfiguration
+  _mutex.Unlock();
-  // unlock.
-  if (_crit.TryEnter()) {
-    rtc::AtomicOps::Decrement(&_lockRecursionCount);
-    // One unlock for the tryLock, and another one to actually unlock. If this
-    // was called without anyone calling lock, we will hit an assertion.
-    _crit.Leave();
-    _crit.Leave();
-  }
 }
 
 #pragma clang diagnostic pop
@@ -346,13 +332,8 @@ NSString * const kRTCAudioSessionOutputVolumeSelector = @"outputVolume";
   return self.session.preferredIOBufferDuration;
 }
 
-// TODO(tkchin): Simplify the amount of locking happening here. Likely that we
-// can just do atomic increments / decrements.
 - (BOOL)setActive:(BOOL)active
             error:(NSError **)outError {
-  if (![self checkLock:outError]) {
-    return NO;
-  }
   int activationCount = _activationCount;
   if (!active && activationCount == 0) {
     RTCLogWarning(@"Attempting to deactivate without prior activation.");
@@ -409,85 +390,52 @@ NSString * const kRTCAudioSessionOutputVolumeSelector = @"outputVolume";
 - (BOOL)setCategory:(NSString *)category
         withOptions:(AVAudioSessionCategoryOptions)options
               error:(NSError **)outError {
-  if (![self checkLock:outError]) {
-    return NO;
-  }
   return [self.session setCategory:category withOptions:options error:outError];
 }
 
 - (BOOL)setMode:(NSString *)mode error:(NSError **)outError {
-  if (![self checkLock:outError]) {
-    return NO;
-  }
   return [self.session setMode:mode error:outError];
 }
 
 - (BOOL)setInputGain:(float)gain error:(NSError **)outError {
-  if (![self checkLock:outError]) {
-    return NO;
-  }
   return [self.session setInputGain:gain error:outError];
 }
 
 - (BOOL)setPreferredSampleRate:(double)sampleRate error:(NSError **)outError {
-  if (![self checkLock:outError]) {
-    return NO;
-  }
   return [self.session setPreferredSampleRate:sampleRate error:outError];
 }
 
 - (BOOL)setPreferredIOBufferDuration:(NSTimeInterval)duration
                                error:(NSError **)outError {
-  if (![self checkLock:outError]) {
-    return NO;
-  }
   return [self.session setPreferredIOBufferDuration:duration error:outError];
 }
 
 - (BOOL)setPreferredInputNumberOfChannels:(NSInteger)count
                                     error:(NSError **)outError {
-  if (![self checkLock:outError]) {
-    return NO;
-  }
   return [self.session setPreferredInputNumberOfChannels:count error:outError];
 }
 - (BOOL)setPreferredOutputNumberOfChannels:(NSInteger)count
                                      error:(NSError **)outError {
-  if (![self checkLock:outError]) {
-    return NO;
-  }
   return [self.session setPreferredOutputNumberOfChannels:count error:outError];
 }
 
 - (BOOL)overrideOutputAudioPort:(AVAudioSessionPortOverride)portOverride
                           error:(NSError **)outError {
-  if (![self checkLock:outError]) {
-    return NO;
-  }
   return [self.session overrideOutputAudioPort:portOverride error:outError];
 }
 
 - (BOOL)setPreferredInput:(AVAudioSessionPortDescription *)inPort
                     error:(NSError **)outError {
-  if (![self checkLock:outError]) {
-    return NO;
-  }
   return [self.session setPreferredInput:inPort error:outError];
 }
 
 - (BOOL)setInputDataSource:(AVAudioSessionDataSourceDescription *)dataSource
                      error:(NSError **)outError {
-  if (![self checkLock:outError]) {
-    return NO;
-  }
   return [self.session setInputDataSource:dataSource error:outError];
 }
 
 - (BOOL)setOutputDataSource:(AVAudioSessionDataSourceDescription *)dataSource
                       error:(NSError **)outError {
-  if (![self checkLock:outError]) {
-    return NO;
-  }
   return [self.session setOutputDataSource:dataSource error:outError];
 }
 
@@ -608,18 +556,6 @@ NSString * const kRTCAudioSessionOutputVolumeSelector = @"outputVolume";
 
 #pragma mark - Private
 
-+ (NSError *)lockError {
-  NSDictionary *userInfo = @{
-    NSLocalizedDescriptionKey:
-        @"Must call lockForConfiguration before calling this method."
-  };
-  NSError *error =
-      [[NSError alloc] initWithDomain:kRTCAudioSessionErrorDomain
-                                 code:kRTCAudioSessionErrorLockRequired
-                             userInfo:userInfo];
-  return error;
-}
-
 - (std::vector<__weak id<RTC_OBJC_TYPE(RTCAudioSessionDelegate)> >)delegates {
   @synchronized(self) {
     // Note: this returns a copy.
@@ -681,25 +617,10 @@ NSString * const kRTCAudioSessionOutputVolumeSelector = @"outputVolume";
   }
 }
 
-- (BOOL)checkLock:(NSError **)outError {
-  // Check ivar instead of trying to acquire lock so that we won't accidentally
-  // acquire lock if it hasn't already been called.
-  if (!self.isLocked) {
-    if (outError) {
-      *outError = [RTC_OBJC_TYPE(RTCAudioSession) lockError];
-    }
-    return NO;
-  }
-  return YES;
-}
-
 - (BOOL)beginWebRTCSession:(NSError **)outError {
   if (outError) {
     *outError = nil;
   }
-  if (![self checkLock:outError]) {
-    return NO;
-  }
   rtc::AtomicOps::Increment(&_webRTCSessionCount);
   [self notifyDidStartPlayOrRecord];
   return YES;
@@ -709,9 +630,6 @@ NSString * const kRTCAudioSessionOutputVolumeSelector = @"outputVolume";
   if (outError) {
     *outError = nil;
   }
-  if (![self checkLock:outError]) {
-    return NO;
-  }
   rtc::AtomicOps::Decrement(&_webRTCSessionCount);
   [self notifyDidStopPlayOrRecord];
   return YES;
@@ -721,9 +639,6 @@ NSString * const kRTCAudioSessionOutputVolumeSelector = @"outputVolume";
   if (outError) {
     *outError = nil;
   }
-  if (![self checkLock:outError]) {
-    return NO;
-  }
   RTCLog(@"Configuring audio session for WebRTC.");
 
   // Configure the AVAudioSession and activate it.
@@ -784,9 +699,6 @@ NSString * const kRTCAudioSessionOutputVolumeSelector = @"outputVolume";
   if (outError) {
     *outError = nil;
   }
-  if (![self checkLock:outError]) {
-    return NO;
-  }
   RTCLog(@"Unconfiguring audio session for WebRTC.");
   [self setActive:NO error:outError];
 

sdk/objc/native/src/audio/audio_device_ios.h

@@ -192,6 +192,10 @@ class AudioDeviceIOS : public AudioDeviceGeneric,
 
   // Configures the audio session for WebRTC.
   bool ConfigureAudioSession();
+
+  // Like above, but requires caller to already hold session lock.
+  bool ConfigureAudioSessionLocked();
+
   // Unconfigures the audio session.
   void UnconfigureAudioSession();
 

sdk/objc/native/src/audio/audio_device_ios.mm

@@ -844,6 +844,24 @@ bool AudioDeviceIOS::ConfigureAudioSession() {
   return success;
 }
 
+bool AudioDeviceIOS::ConfigureAudioSessionLocked() {
+  RTC_DCHECK_RUN_ON(&thread_checker_);
+  RTCLog(@"Configuring audio session.");
+  if (has_configured_session_) {
+    RTCLogWarning(@"Audio session already configured.");
+    return false;
+  }
+  RTC_OBJC_TYPE(RTCAudioSession)* session = [RTC_OBJC_TYPE(RTCAudioSession) sharedInstance];
+  bool success = [session configureWebRTCSession:nil];
+  if (success) {
+    has_configured_session_ = true;
+    RTCLog(@"Configured audio session.");
+  } else {
+    RTCLog(@"Failed to configure audio session.");
+  }
+  return success;
+}
+
 void AudioDeviceIOS::UnconfigureAudioSession() {
   RTC_DCHECK_RUN_ON(&thread_checker_);
   RTCLog(@"Unconfiguring audio session.");
@@ -887,7 +905,7 @@ bool AudioDeviceIOS::InitPlayOrRecord() {
   // If we are ready to play or record, and if the audio session can be
   // configured, then initialize the audio unit.
   if (session.canPlayOrRecord) {
-    if (!ConfigureAudioSession()) {
+    if (!ConfigureAudioSessionLocked()) {
       // One possible reason for failure is if an attempt was made to use the
       // audio session during or after a Media Services failure.
       // See AVAudioSessionErrorCodeMediaServicesFailed for details.

sdk/objc/unittests/RTCAudioSessionTest.mm

@@ -113,26 +113,10 @@
 
 @interface RTCAudioSessionTest : NSObject
 
-- (void)testLockForConfiguration;
-
 @end
 
 @implementation RTCAudioSessionTest
 
-- (void)testLockForConfiguration {
-  RTC_OBJC_TYPE(RTCAudioSession) *session = [RTC_OBJC_TYPE(RTCAudioSession) sharedInstance];
-
-  for (size_t i = 0; i < 2; i++) {
-    [session lockForConfiguration];
-    EXPECT_TRUE(session.isLocked);
-  }
-  for (size_t i = 0; i < 2; i++) {
-    EXPECT_TRUE(session.isLocked);
-    [session unlockForConfiguration];
-  }
-  EXPECT_FALSE(session.isLocked);
-}
-
 - (void)testAddAndRemoveDelegates {
   RTC_OBJC_TYPE(RTCAudioSession) *session = [RTC_OBJC_TYPE(RTCAudioSession) sharedInstance];
   NSMutableArray *delegates = [NSMutableArray array];
@@ -264,7 +248,6 @@ OCMLocation *OCMMakeLocation(id testCase, const char *fileCString, int line){
   RTC_OBJC_TYPE(RTCAudioSession) *audioSession = mockAudioSession;
   EXPECT_EQ(0, audioSession.activationCount);
   [audioSession lockForConfiguration];
-  EXPECT_TRUE([audioSession checkLock:nil]);
   // configureWebRTCSession is forced to fail in the above mock interface,
   // so activationCount should remain 0
   OCMExpect([[mockAVAudioSession ignoringNonObjectArgs] setActive:YES
@@ -316,11 +299,6 @@ class AudioSessionTest : public ::testing::Test {
   }
 };
 
-TEST_F(AudioSessionTest, LockForConfiguration) {
-  RTCAudioSessionTest *test = [[RTCAudioSessionTest alloc] init];
-  [test testLockForConfiguration];
-}
-
 TEST_F(AudioSessionTest, AddAndRemoveDelegates) {
   RTCAudioSessionTest *test = [[RTCAudioSessionTest alloc] init];
   [test testAddAndRemoveDelegates];