<html> <head> <meta content="text/html; charset=utf-8" http-equiv="Content-Type"> </head> <body text="#000000" bgcolor="#FFFFFF"> <blockquote cite="mid:54D738C0.4030903@gmail.com" type="cite"> <blockquote type="cite"> <blockquote type="cite"> <blockquote type="cite">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? </blockquote> For timer based scheduling the regulator works perfect, you would not even need a stop criterion, so why bother? </blockquote> </blockquote> 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. </blockquote> OK, understood. I don't have anything to test it though. <blockquote cite="mid:54D738C0.4030903@gmail.com" type="cite"> <blockquote type="cite"> <blockquote type="cite"> <blockquote type="cite">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. </blockquote> 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? </blockquote> 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. </blockquote> 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? </blockquote> 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. <blockquote cite="mid:54D738C0.4030903@gmail.com" type="cite">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. </blockquote> 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. <blockquote cite="mid:54D738C0.4030903@gmail.com" type="cite"> 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. </blockquote> 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. <blockquote cite="mid:54D738C0.4030903@gmail.com" type="cite">If you want, I can do the splitting for you. </blockquote> Thanks, please do so. It makes more sense when you do it because you better know what you want to separate. <blockquote cite="mid:54D738C0.4030903@gmail.com" type="cite"> <fieldset class="mimeAttachmentHeader"></fieldset> <pre wrap="">_______________________________________________ pulseaudio-discuss mailing list <a class="moz-txt-link-abbreviated" href="mailto:pulseaudio-discuss@lists.freedesktop.org">pulseaudio-discuss@lists.freedesktop.org</a> <a class="moz-txt-link-freetext" href="http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss">http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss</a> </pre> </blockquote> </body> </html>