SDL_mixer deadlock patch

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:

  1. 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.

  2. 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. :slight_smile:

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;
}
}

  •   SDL_mutexV(mixer_lock);
    
    } else {
  •   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 … */

Here’s that patch I promised.

Okay, the fixed version is in CVS.

Anybody who is experiencing the deadlock, please try out the new
version: http://www.libsdl.org/cvs/SDL_mixer-1.2.4.tar.gz

Note that this is PRERELEASE code. Please don’t distribute it.
If this fixes the deadlock problems, I’ll make a new release next week.

Thanks!
-Sam Lantinga, Software Engineer, Blizzard Entertainment

Thanks Ray and Sam, I got no problem.

Kentarou

Sam Lantinga writes:

Here’s that patch I promised.

Okay, the fixed version is in CVS.

Anybody who is experiencing the deadlock, please try out the new
version: http://www.libsdl.org/cvs/SDL_mixer-1.2.4.tar.gz

Note that this is PRERELEASE code. Please don’t distribute it.
If this fixes the deadlock problems, I’ll make a new release next week.

Latest SDL_mixer CVS seems to fix the problems I saw on the
Zaurus. Been running a test app for quite some time without problems.–
[ Below is a random fortune, which is unrelated to the above message. ]
millihelen, n.:
The amount of beauty required to launch one ship.