[pulseaudio-discuss] [PATCH 06/13] loopback: Restart the timer right away

[Georg Chini]

On 19.11.2015 23:02, Tanu Kaskinen wrote:
> 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...
>
>
I tested it, the two methods are more or less equivalent. I also found that
the precision of the interval is about 0.1 percent in both cases.