Re: [pulseaudio-discuss] [PATCH] pulse: Fix invalid buffer pointer return value

2010-04-20 Thread David Henningsson 
Nix wrote:
 On 18 Apr 2010, David Henningsson spake thusly:
 
 Both of them were tested by Daniel, and AFAIK neither have been applied
 upstream.
 
 These are both alsa-plugin things, right?

Yes.

 In that case, they can't fix the hangs I reported back in February,
 since these are hangs experienced even --- especially --- with native PA
 applications such as mpd, when seeking. 

Correct, since alsa-plugins is not in the audio stack in that case.

// David

___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] pulse: Fix invalid buffer pointer return value

2010-04-19 Thread Nix 
On 18 Apr 2010, David Henningsson spake thusly:

 Both of them were tested by Daniel, and AFAIK neither have been applied
 upstream.

These are both alsa-plugin things, right?

In that case, they can't fix the hangs I reported back in February,
since these are hangs experienced even --- especially --- with native PA
applications such as mpd, when seeking. (See the thread starting at
https://tango.0pointer.de/pipermail/pulseaudio-discuss/2010-February/006727.html.)

I haven't chased it up yet because other bug-hunting (and job hunting)
has taken priority...
___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] pulse: Fix invalid buffer pointer return value

2010-04-18 Thread David Henningsson 
Colin Guthrie wrote:
 'Twas brillig, and David Henningsson at 16/01/10 07:07 did gyre and gimble:
 David Henningsson wrote:
 Lennart Poettering wrote:
 On Sat, 09.01.10 10:00, David Henningsson (launchpad@epost.diwic.se) 
 wrote:

 The pulse ALSA plugin has been known, for a while, to not work properly,
 causing underruns, hangs etc. I sat down yesterday trying to figure it
 out, and I'm pretty certain this patch improves the situation, but I
 don't mind getting some help testing it before it is committed
 upstream.
 I am kinda convinced that the actual fix for this issue is this
 patch. Could you check that?

 http://git.0pointer.de/?p=pulseaudio.git;a=patch;h=8d356659e69556fa25d0579a66084f820683e2b8
 I have now checked it with the latest version of pulseaudio and without
 my patch posted earlier, I still experience hangs. That and positive
 feedback given from https://bugs.launchpad.net/bugs/485488 makes me
 believe that the patch is ready for inclusion upstream (it is already in
 Ubuntu, thanks Dan).

 Now onto the next problem. If an underrun occurs in the pulseaudio
 front-end, it is delivered asynchonously to an underrun callback. This
 sometimes happens after more buffers are sent (through
 pa_stream_write()). So these new buffers will end the underrun in
 parallell with the processing of the underrun in the client, making the
 underrun obsolete. After all, seen from the client program's
 perspective, if I write a buffer and it returns underrun (-EPIPE), I
 assume that all other buffers I've written, which returned successfully,
 have already been played back.

 The best action in this case should be to ignore the underrun, if I can
 determine that the above case is true. Now, how do I do that? I have
 tried to look at pa_stream_get_timing_info() [1], but I can't find a
 reliable way to distinguish valid underruns from obsolete ones.

 // David

 [1] which needs to be updated, and I can't use pa_wait_operation from
 the callback so I'll have to use a second callback for the timing info...
 
 
 Just pinging this one up. I forgot I'd applied this to my own personal
 alsa-plugins build and only noticed it when I updated to 1.0.23 upstream
 version.
 
 I've not seen any specific problems with this but also not sure what it
 fixes.

It fixes (at least) the mpg123 startup race (which lead to dropped
buffers on an underrun).

 Is this still applied in Ubuntu 

Daniel Chen volunteered to do some testing and he found no regressions,
so he applied my remove underrun handling patch, and AFAIK it is still
applied.

I haven't heard of any regressions so far, so I believe it should be
pretty safe to upstream.

 and should we get further review from
 Lennart when he's got a mo (which is sadly not often in recent months)
 about the issue with a view to getting it upstreamed?

Lennart's, anyone else's opinion, and upstreaming, is very welcome. If
you feel I should do something to help this happen, go ahead and tell
me. :-)

// David

___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] pulse: Fix invalid buffer pointer return value

2010-04-17 Thread Colin Guthrie 
'Twas brillig, and David Henningsson at 16/01/10 07:07 did gyre and gimble:
 David Henningsson wrote:
 Lennart Poettering wrote:
 On Sat, 09.01.10 10:00, David Henningsson (launchpad@epost.diwic.se) 
 wrote:

 The pulse ALSA plugin has been known, for a while, to not work properly,
 causing underruns, hangs etc. I sat down yesterday trying to figure it
 out, and I'm pretty certain this patch improves the situation, but I
 don't mind getting some help testing it before it is committed
 upstream.
 I am kinda convinced that the actual fix for this issue is this
 patch. Could you check that?

 http://git.0pointer.de/?p=pulseaudio.git;a=patch;h=8d356659e69556fa25d0579a66084f820683e2b8
 
 I have now checked it with the latest version of pulseaudio and without
 my patch posted earlier, I still experience hangs. That and positive
 feedback given from https://bugs.launchpad.net/bugs/485488 makes me
 believe that the patch is ready for inclusion upstream (it is already in
 Ubuntu, thanks Dan).
 
 Now onto the next problem. If an underrun occurs in the pulseaudio
 front-end, it is delivered asynchonously to an underrun callback. This
 sometimes happens after more buffers are sent (through
 pa_stream_write()). So these new buffers will end the underrun in
 parallell with the processing of the underrun in the client, making the
 underrun obsolete. After all, seen from the client program's
 perspective, if I write a buffer and it returns underrun (-EPIPE), I
 assume that all other buffers I've written, which returned successfully,
 have already been played back.
 
 The best action in this case should be to ignore the underrun, if I can
 determine that the above case is true. Now, how do I do that? I have
 tried to look at pa_stream_get_timing_info() [1], but I can't find a
 reliable way to distinguish valid underruns from obsolete ones.
 
 // David
 
 [1] which needs to be updated, and I can't use pa_wait_operation from
 the callback so I'll have to use a second callback for the timing info...


Just pinging this one up. I forgot I'd applied this to my own personal
alsa-plugins build and only noticed it when I updated to 1.0.23 upstream
version.

I've not seen any specific problems with this but also not sure what it
fixes.

Is this still applied in Ubuntu and should we get further review from
Lennart when he's got a mo (which is sadly not often in recent months)
about the issue with a view to getting it upstreamed?

Col


-- 

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

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

___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] pulse: Fix invalid buffer pointer return value

2010-01-15 Thread Daniel Chen 
On Fri, Jan 15, 2010 at 2:53 AM, David Henningsson
launchpad@epost.diwic.se wrote:
 @Daniel: I saw Lucid got a new version of pulseaudio today, can you tell
 if this commit is included in that version or whether I must compile it
 myself?

It's in Lucid's current version.

Best,
-Dan
___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] pulse: Fix invalid buffer pointer return value

2010-01-15 Thread David Henningsson 
David Henningsson wrote:
 Lennart Poettering wrote:
 On Sat, 09.01.10 10:00, David Henningsson (launchpad@epost.diwic.se) 
 wrote:

 The pulse ALSA plugin has been known, for a while, to not work properly,
 causing underruns, hangs etc. I sat down yesterday trying to figure it
 out, and I'm pretty certain this patch improves the situation, but I
 don't mind getting some help testing it before it is committed
 upstream.
 I am kinda convinced that the actual fix for this issue is this
 patch. Could you check that?

 http://git.0pointer.de/?p=pulseaudio.git;a=patch;h=8d356659e69556fa25d0579a66084f820683e2b8

I have now checked it with the latest version of pulseaudio and without
my patch posted earlier, I still experience hangs. That and positive
feedback given from https://bugs.launchpad.net/bugs/485488 makes me
believe that the patch is ready for inclusion upstream (it is already in
Ubuntu, thanks Dan).

Now onto the next problem. If an underrun occurs in the pulseaudio
front-end, it is delivered asynchonously to an underrun callback. This
sometimes happens after more buffers are sent (through
pa_stream_write()). So these new buffers will end the underrun in
parallell with the processing of the underrun in the client, making the
underrun obsolete. After all, seen from the client program's
perspective, if I write a buffer and it returns underrun (-EPIPE), I
assume that all other buffers I've written, which returned successfully,
have already been played back.

The best action in this case should be to ignore the underrun, if I can
determine that the above case is true. Now, how do I do that? I have
tried to look at pa_stream_get_timing_info() [1], but I can't find a
reliable way to distinguish valid underruns from obsolete ones.

// David

[1] which needs to be updated, and I can't use pa_wait_operation from
the callback so I'll have to use a second callback for the timing info...


___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] pulse: Fix invalid buffer pointer return value

2010-01-14 Thread Lennart Poettering 
On Sat, 09.01.10 10:00, David Henningsson (launchpad@epost.diwic.se) wrote:

 The pulse ALSA plugin has been known, for a while, to not work properly,
 causing underruns, hangs etc. I sat down yesterday trying to figure it
 out, and I'm pretty certain this patch improves the situation, but I
 don't mind getting some help testing it before it is committed
 upstream.

I am kinda convinced that the actual fix for this issue is this
patch. Could you check that?

http://git.0pointer.de/?p=pulseaudio.git;a=patch;h=8d356659e69556fa25d0579a66084f820683e2b8

Lennart

-- 
Lennart PoetteringRed Hat, Inc.
lennart [at] poettering [dot] net
http://0pointer.net/lennart/   GnuPG 0x1A015CC4
___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] pulse: Fix invalid buffer pointer return value

2010-01-14 Thread David Henningsson 
Lennart Poettering wrote:
 On Sat, 09.01.10 10:00, David Henningsson (launchpad@epost.diwic.se) 
 wrote:
 
 The pulse ALSA plugin has been known, for a while, to not work properly,
 causing underruns, hangs etc. I sat down yesterday trying to figure it
 out, and I'm pretty certain this patch improves the situation, but I
 don't mind getting some help testing it before it is committed
 upstream.
 
 I am kinda convinced that the actual fix for this issue is this
 patch. Could you check that?
 
 http://git.0pointer.de/?p=pulseaudio.git;a=patch;h=8d356659e69556fa25d0579a66084f820683e2b8

What I saw happened during the hang inside snd_pcm_writei, was that we
had a tlength/buffersize of say 2048 frames with 4 fragments, but
pulseaudio's pa_stream_writable_size() returned say 30800, possibly
because it had adjusted the latency upwards. What then happened was that
the hw_ptr in the fake ringbuffer advanced with 30800 % 2048 = 80, less
than a fragment. So snd_pcm_writei waits forever for 512 frames to
become available before it writes anything, because it thinks there are
just 80 available. The patch corrects that situation by advancing the
hw_ptr with 2047 % 2048 instead.

So I'm kinda convinced too, but I'll test your patch if you test mine ;-)

@Daniel: I saw Lucid got a new version of pulseaudio today, can you tell
if this commit is included in that version or whether I must compile it
myself?

// David
___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] pulse: Fix invalid buffer pointer return value

2010-01-10 Thread David Henningsson 
 'Twas brillig, and David Henningsson at 09/01/10 09:00 did gyre and
 gimble:
 The pulse ALSA plugin has been known, for a while, to not work
 properly,
 causing underruns, hangs etc. I sat down yesterday trying to figure it
 out, and I'm pretty certain this patch improves the situation, but I
 don't mind getting some help testing it before it is committed
 upstream.

 Related bug: https://bugs.launchpad.net/bugs/485488

 The patch is for the alsa-plugins tree at git.alsa-projects.org.

 I applied this to 1.0.22 (cleanly) and sadly it's not helping my test
 case using mpg123.

 I've tried to debug the mpg123 issue a little further.

 Assuming my analysis is correct, what happens is that pa_stream_cork() is
 called just after the first 623 samples have been written,

I'll have to correct myself. Not only does it uncork the stream, it also
calls pa_stream_trigger right after the first write. So this behavior is
obviously on purpose, which probably means that changing the behavior is
not unlikely to cause regressions somewhere else. :-(

What about the underruns then? I guess we also have a problem in that they
are delivered asynchronously, i e there are buffers in queue from ALSA to
PA which are about to end the underrun, at the same time as the underrun
is delivered back to ALSA. Can we call pa_update/get_timing_info from the
underrun callback to determine whether that is the case, and if so, just
ignore the underrun?

// David


___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] pulse: Fix invalid buffer pointer return value

2010-01-10 Thread David Henningsson 
David Henningsson wrote:
 'Twas brillig, and David Henningsson at 09/01/10 09:00 did gyre and
 gimble:
 The pulse ALSA plugin has been known, for a while, to not work properly,
 causing underruns, hangs etc. I sat down yesterday trying to figure it
 out, and I'm pretty certain this patch improves the situation, but I
 don't mind getting some help testing it before it is committed upstream.

 Related bug: https://bugs.launchpad.net/bugs/485488

 The patch is for the alsa-plugins tree at git.alsa-projects.org.
 I applied this to 1.0.22 (cleanly) and sadly it's not helping my test
 case using mpg123.
 
 I've tried to debug the mpg123 issue a little further.

To sum up, I have two solutions/workarounds which both (independently)
fixes the mpg123 issue, but I could use some discussion regarding we
should apply either or both. And as said earlier, this is something
different from the patch posted earlier.

First, we can avoid the initial underrun altogether by avoiding calling
pa_stream_cork() and pa_stream_trigger() after having written just a few
bytes. If we do that, playback will start when prebuf number of bytes
have been written instead.

Second, we can ignore underruns if more buffers have been pushed since
the underrun occurred. Assuming the first fix is not applied, this will
cause an initial - to me inaudible - underrun, but since more buffers
are pushed in queue, the underrun will end automatically and after that
the stream will hopefully flow the way it should.

Both fixes can, at least in theory, cause regressions. (And according to
both Murphy's law and own experience - if it breaks in theory, it
probably breaks in practice as well!) The first fix could cause a
regression for short event sounds where we never reach the prebuf limit.
The second could perhaps cause regressions for ALSA clients
automatically increasing buffer size if they find an underrun. Hiding
that underrun would then be a bad thing.

// David


___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] pulse: Fix invalid buffer pointer return value

2010-01-10 Thread Colin Guthrie 
'Twas brillig, and David Henningsson at 10/01/10 18:35 did gyre and gimble:
 David Henningsson wrote:
 'Twas brillig, and David Henningsson at 09/01/10 09:00 did gyre and
 gimble:
 The pulse ALSA plugin has been known, for a while, to not work properly,
 causing underruns, hangs etc. I sat down yesterday trying to figure it
 out, and I'm pretty certain this patch improves the situation, but I
 don't mind getting some help testing it before it is committed upstream.

 Related bug: https://bugs.launchpad.net/bugs/485488

 The patch is for the alsa-plugins tree at git.alsa-projects.org.
 I applied this to 1.0.22 (cleanly) and sadly it's not helping my test
 case using mpg123.

 I've tried to debug the mpg123 issue a little further.
 
 To sum up, I have two solutions/workarounds which both (independently)
 fixes the mpg123 issue, but I could use some discussion regarding we
 should apply either or both. And as said earlier, this is something
 different from the patch posted earlier.
 
 First, we can avoid the initial underrun altogether by avoiding calling
 pa_stream_cork() and pa_stream_trigger() after having written just a few
 bytes. If we do that, playback will start when prebuf number of bytes
 have been written instead.
 
 Second, we can ignore underruns if more buffers have been pushed since
 the underrun occurred. Assuming the first fix is not applied, this will
 cause an initial - to me inaudible - underrun, but since more buffers
 are pushed in queue, the underrun will end automatically and after that
 the stream will hopefully flow the way it should.
 
 Both fixes can, at least in theory, cause regressions. (And according to
 both Murphy's law and own experience - if it breaks in theory, it
 probably breaks in practice as well!) The first fix could cause a
 regression for short event sounds where we never reach the prebuf limit.
 The second could perhaps cause regressions for ALSA clients
 automatically increasing buffer size if they find an underrun. Hiding
 that underrun would then be a bad thing.

Awesome work, thanks for looking into this in such depth.

Lennart is the best person to comment on this as I'm nowhere near
qualified. Hopefully he'll do a purge of his ML queue soon and provide
some feedback :)

Col

-- 

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

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

___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] [PATCH] pulse: Fix invalid buffer pointer return value

2010-01-09 Thread David Henningsson 
The pulse ALSA plugin has been known, for a while, to not work properly,
causing underruns, hangs etc. I sat down yesterday trying to figure it
out, and I'm pretty certain this patch improves the situation, but I
don't mind getting some help testing it before it is committed upstream.

Related bug: https://bugs.launchpad.net/bugs/485488

The patch is for the alsa-plugins tree at git.alsa-projects.org.

// David

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

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

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

diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c
index 02a837e..3776af5 100644
--- a/pulse/pcm_pulse.c
+++ b/pulse/pcm_pulse.c
@@ -90,6 +90,10 @@ static int update_ptr(snd_pcm_pulse_t *pcm)
 	if (pcm-io.stream == SND_PCM_STREAM_CAPTURE)
 		size -= pcm-offset;
 
+	/* Prevent accidental overrun of the fake ringbuffer */
+	if (size = pcm-buffer_attr.tlength) 
+		size = pcm-buffer_attr.tlength-1;
+
 	if (size  pcm-last_size) {
 		pcm-ptr += size - pcm-last_size;
 		pcm-ptr %= pcm-buffer_attr.tlength;
@@ -424,6 +428,7 @@ static snd_pcm_sframes_t pulse_write(snd_pcm_ioplug_t * io,
 	snd_pcm_pulse_t *pcm = io-private_data;
 	const char *buf;
 	snd_pcm_sframes_t ret = 0;
+	size_t writebytes;
 
 	assert(pcm);
 
@@ -445,13 +450,15 @@ static snd_pcm_sframes_t pulse_write(snd_pcm_ioplug_t * io,
 	(char *) areas-addr + (areas-first +
 areas-step * offset) / 8;
 
-	ret = pa_stream_write(pcm-stream, buf, size * pcm-frame_size, NULL, 0, 0);
+	writebytes = size * pcm-frame_size;
+	ret = pa_stream_write(pcm-stream, buf, writebytes, NULL, 0, 0);
 	if (ret  0) {
 		ret = -EIO;
 		goto finish;
 	}
 
 	/* Make sure the buffer pointer is in sync */
+	pcm-last_size -= writebytes;
 	ret = update_ptr(pcm);
 	if (ret  0)
 		goto finish;
@@ -528,6 +535,7 @@ static snd_pcm_sframes_t pulse_read(snd_pcm_ioplug_t * io,
 
 		dst_buf = (char *) dst_buf + frag_length;
 		remain_size -= frag_length;
+		pcm-last_size -= frag_length;
 	}
 
 	/* Make sure the buffer pointer is in sync */
@@ -730,6 +738,11 @@ static int pulse_prepare(snd_pcm_ioplug_t * io)
 	pcm-offset = 0;
 	pcm-underrun = 0;
 
+	/* Reset fake ringbuffer */
+	pcm-last_size = 0;
+	pcm-ptr = 0;
+	update_ptr(pcm);
+
   finish:
 	pa_threaded_mainloop_unlock(pcm-p-mainloop);
 
-- 
1.6.5

___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] pulse: Fix invalid buffer pointer return value

2010-01-09 Thread Colin Guthrie 
'Twas brillig, and David Henningsson at 09/01/10 09:00 did gyre and gimble:
 The pulse ALSA plugin has been known, for a while, to not work properly,
 causing underruns, hangs etc. I sat down yesterday trying to figure it
 out, and I'm pretty certain this patch improves the situation, but I
 don't mind getting some help testing it before it is committed upstream.
 
 Related bug: https://bugs.launchpad.net/bugs/485488
 
 The patch is for the alsa-plugins tree at git.alsa-projects.org.

I applied this to 1.0.22 (cleanly) and sadly it's not helping my test
case using mpg123.

I'm sure you've discussed the problem with Daniel (who pointed me at
mpg123 in the first place), but I do still get crazy startups. It seems
to be pretty stable after startup tho', so it's maybe just an
initialisation issue?

Before the patch mpg123 would hang once in a while. I've tried to get it
to do the same after the patch, but so far it's not hung so I think
after the initial startup it does seem to stablise better than before.

Now there are of course potential bugs in mpg123's alsa implem. I've not
looked at this at all - just using it blindly as a test case.

HTHs

Col

-- 

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

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

___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss