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 latencies. 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 somewhere else. 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. https://launchpad.net/~diwic
