[pulseaudio-discuss] Merging soxr

[Andrey Semashev]

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.

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.

Am I missing something?