On 19.01.2016 12:38, Tanu Kaskinen wrote:
> On Sun, 2016-01-17 at 19:07 +0100, Georg Chini wrote:
>> On 17.01.2016 17:17, Georg Chini wrote:
>>> On 17.01.2016 14:40, Tanu Kaskinen wrote:
>>>> On Sat, 2016-01-16 at 23:22 +0100, Georg Chini wrote:
>>>>> Hello,
>>>>>
>>>>> I have been working on a new version of module-loopback for quite a
>>>>> while now.
>>>>> I am also replacing the smoother in alsa-source and alsa-sink to
>>>>> achieve
>>>>> more
>>>>> precise latency reports.
>>>>> Now I hit a problem, were I am not sure how to solve it. The
>>>>> get_latency
>>>>> calls
>>>>> of alsa-sink and alsa-source truncate the latency to 0 when it becomes
>>>>> negative.
>>>>> Would it be possible to let the calls return negative values?
>>>>> Due to the error in the initial conditions those values still make
>>>>> sense
>>>>> and clipping
>>>>> them causes massive problems for the rate controller of
>>>>> module-loopback.
>>>>>
>>>>> If it is not possible to let the calls return negative values, do you
>>>>> have any other
>>>>> idea how to work around the problem?
>>>> Reporting negative latency doesn't make sense, since it implies time
>>>> travel. Where do those negative latency values come from?
>>> I know it requires time travel ...
>>> Negative values are due to the fact that you have to assume
>>> some point in time as the starting point. In my new code this
>>> is done directly while the smoother does the same more indirectly
>>> (and also delivers negative values).
>>> At that starting point, a certain number of samples have already been
>>> played or recorded. If that number has an error, you can get negative
>>> latency values (which are in the order of a few hundred usec).
> Ok. It's not necessary to assume a starting time, though, unless you
> use the system clock in the latency reports. The current implementation
> uses the system clock, and apparenty you use it too in your new code.
Yes, like the smoother I am trying to find a conversion between
system time and sound card time, though my approach is simpler
and more precise. The problem with the direct approach is that the
delays reported by the alsa devices are unreliable and it is very
difficult to work with them without having something stable to
compare to.
>
>>>> Filtering out obviously incorrect values seems appropriate for most use
>>>> cases. If you really need access to "unfiltered" values, maybe
>>>> pa_sink_get_latency() could have an "allow_negative" parameter or
>>>> something similar.
>>> Wouldn't that require changes also on the client side? Or do clients
>>> never
>>> call this function directly?
> Clients can't call pa_sink_get_latency() directly. The pa_sink_*
> functions aren't included in the client library. Even if a client would
> be misguided enough to link to the server's internal library, it
> wouldn't work, because the client runs in a different process.
>
>>> Would it perhaps make sense to add a
>>> pa_sink_get_unfiltered_latency() call?
>>>
>> Meanwhile I implemented those calls (or rather their _within_thread_
>> variants)
>> for alsa sink and source.
>> If a sink or source does not have it (like bluetooth), the "normal" latency
>> is returned.
>> Would you be OK with that approach?
> I suggested a boolean flag, because I expect that to result in less
> code duplication. If my expectation is wrong, or there's some problem
> with using the flag, then adding separate functions is fine.
>
I think it is not worth adding a flag. You would need a couple of if-clauses
that make up as much code as is duplicated. You also would have to use
type casts. From my point of view it is cleaner to have different functions.
I called them pa_{sink,source}_get_raw_latency_within_thread().
The code will go through your review anyway and if you have some strong
objection I can still change it to use a flag.
