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.