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

[Georg Chini]

On 23.03.2016 11:50, Tanu Kaskinen wrote:
> 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.
>
> -- 
>
Thanks, I'll add some comment. Anyway this was a valuable hint.
I can stop to track the "no peek" events now, it looks like this was
only necessary because I missed the underuns that were masked
by the "if (!in_pop)" condition.