[pulseaudio-discuss] [PATCH v6 17/25] loopback: Track and use average adjust time

[Tanu Kaskinen]

On Sun, 2016-06-05 at 21:05 +0200, Georg Chini wrote:
> > @@ -79,11 +79,18 @@ struct userdata {
>  
>      pa_time_event *time_event;
>  
> +    /* Variables used to calculate the average time between
> +     * subsequent calls of adjust_rates() */
> +    pa_usec_t time_stamp;

"time_stamp" is quite generic name. I'd prefer
"last_adjustment(_time_stamp)" or something like that.

> +    pa_usec_t real_adjust_time;
> +    pa_usec_t real_adjust_time_sum;
> +
>      /* Various counters */
>      int64_t recv_counter;
>      int64_t send_counter;
>      uint32_t iteration_counter;
>      uint32_t underrun_counter;
> +    uint32_t adjust_counter;

iteration_counter and adjust_counter can be merged to just one
counter. 

The counters have two differences. One difference is that
iteration_counter counts all adjust_rate() calls, while adjust_counter
skips the first call. The other difference is that iteration_counter is
reset when streams move or device latency changes, while adjust_counter
is not.

iteration_counter is used for calculating the time since the last reset
(stream move or device latency change), but that calculation ends up
being wrong, because the first adjust_rate() call is done much quicker
than the other calls. If iteration_counter would also skip the first
adjust_rate() call, the calculation would be more correct, although
still wrong, because the short first interval isn't included in the
calculation.

The other difference between the counters is that iteration_counter is
reset when streams move or device latency changes, while adjust_counter
is not reset. However, I don't see any harm in resetting adjust_counter
too.

So, in conclusion, both differences can be removed, and then only one
counter will be needed.

> @@ -232,11 +239,21 @@ static void adjust_rates(struct userdata *u) {
>      }
>  
>      /* Allow one underrun per hour */
> -    if (u->iteration_counter * u->adjust_time / PA_USEC_PER_SEC / 3600 > hours) {
> +    if (u->iteration_counter * u->real_adjust_time / PA_USEC_PER_SEC / 3600 > hours) {
>          pa_log_info("Underrun counter: %u", u->underrun_counter);
>          u->underrun_counter = PA_CLIP_SUB(u->underrun_counter, 1u);
>      }
>  
> +    /* Calculate real adjust time */
> +    if (u->source_sink_changed)
> +        u->time_stamp = pa_rtclock_now();
> +    else {
> +        u->adjust_counter++;
> +        u->real_adjust_time_sum += pa_rtclock_now() - u->time_stamp;
> +        u->time_stamp = pa_rtclock_now();
> +        u->real_adjust_time = u->real_adjust_time_sum / u->adjust_counter;
> +    }

u->time_stamp = pa_rtclock_now() is done in both branches, so it can be
moved out. Also, calling pa_rtclock_now() twice results in some timy
error in the calculations. I'd change this to:

now = pa_rtclock_now()

if (!u->source_sink_changed) {
    u->adjust_counter++;
    u->real_adjust_time_sum = now - u->time_stamp;
    u->real_adjust_time = u->real_adjust_time_sum / u->adjust_counter;
}

u->time_stamp = now;

-- 
Tanu