[pulseaudio-discuss] Merging soxr

[Alexander E. Patrakov]

06.01.2015 19:17, Andrey Semashev wrote:
> On Wednesday 19 November 2014 00:58:04 Alexander E. Patrakov wrote:
>> 19.11.2014 00:42, Andrey Semashev wrote:
>>> On Tuesday 18 November 2014 21:32:53 Alexander E. Patrakov wrote:
>>>> 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.
>>>
>>> So, what will be the resolution of this problem? Should I work towards
>>> relaxing the requirement on pa_resampler_request() being precise or is
>>> this
>>> requirement permanent?
>>
>> I think that the temporary resolution would be to add a loop that calls
>> pa_resampler_run repeatedly. IOW, the loop that David assumed as
>> existing but which actually doesn't exist in pa_sink_input_peek().
>
> I finally got some time to dive into the code, sorry for the delay.
>
> As far as I understand the code, the loop is already there in
> pa_sink_input_peek() (see sink-input.c:924, "while (tchunk.length > 0)"). The
> outer loop (sink-input.c:893, "while (!pa_memblockq_is_readable(i-
>> thread_info.render_memblockq))") will end when either render_memblockq is not
> empty or the sink input is drained; in the latter case render_memblockq can be
> empty.

The "while (tchunk.length > 0)" loop calls the resampler until it eats 
all of the available samples in tchunk. This looks insufficient, because 
the testcase quoted above relies on the condition that the resampler 
eats all of the available samples in tchunk (thus ending the loop), but 
produces nothing.

>
> However, render_memblockq is initialized with a silence chunk in
> pa_sink_input_new(), which means that when the queue is empty it should return
> silence. This means that pa_memblockq_peek() in sink-input.c:993 and the
> following asserts should never fail. Same for the asserts in fill_mix_info(),
> the loop should be cut short by pa_memblock_is_silence(). "pa_assert(info-
>> chunk.memblock);" could be moved upper though, but it it doesn't matter for
> the case in point.

So, instead of an assertion failure that I predicted, we get a chunk of 
silence regularly inserted into the middle of the low-latency stream 
being resampled. Still bad. Thanks for correcting my logic, though.

>
> Am I missing something?

-- 
Alexander E. Patrakov