On Wed, 2015-11-18 at 18:27 +0100, Georg Chini wrote:
> On 18.11.2015 15:10, Tanu Kaskinen wrote:
> > On Wed, 2015-02-25 at 19:43 +0100, Georg Chini wrote:
> > > Move the timer restart to the beginning of the callback function.
> > Please try to remember to always answer the question "why" in commit
> > messages.
>
> I will in the future ...
>
> >
> > > ---
> > > Â src/modules/module-loopback.c | 14 ++++++++------
> > > Â 1 file changed, 8 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/src/modules/module-loopback.c b/src/modules/module-loopback.c
> > > index f9255ee..79bd106 100644
> > > --- a/src/modules/module-loopback.c
> > > +++ b/src/modules/module-loopback.c
> > > @@ -196,9 +196,6 @@ static void adjust_rates(struct userdata *u) {
> > > Â Â Â Â Â Â pa_assert(u);
> > > Â Â Â Â Â Â pa_assert_ctl_context();
> > > Â
> > > -Â Â Â Â pa_asyncmsgq_send(u->sink_input->sink->asyncmsgq, PA_MSGOBJECT(u->sink_input), SINK_INPUT_MESSAGE_LATENCY_SNAPSHOT, NULL, 0, NULL);
> > > -Â Â Â Â pa_asyncmsgq_send(u->source_output->source->asyncmsgq, PA_MSGOBJECT(u->source_output), SOURCE_OUTPUT_MESSAGE_LATENCY_SNAPSHOT, NULL, 0, NULL);
> > > -
> > > Â Â Â Â Â Â /* Rates and latencies*/
> > > Â Â Â Â Â Â old_rate = u->sink_input->sample_spec.rate;
> > > Â Â Â Â Â Â base_rate = u->source_output->sample_spec.rate;
> > > @@ -236,8 +233,6 @@ static void adjust_rates(struct userdata *u) {
> > > Â Â Â Â Â Â /* Set rate */
> > > Â Â Â Â Â Â pa_sink_input_set_rate(u->sink_input, new_rate);
> > > Â Â Â Â Â Â pa_log_debug("[%s] Updated sampling rate to %lu Hz.", u->sink_input->sink->name, (unsigned long) new_rate);
> > > -
> > > -Â Â Â Â pa_core_rttime_restart(u->core, u->time_event, pa_rtclock_now() + u->adjust_time);
> > > Â }
> > > Â
> > > Â /* Called from main context */
> > > @@ -248,6 +243,13 @@ static void time_callback(pa_mainloop_api *a, pa_time_event *e, const struct tim
> > > Â Â Â Â Â Â pa_assert(a);
> > > Â Â Â Â Â Â pa_assert(u->time_event == e);
> > > Â
> > > +Â Â Â Â /* Restart timer right away */
> > > +Â Â Â Â pa_core_rttime_restart(u->core, u->time_event, pa_rtclock_now() + u->adjust_time);
> > If the goal of this change is to make the wakeups happen exactly with
> > adjust_time intervals instead of adjust_time + time taken in servicing
> > the timer interrupt, wouldn't it be better to have a counter of the
> > timer events and use start_time + counter * adjust_time as the time
> > value here? That way the average interval should become exactly
> > adjust_time.
>
> Yes, the goal is to minimize the influence of the interrupt service time.
> It is more important that two consecutive interrupts are spaced as
> exactly as possible by adjust_time, than that the average time interval
> is correct.
Why is that so? I don't know what problem the patch is trying to solve,
so I can't judge myself which quality is more important. (Note also
that if the scheduling delay is constant, my method is more accurate in
both senses. If the delay is jittery, like it probably is, your method
may have lower average error.)
Obviously the patch improves the accuracy from the status quo, so I'm
not against applying it, and I also expect that choosing between the
two methods doesn't make meaningful difference, so maybe discussing
this further is pointless...
> So I believe the best way is to restart the timer as early as
> possible in the interrupt routine.
>Â
> > > +
> > > +Â Â Â Â /* Get sink and source latency snapshot */
> > > +Â Â Â Â pa_asyncmsgq_send(u->sink_input->sink->asyncmsgq, PA_MSGOBJECT(u->sink_input), SINK_INPUT_MESSAGE_LATENCY_SNAPSHOT, NULL, 0, NULL);
> > > +Â Â Â Â pa_asyncmsgq_send(u->source_output->source->asyncmsgq, PA_MSGOBJECT(u->source_output), SOURCE_OUTPUT_MESSAGE_LATENCY_SNAPSHOT, NULL, 0, NULL);
> > > +
> > > Â Â Â Â Â Â adjust_rates(u);
> > > Â }
> > > Â
> > > @@ -257,7 +259,7 @@ static void enable_adjust_timer(struct userdata *u, bool enable) {
> > > Â Â Â Â Â Â Â Â Â Â if (u->time_event || u->adjust_time <= 0)
> > > Â Â Â Â Â Â Â Â Â Â Â Â Â Â return;
> > > Â
> > > -Â Â Â Â Â Â Â Â u->time_event = pa_core_rttime_new(u->module->core, pa_rtclock_now() + u->adjust_time, time_callback, u);
> > > +Â Â Â Â Â Â Â Â u->time_event = pa_core_rttime_new(u->module->core, pa_rtclock_now() + 333 * PA_USEC_PER_MSEC, time_callback, u);
> > What is this about?
> >
> Well, actually the patch does two things: One is to move the timer
> restart so that
> the interval between calls is as constant and as near to adjust_time as
> possible, the
> other is starting the regulation as soon as possible.
Ok. I think these two changes should be in separate patches.
> In the current
> version of module
> loopback  - after start up or a sink / source switch - the interrupt
> routine is called the
> first time after adjust_time has passed. With the default adjust time
> this means 10
> seconds before the latency regulation kicks in, which can lead to
> underruns especially
> after switching source or sink.
> Replacing adjust_time by one third of a second gives the source/sink
> enough time to settle
> down to report reliable latency values and starts the regulation process
> as early as
> possible (thereby also resetting any previous deviations from the base
> rate).
The code should have a comment about how the magic number was chosen. I
don't require the selection methodology to be rigorous, but it should
be documented. So if you only compared ten seconds and 1/3 seconds,
mention that.
--Â
Tanu
