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

[Tanu Kaskinen]

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