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

[David Henningsson]

David Henningsson wrote:
> 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.web at 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

Just to clarify. There are currently two patches in alsa-plugins which
I've written and which are applied to Ubuntu. The first one corrects an
error in calculating space in the emulated mmap buffer, and that is what
the subject of this email is referring to. Without the patch, in some
cases this could lead to random hangs inside alsa-lib/alsa-plugins,
waiting indefinitely for buffer space to become available. It could also
cause repeatedly underruns in low-latency applications.

The second one removes all underrun handling, which fixes the mpg123
buffer drops on startup.

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

// David