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
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
