Re: [PATCH] alsa-plugins: Pulse: only underrun if no more data has been written

[Takashi Iwai <tiwai@suse.de>]

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