Bug 525401 - sa_stream_drain should return an error if called when stream not playing. r=chris.double
☠☠ backed out by 5988d1cd74ff ☠ ☠
authorMatthew Gregan <kinetik@flim.org>
Fri, 05 Feb 2010 14:31:18 +1300
changeset 38762 b876c36242dc3bfd4344caffae55a9590ee1b42d
parent 38761 9521f53168f5c9ad84629e0a01fa8e7ed0b6f17c
child 38763 9b33c91c851d1a722f8c1c64f210c7c593faf6ae
child 38769 5988d1cd74ff7054523dbb4a1983fa1af3a882c3
push idunknown
push userunknown
push dateunknown
reviewerschris.double
bugs525401
milestone1.9.3a2pre
Bug 525401 - sa_stream_drain should return an error if called when stream not playing. r=chris.double
media/libsydneyaudio/bug525401_drain_deadlock.patch
media/libsydneyaudio/src/sydney_audio_alsa.c
media/libsydneyaudio/src/sydney_audio_mac.c
media/libsydneyaudio/src/sydney_audio_waveapi.c
media/libsydneyaudio/update.sh
new file mode 100644
--- /dev/null
+++ b/media/libsydneyaudio/bug525401_drain_deadlock.patch
@@ -0,0 +1,235 @@
+diff --git a/media/libsydneyaudio/src/sydney_audio_alsa.c b/media/libsydneyaudio/src/sydney_audio_alsa.c
+--- a/media/libsydneyaudio/src/sydney_audio_alsa.c
++++ b/media/libsydneyaudio/src/sydney_audio_alsa.c
+@@ -315,16 +315,19 @@ sa_stream_resume(sa_stream_t *s) {
+ 
+ 
+ int
+ sa_stream_drain(sa_stream_t *s)
+ {
+   if (s == NULL || s->output_unit == NULL) {
+     return SA_ERROR_NO_INIT;
+   }
++  if (snd_pcm_state(s->output_unit) != SND_PCM_STATE_RUNNING) {
++    return SA_ERROR_INVALID;
++  }
+   snd_pcm_drain(s->output_unit);
+   return SA_SUCCESS;
+ }
+ 
+ 
+ 
+ /*
+  * -----------------------------------------------------------------------------
+diff --git a/media/libsydneyaudio/src/sydney_audio_mac.c b/media/libsydneyaudio/src/sydney_audio_mac.c
+--- a/media/libsydneyaudio/src/sydney_audio_mac.c
++++ b/media/libsydneyaudio/src/sydney_audio_mac.c
+@@ -396,20 +396,20 @@ sa_stream_write(sa_stream_t *s, const vo
+ 
+   /*
+    * Once we have our first block of audio data, enable the audio callback
+    * function. This doesn't need to be protected by the mutex, because
+    * s->playing is not used in the audio callback thread, and it's probably
+    * better not to be inside the lock when we enable the audio callback.
+    */
+   if (!s->playing) {
++    if (AudioOutputUnitStart(s->output_unit) != 0) {
++      return SA_ERROR_SYSTEM;
++    }
+     s->playing = TRUE;
+-    if (AudioOutputUnitStart(s->output_unit) != 0) {
+-      result = SA_ERROR_SYSTEM;
+-    }
+   }
+ 
+   return result;
+ }
+ 
+ 
+ static OSStatus
+ audio_callback(
+@@ -553,17 +553,20 @@ sa_stream_pause(sa_stream_t *s) {
+   }
+ 
+   /*
+    * Don't hold the mutex when stopping the audio device, because it is
+    * possible to deadlock with this thread holding mutex then waiting on an
+    * internal Core Audio lock, and with the callback thread holding the Core
+    * Audio lock and waiting on the mutex.
+   */
+-  AudioOutputUnitStop(s->output_unit);
++  if (AudioOutputUnitStop(s->output_unit) != 0) {
++    return SA_ERROR_SYSTEM;
++  }
++  s->playing = FALSE;
+ 
+   return SA_SUCCESS;
+ }
+ 
+ 
+ int
+ sa_stream_resume(sa_stream_t *s) {
+ 
+@@ -581,17 +584,20 @@ sa_stream_resume(sa_stream_t *s) {
+   pthread_mutex_unlock(&s->mutex);
+ 
+   /*
+    * Don't hold the mutex when starting the audio device, because it is
+    * possible to deadlock with this thread holding mutex then waiting on an
+    * internal Core Audio lock, and with the callback thread holding the Core
+    * Audio lock and waiting on the mutex.
+   */
+-  AudioOutputUnitStart(s->output_unit);
++  if (AudioOutputUnitStart(s->output_unit) != 0) {
++    return SA_ERROR_SYSTEM;
++  }
++  s->playing = TRUE;
+ 
+   return SA_SUCCESS;
+ }
+ 
+ 
+ static sa_buf *
+ new_buffer(void) {
+   sa_buf  * b = malloc(sizeof(sa_buf) + BUF_SIZE);
+@@ -607,16 +613,20 @@ new_buffer(void) {
+ 
+ int
+ sa_stream_drain(sa_stream_t *s)
+ {
+   if (s == NULL || s->output_unit == NULL) {
+     return SA_ERROR_NO_INIT;
+   }
+ 
++  if (!s->playing) {
++    return SA_ERROR_INVALID;
++  }
++
+   while (1) {
+     pthread_mutex_lock(&s->mutex);
+     sa_buf  * b;
+     size_t    used = 0;
+     for (b = s->bl_head; b != NULL; b = b->next) {
+       used += b->end - b->start;
+     }
+     pthread_mutex_unlock(&s->mutex);
+diff --git a/media/libsydneyaudio/src/sydney_audio_waveapi.c b/media/libsydneyaudio/src/sydney_audio_waveapi.c
+--- a/media/libsydneyaudio/src/sydney_audio_waveapi.c
++++ b/media/libsydneyaudio/src/sydney_audio_waveapi.c
+@@ -111,16 +111,18 @@ struct sa_stream {
+   sa_pcm_format_t	format;   
+  
+   HWAVEOUT			  hWaveOut;
+   HANDLE			    callbackEvent;
+   CRITICAL_SECTION  waveCriticalSection;  
+   WAVEHDR*			  waveBlocks;  
+   volatile int		waveFreeBlockCount;
+   int				      waveCurrentBlock;
++
++  int playing;
+ };
+ 
+ 
+ /** Forward definitions of audio api specific functions */
+ int allocateBlocks(int size, int count, WAVEHDR** blocks);
+ int freeBlocks(WAVEHDR* blocks);
+ int openAudio(sa_stream_t *s);
+ int closeAudio(sa_stream_t * s);
+@@ -157,16 +159,17 @@ int sa_stream_create_pcm(sa_stream_t **s
+   }
+    
+   _s->rwMode = mode;
+   _s->format = format;
+   _s->rate = rate;
+   _s->channels = nchannels;
+   _s->deviceName = DEFAULT_DEVICE_NAME;
+   _s->device = DEFAULT_DEVICE;
++  _s->playing = 0;
+ 
+   *s = _s; 
+   return SA_SUCCESS;
+ }
+ 
+ /** Initialise the device */
+ int sa_stream_open(sa_stream_t *s) {  
+   int status = SA_SUCCESS;
+@@ -300,33 +303,41 @@ int sa_stream_get_position(sa_stream_t *
+ int sa_stream_resume(sa_stream_t *s) {
+   int status;  
+   
+   ERROR_IF_NO_INIT(s);
+ 
+   status = waveOutRestart(s->hWaveOut);
+   HANDLE_WAVE_ERROR(status, "resuming audio playback");
+ 
++  s->playing = 1;
++
+   return SA_SUCCESS;
+ }
+ /** Pause audio playback (do not empty the buffer) */
+ int sa_stream_pause(sa_stream_t *s) {
+   int status;
+ 
+   ERROR_IF_NO_INIT(s);
+   
+   status = waveOutPause(s->hWaveOut);
+   HANDLE_WAVE_ERROR(status, "resuming audio playback");
+ 
++  s->playing = 0;
++
+   return SA_SUCCESS;
+ }
+ /** Block until all audio has been played */
+ int sa_stream_drain(sa_stream_t *s) {
+   ERROR_IF_NO_INIT(s);
+   
++  if (!s->playing) {
++    return SA_ERROR_INVALID;
++  }
++
+   /* wait for all blocks to complete */
+   EnterCriticalSection(&(s->waveCriticalSection));
+   while(s->waveFreeBlockCount < BLOCK_COUNT) {
+     LeaveCriticalSection(&(s->waveCriticalSection));
+     Sleep(10);
+     EnterCriticalSection(&(s->waveCriticalSection));
+   }
+   LeaveCriticalSection(&(s->waveCriticalSection));
+@@ -484,16 +495,18 @@ int closeAudio(sa_stream_t * s) {
+     s->waveBlocks = NULL;
+   }
+ 
+   status = waveOutClose(s->hWaveOut);    
+   if (status != MMSYSERR_NOERROR) {
+     result = getSAErrorCode(status);
+   }
+ 
++  s->playing = 0;
++
+   DeleteCriticalSection(&(s->waveCriticalSection));
+   CloseHandle(s->callbackEvent);
+   
+   return result;
+ }
+ /**
+  * \brief - writes PCM audio samples to audio device
+  * \param s - valid handle to opened sydney stream
+@@ -545,16 +558,18 @@ int writeAudio(sa_stream_t *s, LPSTR dat
+     /*
+      * point to the next block
+      */
+     (s->waveCurrentBlock)++;
+     (s->waveCurrentBlock) %= BLOCK_COUNT;		
+ 
+     current = &(s->waveBlocks[s->waveCurrentBlock]);
+     current->dwUser = 0;
++
++    s->playing = 1;
+   }
+   return SA_SUCCESS;
+ }
+ 
+ /**
+  * \brief - audio callback function called when next WAVE header is played by audio device
+  */
+ void CALLBACK waveOutProc(
--- a/media/libsydneyaudio/src/sydney_audio_alsa.c
+++ b/media/libsydneyaudio/src/sydney_audio_alsa.c
@@ -315,16 +315,19 @@ sa_stream_resume(sa_stream_t *s) {
 
 
 int
 sa_stream_drain(sa_stream_t *s)
 {
   if (s == NULL || s->output_unit == NULL) {
     return SA_ERROR_NO_INIT;
   }
+  if (snd_pcm_state(s->output_unit) != SND_PCM_STATE_RUNNING) {
+    return SA_ERROR_INVALID;
+  }
   snd_pcm_drain(s->output_unit);
   return SA_SUCCESS;
 }
 
 
 
 /*
  * -----------------------------------------------------------------------------
--- a/media/libsydneyaudio/src/sydney_audio_mac.c
+++ b/media/libsydneyaudio/src/sydney_audio_mac.c
@@ -396,20 +396,20 @@ sa_stream_write(sa_stream_t *s, const vo
 
   /*
    * Once we have our first block of audio data, enable the audio callback
    * function. This doesn't need to be protected by the mutex, because
    * s->playing is not used in the audio callback thread, and it's probably
    * better not to be inside the lock when we enable the audio callback.
    */
   if (!s->playing) {
+    if (AudioOutputUnitStart(s->output_unit) != 0) {
+      return SA_ERROR_SYSTEM;
+    }
     s->playing = TRUE;
-    if (AudioOutputUnitStart(s->output_unit) != 0) {
-      result = SA_ERROR_SYSTEM;
-    }
   }
 
   return result;
 }
 
 
 static OSStatus
 audio_callback(
@@ -553,17 +553,20 @@ sa_stream_pause(sa_stream_t *s) {
   }
 
   /*
    * Don't hold the mutex when stopping the audio device, because it is
    * possible to deadlock with this thread holding mutex then waiting on an
    * internal Core Audio lock, and with the callback thread holding the Core
    * Audio lock and waiting on the mutex.
   */
-  AudioOutputUnitStop(s->output_unit);
+  if (AudioOutputUnitStop(s->output_unit) != 0) {
+    return SA_ERROR_SYSTEM;
+  }
+  s->playing = FALSE;
 
   return SA_SUCCESS;
 }
 
 
 int
 sa_stream_resume(sa_stream_t *s) {
 
@@ -581,17 +584,20 @@ sa_stream_resume(sa_stream_t *s) {
   pthread_mutex_unlock(&s->mutex);
 
   /*
    * Don't hold the mutex when starting the audio device, because it is
    * possible to deadlock with this thread holding mutex then waiting on an
    * internal Core Audio lock, and with the callback thread holding the Core
    * Audio lock and waiting on the mutex.
   */
-  AudioOutputUnitStart(s->output_unit);
+  if (AudioOutputUnitStart(s->output_unit) != 0) {
+    return SA_ERROR_SYSTEM;
+  }
+  s->playing = TRUE;
 
   return SA_SUCCESS;
 }
 
 
 static sa_buf *
 new_buffer(void) {
   sa_buf  * b = malloc(sizeof(sa_buf) + BUF_SIZE);
@@ -607,16 +613,20 @@ new_buffer(void) {
 
 int
 sa_stream_drain(sa_stream_t *s)
 {
   if (s == NULL || s->output_unit == NULL) {
     return SA_ERROR_NO_INIT;
   }
 
+  if (!s->playing) {
+    return SA_ERROR_INVALID;
+  }
+
   while (1) {
     pthread_mutex_lock(&s->mutex);
     sa_buf  * b;
     size_t    used = 0;
     for (b = s->bl_head; b != NULL; b = b->next) {
       used += b->end - b->start;
     }
     pthread_mutex_unlock(&s->mutex);
--- a/media/libsydneyaudio/src/sydney_audio_waveapi.c
+++ b/media/libsydneyaudio/src/sydney_audio_waveapi.c
@@ -111,16 +111,18 @@ struct sa_stream {
   sa_pcm_format_t	format;   
  
   HWAVEOUT			  hWaveOut;
   HANDLE			    callbackEvent;
   CRITICAL_SECTION  waveCriticalSection;  
   WAVEHDR*			  waveBlocks;  
   volatile int		waveFreeBlockCount;
   int				      waveCurrentBlock;
+
+  int playing;
 };
 
 
 /** Forward definitions of audio api specific functions */
 int allocateBlocks(int size, int count, WAVEHDR** blocks);
 int freeBlocks(WAVEHDR* blocks);
 int openAudio(sa_stream_t *s);
 int closeAudio(sa_stream_t * s);
@@ -157,16 +159,17 @@ int sa_stream_create_pcm(sa_stream_t **s
   }
    
   _s->rwMode = mode;
   _s->format = format;
   _s->rate = rate;
   _s->channels = nchannels;
   _s->deviceName = DEFAULT_DEVICE_NAME;
   _s->device = DEFAULT_DEVICE;
+  _s->playing = 0;
 
   *s = _s; 
   return SA_SUCCESS;
 }
 
 /** Initialise the device */
 int sa_stream_open(sa_stream_t *s) {  
   int status = SA_SUCCESS;
@@ -300,33 +303,41 @@ int sa_stream_get_position(sa_stream_t *
 int sa_stream_resume(sa_stream_t *s) {
   int status;  
   
   ERROR_IF_NO_INIT(s);
 
   status = waveOutRestart(s->hWaveOut);
   HANDLE_WAVE_ERROR(status, "resuming audio playback");
 
+  s->playing = 1;
+
   return SA_SUCCESS;
 }
 /** Pause audio playback (do not empty the buffer) */
 int sa_stream_pause(sa_stream_t *s) {
   int status;
 
   ERROR_IF_NO_INIT(s);
   
   status = waveOutPause(s->hWaveOut);
   HANDLE_WAVE_ERROR(status, "resuming audio playback");
 
+  s->playing = 0;
+
   return SA_SUCCESS;
 }
 /** Block until all audio has been played */
 int sa_stream_drain(sa_stream_t *s) {
   ERROR_IF_NO_INIT(s);
   
+  if (!s->playing) {
+    return SA_ERROR_INVALID;
+  }
+
   /* wait for all blocks to complete */
   EnterCriticalSection(&(s->waveCriticalSection));
   while(s->waveFreeBlockCount < BLOCK_COUNT) {
     LeaveCriticalSection(&(s->waveCriticalSection));
     Sleep(10);
     EnterCriticalSection(&(s->waveCriticalSection));
   }
   LeaveCriticalSection(&(s->waveCriticalSection));
@@ -484,16 +495,18 @@ int closeAudio(sa_stream_t * s) {
     s->waveBlocks = NULL;
   }
 
   status = waveOutClose(s->hWaveOut);    
   if (status != MMSYSERR_NOERROR) {
     result = getSAErrorCode(status);
   }
 
+  s->playing = 0;
+
   DeleteCriticalSection(&(s->waveCriticalSection));
   CloseHandle(s->callbackEvent);
   
   return result;
 }
 /**
  * \brief - writes PCM audio samples to audio device
  * \param s - valid handle to opened sydney stream
@@ -545,16 +558,18 @@ int writeAudio(sa_stream_t *s, LPSTR dat
     /*
      * point to the next block
      */
     (s->waveCurrentBlock)++;
     (s->waveCurrentBlock) %= BLOCK_COUNT;		
 
     current = &(s->waveBlocks[s->waveCurrentBlock]);
     current->dwUser = 0;
+
+    s->playing = 1;
   }
   return SA_SUCCESS;
 }
 
 /**
  * \brief - audio callback function called when next WAVE header is played by audio device
  */
 void CALLBACK waveOutProc(
--- a/media/libsydneyaudio/update.sh
+++ b/media/libsydneyaudio/update.sh
@@ -5,10 +5,11 @@
 cp $1/include/sydney_audio.h include/sydney_audio.h
 cp $1/src/*.c src/
 cp $1/AUTHORS ./AUTHORS
 patch -p4 <pause-resume.patch
 patch -p4 <include-CoreServices.patch
 patch -p4 <sydney_os2_base.patch
 patch -p4 <sydney_os2_moz.patch
 patch -p3 <bug495794_closeAudio.patch
-patch -p3 < bug495558_alsa_endian.patch
+patch -p3 <bug495558_alsa_endian.patch
+patch -p3 <bug525401_drain_deadlock.patch
 patch -p3 <bug526411_latency.patch