[pulseaudio-discuss] [PATCH 2/2] modules, pulse, core: React to failing pa_memblockq_push calls.
david.henningsson at canonical.com
Thu May 24 04:14:35 PDT 2012
On 05/24/2012 09:38 AM, Jarkko Suontausta wrote:
> This adds an assertion to pa_memblockq_push() and pa_memblockq_push_align()
> calls, the return value of which was mostly ignored in the core and modules.
> The calls return a negative value in case the maximum memblock queue size
> would be exceeded.
> The maximum queue sizes are currently set to be at least 16 megabytes, which
> corresponds to about 22 seconds of 16-bit 8-channel audio at 48 kHz, or well
> over a minute of stereo audio. If the queue gets full, it should be a sign
> of an internal error.
Just to point out, this assumption is not strictly true: maxlength for
some buffers are very settable by the client, although most clients do
not, I believe it's a way for the client to prefer underruns over higher
Hopefully all those buffers fall under the FIXME conditions below?
Remember, there are flags that make the client buffers affect other
buffers as well - PA_STREAM_ADJUST_LATENCY comes to mind, but there
might be more.
> In some cases, mostly when the output memblock queue gets full due to
> external circumstances, asserting is not feasible. Those parts of the
> code are marked with a FIXME comment instead.
Also, we don't want to crash PulseAudio on broken/misbehaving clients.
Is there a risk that if the client reads or writes half a sample, that
the first patch will cause PulseAudio to crash?
I've talked about over-usage of assertions earlier, and maybe this is
one of those cases. Sometimes it seems we accept a work flow as follows:
1) We discover that the return value is not checked, and after various
degrees of thinking it through, we add an assertion because we can't
figure out how it could fail without things being very utterly broken
2) We discover that PulseAudio starts to crash due to the added
assertion, we figure out why this actually is a proper use case for
failure and add proper error handling.
3) Everything is great, hooray!
The problem is that for the end user, the assertion is a worse
experience than the unchecked return value. I feel responsible for
shipping a stable PulseAudio to millions of Ubuntu users, so even if
something just happens once in a million, I'm going to end up seeing bug
reports for it. Also, the desktop space is more diverse than the mobile
space (that I'm assuming Digia works in), so we're more likely to have
misbehaving clients, weird module setups, and so on.
And from there to figuring out why the assertion fails, what the proper
patch etc is, that's complex and time intensive for me, especially as I
usually can not reproduce the error myself - at best, I get a stack
trace to work with.
Combine this with the fact that you say you don't have time to fix the
FIXMEs (who do you expect to do that?), and I become worried that the
level of thinking through before adding these assertions is not high
enough. Hopefully Tanu has done more thinking.
I don't want to block stuff, or be a moaner, but I can't help to wonder
if this patch is more likely to improve things or regress them... :-/
David Henningsson, Canonical Ltd.
More information about the pulseaudio-discuss