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

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

[John Lindgren]

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 */

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

[Clemens Ladisch]

John Lindgren wrote:
> 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.

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.

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

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

> During this time, snd_pcm_delay will return a constant fill level of 0.1
> second.

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.

> On the application side, the playback time counter will be calculated
> during this time as 0.5 seconds written to ALSA

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

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

[John Lindgren]

Thanks for your reply.

On Thu, 2010-06-03 at 08:40 +0200, Clemens Ladisch wrote:
> John Lindgren wrote:
> > 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.
> 
> 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.

>From http://alsa-project.org/main/index.php/Main_Page:

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

So, that's a big lie?

> ...
> > On the application side, the playback time counter will be calculated
> > during this time as 0.5 seconds written to ALSA
> 
> 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().

Would it work to simply call snd_pcm_wait?

John Lindgren

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

[Clemens Ladisch]

John Lindgren wrote:
> On Thu, 2010-06-03 at 08:40 +0200, Clemens Ladisch wrote:
> > Alsa-lib is not thread safe.
> 
> From http://alsa-project.org/main/index.php/Main_Page:
> 
>         ALSA has the following significant features:
>         ...
>         SMP and thread-safe design.
> 
> So, that's a big lie?

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

> > ... poll()
> 
> Would it work to simply call snd_pcm_wait?

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

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

[John Lindgren]

On Thu, 2010-06-03 at 16:48 +0200, Clemens Ladisch wrote:
> 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.)

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

> > Would it work to simply call snd_pcm_wait?
> 
> 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.)

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

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

[Clemens Ladisch]

John Lindgren wrote:
> 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.

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

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

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.

> > > Would it work to simply call snd_pcm_wait?
> > 
> > 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.)
> 
> It is permissible, then, to call snd_pcm_delay during a snd_pcm_wait
> call?

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

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

*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

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

[John Lindgren]

On Thu, 2010-06-03 at 19:03 +0200, Clemens Ladisch wrote:
> John Lindgren wrote:
> > 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.
> 
> These values are not wrong; the problem is that your program does not
> have enough information to interpret it correctly.

Same thing.

> > What would be the cleanest way to interrupt snd_pcm_wait when we need to
> > stop the stream?  Will snd_pcm_drop work?
> 
> *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.

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

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

[James Courtier-Dutton]

On 2 June 2010 22:29, John Lindgren <john.lindgren@tds.net> wrote:
> 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.
>

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
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[John Lindgren]

On Thu, 2010-06-03 at 15:40 +0100, James Courtier-Dutton wrote:
> On 2 June 2010 22:29, John Lindgren <john.lindgren@tds.net> wrote:
> > 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.
> 
> 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.

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

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

[James Courtier-Dutton]

On 3 June 2010 17:10, John Lindgren <john.lindgren@tds.net> wrote:
>
> 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?
>

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
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[John Lindgren]

On Thu, 2010-06-03 at 17:34 +0100, James Courtier-Dutton wrote:
> On 3 June 2010 17:10, John Lindgren <john.lindgren@tds.net> wrote:
> >
> > 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?
> 
> 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.

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

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

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

> Adding more locks just makes another hit on performances so should be avoided.
> Locks really kill performance on a multi processor SMP machine.

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.

> I would argue that modifying your program to fit in the the above
> contraints is the way to go.

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

John Lindgren

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

[VDR User]

On Wed, Jun 2, 2010 at 2:29 PM, John Lindgren <john.lindgren@tds.net> wrote:
> 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.

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

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

[John Lindgren]

[-- Attachment #1: Type: text/plain, Size: 1014 bytes --]

On Thu, 2010-06-03 at 10:40 -0700, VDR User wrote:
> On Wed, Jun 2, 2010 at 2:29 PM, John Lindgren <john.lindgren@tds.net> wrote:
> > 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.
> 
> 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.

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

John Lindgren

[-- Attachment #2: alsa-timing.diff --]
[-- Type: text/x-patch, Size: 7251 bytes --]

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 output to ring buffer */
+	pthread_mutex_t * write_mutex;  /* mutex passed to snd_pcm_write*_locked */
 };
 
 /* make local functions really local */

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Clemens Ladisch]

VDR User wrote:
> I've been getting "pcm_hw.c: snd_pcm_hw_delay() SNDRV_PCM_IOCTL_DELAY
> failed." in my xine log

The actual error (code) is missing.

> and after some talks with one of the devs, he suggested that alsa was
> not getting data fast enough in some cases (iirc).

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