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