[pulseaudio-discuss] Merging soxr

[Andrey Semashev]

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?

Out of my experience, you generally can't make a function like 
pa_resampler_request() to be always correct. Speex is rather special in the 
way it generates silence while filling, and it has constant delay. I could 
probably emulate such behavior for soxr by adding a repacketizer before the 
resampler and then adding silence samples before the first output sample is 
produced. pa_resampler_request() for soxr then could return something like 40 
ms, just to be sure you'll get some samples with this amount of input. Would 
this be an acceptable solution?