[pulseaudio-discuss] Merging soxr

[Alexander E. Patrakov]

18.11.2014 19:14, David Henningsson wrote:
>
>
> On 2014-11-18 14:46, Alexander E. Patrakov wrote:
>> Add support for libsoxr resampler: David's objection about overriding
>> pa_resampler_request is 100% valid, and the patchset cannot be merged
>> without taking it into account.
>
> Well, the result will be inoptimal rather than completely not working
> without a working pa_resampler_request (especially given that Andrey
> seems to be satisfied with the current behaviour). If we're given fewer
> samples back than we expected, we'll just go through another round of
> resampling/mixing/etc, which I assume is what happens here.

Well, now I have looked at the code in sink.c and sink-input.c, and I 
must say that I don't like it. Namely, there are assertions in 
fill_mix_info():

         pa_assert(info->chunk.memblock);
         pa_assert(info->chunk.length > 0);

At the very least, the first assertion should be moved up, because just 
above them, in the conditional statement, info->chunk.memblock is passed 
to pa_memblock_is_silence().

Also there are assertions in pa_sink_input_peek() that are very similar 
in nature, and I don't see how it is guaranteed that the assertions 
never fail.

So the devious sequence of events seems to be (assuming S16 stereo samples):

pa_sink_input_peek is called with slength == 8 or something like that.

pa_resampler_request() returns 8 or something like that.

i->pop(), when asked to provide 8 bytes, creates a memchunk (tchunk) of 
this length.

pa_resampler_run() eats the full tchunk, but produces nothing (an empty 
rchunk).

As rchunk is empty, nothing gets pushed onto render_memblockq.

Then pa_memblockq_peek() gets called, and it is asserted that the 
returned chunk exists and is not empty. Which looks dubious, and I think 
that we can try triggering this with a very-low-latency client 
(unpatched wine or maybe qemu?).

So, incorrect results from pa_resampler_request() look dangerous when 
the difference results in zero vs non-zero output samples from 
pa_resampler_run().

Of course, all of the above is in no way specific to the soxr resampler. 
An imprecise pa_resampler_request() is a bug. What bothers me is that 
soxr has a higher chance to trigger this bug.

-- 
Alexander E. Patrakov