On 12.12.2015 17:04, Alexander E. Patrakov wrote:
> 12.12.2015 20:39, Tanu Kaskinen wrote:
>> On Sat, 2015-12-12 at 11:52 +0100, Georg Chini wrote:
>>> On 08.12.2015 21:47, Tanu Kaskinen wrote:
>>>> On Wed, 2015-02-25 at 19:43 +0100, Georg Chini wrote:
>>>>> ---
>>>>> src/modules/module-loopback.c | 18 +++++++++++++++++-
>>>>> 1 file changed, 17 insertions(+), 1 deletion(-)
>>>> The commit message should say something about why the jitter is
>>>> tracked.
>>>
>>> OK
>>>
>>>>> diff --git a/src/modules/module-loopback.c
>>>>> b/src/modules/module-loopback.c
>>>>> index cbd0ac9..b733663 100644
>>>>> --- a/src/modules/module-loopback.c
>>>>> +++ b/src/modules/module-loopback.c
>>>>> @@ -95,6 +95,8 @@ struct userdata {
>>>>>
>>>>> pa_usec_t source_latency_sum;
>>>>> pa_usec_t sink_latency_sum;
>>>>> + pa_usec_t next_latency;
>>>>> + double latency_error;
>>>>>
>>>>> bool in_pop;
>>>>> bool pop_called;
>>>>> @@ -263,15 +265,22 @@ static void adjust_rates(struct userdata *u) {
>>>>> (double) current_latency / PA_USEC_PER_MSEC,
>>>>> (double) corrected_latency / PA_USEC_PER_MSEC,
>>>>> ((double) u->latency_snapshot.sink_latency +
>>>>> current_buffer_latency + u->latency_snapshot.source_latency) /
>>>>> PA_USEC_PER_MSEC);
>>>>> - pa_log_debug("Latency difference: %0.2f ms, rate difference:
>>>>> %i Hz",
>>>>> + pa_log_debug("Latency difference: %0.2f ± %0.2f ms, rate
>>>>> difference: %i Hz",
>>>> What does "± %0.2f ms" mean? Is the real latency difference between
>>>> those bounds with 100% confidence, or less than 100% confidence?
>>>
>>> This is just an indication of the current error (with respect to the
>>> model),
>>> so not 100% confidence.
>>
>> Can you assign any particular confidence to the numbers? If not, then
>> the numbers have very little meaning... They can be useful for seeing
>> whether the jitter is higher or lower compared to some other
>> measurements, but they say very little about whether the latency
>> difference really is between those bounds.
>>
>> At the very least there should be a comment explaining how the numbers
>> should be interpreted.
>>
>>>>
>>>>> (double) latency_difference / PA_USEC_PER_MSEC,
>>>>> + (double) 2.5 * u->latency_error * final_latency /
>>>>> PA_USEC_PER_MSEC,
>>>> Why is that 2.5 there?
>>>>
>>>
>>> Good question. I think it was copied over from the definition of the
>>> deadband in the next
>>> patch. The observed errors are however in good agreement with that
>>> factor.
>>
>> What does that mean? What would be different if the factor wasn't
>> there? How would it be worse than with the magic factor?
>
I think I explained it in one of the other mails. With the factor 2.5 it
matches the observation,
meaning that when it says plus/minus 150 usec then it is highly likely
that the next measurement
will fit into that interval. I cannot give you a level of confidence
however.
> Well, the logic here is still based on the equivalent of the
> n-sigma-rule. We need to figure out whether the latency difference
> that we see can be adequately explained by "the usual noise" in the
> measurements (and thus, according to the current version of the
> patchset does not have to be corrected), or not (and thus has to be
> corrected as quickly as possible). By changing the magic number, one
> can trade one kind of errors for the other. If it is too low, then,
> quite often, the deadband logic will think "oh, we need to correct
> this latency difference", only to find out during the next cycle that
> the correction has to be undone. If it is too high, then more of the
> latency difference (due to the clock drift) will need to accumulate
> before the controller finally decides to correct that.
>
> Future versions of the patchset, if I understand the situation
> correctly, will still contain similar logic, in order to switch
> between the "correct this slowly and carefully" and "correct this as
> quickly as possible" modes.
>
No, the strength of the P-controller is only dependent on the distance
to the target
latency in the latest version. The average error is only used to modify
one parameter
of the drift controller.
