alsa-lib: snd_pcm_delay and friends do not account for a write being currently in progress

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.

---

 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(-)

index f3618c3..2234279 100644
@@ -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);
 

index 9d243d5..6a428d8 100644
@@ -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;

index 9aa81e1..53eef78 100644
@@ -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 */

Alsa-lib is not thread safe. In theory, you are not even allowed to call snd_pcm_delay while another function on the same PCM device has not yet returned.

It will write a chunk whenever it is woken up by the driver, which is usually at period boundaries.

Depending on how big the granularity of the hardware's readable DMA pointer is, but on most hardware, it will fluctuate between a full buffer and one period less than that, not counting further fluctuations due to scheduling delays.

This is wrong; as long as the write call has not returned, you do not know how much has been written (and when an error occurs, writing can stop before).

To keep track of the actual amount of data written, use non-blocking mode and in a loop, write as much as possible in one call, then update your write counter, then wait for some more free space in the buffer with poll().

Regards,
Clemens

Thanks for your reply.

ALSA has the following significant features:
...
SMP and thread-safe design.

So, that's a big lie?

Would it work to simply call snd_pcm_wait?

John Lindgren

That applies to the kernel code.

Most functions in alsa-lib must not be called at the same time on the same device handle. (Don't ask me where this is documented.)

Yes. (I usually suggest poll because the code that writes audio data often wants to be informed of some other event. If your writing loop doesn't need to be interrupted, snd_pcm_wait works just fine.)

Regards,
Clemens

Do you have a problem with patches that improve the current situation?

It is permissible, then, to call snd_pcm_delay during a snd_pcm_wait call?

What would be the cleanest way to interrupt snd_pcm_wait when we need to stop the stream? Will snd_pcm_drop work?

John Lindgren

These values are not wrong; the problem is that your program does not have enough information to interpret it correctly.

A blocking write call is an abstraction on top of the underlying non-blocking writes. Using a blocking write implies that your program wants to write the entire block and does not care about how the timing of the chunks that are actually written.

I do not consider it an improvement if a patch adds complexity, if the same functionality is already available. Non-blocking mode was designed for the case where you want to know the actual amount of data at every point in time.

It will work in practice (snd_pcm_wait is just a wrapper around poll).

*sigh*
This will _not_ work.

You have to use poll so that you can send your loop a message (through a pipe/eventfd/whatever) that tells it to stop.

Regards,
Clemens

Same thing.

Ugh. It seems you are determined to avoid complexity in ALSA at the expense of complexity in programs that use it.

At any rate, at least I know what needs to be done now, so thanks for that.

John Lindgren

I believe the definition of snd_pcm_delay() is it returns a value that would be fairly accurate at this instance of time. If you followed the snd_pcm_delay() with a snd_pcm_writei(), the samples would reach the speaker "delay" time later.

I think it would be fair to say that the value of snd_pcm_delay() is undefined if called during a snd_pcm_writei() call, because you will get a return value from snd_pcm_delay() but you will have no idea how many samples of the current snd_pcm_writei() have been written and thus no idea what the delay will be on the next snd_pcm_writei().

Even in multi-threaded applications calling two functions at the same time that interfere with each other it not good.
If a function is re-entrant it is generally considered to be thread safe. It does not necessarily mean that it is sensible to call two different functions at the same time.
_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

I understand that snd_pcm_delay and snd_pcm_writei currently "interfere with each other" in that snd_pcm_delay returns wrong values if called during snd_pcm_writei. That is the problem my patch tries to correct. Do you disagree with this improvement? If so, why?

John Lindgren

I disagree because I believe the changes are unnecessary.
The alsa api is big enough already, so adding anything extra must have an extremely good reason for it.
snd_pcm_writei and snd_pcm_delay were always intended to be used in the same thread.
Calling snd_pcm_writei and snd_pcm_delay at the same time is non-deterministic so fairly pointless to do.
Adding more locks just makes another hit on performances so should be avoided. Locks really kill performance on a multi processor SMP machine.

I would argue that modifying your program to fit in the the above contraints is the way to go.
_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

I thought there was good enough reason, but I won't argue the point.

All you're saying is that because it currently doesn't work, it's pointless to make it work.

The locks have to be there somewhere; it's just a question of whether that will be on the application side or the ALSA side. We have an interface running in one thread, and an audio decoder running in another. As long as output time is calculated from written time + delay, there must be some thread interaction to ensure that those values are in sync.

That will be ugly, but it seems that is what I will have to do.

John Lindgren

Hi. I've been getting "pcm_hw.c: snd_pcm_hw_delay() SNDRV_PCM_IOCTL_DELAY failed." in my xine log and after some talks with one of the devs, he suggested that alsa was not getting data fast enough in some cases (iirc). Could you please email me a copy of your patch (so I don't have to hand-patch) to try? Hopefully it will fix this damn problem once and for all.

Thanks,
Derek

I don't think my patch has anything to do with that problem, but I suppose you can try it.

John Lindgren

index f3618c3..2234279 100644
@@ -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);
 

index 9d243d5..6a428d8 100644
@@ -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;

index 9aa81e1..53eef78 100644
@@ -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 output to ring buffer */
+	pthread_mutex_t * write_mutex;  /* mutex passed to snd_pcm_write*_locked */
 };
 
 /* make local functions really local */

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

The actual error (code) is missing.

That would be an underrun. This has nothing to do with the interpretation of snd_pcm_delay's return value; this function just appears in the log because it happend to be the first one that detected this error condition. An underrung cannot be prevented by any change in alsa-lib; xine needs to use a larger buffer, or the scheduling needs to be improved so that some other thread or program doesn't prevent xine from being executed.

Regards,
Clemens