[pulseaudio-discuss] Merging soxr

[Alexander E. Patrakov]

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