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
