Mirrors/BackgroundMusic
2
0
Fork 0
mirror of https://github.com/kyleneideck/BackgroundMusic synced 2024-11-23 12:43:05 +00:00
Code Issues Projects Releases Packages Wiki Activity

Fix possible deadlock when starting IO.

Browse source 
BGM_Device::StartIO was holding the state mutex longer than it needed
to, which meant HasProperty, GetProperty, etc. couldn't return. If
BGMPlayThrough was notified about IO starting after StartIO locked the
mutex, BGMPlayThrough would get stuck trying to get one of BGMDevice's
properties.

Fixes #46.
This commit is contained in:
Kyle Neideck 2016-04-30 21:01:37 +10:00
parent 960fe0d28d
commit b58ad2a1f8
4 changed files with 89 additions and 77 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

35
BGMApp/BGMApp/BGMPlayThrough.cpp
View file

@ -459,7 +459,19 @@ OSStatus BGMPlayThrough::BGMDeviceListenerProc(AudioObjectID inObjectID,
{
DebugMsg("BGMPlayThrough::BGMDeviceListenerProc: Got kAudioDevicePropertyDeviceIsRunning notification");
auto deviceIsRunningHandler = [refCon] {
// This is dispatched because it can block and
// - we might be on a real-time thread, or
// - BGMXPCListener::waitForOutputDeviceToStartWithReply might get called on the same thread just
// before this and timeout waiting for this to run.
//
// TODO: We should find a way to do this without dispatching because dispatching isn't real-time safe.
dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INTERACTIVE, 0), ^{
if(refCon->mActive)
{
DebugMsg("BGMPlayThrough::BGMDeviceListenerProc: Handling "
"kAudioDevicePropertyDeviceIsRunning notification in dispatched block");
CAMutex::Locker stateLocker(refCon->mStateMutex);
// IsRunning doesn't always return true when IO is starting. Not sure why. But using
// RunningSomewhereOtherThanBGMApp instead seems to be working so far.
//
@ -471,30 +483,9 @@ OSStatus BGMPlayThrough::BGMDeviceListenerProc(AudioObjectID inObjectID,
#endif
refCon->Start();
}
};
CAMutex::Tryer stateTrier(refCon->mStateMutex);
if(stateTrier.HasLock())
{
// In the vast majority of cases (when we actually start playthrough here) we get the state lock
// and can invoke the handler directly
deviceIsRunningHandler();
}
else
{
// TODO: This should be rare, but we still shouldn't dispatch on the IO thread because it isn't
// real-time safe.
dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INTERACTIVE, 0), ^{
if(refCon->mActive)
{
DebugMsg("BGMPlayThrough::BGMDeviceListenerProc: Handling "
"kAudioDevicePropertyDeviceIsRunning notification in dispatched block");
CAMutex::Locker stateLocker(refCon->mStateMutex);
deviceIsRunningHandler();
}
});
}
}
break;
case kAudioDeviceCustomPropertyDeviceIsRunningSomewhereOtherThanBGMApp:

16
BGMApp/BGMApp/BGMXPCListener.mm
View file

@ -175,8 +175,22 @@
}
- (void) waitForOutputDeviceToStartWithReply:(void (^)(NSError*))reply {
OSStatus err = [audioDevices waitForOutputDeviceToStart];
NSString* description;
OSStatus err;
try {
err = [audioDevices waitForOutputDeviceToStart];
} catch (CAException e) {
DebugMsg("BGMXPCListener::waitForOutputDeviceToStartWithReply: Caught CAException (%d). Replying kBGMXPC_HardwareError.",
e.GetError());
err = kBGMXPC_HardwareError;
} catch (...) {
DebugMsg("BGMXPCListener::waitForOutputDeviceToStartWithReply: Caught unknown exception. Replying kBGMXPC_InternalError.");
err = kBGMXPC_InternalError;
#if DEBUG
throw;
#endif
}
switch (err) {
case noErr:

17
BGMDriver/BGMDriver/BGM_Device.cpp
View file

@ -1820,6 +1820,9 @@ void BGM_Device::Control_SetPropertyData(AudioObjectID inObjectID, pid_t inClien
#pragma mark IO Operations
void BGM_Device::StartIO(UInt32 inClientID)
{
bool clientIsBGMApp, bgmAppHasClientRegistered;
{
CAMutex::Locker theStateLocker(mStateMutex);
@ -1832,22 +1835,27 @@ void BGM_Device::StartIO(UInt32 inClientID)
// Update our client data.
//
// We add the work to the task queue, rather than doing it here, because BeginIOOperation and EndIOOperation also
// add this task to the queue and the updates should be done in order.
// We add the work to the task queue, rather than doing it here, because BeginIOOperation and EndIOOperation
// also add this task to the queue and the updates should be done in order.
bool didStartIO = mTaskQueue.QueueSync_StartClientIO(&mClients, inClientID);
// We only tell the hardware to start if this is the first time IO has been started
// We only tell the hardware to start if this is the first time IO has been started.
if(didStartIO)
{
kern_return_t theError = _HW_StartIO();
ThrowIfKernelError(theError,
CAException(theError),
"BGM_Device::StartIO: Failed to start because of an error calling down to the driver.");
}
clientIsBGMApp = mClients.IsBGMApp(inClientID);
bgmAppHasClientRegistered = mClients.BGMAppHasClientRegistered();
}
// We only return from StartIO after BGMApp is ready to pass the audio through to the output device. That way
// the HAL doesn't start sending us data before BGMApp can play it, which would mean we'd have to either drop
// frames or increase latency.
if(!mClients.IsBGMApp(inClientID) && mClients.BGMAppHasClientRegistered())
if(!clientIsBGMApp && bgmAppHasClientRegistered)
{
UInt64 theXPCError = WaitForBGMAppToStartOutputDevice();
@ -1868,7 +1876,6 @@ void BGM_Device::StartIO(UInt32 inClientID)
}
}
}
}
void BGM_Device::StopIO(UInt32 inClientID)
{

4
BGMDriver/BGMDriver/BGM_XPCHelper.m
View file

@ -84,8 +84,8 @@ UInt64 WaitForBGMAppToStartOutputDevice()
theConnection.interruptionHandler = failureHandler;
theConnection.invalidationHandler = failureHandler;
// This remote call to BGMXPCHelper will send a reply when the output device is ready to receive IO. Note that we shouldn't trust
// the reply string.
// This remote call to BGMXPCHelper will send a reply when the output device is ready to receive IO. Note that, for security
// reasons, we shouldn't trust the reply object.
[[theConnection remoteObjectProxyWithErrorHandler:^(NSError* error) {
#if !DEBUG
#pragma unused (error)