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?
