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?
