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().
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?
Well, that statement is only true for library-based resamplers. And my
viewpoint here (which is an extremist viewpoint not shared by other
developers) is: all library-based resamplers must eventually die, unless
a 100% working one appears. One of the reasons is that none of them
implement rewinding, the other reason is pa_resampler_request() being
imprecise (exactly because the precise version would be specific for
each resampler). So, my viewpoint is that PulseAudio should have its own
resampler, not based on any library, but based on the best-of-the-breed
algorithms, that implements fast high-quality resampling, in a
rewindable way and with precise estimation of the number of input
samples needed to produce the given number of output samples, and the
other way round.
Yes, I think we must invent the wheel, because all existing stock wheels
are square.
--
Alexander E. Patrakov
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
