Re: [pulseaudio-discuss] [PATCH v6 17/25] loopback: Track and use average adjust time
On Sat, 2016-08-20 at 14:55 +0200, Georg Chini wrote:
> 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.
Ah, indeed, so for this part iteration_counter and adjust_counter
behave identically.
> > 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.
Well, changing the sink or source of a loopback is rare, I believe, and
I also believe that the harm from the loss of precision is negligible.
But if you think that the precision issue is grave enough to justify
another counter, then ok, let's leave that in.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v6 17/25] loopback: Track and use average adjust time
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.
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v6 17/25] loopback: Track and use average adjust time
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
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
