Commit dc62fec5e99add18d2805743a3dda53826003c2b

Ryan C. Gordon 2022-05-20T21:07:25

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.

diff --git a/src/audio/coreaudio/SDL_coreaudio.m b/src/audio/coreaudio/SDL_coreaudio.m
index 304599f..5c267fb 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 722f5d0..bfe4282 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 bb6b78c..aec5f4a 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 bf48165..c38070d 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 8348a34..13d8f8c 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 7e318f1..387028e 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;