[alsa-devel] [PATCH] alsa-plugins: Pulse: only underrun if no more data has been written
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.
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:
- 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?
- 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
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:
- 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)
- 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 Henningssondavid.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 Henningssondavid.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
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
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 Henningssondavid.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 Henningssondavid.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.
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 Henningssondavid.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 Henningssondavid.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;
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 Henningssondavid.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 Henningssondavid.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;
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 Henningssondavid.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 Henningssondavid.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
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;
On 08/25/2011 07:53 AM, Raymond Yau wrote:
2011/8/23 Takashi Iwaitiwai@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.
2011/8/25 David Henningsson david.henningsson@canonical.com:
On 08/25/2011 07:53 AM, Raymond Yau wrote:
2011/8/23 Takashi Iwaitiwai@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
On 08/25/2011 08:25 AM, Raymond Yau wrote:
2011/8/25 David Henningssondavid.henningsson@canonical.com:
On 08/25/2011 07:53 AM, Raymond Yau wrote:
2011/8/23 Takashi Iwaitiwai@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?
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 Iwaitiwai@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))
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"
participants (3)
-
David Henningsson -
Raymond Yau -
Takashi Iwai