[pulseaudio-discuss] [PATCH v6 24/25] loopback: Add adjust_threshold_usec parameter

[Tanu Kaskinen]

On Sun, 2016-06-05 at 21:05 +0200, Georg Chini wrote:
> In most situations, the P-controller is too sensitive and therefore exhibits rate hunting.
> To avoid rate hunting, the sensibility of the controller is set by the new parameter
> adjust_threshold_usec. The parameter value is the deviation from the target latency in usec
> which is needed to produce a 1 Hz deviation from the optimum sample rate.
> The default is set to 200 usec, which should be sufficient in most cases.

I don't follow this explanation. First you seem to define the threshold
value as something that is derived from the optimum sample rate, but
then you say that there's a fixed default value that obviously has
nothing to do with the optimum sample rate.

What are the cases where the default threshold is bad? How is the user
expected to figure out what the threshold should be?

> For details of the implementation, refer to "rate_estimator.odt".
> 
> ---
>  src/modules/module-loopback.c | 42 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 34 insertions(+), 8 deletions(-)
> 
> diff --git a/src/modules/module-loopback.c b/src/modules/module-loopback.c
> index 35a817b..f4e2c2f 100644
> --- a/src/modules/module-loopback.c
> +++ b/src/modules/module-loopback.c
> @@ -47,6 +47,7 @@ PA_MODULE_USAGE(
>          "sink=<sink to connect to> "
>          "adjust_time=<how often to readjust rates in s, values >= 100 are in ms> "
>          "latency_msec=<latency in ms> "
> +        "adjust_threshold_usec=<threshold for latency adjustment in usec> "
>          "low_device_latency=<boolean, use half of the normal device latency> "
>          "format=<sample format> "
>          "rate=<sample rate> "
> @@ -62,6 +63,8 @@ PA_MODULE_USAGE(
>  
>  #define FILTER_PARAMETER 0.125
>  
> +#define DEFAULT_ADJUST_THRESHOLD_USEC 200
> +
>  #define MEMBLOCKQ_MAXLENGTH (1024*1024*32)
>  
>  #define MIN_DEVICE_LATENCY (2.5*PA_USEC_PER_MSEC)
> @@ -97,6 +100,7 @@ struct userdata {
>  
>      /* Values from command line configuration */
>      pa_usec_t latency;
> +    uint32_t adjust_threshold;
>      pa_usec_t adjust_time;
>      bool low_device_latency;
>  
> @@ -151,6 +155,7 @@ static const char* const valid_modargs[] = {
>      "sink",
>      "adjust_time",
>      "latency_msec",
> +    "adjust_threshold_usec",
>      "low_device_latency",
>      "format",
>      "rate",
> @@ -213,10 +218,9 @@ static void teardown(struct userdata *u) {
>  }
>  
>  /* rate controller
> - * - maximum deviation from base rate is less than 1%
> - * - controller step size is limited to 2.01‰
> + * - maximum deviation from optimum rate for P-controller is less than 1%
> + * - P-controller step size is limited to 2.01‰
>   * - implements adaptive re-sampling
> - * - exhibits hunting with USB or Bluetooth sources
>   */

Am I right that we have two independent rate controllers (one of which
you call "P-controller" and one that doesn't have a name), and we
choose between those in every iteration? That is, the unnamed
controller produces new_rate_1 and the P-controller produces
new_rate_2. The overview comments should be more clear about this, and
I'd like to also have some characterization of the two controllers
(explain their behaviour, strengths and weaknesses in a general level).
Moving the two controllers in their own functions could be helpful too
in making the code easier to follow.

>  static uint32_t rate_controller(
>                  struct userdata *u,
> @@ -225,7 +229,21 @@ static uint32_t rate_controller(
>                  int32_t latency_difference_usec_2) {
>  
>      double new_rate_1, new_rate_2, new_rate;
> -    double min_cycles_1, min_cycles_2, drift_rate, latency_drift;
> +    double min_cycles_1, min_cycles_2, drift_rate, latency_drift, controller_weight, min_weight;
> +    uint32_t base_rate_with_drift;
> +
> +    base_rate_with_drift = (int)(base_rate + u->drift_compensation_rate);
> +
> +    /* if we are less than 2‰ away from the base_rate, lower weight of the

Is base_rate_with_drift the same thing as the "optimum rate"? If it is,
wouldn't "optimum_rate" be a better variable name than
"base_rate_with_drift"? Also, the code compares the old rate to
base_rate_with_drift, not base_rate, so the comment is confusing.

> +     * P-controller. The weight is determined by the fact that a correction
> +     * of 0.5 Hz needs to be applied by the controller when the latency
> +     * difference gets larger than the threshold. The weight follows
> +     * from the definition of the controller. The minimum will only
> +     * be reached when one adjust threshold away from the target. */
> +    controller_weight = 1;
> +    min_weight = PA_CLAMP(0.5 / (double)base_rate * (100.0 + (double)u->real_adjust_time / u->adjust_threshold), 0, 1.0);
> +    if ((double)abs((int)(old_rate - base_rate_with_drift)) / base_rate_with_drift < 0.002)
> +        controller_weight = PA_CLAMP((double)abs(latency_difference_usec) / u->adjust_threshold * min_weight, min_weight, 1.0);
>  
>      /* Calculate next rate that is not more than 2‰ away from the last rate */
>      min_cycles_1 = (double)abs(latency_difference_usec) / u->real_adjust_time / 0.002 + 1;

-- 
Tanu