[pulseaudio-discuss] [PATCH 08/13] loopback: Track underruns and cant-peek events

[Tanu Kaskinen]

On Wed, 2016-03-23 at 11:18 +0100, Georg Chini wrote:
> On 23.03.2016 10:55, Tanu Kaskinen wrote:
> > 
> > On Tue, 2016-03-22 at 12:12 +0100, Georg Chini wrote:
> > > 
> > > On 06.12.2015 22:38, Tanu Kaskinen wrote:
> > > > This seems buggy (also without your patch). This code is guarded by "if
> > > > (!in_pop)". The message queue is drained in the beginning of the pop
> > > > callback, and if the POST message that ends an underrun is processed in
> > > > the pop callback, then the rewind is not done, and the underrun won't
> > > > be counted in u->underruns.
> > > > 
> > > > Neither of the problems is particularly serious, so we could ignore
> > > > them, but the fix seems simple to me: it should be safe to just remove
> > > > the message queue draining from the pop callback. It would mean that if
> > > > the memblockq is empty, and a POST message is ready for processing when
> > > > the pop callback is called, the audio in the message won't be used, and
> > > > an unnecessary underrun will occur. I believe that's not a real
> > > > problem, however. The memblockq should be empty only when the sink and
> > > > source buffers are full, and if the sink buffer is full, the pop
> > > > callback won't be called.
> > > > 
> > > Hi Tanu,
> > > 
> > > it looks like this _is_ relevant when running very short latencies.
> > > Those "unnecessary underruns" occur for example when you
> > > try to run HDA -> HDA at 5 ms latency. So I would suggest
> > > to keep it mostly like it is.
> > > Instead of masking the whole block, I would suggest to only mask
> > > the rewind with "if (!in_pop)", so that at least the underruns are still
> > > counted.
> > What are the configured sink and source latencies in that "5 ms
> > latency" setup, and is timer-based scheduling enabled or not? My
> > recollection of the past discussion is that you never agreed to
> > configure the target loopback latency high enough to cover the case
> > where both the sink and source buffers are full. If the target latency
> > is too low in relation to the sink and source latencies, that would
> > explain the underruns.
> Sink & source latency are at 2.33 ms, which seems to be the lowest
> latency that HDA devices support without producing underruns
> or "memblock pool full" messages. Timer based scheduling is enabled.

Ok, 2 x 2.33 ms seems fine.

> > If it's impossible for me to convince you to avoid configuring
> > inherently unsafe target latencies,
> 5ms should not be inherently unsafe and it is working fine with
> the current code. You can run it for hours without issues, tried
> it often enough meanwhile. Just with your proposed change I am
> hitting underruns nearly immediately and I think the code was put
> there in the first place to avoid this situation. It was not me who
> coded this part.

Ok, so it's a mystery why checking the message queue is necessary. This
mystery should at least be noted in a comment in the code.

Maybe the reason is that the measured latency contains the full latency
as reported by snd_pcm_delay, while the target latency configuration
only considers the buffer sizes? If the extra latency that is not
included in the buffer size is significant, that makes the latency
controller reduce the amount that is kept in the loopback memblockq.

What to do about this? I suggest just adding a comment. Feel free to
use the above speculation, if you don't have a better theory.

-- 
Tanu