audio: Clean up some CloseDevice() interface details.

- It's now always called if device->hidden isn't NULL, even if OpenDevice() failed halfway through. This lets implementation code not have to clean up itself on every possible failure point; just return an error and SDL will handle it for you. - Implementations can assume this->hidden != NULL and not check for it. - implementations don't have to set this->hidden = NULL when done, because the caller is always about to free(this). - Don't reset other fields that are in a block of memory about to be free()'d. - Implementations all now free things like internal mix buffers last, after closing devices and such, to guarantee they definitely aren't in use anymore at the point of deallocation.
libsdl-org · Aug 5, 2016 · 9b64772 · 9b64772
1 parent 30a9139
commit 9b64772
Show file tree

Hide file tree

Showing 24 changed files with 168 additions and 370 deletions.
diff --git a/src/audio/SDL_audio.c b/src/audio/SDL_audio.c
@@ -982,9 +982,8 @@ close_audio_device(SDL_AudioDevice * device)
     if (device->convert.needed) {
         SDL_FreeAudioMem(device->convert.buf);
     }
-    if (device->opened) {
+    if (device->hidden != NULL) {
         current_audio.impl.CloseDevice(device);
-        device->opened = SDL_FALSE;
     }
 
     free_audio_queue(device->buffer_queue_head);
@@ -1193,7 +1192,10 @@ open_audio_device(const char *devname, int iscapture,
         close_audio_device(device);
         return 0;
     }
-    device->opened = SDL_TRUE;
+
+    /* if your target really doesn't need it, set it to 0x1 or something. */
+    /* otherwise, close_audio_device() won't call impl.CloseDevice(). */
+    SDL_assert(device->hidden != NULL);
 
     /* See if we need to do any conversion */
     build_cvt = SDL_FALSE;

diff --git a/src/audio/SDL_sysaudio.h b/src/audio/SDL_sysaudio.h
@@ -162,7 +162,6 @@ struct SDL_AudioDevice
     SDL_atomic_t shutdown; /* true if we are signaling the play thread to end. */
     SDL_atomic_t enabled;  /* true if device is functioning and connected. */
     SDL_atomic_t paused;
-    SDL_bool opened;
     SDL_bool iscapture;
 
     /* Fake audio buffer for when the audio hardware is busy */

diff --git a/src/audio/alsa/SDL_alsa_audio.c b/src/audio/alsa/SDL_alsa_audio.c
@@ -402,17 +402,12 @@ ALSA_FlushCapture(_THIS)
 static void
 ALSA_CloseDevice(_THIS)
 {
-    if (this->hidden != NULL) {
-        SDL_FreeAudioMem(this->hidden->mixbuf);
-        this->hidden->mixbuf = NULL;
-        if (this->hidden->pcm_handle) {
-            ALSA_snd_pcm_drain(this->hidden->pcm_handle);
-            ALSA_snd_pcm_close(this->hidden->pcm_handle);
-            this->hidden->pcm_handle = NULL;
-        }
-        SDL_free(this->hidden);
-        this->hidden = NULL;
+    if (this->hidden->pcm_handle) {
+        ALSA_snd_pcm_drain(this->hidden->pcm_handle);
+        ALSA_snd_pcm_close(this->hidden->pcm_handle);
     }
+    SDL_FreeAudioMem(this->hidden->mixbuf);
+    SDL_free(this->hidden);
 }
 
 static int
@@ -555,7 +550,6 @@ ALSA_OpenDevice(_THIS, void *handle, const char *devname, int iscapture)
                 SND_PCM_NONBLOCK);
 
     if (status < 0) {
-        ALSA_CloseDevice(this);
         return SDL_SetError("ALSA: Couldn't open audio device: %s",
                             ALSA_snd_strerror(status));
     }
@@ -566,7 +560,6 @@ ALSA_OpenDevice(_THIS, void *handle, const char *devname, int iscapture)
     snd_pcm_hw_params_alloca(&hwparams);
     status = ALSA_snd_pcm_hw_params_any(pcm_handle, hwparams);
     if (status < 0) {
-        ALSA_CloseDevice(this);
         return SDL_SetError("ALSA: Couldn't get hardware config: %s",
                             ALSA_snd_strerror(status));
     }
@@ -575,7 +568,6 @@ ALSA_OpenDevice(_THIS, void *handle, const char *devname, int iscapture)
     status = ALSA_snd_pcm_hw_params_set_access(pcm_handle, hwparams,
                                                SND_PCM_ACCESS_RW_INTERLEAVED);
     if (status < 0) {
-        ALSA_CloseDevice(this);
         return SDL_SetError("ALSA: Couldn't set interleaved access: %s",
                      ALSA_snd_strerror(status));
     }
@@ -629,7 +621,6 @@ ALSA_OpenDevice(_THIS, void *handle, const char *devname, int iscapture)
         }
     }
     if (status < 0) {
-        ALSA_CloseDevice(this);
         return SDL_SetError("ALSA: Couldn't find any hardware audio formats");
     }
     this->spec.format = test_format;
@@ -641,7 +632,6 @@ ALSA_OpenDevice(_THIS, void *handle, const char *devname, int iscapture)
     if (status < 0) {
         status = ALSA_snd_pcm_hw_params_get_channels(hwparams, &channels);
         if (status < 0) {
-            ALSA_CloseDevice(this);
             return SDL_SetError("ALSA: Couldn't set audio channels");
         }
         this->spec.channels = channels;
@@ -652,7 +642,6 @@ ALSA_OpenDevice(_THIS, void *handle, const char *devname, int iscapture)
     status = ALSA_snd_pcm_hw_params_set_rate_near(pcm_handle, hwparams,
                                                   &rate, NULL);
     if (status < 0) {
-        ALSA_CloseDevice(this);
         return SDL_SetError("ALSA: Couldn't set audio frequency: %s",
                             ALSA_snd_strerror(status));
     }
@@ -664,34 +653,29 @@ ALSA_OpenDevice(_THIS, void *handle, const char *devname, int iscapture)
         /* Failed to set desired buffer size, do the best you can... */
         status = ALSA_set_period_size(this, hwparams, 1);
         if (status < 0) {
-            ALSA_CloseDevice(this);
             return SDL_SetError("Couldn't set hardware audio parameters: %s", ALSA_snd_strerror(status));
         }
     }
     /* Set the software parameters */
     snd_pcm_sw_params_alloca(&swparams);
     status = ALSA_snd_pcm_sw_params_current(pcm_handle, swparams);
     if (status < 0) {
-        ALSA_CloseDevice(this);
         return SDL_SetError("ALSA: Couldn't get software config: %s",
                             ALSA_snd_strerror(status));
     }
     status = ALSA_snd_pcm_sw_params_set_avail_min(pcm_handle, swparams, this->spec.samples);
     if (status < 0) {
-        ALSA_CloseDevice(this);
         return SDL_SetError("Couldn't set minimum available samples: %s",
                             ALSA_snd_strerror(status));
     }
     status =
         ALSA_snd_pcm_sw_params_set_start_threshold(pcm_handle, swparams, 1);
     if (status < 0) {
-        ALSA_CloseDevice(this);
         return SDL_SetError("ALSA: Couldn't set start threshold: %s",
                             ALSA_snd_strerror(status));
     }
     status = ALSA_snd_pcm_sw_params(pcm_handle, swparams);
     if (status < 0) {
-        ALSA_CloseDevice(this);
         return SDL_SetError("Couldn't set software audio parameters: %s",
                             ALSA_snd_strerror(status));
     }
@@ -704,7 +688,6 @@ ALSA_OpenDevice(_THIS, void *handle, const char *devname, int iscapture)
         this->hidden->mixlen = this->spec.size;
         this->hidden->mixbuf = (Uint8 *) SDL_AllocAudioMem(this->hidden->mixlen);
         if (this->hidden->mixbuf == NULL) {
-            ALSA_CloseDevice(this);
             return SDL_OutOfMemory();
         }
         SDL_memset(this->hidden->mixbuf, this->spec.silence, this->hidden->mixlen);

diff --git a/src/audio/android/SDL_androidaudio.c b/src/audio/android/SDL_androidaudio.c
@@ -44,6 +44,7 @@ AndroidAUD_OpenDevice(_THIS, void *handle, const char *devname, int iscapture)
         return SDL_SetError("Capture not supported on Android");
     }
 
+    /* !!! FIXME: higher level will prevent this now. Lose this check (and global?). */
     if (audioDevice != NULL) {
         return SDL_SetError("Only one audio device at a time please!");
     }
@@ -115,10 +116,7 @@ AndroidAUD_CloseDevice(_THIS)
     Android_JNI_CloseAudioDevice();
 
     if (audioDevice == this) {
-        if (audioDevice->hidden != NULL) {
-            SDL_free(this->hidden);
-            this->hidden = NULL;
-        }
+        SDL_free(this->hidden);
         audioDevice = NULL;
     }
 }

diff --git a/src/audio/arts/SDL_artsaudio.c b/src/audio/arts/SDL_artsaudio.c
@@ -203,17 +203,12 @@ ARTS_GetDeviceBuf(_THIS)
 static void
 ARTS_CloseDevice(_THIS)
 {
-    if (this->hidden != NULL) {
-        SDL_FreeAudioMem(this->hidden->mixbuf);
-        this->hidden->mixbuf = NULL;
-        if (this->hidden->stream) {
-            SDL_NAME(arts_close_stream) (this->hidden->stream);
-            this->hidden->stream = 0;
-        }
-        SDL_NAME(arts_free) ();
-        SDL_free(this->hidden);
-        this->hidden = NULL;
+    if (this->hidden->stream) {
+        SDL_NAME(arts_close_stream) (this->hidden->stream);
     }
+    SDL_NAME(arts_free) ();
+    SDL_FreeAudioMem(this->hidden->mixbuf);
+    SDL_free(this->hidden);
 }
 
 static int
@@ -267,19 +262,16 @@ ARTS_OpenDevice(_THIS, void *handle, const char *devname, int iscapture)
         }
     }
     if (format == 0) {
-        ARTS_CloseDevice(this);
         return SDL_SetError("Couldn't find any hardware audio formats");
     }
     this->spec.format = test_format;
 
     if ((rc = SDL_NAME(arts_init) ()) != 0) {
-        ARTS_CloseDevice(this);
         return SDL_SetError("Unable to initialize ARTS: %s",
                             SDL_NAME(arts_error_text) (rc));
     }
 
     if (!ARTS_Suspend()) {
-        ARTS_CloseDevice(this);
         return SDL_SetError("ARTS can not open audio device");
     }
 
@@ -297,7 +289,6 @@ ARTS_OpenDevice(_THIS, void *handle, const char *devname, int iscapture)
     /* Determine the power of two of the fragment size */
     for (frag_spec = 0; (0x01 << frag_spec) < this->spec.size; ++frag_spec);
     if ((0x01 << frag_spec) != this->spec.size) {
-        ARTS_CloseDevice(this);
         return SDL_SetError("Fragment size must be a power of two");
     }
     frag_spec |= 0x00020000;    /* two fragments, for low latency */
@@ -318,7 +309,6 @@ ARTS_OpenDevice(_THIS, void *handle, const char *devname, int iscapture)
     this->hidden->mixlen = this->spec.size;
     this->hidden->mixbuf = (Uint8 *) SDL_AllocAudioMem(this->hidden->mixlen);
     if (this->hidden->mixbuf == NULL) {
-        ARTS_CloseDevice(this);
         return SDL_OutOfMemory();
     }
     SDL_memset(this->hidden->mixbuf, this->spec.silence, this->spec.size);

diff --git a/src/audio/bsd/SDL_bsdaudio.c b/src/audio/bsd/SDL_bsdaudio.c
@@ -268,16 +268,11 @@ BSDAUDIO_FlushCapture(_THIS)
 static void
 BSDAUDIO_CloseDevice(_THIS)
 {
-    if (this->hidden != NULL) {
-        SDL_FreeAudioMem(this->hidden->mixbuf);
-        this->hidden->mixbuf = NULL;
-        if (this->hidden->audio_fd >= 0) {
-            close(this->hidden->audio_fd);
-            this->hidden->audio_fd = -1;
-        }
-        SDL_free(this->hidden);
-        this->hidden = NULL;
+    if (this->hidden->audio_fd >= 0) {
+        close(this->hidden->audio_fd);
     }
+    SDL_FreeAudioMem(this->hidden->mixbuf);
+    SDL_free(this->hidden);
 }
 
 static int
@@ -319,7 +314,6 @@ BSDAUDIO_OpenDevice(_THIS, void *handle, const char *devname, int iscapture)
     /* Set to play mode */
     info.mode = iscapture ? AUMODE_RECORD : AUMODE_PLAY;
     if (ioctl(this->hidden->audio_fd, AUDIO_SETINFO, &info) < 0) {
-        BSDAUDIO_CloseDevice(this);
         return SDL_SetError("Couldn't put device into play mode");
     }
 
@@ -361,7 +355,6 @@ BSDAUDIO_OpenDevice(_THIS, void *handle, const char *devname, int iscapture)
     }
 
     if (!format) {
-        BSDAUDIO_CloseDevice(this);
         return SDL_SetError("No supported encoding for 0x%x", this->spec.format);
     }
 
@@ -386,7 +379,6 @@ BSDAUDIO_OpenDevice(_THIS, void *handle, const char *devname, int iscapture)
         this->hidden->mixlen = this->spec.size;
         this->hidden->mixbuf = (Uint8 *) SDL_AllocAudioMem(this->hidden->mixlen);
         if (this->hidden->mixbuf == NULL) {
-            BSDAUDIO_CloseDevice(this);
             return SDL_OutOfMemory();
         }
         SDL_memset(this->hidden->mixbuf, this->spec.silence, this->spec.size);

diff --git a/src/audio/coreaudio/SDL_coreaudio.c b/src/audio/coreaudio/SDL_coreaudio.c
@@ -22,6 +22,8 @@
 
 #if SDL_AUDIO_DRIVER_COREAUDIO
 
+/* !!! FIXME: clean out some of the macro salsa in here. */
+
 #include "SDL_audio.h"
 #include "../SDL_audio_c.h"
 #include "../SDL_sysaudio.h"
@@ -30,11 +32,8 @@
 
 #define DEBUG_COREAUDIO 0
 
-static void COREAUDIO_CloseDevice(_THIS);
-
 #define CHECK_RESULT(msg) \
     if (result != noErr) { \
-        COREAUDIO_CloseDevice(this); \
         SDL_SetError("CoreAudio error (%s): %d", msg, (int) result); \
         return 0; \
     }
@@ -436,45 +435,39 @@ device_unplugged(AudioObjectID devid, UInt32 num_addr, const AudioObjectProperty
 static void
 COREAUDIO_CloseDevice(_THIS)
 {
-    if (this->hidden != NULL) {
-        const int iscapture = this->iscapture;
-        if (this->hidden->audioUnitOpened) {
-            #if MACOSX_COREAUDIO
-            /* Unregister our disconnect callback. */
-            AudioObjectRemovePropertyListener(this->hidden->deviceID, &alive_address, device_unplugged, this);
-            #endif
-
-            AURenderCallbackStruct callback;
-            const AudioUnitElement output_bus = 0;
-            const AudioUnitElement input_bus = 1;
-            const AudioUnitElement bus =
-                ((iscapture) ? input_bus : output_bus);
-
-            /* stop processing the audio unit */
-            AudioOutputUnitStop(this->hidden->audioUnit);
-
-            /* Remove the input callback */
-            SDL_zero(callback);
-            AudioUnitSetProperty(this->hidden->audioUnit,
-                                 iscapture ? kAudioOutputUnitProperty_SetInputCallback : kAudioUnitProperty_SetRenderCallback,
-                                 kAudioUnitScope_Global, bus, &callback, sizeof(callback));
-            AudioComponentInstanceDispose(this->hidden->audioUnit);
-            this->hidden->audioUnitOpened = 0;
-
-            SDL_free(this->hidden->captureBufferList.mBuffers[0].mData);
+    const int iscapture = this->iscapture;
+    if (this->hidden->audioUnitOpened) {
+        #if MACOSX_COREAUDIO
+        /* Unregister our disconnect callback. */
+        AudioObjectRemovePropertyListener(this->hidden->deviceID, &alive_address, device_unplugged, this);
+        #endif
+
+        AURenderCallbackStruct callback;
+        const AudioUnitElement output_bus = 0;
+        const AudioUnitElement input_bus = 1;
+        const AudioUnitElement bus = ((iscapture) ? input_bus : output_bus);
+
+        /* stop processing the audio unit */
+        AudioOutputUnitStop(this->hidden->audioUnit);
+
+        /* Remove the input callback */
+        SDL_zero(callback);
+        AudioUnitSetProperty(this->hidden->audioUnit,
+                             iscapture ? kAudioOutputUnitProperty_SetInputCallback : kAudioUnitProperty_SetRenderCallback,
+                             kAudioUnitScope_Global, bus, &callback, sizeof(callback));
+        AudioComponentInstanceDispose(this->hidden->audioUnit);
+    }
 
-        }
-        SDL_free(this->hidden->buffer);
-        SDL_free(this->hidden);
-        this->hidden = NULL;
+    SDL_free(this->hidden->captureBufferList.mBuffers[0].mData);
+    SDL_free(this->hidden->buffer);
+    SDL_free(this->hidden);
 
-        if (iscapture) {
-            open_capture_devices--;
-        } else {
-            open_playback_devices--;
-        }
-        update_audio_session();
+    if (iscapture) {
+        open_capture_devices--;
+    } else {
+        open_playback_devices--;
     }
+    update_audio_session();
 }
 
 #if MACOSX_COREAUDIO
@@ -623,7 +616,6 @@ prepare_audiounit(_THIS, void *handle, int iscapture,
         framesize *= SDL_AUDIO_BITSIZE(this->spec.format) / 8;
         ptr = SDL_calloc(1, framesize);
         if (ptr == NULL) {
-            COREAUDIO_CloseDevice(this);
             SDL_OutOfMemory();
             return 0;
         }
@@ -655,7 +647,6 @@ prepare_audiounit(_THIS, void *handle, int iscapture,
 
     this->hidden->buffer = SDL_malloc(this->hidden->bufferSize);
     if (this->hidden->buffer == NULL) {
-        COREAUDIO_CloseDevice(this);
         SDL_OutOfMemory();
         return 0;
     }
@@ -737,7 +728,6 @@ COREAUDIO_OpenDevice(_THIS, void *handle, const char *devname, int iscapture)
     }
 
     if (!valid_datatype) {      /* shouldn't happen, but just in case... */
-        COREAUDIO_CloseDevice(this);
         return SDL_SetError("Unsupported audio format");
     }
 
@@ -747,7 +737,6 @@ COREAUDIO_OpenDevice(_THIS, void *handle, const char *devname, int iscapture)
         strdesc.mBytesPerFrame * strdesc.mFramesPerPacket;
 
     if (!prepare_audiounit(this, handle, iscapture, &strdesc)) {
-        COREAUDIO_CloseDevice(this);
         return -1;      /* prepare_audiounit() will call SDL_SetError()... */
     }