[PATCH] PulseAudio plugin: report XRUN state back to application

[PATCH] PulseAudio plugin: report XRUN state back to application

[Lennart Poettering]

Hi!

Please consider merging this patch we ship in Fedora:

http://cvs.fedoraproject.org/viewcvs/*checkout*/devel/alsa-plugins/1.0.14-state-xrun.patch?rev=1.1

It adds support to report back XRUN to the application if one
happens. This is required to make some applications work on top of the
pulse plugin. One being XMMS, which checks if a song finished to play
by waiting for an XRUN (yes, I don't argue that XMMS shouldn't do
that, but nonetheless it is a good thing if XRUNs are reported
properly.)

Now, this patch is a bit problematic, since it touches pcm->io.state
which is not supposed to be touched by ioplug-based plugins --
according to the comments in ioplug. However, I saw no other way to
fix this, and from judging the ioplug code overwriting the variable
should be safe. Thus I decided to simply touch the variable but
documented it in a comment.

Alternatively, ioplug could be beefed up to properly handle XRUNs, but
I thought this fix is simpler.

Also reported on the ALSA BTS:

https://bugtrack.alsa-project.org/alsa-bug/view.php?id=3579

Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net         ICQ# 11060553
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: [PATCH] PulseAudio plugin: report XRUN state back to application

[Takashi Iwai]

At Sun, 25 Nov 2007 22:12:26 +0100,
Lennart Poettering wrote:
> 
> Hi!
> 
> Please consider merging this patch we ship in Fedora:
> 
> http://cvs.fedoraproject.org/viewcvs/*checkout*/devel/alsa-plugins/1.0.14-state-xrun.patch?rev=1.1
> 
> It adds support to report back XRUN to the application if one
> happens. This is required to make some applications work on top of the
> pulse plugin. One being XMMS, which checks if a song finished to play
> by waiting for an XRUN (yes, I don't argue that XMMS shouldn't do
> that, but nonetheless it is a good thing if XRUNs are reported
> properly.)
> 
> Now, this patch is a bit problematic, since it touches pcm->io.state
> which is not supposed to be touched by ioplug-based plugins --
> according to the comments in ioplug. However, I saw no other way to
> fix this, and from judging the ioplug code overwriting the variable
> should be safe. Thus I decided to simply touch the variable but
> documented it in a comment.
> 
> Alternatively, ioplug could be beefed up to properly handle XRUNs, but
> I thought this fix is simpler.

But it's a layer violation :)

We need a proper state-notification function.  It'd be simply setting
the state in the end, but at least, it's clear and the ioplug can do
any action or sanity-check if once needed.

A proposed patch is below.  Comments?


Takashi


diff -r 672c5387645d include/pcm_ioplug.h
--- a/include/pcm_ioplug.h	Fri Nov 23 15:46:48 2007 +0100
+++ b/include/pcm_ioplug.h	Mon Nov 26 11:32:10 2007 +0100
@@ -208,6 +208,9 @@ int snd_pcm_ioplug_set_param_minmax(snd_
 int snd_pcm_ioplug_set_param_minmax(snd_pcm_ioplug_t *io, int type, unsigned int min, unsigned int max);
 int snd_pcm_ioplug_set_param_list(snd_pcm_ioplug_t *io, int type, unsigned int num_list, const unsigned int *list);
 
+/* change PCM status */
+int snd_pcm_ioplug_set_state(snd_pcm_ioplug_t *ioplug, snd_pcm_state_t state);
+
 /** \} */
 
 #endif /* __ALSA_PCM_IOPLUG_H */
diff -r 672c5387645d src/pcm/pcm_ioplug.c
--- a/src/pcm/pcm_ioplug.c	Fri Nov 23 15:46:48 2007 +0100
+++ b/src/pcm/pcm_ioplug.c	Mon Nov 26 11:32:10 2007 +0100
@@ -614,6 +614,8 @@ static snd_pcm_sframes_t snd_pcm_ioplug_
 	snd_pcm_uframes_t avail;
 
 	snd_pcm_ioplug_hw_ptr_update(pcm);
+	if (io->data->state == SNDRV_PCM_STATE_XRUN)
+		return -EPIPE;
 	if (pcm->stream == SND_PCM_STREAM_CAPTURE &&
 	    pcm->access != SND_PCM_ACCESS_RW_INTERLEAVED &&
 	    pcm->access != SND_PCM_ACCESS_RW_NONINTERLEAVED) {
@@ -1037,3 +1039,19 @@ const snd_pcm_channel_area_t *snd_pcm_io
 		return snd_pcm_mmap_areas(ioplug->pcm);
 	return NULL;
 }
+
+/**
+ * \brief Change the ioplug PCM status
+ * \param ioplug the ioplug handle
+ * \param state the PCM status
+ * \return zero if successful or a negative error code
+ *
+ * Changes the PCM status of the ioplug to the given value.
+ * This function can be used for external plugins to notify the status
+ * change, e.g. XRUN.
+ */
+int snd_pcm_ioplug_set_state(snd_pcm_ioplug_t *ioplug, snd_pcm_state_t state)
+{
+	ioplug->state = state;
+	return 0;
+}

Re: [PATCH] PulseAudio plugin: report XRUN state back to application

[Lennart Poettering]

On Mon, 26.11.07 11:13, Takashi Iwai (tiwai@suse.de) wrote:

> > Alternatively, ioplug could be beefed up to properly handle XRUNs, but
> > I thought this fix is simpler.
> 
> But it's a layer violation :)
> 
> We need a proper state-notification function.  It'd be simply setting
> the state in the end, but at least, it's clear and the ioplug can do
> any action or sanity-check if once needed.
> 
> A proposed patch is below.  Comments?

Looks good to me. 

Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net         ICQ# 11060553
http://0pointer.net/lennart/           GnuPG 0x1A015CC4