From dc62fec5e99add18d2805743a3dda53826003c2b Mon Sep 17 00:00:00 2001
From: "Ryan C. Gordon" <[EMAIL REDACTED]>
Date: Fri, 20 May 2022 21:07:25 -0400
Subject: [PATCH] audio: Fix locking in backends that manage their own callback
threads.
Otherwise you might get a race where an app pauses the device, but
the audio callback still manages to run after the pause is in place.
---
src/audio/coreaudio/SDL_coreaudio.m | 12 +++---
src/audio/emscripten/SDL_emscriptenaudio.c | 5 +++
src/audio/haiku/SDL_haikuaudio.cc | 47 +++++++++++----------
src/audio/nacl/SDL_naclaudio.c | 49 ++++++++++------------
src/audio/nacl/SDL_naclaudio.h | 3 +-
src/audio/pipewire/SDL_pipewire.c | 7 +---
6 files changed, 61 insertions(+), 62 deletions(-)
diff --git a/src/audio/coreaudio/SDL_coreaudio.m b/src/audio/coreaudio/SDL_coreaudio.m
index 304599fff9f..5c267fb3642 100644
--- a/src/audio/coreaudio/SDL_coreaudio.m
+++ b/src/audio/coreaudio/SDL_coreaudio.m
@@ -522,8 +522,12 @@ static BOOL update_audio_session(_THIS, SDL_bool open, SDL_bool allow_playandrec
outputCallback(void *inUserData, AudioQueueRef inAQ, AudioQueueBufferRef inBuffer)
{
SDL_AudioDevice *this = (SDL_AudioDevice *) inUserData;
+
+ SDL_LockMutex(this->mixer_lock);
+
if (SDL_AtomicGet(&this->hidden->shutdown)) {
- return; /* don't do anything. */
+ SDL_UnlockMutex(this->mixer_lock);
+ return; /* don't do anything, since we don't even want to enqueue this buffer again. */
}
if (!SDL_AtomicGet(&this->enabled) || SDL_AtomicGet(&this->paused)) {
@@ -536,10 +540,8 @@ static BOOL update_audio_session(_THIS, SDL_bool open, SDL_bool allow_playandrec
while (remaining > 0) {
if (SDL_AudioStreamAvailable(this->stream) == 0) {
/* Generate the data */
- SDL_LockMutex(this->mixer_lock);
(*this->callbackspec.callback)(this->callbackspec.userdata,
this->hidden->buffer, this->hidden->bufferSize);
- SDL_UnlockMutex(this->mixer_lock);
this->hidden->bufferOffset = 0;
SDL_AudioStreamPut(this->stream, this->hidden->buffer, this->hidden->bufferSize);
}
@@ -565,10 +567,8 @@ static BOOL update_audio_session(_THIS, SDL_bool open, SDL_bool allow_playandrec
UInt32 len;
if (this->hidden->bufferOffset >= this->hidden->bufferSize) {
/* Generate the data */
- SDL_LockMutex(this->mixer_lock);
(*this->callbackspec.callback)(this->callbackspec.userdata,
this->hidden->buffer, this->hidden->bufferSize);
- SDL_UnlockMutex(this->mixer_lock);
this->hidden->bufferOffset = 0;
}
@@ -587,6 +587,8 @@ static BOOL update_audio_session(_THIS, SDL_bool open, SDL_bool allow_playandrec
AudioQueueEnqueueBuffer(this->hidden->audioQueue, inBuffer, 0, NULL);
inBuffer->mAudioDataByteSize = inBuffer->mAudioDataBytesCapacity;
+
+ SDL_UnlockMutex(this->mixer_lock);
}
static void
diff --git a/src/audio/emscripten/SDL_emscriptenaudio.c b/src/audio/emscripten/SDL_emscriptenaudio.c
index 722f5d0fae6..bfe42821a48 100644
--- a/src/audio/emscripten/SDL_emscriptenaudio.c
+++ b/src/audio/emscripten/SDL_emscriptenaudio.c
@@ -28,6 +28,11 @@
#include <emscripten/emscripten.h>
+/* !!! FIXME: this currently expects that the audio callback runs in the main thread,
+ !!! FIXME: in intervals when the application isn't running, but that may not be
+ !!! FIXME: true always once pthread support becomes widespread. Revisit this code
+ !!! FIXME: at some point and see what needs to be done for that! */
+
static void
FeedAudioDevice(_THIS, const void *buf, const int buflen)
{
diff --git a/src/audio/haiku/SDL_haikuaudio.cc b/src/audio/haiku/SDL_haikuaudio.cc
index bb6b78c1d16..aec5f4a2551 100644
--- a/src/audio/haiku/SDL_haikuaudio.cc
+++ b/src/audio/haiku/SDL_haikuaudio.cc
@@ -49,39 +49,40 @@ FillSound(void *device, void *stream, size_t len,
SDL_AudioDevice *audio = (SDL_AudioDevice *) device;
SDL_AudioCallback callback = audio->callbackspec.callback;
+ SDL_LockMutex(audio->mixer_lock);
+
/* Only do something if audio is enabled */
if (!SDL_AtomicGet(&audio->enabled) || SDL_AtomicGet(&audio->paused)) {
if (audio->stream) {
SDL_AudioStreamClear(audio->stream);
}
SDL_memset(stream, audio->spec.silence, len);
- return;
- }
-
- SDL_assert(audio->spec.size == len);
-
- if (audio->stream == NULL) { /* no conversion necessary. */
- SDL_LockMutex(audio->mixer_lock);
- callback(audio->callbackspec.userdata, (Uint8 *) stream, len);
- SDL_UnlockMutex(audio->mixer_lock);
- } else { /* streaming/converting */
- const int stream_len = audio->callbackspec.size;
- const int ilen = (int) len;
- while (SDL_AudioStreamAvailable(audio->stream) < ilen) {
- callback(audio->callbackspec.userdata, audio->work_buffer, stream_len);
- if (SDL_AudioStreamPut(audio->stream, audio->work_buffer, stream_len) == -1) {
- SDL_AudioStreamClear(audio->stream);
- SDL_AtomicSet(&audio->enabled, 0);
- break;
+ } else {
+ SDL_assert(audio->spec.size == len);
+
+ if (audio->stream == NULL) { /* no conversion necessary. */
+ callback(audio->callbackspec.userdata, (Uint8 *) stream, len);
+ } else { /* streaming/converting */
+ const int stream_len = audio->callbackspec.size;
+ const int ilen = (int) len;
+ while (SDL_AudioStreamAvailable(audio->stream) < ilen) {
+ callback(audio->callbackspec.userdata, audio->work_buffer, stream_len);
+ if (SDL_AudioStreamPut(audio->stream, audio->work_buffer, stream_len) == -1) {
+ SDL_AudioStreamClear(audio->stream);
+ SDL_AtomicSet(&audio->enabled, 0);
+ break;
+ }
}
- }
- const int got = SDL_AudioStreamGet(audio->stream, stream, ilen);
- SDL_assert((got < 0) || (got == ilen));
- if (got != ilen) {
- SDL_memset(stream, audio->spec.silence, len);
+ const int got = SDL_AudioStreamGet(audio->stream, stream, ilen);
+ SDL_assert((got < 0) || (got == ilen));
+ if (got != ilen) {
+ SDL_memset(stream, audio->spec.silence, len);
+ }
}
}
+
+ SDL_UnlockMutex(audio->mixer_lock);
}
static void
diff --git a/src/audio/nacl/SDL_naclaudio.c b/src/audio/nacl/SDL_naclaudio.c
index bf48165f53a..c38070d8d8d 100644
--- a/src/audio/nacl/SDL_naclaudio.c
+++ b/src/audio/nacl/SDL_naclaudio.c
@@ -49,8 +49,8 @@ static void nacl_audio_callback(void* stream, uint32_t buffer_size, PP_TimeDelta
const int len = (int) buffer_size;
SDL_AudioDevice* _this = (SDL_AudioDevice*) data;
SDL_AudioCallback callback = _this->callbackspec.callback;
-
- SDL_LockMutex(private->mutex); /* !!! FIXME: is this mutex necessary? */
+
+ SDL_LockMutex(_this->mixer_lock);
/* Only do something if audio is enabled */
if (!SDL_AtomicGet(&_this->enabled) || SDL_AtomicGet(&_this->paused)) {
@@ -58,34 +58,31 @@ static void nacl_audio_callback(void* stream, uint32_t buffer_size, PP_TimeDelta
SDL_AudioStreamClear(_this->stream);
}
SDL_memset(stream, _this->spec.silence, len);
- return;
- }
-
- SDL_assert(_this->spec.size == len);
-
- if (_this->stream == NULL) { /* no conversion necessary. */
- SDL_LockMutex(_this->mixer_lock);
- callback(_this->callbackspec.userdata, stream, len);
- SDL_UnlockMutex(_this->mixer_lock);
- } else { /* streaming/converting */
- const int stream_len = _this->callbackspec.size;
- while (SDL_AudioStreamAvailable(_this->stream) < len) {
- callback(_this->callbackspec.userdata, _this->work_buffer, stream_len);
- if (SDL_AudioStreamPut(_this->stream, _this->work_buffer, stream_len) == -1) {
- SDL_AudioStreamClear(_this->stream);
- SDL_AtomicSet(&_this->enabled, 0);
- break;
+ } else {
+ SDL_assert(_this->spec.size == len);
+
+ if (_this->stream == NULL) { /* no conversion necessary. */
+ callback(_this->callbackspec.userdata, stream, len);
+ } else { /* streaming/converting */
+ const int stream_len = _this->callbackspec.size;
+ while (SDL_AudioStreamAvailable(_this->stream) < len) {
+ callback(_this->callbackspec.userdata, _this->work_buffer, stream_len);
+ if (SDL_AudioStreamPut(_this->stream, _this->work_buffer, stream_len) == -1) {
+ SDL_AudioStreamClear(_this->stream);
+ SDL_AtomicSet(&_this->enabled, 0);
+ break;
+ }
}
- }
- const int got = SDL_AudioStreamGet(_this->stream, stream, len);
- SDL_assert((got < 0) || (got == len));
- if (got != len) {
- SDL_memset(stream, _this->spec.silence, len);
+ const int got = SDL_AudioStreamGet(_this->stream, stream, len);
+ SDL_assert((got < 0) || (got == len));
+ if (got != len) {
+ SDL_memset(stream, _this->spec.silence, len);
+ }
}
}
- SDL_UnlockMutex(private->mutex);
+ SDL_UnlockMutex(_this->mixer_lock);
}
static void NACLAUDIO_CloseDevice(SDL_AudioDevice *device) {
@@ -94,7 +91,6 @@ static void NACLAUDIO_CloseDevice(SDL_AudioDevice *device) {
SDL_PrivateAudioData *hidden = (SDL_PrivateAudioData *) device->hidden;
ppb_audio->StopPlayback(hidden->audio);
- SDL_DestroyMutex(hidden->mutex);
core->ReleaseResource(hidden->audio);
}
@@ -109,7 +105,6 @@ NACLAUDIO_OpenDevice(_THIS, const char *devname) {
return SDL_OutOfMemory();
}
- private->mutex = SDL_CreateMutex();
_this->spec.freq = 44100;
_this->spec.format = AUDIO_S16LSB;
_this->spec.channels = 2;
diff --git a/src/audio/nacl/SDL_naclaudio.h b/src/audio/nacl/SDL_naclaudio.h
index 8348a34e81d..13d8f8ca469 100644
--- a/src/audio/nacl/SDL_naclaudio.h
+++ b/src/audio/nacl/SDL_naclaudio.h
@@ -34,8 +34,7 @@
#define private _this->hidden
typedef struct SDL_PrivateAudioData {
- SDL_mutex* mutex;
- PP_Resource audio;
+ PP_Resource audio;
} SDL_PrivateAudioData;
#endif /* SDL_naclaudio_h_ */
diff --git a/src/audio/pipewire/SDL_pipewire.c b/src/audio/pipewire/SDL_pipewire.c
index 7e318f1bc48..387028edb27 100644
--- a/src/audio/pipewire/SDL_pipewire.c
+++ b/src/audio/pipewire/SDL_pipewire.c
@@ -910,6 +910,7 @@ output_callback(void *data)
* and run the callback with the work buffer to keep the callback
* firing regularly in case the audio is being used as a timer.
*/
+ SDL_LockMutex(this->mixer_lock);
if (!SDL_AtomicGet(&this->paused)) {
if (SDL_AtomicGet(&this->enabled)) {
dst = spa_buf->datas[0].data;
@@ -919,18 +920,13 @@ output_callback(void *data)
}
if (!this->stream) {
- SDL_LockMutex(this->mixer_lock);
this->callbackspec.callback(this->callbackspec.userdata, dst, this->callbackspec.size);
- SDL_UnlockMutex(this->mixer_lock);
} else {
int got;
/* Fire the callback until we have enough to fill a buffer */
while (SDL_AudioStreamAvailable(this->stream) < this->spec.size) {
- SDL_LockMutex(this->mixer_lock);
this->callbackspec.callback(this->callbackspec.userdata, this->work_buffer, this->callbackspec.size);
- SDL_UnlockMutex(this->mixer_lock);
-
SDL_AudioStreamPut(this->stream, this->work_buffer, this->callbackspec.size);
}
@@ -940,6 +936,7 @@ output_callback(void *data)
} else {
SDL_memset(spa_buf->datas[0].data, this->spec.silence, this->spec.size);
}
+ SDL_UnlockMutex(this->mixer_lock);
spa_buf->datas[0].chunk->offset = 0;
spa_buf->datas[0].chunk->stride = this->hidden->stride;