[PATCH] Two patches for Alsa-plugins (pulse)

[PATCH] Two patches for Alsa-plugins (pulse)

[David Henningsson]

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

These two patches are being used in Ubuntu Lucid (released this April)
and are working well enough to have people asking us to backport them to
Karmic. Today I got an email, asking for the upstream status of one of
these patches, since he wanted them in Fedora. If posting them here
isn't the correct way to upstream them, please tell me so.

The first one (Fix invalid buffer pointer return value) fixes broken logic:

This patch improves recovering from underruns, and prevents hangs inside
snd_pcm_write* and snd_pcm_read* due to snd_pcm_avail* returning too
low values. It especially helps low latency situations.

The second one (Do not report underruns to the ALSA layer) is more a
change of behavior, which could be questioned. The arguments for
removing that code are these:

 * If pulseaudio gets an underrun, the normal way to end that underrun
is to feed it with more buffers. This is different from the ALSA way of
dealing with underruns, which requires hardware buffer pointers to be reset.
 * In addition, underrun signals are delivered asynchronously from
pulseaudio. This means that there might be more buffers on the way to
pulseaudio when the underrun is reported, making the underrun obsolete.
Unfortunately, there is currently no known way to determine whether this
is the case or not.

// David

[-- Attachment #2: lp464008lp503174.patch --]
[-- Type: text/x-patch, Size: 3241 bytes --]

Description: Do not report underruns to the ALSA layer.
 Reporting underruns to ALSA seems to do more bad than good, for these reasons:
 * If pulseaudio gets an underrun, the normal way to end that underrun is to 
   feed it with more buffers. This is different from the ALSA way of dealing
   with underruns, which requires hardware buffer pointers to be reset.
 * In addition, underrun signals are delivered asynchronously from pulseaudio.
   This means that there might be more buffers on the way to pulseaudio when
   the underrun is reported, making the underrun obsolete. Unfortunately, 
   there is currently no known way to determine whether this is the case or 
   not.
Forwarded: yes
Bug-ubuntu: https://bugs.launchpad.net/bugs/464008
Author: David Henningsson <diwic@ubuntu.com>

Index: alsa-plugins-1.0.22/pulse/pcm_pulse.c
===================================================================
--- alsa-plugins-1.0.22.orig/pulse/pcm_pulse.c	2010-02-28 17:11:29.000000000 -0500
+++ alsa-plugins-1.0.22/pulse/pcm_pulse.c	2010-02-28 17:11:56.000000000 -0500
@@ -38,7 +38,7 @@
 	/* Since ALSA expects a ring buffer we must do some voodoo. */
 	size_t last_size;
 	size_t ptr;
-	int underrun;
+	/* int underrun; */
 
 	size_t offset;
 
@@ -220,7 +220,7 @@
 
 	u = pa_stream_trigger(pcm->stream, stream_success_cb, pcm);
 
-	pcm->underrun = 0;
+	/* pcm->underrun = 0; */
 	err_o = pulse_wait_operation(pcm->p, o);
 	if (u)
 		err_u = pulse_wait_operation(pcm->p, u);
@@ -347,10 +347,10 @@
 	if (ret < 0)
 		goto finish;
 
-	if (pcm->underrun) {
+	/*if (pcm->underrun) {
 		ret = -EPIPE;
 		goto finish;
-	}
+	}*/
 
 	ret = update_ptr(pcm);
 	if (ret < 0) {
@@ -358,9 +358,9 @@
 		goto finish;
 	}
 
-	if (pcm->underrun)
+	/*if (pcm->underrun)
 		ret = -EPIPE;
-	else
+	else*/
 		ret = snd_pcm_bytes_to_frames(io->pcm, pcm->ptr);
 
 finish:
@@ -408,8 +408,8 @@
 
 finish:
 
-	if (pcm->underrun && pcm->io.state == SND_PCM_STATE_RUNNING)
-		snd_pcm_ioplug_set_state(io, SND_PCM_STATE_XRUN);
+	/*if (pcm->underrun && pcm->io.state == SND_PCM_STATE_RUNNING)
+		snd_pcm_ioplug_set_state(io, SND_PCM_STATE_XRUN);*/
 
 	pa_threaded_mainloop_unlock(pcm->p->mainloop);
 
@@ -462,7 +462,7 @@
 		goto finish;
 
 	ret = size;
-	pcm->underrun = 0;
+	/*pcm->underrun = 0;*/
 
 finish:
 	pa_threaded_mainloop_unlock(pcm->p->mainloop);
@@ -575,7 +575,7 @@
 
 	update_active(pcm);
 }
-
+/*
 static void stream_underrun_cb(pa_stream * p, void *userdata)
 {
 	snd_pcm_pulse_t *pcm = userdata;
@@ -587,7 +587,7 @@
 
 	pcm->underrun = 1;
 }
-
+*/
 static void stream_latency_cb(pa_stream *p, void *userdata) {
 	snd_pcm_pulse_t *pcm = userdata;
 
@@ -688,8 +688,8 @@
 	if (io->stream == SND_PCM_STREAM_PLAYBACK) {
 		pa_stream_set_write_callback(pcm->stream,
 					     stream_request_cb, pcm);
-		pa_stream_set_underflow_callback(pcm->stream,
-						 stream_underrun_cb, pcm);
+		/*pa_stream_set_underflow_callback(pcm->stream,
+						 stream_underrun_cb, pcm);*/
 		r = pa_stream_connect_playback(pcm->stream, pcm->device,
 					       &pcm->buffer_attr,
 					       PA_STREAM_AUTO_TIMING_UPDATE |
@@ -728,7 +728,7 @@
 	}
 
 	pcm->offset = 0;
-	pcm->underrun = 0;
+	/*pcm->underrun = 0;*/
 
       finish:
 	pa_threaded_mainloop_unlock(pcm->p->mainloop);

[-- Attachment #3: lp485488.patch --]
[-- Type: text/x-patch, Size: 2220 bytes --]

>From 2e9f8fa4d6f7a0cabe5aea1cbfa98980cdfb6d84 Mon Sep 17 00:00:00 2001
From: David Henningsson <diwic@ubuntu.com>
Date: Sat, 9 Jan 2010 09:09:14 +0100
Subject: [PATCH] pulse: Fix invalid buffer pointer return value

This patch improves recovering from underruns, and prevents hangs inside
snd_pcm_write* and snd_pcm_read* due to snd_pcm_avail* returning too
low values. It especially helps low latency situations.

Signed-off-by: David Henningsson <diwic@ubuntu.com>
---
 pulse/pcm_pulse.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

Index: alsa-plugins-1.0.22/pulse/pcm_pulse.c
===================================================================
--- alsa-plugins-1.0.22.orig/pulse/pcm_pulse.c	2010-02-28 17:12:02.000000000 -0500
+++ alsa-plugins-1.0.22/pulse/pcm_pulse.c	2010-02-28 17:12:25.000000000 -0500
@@ -90,6 +90,10 @@
 	if (pcm->io.stream == SND_PCM_STREAM_CAPTURE)
 		size -= pcm->offset;
 
+	/* Prevent accidental overrun of the fake ringbuffer */
+	if (size >= pcm->buffer_attr.tlength)
+		size = pcm->buffer_attr.tlength-1;
+
 	if (size > pcm->last_size) {
 		pcm->ptr += size - pcm->last_size;
 		pcm->ptr %= pcm->buffer_attr.tlength;
@@ -424,6 +428,7 @@
 	snd_pcm_pulse_t *pcm = io->private_data;
 	const char *buf;
 	snd_pcm_sframes_t ret = 0;
+	size_t writebytes;
 
 	assert(pcm);
 
@@ -445,13 +450,15 @@
 	    (char *) areas->addr + (areas->first +
 				    areas->step * offset) / 8;
 
-	ret = pa_stream_write(pcm->stream, buf, size * pcm->frame_size, NULL, 0, 0);
+	writebytes = size * pcm->frame_size;
+	ret = pa_stream_write(pcm->stream, buf, writebytes, NULL, 0, 0);
 	if (ret < 0) {
 		ret = -EIO;
 		goto finish;
 	}
 
 	/* Make sure the buffer pointer is in sync */
+	pcm->last_size -= writebytes;
 	ret = update_ptr(pcm);
 	if (ret < 0)
 		goto finish;
@@ -528,6 +535,7 @@
 
 		dst_buf = (char *) dst_buf + frag_length;
 		remain_size -= frag_length;
+		pcm->last_size -= frag_length;
 	}
 
 	/* Make sure the buffer pointer is in sync */
@@ -730,6 +738,11 @@
 	pcm->offset = 0;
 	/*pcm->underrun = 0;*/
 
+	/* Reset fake ringbuffer */
+	pcm->last_size = 0;
+	pcm->ptr = 0;
+	update_ptr(pcm);
+
       finish:
 	pa_threaded_mainloop_unlock(pcm->p->mainloop);
 

[-- Attachment #4: 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] Two patches for Alsa-plugins (pulse)

[Raymond Yau]

2010/6/24 David Henningsson <launchpad.web@epost.diwic.se>

> These two patches are being used in Ubuntu Lucid (released this April)
> and are working well enough to have people asking us to backport them to
> Karmic. Today I got an email, asking for the upstream status of one of
> these patches, since he wanted them in Fedora. If posting them here
> isn't the correct way to upstream them, please tell me so.
>
> The first one (Fix invalid buffer pointer return value) fixes broken logic:
>
> This patch improves recovering from underruns, and prevents hangs inside
> snd_pcm_write* and snd_pcm_read* due to snd_pcm_avail* returning too
> low values. It especially helps low latency situations.
>
> The second one (Do not report underruns to the ALSA layer) is more a
> change of behavior, which could be questioned. The arguments for
> removing that code are these:
>
>  * If pulseaudio gets an underrun, the normal way to end that underrun
> is to feed it with more buffers. This is different from the ALSA way of
> dealing with underruns, which requires hardware buffer pointers to be
> reset.
>  * In addition, underrun signals are delivered asynchronously from
> pulseaudio. This means that there might be more buffers on the way to
> pulseaudio when the underrun is reported, making the underrun obsolete.
> Unfortunately, there is currently no known way to determine whether this
> is the case or not.
>
> // David
>
>
>
Are there any test program which can reproduce this condition ?

any program similar to alsa-time-test.c which PA developer use to test the
alsa-driver but alsa-time-test.c aborted when I specify to use the pulse
device

Re: [PATCH] Two patches for Alsa-plugins (pulse)

[Takashi Iwai]

At Wed, 23 Jun 2010 21:57:01 +0200,
David Henningsson wrote:
> 
> These two patches are being used in Ubuntu Lucid (released this April)
> and are working well enough to have people asking us to backport them to
> Karmic. Today I got an email, asking for the upstream status of one of
> these patches, since he wanted them in Fedora. If posting them here
> isn't the correct way to upstream them, please tell me so.
> 
> The first one (Fix invalid buffer pointer return value) fixes broken logic:
> 
> This patch improves recovering from underruns, and prevents hangs inside
> snd_pcm_write* and snd_pcm_read* due to snd_pcm_avail* returning too
> low values. It especially helps low latency situations.

This one looks OK.  Applied now.


> The second one (Do not report underruns to the ALSA layer) is more a
> change of behavior, which could be questioned. The arguments for
> removing that code are these:
> 
>  * If pulseaudio gets an underrun, the normal way to end that underrun
> is to feed it with more buffers. This is different from the ALSA way of
> dealing with underruns, which requires hardware buffer pointers to be reset.
>  * In addition, underrun signals are delivered asynchronously from
> pulseaudio. This means that there might be more buffers on the way to
> pulseaudio when the underrun is reported, making the underrun obsolete.
> Unfortunately, there is currently no known way to determine whether this
> is the case or not.

I think this helps for normal use-cases, so it'd be good to apply.
But, I prefer having a runtime option for such a behavior change
(although the default value can be the new behavior).

Care to add it?


thanks,

Takashi

Re: [PATCH] Two patches for Alsa-plugins (pulse)

[David Henningsson]

On 2010-06-24 08:42, Takashi Iwai wrote:
> At Wed, 23 Jun 2010 21:57:01 +0200,
> David Henningsson wrote:
>>
>> These two patches are being used in Ubuntu Lucid (released this April)
>> and are working well enough to have people asking us to backport them to
>> Karmic. Today I got an email, asking for the upstream status of one of
>> these patches, since he wanted them in Fedora. If posting them here
>> isn't the correct way to upstream them, please tell me so.
>>
>> The first one (Fix invalid buffer pointer return value) fixes broken logic:
>>
>> This patch improves recovering from underruns, and prevents hangs inside
>> snd_pcm_write* and snd_pcm_read* due to snd_pcm_avail* returning too
>> low values. It especially helps low latency situations.
> 
> This one looks OK.  Applied now.

Thanks!

>> The second one (Do not report underruns to the ALSA layer) is more a
>> change of behavior, which could be questioned. The arguments for
>> removing that code are these:
>>
>>  * If pulseaudio gets an underrun, the normal way to end that underrun
>> is to feed it with more buffers. This is different from the ALSA way of
>> dealing with underruns, which requires hardware buffer pointers to be reset.
>>  * In addition, underrun signals are delivered asynchronously from
>> pulseaudio. This means that there might be more buffers on the way to
>> pulseaudio when the underrun is reported, making the underrun obsolete.
>> Unfortunately, there is currently no known way to determine whether this
>> is the case or not.
> 
> I think this helps for normal use-cases, so it'd be good to apply.
> But, I prefer having a runtime option for such a behavior change
> (although the default value can be the new behavior).
> 
> Care to add it?

Sure, I can do that - it sounds like a good idea to have it configurable.
Just so I understand you right, exactly how do you expect the user /
application to configure it?

// David

Re: [PATCH] Two patches for Alsa-plugins (pulse)

[Colin Guthrie]

'Twas brillig, and David Henningsson at 25/06/10 11:41 did gyre and gimble:
> Sure, I can do that - it sounds like a good idea to have it configurable.
> Just so I understand you right, exactly how do you expect the user /
> application to configure it?

At a guess I'd say some option in the ~/.asoundrc when using the pulse
plugin... I've never looked at how this is done tho', so could be off
base :D

Col


-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited [http://www.tribalogic.net/]
Open Source:
  Mandriva Linux Contributor [http://www.mandriva.com/]
  PulseAudio Hacker [http://www.pulseaudio.org/]
  Trac Hacker [http://trac.edgewall.org/]

Re: [PATCH] Two patches for Alsa-plugins (pulse)

[Takashi Iwai]

At Fri, 25 Jun 2010 12:41:46 +0200,
David Henningsson wrote:
> 
> On 2010-06-24 08:42, Takashi Iwai wrote:
> > At Wed, 23 Jun 2010 21:57:01 +0200,
> > David Henningsson wrote:
> >>
> >> These two patches are being used in Ubuntu Lucid (released this April)
> >> and are working well enough to have people asking us to backport them to
> >> Karmic. Today I got an email, asking for the upstream status of one of
> >> these patches, since he wanted them in Fedora. If posting them here
> >> isn't the correct way to upstream them, please tell me so.
> >>
> >> The first one (Fix invalid buffer pointer return value) fixes broken logic:
> >>
> >> This patch improves recovering from underruns, and prevents hangs inside
> >> snd_pcm_write* and snd_pcm_read* due to snd_pcm_avail* returning too
> >> low values. It especially helps low latency situations.
> > 
> > This one looks OK.  Applied now.
> 
> Thanks!
> 
> >> The second one (Do not report underruns to the ALSA layer) is more a
> >> change of behavior, which could be questioned. The arguments for
> >> removing that code are these:
> >>
> >>  * If pulseaudio gets an underrun, the normal way to end that underrun
> >> is to feed it with more buffers. This is different from the ALSA way of
> >> dealing with underruns, which requires hardware buffer pointers to be reset.
> >>  * In addition, underrun signals are delivered asynchronously from
> >> pulseaudio. This means that there might be more buffers on the way to
> >> pulseaudio when the underrun is reported, making the underrun obsolete.
> >> Unfortunately, there is currently no known way to determine whether this
> >> is the case or not.
> > 
> > I think this helps for normal use-cases, so it'd be good to apply.
> > But, I prefer having a runtime option for such a behavior change
> > (although the default value can be the new behavior).
> > 
> > Care to add it?
> 
> Sure, I can do that - it sounds like a good idea to have it configurable.
> Just so I understand you right, exactly how do you expect the user /
> application to configure it?

I suppose it can be simply added as an alsa-lib plugin config, i.e.
in SND_PCM_PLUGIN_DEFINE_FUNC(pulse) of alsa-plugins/pulse/pcm_pulse.c,
add the code like below:

	int handle_underrun = 0;
	...

	snd_config_for_each(i, next, conf) {
		...
		if (strcmp(id, "handle_underrun") == 0) {
			handle_underrun = snd_config_get_bool(n);
			if (handle_underrun < 0) {
				SNDERR("Invalid value for %s", id);
				return handle_underrun;
			}
			continue;
		}
	}


Takashi

Re: [PATCH] Two patches for Alsa-plugins (pulse)

[Takashi Iwai]

At Mon, 05 Jul 2010 08:06:45 +0200,
I wrote:
> 
> At Fri, 25 Jun 2010 12:41:46 +0200,
> David Henningsson wrote:
> > 
> > On 2010-06-24 08:42, Takashi Iwai wrote:
> > > At Wed, 23 Jun 2010 21:57:01 +0200,
> > > David Henningsson wrote:
> > >>
> > >> These two patches are being used in Ubuntu Lucid (released this April)
> > >> and are working well enough to have people asking us to backport them to
> > >> Karmic. Today I got an email, asking for the upstream status of one of
> > >> these patches, since he wanted them in Fedora. If posting them here
> > >> isn't the correct way to upstream them, please tell me so.
> > >>
> > >> The first one (Fix invalid buffer pointer return value) fixes broken logic:
> > >>
> > >> This patch improves recovering from underruns, and prevents hangs inside
> > >> snd_pcm_write* and snd_pcm_read* due to snd_pcm_avail* returning too
> > >> low values. It especially helps low latency situations.
> > > 
> > > This one looks OK.  Applied now.
> > 
> > Thanks!
> > 
> > >> The second one (Do not report underruns to the ALSA layer) is more a
> > >> change of behavior, which could be questioned. The arguments for
> > >> removing that code are these:
> > >>
> > >>  * If pulseaudio gets an underrun, the normal way to end that underrun
> > >> is to feed it with more buffers. This is different from the ALSA way of
> > >> dealing with underruns, which requires hardware buffer pointers to be reset.
> > >>  * In addition, underrun signals are delivered asynchronously from
> > >> pulseaudio. This means that there might be more buffers on the way to
> > >> pulseaudio when the underrun is reported, making the underrun obsolete.
> > >> Unfortunately, there is currently no known way to determine whether this
> > >> is the case or not.
> > > 
> > > I think this helps for normal use-cases, so it'd be good to apply.
> > > But, I prefer having a runtime option for such a behavior change
> > > (although the default value can be the new behavior).
> > > 
> > > Care to add it?
> > 
> > Sure, I can do that - it sounds like a good idea to have it configurable.
> > Just so I understand you right, exactly how do you expect the user /
> > application to configure it?
> 
> I suppose it can be simply added as an alsa-lib plugin config, i.e.
> in SND_PCM_PLUGIN_DEFINE_FUNC(pulse) of alsa-plugins/pulse/pcm_pulse.c,
> add the code like below:
> 
> 	int handle_underrun = 0;
> 	...
> 
> 	snd_config_for_each(i, next, conf) {
> 		...
> 		if (strcmp(id, "handle_underrun") == 0) {
> 			handle_underrun = snd_config_get_bool(n);
> 			if (handle_underrun < 0) {
> 				SNDERR("Invalid value for %s", id);
> 				return handle_underrun;
> 			}
> 			continue;
> 		}
> 	}

FYI, I modified the patch and applied to git tree right now.


thanks,

Takashi

Re: [PATCH] Two patches for Alsa-plugins (pulse)

[Raymond Yau]

2010/7/9 Takashi Iwai <tiwai@suse.de>

> At Mon, 05 Jul 2010 08:06:45 +0200,
> I wrote:
> >
> > At Fri, 25 Jun 2010 12:41:46 +0200,
> > David Henningsson wrote:
> > >
> > > On 2010-06-24 08:42, Takashi Iwai wrote:
> > > > At Wed, 23 Jun 2010 21:57:01 +0200,
> > > > David Henningsson wrote:
> > > >>
> > > >> These two patches are being used in Ubuntu Lucid (released this
> April)
> > > >> and are working well enough to have people asking us to backport
> them to
> > > >> Karmic. Today I got an email, asking for the upstream status of one
> of
> > > >> these patches, since he wanted them in Fedora. If posting them here
> > > >> isn't the correct way to upstream them, please tell me so.
> > > >>
> > > >> The first one (Fix invalid buffer pointer return value) fixes broken
> logic:
> > > >>
> > > >> This patch improves recovering from underruns, and prevents hangs
> inside
> > > >> snd_pcm_write* and snd_pcm_read* due to snd_pcm_avail* returning too
> > > >> low values. It especially helps low latency situations.
> > > >
> > > > This one looks OK.  Applied now.
> > >
> > > Thanks!
> > >
> > > >> The second one (Do not report underruns to the ALSA layer) is more a
> > > >> change of behavior, which could be questioned. The arguments for
> > > >> removing that code are these:
> > > >>
> > > >>  * If pulseaudio gets an underrun, the normal way to end that
> underrun
> > > >> is to feed it with more buffers. This is different from the ALSA way
> of
> > > >> dealing with underruns, which requires hardware buffer pointers to
> be reset.
> > > >>  * In addition, underrun signals are delivered asynchronously from
> > > >> pulseaudio. This means that there might be more buffers on the way
> to
> > > >> pulseaudio when the underrun is reported, making the underrun
> obsolete.
> > > >> Unfortunately, there is currently no known way to determine whether
> this
> > > >> is the case or not.
> > > >
> > > > I think this helps for normal use-cases, so it'd be good to apply.
> > > > But, I prefer having a runtime option for such a behavior change
> > > > (although the default value can be the new behavior).
> > > >
> > > > Care to add it?
> > >
> > > Sure, I can do that - it sounds like a good idea to have it
> configurable.
> > > Just so I understand you right, exactly how do you expect the user /
> > > application to configure it?
> >
> > I suppose it can be simply added as an alsa-lib plugin config, i.e.
> > in SND_PCM_PLUGIN_DEFINE_FUNC(pulse) of alsa-plugins/pulse/pcm_pulse.c,
> > add the code like below:
> >
> >       int handle_underrun = 0;
> >       ...
> >
> >       snd_config_for_each(i, next, conf) {
> >               ...
> >               if (strcmp(id, "handle_underrun") == 0) {
> >                       handle_underrun = snd_config_get_bool(n);
> >                       if (handle_underrun < 0) {
> >                               SNDERR("Invalid value for %s", id);
> >                               return handle_underrun;
> >                       }
> >                       continue;
> >               }
> >       }
>
> FYI, I modified the patch and applied to git tree right now.
>
>
> thanks,
>
> Takashi
>


This behaviour is a little bit strange , using the same buffer size 128

aplay -Dhw:0,0 --buffer-size=128 test.wav

report underrun


aplay -D pulse --buffer-size=128 test.wav

did not report underrun but the sound is distorted

Re: [PATCH] Two patches for Alsa-plugins (pulse)

[Takashi Iwai]

At Fri, 9 Jul 2010 21:13:52 +0800,
Raymond Yau wrote:
> 
> 2010/7/9 Takashi Iwai <tiwai@suse.de>
> 
> > At Mon, 05 Jul 2010 08:06:45 +0200,
> > I wrote:
> > >
> > > At Fri, 25 Jun 2010 12:41:46 +0200,
> > > David Henningsson wrote:
> > > >
> > > > On 2010-06-24 08:42, Takashi Iwai wrote:
> > > > > At Wed, 23 Jun 2010 21:57:01 +0200,
> > > > > David Henningsson wrote:
> > > > >>
> > > > >> These two patches are being used in Ubuntu Lucid (released this
> > April)
> > > > >> and are working well enough to have people asking us to backport
> > them to
> > > > >> Karmic. Today I got an email, asking for the upstream status of one
> > of
> > > > >> these patches, since he wanted them in Fedora. If posting them here
> > > > >> isn't the correct way to upstream them, please tell me so.
> > > > >>
> > > > >> The first one (Fix invalid buffer pointer return value) fixes broken
> > logic:
> > > > >>
> > > > >> This patch improves recovering from underruns, and prevents hangs
> > inside
> > > > >> snd_pcm_write* and snd_pcm_read* due to snd_pcm_avail* returning too
> > > > >> low values. It especially helps low latency situations.
> > > > >
> > > > > This one looks OK.  Applied now.
> > > >
> > > > Thanks!
> > > >
> > > > >> The second one (Do not report underruns to the ALSA layer) is more a
> > > > >> change of behavior, which could be questioned. The arguments for
> > > > >> removing that code are these:
> > > > >>
> > > > >>  * If pulseaudio gets an underrun, the normal way to end that
> > underrun
> > > > >> is to feed it with more buffers. This is different from the ALSA way
> > of
> > > > >> dealing with underruns, which requires hardware buffer pointers to
> > be reset.
> > > > >>  * In addition, underrun signals are delivered asynchronously from
> > > > >> pulseaudio. This means that there might be more buffers on the way
> > to
> > > > >> pulseaudio when the underrun is reported, making the underrun
> > obsolete.
> > > > >> Unfortunately, there is currently no known way to determine whether
> > this
> > > > >> is the case or not.
> > > > >
> > > > > I think this helps for normal use-cases, so it'd be good to apply.
> > > > > But, I prefer having a runtime option for such a behavior change
> > > > > (although the default value can be the new behavior).
> > > > >
> > > > > Care to add it?
> > > >
> > > > Sure, I can do that - it sounds like a good idea to have it
> > configurable.
> > > > Just so I understand you right, exactly how do you expect the user /
> > > > application to configure it?
> > >
> > > I suppose it can be simply added as an alsa-lib plugin config, i.e.
> > > in SND_PCM_PLUGIN_DEFINE_FUNC(pulse) of alsa-plugins/pulse/pcm_pulse.c,
> > > add the code like below:
> > >
> > >       int handle_underrun = 0;
> > >       ...
> > >
> > >       snd_config_for_each(i, next, conf) {
> > >               ...
> > >               if (strcmp(id, "handle_underrun") == 0) {
> > >                       handle_underrun = snd_config_get_bool(n);
> > >                       if (handle_underrun < 0) {
> > >                               SNDERR("Invalid value for %s", id);
> > >                               return handle_underrun;
> > >                       }
> > >                       continue;
> > >               }
> > >       }
> >
> > FYI, I modified the patch and applied to git tree right now.
> >
> >
> > thanks,
> >
> > Takashi
> >
> 
> 
> This behaviour is a little bit strange , using the same buffer size 128
> 
> aplay -Dhw:0,0 --buffer-size=128 test.wav
> 
> report underrun
> 
> 
> aplay -D pulse --buffer-size=128 test.wav
> 
> did not report underrun but the sound is distorted

Other subsystem, other behavior. 
The detection of underrun is even unreliable on ALSA native, so user
can't expect it'll get underrun.

The biggest drawback by this change is that whether the sound goes
well or not isn't reported any more to the application layer.
It's an unfortunate design, but it's life with PA.


Takashi

Re: [PATCH] Two patches for Alsa-plugins (pulse)

[David Henningsson]

2010-07-09 18:23, Takashi Iwai skrev:
> At Fri, 9 Jul 2010 21:13:52 +0800,
> Raymond Yau wrote:
>>>>>>> The second one (Do not report underruns to the ALSA layer) is more a
>>>>>>> change of behavior, which could be questioned. The arguments for
>>>>>>> removing that code are these:
>>>>>>>
>>>>>>>  * If pulseaudio gets an underrun, the normal way to end that
>>> underrun
>>>>>>> is to feed it with more buffers. This is different from the ALSA way
>>> of
>>>>>>> dealing with underruns, which requires hardware buffer pointers to
>>> be reset.
>>>>>>>  * In addition, underrun signals are delivered asynchronously from
>>>>>>> pulseaudio. This means that there might be more buffers on the way
>>> to
>>>>>>> pulseaudio when the underrun is reported, making the underrun
>>> obsolete.
>>>>>>> Unfortunately, there is currently no known way to determine whether
>>> this
>>>>>>> is the case or not.
>>>>>>
>>>>>> I think this helps for normal use-cases, so it'd be good to apply.
>>>>>> But, I prefer having a runtime option for such a behavior change
>>>>>> (although the default value can be the new behavior).
>>>>>>
>>>>>> Care to add it?
>>>>>
>>>>> Sure, I can do that - it sounds like a good idea to have it
>>> configurable.
>>>>> Just so I understand you right, exactly how do you expect the user /
>>>>> application to configure it?
>>>>
>>>> I suppose it can be simply added as an alsa-lib plugin config, i.e.
>>>> in SND_PCM_PLUGIN_DEFINE_FUNC(pulse) of alsa-plugins/pulse/pcm_pulse.c,
>>>> add the code like below:
>>>>
>>>>       int handle_underrun = 0;
>>>>       ...
>>>>
>>>>       snd_config_for_each(i, next, conf) {
>>>>               ...
>>>>               if (strcmp(id, "handle_underrun") == 0) {
>>>>                       handle_underrun = snd_config_get_bool(n);
>>>>                       if (handle_underrun < 0) {
>>>>                               SNDERR("Invalid value for %s", id);
>>>>                               return handle_underrun;
>>>>                       }
>>>>                       continue;
>>>>               }
>>>>       }
>>>
>>> FYI, I modified the patch and applied to git tree right now.

Thanks! I've been on holiday this week but thought I would do it in the
following week. Only nice to see it being done :-)

>> This behaviour is a little bit strange , using the same buffer size 128
>>
>> aplay -Dhw:0,0 --buffer-size=128 test.wav
>>
>> report underrun
>>
>>
>> aplay -D pulse --buffer-size=128 test.wav
>>
>> did not report underrun but the sound is distorted
> 
> Other subsystem, other behavior. 
> The detection of underrun is even unreliable on ALSA native, so user
> can't expect it'll get underrun.
> 
> The biggest drawback by this change is that whether the sound goes
> well or not isn't reported any more to the application layer.
> It's an unfortunate design, but it's life with PA.

The question is - is there an application that detects the underrun
condition and actually acts on that condition? If so, this could be a
regression for that app. For other apps it's an improvement, for the
reasons originally stated.

// David

Re: [PATCH] Two patches for Alsa-plugins (pulse)

[Raymond Yau]

2010/7/10 David Henningsson <launchpad.web@epost.diwic.se>

> 2010-07-09 18:23, Takashi Iwai skrev:
> > At Fri, 9 Jul 2010 21:13:52 +0800,
> > Raymond Yau wrote:
> >>>>>>> The second one (Do not report underruns to the ALSA layer) is more
> a
> >>>>>>> change of behavior, which could be questioned. The arguments for
> >>>>>>> removing that code are these:
> >>>>>>>
> >>>>>>>  * If pulseaudio gets an underrun, the normal way to end that
> >>> underrun
> >>>>>>> is to feed it with more buffers. This is different from the ALSA
> way
> >>> of
> >>>>>>> dealing with underruns, which requires hardware buffer pointers to
> >>> be reset.
> >>>>>>>  * In addition, underrun signals are delivered asynchronously from
> >>>>>>> pulseaudio. This means that there might be more buffers on the way
> >>> to
> >>>>>>> pulseaudio when the underrun is reported, making the underrun
> >>> obsolete.
> >>>>>>> Unfortunately, there is currently no known way to determine whether
> >>> this
> >>>>>>> is the case or not.
> >>>>>>
> >>>>>> I think this helps for normal use-cases, so it'd be good to apply.
> >>>>>> But, I prefer having a runtime option for such a behavior change
> >>>>>> (although the default value can be the new behavior).
> >>>>>>
> >>>>>> Care to add it?
> >>>>>
> >>>>> Sure, I can do that - it sounds like a good idea to have it
> >>> configurable.
> >>>>> Just so I understand you right, exactly how do you expect the user /
> >>>>> application to configure it?
> >>>>
> >>>> I suppose it can be simply added as an alsa-lib plugin config, i.e.
> >>>> in SND_PCM_PLUGIN_DEFINE_FUNC(pulse) of
> alsa-plugins/pulse/pcm_pulse.c,
> >>>> add the code like below:
> >>>>
> >>>>       int handle_underrun = 0;
> >>>>       ...
> >>>>
> >>>>       snd_config_for_each(i, next, conf) {
> >>>>               ...
> >>>>               if (strcmp(id, "handle_underrun") == 0) {
> >>>>                       handle_underrun = snd_config_get_bool(n);
> >>>>                       if (handle_underrun < 0) {
> >>>>                               SNDERR("Invalid value for %s", id);
> >>>>                               return handle_underrun;
> >>>>                       }
> >>>>                       continue;
> >>>>               }
> >>>>       }
> >>>
> >>> FYI, I modified the patch and applied to git tree right now.
>
> Thanks! I've been on holiday this week but thought I would do it in the
> following week. Only nice to see it being done :-)
>
> >> This behaviour is a little bit strange , using the same buffer size 128
> >>
> >> aplay -Dhw:0,0 --buffer-size=128 test.wav
> >>
> >> report underrun
> >>
> >>
> >> aplay -D pulse --buffer-size=128 test.wav
> >>
> >> did not report underrun but the sound is distorted
> >
> > Other subsystem, other behavior.
> > The detection of underrun is even unreliable on ALSA native, so user
> > can't expect it'll get underrun.
>


Before this patch , using pulse device also report underrun


When using hw device ,the sound is quite good , at least I cannot hear any
distortion and the reported underrun  only appear a few times on the
terminal


underrun!!! (at least 0.210 ms long)

Did alsa-lib calculate 0.210 ms from hw_ptr , app_ptr and sample rate ?

aplay -v -Dhw:0,0 --buffer-size=128 t3.wav
Playing WAVE 't3.wav' : Signed 16 bit Little Endian, Rate 44100 Hz, Stereo
Hardware PCM card 0 'Aureal Vortex au8830' device 0 subdevice 0
Its setup is:
  stream       : PLAYBACK
  access       : RW_INTERLEAVED
  format       : S16_LE
  subformat    : STD
  channels     : 2
  rate         : 44100
  exact rate   : 44100 (44100/1)
  msbits       : 16
  buffer_size  : 128
  period_size  : 32
  period_time  : 725
  tstamp_mode  : NONE
  period_step  : 1
  avail_min    : 32
  period_event : 0
  start_threshold  : 128
  stop_threshold   : 128
  silence_threshold: 0
  silence_size : 0
  boundary     : 1073741824
  appl_ptr     : 0
  hw_ptr       : 0
underrun!!! (at least 0.210 ms long)
Status:
  state       : XRUN
  trigger_time: 2794.391051578
  tstamp      : 2794.393149452
  delay       : 0
  avail       : 153
  avail_max   : 153
underrun!!! (at least 0.001 ms long)
Status:
  state       : XRUN
  trigger_time: 2801.590388423
  tstamp      : 2801.590394868
  delay       : 0
  avail       : 135
  avail_max   : 135
underrun!!! (at least 0.032 ms long)
Status:
  state       : XRUN
  trigger_time: 2801.718347873
  tstamp      : 2801.718661699
  delay       : 0
  avail       : 153
  avail_max   : 153
underrun!!! (at least 0.067 ms long)
Status:
  state       : XRUN
  trigger_time: 2805.978507564
  tstamp      : 2805.979173807
  delay       : 0
  avail       : 162
  avail_max   : 162
underrun!!! (at least 0.065 ms long)
Status:
  state       : XRUN
  trigger_time: 2807.118761300
  tstamp      : 2807.119411502
  delay       : 0
  avail       : 162
  avail_max   : 162
underrun!!! (at least 0.058 ms long)
Status:
  state       : XRUN
  trigger_time: 2808.685565670
  tstamp      : 2808.686139666
  delay       : 0
  avail       : 153
  avail_max   : 153
underrun!!! (at least 0.001 ms long)
Status:
  state       : XRUN
  trigger_time: 2808.699838404
  tstamp      : 2808.699844705
  delay       : 0
  avail       : 133
  avail_max   : 133



>
> > The biggest drawback by this change is that whether the sound goes
> > well or not isn't reported any more to the application layer.
> > It's an unfortunate design, but it's life with PA.
>
> The question is - is there an application that detects the underrun
> condition and actually acts on that condition? If so, this could be a
> regression for that app. For other apps it's an improvement, for the
> reasons originally stated.
>
> // David
>

You have to ask jackd user , they usually fine-tune(increase/decrease) the
period-size of jackd manually based on the occurrence of underrun in order
to achieve the lowest latency


This give a false impression to the user that PA can provide low latency
without under-run and pulse device is even perform better than hw device.


>>  Reporting underruns to ALSA seems to do more bad than good, for these
reasons:

 * If pulseaudio gets an underrun, the normal way to end that underrun is to

   feed it with more buffers. This is different from the ALSA way of dealing

   with underruns, which requires hardware buffer pointers to be reset.

 * In addition, underrun signals are delivered asynchronously from
pulseaudio.

   This means that there might be more buffers on the way to pulseaudio when

   the underrun is reported, making the underrun obsolete. Unfortunately,

   there is currently no known way to determine whether this is the case or

   not.

Do you mean PA server get underrun from the sound driver when you mention
that
if pulseaudio get underrun, the normal way to end that underrun is to feed
it with more buffers." ?

The normal way for ALSA application is to prevent underrun occur rather to
let underrun occur be selecting a larger period size or wake up more
frequently to feed data to the sound card



PA developer often complain the driver did not provide accurate playback
position by the pcm_pointer callback

Did the pulse_pointer callback also provide accurate position of the pulse
device ?

Re: [PATCH] Two patches for Alsa-plugins (pulse)

[Raymond Yau]

2010/7/10 David Henningsson <launchpad.web@epost.diwic.se>

>
>
> The question is - is there an application that detects the underrun
> condition and actually acts on that condition? If so, this could be a
> regression for that app. For other apps it's an improvement, for the
> reasons originally stated.
>
> // David
>

Some application (e.g. xmms and audacious) allow user to configure the
period/buffer time

I don't know why the alsa-lib/test/latency did not run when using pulse
device


./latency -P pulse -C pulse -m 8192 -M 8192 -t 1 -p
/home/raymond/alsa-lib/test/.libs/lt-latency: invalid option -- 't'
!!!Scheduler set to Round Robin with priority 99 FAILED!!!
Playback device is pulse
Capture device is pulse
Parameters are 22050Hz, S16_LE, 2 channels, non-blocking mode
Poll mode: yes
Loop limit is 661500 frames, minimum latency = 8192, maximum latency = 8192



alsa-lib/test/latency -P hw:1,0 -C hw:1,0 -m 8192 -M 8192 -t 1 -p
/home/raymond/alsa-lib/test/.libs/lt-latency: invalid option -- 't'
!!!Scheduler set to Round Robin with priority 99 FAILED!!!
Playback device is hw:1,0
Capture device is hw:1,0
Parameters are 22050Hz, S16_LE, 2 channels, non-blocking mode
Poll mode: yes
Loop limit is 661500 frames, minimum latency = 8192, maximum latency = 8192
Hardware PCM card 1 'HDA Intel' device 0 subdevice 0
Its setup is:
  stream       : PLAYBACK
  access       : RW_INTERLEAVED
  format       : S16_LE
  subformat    : STD
  channels     : 2
  rate         : 22050
  exact rate   : 22050 (22050/1)
  msbits       : 16
  buffer_size  : 8192
  period_size  : 4096
  period_time  : 185759
  tstamp_mode  : NONE
  period_step  : 1
  avail_min    : 4096
  period_event : 0
  start_threshold  : 2147483647
  stop_threshold   : 8192
  silence_threshold: 0
  silence_size : 0
  boundary     : 1073741824
  appl_ptr     : 0
  hw_ptr       : 0
Hardware PCM card 1 'HDA Intel' device 0 subdevice 0
Its setup is:
  stream       : CAPTURE
  access       : RW_INTERLEAVED
  format       : S16_LE
  subformat    : STD
  channels     : 2
  rate         : 22050
  exact rate   : 22050 (22050/1)
  msbits       : 16
  buffer_size  : 8192
  period_size  : 4096
  period_time  : 185759
  tstamp_mode  : NONE
  period_step  : 1
  avail_min    : 4096
  period_event : 0
  start_threshold  : 2147483647
  stop_threshold   : 8192
  silence_threshold: 0
  silence_size : 0
  boundary     : 1073741824
  appl_ptr     : 0
  hw_ptr       : 0
Trying latency 8192 frames, 371519.274us, 371.519274ms (2.6917Hz)
Success
Playback:
*** frames = 671744 ***
  state       : RUNNING
  trigger_time: 14895.45132386
  tstamp      : 14925.138525741
  delay       : 8144
  avail       : 48
  avail_max   : 4152
Capture:
*** frames = 663552 ***
  state       : RUNNING
  trigger_time: 14895.45132386
  tstamp      : 14925.138585923
  delay       : 1
  avail       : 1
  avail_max   : 4097
Maximum read: 4096 frames
Maximum read latency: 185759.637us, 185.759637ms (5.3833Hz)
Hardware sync
Playback time = 14895.45132, Record time = 14895.45132, diff = 0

Re: [PATCH] Two patches for Alsa-plugins (pulse)

[David Henningsson]

2010-07-10 00:58, Raymond Yau skrev:
> 2010/7/10 David Henningsson <launchpad.web@epost.diwic.se>
> 
>> 2010-07-09 18:23, Takashi Iwai skrev:
>>> At Fri, 9 Jul 2010 21:13:52 +0800,
>>> Raymond Yau wrote:
>>>>>>>>> The second one (Do not report underruns to the ALSA layer) is more
>> a
>>>>>>>>> change of behavior, which could be questioned. The arguments for
>>>>>>>>> removing that code are these:
>>>>>>>>>
>>>>>>>>>  * If pulseaudio gets an underrun, the normal way to end that
>>>>> underrun
>>>>>>>>> is to feed it with more buffers. This is different from the ALSA
>> way
>>>>> of
>>>>>>>>> dealing with underruns, which requires hardware buffer pointers to
>>>>> be reset.
>>>>>>>>>  * In addition, underrun signals are delivered asynchronously from
>>>>>>>>> pulseaudio. This means that there might be more buffers on the way
>>>>> to
>>>>>>>>> pulseaudio when the underrun is reported, making the underrun
>>>>> obsolete.
>>>>>>>>> Unfortunately, there is currently no known way to determine whether
>>>>> this
>>>>>>>>> is the case or not.
>>>>>>>>
>>>>>>>> I think this helps for normal use-cases, so it'd be good to apply.
>>>>>>>> But, I prefer having a runtime option for such a behavior change
>>>>>>>> (although the default value can be the new behavior).
>>>>>>>>
>>>>>>>> Care to add it?
>>>>>>>
>>>>>>> Sure, I can do that - it sounds like a good idea to have it
>>>>> configurable.
>>>>>>> Just so I understand you right, exactly how do you expect the user /
>>>>>>> application to configure it?
>>>>>>
>>>>>> I suppose it can be simply added as an alsa-lib plugin config, i.e.
>>>>>> in SND_PCM_PLUGIN_DEFINE_FUNC(pulse) of
>> alsa-plugins/pulse/pcm_pulse.c,
>>>>>> add the code like below:
>>>>>>
>>>>>>       int handle_underrun = 0;
>>>>>>       ...
>>>>>>
>>>>>>       snd_config_for_each(i, next, conf) {
>>>>>>               ...
>>>>>>               if (strcmp(id, "handle_underrun") == 0) {
>>>>>>                       handle_underrun = snd_config_get_bool(n);
>>>>>>                       if (handle_underrun < 0) {
>>>>>>                               SNDERR("Invalid value for %s", id);
>>>>>>                               return handle_underrun;
>>>>>>                       }
>>>>>>                       continue;
>>>>>>               }
>>>>>>       }
>>>>>
>>>>> FYI, I modified the patch and applied to git tree right now.
>>
>> Thanks! I've been on holiday this week but thought I would do it in the
>> following week. Only nice to see it being done :-)
>>
>>>> This behaviour is a little bit strange , using the same buffer size 128
>>>>
>>>> aplay -Dhw:0,0 --buffer-size=128 test.wav
>>>>
>>>> report underrun
>>>>
>>>>
>>>> aplay -D pulse --buffer-size=128 test.wav
>>>>
>>>> did not report underrun but the sound is distorted
>>>
>>> Other subsystem, other behavior.
>>> The detection of underrun is even unreliable on ALSA native, so user
>>> can't expect it'll get underrun.
>>
> 
> 
> Before this patch , using pulse device also report underrun
> 
> 
> When using hw device ,the sound is quite good , at least I cannot hear any
> distortion and the reported underrun  only appear a few times on the
> terminal
> 
> 
> underrun!!! (at least 0.210 ms long)
> 
> Did alsa-lib calculate 0.210 ms from hw_ptr , app_ptr and sample rate ?

Good question, I'm sure you can read that in the source :-)

http://git.alsa-project.org/?p=alsa-utils.git;a=blob;f=aplay/aplay.c;h=e1d8e6aab69642c2a57bd53b2d4ce9d2a24ad0b6;hb=HEAD

>>> The biggest drawback by this change is that whether the sound goes
>>> well or not isn't reported any more to the application layer.
>>> It's an unfortunate design, but it's life with PA.
>>
>> The question is - is there an application that detects the underrun
>> condition and actually acts on that condition? If so, this could be a
>> regression for that app. For other apps it's an improvement, for the
>> reasons originally stated.
> 
> You have to ask jackd user , they usually fine-tune(increase/decrease) the
> period-size of jackd manually based on the occurrence of underrun in order
> to achieve the lowest latency

Correct me if I'm wrong, but I think jackd users are not likely to run
jackd over the alsa-pulse bridge but rather run PA on top of jackd, so
that is not an issue.

> This give a false impression to the user that PA can provide low latency
> without under-run and pulse device is even perform better than hw device.

To complicate matters more, with PA in the stack you can have underruns
both on the client app <=> PA side, and on the PA <=> alsa-driver side.
So the false impression was there before, only it is slightly worse now.

>>>  Reporting underruns to ALSA seems to do more bad than good, for these
> reasons:
> 
>  * If pulseaudio gets an underrun, the normal way to end that underrun is to
>    feed it with more buffers. This is different from the ALSA way of dealing
>    with underruns, which requires hardware buffer pointers to be reset.
>  * In addition, underrun signals are delivered asynchronously from
> pulseaudio.
>    This means that there might be more buffers on the way to pulseaudio when
>    the underrun is reported, making the underrun obsolete. Unfortunately,
>    there is currently no known way to determine whether this is the case or
>    not.
> 
> Do you mean PA server get underrun from the sound driver when you mention
> that
> if pulseaudio get underrun, the normal way to end that underrun is to feed
> it with more buffers." ?

I'm talking about the client-app <=> PA underrun here. The PA <=> sound
driver underrun is handled by PA internally, and, IIRC, is never
reported to the client app (even before the patch).

> The normal way for ALSA application is to prevent underrun occur rather to
> let underrun occur be selecting a larger period size or wake up more
> frequently to feed data to the sound card

Do you know an ALSA application which actually does this automatically,
i e selects a larger period size when it detects an underrun?

> PA developer often complain the driver did not provide accurate playback
> position by the pcm_pointer callback
> 
> Did the pulse_pointer callback also provide accurate position of the pulse
> device ?

Yes, but it provides the pointer to the fake ring buffer (the alsa-lib
plugin system expects that all pcm plugins are ring buffers, which seems
to me like a design flaw).

// David

Re: [PATCH] Two patches for Alsa-plugins (pulse)

[Raymond Yau]

2010/6/24 David Henningsson <launchpad.web@epost.diwic.se>

> These two patches are being used in Ubuntu Lucid (released this April)
> and are working well enough to have people asking us to backport them to
> Karmic. Today I got an email, asking for the upstream status of one of
> these patches, since he wanted them in Fedora. If posting them here
> isn't the correct way to upstream them, please tell me so.
>
> The first one (Fix invalid buffer pointer return value) fixes broken logic:
>
> This patch improves recovering from underruns, and prevents hangs inside
> snd_pcm_write* and snd_pcm_read* due to snd_pcm_avail* returning too
> low values. It especially helps low latency situations.
>
> The second one (Do not report underruns to the ALSA layer) is more a
> change of behavior, which could be questioned. The arguments for
> removing that code are these:
>
>  * If pulseaudio gets an underrun, the normal way to end that underrun
> is to feed it with more buffers. This is different from the ALSA way of
> dealing with underruns, which requires hardware buffer pointers to be
> reset.
>  * In addition, underrun signals are delivered asynchronously from
> pulseaudio. This means that there might be more buffers on the way to
> pulseaudio when the underrun is reported, making the underrun obsolete.
> Unfortunately, there is currently no known way to determine whether this
> is the case or not.
>
> // David
>
> For your test case (mpg123 produces skippy output in 9.10
https://bugs.launchpad.net/ubuntu/+source/alsa-plugins/+bug/464008


play silence when there is underrun

http://www.mpg123.de/cgi-bin/viewvc.cgi/trunk/src/output/alsa.c?r1=377&r2=385


static void flush_alsa(audio_output_t *ao)
{
	snd_pcm_t *pcm=(snd_pcm_t*)ao->userptr;

	/* is this the optimal solution? - we should figure out what we
really whant from this function */

debug("alsa drop");
	snd_pcm_drop(pcm);
debug("alsa prepare");
	snd_pcm_prepare(pcm);
debug("alsa flush done");
}


it  implement pause by create an underrun


[libmpg123.c:1083] debug: tell: 2207/1 first 0 buffer 0; frame_outs=2542464
[libmpg123.c:623] debug: read of frame 2207 returned 1 (to_decode=1) at
sample 2541935
[libmpg123.c:904] debug: got next frame, 1
[libmpg123.c:885] debug: decoding
[term.c:158] debug: control for frame: 2207
[alsa.c:246] debug: alsa drop
[alsa.c:248] debug: alsa prepare
[alsa.c:250] debug: alsa flush done
Stopped
[alsa.c:246] debug: alsa drop
[alsa.c:248] debug: alsa prepare
[alsa.c:250] debug: alsa flush done
[mpg123.c:632] debug: play_frame
[libmpg123.c:620] debug: read frame
[parse.c:456] debug: trying to get frame 2208 at 923915
[parse.c:772] debug: Frame 2208 fffb9260 414, next filepos=924333
[index.c:85] debug: wanting to add to fill 552, step 4, size 1000

Re: [PATCH] Two patches for Alsa-plugins (pulse)

[Raymond Yau]

2010/7/10 David Henningsson <launchpad.web@epost.diwic.se>

>
>
> > PA developer often complain the driver did not provide accurate playback
> > position by the pcm_pointer callback
> >
> > Did the pulse_pointer callback also provide accurate position of the
> pulse
> > device ?
>
> Yes, but it provides the pointer to the fake ring buffer (the alsa-lib
> plugin system expects that all pcm plugins are ring buffers, which seems
> to me like a design flaw).
>
>
This mean that pulse_pointer jump suddenly when PA wake up from sleep and
write audio to sound card until the ring buffer is empty, Those "0 bytes in
queue " message is the result of PA server flush the ring buffer.

Did alsa-lib regard this condition as underrun, since the application
pointer and hardware pointer of the pulse device  seem at the same point ?

Is this the reason why alsa-time-test.c fail on the pulse device ?

Re: [PATCH] Two patches for Alsa-plugins (pulse)

[David Henningsson]

2010-07-13 07:08, Raymond Yau skrev:
> 2010/7/10 David Henningsson <launchpad.web@epost.diwic.se>
> 
>>> PA developer often complain the driver did not provide accurate playback
>>> position by the pcm_pointer callback
>>>
>>> Did the pulse_pointer callback also provide accurate position of the
>> pulse
>>> device ?
>>
>> Yes, but it provides the pointer to the fake ring buffer (the alsa-lib
>> plugin system expects that all pcm plugins are ring buffers, which seems
>> to me like a design flaw).
>>
> This mean that pulse_pointer jump suddenly when PA wake up from sleep and
> write audio to sound card until the ring buffer is empty, Those "0 bytes in
> queue " message is the result of PA server flush the ring buffer.

> Did alsa-lib regard this condition as underrun, since the application
> pointer and hardware pointer of the pulse device  seem at the same point ?

No, I don't think so. An underrun message is a special message sent from
the PA server and should not be directly related to the fake ring
buffer, and it is that message now being ignored (configurable, thanks
to Takashi).

My first patch
(http://git.alsa-project.org/?p=alsa-plugins.git;a=commit;h=1675414)
fixes so that if pa_stream_writeable_size returns a value greater than
the ring buffer's size, it only reports size-1 bytes available, which
means that the alsa app won't see it as the application pointer and hw
pointer being the same.

> Is this the reason why alsa-time-test.c fail on the pulse device ?

No idea, haven't looked further into that application.

// David

Re: [PATCH] Two patches for Alsa-plugins (pulse)

[Raymond Yau]

2010/7/13 David Henningsson <launchpad.web@epost.diwic.se>

> 2010-07-13 07:08, Raymond Yau skrev:
> > 2010/7/10 David Henningsson <launchpad.web@epost.diwic.se>
> >
> >>> PA developer often complain the driver did not provide accurate
> playback
> >>> position by the pcm_pointer callback
> >>>
> >>> Did the pulse_pointer callback also provide accurate position of the
> >> pulse
> >>> device ?
> >>
> >> Yes, but it provides the pointer to the fake ring buffer (the alsa-lib
> >> plugin system expects that all pcm plugins are ring buffers, which seems
> >> to me like a design flaw).
> >>
> > This mean that pulse_pointer jump suddenly when PA wake up from sleep and
> > write audio to sound card until the ring buffer is empty, Those "0 bytes
> in
> > queue " message is the result of PA server flush the ring buffer.
>
> > Did alsa-lib regard this condition as underrun, since the application
> > pointer and hardware pointer of the pulse device  seem at the same point
> ?
>
> No, I don't think so. An underrun message is a special message sent from
> the PA server and should not be directly related to the fake ring
> buffer, and it is that message now being ignored (configurable, thanks
> to Takashi).
>

In fedora 13 , the two underrun messages appear almost immediately after
starting mpg123
but mpg123 does not report any underrun on Fedora 10

So it look like regression in PA server side if you told me that "An
underrun message is a special message sent from the PA server " instead of
underrun detected by alsa-lib by checking the pulse_pointer and the
application ptr of the pulse device

Under what condition did PA server send the underrun message ?


>
> My first patch
> (http://git.alsa-project.org/?p=alsa-plugins.git;a=commit;h=1675414)
> fixes so that if pa_stream_writeable_size returns a value greater than
> the ring buffer's size, it only reports size-1 bytes available, which
> means that the alsa app won't see it as the application pointer and hw
> pointer being the same.
>
> > Is this the reason why alsa-time-test.c fail on the pulse device ?
>
> No idea, haven't looked further into that application.
>
> // David
>

Re: [PATCH] Two patches for Alsa-plugins (pulse)

[Raymond Yau]

2010/7/10 Takashi Iwai <tiwai@suse.de>

> At Fri, 9 Jul 2010 21:13:52 +0800,
> Raymond Yau wrote:
>
>
> Other subsystem, other behavior.
> The detection of underrun is even unreliable on ALSA native, so user
> can't expect it'll get underrun.
>

>From my testing , only hda_intel can run the alsa-lib/test/latency with
-m512 -M512 , au8830 always fail because of xrun and pulse cannot start the
test


Did the latency test require the driver to support SNDRV_PCM_INFO_SYNC_START
?

latency.c seem using a very large value for the start_threshold

    err = snd_pcm_sw_params_set_start_threshold(handle, swparams,
0x7fffffff);



 ./latency -r 44100 -Phw:1 -Chw:1 -m 512 -M 512
!!!Scheduler set to Round Robin with priority 99 FAILED!!!
Playback device is hw:1
Capture device is hw:1
Parameters are 44100Hz, S16_LE, 2 channels, non-blocking mode
Poll mode: no
Loop limit is 1323000 frames, minimum latency = 512, maximum latency = 512
buffer_size :0 252
Hardware PCM card 1 'HDA Intel' device 0 subdevice 0
Its setup is:
  stream       : PLAYBACK
  access       : RW_INTERLEAVED
  format       : S16_LE
  subformat    : STD
  channels     : 2
  rate         : 44100
  exact rate   : 44100 (44100/1)
  msbits       : 16
  buffer_size  : 512
  period_size  : 256
  period_time  : 5804
  tstamp_mode  : NONE
  period_step  : 1
  avail_min    : 256
  period_event : 0
  start_threshold  : 2147483647
  stop_threshold   : 512
  silence_threshold: 0
  silence_size : 0
  boundary     : 1073741824
  appl_ptr     : 0
  hw_ptr       : 0
Hardware PCM card 1 'HDA Intel' device 0 subdevice 0
Its setup is:
  stream       : CAPTURE
  access       : RW_INTERLEAVED
  format       : S16_LE
  subformat    : STD
  channels     : 2
  rate         : 44100
  exact rate   : 44100 (44100/1)
  msbits       : 16
  buffer_size  : 512
  period_size  : 256
  period_time  : 5804
  tstamp_mode  : NONE
  period_step  : 1
  avail_min    : 256
  period_event : 0
  start_threshold  : 2147483647
  stop_threshold   : 512
  silence_threshold: 0
  silence_size : 0
  boundary     : 1073741824
  appl_ptr     : 0
  hw_ptr       : 0
Trying latency 512 frames, 11609.977us, 11.609977ms (86.1328Hz)
Success
Playback:
*** frames = 1323512 ***
  state       : RUNNING
  trigger_time: 18306.838476235
  tstamp      : 18336.838630238
  delay       : 464
  avail       : 48
  avail_max   : 80
Capture:
*** frames = 1323000 ***
  state       : RUNNING
  trigger_time: 18306.838476235
  tstamp      : 18336.838668663
  delay       : 0
  avail       : 0
  avail_max   : 32
Maximum read: 32 frames
Maximum read latency: 725.624us, 0.725624ms (1378.1250Hz)
Hardware sync
Playback time = 18306.838476, Record time = 18306.838476, diff = 0

Re: [PATCH] Two patches for Alsa-plugins (pulse)

[Raymond Yau]

2010/7/10 David Henningsson <launchpad.web@epost.diwic.se>

> 2010-07-09 18:23, Takashi Iwai skrev:
> > At Fri, 9 Jul 2010 21:13:52 +0800,
> > Raymond Yau wrote:
> >>>>>>> The second one (Do not report underruns to the ALSA layer) is more
> a
> >>>>>>> change of behavior, which could be questioned. The arguments for
> >>>>>>> removing that code are these:
> >>>>>>>
> >>>>>>>  * If pulseaudio gets an underrun, the normal way to end that
> >>> underrun
> >>>>>>> is to feed it with more buffers. This is different from the ALSA
> way
> >>> of
> >>>>>>> dealing with underruns, which requires hardware buffer pointers to
> >>> be reset.
> >>>>>>>  * In addition, underrun signals are delivered asynchronously from
> >>>>>>> pulseaudio. This means that there might be more buffers on the way
> >>> to
> >>>>>>> pulseaudio when the underrun is reported, making the underrun
> >>> obsolete.
> >>>>>>> Unfortunately, there is currently no known way to determine whether
> >>> this
> >>>>>>> is the case or not.
> >>>>>>
> >>>>>> I think this helps for normal use-cases, so it'd be good to apply.
> >>>>>> But, I prefer having a runtime option for such a behavior change
> >>>>>> (although the default value can be the new behavior).
> >>>>>>
> >>>>>> Care to add it?
> >>>>>
> >>>>> Sure, I can do that - it sounds like a good idea to have it
> >>> configurable.
> >>>>> Just so I understand you right, exactly how do you expect the user /
> >>>>> application to configure it?
> >>>>
> >>>> I suppose it can be simply added as an alsa-lib plugin config, i.e.
> >>>> in SND_PCM_PLUGIN_DEFINE_FUNC(pulse) of
> alsa-plugins/pulse/pcm_pulse.c,
> >>>> add the code like below:
> >>>>
> >>>>       int handle_underrun = 0;
> >>>>       ...
> >>>>
> >>>>       snd_config_for_each(i, next, conf) {
> >>>>               ...
> >>>>               if (strcmp(id, "handle_underrun") == 0) {
> >>>>                       handle_underrun = snd_config_get_bool(n);
> >>>>                       if (handle_underrun < 0) {
> >>>>                               SNDERR("Invalid value for %s", id);
> >>>>                               return handle_underrun;
> >>>>                       }
> >>>>                       continue;
> >>>>               }
> >>>>       }
> >>>
> >>> FYI, I modified the patch and applied to git tree right now.
>
> Thanks! I've been on holiday this week but thought I would do it in the
> following week. Only nice to see it being done :-)
>
> >> This behaviour is a little bit strange , using the same buffer size 128
> >>
> >> aplay -Dhw:0,0 --buffer-size=128 test.wav
> >>
> >> report underrun
> >>
> >>
> >> aplay -D pulse --buffer-size=128 test.wav
> >>
> >> did not report underrun but the sound is distorted
> >
> > Other subsystem, other behavior.
> > The detection of underrun is even unreliable on ALSA native, so user
> > can't expect it'll get underrun.
> >
> > The biggest drawback by this change is that whether the sound goes
> > well or not isn't reported any more to the application layer.
> > It's an unfortunate design, but it's life with PA.
>
> The question is - is there an application that detects the underrun
> condition and actually acts on that condition? If so, this could be a
> regression for that app. For other apps it's an improvement, for the
> reasons originally stated.
>
>
This patch by Lennart Poettering should answer your question

http://git.alsa-project.org/?p=alsa-plugins.git;a=commit;h=660e0f16145167a966e9b8387e9a8e08274a8f7c

Re: [PATCH] Two patches for Alsa-plugins (pulse)

[David Henningsson]

2010-07-29 09:11, Raymond Yau skrev:
> 2010/7/10 David Henningsson <launchpad.web@epost.diwic.se>
>>> The biggest drawback by this change is that whether the sound goes
>>> well or not isn't reported any more to the application layer.
>>> It's an unfortunate design, but it's life with PA.
>>
>> The question is - is there an application that detects the underrun
>> condition and actually acts on that condition? If so, this could be a
>> regression for that app. For other apps it's an improvement, for the
>> reasons originally stated.
>>
>>
> This patch by Lennart Poettering should answer your question
> 
> http://git.alsa-project.org/?p=alsa-plugins.git;a=commit;h=660e0f16145167a966e9b8387e9a8e08274a8f7c

Okay, so we've broken XMMS, at least the 2007 version of it?

The better solution to this problem would be to add a write_pointer to
the xrun callback (and PA's native protocol), so we can detect whether
the underrun is obsolete or not and act accordingly.

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic

Re: [PATCH] Two patches for Alsa-plugins (pulse)

[Raymond Yau]

2010/7/29 David Henningsson <david.henningsson@canonical.com>

> 2010-07-29 09:11, Raymond Yau skrev:
> > 2010/7/10 David Henningsson <launchpad.web@epost.diwic.se>
> >>> The biggest drawback by this change is that whether the sound goes
> >>> well or not isn't reported any more to the application layer.
> >>> It's an unfortunate design, but it's life with PA.
> >>
> >> The question is - is there an application that detects the underrun
> >> condition and actually acts on that condition? If so, this could be a
> >> regression for that app. For other apps it's an improvement, for the
> >> reasons originally stated.
> >>
> >>
> > This patch by Lennart Poettering should answer your question
> >
> >
> http://git.alsa-project.org/?p=alsa-plugins.git;a=commit;h=660e0f16145167a966e9b8387e9a8e08274a8f7c
>
> Okay, so we've broken XMMS, at least the 2007 version of it?
>
> The better solution to this problem would be to add a write_pointer to
> the xrun callback (and PA's native protocol), so we can detect whether
> the underrun is obsolete or not and act accordingly.
>
>
when using xmms  libao plugin in mandriva 2010 , xmms freeze at the end of
the playback