[pulseaudio-discuss] [PATCH 12/13] loopback: Validate the rate parameter

[Georg Chini]

On 18.12.2015 09:07, Tanu Kaskinen wrote:
> On Fri, 2015-12-18 at 07:47 +0100, Georg Chini wrote:
>> On 18.12.2015 06:49, Tanu Kaskinen wrote:
>>> On Wed, 2015-02-25 at 19:43 +0100, Georg Chini wrote:
>>>> ---
>>>>     src/modules/module-loopback.c | 5 +++++
>>>>     src/pulse/sample.c            | 5 ++++-
>>>>     2 files changed, 9 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/modules/module-loopback.c b/src/modules/module-loopback.c
>>>> index d22afb5..7474ef2 100644
>>>> --- a/src/modules/module-loopback.c
>>>> +++ b/src/modules/module-loopback.c
>>>> @@ -1039,6 +1039,11 @@ int pa__init(pa_module *m) {
>>>>             goto fail;
>>>>         }
>>>>     
>>>> +    if (ss.rate < 4000 || ss.rate > PA_RATE_MAX) {
>>>> +        pa_log("Invalid rate specification, valid range is 4000 Hz to %i Hz", PA_RATE_MAX);
>>>> +        goto fail;
>>>> +    }
>>>> +
>>> I pushed this change...
>>>
>>>>         if (pa_modargs_get_value(ma, "format", NULL))
>>>>             format_set = true;
>>>>     
>>>> diff --git a/src/pulse/sample.c b/src/pulse/sample.c
>>>> index 1331c72..69ac83f 100644
>>>> --- a/src/pulse/sample.c
>>>> +++ b/src/pulse/sample.c
>>>> @@ -106,7 +106,10 @@ int pa_sample_format_valid(unsigned format) {
>>>>     }
>>>>     
>>>>     int pa_sample_rate_valid(uint32_t rate) {
>>>> -    return rate > 0 && rate <= PA_RATE_MAX;
>>>> +    /* The extra 1% is due to module-loopback: it temporarily sets
>>>> +     * a higher-than-nominal rate to get rid of excessive buffer
>>>> +     * latency */
>>>> +    return rate > 0 && rate <= PA_RATE_MAX * 101 / 100;
>>> ...but I left this one out. The two changes fix two separate issues, so
>>> they should be in separate patches.
>>>
>>> Making pa_sample_rate_valid() accept values above PA_RATE_MAX isn't
>>> very nice, but I can see how it's better than the current behaviour,
>>> which probably can cause the daemon to crash. Alexander mentioned in
>>> IRC that he'd prefer doing the adaptive resampling inside module-
>>> loopback rather than using pa_sink_input_set_rate(), and this change
>>> demonstrates one good reason for that. I'm not asking you to implement
>>> that change, but it certainly would be nice.
>> Yes, doing it inside the module would really be a good idea. The
>> main issue my controller is fighting against are the systematic
>> errors introduced by changing the rate of the sink input. When
>> you change the rate, the next latency report has an extremely
>> high error.
>> Can you point me to some code that implements resampling
>> so that I have an example of what to do?
> Good that you asked, because it got me thinking about this a bit more,
> and I'm starting to think that it's not worth it after all. Adding
> another resampler would in many situations mean that the audio would
> get resampled twice (or three times, if we count the source ouput
> resampler, but that's beside the point). That doesn't seem like a good
> idea due to the extra cpu load.
>
> Something that could be considered is to have separate "nominal rate"
> and "real rate" stored in pa_sink_input, so that the small rate tweaks
> that module-loopback applies wouldn't be visible where they don't need
> to be visible, and pa_sample_rate_valid() wouldn't have to accept rates
> higher than PA_RATE_MAX.
>
> (If you anyway want to see code that does resampling, grep for
> "pa_resampler_run".)
>
> -- 
> Tanu
Wouldn't it avoid additional resampling when you set the sink_input rate
to the sink rate?