[PATCH] Alsa-plugins: Pulse: Fix snd_pcm_avail returning 0 in some cases

[PATCH] Alsa-plugins: Pulse: Fix snd_pcm_avail returning 0 in some cases

[David Henningsson]

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

Due to a round-off error, snd_pcm_avail could in some cases
return 0 even though more data could be written to the stream.

This was discovered by Maarten Lankhorst [1], and there is also a test 
program available that triggers this error [2].

[1] 
https://tango.0pointer.de/pipermail/pulseaudio-discuss/2011-April/009935.html

[2] 
https://tango.0pointer.de/pipermail/pulseaudio-discuss/attachments/20110420/3c852d6e/attachment.c


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

[-- Attachment #2: 0001-Pulse-Fix-snd_pcm_avail-returning-0-in-some-cases.patch --]
[-- Type: text/x-patch, Size: 1119 bytes --]

>From 73ff6f83e1e4c53181e1794bf2cec3baee7c81b7 Mon Sep 17 00:00:00 2001
From: David Henningsson <david.henningsson@canonical.com>
Date: Thu, 21 Apr 2011 15:10:19 +0200
Subject: [PATCH] Pulse: Fix snd_pcm_avail returning 0 in some cases

Due to a round-off error, snd_pcm_avail could in some cases
return 0 even though more data could be written to the stream.

Reported-by: Maarten Lankhorst <m.b.lankhorst@gmail.com>
Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 pulse/pcm_pulse.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c
index 2df0a80..9105d4d 100644
--- a/pulse/pcm_pulse.c
+++ b/pulse/pcm_pulse.c
@@ -92,8 +92,8 @@ static int update_ptr(snd_pcm_pulse_t *pcm)
 		size -= pcm->offset;
 
 	/* Prevent accidental overrun of the fake ringbuffer */
-	if (size >= pcm->buffer_attr.tlength)
-		size = pcm->buffer_attr.tlength-1;
+	if (size > pcm->buffer_attr.tlength - pcm->frame_size)
+		size = pcm->buffer_attr.tlength - pcm->frame_size;
 
 	if (size > pcm->last_size) {
 		pcm->ptr += size - pcm->last_size;
-- 
1.7.4.1


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

Re: [PATCH] Alsa-plugins: Pulse: Fix snd_pcm_avail returning 0 in some cases

[Raymond Yau]

2011/4/21 David Henningsson <david.henningsson@canonical.com>

> Due to a round-off error, snd_pcm_avail could in some cases
> return 0 even though more data could be written to the stream.
>
> This was discovered by Maarten Lankhorst [1], and there is also a test
> program available that triggers this error [2].
>
> [1]
> https://tango.0pointer.de/pipermail/pulseaudio-discuss/2011-April/009935.html
>
> [2]
> https://tango.0pointer.de/pipermail/pulseaudio-discuss/attachments/20110420/3c852d6e/attachment.c
>
>
> --
> David Henningsson, Canonical Ltd.
> http://launchpad.net/~diwic
>

if the test program can force under-run occur with "hw" device and "pulse"
device with his patch in
https://tango.0pointer.de/pipermail/pulseaudio-discuss/2011-April/009918.html

Is it normal that underrun does not occur with the test program and your
patch

Re: [PATCH] Alsa-plugins: Pulse: Fix snd_pcm_avail returning 0 in some cases

[David Henningsson]

On 2011-04-22 01:25, Raymond Yau wrote:
> 2011/4/21 David Henningsson<david.henningsson@canonical.com>
>
>> Due to a round-off error, snd_pcm_avail could in some cases
>> return 0 even though more data could be written to the stream.
>>
>> This was discovered by Maarten Lankhorst [1], and there is also a test
>> program available that triggers this error [2].
>>
>> [1]
>> https://tango.0pointer.de/pipermail/pulseaudio-discuss/2011-April/009935.html
>>
>> [2]
>> https://tango.0pointer.de/pipermail/pulseaudio-discuss/attachments/20110420/3c852d6e/attachment.c
>>
>
> if the test program can force under-run occur with "hw" device and "pulse"
> device with his patch in
> https://tango.0pointer.de/pipermail/pulseaudio-discuss/2011-April/009918.html
>
> Is it normal that underrun does not occur with the test program and your
> patch

Yes; underruns are not reported to the application due to the risk of 
the underrun being obsolete at that time. As for Maarten's patch in the 
post you refer to,

1) if underruns are being reported (this is configurable), it might be a 
good idea to call "pulse_start".

2) it changes underruns to being reported by default, which is what I'm 
opposed to.

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

Re: [PATCH] Alsa-plugins: Pulse: Fix snd_pcm_avail returning 0 in some cases

[Takashi Iwai]

At Tue, 26 Apr 2011 10:01:23 +0200,
David Henningsson wrote:
> 
> On 2011-04-22 01:25, Raymond Yau wrote:
> > 2011/4/21 David Henningsson<david.henningsson@canonical.com>
> >
> >> Due to a round-off error, snd_pcm_avail could in some cases
> >> return 0 even though more data could be written to the stream.
> >>
> >> This was discovered by Maarten Lankhorst [1], and there is also a test
> >> program available that triggers this error [2].
> >>
> >> [1]
> >> https://tango.0pointer.de/pipermail/pulseaudio-discuss/2011-April/009935.html
> >>
> >> [2]
> >> https://tango.0pointer.de/pipermail/pulseaudio-discuss/attachments/20110420/3c852d6e/attachment.c
> >>
> >
> > if the test program can force under-run occur with "hw" device and "pulse"
> > device with his patch in
> > https://tango.0pointer.de/pipermail/pulseaudio-discuss/2011-April/009918.html
> >
> > Is it normal that underrun does not occur with the test program and your
> > patch
> 
> Yes; underruns are not reported to the application due to the risk of 
> the underrun being obsolete at that time. As for Maarten's patch in the 
> post you refer to,
> 
> 1) if underruns are being reported (this is configurable), it might be a 
> good idea to call "pulse_start".
> 
> 2) it changes underruns to being reported by default, which is what I'm 
> opposed to.

Applied now.  Thanks.


Takashi

Re: [PATCH] Alsa-plugins: Pulse: Fix snd_pcm_avail returning 0 in some cases

[Raymond Yau]

2011/4/26 David Henningsson <david.henningsson@canonical.com>

> On 2011-04-22 01:25, Raymond Yau wrote:
>
>> 2011/4/21 David Henningsson<david.henningsson@canonical.com>
>>
>>  Due to a round-off error, snd_pcm_avail could in some cases
>>> return 0 even though more data could be written to the stream.
>>>
>>> This was discovered by Maarten Lankhorst [1], and there is also a test
>>> program available that triggers this error [2].
>>>
>>> [1]
>>>
>>> https://tango.0pointer.de/pipermail/pulseaudio-discuss/2011-April/009935.html
>>>
>>> [2]
>>>
>>> https://tango.0pointer.de/pipermail/pulseaudio-discuss/attachments/20110420/3c852d6e/attachment.c
>>>
>>>
>> if the test program can force under-run occur with "hw" device and "pulse"
>> device with his patch in
>>
>> https://tango.0pointer.de/pipermail/pulseaudio-discuss/2011-April/009918.html
>>
>> Is it normal that underrun does not occur with the test program and your
>> patch
>>
>
> Yes; underruns are not reported to the application due to the risk of the
> underrun being obsolete at that time. As for Maarten's patch in the post you
> refer to,
>
> 1) if underruns are being reported (this is configurable), it might be a
> good idea to call "pulse_start".
>
> 2) it changes underruns to being reported by default, which is what I'm
> opposed to.
>
> >>> However if I force underruns to occur, the state will stay running and
it appears
>>> there is still some data left in the buffer, so sound stalls entirely.
The latency gets >>> updated to something like 0x7bdXXXXXXXXXXXXX which
looks suspiciously
>>> much like a pointer to me, which may be a bug.
>>> So my questions are.
>>> 1. Is the weird latency value update a bug? Digging it up I can only
assume it's a
>>> bug in src/stream.c , but I haven't figured out why yet.  Tested with
pulseaudio.c
>>> 2. Any comments on this patch for alsa-plugins?

There are some difference between devices "hw" , "plug:dmix" and "pulse"
when I run "aplay -v --test-position any.wav" with different period size and
buffer size

It seem that the delay of  device "pulse" is much larger than those of
"dmix" and "hw"

Re: [PATCH] Alsa-plugins: Pulse: Fix snd_pcm_avail returning 0 in some cases

[Raymond Yau]

On 2011-04-20 12:09, Maarten Lankhorst wrote:
>* Hi David,
*>*
*>* Op 20-04-11 09:33, David Henningsson schreef:
*>>* On 2011-04-19 18:12, Maarten Lankhorst wrote:
*>>>* Hi all,
*>>>*
*>>>* For wine I was investigating a bug with pulseaudio, it seems
*>>>* alsa-plugins' pulse driver ignores underruns.


*If you follow the instruction in
http://colin.guthr.ie/2010/09/compiling-and-running-pulseaudio-from-git/

to run a more advanced client application and define DEBUG_TIMING in
alsa-sink.c

you can test it with

aplay -Dpulse --period-time=10000 any.wav

and

aplay -Dhw:0,0 --period-time=10000 any.wav

The period time "10ms" which work with "hw:0,0", may not work with
"pulse" any more
since PA server is expecting the sound card to run accurately at 5ms
period time**

aplay -Dpulse -v --period-time=10000 any.wav
Playing WAVE 'any.wav' : Signed 16 bit Little Endian, Rate 44100 Hz, Stereo
ALSA <-> PulseAudio PCM I/O Plugin
Its setup is:
  stream       : PLAYBACK

  access       : RW_INTERLEAVED
  format       : S16_LE
  subformat    : STD
  channels     : 2
  rate         : 44100
  exact rate   : 44100 (44100/1)
  msbits       : 16
  buffer_size  : 22050
  period_size  : 441

  period_time  : 10000
  tstamp_mode  : NONE
  period_step  : 1
  avail_min    : 441
  period_event : 0
  start_threshold  : 1323
  stop_threshold   : 1323
  silence_threshold: 0
  silence_size : 0

  boundary     : 1387266048



D: alsa-sink.c: Wrote 928 bytes (of possible 928 bytes)
D: alsa-sink.c: avail: 63804
D: alsa-sink.c: 9.82 ms left to play; inc threshold = 0.00 ms; dec
threshold = 100.00 ms

D: alsa-sink.c: Buffer time: 10 ms; Sleep time: 4 ms; Process time: 5 ms
D: alsa-sink.c: avail: 63868
D: alsa-sink.c: 9.46 ms left to play; inc threshold = 0.00 ms; dec
threshold = 100.00 ms
D: alsa-sink.c: Buffer time: 10 ms; Sleep time: 4 ms; Process time: 5 ms

D: alsa-sink.c: avail: 63900
D: alsa-sink.c: 9.27 ms left to play; inc threshold = 0.00 ms; dec
threshold = 100.00 ms
*


when using aplay to play those audio which are not the default sample
rate/ channels of PA server
such as 48000Hz and mono (e.g. /usr/share/sounds/alsa/*.wav"

5ms seem not enough for resampling/enable logging

underrun occur (avail 66552 > buffer size 65536) and PA server abort

D: alsa-sink.c: 9.46 ms left to play; inc threshold = 0.00 ms; dec
threshold = 100.00 ms
D: alsa-sink.c: Buffer time: 10 ms; Sleep time: 4 ms; Process time: 5 ms
D: alsa-sink.c: avail: 64288
D: alsa-sink.c: 7.07 ms left to play; inc threshold = 0.00 ms; dec
threshold = 100.00 ms
D: alsa-sink.c: Wrote 516 bytes (of possible 516 bytes)
D: alsa-sink.c: avail: 66552
Trace/breakpoint trap



*>>*
*>>* Hmm, doesn't wine come with a native pulse driver these days?
*>* Nope, but distros patch in a buggy winepulse driver, wine is working on
*>* rearchitecting its driver model, so that a pulseaudio driver might be
*>* added after that, but the current winepulse driver was a bad
*>* copy/replace job of the wrong sound driver. :)

*winepulse is almost useless if it does not support mixer and midi

> My main point is that the underrun is often obsolete when the message

> reached the application, because more data has already been written,
> therefore reporting it to the app does more harm than good.
> At least until the underrun callback (and PA protocol) supports sending

> the position of underrun, together with the underrun message. If we had
> that position, we could compare that with the current write pointer to
> determine whether the underrun is actually obsolete or not.

Refer to the output in "Running a Client Application"
If seem that underrun is unavoidable , as you should notice that
"paplay -vv" also get "Stream underrun" at the end

it is not granunette that snd_pcm_close() must be called after
snd_pcm_drain() or snd_pcm_drop()

src/paplay -vv /usr/share/sounds/ia_ora-startup.wav
Opening a playback stream with sample specification 's16le 2ch
22050Hz' and channel map 'front-left,front-right'.
Connection established.

Stream successfully created.
Buffer metrics: maxlength=4194304, tlength=176400, prebuf=174640, minreq=1764
Using sample spec 's16le 2ch 22050Hz', channel map 'front-left,front-right'.
Connected to device alsa_output.pci-0000_00_1b.0.analog-stereo (0, not
suspended).

Stream started.
Stream underrun.
Playback stream drained.: 1007045 usec.
Draining connection to server.
*
*

Re: [PATCH] Alsa-plugins: Pulse: Fix snd_pcm_avail returning 0 in some cases

[Raymond Yau]

2011/4/26 Takashi Iwai <tiwai@suse.de>

> At Tue, 26 Apr 2011 10:01:23 +0200,
> David Henningsson wrote:
> >
> > On 2011-04-22 01:25, Raymond Yau wrote:
> > > 2011/4/21 David Henningsson<david.henningsson@canonical.com>
> > >
> > >> Due to a round-off error, snd_pcm_avail could in some cases
> > >> return 0 even though more data could be written to the stream.
> > >>
> > >> This was discovered by Maarten Lankhorst [1], and there is also a test
> > >> program available that triggers this error [2].
> > >>
> > >> [1]
> > >>
> https://tango.0pointer.de/pipermail/pulseaudio-discuss/2011-April/009935.html
> > >>
> > >> [2]
> > >>
> https://tango.0pointer.de/pipermail/pulseaudio-discuss/attachments/20110420/3c852d6e/attachment.c
> > >>
> > >
> > > if the test program can force under-run occur with "hw" device and
> "pulse"
> > > device with his patch in
> > >
> https://tango.0pointer.de/pipermail/pulseaudio-discuss/2011-April/009918.html
> > >
> > > Is it normal that underrun does not occur with the test program and
> your
> > > patch
> >
> > Yes; underruns are not reported to the application due to the risk of
> > the underrun being obsolete at that time. As for Maarten's patch in the
> > post you refer to,
> >
> > 1) if underruns are being reported (this is configurable), it might be a
> > good idea to call "pulse_start".
> >
> > 2) it changes underruns to being reported by default, which is what I'm
> > opposed to.
>
> Applied now.  Thanks.
>
>
> Takashi
>

With this patch and Maarten's example.c , it seem that pulse plugin return
SND_PCM_STATE_XRUN when the server is aborted