From 0e614d9179ddfa1904e0ce06a22b825a3aba016e Mon Sep 17 00:00:00 2001 From: "Ryan C. Gordon" Date: Wed, 1 Nov 2023 00:10:25 -0400 Subject: [PATCH] audio: Massive reworking on thread locking. This cleans up a ton of race conditions, and starts moving towards something we can use with Clang's -Wthread-safety (but that has a ways to go still). --- src/audio/SDL_audio.c | 641 +++++++++++++++++++++++------------------- 1 file changed, 351 insertions(+), 290 deletions(-) diff --git a/src/audio/SDL_audio.c b/src/audio/SDL_audio.c index 9c6afb39e..a46f4efa5 100644 --- a/src/audio/SDL_audio.c +++ b/src/audio/SDL_audio.c @@ -289,6 +289,134 @@ static SDL_AudioDeviceID AssignAudioDeviceInstanceId(SDL_bool iscapture, SDL_boo return instance_id; } +static void ObtainPhysicalAudioDeviceObj(SDL_AudioDevice *device) SDL_NO_THREAD_SAFETY_ANALYSIS // !!! FIXMEL SDL_ACQUIRE +{ + if (device) { + RefPhysicalAudioDevice(device); + SDL_LockMutex(device->lock); + } +} + +static void ReleaseAudioDevice(SDL_AudioDevice *device) SDL_NO_THREAD_SAFETY_ANALYSIS // !!! FIXME: SDL_RELEASE +{ + if (device) { + SDL_UnlockMutex(device->lock); + UnrefPhysicalAudioDevice(device); + } +} + +// If found, this locks _the physical device_ this logical device is associated with, before returning. +static SDL_LogicalAudioDevice *ObtainLogicalAudioDevice(SDL_AudioDeviceID devid, SDL_AudioDevice **device) SDL_NO_THREAD_SAFETY_ANALYSIS // !!! FIXME: SDL_ACQUIRE +{ + SDL_assert(device != NULL); + + *device = NULL; + + if (!SDL_GetCurrentAudioDriver()) { + SDL_SetError("Audio subsystem is not initialized"); + return NULL; + } + + SDL_LogicalAudioDevice *logdev = NULL; + + // bit #1 of devid is set for physical devices and unset for logical. + const SDL_bool islogical = (devid & (1<<1)) ? SDL_FALSE : SDL_TRUE; + if (islogical) { // don't bother looking if it's not a logical device id value. + SDL_LockRWLockForReading(current_audio.device_hash_lock); + SDL_FindInHashTable(current_audio.device_hash, (const void *) (uintptr_t) devid, (const void **) &logdev); + if (logdev) { + *device = logdev->physical_device; + RefPhysicalAudioDevice(*device); // reference it, in case the logical device migrates to a new default. + } + SDL_UnlockRWLock(current_audio.device_hash_lock); + } + + if (!logdev) { + SDL_SetError("Invalid audio device instance ID"); + } else { + SDL_assert(*device != NULL); + SDL_LockMutex((*device)->lock); + } + + return logdev; +} + + +/* this finds the physical device associated with `devid` and locks it for use. + Note that a logical device instance id will return its associated physical device! */ +static SDL_AudioDevice *ObtainPhysicalAudioDevice(SDL_AudioDeviceID devid) // !!! FIXME: SDL_ACQUIRE +{ + SDL_AudioDevice *device = NULL; + + // bit #1 of devid is set for physical devices and unset for logical. + const SDL_bool islogical = (devid & (1<<1)) ? SDL_FALSE : SDL_TRUE; + if (islogical) { + ObtainLogicalAudioDevice(devid, &device); + } else if (!SDL_GetCurrentAudioDriver()) { // (the `islogical` path, above, checks this in ObtainLogicalAudioDevice.) + SDL_SetError("Audio subsystem is not initialized"); + } else { + SDL_LockRWLockForReading(current_audio.device_hash_lock); + SDL_FindInHashTable(current_audio.device_hash, (const void *) (uintptr_t) devid, (const void **) &device); + SDL_UnlockRWLock(current_audio.device_hash_lock); + + if (!device) { + SDL_SetError("Invalid audio device instance ID"); + } else { + ObtainPhysicalAudioDeviceObj(device); + } + } + + return device; +} + +static SDL_AudioDevice *ObtainPhysicalAudioDeviceDefaultAllowed(SDL_AudioDeviceID devid) // !!! FIXME: SDL_ACQUIRE +{ + const SDL_bool wants_default = ((devid == SDL_AUDIO_DEVICE_DEFAULT_OUTPUT) || (devid == SDL_AUDIO_DEVICE_DEFAULT_CAPTURE)) ? SDL_TRUE : SDL_FALSE; + if (!wants_default) { + return ObtainPhysicalAudioDevice(devid); + } + + const SDL_AudioDeviceID orig_devid = devid; + + while (SDL_TRUE) { + SDL_LockRWLockForReading(current_audio.device_hash_lock); + if (orig_devid == SDL_AUDIO_DEVICE_DEFAULT_OUTPUT) { + devid = current_audio.default_output_device_id; + } else if (orig_devid == SDL_AUDIO_DEVICE_DEFAULT_CAPTURE) { + devid = current_audio.default_capture_device_id; + } + SDL_UnlockRWLock(current_audio.device_hash_lock); + + if (devid == 0) { + SDL_SetError("No default audio device available"); + break; + } + + SDL_AudioDevice *device = ObtainPhysicalAudioDevice(devid); + if (!device) { + break; + } + + // make sure the default didn't change while we were waiting for the lock... + SDL_bool got_it = SDL_FALSE; + SDL_LockRWLockForReading(current_audio.device_hash_lock); + if ((orig_devid == SDL_AUDIO_DEVICE_DEFAULT_OUTPUT) && (devid == current_audio.default_output_device_id)) { + got_it = SDL_TRUE; + } else if ((orig_devid == SDL_AUDIO_DEVICE_DEFAULT_CAPTURE) && (devid == current_audio.default_capture_device_id)) { + got_it = SDL_TRUE; + } + SDL_UnlockRWLock(current_audio.device_hash_lock); + + if (got_it) { + return device; + } + + ReleaseAudioDevice(device); // let it go and try again. + } + + return NULL; +} + // this assumes you hold the _physical_ device lock for this logical device! This will not unlock the lock or close the physical device! // It also will not unref the physical device, since we might be shutting down; SDL_CloseAudioDevice handles the unref. static void DestroyLogicalAudioDevice(SDL_LogicalAudioDevice *logdev) @@ -334,11 +462,11 @@ static void DestroyPhysicalAudioDevice(SDL_AudioDevice *device) } // Destroy any logical devices that still exist... - SDL_LockMutex(device->lock); + SDL_LockMutex(device->lock); // don't use ObtainPhysicalAudioDeviceObj because we don't want to change refcounts while destroying. while (device->logical_devices != NULL) { DestroyLogicalAudioDevice(device->logical_devices); } - SDL_UnlockMutex(device->lock); + SDL_UnlockMutex(device->lock); // don't use ReleaseAudioDevice because we don't want to change refcounts while destroying. // it's safe to not hold the lock for this (we can't anyhow, or the audio thread won't quit), because we shouldn't be in the device list at this point. ClosePhysicalAudioDevice(device); @@ -483,27 +611,6 @@ void SDL_AudioDeviceDisconnected(SDL_AudioDevice *device) return; } - SDL_LockMutex(device->lock); - - if (!SDL_AtomicCAS(&device->zombie, 0, 1)) { - SDL_UnlockMutex(device->lock); - return; // already disconnected this device, don't do it twice. - } - - // Swap in "Zombie" versions of the usual platform interfaces, so the device will keep - // making progress until the app closes it. Otherwise, streams might continue to - // accumulate waste data that never drains, apps that depend on audio callbacks to - // progress will freeze, etc. - device->WaitDevice = ZombieWaitDevice; - device->GetDeviceBuf = ZombieGetDeviceBuf; - device->PlayDevice = ZombiePlayDevice; - device->WaitCaptureDevice = ZombieWaitDevice; - device->CaptureFromDevice = ZombieCaptureFromDevice; - device->FlushCapture = ZombieFlushCapture; - - const SDL_AudioDeviceID devid = device->instance_id; - const SDL_bool is_default_device = ((devid == current_audio.default_output_device_id) || (devid == current_audio.default_capture_device_id)) ? SDL_TRUE : SDL_FALSE; - // Save off removal info in a list so we can send events for each, next // time the event queue pumps, in case something tries to close a device // from an event filter, as this would risk deadlocks and other disasters @@ -512,45 +619,64 @@ void SDL_AudioDeviceDisconnected(SDL_AudioDevice *device) pending.next = NULL; SDL_PendingAudioDeviceEvent *pending_tail = &pending; - // on default devices, dump any logical devices that explicitly opened this device. Things that opened the system default can stay. - // on non-default devices, dump everything. - // (by "dump" we mean send a REMOVED event; the zombie will keep consuming audio data for these logical devices until explicitly closed.) - for (SDL_LogicalAudioDevice *logdev = device->logical_devices; logdev != NULL; logdev = logdev->next) { - if (!is_default_device || !logdev->opened_as_default) { // if opened as a default, leave it on the zombie device for later migration. - SDL_PendingAudioDeviceEvent *p = (SDL_PendingAudioDeviceEvent *) SDL_malloc(sizeof (SDL_PendingAudioDeviceEvent)); - if (p) { // if this failed, no event for you, but you have deeper problems anyhow. - p->type = SDL_EVENT_AUDIO_DEVICE_REMOVED; - p->devid = logdev->instance_id; - p->next = NULL; - pending_tail->next = p; - pending_tail = p; + ObtainPhysicalAudioDeviceObj(device); + + SDL_LockRWLockForReading(current_audio.device_hash_lock); + const SDL_AudioDeviceID devid = device->instance_id; + const SDL_bool is_default_device = ((devid == current_audio.default_output_device_id) || (devid == current_audio.default_capture_device_id)) ? SDL_TRUE : SDL_FALSE; + SDL_UnlockRWLock(current_audio.device_hash_lock); + + const SDL_bool first_disconnect = SDL_AtomicCAS(&device->zombie, 0, 1); + if (first_disconnect) { // if already disconnected this device, don't do it twice. + // Swap in "Zombie" versions of the usual platform interfaces, so the device will keep + // making progress until the app closes it. Otherwise, streams might continue to + // accumulate waste data that never drains, apps that depend on audio callbacks to + // progress will freeze, etc. + device->WaitDevice = ZombieWaitDevice; + device->GetDeviceBuf = ZombieGetDeviceBuf; + device->PlayDevice = ZombiePlayDevice; + device->WaitCaptureDevice = ZombieWaitDevice; + device->CaptureFromDevice = ZombieCaptureFromDevice; + device->FlushCapture = ZombieFlushCapture; + + // on default devices, dump any logical devices that explicitly opened this device. Things that opened the system default can stay. + // on non-default devices, dump everything. + // (by "dump" we mean send a REMOVED event; the zombie will keep consuming audio data for these logical devices until explicitly closed.) + for (SDL_LogicalAudioDevice *logdev = device->logical_devices; logdev != NULL; logdev = logdev->next) { + if (!is_default_device || !logdev->opened_as_default) { // if opened as a default, leave it on the zombie device for later migration. + SDL_PendingAudioDeviceEvent *p = (SDL_PendingAudioDeviceEvent *) SDL_malloc(sizeof (SDL_PendingAudioDeviceEvent)); + if (p) { // if this failed, no event for you, but you have deeper problems anyhow. + p->type = SDL_EVENT_AUDIO_DEVICE_REMOVED; + p->devid = logdev->instance_id; + p->next = NULL; + pending_tail->next = p; + pending_tail = p; + } } } + + SDL_PendingAudioDeviceEvent *p = (SDL_PendingAudioDeviceEvent *) SDL_malloc(sizeof (SDL_PendingAudioDeviceEvent)); + if (p) { // if this failed, no event for you, but you have deeper problems anyhow. + p->type = SDL_EVENT_AUDIO_DEVICE_REMOVED; + p->devid = device->instance_id; + p->next = NULL; + pending_tail->next = p; + pending_tail = p; + } } - SDL_PendingAudioDeviceEvent *p = (SDL_PendingAudioDeviceEvent *) SDL_malloc(sizeof (SDL_PendingAudioDeviceEvent)); - if (p) { // if this failed, no event for you, but you have deeper problems anyhow. - p->type = SDL_EVENT_AUDIO_DEVICE_REMOVED; - p->devid = device->instance_id; - p->next = NULL; - pending_tail->next = p; - pending_tail = p; - } + ReleaseAudioDevice(device); - SDL_UnlockMutex(device->lock); + if (first_disconnect) { + if (pending.next) { // NULL if event is disabled or disaster struck. + SDL_LockRWLockForWriting(current_audio.device_hash_lock); + SDL_assert(current_audio.pending_events_tail != NULL); + SDL_assert(current_audio.pending_events_tail->next == NULL); + current_audio.pending_events_tail->next = pending.next; + current_audio.pending_events_tail = pending_tail; + SDL_UnlockRWLock(current_audio.device_hash_lock); + } - if (pending.next) { // NULL if event is disabled or disaster struck. - SDL_LockRWLockForWriting(current_audio.device_hash_lock); - SDL_assert(current_audio.pending_events_tail != NULL); - SDL_assert(current_audio.pending_events_tail->next == NULL); - current_audio.pending_events_tail->next = pending.next; - current_audio.pending_events_tail = pending_tail; - SDL_UnlockRWLock(current_audio.device_hash_lock); - } - - // Is this a non-default device? We can unref it now. - // Otherwise, we'll unref it when a new default device is chosen. - if (!is_default_device) { UnrefPhysicalAudioDevice(device); } } @@ -622,9 +748,10 @@ static void CompleteAudioEntryPoints(void) #undef FILL_STUB } -static SDL_AudioDeviceID GetFirstAddedAudioDeviceID(const SDL_bool iscapture) +static SDL_AudioDevice *GetFirstAddedAudioDevice(const SDL_bool iscapture) { - SDL_AudioDeviceID retval = (SDL_AudioDeviceID) SDL_AUDIO_DEVICE_DEFAULT_OUTPUT; // According to AssignAudioDeviceInstanceId, nothing can have a value this large. + SDL_AudioDeviceID highest = (SDL_AudioDeviceID) SDL_AUDIO_DEVICE_DEFAULT_OUTPUT; // According to AssignAudioDeviceInstanceId, nothing can have a value this large. + SDL_AudioDevice *retval = NULL; // (Device IDs increase as new devices are added, so the first device added has the lowest SDL_AudioDeviceID value.) SDL_LockRWLockForReading(current_audio.device_hash_lock); @@ -633,16 +760,17 @@ static SDL_AudioDeviceID GetFirstAddedAudioDeviceID(const SDL_bool iscapture) const void *value; void *iter = NULL; while (SDL_IterateHashTable(current_audio.device_hash, &key, &value, &iter)) { - const SDL_AudioDeviceID devid = (SDL_AudioDeviceID) (uintptr_t) value; + const SDL_AudioDeviceID devid = (SDL_AudioDeviceID) (uintptr_t) key; // bit #1 of devid is set for physical devices and unset for logical. const SDL_bool isphysical = (devid & (1<<1)) ? SDL_TRUE : SDL_FALSE; - if (isphysical && (devid < retval)) { - retval = devid; + if (isphysical && (devid < highest)) { + highest = devid; + retval = (SDL_AudioDevice *) value; } } SDL_UnlockRWLock(current_audio.device_hash_lock); - return (retval == SDL_AUDIO_DEVICE_DEFAULT_OUTPUT) ? 0 : retval; + return retval; } static Uint32 HashAudioDeviceID(const void *key, void *data) @@ -779,20 +907,23 @@ int SDL_InitAudio(const char *driver_name) SDL_AudioDevice *default_capture = NULL; current_audio.impl.DetectDevices(&default_output, &default_capture); - // these are only set if default_* is non-NULL, in case the backend just called SDL_DefaultAudioDeviceChanged directly during DetectDevices. - if (default_output) { - current_audio.default_output_device_id = default_output->instance_id; - } - if (default_capture) { - current_audio.default_capture_device_id = default_capture->instance_id; + // If no default was _ever_ specified, just take the first device we see, if any. + if (!default_output) { + default_output = GetFirstAddedAudioDevice(/*iscapture=*/SDL_FALSE); } - // If no default was _ever_ specified, just take the first device we see, if any. - if (!current_audio.default_output_device_id) { - current_audio.default_output_device_id = GetFirstAddedAudioDeviceID(/*iscapture=*/SDL_FALSE); + if (!default_capture) { + default_capture = GetFirstAddedAudioDevice(/*iscapture=*/SDL_TRUE); } - if (!current_audio.default_capture_device_id) { - current_audio.default_capture_device_id = GetFirstAddedAudioDeviceID(/*iscapture=*/SDL_TRUE); + + if (default_output) { + current_audio.default_output_device_id = default_output->instance_id; + RefPhysicalAudioDevice(default_output); // extra ref on default devices. + } + + if (default_capture) { + current_audio.default_capture_device_id = default_capture->instance_id; + RefPhysicalAudioDevice(default_capture); // extra ref on default devices. } return 0; @@ -1171,62 +1302,6 @@ SDL_AudioDeviceID *SDL_GetAudioCaptureDevices(int *count) return GetAudioDevices(count, SDL_TRUE); } -// If found, this locks _the physical device_ this logical device is associated with, before returning. -static SDL_LogicalAudioDevice *ObtainLogicalAudioDevice(SDL_AudioDeviceID devid) -{ - if (!SDL_GetCurrentAudioDriver()) { - SDL_SetError("Audio subsystem is not initialized"); - return NULL; - } - - SDL_LogicalAudioDevice *logdev = NULL; - - // bit #1 of devid is set for physical devices and unset for logical. - const SDL_bool islogical = (devid & (1<<1)) ? SDL_FALSE : SDL_TRUE; - if (islogical) { // don't bother looking if it's not a logical device id value. - SDL_LockRWLockForReading(current_audio.device_hash_lock); - SDL_FindInHashTable(current_audio.device_hash, (const void *) (uintptr_t) devid, (const void **) &logdev); - SDL_UnlockRWLock(current_audio.device_hash_lock); - } - - if (!logdev) { - SDL_SetError("Invalid audio device instance ID"); - } else { - SDL_LockMutex(logdev->physical_device->lock); - } - - return logdev; -} - -/* this finds the physical device associated with `devid` and locks it for use. - Note that a logical device instance id will return its associated physical device! */ -static SDL_AudioDevice *ObtainPhysicalAudioDevice(SDL_AudioDeviceID devid) -{ - SDL_AudioDevice *device = NULL; - - // bit #1 of devid is set for physical devices and unset for logical. - const SDL_bool islogical = (devid & (1<<1)) ? SDL_FALSE : SDL_TRUE; - if (islogical) { - SDL_LogicalAudioDevice *logdev = ObtainLogicalAudioDevice(devid); - if (logdev) { - device = logdev->physical_device; - } - } else if (!SDL_GetCurrentAudioDriver()) { // (the `islogical` path, above, checks this in ObtainLogicalAudioDevice.) - SDL_SetError("Audio subsystem is not initialized"); - } else { - SDL_LockRWLockForReading(current_audio.device_hash_lock); - SDL_FindInHashTable(current_audio.device_hash, (const void *) (uintptr_t) devid, (const void **) &device); - SDL_UnlockRWLock(current_audio.device_hash_lock); - - if (!device) { - SDL_SetError("Invalid audio device instance ID"); - } else { - SDL_LockMutex(device->lock); // caller must unlock. - } - } - - return device; -} SDL_AudioDevice *SDL_FindPhysicalAudioDeviceByCallback(SDL_bool (*callback)(SDL_AudioDevice *device, void *userdata), void *userdata) { @@ -1270,17 +1345,15 @@ SDL_AudioDevice *SDL_FindPhysicalAudioDeviceByHandle(void *handle) char *SDL_GetAudioDeviceName(SDL_AudioDeviceID devid) { + char *retval = NULL; SDL_AudioDevice *device = ObtainPhysicalAudioDevice(devid); - if (!device) { - return NULL; + if (device) { + retval = SDL_strdup(device->name); + if (!retval) { + SDL_OutOfMemory(); + } } - - char *retval = SDL_strdup(device->name); - if (!retval) { - SDL_OutOfMemory(); - } - - SDL_UnlockMutex(device->lock); + ReleaseAudioDevice(device); return retval; } @@ -1291,31 +1364,18 @@ int SDL_GetAudioDeviceFormat(SDL_AudioDeviceID devid, SDL_AudioSpec *spec, int * return SDL_InvalidParamError("spec"); } - SDL_bool wants_default = SDL_FALSE; - if (devid == SDL_AUDIO_DEVICE_DEFAULT_OUTPUT) { - devid = current_audio.default_output_device_id; - wants_default = SDL_TRUE; - } else if (devid == SDL_AUDIO_DEVICE_DEFAULT_CAPTURE) { - devid = current_audio.default_capture_device_id; - wants_default = SDL_TRUE; + int retval = -1; + SDL_AudioDevice *device = ObtainPhysicalAudioDeviceDefaultAllowed(devid); // !!! FIXME: this needs an ObtainBlahDefaultAllowed to catch default device changes. + if (device) { + SDL_copyp(spec, &device->spec); + if (sample_frames) { + *sample_frames = device->sample_frames; + } + retval = 0; } + ReleaseAudioDevice(device); - if ((devid == 0) && wants_default) { - return SDL_SetError("No default audio device available"); - } - - SDL_AudioDevice *device = ObtainPhysicalAudioDevice(devid); - if (!device) { - return -1; - } - - SDL_copyp(spec, &device->spec); - if (sample_frames) { - *sample_frames = device->sample_frames; - } - SDL_UnlockMutex(device->lock); - - return 0; + return retval; } // this expects the device lock to be held. !!! FIXME: no it doesn't...? @@ -1350,20 +1410,21 @@ static void ClosePhysicalAudioDevice(SDL_AudioDevice *device) void SDL_CloseAudioDevice(SDL_AudioDeviceID devid) { - SDL_LogicalAudioDevice *logdev = ObtainLogicalAudioDevice(devid); + SDL_bool close_physical = SDL_FALSE; + SDL_AudioDevice *device = NULL; + SDL_LogicalAudioDevice *logdev = ObtainLogicalAudioDevice(devid, &device); if (logdev) { - SDL_AudioDevice *device = logdev->physical_device; DestroyLogicalAudioDevice(logdev); + close_physical = (device->logical_devices == NULL); // no more logical devices? Close the physical device, too. + } - const SDL_bool close_physical = (device->logical_devices == NULL); // no more logical devices? Close the physical device, too. - - // !!! FIXME: we _need_ to release this lock, but doing so can cause a race condition if someone opens a device while we're closing it. - SDL_UnlockMutex(device->lock); // can't hold the lock or the audio thread will deadlock while we WaitThread it. If not closing, we're done anyhow. + // !!! FIXME: we _need_ to release this lock, but doing so can cause a race condition if someone opens a device while we're closing it. + ReleaseAudioDevice(device); // can't hold the lock or the audio thread will deadlock while we WaitThread it. If not closing, we're done anyhow. + if (device) { if (close_physical) { ClosePhysicalAudioDevice(device); } - UnrefPhysicalAudioDevice(device); // one reference for each logical device. } } @@ -1514,30 +1575,17 @@ SDL_AudioDeviceID SDL_OpenAudioDevice(SDL_AudioDeviceID devid, const SDL_AudioSp return 0; } - SDL_bool wants_default = SDL_FALSE; - if (devid == SDL_AUDIO_DEVICE_DEFAULT_OUTPUT) { - devid = current_audio.default_output_device_id; - wants_default = SDL_TRUE; - } else if (devid == SDL_AUDIO_DEVICE_DEFAULT_CAPTURE) { - devid = current_audio.default_capture_device_id; - wants_default = SDL_TRUE; - } - - if ((devid == 0) && wants_default) { - SDL_SetError("No default audio device available"); - return 0; - } + SDL_bool wants_default = ((devid == SDL_AUDIO_DEVICE_DEFAULT_OUTPUT) || (devid == SDL_AUDIO_DEVICE_DEFAULT_CAPTURE)) ? SDL_TRUE : SDL_FALSE; // this will let you use a logical device to make a new logical device on the parent physical device. Could be useful? SDL_AudioDevice *device = NULL; - const SDL_bool islogical = (devid & (1<<1)) ? SDL_FALSE : SDL_TRUE; + const SDL_bool islogical = (wants_default || (devid & (1<<1))) ? SDL_FALSE : SDL_TRUE; if (!islogical) { - device = ObtainPhysicalAudioDevice(devid); + device = ObtainPhysicalAudioDeviceDefaultAllowed(devid); } else { - SDL_LogicalAudioDevice *logdev = ObtainLogicalAudioDevice(devid); // this locks the physical device, too. + SDL_LogicalAudioDevice *logdev = ObtainLogicalAudioDevice(devid, &device); if (logdev) { wants_default = logdev->opened_as_default; // was the original logical device meant to be a default? Make this one, too. - device = logdev->physical_device; } } @@ -1565,7 +1613,7 @@ SDL_AudioDeviceID SDL_OpenAudioDevice(SDL_AudioDeviceID devid, const SDL_AudioSp device->logical_devices = logdev; UpdateAudioStreamFormatsPhysical(device); } - SDL_UnlockMutex(device->lock); + ReleaseAudioDevice(device); if (retval) { SDL_LockRWLockForWriting(current_audio.device_hash_lock); @@ -1583,13 +1631,13 @@ SDL_AudioDeviceID SDL_OpenAudioDevice(SDL_AudioDeviceID devid, const SDL_AudioSp static int SetLogicalAudioDevicePauseState(SDL_AudioDeviceID devid, int value) { - SDL_LogicalAudioDevice *logdev = ObtainLogicalAudioDevice(devid); - if (!logdev) { - return -1; // ObtainLogicalAudioDevice will have set an error. + SDL_AudioDevice *device = NULL; + SDL_LogicalAudioDevice *logdev = ObtainLogicalAudioDevice(devid, &device); + if (logdev) { + SDL_AtomicSet(&logdev->paused, value); } - SDL_AtomicSet(&logdev->paused, value); - SDL_UnlockMutex(logdev->physical_device->lock); - return 0; + ReleaseAudioDevice(device); + return logdev ? 0 : -1; // ObtainLogicalAudioDevice will have set an error. } int SDL_PauseAudioDevice(SDL_AudioDeviceID devid) @@ -1604,23 +1652,22 @@ int SDLCALL SDL_ResumeAudioDevice(SDL_AudioDeviceID devid) SDL_bool SDL_AudioDevicePaused(SDL_AudioDeviceID devid) { - SDL_LogicalAudioDevice *logdev = ObtainLogicalAudioDevice(devid); + SDL_AudioDevice *device = NULL; + SDL_LogicalAudioDevice *logdev = ObtainLogicalAudioDevice(devid, &device); SDL_bool retval = SDL_FALSE; - if (logdev) { - if (SDL_AtomicGet(&logdev->paused)) { - retval = SDL_TRUE; - } - SDL_UnlockMutex(logdev->physical_device->lock); + if (logdev && SDL_AtomicGet(&logdev->paused)) { + retval = SDL_TRUE; } + ReleaseAudioDevice(device); return retval; } int SDL_SetAudioPostmixCallback(SDL_AudioDeviceID devid, SDL_AudioPostmixCallback callback, void *userdata) { - SDL_LogicalAudioDevice *logdev = ObtainLogicalAudioDevice(devid); + SDL_AudioDevice *device = NULL; + SDL_LogicalAudioDevice *logdev = ObtainLogicalAudioDevice(devid, &device); int retval = 0; if (logdev) { - SDL_AudioDevice *device = logdev->physical_device; if (callback && !device->postmix_buffer) { device->postmix_buffer = (float *)SDL_aligned_alloc(SDL_SIMDGetAlignment(), device->work_buffer_size); if (device->postmix_buffer == NULL) { @@ -1644,15 +1691,17 @@ int SDL_SetAudioPostmixCallback(SDL_AudioDeviceID devid, SDL_AudioPostmixCallbac } UpdateAudioStreamFormatsPhysical(device); - SDL_UnlockMutex(device->lock); } + ReleaseAudioDevice(device); return retval; } int SDL_BindAudioStreams(SDL_AudioDeviceID devid, SDL_AudioStream **streams, int num_streams) { const SDL_bool islogical = (devid & (1<<1)) ? SDL_FALSE : SDL_TRUE; - SDL_LogicalAudioDevice *logdev; + SDL_AudioDevice *device = NULL; + SDL_LogicalAudioDevice *logdev = NULL; + int retval = 0; if (num_streams == 0) { return 0; // nothing to do @@ -1662,49 +1711,49 @@ int SDL_BindAudioStreams(SDL_AudioDeviceID devid, SDL_AudioStream **streams, int return SDL_InvalidParamError("streams"); } else if (!islogical) { return SDL_SetError("Audio streams are bound to device ids from SDL_OpenAudioDevice, not raw physical devices"); - } else if ((logdev = ObtainLogicalAudioDevice(devid)) == NULL) { - return -1; // ObtainLogicalAudioDevice set the error message. - } else if (logdev->simplified) { - SDL_UnlockMutex(logdev->physical_device->lock); - return SDL_SetError("Cannot change stream bindings on device opened with SDL_OpenAudioDeviceStream"); } - // !!! FIXME: We'll set the device's side's format below, but maybe we should refuse to bind a stream if the app's side doesn't have a format set yet. - // !!! FIXME: Actually, why do we allow there to be an invalid format, again? + logdev = ObtainLogicalAudioDevice(devid, &device); + if (!logdev) { + retval = -1; // ObtainLogicalAudioDevice set the error string. + } else if (logdev->simplified) { + retval = SDL_SetError("Cannot change stream bindings on device opened with SDL_OpenAudioDeviceStream"); + } else { - // make sure start of list is sane. - SDL_assert(!logdev->bound_streams || (logdev->bound_streams->prev_binding == NULL)); + // !!! FIXME: We'll set the device's side's format below, but maybe we should refuse to bind a stream if the app's side doesn't have a format set yet. + // !!! FIXME: Actually, why do we allow there to be an invalid format, again? - SDL_AudioDevice *device = logdev->physical_device; - const SDL_bool iscapture = device->iscapture; - int retval = 0; + // make sure start of list is sane. + SDL_assert(!logdev->bound_streams || (logdev->bound_streams->prev_binding == NULL)); - // lock all the streams upfront, so we can verify they aren't bound elsewhere and add them all in one block, as this is intended to add everything or nothing. - for (int i = 0; i < num_streams; i++) { - SDL_AudioStream *stream = streams[i]; - if (stream == NULL) { - retval = SDL_SetError("Stream #%d is NULL", i); - } else { - SDL_LockMutex(stream->lock); - SDL_assert((stream->bound_device == NULL) == ((stream->prev_binding == NULL) || (stream->next_binding == NULL))); - if (stream->bound_device) { - retval = SDL_SetError("Stream #%d is already bound to a device", i); - } else if (stream->simplified) { // You can get here if you closed the device instead of destroying the stream. - retval = SDL_SetError("Cannot change binding on a stream created with SDL_OpenAudioDeviceStream"); + // lock all the streams upfront, so we can verify they aren't bound elsewhere and add them all in one block, as this is intended to add everything or nothing. + for (int i = 0; i < num_streams; i++) { + SDL_AudioStream *stream = streams[i]; + if (stream == NULL) { + retval = SDL_SetError("Stream #%d is NULL", i); + } else { + SDL_LockMutex(stream->lock); + SDL_assert((stream->bound_device == NULL) == ((stream->prev_binding == NULL) || (stream->next_binding == NULL))); + if (stream->bound_device) { + retval = SDL_SetError("Stream #%d is already bound to a device", i); + } else if (stream->simplified) { // You can get here if you closed the device instead of destroying the stream. + retval = SDL_SetError("Cannot change binding on a stream created with SDL_OpenAudioDeviceStream"); + } } - } - if (retval != 0) { - int j; - for (j = 0; j <= i; j++) { - SDL_UnlockMutex(streams[j]->lock); + if (retval != 0) { + int j; + for (j = 0; j <= i; j++) { + SDL_UnlockMutex(streams[j]->lock); + } + break; } - break; } } if (retval == 0) { // Now that everything is verified, chain everything together. + const SDL_bool iscapture = device->iscapture; for (int i = 0; i < num_streams; i++) { SDL_AudioStream *stream = streams[i]; @@ -1729,7 +1778,7 @@ int SDL_BindAudioStreams(SDL_AudioDeviceID devid, SDL_AudioStream **streams, int UpdateAudioStreamFormatsPhysical(device); - SDL_UnlockMutex(device->lock); + ReleaseAudioDevice(device); return retval; } @@ -1739,6 +1788,7 @@ int SDL_BindAudioStream(SDL_AudioDeviceID devid, SDL_AudioStream *stream) return SDL_BindAudioStreams(devid, &stream, 1); } +// !!! FIXME: this and BindAudioStreams are mutex nightmares. :/ void SDL_UnbindAudioStreams(SDL_AudioStream **streams, int num_streams) { /* to prevent deadlock when holding both locks, we _must_ lock the device first, and the stream second, as that is the order the audio thread will do it. @@ -1831,51 +1881,51 @@ SDL_AudioStream *SDL_OpenAudioDeviceStream(SDL_AudioDeviceID devid, const SDL_Au return NULL; // error string should already be set. } - SDL_LogicalAudioDevice *logdev = ObtainLogicalAudioDevice(logdevid); - if (logdev == NULL) { // this shouldn't happen, but just in case. - SDL_CloseAudioDevice(logdevid); - return NULL; // error string should already be set. - } - - SDL_AudioDevice *physdevice = logdev->physical_device; - SDL_assert(physdevice != NULL); - - SDL_AtomicSet(&logdev->paused, 1); // start the device paused, to match SDL2. - SDL_UnlockMutex(physdevice->lock); // we don't need to hold the lock for any of this. - - const SDL_bool iscapture = physdevice->iscapture; - + SDL_bool failed = SDL_FALSE; SDL_AudioStream *stream = NULL; - if (iscapture) { - stream = SDL_CreateAudioStream(&physdevice->spec, spec); + SDL_AudioDevice *device = NULL; + SDL_LogicalAudioDevice *logdev = ObtainLogicalAudioDevice(logdevid, &device); + if (logdev == NULL) { // this shouldn't happen, but just in case. + failed = SDL_TRUE; } else { - stream = SDL_CreateAudioStream(spec, &physdevice->spec); + SDL_AtomicSet(&logdev->paused, 1); // start the device paused, to match SDL2. + + SDL_assert(device != NULL); + const SDL_bool iscapture = device->iscapture; + + if (iscapture) { + stream = SDL_CreateAudioStream(&device->spec, spec); + } else { + stream = SDL_CreateAudioStream(spec, &device->spec); + } + + if (!stream || (SDL_BindAudioStream(logdevid, stream) == -1)) { + failed = SDL_TRUE; + } else { + logdev->simplified = SDL_TRUE; // forbid further binding changes on this logical device. + stream->simplified = SDL_TRUE; // so we know to close the audio device when this is destroyed. + + if (callback) { + int rc; + if (iscapture) { + rc = SDL_SetAudioStreamPutCallback(stream, callback, userdata); + } else { + rc = SDL_SetAudioStreamGetCallback(stream, callback, userdata); + } + SDL_assert(rc == 0); // should only fail if stream==NULL atm. + } + } } - if (!stream) { - SDL_CloseAudioDevice(logdevid); - return NULL; // error string should already be set. - } - if (SDL_BindAudioStream(logdevid, stream) == -1) { + ReleaseAudioDevice(device); + + if (failed) { SDL_DestroyAudioStream(stream); SDL_CloseAudioDevice(logdevid); - return NULL; // error string should already be set. + stream = NULL; } - logdev->simplified = SDL_TRUE; // forbid further binding changes on this logical device. - stream->simplified = SDL_TRUE; // so we know to close the audio device when this is destroyed. - - if (callback) { - int rc; - if (iscapture) { - rc = SDL_SetAudioStreamPutCallback(stream, callback, userdata); - } else { - rc = SDL_SetAudioStreamGetCallback(stream, callback, userdata); - } - SDL_assert(rc == 0); // should only fail if stream==NULL atm. - } - - return stream; // ready to rock. + return stream; } #define NUM_FORMATS 8 @@ -1913,9 +1963,21 @@ void SDL_DefaultAudioDeviceChanged(SDL_AudioDevice *new_default_device) } const SDL_bool iscapture = new_default_device->iscapture; - const SDL_AudioDeviceID current_devid = iscapture ? current_audio.default_capture_device_id : current_audio.default_output_device_id; - if (new_default_device->instance_id == current_devid) { + // change the official default over right away, so new opens will go to the new device. + SDL_LockRWLockForWriting(current_audio.device_hash_lock); + const SDL_AudioDeviceID current_devid = iscapture ? current_audio.default_capture_device_id : current_audio.default_output_device_id; + const SDL_bool is_already_default = (new_default_device->instance_id == current_devid) ? SDL_TRUE : SDL_FALSE; + if (!is_already_default) { + if (iscapture) { + current_audio.default_capture_device_id = new_default_device->instance_id; + } else { + current_audio.default_output_device_id = new_default_device->instance_id; + } + } + SDL_UnlockRWLock(current_audio.device_hash_lock); + + if (is_already_default) { return; // this is already the default. } @@ -1926,18 +1988,13 @@ void SDL_DefaultAudioDeviceChanged(SDL_AudioDevice *new_default_device) pending.next = NULL; SDL_PendingAudioDeviceEvent *pending_tail = &pending; - SDL_LockMutex(new_default_device->lock); + // Default device gets an extra ref, so it lives until a new default replaces it, even if disconnected. + RefPhysicalAudioDevice(new_default_device); + + ObtainPhysicalAudioDeviceObj(new_default_device); SDL_AudioDevice *current_default_device = ObtainPhysicalAudioDevice(current_devid); - /* change the official default ID over while we have locks on both devices, so if something raced to open the default during - this, it either gets the new device or is ready on the old and can be migrated. */ - if (iscapture) { - current_audio.default_capture_device_id = new_default_device->instance_id; - } else { - current_audio.default_output_device_id = new_default_device->instance_id; - } - if (current_default_device) { // migrate any logical devices that were opened as a default to the new physical device... @@ -1984,7 +2041,8 @@ void SDL_DefaultAudioDeviceChanged(SDL_AudioDevice *new_default_device) continue; // not opened as a default, leave it on the current physical device. } - // now migrate the logical device. + // now migrate the logical device. Hold device_hash_lock so ObtainLogicalAudioDevice doesn't get a device in the middle of transition. + SDL_LockRWLockForWriting(current_audio.device_hash_lock); if (logdev->next) { logdev->next->prev = logdev->prev; } @@ -1999,6 +2057,7 @@ void SDL_DefaultAudioDeviceChanged(SDL_AudioDevice *new_default_device) logdev->prev = NULL; logdev->next = new_default_device->logical_devices; new_default_device->logical_devices = logdev; + SDL_UnlockRWLock(current_audio.device_hash_lock); SDL_assert(SDL_AtomicGet(¤t_default_device->refcount) > 1); // we should hold at least one extra reference to this device, beyond logical devices, during this phase... RefPhysicalAudioDevice(new_default_device); @@ -2024,19 +2083,21 @@ void SDL_DefaultAudioDeviceChanged(SDL_AudioDevice *new_default_device) if (current_default_device->logical_devices == NULL) { // nothing left on the current physical device, close it. // !!! FIXME: we _need_ to release this lock, but doing so can cause a race condition if someone opens a device while we're closing it. - SDL_UnlockMutex(current_default_device->lock); // can't hold the lock or the audio thread will deadlock while we WaitThread it. + RefPhysicalAudioDevice(current_default_device); // hold a temp ref for a moment while we release... + ReleaseAudioDevice(current_default_device); // can't hold the lock or the audio thread will deadlock while we WaitThread it. ClosePhysicalAudioDevice(current_default_device); - SDL_LockMutex(current_default_device->lock); // we're about to unlock this again, so make sure the locks match. + ObtainPhysicalAudioDeviceObj(current_default_device); // we're about to unlock this again, so make sure the locks match. + UnrefPhysicalAudioDevice(current_default_device); // drop temp ref. } } - SDL_UnlockMutex(current_default_device->lock); + ReleaseAudioDevice(current_default_device); } - SDL_UnlockMutex(new_default_device->lock); + ReleaseAudioDevice(new_default_device); - // was current device already dead and just kept around to migrate to a new default device? Now we can kill it. Aim for the brain. - if (current_default_device && SDL_AtomicGet(¤t_default_device->zombie)) { + // Default device gets an extra ref, so it lives until a new default replaces it, even if disconnected. + if (current_default_device) { // (despite the name, it's no longer current at this point) UnrefPhysicalAudioDevice(current_default_device); } @@ -2136,9 +2197,9 @@ int SDL_AudioDeviceFormatChangedAlreadyLocked(SDL_AudioDevice *device, const SDL int SDL_AudioDeviceFormatChanged(SDL_AudioDevice *device, const SDL_AudioSpec *newspec, int new_sample_frames) { - SDL_LockMutex(device->lock); + ObtainPhysicalAudioDeviceObj(device); const int retval = SDL_AudioDeviceFormatChangedAlreadyLocked(device, newspec, new_sample_frames); - SDL_UnlockMutex(device->lock); + ReleaseAudioDevice(device); return retval; }