All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] alsa-plugins: Pulse: only underrun if no more data has been written
@ 2011-08-18 15:06 David Henningsson
  2011-08-19  7:43 ` Takashi Iwai
  0 siblings, 1 reply; 14+ messages in thread
From: David Henningsson @ 2011-08-18 15:06 UTC (permalink / raw)
  To: ALSA Development Mailing List

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

With the new/upcoming version of PulseAudio (0.99.x) there is a protocol 
addition that makes it possible to handle underruns better in the pulse 
plugin.

The attached patch implements that, but it has two flaws I wouldn't mind 
getting some help with if possible:

1) Since this uses the new API function pa_stream_get_underflow_index, 
it won't compile with current stable PulseAudio versions, only the 
upcoming version.

2) So now there are three possibilities for handle_underrun ( 0 = never, 
1 = always, 2 = the new improved one that IMO should be used). Since 
handle_underrun is a bool in the config, it can not be set to "2", only 
0 or 1.

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

[-- Attachment #2: 0001-alsa-plugins-Pulse-only-underrun-if-no-more-data-has.patch --]
[-- Type: text/x-patch, Size: 2032 bytes --]

>From 4e31ba395b751a6ab3254256dc8a227b3be67932 Mon Sep 17 00:00:00 2001
From: David Henningsson <david.henningsson@canonical.com>
Date: Tue, 2 Aug 2011 14:49:04 +0200
Subject: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been written

If more data has already been written after the underrun, the underrun
will automatically end and therefore we should not report it or
restart the stream.

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 pulse/pcm_pulse.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c
index d6c6792..9c4a7e5 100644
--- a/pulse/pcm_pulse.c
+++ b/pulse/pcm_pulse.c
@@ -39,9 +39,10 @@ typedef struct snd_pcm_pulse {
 	size_t last_size;
 	size_t ptr;
 	int underrun;
-	int handle_underrun;
+	int handle_underrun; /* can be 0=never, 1=always or 2=only if more data has not been written */
 
 	size_t offset;
+	int64_t written;
 
 	pa_stream *stream;
 
@@ -459,6 +460,7 @@ static snd_pcm_sframes_t pulse_write(snd_pcm_ioplug_t * io,
 	}
 
 	/* Make sure the buffer pointer is in sync */
+	pcm->written += writebytes;
 	pcm->last_size -= writebytes;
 	ret = update_ptr(pcm);
 	if (ret < 0)
@@ -594,7 +596,8 @@ static void stream_underrun_cb(pa_stream * p, void *userdata)
 	if (!pcm->p)
 		return;
 
-	pcm->underrun = 1;
+	if (pcm->handle_underrun == 1 || pcm->written <= pa_stream_get_underflow_index(p))
+		pcm->underrun = 1;
 }
 
 static void stream_latency_cb(pa_stream *p, void *userdata) {
@@ -691,6 +694,7 @@ static int pulse_prepare(snd_pcm_ioplug_t * io)
 		goto finish;
 	}
 
+	pcm->written = 0;
 	pa_stream_set_state_callback(pcm->stream, stream_state_cb, pcm);
 	pa_stream_set_latency_update_callback(pcm->stream, stream_latency_cb, pcm);
 
@@ -983,7 +987,7 @@ SND_PCM_PLUGIN_DEFINE_FUNC(pulse)
 	const char *server = NULL;
 	const char *device = NULL;
 	const char *fallback_name = NULL;
-	int handle_underrun = 0;
+	int handle_underrun = 2;
 	int err;
 	snd_pcm_pulse_t *pcm;
 
-- 
1.7.4.1


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



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been written
  2011-08-18 15:06 [PATCH] alsa-plugins: Pulse: only underrun if no more data has been written David Henningsson
@ 2011-08-19  7:43 ` Takashi Iwai
  2011-08-19  9:49   ` David Henningsson
  0 siblings, 1 reply; 14+ messages in thread
From: Takashi Iwai @ 2011-08-19  7:43 UTC (permalink / raw)
  To: David Henningsson; +Cc: ALSA Development Mailing List

At Thu, 18 Aug 2011 17:06:15 +0200,
David Henningsson wrote:
> 
> With the new/upcoming version of PulseAudio (0.99.x) there is a protocol 
> addition that makes it possible to handle underruns better in the pulse 
> plugin.
> 
> The attached patch implements that, but it has two flaws I wouldn't mind 
> getting some help with if possible:
> 
> 1) Since this uses the new API function pa_stream_get_underflow_index, 
> it won't compile with current stable PulseAudio versions, only the 
> upcoming version.

Any way (like ifdef some constant or a version number) to detect in
build time, or do we need to check it in configure script?

> 2) So now there are three possibilities for handle_underrun ( 0 = never, 
> 1 = always, 2 = the new improved one that IMO should be used). Since 
> handle_underrun is a bool in the config, it can not be set to "2", only 
> 0 or 1.

Changing the value syntax doesn't sound good.
IMO, better to add another option to enable/disable the advanced
underrun handling.


thanks,

Takashi


> -- 
> David Henningsson, Canonical Ltd.
> http://launchpad.net/~diwic
> [2 0001-alsa-plugins-Pulse-only-underrun-if-no-more-data-has.patch <text/x-patch (7bit)>]
> >From 4e31ba395b751a6ab3254256dc8a227b3be67932 Mon Sep 17 00:00:00 2001
> From: David Henningsson <david.henningsson@canonical.com>
> Date: Tue, 2 Aug 2011 14:49:04 +0200
> Subject: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been written
> 
> If more data has already been written after the underrun, the underrun
> will automatically end and therefore we should not report it or
> restart the stream.
> 
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
>  pulse/pcm_pulse.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c
> index d6c6792..9c4a7e5 100644
> --- a/pulse/pcm_pulse.c
> +++ b/pulse/pcm_pulse.c
> @@ -39,9 +39,10 @@ typedef struct snd_pcm_pulse {
>  	size_t last_size;
>  	size_t ptr;
>  	int underrun;
> -	int handle_underrun;
> +	int handle_underrun; /* can be 0=never, 1=always or 2=only if more data has not been written */
>  
>  	size_t offset;
> +	int64_t written;
>  
>  	pa_stream *stream;
>  
> @@ -459,6 +460,7 @@ static snd_pcm_sframes_t pulse_write(snd_pcm_ioplug_t * io,
>  	}
>  
>  	/* Make sure the buffer pointer is in sync */
> +	pcm->written += writebytes;
>  	pcm->last_size -= writebytes;
>  	ret = update_ptr(pcm);
>  	if (ret < 0)
> @@ -594,7 +596,8 @@ static void stream_underrun_cb(pa_stream * p, void *userdata)
>  	if (!pcm->p)
>  		return;
>  
> -	pcm->underrun = 1;
> +	if (pcm->handle_underrun == 1 || pcm->written <= pa_stream_get_underflow_index(p))
> +		pcm->underrun = 1;
>  }
>  
>  static void stream_latency_cb(pa_stream *p, void *userdata) {
> @@ -691,6 +694,7 @@ static int pulse_prepare(snd_pcm_ioplug_t * io)
>  		goto finish;
>  	}
>  
> +	pcm->written = 0;
>  	pa_stream_set_state_callback(pcm->stream, stream_state_cb, pcm);
>  	pa_stream_set_latency_update_callback(pcm->stream, stream_latency_cb, pcm);
>  
> @@ -983,7 +987,7 @@ SND_PCM_PLUGIN_DEFINE_FUNC(pulse)
>  	const char *server = NULL;
>  	const char *device = NULL;
>  	const char *fallback_name = NULL;
> -	int handle_underrun = 0;
> +	int handle_underrun = 2;
>  	int err;
>  	snd_pcm_pulse_t *pcm;
>  
> -- 
> 1.7.4.1
> 
> [3  <text/plain; us-ascii (7bit)>]
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been written
  2011-08-19  7:43 ` Takashi Iwai
@ 2011-08-19  9:49   ` David Henningsson
  2011-08-19 12:46     ` Takashi Iwai
  0 siblings, 1 reply; 14+ messages in thread
From: David Henningsson @ 2011-08-19  9:49 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: ALSA Development Mailing List

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

On 08/19/2011 09:43 AM, Takashi Iwai wrote:
> At Thu, 18 Aug 2011 17:06:15 +0200,
> David Henningsson wrote:
>>
>> With the new/upcoming version of PulseAudio (0.99.x) there is a protocol
>> addition that makes it possible to handle underruns better in the pulse
>> plugin.
>>
>> The attached patch implements that, but it has two flaws I wouldn't mind
>> getting some help with if possible:
>>
>> 1) Since this uses the new API function pa_stream_get_underflow_index,
>> it won't compile with current stable PulseAudio versions, only the
>> upcoming version.
>
> Any way (like ifdef some constant or a version number) to detect in
> build time, or do we need to check it in configure script?

I don't mess around with autotools that often, but I think I got it 
working (see attached patch)

>
>> 2) So now there are three possibilities for handle_underrun ( 0 = never,
>> 1 = always, 2 = the new improved one that IMO should be used). Since
>> handle_underrun is a bool in the config, it can not be set to "2", only
>> 0 or 1.
>
> Changing the value syntax doesn't sound good.
> IMO, better to add another option to enable/disable the advanced
> underrun handling.

Ok, I have now done so in the attached patch.

>
>
> thanks,
>
> Takashi
>
>
>> --
>> David Henningsson, Canonical Ltd.
>> http://launchpad.net/~diwic
>> [2 0001-alsa-plugins-Pulse-only-underrun-if-no-more-data-has.patch<text/x-patch (7bit)>]
>> > From 4e31ba395b751a6ab3254256dc8a227b3be67932 Mon Sep 17 00:00:00 2001
>> From: David Henningsson<david.henningsson@canonical.com>
>> Date: Tue, 2 Aug 2011 14:49:04 +0200
>> Subject: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been written
>>
>> If more data has already been written after the underrun, the underrun
>> will automatically end and therefore we should not report it or
>> restart the stream.
>>
>> Signed-off-by: David Henningsson<david.henningsson@canonical.com>
>> ---
>>   pulse/pcm_pulse.c |   10 +++++++---
>>   1 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c
>> index d6c6792..9c4a7e5 100644
>> --- a/pulse/pcm_pulse.c
>> +++ b/pulse/pcm_pulse.c
>> @@ -39,9 +39,10 @@ typedef struct snd_pcm_pulse {
>>   	size_t last_size;
>>   	size_t ptr;
>>   	int underrun;
>> -	int handle_underrun;
>> +	int handle_underrun; /* can be 0=never, 1=always or 2=only if more data has not been written */
>>
>>   	size_t offset;
>> +	int64_t written;
>>
>>   	pa_stream *stream;
>>
>> @@ -459,6 +460,7 @@ static snd_pcm_sframes_t pulse_write(snd_pcm_ioplug_t * io,
>>   	}
>>
>>   	/* Make sure the buffer pointer is in sync */
>> +	pcm->written += writebytes;
>>   	pcm->last_size -= writebytes;
>>   	ret = update_ptr(pcm);
>>   	if (ret<  0)
>> @@ -594,7 +596,8 @@ static void stream_underrun_cb(pa_stream * p, void *userdata)
>>   	if (!pcm->p)
>>   		return;
>>
>> -	pcm->underrun = 1;
>> +	if (pcm->handle_underrun == 1 || pcm->written<= pa_stream_get_underflow_index(p))
>> +		pcm->underrun = 1;
>>   }
>>
>>   static void stream_latency_cb(pa_stream *p, void *userdata) {
>> @@ -691,6 +694,7 @@ static int pulse_prepare(snd_pcm_ioplug_t * io)
>>   		goto finish;
>>   	}
>>
>> +	pcm->written = 0;
>>   	pa_stream_set_state_callback(pcm->stream, stream_state_cb, pcm);
>>   	pa_stream_set_latency_update_callback(pcm->stream, stream_latency_cb, pcm);
>>
>> @@ -983,7 +987,7 @@ SND_PCM_PLUGIN_DEFINE_FUNC(pulse)
>>   	const char *server = NULL;
>>   	const char *device = NULL;
>>   	const char *fallback_name = NULL;
>> -	int handle_underrun = 0;
>> +	int handle_underrun = 2;
>>   	int err;
>>   	snd_pcm_pulse_t *pcm;
>>
>> --
>> 1.7.4.1
>>
>> [3<text/plain; us-ascii (7bit)>]
>> _______________________________________________
>> Alsa-devel mailing list
>> Alsa-devel@alsa-project.org
>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>



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

[-- Attachment #2: 0001-alsa-plugins-Pulse-only-underrun-if-no-more-data-has.patch --]
[-- Type: text/x-patch, Size: 3966 bytes --]

>From 8098c77a447a80d2860588387d18b3aa6d23b6bb Mon Sep 17 00:00:00 2001
From: David Henningsson <david.henningsson@canonical.com>
Date: Fri, 19 Aug 2011 11:47:07 +0200
Subject: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been
 written

If more data has already been written after the underrun, the underrun
will automatically end and therefore we should not report it or
restart the stream.

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 configure.in      |    7 +++++++
 pulse/pcm_pulse.c |   28 ++++++++++++++++++++++++++--
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/configure.in b/configure.in
index ccf59ba..f6886b1 100644
--- a/configure.in
+++ b/configure.in
@@ -31,8 +31,14 @@ AC_ARG_ENABLE([pulseaudio],
 
 if test "x$enable_pulseaudio" != "xno"; then
   PKG_CHECK_MODULES(pulseaudio, [libpulse >= 0.9.11], [HAVE_PULSE=yes], [HAVE_PULSE=no])
+  PKG_CHECK_MODULES(pulseaudio_099, [libpulse >= 0.99.1], 
+    [HAVE_PULSE_UNDERRUN_INDEX=yes], [HAVE_PULSE_UNDERRUN_INDEX=no])
+  
 fi
 AM_CONDITIONAL(HAVE_PULSE, test x$HAVE_PULSE = xyes)
+if test "$HAVE_PULSE_UNDERRUN_INDEX" = "yes"; then
+  AC_DEFINE([HAVE_PULSE_UNDERRUN_INDEX], 1, Pulseaudio underrun index available)
+fi 
 
 AC_ARG_ENABLE([samplerate],
       AS_HELP_STRING([--disable-samplerate], [Disable building of samplerate plugin]))
@@ -185,6 +191,7 @@ echo "Pulseaudio plugin:  $HAVE_PULSE"
 if test "$HAVE_PULSE" = "yes"; then
   echo "  pulseaudio_CFLAGS: $pulseaudio_CFLAGS"
   echo "  pulseaudio_LIBS: $pulseaudio_LIBS"
+  echo "  pulseaudio underrun index: $HAVE_PULSE_UNDERRUN_INDEX" 
 fi
 echo "Samplerate plugin:  $HAVE_SAMPLERATE"
 if test "$HAVE_SAMPLERATE" = "yes"; then
diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c
index d6c6792..61f1c0c 100644
--- a/pulse/pcm_pulse.c
+++ b/pulse/pcm_pulse.c
@@ -26,6 +26,10 @@
 #include <alsa/asoundlib.h>
 #include <alsa/pcm_external.h>
 
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
 #include "pulse.h"
 
 typedef struct snd_pcm_pulse {
@@ -39,9 +43,10 @@ typedef struct snd_pcm_pulse {
 	size_t last_size;
 	size_t ptr;
 	int underrun;
-	int handle_underrun;
+	int handle_underrun; /* can be 0=never, 1=always or 2,3=only if more data has not been written */
 
 	size_t offset;
+	int64_t written;
 
 	pa_stream *stream;
 
@@ -459,6 +464,7 @@ static snd_pcm_sframes_t pulse_write(snd_pcm_ioplug_t * io,
 	}
 
 	/* Make sure the buffer pointer is in sync */
+	pcm->written += writebytes;
 	pcm->last_size -= writebytes;
 	ret = update_ptr(pcm);
 	if (ret < 0)
@@ -594,7 +600,12 @@ static void stream_underrun_cb(pa_stream * p, void *userdata)
 	if (!pcm->p)
 		return;
 
-	pcm->underrun = 1;
+	if (pcm->handle_underrun == 1
+#ifdef HAVE_PULSE_UNDERRUN_INDEX
+		|| pcm->written <= pa_stream_get_underflow_index(p)
+#endif
+	)
+		pcm->underrun = 1;
 }
 
 static void stream_latency_cb(pa_stream *p, void *userdata) {
@@ -691,6 +702,7 @@ static int pulse_prepare(snd_pcm_ioplug_t * io)
 		goto finish;
 	}
 
+	pcm->written = 0;
 	pa_stream_set_state_callback(pcm->stream, stream_state_cb, pcm);
 	pa_stream_set_latency_update_callback(pcm->stream, stream_latency_cb, pcm);
 
@@ -983,7 +995,11 @@ SND_PCM_PLUGIN_DEFINE_FUNC(pulse)
 	const char *server = NULL;
 	const char *device = NULL;
 	const char *fallback_name = NULL;
+#ifdef HAVE_PULSE_UNDERRUN_INDEX
+	int handle_underrun = 2;
+#else
 	int handle_underrun = 0;
+#endif
 	int err;
 	snd_pcm_pulse_t *pcm;
 
@@ -1017,6 +1033,14 @@ SND_PCM_PLUGIN_DEFINE_FUNC(pulse)
 			handle_underrun = err;
 			continue;
 		}
+		if (strcmp(id, "handle_underrun_detect") == 0) {
+			if ((err = snd_config_get_bool(n)) < 0) {
+				SNDERR("Invalid value for %s", id);
+				return -EINVAL;
+			}
+			handle_underrun = err ? handle_underrun | 2 : handle_underrun & ~2;
+			continue;
+		}
 		if (strcmp(id, "fallback") == 0) {
 			if (snd_config_get_string(n, &fallback_name) < 0) {
 				SNDERR("Invalid value for %s", id);
-- 
1.7.5.4


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



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been written
  2011-08-19  9:49   ` David Henningsson
@ 2011-08-19 12:46     ` Takashi Iwai
  2011-08-23 11:56       ` David Henningsson
  0 siblings, 1 reply; 14+ messages in thread
From: Takashi Iwai @ 2011-08-19 12:46 UTC (permalink / raw)
  To: David Henningsson; +Cc: ALSA Development Mailing List

At Fri, 19 Aug 2011 11:49:25 +0200,
David Henningsson wrote:
> 
> >From 8098c77a447a80d2860588387d18b3aa6d23b6bb Mon Sep 17 00:00:00 2001
> From: David Henningsson <david.henningsson@canonical.com>
> Date: Fri, 19 Aug 2011 11:47:07 +0200
> Subject: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been
>  written
> 
> If more data has already been written after the underrun, the underrun
> will automatically end and therefore we should not report it or
> restart the stream.
> 
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
>  configure.in      |    7 +++++++
>  pulse/pcm_pulse.c |   28 ++++++++++++++++++++++++++--
>  2 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/configure.in b/configure.in
> index ccf59ba..f6886b1 100644
> --- a/configure.in
> +++ b/configure.in
> @@ -31,8 +31,14 @@ AC_ARG_ENABLE([pulseaudio],
>  
>  if test "x$enable_pulseaudio" != "xno"; then
>    PKG_CHECK_MODULES(pulseaudio, [libpulse >= 0.9.11], [HAVE_PULSE=yes], [HAVE_PULSE=no])
> +  PKG_CHECK_MODULES(pulseaudio_099, [libpulse >= 0.99.1], 
> +    [HAVE_PULSE_UNDERRUN_INDEX=yes], [HAVE_PULSE_UNDERRUN_INDEX=no])

Hm, can we use PA_CHECK_VERSION() or such simply in the code?

> diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c
> index d6c6792..61f1c0c 100644
> --- a/pulse/pcm_pulse.c
> +++ b/pulse/pcm_pulse.c
> @@ -26,6 +26,10 @@
>  #include <alsa/asoundlib.h>
>  #include <alsa/pcm_external.h>
>  
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
>  #include "pulse.h"
>  
>  typedef struct snd_pcm_pulse {
> @@ -39,9 +43,10 @@ typedef struct snd_pcm_pulse {
>  	size_t last_size;
>  	size_t ptr;
>  	int underrun;
> -	int handle_underrun;
> +	int handle_underrun; /* can be 0=never, 1=always or 2,3=only if more data has not been written */

I think it'd be simpler to introduce one more boolean variable.
Otherwise the logic looks too complex than needed.

> @@ -594,7 +600,12 @@ static void stream_underrun_cb(pa_stream * p, void *userdata)
>  	if (!pcm->p)
>  		return;
>  
> -	pcm->underrun = 1;
> +	if (pcm->handle_underrun == 1
> +#ifdef HAVE_PULSE_UNDERRUN_INDEX
> +		|| pcm->written <= pa_stream_get_underflow_index(p)
> +#endif
> +	)
> +		pcm->underrun = 1;

A nicer way is to define a macro such as

#ifdef HAVE_PULSE_UNDERRUN_INDEX
static inline int handle_underrun_detect(snd_pcm_pulse_t *pcm, pa_stream *p)
{
	return !pcm->handle_underrun_detect ||
		pcm->written <= pa_stream_get_underflow_index(p);
}
#else
#define handle_underrun_detect(pcm, p)	1
#endif

Then

	if (pcm->handle_underrun && handle_underrun_detect(pcm, p))
		pcm->underrun = 1;


thanks,

Takashi

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been written
  2011-08-19 12:46     ` Takashi Iwai
@ 2011-08-23 11:56       ` David Henningsson
  2011-08-23 13:40         ` Takashi Iwai
  0 siblings, 1 reply; 14+ messages in thread
From: David Henningsson @ 2011-08-23 11:56 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: ALSA Development Mailing List

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

Behold the third version of the patch.

On 08/19/2011 02:46 PM, Takashi Iwai wrote:
> At Fri, 19 Aug 2011 11:49:25 +0200,
> David Henningsson wrote:
>>
>> > From 8098c77a447a80d2860588387d18b3aa6d23b6bb Mon Sep 17 00:00:00 2001
>> From: David Henningsson<david.henningsson@canonical.com>
>> Date: Fri, 19 Aug 2011 11:47:07 +0200
>> Subject: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been
>>   written
>>
>> If more data has already been written after the underrun, the underrun
>> will automatically end and therefore we should not report it or
>> restart the stream.
>>
>> Signed-off-by: David Henningsson<david.henningsson@canonical.com>
>> ---
>>   configure.in      |    7 +++++++
>>   pulse/pcm_pulse.c |   28 ++++++++++++++++++++++++++--
>>   2 files changed, 33 insertions(+), 2 deletions(-)
>>
>> diff --git a/configure.in b/configure.in
>> index ccf59ba..f6886b1 100644
>> --- a/configure.in
>> +++ b/configure.in
>> @@ -31,8 +31,14 @@ AC_ARG_ENABLE([pulseaudio],
>>
>>   if test "x$enable_pulseaudio" != "xno"; then
>>     PKG_CHECK_MODULES(pulseaudio, [libpulse>= 0.9.11], [HAVE_PULSE=yes], [HAVE_PULSE=no])
>> +  PKG_CHECK_MODULES(pulseaudio_099, [libpulse>= 0.99.1],
>> +    [HAVE_PULSE_UNDERRUN_INDEX=yes], [HAVE_PULSE_UNDERRUN_INDEX=no])
>
> Hm, can we use PA_CHECK_VERSION() or such simply in the code?

Ok, good idea.

>
>> diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c
>> index d6c6792..61f1c0c 100644
>> --- a/pulse/pcm_pulse.c
>> +++ b/pulse/pcm_pulse.c
>> @@ -26,6 +26,10 @@
>>   #include<alsa/asoundlib.h>
>>   #include<alsa/pcm_external.h>
>>
>> +#ifdef HAVE_CONFIG_H
>> +#include<config.h>
>> +#endif
>> +
>>   #include "pulse.h"
>>
>>   typedef struct snd_pcm_pulse {
>> @@ -39,9 +43,10 @@ typedef struct snd_pcm_pulse {
>>   	size_t last_size;
>>   	size_t ptr;
>>   	int underrun;
>> -	int handle_underrun;
>> +	int handle_underrun; /* can be 0=never, 1=always or 2,3=only if more data has not been written */
>
> I think it'd be simpler to introduce one more boolean variable.
> Otherwise the logic looks too complex than needed.

I've now done that. I don't know if it became simpler though.

>
>> @@ -594,7 +600,12 @@ static void stream_underrun_cb(pa_stream * p, void *userdata)
>>   	if (!pcm->p)
>>   		return;
>>
>> -	pcm->underrun = 1;
>> +	if (pcm->handle_underrun == 1
>> +#ifdef HAVE_PULSE_UNDERRUN_INDEX
>> +		|| pcm->written<= pa_stream_get_underflow_index(p)
>> +#endif
>> +	)
>> +		pcm->underrun = 1;
>
> A nicer way is to define a macro such as
>
> #ifdef HAVE_PULSE_UNDERRUN_INDEX
> static inline int handle_underrun_detect(snd_pcm_pulse_t *pcm, pa_stream *p)
> {
> 	return !pcm->handle_underrun_detect ||
> 		pcm->written<= pa_stream_get_underflow_index(p);
> }
> #else
> #define handle_underrun_detect(pcm, p)	1
> #endif
>
> Then
>
> 	if (pcm->handle_underrun&&  handle_underrun_detect(pcm, p))
> 		pcm->underrun = 1;

Ok, works for me.

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

[-- Attachment #2: 0001-alsa-plugins-Pulse-only-underrun-if-no-more-data-has.patch --]
[-- Type: text/x-patch, Size: 3597 bytes --]

>From a57884821509b373ef3acd82f73b719d8d2f6e23 Mon Sep 17 00:00:00 2001
From: David Henningsson <david.henningsson@canonical.com>
Date: Tue, 23 Aug 2011 13:42:33 +0200
Subject: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been
 written

If more data has already been written after the underrun, the underrun
will automatically end and therefore we should not report it or
restart the stream.

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 pulse/pcm_pulse.c |   36 ++++++++++++++++++++++++++++++++++--
 1 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c
index d6c6792..92f4777 100644
--- a/pulse/pcm_pulse.c
+++ b/pulse/pcm_pulse.c
@@ -40,8 +40,10 @@ typedef struct snd_pcm_pulse {
 	size_t ptr;
 	int underrun;
 	int handle_underrun;
+	int handle_underrun_detect;
 
 	size_t offset;
+	int64_t written;
 
 	pa_stream *stream;
 
@@ -460,6 +462,7 @@ static snd_pcm_sframes_t pulse_write(snd_pcm_ioplug_t * io,
 
 	/* Make sure the buffer pointer is in sync */
 	pcm->last_size -= writebytes;
+	pcm->written += writebytes;
 	ret = update_ptr(pcm);
 	if (ret < 0)
 		goto finish;
@@ -585,6 +588,23 @@ static void stream_request_cb(pa_stream * p, size_t length, void *userdata)
 	update_active(pcm);
 }
 
+#if defined(PA_CHECK_VERSION) && PA_CHECK_VERSION(0,99,0)
+
+#define has_underrun_detect 1
+
+static inline int do_underrun_detect(snd_pcm_pulse_t *pcm, pa_stream *p)
+{
+	return pcm->handle_underrun_detect &&
+		pcm->written <= pa_stream_get_underflow_index(p);
+}
+
+#else
+
+#define has_underrun_detect 0
+#define do_underrun_detect(pcm, p) 0
+
+#endif
+
 static void stream_underrun_cb(pa_stream * p, void *userdata)
 {
 	snd_pcm_pulse_t *pcm = userdata;
@@ -594,7 +614,8 @@ static void stream_underrun_cb(pa_stream * p, void *userdata)
 	if (!pcm->p)
 		return;
 
-	pcm->underrun = 1;
+	if (pcm->handle_underrun || do_underrun_detect(pcm, p))
+		pcm->underrun = 1;
 }
 
 static void stream_latency_cb(pa_stream *p, void *userdata) {
@@ -697,7 +718,7 @@ static int pulse_prepare(snd_pcm_ioplug_t * io)
 	if (io->stream == SND_PCM_STREAM_PLAYBACK) {
 		pa_stream_set_write_callback(pcm->stream,
 					     stream_request_cb, pcm);
-		if (pcm->handle_underrun)
+		if (pcm->handle_underrun_detect || pcm->handle_underrun)
 			pa_stream_set_underflow_callback(pcm->stream,
 							 stream_underrun_cb, pcm);
 		r = pa_stream_connect_playback(pcm->stream, pcm->device,
@@ -739,6 +760,7 @@ static int pulse_prepare(snd_pcm_ioplug_t * io)
 
 	pcm->offset = 0;
 	pcm->underrun = 0;
+	pcm->written = 0;
 
 	/* Reset fake ringbuffer */
 	pcm->last_size = 0;
@@ -984,6 +1006,7 @@ SND_PCM_PLUGIN_DEFINE_FUNC(pulse)
 	const char *device = NULL;
 	const char *fallback_name = NULL;
 	int handle_underrun = 0;
+	int handle_underrun_detect = 1;
 	int err;
 	snd_pcm_pulse_t *pcm;
 
@@ -1017,6 +1040,14 @@ SND_PCM_PLUGIN_DEFINE_FUNC(pulse)
 			handle_underrun = err;
 			continue;
 		}
+		if (strcmp(id, "handle_underrun_detect") == 0) {
+			if ((err = snd_config_get_bool(n)) < 0) {
+				SNDERR("Invalid value for %s", id);
+				return -EINVAL;
+			}
+			handle_underrun_detect = err;
+			continue;
+		}
 		if (strcmp(id, "fallback") == 0) {
 			if (snd_config_get_string(n, &fallback_name) < 0) {
 				SNDERR("Invalid value for %s", id);
@@ -1051,6 +1082,7 @@ SND_PCM_PLUGIN_DEFINE_FUNC(pulse)
 	}
 
 	pcm->handle_underrun = handle_underrun;
+	pcm->handle_underrun_detect = has_underrun_detect && handle_underrun_detect;
 
 	err = pulse_connect(pcm->p, server, fallback_name != NULL);
 	if (err < 0)
-- 
1.7.5.4


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



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been written
  2011-08-23 11:56       ` David Henningsson
@ 2011-08-23 13:40         ` Takashi Iwai
  2011-08-23 13:57           ` David Henningsson
  2011-08-25  5:53           ` Raymond Yau
  0 siblings, 2 replies; 14+ messages in thread
From: Takashi Iwai @ 2011-08-23 13:40 UTC (permalink / raw)
  To: David Henningsson; +Cc: ALSA Development Mailing List

At Tue, 23 Aug 2011 13:56:58 +0200,
David Henningsson wrote:
> 
> Behold the third version of the patch.
> 
> On 08/19/2011 02:46 PM, Takashi Iwai wrote:
> > At Fri, 19 Aug 2011 11:49:25 +0200,
> > David Henningsson wrote:
> >>
> >> > From 8098c77a447a80d2860588387d18b3aa6d23b6bb Mon Sep 17 00:00:00 2001
> >> From: David Henningsson<david.henningsson@canonical.com>
> >> Date: Fri, 19 Aug 2011 11:47:07 +0200
> >> Subject: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been
> >>   written
> >>
> >> If more data has already been written after the underrun, the underrun
> >> will automatically end and therefore we should not report it or
> >> restart the stream.
> >>
> >> Signed-off-by: David Henningsson<david.henningsson@canonical.com>
> >> ---
> >>   configure.in      |    7 +++++++
> >>   pulse/pcm_pulse.c |   28 ++++++++++++++++++++++++++--
> >>   2 files changed, 33 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/configure.in b/configure.in
> >> index ccf59ba..f6886b1 100644
> >> --- a/configure.in
> >> +++ b/configure.in
> >> @@ -31,8 +31,14 @@ AC_ARG_ENABLE([pulseaudio],
> >>
> >>   if test "x$enable_pulseaudio" != "xno"; then
> >>     PKG_CHECK_MODULES(pulseaudio, [libpulse>= 0.9.11], [HAVE_PULSE=yes], [HAVE_PULSE=no])
> >> +  PKG_CHECK_MODULES(pulseaudio_099, [libpulse>= 0.99.1],
> >> +    [HAVE_PULSE_UNDERRUN_INDEX=yes], [HAVE_PULSE_UNDERRUN_INDEX=no])
> >
> > Hm, can we use PA_CHECK_VERSION() or such simply in the code?
> 
> Ok, good idea.
> 
> >
> >> diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c
> >> index d6c6792..61f1c0c 100644
> >> --- a/pulse/pcm_pulse.c
> >> +++ b/pulse/pcm_pulse.c
> >> @@ -26,6 +26,10 @@
> >>   #include<alsa/asoundlib.h>
> >>   #include<alsa/pcm_external.h>
> >>
> >> +#ifdef HAVE_CONFIG_H
> >> +#include<config.h>
> >> +#endif
> >> +
> >>   #include "pulse.h"
> >>
> >>   typedef struct snd_pcm_pulse {
> >> @@ -39,9 +43,10 @@ typedef struct snd_pcm_pulse {
> >>   	size_t last_size;
> >>   	size_t ptr;
> >>   	int underrun;
> >> -	int handle_underrun;
> >> +	int handle_underrun; /* can be 0=never, 1=always or 2,3=only if more data has not been written */
> >
> > I think it'd be simpler to introduce one more boolean variable.
> > Otherwise the logic looks too complex than needed.
> 
> I've now done that. I don't know if it became simpler though.

Hm, indeed.

I think we can start off from more simple.  With the new PA, the
underrun detection should work better.  So why we do we need yet
another mode to disable it?  It should be enough just to have a
boolean for underrun_detect yes/no.

So, the patch would be like below.

What do you think?


Takashi

---
diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c
index d6c6792..b0e52ab 100644
--- a/pulse/pcm_pulse.c
+++ b/pulse/pcm_pulse.c
@@ -42,6 +42,7 @@ typedef struct snd_pcm_pulse {
 	int handle_underrun;
 
 	size_t offset;
+	int64_t written;
 
 	pa_stream *stream;
 
@@ -460,6 +461,7 @@ static snd_pcm_sframes_t pulse_write(snd_pcm_ioplug_t * io,
 
 	/* Make sure the buffer pointer is in sync */
 	pcm->last_size -= writebytes;
+	pcm->written += writebytes;
 	ret = update_ptr(pcm);
 	if (ret < 0)
 		goto finish;
@@ -585,6 +587,15 @@ static void stream_request_cb(pa_stream * p, size_t length, void *userdata)
 	update_active(pcm);
 }
 
+#if defined(PA_CHECK_VERSION) && PA_CHECK_VERSION(0,99,0)
+#define DEFAULT_HANDLE_UNDERRUN		1
+#define do_underrun_detect(pcm, p) \
+	((pcm)->written <= pa_stream_get_underflow_index(p))
+#else
+#define DEFAULT_HANDLE_UNDERRUN		0
+#define do_underrun_detect(pcm, p)	1	/* always true */
+#endif
+
 static void stream_underrun_cb(pa_stream * p, void *userdata)
 {
 	snd_pcm_pulse_t *pcm = userdata;
@@ -594,7 +605,8 @@ static void stream_underrun_cb(pa_stream * p, void *userdata)
 	if (!pcm->p)
 		return;
 
-	pcm->underrun = 1;
+	if (do_underrun_detect(pcm, p))
+		pcm->underrun = 1;
 }
 
 static void stream_latency_cb(pa_stream *p, void *userdata) {
@@ -739,6 +751,7 @@ static int pulse_prepare(snd_pcm_ioplug_t * io)
 
 	pcm->offset = 0;
 	pcm->underrun = 0;
+	pcm->written = 0;
 
 	/* Reset fake ringbuffer */
 	pcm->last_size = 0;
@@ -983,7 +996,7 @@ SND_PCM_PLUGIN_DEFINE_FUNC(pulse)
 	const char *server = NULL;
 	const char *device = NULL;
 	const char *fallback_name = NULL;
-	int handle_underrun = 0;
+	int handle_underrun = DEFAULT_HANDLE_UNDERRUN;
 	int err;
 	snd_pcm_pulse_t *pcm;
 

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been written
  2011-08-23 13:40         ` Takashi Iwai
@ 2011-08-23 13:57           ` David Henningsson
  2011-08-23 15:07             ` Takashi Iwai
  2011-08-25  5:53           ` Raymond Yau
  1 sibling, 1 reply; 14+ messages in thread
From: David Henningsson @ 2011-08-23 13:57 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: ALSA Development Mailing List

On 2011-08-23 15:40, Takashi Iwai wrote:
> At Tue, 23 Aug 2011 13:56:58 +0200,
> David Henningsson wrote:
>>
>> Behold the third version of the patch.
>>
>> On 08/19/2011 02:46 PM, Takashi Iwai wrote:
>>> At Fri, 19 Aug 2011 11:49:25 +0200,
>>> David Henningsson wrote:
>>>>
>>>>>  From 8098c77a447a80d2860588387d18b3aa6d23b6bb Mon Sep 17 00:00:00 2001
>>>> From: David Henningsson<david.henningsson@canonical.com>
>>>> Date: Fri, 19 Aug 2011 11:47:07 +0200
>>>> Subject: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been
>>>>    written
>>>>
>>>> If more data has already been written after the underrun, the underrun
>>>> will automatically end and therefore we should not report it or
>>>> restart the stream.
>>>>
>>>> Signed-off-by: David Henningsson<david.henningsson@canonical.com>
>>>> ---
>>>>    configure.in      |    7 +++++++
>>>>    pulse/pcm_pulse.c |   28 ++++++++++++++++++++++++++--
>>>>    2 files changed, 33 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/configure.in b/configure.in
>>>> index ccf59ba..f6886b1 100644
>>>> --- a/configure.in
>>>> +++ b/configure.in
>>>> @@ -31,8 +31,14 @@ AC_ARG_ENABLE([pulseaudio],
>>>>
>>>>    if test "x$enable_pulseaudio" != "xno"; then
>>>>      PKG_CHECK_MODULES(pulseaudio, [libpulse>= 0.9.11], [HAVE_PULSE=yes], [HAVE_PULSE=no])
>>>> +  PKG_CHECK_MODULES(pulseaudio_099, [libpulse>= 0.99.1],
>>>> +    [HAVE_PULSE_UNDERRUN_INDEX=yes], [HAVE_PULSE_UNDERRUN_INDEX=no])
>>>
>>> Hm, can we use PA_CHECK_VERSION() or such simply in the code?
>>
>> Ok, good idea.
>>
>>>
>>>> diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c
>>>> index d6c6792..61f1c0c 100644
>>>> --- a/pulse/pcm_pulse.c
>>>> +++ b/pulse/pcm_pulse.c
>>>> @@ -26,6 +26,10 @@
>>>>    #include<alsa/asoundlib.h>
>>>>    #include<alsa/pcm_external.h>
>>>>
>>>> +#ifdef HAVE_CONFIG_H
>>>> +#include<config.h>
>>>> +#endif
>>>> +
>>>>    #include "pulse.h"
>>>>
>>>>    typedef struct snd_pcm_pulse {
>>>> @@ -39,9 +43,10 @@ typedef struct snd_pcm_pulse {
>>>>    	size_t last_size;
>>>>    	size_t ptr;
>>>>    	int underrun;
>>>> -	int handle_underrun;
>>>> +	int handle_underrun; /* can be 0=never, 1=always or 2,3=only if more data has not been written */
>>>
>>> I think it'd be simpler to introduce one more boolean variable.
>>> Otherwise the logic looks too complex than needed.
>>
>> I've now done that. I don't know if it became simpler though.
>
> Hm, indeed.
>
> I think we can start off from more simple.  With the new PA, the
> underrun detection should work better.  So why we do we need yet
> another mode to disable it?  It should be enough just to have a
> boolean for underrun_detect yes/no.
>
> So, the patch would be like below.
>
> What do you think?

Sure. For me, I'd go with the new functionality without ever configuring 
anything else. So I'm happy to exchange "always underrun" for the new 
underrun detection or even to remove the configuration functionality 
entirely.

>
>
> Takashi
>
> ---
> diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c
> index d6c6792..b0e52ab 100644
> --- a/pulse/pcm_pulse.c
> +++ b/pulse/pcm_pulse.c
> @@ -42,6 +42,7 @@ typedef struct snd_pcm_pulse {
>   	int handle_underrun;
>
>   	size_t offset;
> +	int64_t written;
>
>   	pa_stream *stream;
>
> @@ -460,6 +461,7 @@ static snd_pcm_sframes_t pulse_write(snd_pcm_ioplug_t * io,
>
>   	/* Make sure the buffer pointer is in sync */
>   	pcm->last_size -= writebytes;
> +	pcm->written += writebytes;
>   	ret = update_ptr(pcm);
>   	if (ret<  0)
>   		goto finish;
> @@ -585,6 +587,15 @@ static void stream_request_cb(pa_stream * p, size_t length, void *userdata)
>   	update_active(pcm);
>   }
>
> +#if defined(PA_CHECK_VERSION)&&  PA_CHECK_VERSION(0,99,0)
> +#define DEFAULT_HANDLE_UNDERRUN		1
> +#define do_underrun_detect(pcm, p) \
> +	((pcm)->written<= pa_stream_get_underflow_index(p))
> +#else
> +#define DEFAULT_HANDLE_UNDERRUN		0
> +#define do_underrun_detect(pcm, p)	1	/* always true */
> +#endif
> +
>   static void stream_underrun_cb(pa_stream * p, void *userdata)
>   {
>   	snd_pcm_pulse_t *pcm = userdata;
> @@ -594,7 +605,8 @@ static void stream_underrun_cb(pa_stream * p, void *userdata)
>   	if (!pcm->p)
>   		return;
>
> -	pcm->underrun = 1;
> +	if (do_underrun_detect(pcm, p))
> +		pcm->underrun = 1;
>   }
>
>   static void stream_latency_cb(pa_stream *p, void *userdata) {
> @@ -739,6 +751,7 @@ static int pulse_prepare(snd_pcm_ioplug_t * io)
>
>   	pcm->offset = 0;
>   	pcm->underrun = 0;
> +	pcm->written = 0;
>
>   	/* Reset fake ringbuffer */
>   	pcm->last_size = 0;
> @@ -983,7 +996,7 @@ SND_PCM_PLUGIN_DEFINE_FUNC(pulse)
>   	const char *server = NULL;
>   	const char *device = NULL;
>   	const char *fallback_name = NULL;
> -	int handle_underrun = 0;
> +	int handle_underrun = DEFAULT_HANDLE_UNDERRUN;
>   	int err;
>   	snd_pcm_pulse_t *pcm;
>
>


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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been written
  2011-08-23 13:57           ` David Henningsson
@ 2011-08-23 15:07             ` Takashi Iwai
  0 siblings, 0 replies; 14+ messages in thread
From: Takashi Iwai @ 2011-08-23 15:07 UTC (permalink / raw)
  To: David Henningsson; +Cc: ALSA Development Mailing List

At Tue, 23 Aug 2011 15:57:05 +0200,
David Henningsson wrote:
> 
> On 2011-08-23 15:40, Takashi Iwai wrote:
> > At Tue, 23 Aug 2011 13:56:58 +0200,
> > David Henningsson wrote:
> >>
> >> Behold the third version of the patch.
> >>
> >> On 08/19/2011 02:46 PM, Takashi Iwai wrote:
> >>> At Fri, 19 Aug 2011 11:49:25 +0200,
> >>> David Henningsson wrote:
> >>>>
> >>>>>  From 8098c77a447a80d2860588387d18b3aa6d23b6bb Mon Sep 17 00:00:00 2001
> >>>> From: David Henningsson<david.henningsson@canonical.com>
> >>>> Date: Fri, 19 Aug 2011 11:47:07 +0200
> >>>> Subject: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been
> >>>>    written
> >>>>
> >>>> If more data has already been written after the underrun, the underrun
> >>>> will automatically end and therefore we should not report it or
> >>>> restart the stream.
> >>>>
> >>>> Signed-off-by: David Henningsson<david.henningsson@canonical.com>
> >>>> ---
> >>>>    configure.in      |    7 +++++++
> >>>>    pulse/pcm_pulse.c |   28 ++++++++++++++++++++++++++--
> >>>>    2 files changed, 33 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/configure.in b/configure.in
> >>>> index ccf59ba..f6886b1 100644
> >>>> --- a/configure.in
> >>>> +++ b/configure.in
> >>>> @@ -31,8 +31,14 @@ AC_ARG_ENABLE([pulseaudio],
> >>>>
> >>>>    if test "x$enable_pulseaudio" != "xno"; then
> >>>>      PKG_CHECK_MODULES(pulseaudio, [libpulse>= 0.9.11], [HAVE_PULSE=yes], [HAVE_PULSE=no])
> >>>> +  PKG_CHECK_MODULES(pulseaudio_099, [libpulse>= 0.99.1],
> >>>> +    [HAVE_PULSE_UNDERRUN_INDEX=yes], [HAVE_PULSE_UNDERRUN_INDEX=no])
> >>>
> >>> Hm, can we use PA_CHECK_VERSION() or such simply in the code?
> >>
> >> Ok, good idea.
> >>
> >>>
> >>>> diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c
> >>>> index d6c6792..61f1c0c 100644
> >>>> --- a/pulse/pcm_pulse.c
> >>>> +++ b/pulse/pcm_pulse.c
> >>>> @@ -26,6 +26,10 @@
> >>>>    #include<alsa/asoundlib.h>
> >>>>    #include<alsa/pcm_external.h>
> >>>>
> >>>> +#ifdef HAVE_CONFIG_H
> >>>> +#include<config.h>
> >>>> +#endif
> >>>> +
> >>>>    #include "pulse.h"
> >>>>
> >>>>    typedef struct snd_pcm_pulse {
> >>>> @@ -39,9 +43,10 @@ typedef struct snd_pcm_pulse {
> >>>>    	size_t last_size;
> >>>>    	size_t ptr;
> >>>>    	int underrun;
> >>>> -	int handle_underrun;
> >>>> +	int handle_underrun; /* can be 0=never, 1=always or 2,3=only if more data has not been written */
> >>>
> >>> I think it'd be simpler to introduce one more boolean variable.
> >>> Otherwise the logic looks too complex than needed.
> >>
> >> I've now done that. I don't know if it became simpler though.
> >
> > Hm, indeed.
> >
> > I think we can start off from more simple.  With the new PA, the
> > underrun detection should work better.  So why we do we need yet
> > another mode to disable it?  It should be enough just to have a
> > boolean for underrun_detect yes/no.
> >
> > So, the patch would be like below.
> >
> > What do you think?
> 
> Sure. For me, I'd go with the new functionality without ever configuring 
> anything else. So I'm happy to exchange "always underrun" for the new 
> underrun detection or even to remove the configuration functionality 
> entirely.

The configuration is still needed for older PA, so let's keep it for
now.  I committed the previous patch now to git tree.


Thanks!

Takashi

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been written
  2011-08-23 13:40         ` Takashi Iwai
  2011-08-23 13:57           ` David Henningsson
@ 2011-08-25  5:53           ` Raymond Yau
  2011-08-25  6:17             ` David Henningsson
  1 sibling, 1 reply; 14+ messages in thread
From: Raymond Yau @ 2011-08-25  5:53 UTC (permalink / raw)
  To: Takashi Iwai, ALSA Development Mailing List

2011/8/23 Takashi Iwai <tiwai@suse.de>:
 be enough just to have a
> boolean for underrun_detect yes/no.
>
> So, the patch would be like below.
>
> What do you think?
>
>
> Takashi
>
> ---
> diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c
> index d6c6792..b0e52ab 100644
> --- a/pulse/pcm_pulse.c
> +++ b/pulse/pcm_pulse.c
> @@ -42,6 +42,7 @@ typedef struct snd_pcm_pulse {
>        int handle_underrun;
>
>        size_t offset;
> +       int64_t written;
>
>        pa_stream *stream;
>
> @@ -460,6 +461,7 @@ static snd_pcm_sframes_t pulse_write(snd_pcm_ioplug_t * io,
>
>        /* Make sure the buffer pointer is in sync */
>        pcm->last_size -= writebytes;
> +       pcm->written += writebytes;
>        ret = update_ptr(pcm);
>        if (ret < 0)
>                goto finish;
> @@ -585,6 +587,15 @@ static void stream_request_cb(pa_stream * p, size_t length, void *userdata)
>        update_active(pcm);
>  }
>
> +#if defined(PA_CHECK_VERSION) && PA_CHECK_VERSION(0,99,0)

compile fail at these line since PA_CHECK_VERSION is not defined in
pulseaudio 0.9.14

seem that pcm->wriiten is application pointer of pulse device in bytes

does it mean that pa_stream_get_underflow_index(0 is the hardware pointer ?

does it mean that pulse device no longer can be run 7 x 24 non-stop
since pcm->written may overflow ?

> +#define DEFAULT_HANDLE_UNDERRUN                1
> +#define do_underrun_detect(pcm, p) \
> +       ((pcm)->written <= pa_stream_get_underflow_index(p))
> +#else
> +#define DEFAULT_HANDLE_UNDERRUN                0
> +#define do_underrun_detect(pcm, p)     1       /* always true */
> +#endif
> +
>  static void stream_underrun_cb(pa_stream * p, void *userdata)
>  {
>        snd_pcm_pulse_t *pcm = userdata;
> @@ -594,7 +605,8 @@ static void stream_underrun_cb(pa_stream * p, void *userdata)
>        if (!pcm->p)
>                return;
>
> -       pcm->underrun = 1;
> +       if (do_underrun_detect(pcm, p))
> +               pcm->underrun = 1;
>  }
>
>  static void stream_latency_cb(pa_stream *p, void *userdata) {
> @@ -739,6 +751,7 @@ static int pulse_prepare(snd_pcm_ioplug_t * io)
>
>        pcm->offset = 0;
>        pcm->underrun = 0;
> +       pcm->written = 0;
>
>        /* Reset fake ringbuffer */
>        pcm->last_size = 0;
> @@ -983,7 +996,7 @@ SND_PCM_PLUGIN_DEFINE_FUNC(pulse)
>        const char *server = NULL;
>        const char *device = NULL;
>        const char *fallback_name = NULL;
> -       int handle_underrun = 0;
> +       int handle_underrun = DEFAULT_HANDLE_UNDERRUN;
>        int err;
>        snd_pcm_pulse_t *pcm;
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been written
  2011-08-25  5:53           ` Raymond Yau
@ 2011-08-25  6:17             ` David Henningsson
  2011-08-25  6:25               ` Raymond Yau
  2011-08-26  7:46               ` Takashi Iwai
  0 siblings, 2 replies; 14+ messages in thread
From: David Henningsson @ 2011-08-25  6:17 UTC (permalink / raw)
  To: Raymond Yau; +Cc: Takashi Iwai, ALSA Development Mailing List

On 08/25/2011 07:53 AM, Raymond Yau wrote:
> 2011/8/23 Takashi Iwai<tiwai@suse.de>:
>   be enough just to have a
>> boolean for underrun_detect yes/no.
>>
>> So, the patch would be like below.
>>
>> What do you think?
>>
>>
>> Takashi
>>
>> ---
>> diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c
>> index d6c6792..b0e52ab 100644
>> --- a/pulse/pcm_pulse.c
>> +++ b/pulse/pcm_pulse.c
>> @@ -42,6 +42,7 @@ typedef struct snd_pcm_pulse {
>>         int handle_underrun;
>>
>>         size_t offset;
>> +       int64_t written;
>>
>>         pa_stream *stream;
>>
>> @@ -460,6 +461,7 @@ static snd_pcm_sframes_t pulse_write(snd_pcm_ioplug_t * io,
>>
>>         /* Make sure the buffer pointer is in sync */
>>         pcm->last_size -= writebytes;
>> +       pcm->written += writebytes;
>>         ret = update_ptr(pcm);
>>         if (ret<  0)
>>                 goto finish;
>> @@ -585,6 +587,15 @@ static void stream_request_cb(pa_stream * p, size_t length, void *userdata)
>>         update_active(pcm);
>>   }
>>
>> +#if defined(PA_CHECK_VERSION)&&  PA_CHECK_VERSION(0,99,0)
>
> compile fail at these line since PA_CHECK_VERSION is not defined in
> pulseaudio 0.9.14

I don't have PA 0.9.14 available, but that's why the line starts with 
"if defined(PA_CHECK_VERSION)" so that the second part should never be 
evaluated.

> seem that pcm->wriiten is application pointer of pulse device in bytes
>
> does it mean that pa_stream_get_underflow_index(0 is the hardware pointer ?
>
> does it mean that pulse device no longer can be run 7 x 24 non-stop
> since pcm->written may overflow ?

Hmm. It's an int64, so your computer is likely to break down before 
there is a wraparound, but perhaps there is a slight chance that the 
comparision will be wrong if there is an underrun at the exact 
wraparound moment.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been written
  2011-08-25  6:17             ` David Henningsson
@ 2011-08-25  6:25               ` Raymond Yau
  2011-08-25  6:41                 ` David Henningsson
  2011-08-26  7:46               ` Takashi Iwai
  1 sibling, 1 reply; 14+ messages in thread
From: Raymond Yau @ 2011-08-25  6:25 UTC (permalink / raw)
  To: ALSA Development Mailing List

2011/8/25 David Henningsson <david.henningsson@canonical.com>:
> On 08/25/2011 07:53 AM, Raymond Yau wrote:
>>
>> 2011/8/23 Takashi Iwai<tiwai@suse.de>:
>>  be enough just to have a
>>>
>>> boolean for underrun_detect yes/no.
>>>
>>> So, the patch would be like below.
>>>
>>> What do you think?
>>>
>>>
>>> Takashi
>>>
>>> ---
>>> diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c
>>> index d6c6792..b0e52ab 100644
>>> --- a/pulse/pcm_pulse.c
>>> +++ b/pulse/pcm_pulse.c
>>> @@ -42,6 +42,7 @@ typedef struct snd_pcm_pulse {
>>>        int handle_underrun;
>>>
>>>        size_t offset;
>>> +       int64_t written;
>>>
>>>        pa_stream *stream;
>>>
>>> @@ -460,6 +461,7 @@ static snd_pcm_sframes_t pulse_write(snd_pcm_ioplug_t
>>> * io,
>>>
>>>        /* Make sure the buffer pointer is in sync */
>>>        pcm->last_size -= writebytes;
>>> +       pcm->written += writebytes;
>>>        ret = update_ptr(pcm);
>>>        if (ret<  0)
>>>                goto finish;
>>> @@ -585,6 +587,15 @@ static void stream_request_cb(pa_stream * p, size_t
>>> length, void *userdata)
>>>        update_active(pcm);
>>>  }
>>>
>>> +#if defined(PA_CHECK_VERSION)&&  PA_CHECK_VERSION(0,99,0)
>>
>> compile fail at these line since PA_CHECK_VERSION is not defined in
>> pulseaudio 0.9.14
>
> I don't have PA 0.9.14 available, but that's why the line starts with "if
> defined(PA_CHECK_VERSION)" so that the second part should never be
> evaluated.


/bin/sh ../libtool --tag=CC   --mode=compile gcc -DHAVE_CONFIG_H -I.
-I..    -Wall -g -I/usr/include/alsa    -D_REENTRANT   -D_GNU_SOURCE
-O2 -Wall -W -pipe -g -MT pcm_pulse.lo -MD -MP -MF .deps/pcm_pulse.Tpo
-c -o pcm_pulse.lo pcm_pulse.c
 gcc -DHAVE_CONFIG_H -I. -I.. -Wall -g -I/usr/include/alsa
-D_REENTRANT -D_GNU_SOURCE -O2 -Wall -W -pipe -g -MT pcm_pulse.lo -MD
-MP -MF .deps/pcm_pulse.Tpo -c pcm_pulse.c  -fPIC -DPIC -o
.libs/pcm_pulse.o
pcm_pulse.c: In function ‘stream_success_cb’:
pcm_pulse.c:192: warning: unused parameter ‘p’
pcm_pulse.c:192: warning: unused parameter ‘success’
pcm_pulse.c: In function ‘stream_request_cb’:
pcm_pulse.c:578: warning: unused parameter ‘p’
pcm_pulse.c:578: warning: unused parameter ‘length’
pcm_pulse.c:590:50: error: missing binary operator before token "("
pcm_pulse.c: In function ‘stream_underrun_cb’:
pcm_pulse.c:599: warning: unused parameter ‘p’
pcm_pulse.c: In function ‘stream_latency_cb’:
pcm_pulse.c:612: warning: unused parameter ‘p’
pcm_pulse.c: In function ‘pulse_pcm_poll_revents’:
pcm_pulse.c:624: warning: unused parameter ‘pfd’
pcm_pulse.c:624: warning: unused parameter ‘nfds’
pcm_pulse.c: In function ‘pulse_hw_params’:
pcm_pulse.c:768: warning: unused parameter ‘params’
make: *** [pcm_pulse.lo] Error 1

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been written
  2011-08-25  6:25               ` Raymond Yau
@ 2011-08-25  6:41                 ` David Henningsson
  0 siblings, 0 replies; 14+ messages in thread
From: David Henningsson @ 2011-08-25  6:41 UTC (permalink / raw)
  To: Raymond Yau; +Cc: ALSA Development Mailing List

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

On 08/25/2011 08:25 AM, Raymond Yau wrote:
> 2011/8/25 David Henningsson<david.henningsson@canonical.com>:
>> On 08/25/2011 07:53 AM, Raymond Yau wrote:
>>>
>>> 2011/8/23 Takashi Iwai<tiwai@suse.de>:
>>>   be enough just to have a
>>>>
>>>> boolean for underrun_detect yes/no.
>>>>
>>>> So, the patch would be like below.
>>>>
>>>> What do you think?
>>>>
>>>>
>>>> Takashi
>>>>
>>>> ---
>>>> diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c
>>>> index d6c6792..b0e52ab 100644
>>>> --- a/pulse/pcm_pulse.c
>>>> +++ b/pulse/pcm_pulse.c
>>>> @@ -42,6 +42,7 @@ typedef struct snd_pcm_pulse {
>>>>         int handle_underrun;
>>>>
>>>>         size_t offset;
>>>> +       int64_t written;
>>>>
>>>>         pa_stream *stream;
>>>>
>>>> @@ -460,6 +461,7 @@ static snd_pcm_sframes_t pulse_write(snd_pcm_ioplug_t
>>>> * io,
>>>>
>>>>         /* Make sure the buffer pointer is in sync */
>>>>         pcm->last_size -= writebytes;
>>>> +       pcm->written += writebytes;
>>>>         ret = update_ptr(pcm);
>>>>         if (ret<    0)
>>>>                 goto finish;
>>>> @@ -585,6 +587,15 @@ static void stream_request_cb(pa_stream * p, size_t
>>>> length, void *userdata)
>>>>         update_active(pcm);
>>>>   }
>>>>
>>>> +#if defined(PA_CHECK_VERSION)&&    PA_CHECK_VERSION(0,99,0)
>>>
>>> compile fail at these line since PA_CHECK_VERSION is not defined in
>>> pulseaudio 0.9.14
>>
>> I don't have PA 0.9.14 available, but that's why the line starts with "if
>> defined(PA_CHECK_VERSION)" so that the second part should never be
>> evaluated.
>
>
> /bin/sh ../libtool --tag=CC   --mode=compile gcc -DHAVE_CONFIG_H -I.
> -I..    -Wall -g -I/usr/include/alsa    -D_REENTRANT   -D_GNU_SOURCE
> -O2 -Wall -W -pipe -g -MT pcm_pulse.lo -MD -MP -MF .deps/pcm_pulse.Tpo
> -c -o pcm_pulse.lo pcm_pulse.c
>   gcc -DHAVE_CONFIG_H -I. -I.. -Wall -g -I/usr/include/alsa
> -D_REENTRANT -D_GNU_SOURCE -O2 -Wall -W -pipe -g -MT pcm_pulse.lo -MD
> -MP -MF .deps/pcm_pulse.Tpo -c pcm_pulse.c  -fPIC -DPIC -o
> .libs/pcm_pulse.o
> pcm_pulse.c: In function ‘stream_success_cb’:
> pcm_pulse.c:192: warning: unused parameter ‘p’
> pcm_pulse.c:192: warning: unused parameter ‘success’
> pcm_pulse.c: In function ‘stream_request_cb’:
> pcm_pulse.c:578: warning: unused parameter ‘p’
> pcm_pulse.c:578: warning: unused parameter ‘length’
> pcm_pulse.c:590:50: error: missing binary operator before token "("
> pcm_pulse.c: In function ‘stream_underrun_cb’:
> pcm_pulse.c:599: warning: unused parameter ‘p’
> pcm_pulse.c: In function ‘stream_latency_cb’:
> pcm_pulse.c:612: warning: unused parameter ‘p’
> pcm_pulse.c: In function ‘pulse_pcm_poll_revents’:
> pcm_pulse.c:624: warning: unused parameter ‘pfd’
> pcm_pulse.c:624: warning: unused parameter ‘nfds’
> pcm_pulse.c: In function ‘pulse_hw_params’:
> pcm_pulse.c:768: warning: unused parameter ‘params’
> make: *** [pcm_pulse.lo] Error 1

Hmm, does the attached patch solve the problem?


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

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

diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c
index b0e52ab..c53d3c7 100644
--- a/pulse/pcm_pulse.c
+++ b/pulse/pcm_pulse.c
@@ -587,11 +587,16 @@ static void stream_request_cb(pa_stream * p, size_t length, void *userdata)
 	update_active(pcm);
 }
 
-#if defined(PA_CHECK_VERSION) && PA_CHECK_VERSION(0,99,0)
+#if defined(PA_CHECK_VERSION)
+#if PA_CHECK_VERSION(0,99,0)
+
 #define DEFAULT_HANDLE_UNDERRUN		1
 #define do_underrun_detect(pcm, p) \
 	((pcm)->written <= pa_stream_get_underflow_index(p))
-#else
+
+#endif
+#endif
+#ifndef DEFAULT_HANDLE_UNDERRUN
 #define DEFAULT_HANDLE_UNDERRUN		0
 #define do_underrun_detect(pcm, p)	1	/* always true */
 #endif

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



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been written
  2011-08-25  6:17             ` David Henningsson
  2011-08-25  6:25               ` Raymond Yau
@ 2011-08-26  7:46               ` Takashi Iwai
  2011-09-22  6:12                 ` Raymond Yau
  1 sibling, 1 reply; 14+ messages in thread
From: Takashi Iwai @ 2011-08-26  7:46 UTC (permalink / raw)
  To: David Henningsson; +Cc: Raymond Yau, ALSA Development Mailing List

At Thu, 25 Aug 2011 08:17:17 +0200,
David Henningsson wrote:
> 
> On 08/25/2011 07:53 AM, Raymond Yau wrote:
> > 2011/8/23 Takashi Iwai<tiwai@suse.de>:
> >   be enough just to have a
> >> boolean for underrun_detect yes/no.
> >>
> >> So, the patch would be like below.
> >>
> >> What do you think?
> >>
> >>
> >> Takashi
> >>
> >> ---
> >> diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c
> >> index d6c6792..b0e52ab 100644
> >> --- a/pulse/pcm_pulse.c
> >> +++ b/pulse/pcm_pulse.c
> >> @@ -42,6 +42,7 @@ typedef struct snd_pcm_pulse {
> >>         int handle_underrun;
> >>
> >>         size_t offset;
> >> +       int64_t written;
> >>
> >>         pa_stream *stream;
> >>
> >> @@ -460,6 +461,7 @@ static snd_pcm_sframes_t pulse_write(snd_pcm_ioplug_t * io,
> >>
> >>         /* Make sure the buffer pointer is in sync */
> >>         pcm->last_size -= writebytes;
> >> +       pcm->written += writebytes;
> >>         ret = update_ptr(pcm);
> >>         if (ret<  0)
> >>                 goto finish;
> >> @@ -585,6 +587,15 @@ static void stream_request_cb(pa_stream * p, size_t length, void *userdata)
> >>         update_active(pcm);
> >>   }
> >>
> >> +#if defined(PA_CHECK_VERSION)&&  PA_CHECK_VERSION(0,99,0)
> >
> > compile fail at these line since PA_CHECK_VERSION is not defined in
> > pulseaudio 0.9.14
> 
> I don't have PA 0.9.14 available, but that's why the line starts with 
> "if defined(PA_CHECK_VERSION)" so that the second part should never be 
> evaluated.

It actually is.  gcc gives an error when it's not available.
The preprocessor is not C, so it's a slightly different rule.

I committed the build fix patch below.


thanks,

Takashi

---
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] pulse - Define a dummy PA_CHECK_VERSION() when not available

An old version of PA doesn't define this macro, and gives a build error.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 pulse/pcm_pulse.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c
index b0e52ab..e0fbd4c 100644
--- a/pulse/pcm_pulse.c
+++ b/pulse/pcm_pulse.c
@@ -587,7 +587,11 @@ static void stream_request_cb(pa_stream * p, size_t length, void *userdata)
 	update_active(pcm);
 }
 
-#if defined(PA_CHECK_VERSION) && PA_CHECK_VERSION(0,99,0)
+#ifndef PA_CHECK_VERSION
+#define PA_CHECK_VERSION(x, y, z)	0
+#endif
+
+#if PA_CHECK_VERSION(0,99,0)
 #define DEFAULT_HANDLE_UNDERRUN		1
 #define do_underrun_detect(pcm, p) \
 	((pcm)->written <= pa_stream_get_underflow_index(p))
-- 
1.7.6.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been written
  2011-08-26  7:46               ` Takashi Iwai
@ 2011-09-22  6:12                 ` Raymond Yau
  0 siblings, 0 replies; 14+ messages in thread
From: Raymond Yau @ 2011-09-22  6:12 UTC (permalink / raw)
  To: Takashi Iwai, ALSA Development Mailing List

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

2011/8/26 Takashi Iwai <tiwai@suse.de>:
> At Thu, 25 Aug 2011 08:17:17 +0200,
>
> It actually is.  gcc gives an error when it's not available.
> The preprocessor is not C, so it's a slightly different rule.
>
> I committed the build fix patch below.
>
>
> thanks,
>
> Takashi
>
> ---
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] pulse - Define a dummy PA_CHECK_VERSION() when not available
>
> An old version of PA doesn't define this macro, and gives a build error.
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  pulse/pcm_pulse.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c
> index b0e52ab..e0fbd4c 100644
> --- a/pulse/pcm_pulse.c
> +++ b/pulse/pcm_pulse.c
> @@ -587,7 +587,11 @@ static void stream_request_cb(pa_stream * p, size_t length, void *userdata)
>        update_active(pcm);
>  }
>
> -#if defined(PA_CHECK_VERSION) && PA_CHECK_VERSION(0,99,0)
> +#ifndef PA_CHECK_VERSION
> +#define PA_CHECK_VERSION(x, y, z)      0
> +#endif
> +
> +#if PA_CHECK_VERSION(0,99,0)
>  #define DEFAULT_HANDLE_UNDERRUN                1
>  #define do_underrun_detect(pcm, p) \
>        ((pcm)->written <= pa_stream_get_underflow_index(p))
> --
> 1.7.6.1
>
>

It seem that there is no way to use "handle_underrun=1"  anymore ?

attach a test program which test the playback/capture position

The result seem avail of "pulse" device never reach buffer_size
and the accuracy of the pointer of pulse device depend on the period
time and it is not as accurate as than those of "hw" and "plug:dmix"

[-- Attachment #2: alsa_test.c --]
[-- Type: text/x-csrc, Size: 7040 bytes --]

/*

 Test playback/capture position of alsa playback/capture device 

 Playback - Try to use 2 seconds buffer , or maximum buffer size/maximum period size 
		when sound card does not has a 2 second buffer
            fill the buffer  
            collect the value from snd_pcm_avail() every 1 ms
            call snd_pcm_drain when avail < buffer_size 

 Record -   Try to use 2 seconds buffer , or maximum buffer size/maximum period size 
		when sound card does not has a 2 second buffer
            start capturing
            collect the value from snd_pcm_avail() every 1 ms


 alsa_test device dir rate periods stop_at_underrun

 you can compare the test result of different devices
 
1) hw:0,0  
2) plug:dmix  
3) pulse 
4) plug:jack
5) plughw:0

*/

#include <time.h>
#include <alsa/asoundlib.h>

//#define DISABLE_RESAMPLE 1 

int main(int argc, char *argv[]) {
    const char *dev;
    int cap, err;
    snd_pcm_hw_params_t *hwparams;
    snd_pcm_sw_params_t *swparams;
    snd_pcm_status_t *status;
    snd_pcm_t *pcm;
    snd_output_t *output = NULL;
    snd_pcm_format_t fmt = SND_PCM_FORMAT_S16_LE;
    snd_pcm_uframes_t boundary, buffer_size, period_size, this_period;
    snd_pcm_sframes_t avail;
    int chn = 2;
    unsigned rate;
    unsigned periods;;
    unsigned int this_time, buffer_time;
    int sleep_time = 1000;
    int buffer[65536];
    int stat[65536];
    int dir = 1;
    int stop_at_underrun;
    int i, counter;

    snd_pcm_hw_params_alloca(&hwparams);
    snd_pcm_sw_params_alloca(&swparams);

    dev = argc > 1 ? argv[1] : "hw:0,0";
    cap = argc > 2 ? atoi(argv[2]) : 0;
    rate = argc > 3 ? atoi(argv[3]) : 44100;
    periods = argc > 4 ? atoi(argv[4]) : 2;
    stop_at_underrun = argc > 5 ? atoi(argv[5]) : 1;    

    err = snd_pcm_open(&pcm, dev, cap == 0 ? SND_PCM_STREAM_PLAYBACK : 
		SND_PCM_STREAM_CAPTURE, 0);
    if (err < 0) {
	printf("snd_pcm_open fail %s err %s\n",dev, snd_strerror(err));
        exit(0);
    }

    err = snd_pcm_hw_params_any(pcm, hwparams);
#ifdef DISABLE_RESAMPLE
    err = snd_pcm_hw_params_set_rate_resample(pcm, hwparams, 0);
    if (err < 0) {
	printf("set rate resample err %s\n", snd_strerror(err));
        exit(0);
    };
#endif
    err = snd_pcm_hw_params_set_access(pcm, hwparams, SND_PCM_ACCESS_RW_INTERLEAVED);
    if (err < 0) {
	printf("set access err %s\n", snd_strerror(err));
        exit(0);
    };

    err = snd_pcm_hw_params_set_format(pcm, hwparams, fmt);
    if (err < 0) {
	printf("set format %s err %s\n",snd_pcm_format_name(fmt), snd_strerror(err));
        exit(0);
    };

    err = snd_pcm_hw_params_set_rate_near(pcm, hwparams, &rate, NULL);
    if (err < 0) {
	printf("set rate to %dHz err %s\n",rate, snd_strerror(err));
        exit(0);
    };

    err = snd_pcm_hw_params_set_channels(pcm, hwparams, chn);
    if (err < 0) {
	printf("set channel to %d err %s\n",chn, snd_strerror(err));
        exit(0);
    };
/*
    snd_pcm_hw_params_set_periods_integer(pcm, hwparams);
*/

    dir = 0;
    err = snd_pcm_hw_params_set_periods_near(pcm, hwparams, &periods, &dir);
    if (err < 0) {
	printf("set perods err %s\n", snd_strerror(err));
        exit(0);
    };

    buffer_size = rate * 2;
    err = snd_pcm_hw_params_set_buffer_size_near(pcm, hwparams, &buffer_size);
    if (err < 0) {
	printf("set buffer size near err %s\n", snd_strerror(err));
        exit(0);
    };

    err = snd_pcm_hw_params(pcm, hwparams);
    if (err < 0) {
	printf("hw_params err %s\n", snd_strerror(err));
        exit(0);
    };

    err = snd_pcm_sw_params_current(pcm, swparams);
    if (err < 0) {
	printf("sw_params current %s\n", snd_strerror(err));
        exit(0);
    };

    err = snd_pcm_hw_params_get_buffer_size(hwparams, &buffer_size);
    if (err < 0) {
	printf("get buffer size err %s\n", snd_strerror(err));
        exit(0);
    };
/*
    err = snd_pcm_hw_params_get_buffer_time(hwparams, &buffer_time, 0);
    if (err < 0) {
	printf("get buffer time err %s\n", snd_strerror(err));
        exit(0);
    };
*/
    err = snd_pcm_hw_params_get_period_size(hwparams, &period_size, 0);
    if (err < 0) {
	printf("get period size err %s\n", snd_strerror(err));
        exit(0);
    };

    err = snd_pcm_sw_params_get_boundary(swparams, &boundary);
    if (err < 0) {
        printf("get boundary err %s\n", snd_strerror(err));
        exit(0);
    };

/*
    err = snd_pcm_sw_params_set_avail_min(pcm, swparams, period_size);
    if (err < 0) {
	printf("set avail min err %s\n", snd_strerror(err));
        exit(0);
    };
*/
/*
    err = snd_pcm_sw_params_set_period_event(pcm, swparams, 0);
    assert(err == 0);
*/
    if (cap == 0) {
        err = snd_pcm_sw_params_set_start_threshold(pcm, swparams, buffer_size);
        if (err < 0) {
	    printf("set start threshold err %s\n", snd_strerror(err));
            exit(0);
	};
    };

    if (!stop_at_underrun) {
        err = snd_pcm_sw_params_set_stop_threshold(pcm, swparams, boundary);
        if (err < 0) {
	    printf("set stop threshold err %s\n", snd_strerror(err));
            exit(0);
        };
    };

    err = snd_pcm_sw_params_set_tstamp_mode(pcm, swparams, SND_PCM_TSTAMP_ENABLE);
    if (err < 0) {
	printf("set tstamp mode err %s\n", snd_strerror(err));
        exit(0);
    };

    err = snd_pcm_sw_params(pcm, swparams);
    if (err < 0) {
	printf("sw_params err %s\n", snd_strerror(err));
        exit(0);
    };

    err = snd_pcm_sw_params_current(pcm, swparams);
    if (err < 0) {
	printf("sw_params current %s\n", snd_strerror(err));
        exit(0);
    };

    err = snd_output_stdio_attach(&output, stdout, 0);
    if (err < 0) {
	printf("Output failed: %s\n", snd_strerror(err));
        exit(0);
    };
    snd_pcm_dump(pcm, output);

/*     assert(snd_pcm_hw_params_is_monotonic(hwparams) > 0); */

    if (cap) {
      err = snd_pcm_start(pcm);
      assert(err == 0);
    }
    else {

/*
	FIXME: Write whole buffer instead of periods
*/
        this_period = 0;
        while (this_period < buffer_size) {
            err = snd_pcm_writei(pcm, buffer, period_size);
	    if (err < 0)
 	        printf("pcm write err %d %s\n",err,snd_strerror(err));
	    else
                this_period += period_size;
	};
    };

    avail = 0;
    this_time = 0;
    counter = 0;
    while (counter <= 3000 && avail >= 0) {
        avail = snd_pcm_avail(pcm);
        stat[counter] = avail;
        usleep(sleep_time);
        counter++;
        this_time += sleep_time;
    };
    printf("Time     %s available\n", cap == 0 ? "Playback" : "Capture" );
    for (i=0; i<counter; i++) 
        printf("%f %d\n",(float)(i)*sleep_time/1000000, stat[i]);
    if ( cap == 0) {
        if ((avail > 0) && (avail < buffer_size)) {
            err = snd_pcm_drain(pcm);
	    if (err < 0) {
	        printf("pcm drain err %d %s\n",err,snd_strerror(err));
                exit(0);
	    };
            printf("pcm drain : avail %d\n",snd_pcm_avail(pcm));   
	};
    };
    snd_pcm_dump(pcm, output);
    snd_pcm_close(pcm);
    snd_output_close(output);
    return 0;
}

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



^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2011-09-22  6:12 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-18 15:06 [PATCH] alsa-plugins: Pulse: only underrun if no more data has been written David Henningsson
2011-08-19  7:43 ` Takashi Iwai
2011-08-19  9:49   ` David Henningsson
2011-08-19 12:46     ` Takashi Iwai
2011-08-23 11:56       ` David Henningsson
2011-08-23 13:40         ` Takashi Iwai
2011-08-23 13:57           ` David Henningsson
2011-08-23 15:07             ` Takashi Iwai
2011-08-25  5:53           ` Raymond Yau
2011-08-25  6:17             ` David Henningsson
2011-08-25  6:25               ` Raymond Yau
2011-08-25  6:41                 ` David Henningsson
2011-08-26  7:46               ` Takashi Iwai
2011-09-22  6:12                 ` Raymond Yau

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.