[pulseaudio-discuss] [PATCH v4] Make module loopback honor requested latency

Georg Chini georg at chini.tk
Sun Feb 8 03:27:37 PST 2015

>>>> in the case of timer-based scheduling (where even module-alsa-sink
>>>> does not trust the result, i.e. discards it if it is greater than the
>>>> non-transformed time interval). And, if I recollect correctly, there
>>>> were complaints about it being fooled by batch cards, and they were
>>>> cited as one of the reasons not to enable timer-based scheduling on
>>>> batch cards. So - maybe, for the purposes of timer based-scheduling we
>>>> should just assume the worst case, i.e. the card that is, say, 0.75%
>>>> faster than nominal, and use the nominal rate together with the latest
>>>> snapshot time in {source,sink}_get_latency()? Basically, the fear is
>>>> that the smoother makes a greater mistake in the estimated rate than
>>>> just assuming the nominal one. Maybe you can try this suggestion?
>>> For timer based scheduling the regulator works perfect, you would not
>>> even need a stop criterion,
>>> so why bother?
> I think there is some misunderstanding. Let me repeat in a different way.
> The smoother works perfectly (both for timer-based scheduling and for 
> the needs of your module) on non-batch cards.
> But, even for batch cards, where timer-based scheduling is disabled, 
> the smoother is active and is actually used for reporting the latency 
> to your module. An attempt to use the smoother for timer-based 
> scheduling on batch cards has failed. That's why I suspect that it, on 
> batch cards, also tells lies to your module.

OK, understood. I don't have anything to test it though.

>>>> For Tanu's patch status page: please leave the status of this patch as
>>>> unreviewed. The general idea of the patch does not look brilliant, but
>>>> it's the best known-working idea that we currently have on the topic,
>>>> and I have not reviewed all the fine details.
>>> Well from a practical point of view it does a pretty good job although
>>> the idea may not be brilliant.
>>> I'm willing to implement your better idea when you come up with it.
>>>   Did you ever test it? And compare it to what the current
>>> module-loopback does?
>> I did not test it, will do it now and add some logging in order to
>> verify what you said above. And hopefully will try to implement an
>> alternative latency-snapshotting implementation, just to compare.
> I can confirm (based on a reimplementation attempt) that the code 
> after patching deals with the capture and playback timestamp 
> difference 100% correctly - so it cannot be the problem. Just a minor 
> nitpick: I moved saving of the timestamp to the message handlers. For 
> me, this makes no difference, though. The patch (to be applied on top 
> of yours) is attached. Could you please confirm or disprove that it 
> makes no difference in your setup, either?

I tried the same and measured the time difference between the two 
methods. It is around 1 - 2 us.
So it does not really matter if you obtain the time stamp within the 
snapshot or outside.
I thought it was more simple the way I implemented it, but I have no 
objection to your change.

> So, the current status of the patch, from my viewpoint, is:
> 1. The patch adds a perfectly correct (assuming no xruns) way to 
> account for latency snapshots being made not simultaneously for 
> playback and capture. I think that this is the main improvement, and 
> it needs to be merged even if we disagree on the rest.
> 2. The result has an optimal coefficient that relates the observed 
> latency difference and the resulting rate correction, assuming the 
> currently-implemented way to snapshot the latency and assuming no 
> interference from the smoother - which still has to be verified 
> independently, possibly after merging.
> 3. The patch adds buffer_latency_msec, which seems to be an unrelated 
> improvement, and I think it should be split out. I have no opinion on 
> whether this change should be merged.

I'm fine with separating this out. I am even fine with dropping it 
completely, but I thought
it might be useful for those who know what they are doing and for those 
who have to care
about used CPU cycles.

> 4. The patch has a criterion when to stop adjusting rates, and it is a 
> source of disagreements. But I could not suggest anything 
> constructive. So I think that a good approach would be to split it out 
> and let others comment. Also, it would be a good idea to add a 
> debugging message so that we can see when it happens.

It nearly always happens when you are approaching the requested latency. 
The case where
you hit the base rate spot on is very rare, so a message doesn't make 
sense for me.
Maybe it would make sense to print out the latency_error somewhere 
because it is an
indicator how good your latency can be adjusted. Without stop criterion 
you will see instabilities
with USB devices in certain situations, so I would not merge the patch 
without it.
Even if the regulator were completely stable in all tested situations I 
would still add something
like it, just to make sure. The current module-loopback also contains a 
stop criterion which
in my opinion is insufficient and should be replaced.

> If you want, I can do the splitting for you.

Thanks, please do so. It makes more sense when you do it because you 
better know
what you want to separate.

> _______________________________________________
> pulseaudio-discuss mailing list
> pulseaudio-discuss at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20150208/f9095de1/attachment-0001.html>

More information about the pulseaudio-discuss mailing list