Hi,
In a multi-threaded application it is possible for snd_pcm_delay or an equivalent function to be called by one thread while another is sitting in snd_pcm_writei. In this case, snd_pcm_delay does not take into account that there may not be enough space for all the data passed to snd_pcm_writei to be written to the ring buffer at once, and will return incorrect values.
To illustrate:
Suppose a sound card has a maximum buffer size of 0.1 second. Then, suppose an application writes 0.5 second of audio. snd_pcm_writei will immediately write the first 0.1 second to the ring buffer, then will incrementally write the remaining 0.4 second as the buffer drains. During this time, snd_pcm_delay will return a constant fill level of 0.1 second. On the application side, the playback time counter will be calculated during this time as 0.5 seconds written to ALSA minus a delay of 0.1 second, giving a constant stream position of 0.4 second.
(This problem showed up when a recent change in Audacious caused it to call snd_pcm_writei with larger chunks of audio than formerly -- the result was that the visualization, which is synchronized with the time counter, began to jump, where it should be a smooth 30 FPS animation.)
The problem is a little tough to fix, since there must be some thread semantics involved so that the thread calling snd_pcm_delay knows at what instant snd_pcm_delay will begin taking into account the data passed to snd_pcm_writei. My solution is to create a variant of snd_pcm_writei ("snd_pcm_writei_locked"), which is the passed the additional argument of a pointer to a mutex locked in advance by the caller. snd_pcm_writei_locked will then unlock the mutex once the application can safely assume that snd_pcm_delay will take into account the new data.
I realize this is a substantial change, so please tell me whether this patch is along the right track or if a different solution is called for.
Signed-off-by: John Lindgren john.lindgren@tds.net ---
include/pcm.h | 8 +++++ src/pcm/pcm.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++--- src/pcm/pcm_hw.c | 39 +++++++++++++++++++++++++- src/pcm/pcm_local.h | 3 ++ 4 files changed, 121 insertions(+), 5 deletions(-)
diff --git a/include/pcm.h b/include/pcm.h index f3618c3..2234279 100644 --- a/include/pcm.h +++ b/include/pcm.h @@ -33,6 +33,10 @@ extern "C" { #endif
+#include <pthread.h> + +#define SND_PCM_HAVE_LOCKED_WRITE + /** * \defgroup PCM PCM Interface * See the \ref pcm page for more details. @@ -448,8 +452,12 @@ snd_pcm_sframes_t snd_pcm_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames); snd_pcm_sframes_t snd_pcm_forwardable(snd_pcm_t *pcm); snd_pcm_sframes_t snd_pcm_forward(snd_pcm_t *pcm, snd_pcm_uframes_t frames); snd_pcm_sframes_t snd_pcm_writei(snd_pcm_t *pcm, const void *buffer, snd_pcm_uframes_t size); +snd_pcm_sframes_t snd_pcm_writei_locked (snd_pcm_t * pcm, const void * buffer, + snd_pcm_uframes_t size, pthread_mutex_t * mutex); snd_pcm_sframes_t snd_pcm_readi(snd_pcm_t *pcm, void *buffer, snd_pcm_uframes_t size); snd_pcm_sframes_t snd_pcm_writen(snd_pcm_t *pcm, void **bufs, snd_pcm_uframes_t size); +snd_pcm_sframes_t snd_pcm_writen_locked (snd_pcm_t * pcm, void * * bufs, + snd_pcm_uframes_t size, pthread_mutex_t * mutex); snd_pcm_sframes_t snd_pcm_readn(snd_pcm_t *pcm, void **bufs, snd_pcm_uframes_t size); int snd_pcm_wait(snd_pcm_t *pcm, int timeout);
diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c index f910189..db03067 100644 --- a/src/pcm/pcm.c +++ b/src/pcm/pcm.c @@ -903,8 +903,12 @@ int snd_pcm_sw_params(snd_pcm_t *pcm, snd_pcm_sw_params_t *params) */ int snd_pcm_status(snd_pcm_t *pcm, snd_pcm_status_t *status) { - assert(pcm && status); - return pcm->fast_ops->status(pcm->fast_op_arg, status); + int result; + + assert(pcm && status); + result = pcm->fast_ops->status (pcm->fast_op_arg, status); + status->delay += pcm->fast_op_arg->write_delay; + return result; }
/** @@ -973,12 +977,17 @@ link_warning(snd_pcm_hwsync, "Warning: snd_pcm_hwsync() is deprecated, consider */ int snd_pcm_delay(snd_pcm_t *pcm, snd_pcm_sframes_t *delayp) { + int result; + assert(pcm); if (CHECK_SANITY(! pcm->setup)) { SNDMSG("PCM not set up"); return -EIO; } - return pcm->fast_ops->delay(pcm->fast_op_arg, delayp); + + result = pcm->fast_ops->delay (pcm->fast_op_arg, delayp); + * delayp += pcm->fast_op_arg->write_delay; + return result; }
/** @@ -1248,6 +1257,34 @@ snd_pcm_sframes_t snd_pcm_writei(snd_pcm_t *pcm, const void *buffer, snd_pcm_ufr }
/** + * \brief Variant of snd_pcm_writei for multithreaded applications + * \param mutex Mutex already locked by caller + * + * In a multithreaded application, there is uncertainty inherent in calling + * snd_pcm_delay while another thread is calling and_pcm_writei, since there is + * no way to know whether the data passed to snd_pcm_writei has yet been + * accounted for in the delay calculation. This function provides a solution to + * that problem by unlocking the mutex when it is safe to call snd_pcm_delay. + * The mutex is locked again before the function returns. An application using + * this function should be careful to avoid deadlocks. + * + * Programmers should check that the macro SND_PCM_HAVE_LOCKED_WRITE is defined + * before using this function. + */ + +snd_pcm_sframes_t snd_pcm_writei_locked (snd_pcm_t * pcm, const void * buffer, + snd_pcm_uframes_t size, pthread_mutex_t * mutex) +{ + snd_pcm_sframes_t result; + + assert (pcm && pcm->fast_op_arg && ! pcm->fast_op_arg->write_mutex); + pcm->fast_op_arg->write_mutex = mutex; + result = snd_pcm_writei (pcm, buffer, size); + pcm->fast_op_arg->write_mutex = NULL; + return result; +} + +/** * \brief Write non interleaved frames to a PCM * \param pcm PCM handle * \param bufs frames containing buffers (one for each channel) @@ -1280,6 +1317,25 @@ snd_pcm_sframes_t snd_pcm_writen(snd_pcm_t *pcm, void **bufs, snd_pcm_uframes_t }
/** + * \brief Variant of snd_pcm_writen for multithreaded applications + * \param Mutex already locked by caller + * + * (See snd_pcm_writei_locked for a full description.) + */ + +snd_pcm_sframes_t snd_pcm_writen_locked (snd_pcm_t * pcm, void * * bufs, + snd_pcm_uframes_t size, pthread_mutex_t * mutex) +{ + snd_pcm_sframes_t result; + + assert (pcm && pcm->fast_op_arg && ! pcm->fast_op_arg->write_mutex); + pcm->fast_op_arg->write_mutex = mutex; + result = snd_pcm_writen (pcm, bufs, size); + pcm->fast_op_arg->write_mutex = NULL; + return result; +} + +/** * \brief Read interleaved frames from a PCM * \param pcm PCM handle * \param buffer frames containing buffer @@ -2480,6 +2536,7 @@ int snd_pcm_avail_delay(snd_pcm_t *pcm, sf = pcm->fast_ops->delay(pcm->fast_op_arg, delayp); if (sf < 0) return (int)sf; + * delayp += pcm->fast_op_arg->write_delay; sf = pcm->fast_ops->avail_update(pcm->fast_op_arg); if (sf < 0) return (int)sf; @@ -6652,7 +6709,18 @@ snd_pcm_sframes_t snd_pcm_write_areas(snd_pcm_t *pcm, const snd_pcm_channel_area goto _end; }
- err = snd_pcm_wait(pcm, -1); + if (pcm->write_mutex) { + pcm->write_delay = size; + assert (! pthread_mutex_unlock (pcm->write_mutex)); + } + + err = snd_pcm_wait(pcm, -1); + + if (pcm->write_mutex) { + assert (! pthread_mutex_lock (pcm->write_mutex)); + pcm->write_delay = 0; + } + if (err < 0) break; goto _again; diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c index 9d243d5..6a428d8 100644 --- a/src/pcm/pcm_hw.c +++ b/src/pcm/pcm_hw.c @@ -753,7 +753,8 @@ static int snd_pcm_hw_unlink(snd_pcm_t *pcm) return 0; }
-static snd_pcm_sframes_t snd_pcm_hw_writei(snd_pcm_t *pcm, const void *buffer, snd_pcm_uframes_t size) +static snd_pcm_sframes_t writei_real (snd_pcm_t * pcm, const void * buffer, + snd_pcm_uframes_t size) { int err; snd_pcm_hw_t *hw = pcm->private_data; @@ -772,6 +773,42 @@ static snd_pcm_sframes_t snd_pcm_hw_writei(snd_pcm_t *pcm, const void *buffer, s return xferi.result; }
+static snd_pcm_sframes_t snd_pcm_hw_writei (snd_pcm_t * pcm, const void * + buffer, snd_pcm_uframes_t size) +{ + snd_pcm_sframes_t remain = size; + snd_pcm_sframes_t result; + + while (1) { + if ((result = snd_pcm_hw_avail_update (pcm)) < 0) + return result; + if ((result = writei_real (pcm, buffer, result < remain ? result : + remain)) < 0) + return result; + + buffer = (const char *) buffer + snd_pcm_frames_to_bytes (pcm, result); + remain -= result; + + if (! remain) + return size; + + if (pcm->write_mutex) { + pcm->write_delay = remain; + assert (! pthread_mutex_unlock (pcm->write_mutex)); + } + + result = snd_pcm_wait (pcm, -1); + + if (pcm->write_mutex) { + assert (! pthread_mutex_lock (pcm->write_mutex)); + pcm->write_delay = 0; + } + + if (result < 0) + return result; + } +} + static snd_pcm_sframes_t snd_pcm_hw_writen(snd_pcm_t *pcm, void **bufs, snd_pcm_uframes_t size) { int err; diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h index 9aa81e1..53eef78 100644 --- a/src/pcm/pcm_local.h +++ b/src/pcm/pcm_local.h @@ -229,6 +229,9 @@ struct _snd_pcm { snd_pcm_t *fast_op_arg; void *private_data; struct list_head async_handlers; + snd_pcm_uframes_t write_delay; /* size of data passed to snd_pcm_write* + * but not yet outputted to ring buffer */ + pthread_mutex_t * write_mutex; /* mutex passed to snd_pcm_write*_locked */ };
/* make local functions really local */