Here’s that patch I promised. It looks like a previous patch of mine never
made it into CVS (which might have been exasperating the bug under some
situations), or I’m confused. At either rate, this should fix everyone’s
woes, with two caveats:
-
It requires SDL_LockAudio() to block all calling threads; that is, it
needs to block until the audio callback is not running (and then prevent
it from running again until SDL_UnlockAudio()), and then grant serialized
access to all other calling threads. As SDL_LockAudio() is a simple mutex
on all platforms but MacOS Classic, this requirement is satisfied.
The nature of MacOS Classic satisfies this requirement for other reasons.
Mostly I mention this for future SDL ports and audio targets to keep in
mind. -
It is NEVER safe to call an SDL_mixer API function from inside the
mixer’s audio callback, since almost all of them now call SDL_LockAudio().
Just because this works on Linux and Win32 does NOT mean it is safe to
do this portably (it will cause audio corruption or worse on MacOS
Classic, for starters). The danger of calling SDL_LockAudio() from the SDL
audio callback is well-documented. There are a few places you might be
tempted to do this: if you write a postmix or effect function, they run in
the audio callback. More importantly (since playwave.c does this), the
callbacks that alert you that a chunk is done playing run from the audio
callback…and you can’t call Mix_FreeChunk() there anymore. I’m inclined
to either change Mix_FreeChunk() to not check if the chunk is playing, or
add a Mix_FreeChunkNoCheck() or something.
For all that babbling, you probably won’t need to worry about any of this.
I’m just covering my ass here.
Patch is attached. Apply to latest CVS.
cd /where/i/keep/SDL_mixer ; patch -p0 < SDL_mixer-lockaudio-fix-RYAN-3.diff
–ryan.
-------------- next part --------------
Index: mixer.c===================================================================
RCS file: /home/slouken/libsdl.org/cvs/SDL_mixer/mixer.c,v
retrieving revision 1.35
diff -u -r1.35 mixer.c
— mixer.c 2002/03/24 02:06:12 1.35
+++ mixer.c 2002/05/01 20:46:10
@@ -40,9 +40,7 @@
#define FORM 0x4d524f46 /* “FORM” */
static int audio_opened = 0;
static SDL_AudioSpec mixer;
-static SDL_mutex *mixer_lock;
typedef struct _Mix_effectinfo
{
@@ -101,15 +99,28 @@
return(&linked_mixver);
}
+static int _Mix_remove_all_effects(int channel, effect_info **e);
/* rcg06122001 Cleanup effect callbacks. */
-static void Mix_ChannelDonePlaying(int channel)
+static void _Mix_channel_done_playing(int channel, int lockaudio)
{
- if (lockaudio) {
-
SDL_LockAudio();
- }
- if (channel_done_callback) {
channel_done_callback(channel);
}
- Mix_UnregisterAllEffects(channel);
- /*
-
- Call internal function directly, to avoid locking audio from
-
- inside audio callback.
- */
- _Mix_remove_all_effects(channel, &mix_channel[channel].effects);
- if (lockaudio) {
-
SDL_UnlockAudio();
- }
}
@@ -153,8 +164,7 @@
mix_music(music_data, stream, len);
}
- /* Grab the channels we need to mix */
- SDL_mutexP(mixer_lock);
-
/* Mix any playing channels… */
sdl_ticks = SDL_GetTicks();
for ( i=0; i<num_channels; ++i ) {
if( ! mix_channel[i].paused ) {
@@ -223,7 +233,7 @@/* rcg06072001 Alert app if channel is done playing. */ if (!mix_channel[i].playing) {
-
Mix_ChannelDonePlaying(i);
-
_Mix_channel_done_playing(i, 0); } } }
@@ -235,7 +245,6 @@
if ( mix_postmix ) {
mix_postmix(mix_postmix_data, stream, len);
}
- SDL_mutexV(mixer_lock);
}
static void PrintFormat(char *title, SDL_AudioSpec *fmt)
@@ -276,20 +285,9 @@
PrintFormat(“Audio device”, &mixer);
#endif
- /* Create the channel lock mutex */
- mixer_lock = SDL_CreateMutex();
-#ifndef macintosh /* Hmm… what implications does this have? */ - if ( mixer_lock == NULL ) {
-
SDL_CloseAudio();
-
SDL_SetError("Unable to create mixer lock");
-
return(-1);
- }
-#endif - /* Initialize the music players */
if ( open_music(&mixer) < 0 ) {
SDL_CloseAudio(); -
}SDL_DestroyMutex(mixer_lock); return(-1);
@@ -331,7 +329,7 @@
Mix_HaltChannel(i);
}
}
- SDL_mutexP(mixer_lock);
- SDL_LockAudio();
mix_channel = (struct _Mix_Channel ) realloc(mix_channel, numchans * sizeof(struct _Mix_Channel));
if ( numchans > num_channels ) {
/ Initialize the new channels */
@@ -347,7 +345,7 @@
}
}
num_channels = numchans;
- SDL_mutexV(mixer_lock);
- SDL_UnlockAudio();
return(num_channels);
}
@@ -514,16 +512,15 @@
/* Caution – if the chunk is playing, the mixer will crash /
if ( chunk ) {
/ Guarantee that this chunk isn’t playing */
-
SDL_LockAudio(); if ( mix_channel ) {
-
SDL_mutexP(mixer_lock); for ( i=0; i<num_channels; ++i ) { if ( chunk == mix_channel[i].chunk ) { mix_channel[i].playing = 0; } }
-
SDL_mutexV(mixer_lock); }
-
SDL_UnlockAudio(); /* Actually free the chunk */ if ( chunk->allocated ) { free(chunk->abuf);
@@ -569,9 +566,9 @@
void Mix_ChannelFinished(void (*channel_finished)(int channel))
{
- SDL_LockAudio();
- channel_done_callback = channel_finished;
- SDL_UnlockAudio();
- SDL_LockAudio();
- channel_done_callback = channel_finished;
- SDL_UnlockAudio();
}
@@ -603,7 +600,7 @@
}
/* Lock the mixer while modifying the playing channels */
- SDL_mutexP(mixer_lock);
- SDL_LockAudio();
{
/* If which is -1, play on the first free channel */
if ( which == -1 ) {
@@ -622,7 +619,7 @@
if ( which >= 0 ) {
Uint32 sdl_ticks = SDL_GetTicks();
if (Mix_Playing(which))
-
Mix_ChannelDonePlaying(which);
-
_Mix_channel_done_playing(which, 0); mix_channel[which].samples = chunk->abuf; mix_channel[which].playing = chunk->alen; mix_channel[which].looping = loops;
@@ -633,7 +630,7 @@
mix_channel[which].expire = (ticks>0) ? (sdl_ticks + ticks) : 0;
}
}
- SDL_mutexV(mixer_lock);
-
SDL_UnlockAudio();
/* Return the channel on which the sound is being played */
return(which);
@@ -650,9 +647,9 @@
status += Mix_ExpireChannel(i, ticks);
}
} else if ( which < num_channels ) {
-
SDL_mutexP(mixer_lock);
-
SDL_LockAudio(); mix_channel[which].expire = (ticks>0) ? (SDL_GetTicks() + ticks) : 0;
-
SDL_mutexV(mixer_lock);
-
SDL_UnlockAudio(); ++ status;
}
return(status);
@@ -669,7 +666,7 @@
}/* Lock the mixer while modifying the playing channels */
- SDL_mutexP(mixer_lock);
- SDL_LockAudio();
{
/* If which is -1, play on the first free channel */
if ( which == -1 ) {
@@ -688,7 +685,7 @@
if ( which >= 0 ) {
Uint32 sdl_ticks = SDL_GetTicks();
if (Mix_Playing(which))
-
Mix_ChannelDonePlaying(which);
-
_Mix_channel_done_playing(which, 1); mix_channel[which].samples = chunk->abuf; mix_channel[which].playing = chunk->alen; mix_channel[which].looping = loops;
@@ -702,7 +699,7 @@
mix_channel[which].expire = (ticks > 0) ? (sdl_ticks+ticks) : 0;
}
}
- SDL_mutexV(mixer_lock);
-
SDL_UnlockAudio();
/* Return the channel on which the sound is being played */
return(which);
@@ -756,16 +753,16 @@
Mix_HaltChannel(i);
}
} else {
-
SDL_mutexP(mixer_lock);
-
SDL_LockAudio(); if (mix_channel[which].playing) {
-
Mix_ChannelDonePlaying(which);
-
_Mix_channel_done_playing(which, 1); mix_channel[which].playing = 0; } mix_channel[which].expire = 0; if(mix_channel[which].fading != MIX_NO_FADING) /* Restore volume */ mix_channel[which].volume = mix_channel[which].fade_volume; mix_channel[which].fading = MIX_NO_FADING;
-
SDL_mutexV(mixer_lock);
-
}SDL_UnlockAudio();
return(0);
}
@@ -797,7 +794,7 @@
status += Mix_FadeOutChannel(i, ms);
}
} else {
-
SDL_mutexP(mixer_lock);
-
SDL_LockAudio(); if ( mix_channel[which].playing && (mix_channel[which].volume > 0) && (mix_channel[which].fading != MIX_FADING_OUT) ) {
@@ -808,7 +805,7 @@
mix_channel[which].ticks_fade = SDL_GetTicks();
++status;
}
-
SDL_mutexV(mixer_lock);
-
}SDL_UnlockAudio(); }
return(status);
@@ -886,7 +883,6 @@
close_music();
Mix_HaltChannel(-1);
SDL_CloseAudio();
-
SDL_DestroyMutex(mixer_lock); free(mix_channel); mix_channel = NULL; }
@@ -917,10 +913,11 @@
void Mix_Resume(int which)
{
Uint32 sdl_ticks = SDL_GetTicks();
+
- SDL_LockAudio();
if ( which == -1 ) {
int i;
-
SDL_mutexP(mixer_lock); for ( i=0; i<num_channels; ++i ) { if ( mix_channel[i].playing > 0 ) { if(mix_channel[i].expire > 0)
@@ -928,16 +925,14 @@
mix_channel[i].paused = 0;
}
}
-
} else {SDL_mutexV(mixer_lock);
-
SDL_mutexP(mixer_lock); if ( mix_channel[which].playing > 0 ) { if(mix_channel[which].expire > 0) mix_channel[which].expire += sdl_ticks - mix_channel[which].paused; mix_channel[which].paused = 0; }
-
}SDL_mutexV(mixer_lock);
- SDL_UnlockAudio();
}
int Mix_Paused(int which)
@@ -964,9 +959,9 @@
if ( which < 0 || which > num_channels )
return(0);
- SDL_mutexP(mixer_lock);
- SDL_LockAudio();
mix_channel[which].tag = tag;
- SDL_mutexV(mixer_lock);
- SDL_UnlockAudio();
return(1);
}
@@ -1043,8 +1038,9 @@
- as Mix_SetPanning().
*/
+/* MAKE SURE you hold the audio lock (SDL_LockAudio()) before calling this! */
static int _Mix_register_effect(effect_info **e, Mix_EffectFunc_t f,
-
Mix_EffectDone_t d, void *arg)
-
Mix_EffectDone_t d, void *arg)
{
effect_info *new_e = malloc(sizeof (effect_info));
@@ -1068,8 +1064,6 @@
new_e->udata = arg;
new_e->next = NULL;
-
SDL_LockAudio();
-
/* add new effect to end of linked list… */
if (*e == NULL) {
*e = new_e;
@@ -1084,11 +1078,11 @@
}
} -
SDL_UnlockAudio();
return(1);
}
+/* MAKE SURE you hold the audio lock (SDL_LockAudio()) before calling this! */
static int _Mix_remove_effect(int channel, effect_info **e, Mix_EffectFunc_t f)
{
effect_info *cur;
@@ -1100,7 +1094,6 @@
return(0);
}
-
SDL_LockAudio();
for (cur = *e; cur != NULL; cur = cur->next) {
if (cur->callback == f) {
next = cur->next;
@@ -1114,18 +1107,17 @@
} else {
prev->next = next;
} -
SDL_UnlockAudio(); return(1); } prev = cur;
}
-
SDL_UnlockAudio();
Mix_SetError(“No such effect registered”);
return(0);
}
+/* MAKE SURE you hold the audio lock (SDL_LockAudio()) before calling this! */
static int _Mix_remove_all_effects(int channel, effect_info **e)
{
effect_info *cur;
@@ -1136,7 +1128,6 @@
return(0);
}
-
SDL_LockAudio();
for (cur = *e; cur != NULL; cur = next) {
next = cur->next;
if (cur->done_callback != NULL) {
@@ -1145,16 +1136,16 @@
free(cur);
}
*e = NULL; -
SDL_UnlockAudio();
return(1);
}
int Mix_RegisterEffect(int channel, Mix_EffectFunc_t f,
-
Mix_EffectDone_t d, void *arg)
-
Mix_EffectDone_t d, void *arg)
{
effect_info **e = NULL;
-
int retval;
if (channel == MIX_CHANNEL_POST) {
e = &posteffects;
@@ -1166,13 +1157,17 @@
e = &mix_channel[channel].effects;
}
- return(_Mix_register_effect(e, f, d, arg));
- SDL_LockAudio();
- retval = _Mix_register_effect(e, f, d, arg);
- SDL_UnlockAudio();
- return(retval);
}
int Mix_UnregisterEffect(int channel, Mix_EffectFunc_t f)
{
effect_info **e = NULL;
-
int retval;
if (channel == MIX_CHANNEL_POST) {
e = &posteffects;
@@ -1183,13 +1178,18 @@
}
e = &mix_channel[channel].effects;
}
- return(_Mix_remove_effect(channel, e, f));
- SDL_LockAudio();
- retval = _Mix_remove_effect(channel, e, f);
- SDL_UnlockAudio();
- return(retval);
}
int Mix_UnregisterAllEffects(int channel)
{
effect_info **e = NULL;
-
int retval;
if (channel == MIX_CHANNEL_POST) {
e = &posteffects;
@@ -1201,7 +1201,10 @@
e = &mix_channel[channel].effects;
}
- return(_Mix_remove_all_effects(channel, e));
- SDL_LockAudio();
- retval = _Mix_remove_all_effects(channel, e);
- SDL_UnlockAudio();
- return(retval);
}
/* end of mixer.c … */