From 51746ce3fe1c63877e753a072e7274aa126f4427 Mon Sep 17 00:00:00 2001 From: Niels Moller Date: Mon, 15 Feb 2021 14:51:48 +0000 Subject: [PATCH] Revert "Replace RecursiveCriticalSection with Mutex in RTCAudioSession." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit f8da43d179043f1df2e1c3e2c49494bc23f4ec28. Reason for revert: Appears to break downstream app. 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 > Reviewed-by: Kári Helgason > Commit-Queue: Niels Moller > Cr-Commit-Position: refs/heads/master@{#33259} TBR=nisse@webrtc.org,kthelgason@webrtc.org,hta@webrtc.org,handellm@webrtc.org Change-Id: Id9a97068722c7c72fc5d21102298249fd7a7cd9a No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:11567 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/207431 Reviewed-by: Niels Moller Commit-Queue: Niels Moller Cr-Commit-Position: refs/heads/master@{#33268} --- sdk/BUILD.gn | 1 - .../audio/RTCAudioSession+Configuration.mm | 3 + .../audio/RTCAudioSession+Private.h | 2 + sdk/objc/components/audio/RTCAudioSession.mm | 96 ++++++++++++++++++- sdk/objc/unittests/RTCAudioSessionTest.mm | 22 +++++ 5 files changed, 119 insertions(+), 5 deletions(-) diff --git a/sdk/BUILD.gn b/sdk/BUILD.gn index bcd8056cfc..f2602c27e1 100644 --- a/sdk/BUILD.gn +++ b/sdk/BUILD.gn @@ -334,7 +334,6 @@ if (is_ios || is_mac) { "../rtc_base", "../rtc_base:checks", "../rtc_base:rtc_base_approved", - "../rtc_base/synchronization:mutex", ] } diff --git a/sdk/objc/components/audio/RTCAudioSession+Configuration.mm b/sdk/objc/components/audio/RTCAudioSession+Configuration.mm index 449f31e9dd..b2753f282e 100644 --- a/sdk/objc/components/audio/RTCAudioSession+Configuration.mm +++ b/sdk/objc/components/audio/RTCAudioSession+Configuration.mm @@ -43,6 +43,9 @@ 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 diff --git a/sdk/objc/components/audio/RTCAudioSession+Private.h b/sdk/objc/components/audio/RTCAudioSession+Private.h index 8496ca6bbc..4c1eb1c44a 100644 --- a/sdk/objc/components/audio/RTCAudioSession+Private.h +++ b/sdk/objc/components/audio/RTCAudioSession+Private.h @@ -35,6 +35,8 @@ 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. diff --git a/sdk/objc/components/audio/RTCAudioSession.mm b/sdk/objc/components/audio/RTCAudioSession.mm index 843ce95ff3..520b2d1d37 100644 --- a/sdk/objc/components/audio/RTCAudioSession.mm +++ b/sdk/objc/components/audio/RTCAudioSession.mm @@ -16,7 +16,7 @@ #include "rtc_base/atomic_ops.h" #include "rtc_base/checks.h" -#include "rtc_base/synchronization/mutex.h" +#include "rtc_base/deprecated/recursive_critical_section.h" #import "RTCAudioSessionConfiguration.h" #import "base/RTCLogging.h" @@ -35,9 +35,10 @@ 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) { - webrtc::Mutex _mutex; + rtc::RecursiveCriticalSection _crit; AVAudioSession *_session; volatile int _activationCount; + volatile int _lockRecursionCount; volatile int _webRTCSessionCount; BOOL _isActive; BOOL _useManualAudio; @@ -150,6 +151,10 @@ NSString * const kRTCAudioSessionOutputVolumeSelector = @"outputVolume"; } } +- (BOOL)isLocked { + return _lockRecursionCount > 0; +} + - (void)setUseManualAudio:(BOOL)useManualAudio { @synchronized(self) { if (_useManualAudio == useManualAudio) { @@ -229,11 +234,20 @@ NSString * const kRTCAudioSessionOutputVolumeSelector = @"outputVolume"; #pragma clang diagnostic ignored "-Wthread-safety-analysis" - (void)lockForConfiguration { - _mutex.Lock(); + _crit.Enter(); + rtc::AtomicOps::Increment(&_lockRecursionCount); } - (void)unlockForConfiguration { - _mutex.Unlock(); + // Don't let threads other than the one that called lockForConfiguration + // 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 @@ -332,8 +346,13 @@ 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."); @@ -390,52 +409,85 @@ 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]; } @@ -556,6 +608,18 @@ 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 >)delegates { @synchronized(self) { // Note: this returns a copy. @@ -617,10 +681,25 @@ 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; @@ -630,6 +709,9 @@ NSString * const kRTCAudioSessionOutputVolumeSelector = @"outputVolume"; if (outError) { *outError = nil; } + if (![self checkLock:outError]) { + return NO; + } rtc::AtomicOps::Decrement(&_webRTCSessionCount); [self notifyDidStopPlayOrRecord]; return YES; @@ -639,6 +721,9 @@ 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. @@ -699,6 +784,9 @@ 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]; diff --git a/sdk/objc/unittests/RTCAudioSessionTest.mm b/sdk/objc/unittests/RTCAudioSessionTest.mm index 805f601bdd..9c4775a2be 100644 --- a/sdk/objc/unittests/RTCAudioSessionTest.mm +++ b/sdk/objc/unittests/RTCAudioSessionTest.mm @@ -113,10 +113,26 @@ @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]; @@ -248,6 +264,7 @@ 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 @@ -299,6 +316,11 @@ 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];