[pulseaudio-discuss] sink-input's membockq's read-index rewound to a huge negative causing silence delay, almost 2~3 seconds

[Tanu Kaskinen]

On Thu, 2011-06-09 at 05:42 -0400, xing wang wrote:
> hi all,
> 
> i've a finding about silence delay, the background is looking like
> this post : "[pulseaudio-discuss] [PATCH] core: Drop empty gaps in the
> memblockq when playing data from it."
> (http://www.mail-archive.com/pulseaudio-discuss@mail.0pointer.de/msg09579.html)
> 
> with tsched=1 and using default 2s buffer size,,while alsa-driver
> provides a even bigger buffer(5s),after the first rewind of "latency
> change" before starting playback, the buffer had been shrinked to a
> smaller value according to app's request, this trigger sink-input's
> rewinding , read-index become a negative value,,nearly -buffer_size.
> 
> in pa_sink_input_cb() , pa_memblockq_peek() will return silence before
> the readindex reach 0 from the negative value. that caused obvious
> delay.

I'm not convinced that causes any delay. I tried to reproduce the
problem myself, and while my sound card (or its driver) is capable of
giving only 185ms buffer so I can't really observe the by listening,
adding some debug prints (patch attached) showed that yes, the read
index gets negative, but when the client data arrives and the "end of
underrun" rewind happens, the write index gets reset to be the same as
the read index (ie. negative). Since the write index is not ahead of the
read index, there shouldn't be any silence because of the negative
indexes. This is different from the case that you linked to - there the
write index did not get reset to the read index.

That said, I can't think of any case where it would make sense to rewind
beyond the beginning of a stream (or any other memblockq), so I've
attached a patch that limits pa_memblockq_rewind() so that it never
rewinds to negative read index values. I'm not sure if it breaks
anything, but at least basic playback and volume control seems to work
without glitches... You can try it if it helps in your case.

Maybe the patch should be even committed to master? If the patch doesn't
break anything, it should make these two fixes redundant:

http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=6bd34156b130c07b130de10111a12ef6dab18b52
http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=495c1ed2361f7bab5eaa6978d2fda624a2944bb9

As I say in the commit message, if we're trying to limit the read index
to be always non-negative, then the write index should probably be
limited too. And then the index types could be changed from signed to
unsigned... I wonder if Lennart made them signed for a purpose?

-- 
Tanu