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
