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

[Georg Chini]

On 20.08.2016 14:23, Tanu Kaskinen wrote:
> 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.

This is not really correct. When you take a look at the patch, you
can see that iteration_counter is used before it is incremented, so
the actual state is, that the first short interval is not included.
I ignored that error, because the counter is only used to calculate
the number of hours module-loopback is running under the same
conditions and I am only using the difference of the hours, so an
error of 333 ms for the first hour seems negligible.

>
> 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.

There is a reason why I am using two counters. If I would reset
adjust_counter each time source or sink changed, I would also
have to reset the adjust_time_sum. This however would mean
starting to re-calculate the average adjust time each time
source or sink changes. Since the adjust time cannot be modified
during the runtime of module-loopback, it does not make sense
to restart the calculation and would lower the precision of the
average.
On the other hand, I need a counter which reflects the number
of calls since the last change of source or sink, for the
computation of the time that has passed since the last event.

>
> 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;
>
OK.