[pulseaudio-discuss] [RFC][PATCH] improve missing handling in memblockq (was Re: stream wedged in non-playing state)

[Pierre Ossman]

On Wed, 18 May 2016 15:40:55 +0200
Pierre Ossman <ossman at cendio.se> wrote:

> I suspect the bug is in the memblockq code somewhere. It seems like it
> wedges itself somehow. The "missing" state variable is something I'm
> looking at with suspicion.
> 

This wasn't the problem. The trace revealed that we do not handle
changes to minreq properly. The scenario is:

 1. Large latency, large buffer, large target fill, large minimum
 request. Silence in queue (i.e. buffer is full).

 2. Buffer drains slightly, making it fall below target fill. It is
 however still below the minimum request, so nothing is sent to the
 client.

 3. The client requests a reduced latency, buffer is reduced, target
 fill is reduced, minimum request is reduced. The buffer now greatly
 exceeds target fill as it was almost up to the previous target fill
 level. This means that the server will not be asking the client for
 more data for a while.

 4. Some time later we've drained most of the excess and are almost
 back down to the target fill level. However the data requested in 2 is
 sufficiently large that we never fall back down below target fill.
 Hence we never start requesting for more data. And we already decided
 in 2 not to send a request for the first portion.

I've attached a patch that moves the minreq handling in to memblockq,
which should avoid this and any similar issues as there is now a single
place where things are throttled for minreq.

This will affect all callers of pa_memblockq_pop_missing() and perturb
things a bit. I can't see it being fundamentally worse or better
though, just different. Besides fixing the bugs of course.

> Why do we even have that? Can't we derive that from the other
> variables? Having redundant state is just asking for things to get out
> of sync and for bugs to appear.
> 

Patch added for this as well. The memblockq code makes my head spin,
and killing of "missing" and santising "requested" makes it a bit
easier to comprehend.

Please review as I'd really like this fix to start trickling out to the
distributions sooner rather than later.

Rgds
-- 
Pierre Ossman           Software Development
Cendio AB		https://cendio.com
Teknikringen 8		https://twitter.com/ThinLinc
583 30 Linköping	https://facebook.com/ThinLinc
Phone: +46-13-214600	https://plus.google.com/+CendioThinLinc

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-memblockq-move-minreq-handling-in-to-memblockq.patch
Type: text/x-patch
Size: 2048 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20160519/981c26c0/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-memblockq-remove-internal-missing-state-variable.patch
Type: text/x-patch
Size: 4496 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20160519/981c26c0/attachment-0001.bin>