From dc62fec5e99add18d2805743a3dda53826003c2b Mon Sep 17 00:00:00 2001 From: "Ryan C. Gordon" 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 | 45 +++++++++++---------- src/audio/nacl/SDL_naclaudio.c | 47 ++++++++++------------ src/audio/nacl/SDL_naclaudio.h | 3 +- src/audio/pipewire/SDL_pipewire.c | 7 +--- 6 files changed, 59 insertions(+), 60 deletions(-) diff --git a/src/audio/coreaudio/SDL_coreaudio.m b/src/audio/coreaudio/SDL_coreaudio.m index 304599fff..5c267fb36 100644 --- a/src/audio/coreaudio/SDL_coreaudio.m +++ b/src/audio/coreaudio/SDL_coreaudio.m @@ -522,8 +522,12 @@ static void 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 @@ outputCallback(void *inUserData, AudioQueueRef inAQ, AudioQueueBufferRef inBuffe 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 @@ outputCallback(void *inUserData, AudioQueueRef inAQ, AudioQueueBufferRef inBuffe 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 @@ outputCallback(void *inUserData, AudioQueueRef inAQ, AudioQueueBufferRef inBuffe 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 722f5d0fa..bfe42821a 100644 --- a/src/audio/emscripten/SDL_emscriptenaudio.c +++ b/src/audio/emscripten/SDL_emscriptenaudio.c @@ -28,6 +28,11 @@ #include +/* !!! 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 bb6b78c1d..aec5f4a25 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; - } + } else { + SDL_assert(audio->spec.size == len); - 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; + } + } - 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; + 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 bf48165f5..c38070d8d 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; - } + } else { + SDL_assert(_this->spec.size == len); - 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; + } + } - 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; + 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 8348a34e8..13d8f8ca4 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 7e318f1bc..387028edb 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;