[pulseaudio-discuss] [PATCH v6 19/25] loopback: Implement adaptive re-sampling

[Tanu Kaskinen]

On Sun, 2016-06-05 at 21:05 +0200, Georg Chini wrote:
> To account for the difference between source and sink time domains, adaptive
> re-sampling is implemented. This massively improves latency stability. Details
> of the controller implementation can be found in "rate_estimator.odt".

I don't know whose definition is wrong, but to me "adaptive resampling"
means just dynamically adjusting the resampling to make sure that there
aren't buffer under- or overruns due to clock rate differences, and the
existing code already satisfies this. Therefore, it looks wrong to me
to say that this patch "implements adaptive resampling", as if that
wasn't already implemented.

>  /* rate controller
>   * - maximum deviation from base rate is less than 1%
>   * - controller step size is limited to 2.01‰
> + * - implements adaptive re-sampling
>   * - exhibits hunting with USB or Bluetooth sources
>   */
>  static uint32_t rate_controller(
>                  struct userdata *u,
>                  uint32_t base_rate, uint32_t old_rate,
> -                int32_t latency_difference_usec) {
> +                int32_t latency_difference_usec,
> +                int32_t latency_difference_usec_2) {

Please indicate what the difference between these arguments is. It
seems that the first latency difference is at the estimated optimum
rate and the second is the same latency difference at the current rate.
You can add comments or change the variable names to include the time
domain.

> @@ -220,7 +232,26 @@ static uint32_t rate_controller(
>      else
>          new_rate = new_rate_2;
>  
> -    return new_rate;
> +    /* Calculate rate difference between source and sink. Skip calculation
> +     * after a source/sink change or an underrun */
> +
> +    if (!u->underrun_occured && !u->source_sink_changed) {
> +        /* Latency difference between last iterations */
> +        latency_drift = latency_difference_usec_2 - u->last_latency_difference;
> +
> +        /* Calculate frequency difference between source and sink */
> +        drift_rate = (double)old_rate * latency_drift / u->real_adjust_time + old_rate - base_rate;

I believe the (double) cast is not necessary, because latency_drift is
already a double.

> +
> +        /* 2nd order lowpass filter */
> +        u->drift_filter = (1 - FILTER_PARAMETER) * u->drift_filter + FILTER_PARAMETER * drift_rate;
> +        u->drift_compensation_rate =  (1 - FILTER_PARAMETER) * u->drift_compensation_rate + FILTER_PARAMETER * u->drift_filter;
> +    }
> +
> +    /* Use drift compensation. Though not likely, the rate might exceed the maximum allowed rate now. */
> +    if (new_rate + u->drift_compensation_rate + 0.5 > PA_RATE_MAX * 101 / 100)
> +        return PA_RATE_MAX * 101 / 100;
> +    else
> +        return (int)(new_rate + u->drift_compensation_rate + 0.5);

If you do

    new_rate = (int)(new_rate + u->drift_compensation_rate + 0.5);

before the if statement, you don't need to duplicate the calculation.

> @@ -296,9 +330,13 @@ static void adjust_rates(struct userdata *u) {
>      pa_log_debug("Loopback latency at base rate is %0.2f ms", (double)latency_at_optimum_rate / PA_USEC_PER_MSEC);
>  
>      /* Calculate new rate */
> -    new_rate = rate_controller(u, base_rate, old_rate, latency_difference);
> +    new_rate = rate_controller(u, base_rate, old_rate, (int)(latency_at_optimum_rate - final_latency), latency_difference);

The (int) cast should be applied to the individual terms, not to the
result of the substraction.

> @@ -742,8 +780,10 @@ static int sink_input_process_msg_cb(pa_msgobject *obj, int code, void *data, in
>              if (u->sink_input->thread_info.underrun_for > 0 &&
>                  pa_memblockq_is_readable(u->memblockq)) {
>  
> -                if (!u->source_sink_changed)
> +                if (!u->source_sink_changed) {
>                      u->underrun_counter +=1;
> +                    u->underrun_occured = true;
> +                }

underrun_occurred should be set in the main thread (you probably were
going to do that anyway in your next version, but I'll note this here
anyway so that I remember to check this when reviewing the next
version).

-- 
Tanu