19.11.2014 10:49, David Henningsson wrote:
On 2014-11-18 20:58, 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().
Well, there is at least a loop in pa_sink_render_into_full. But if there
are assertions on the way back to that loop, it would make sense to
remove those assertions based on the fact that it would make it possible
to support resamplers with non-predictable output.
Then we also need to refactor fill_mix_info(): it uses 0 in mixlength as
the indicator that there is actually no value set. So, if we remove
assertions, then, if one sink input yields nothing, the next one will
overwrite the mixlength, defeating the logic that attempts to find the
minimum.
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.
If possible, it would make more sense to improve the library of our
choice (probably speex) to support our needs, instead of rewriting a new
resampler from scratch. That way, other apps using that resampler could
also benefit from our improvements to that resampler.
In general I'd agree, but that assumes that such improvements are
welcome upstream. Which is not a given. In fact, if I were an upstream
of any resampler, I'd reject such patch on the basis that pulseaudio is
the only program that needs it, and that I won't regularly test the new
functionality and won't notice if it breaks.
Still, it does make sense to ask. Will do so for speex.
--
Alexander E. Patrakov
_______________________________________________
pulseaudio-discuss mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
