Hi,
First, wish you all a merry Christmas :-)
On Wed, Dec 21, 2016 at 06:16:36AM +0200, Ahmed S. Darwish wrote:
> On Tue, Dec 20, 2016 at 10:03:22AM +0000, Ahmed S. Darwish wrote:
> > On Tue, Dec 20, 2016 at 03:20:00PM +0530, Arun Raghavan wrote:
> > >
...
> > >
> > > Is that a regression?
> > >
> >
> > Didn't try this on earlier versions of pulse to know for sure;
> > will invesitgate further..
> >
>
> OK, it is a regression introduced after v9.0. I've done some
> bisections; here is the offending commit:
>
> commit 74251f07864c63439ea847d7287024ac54578d64
> Author: Pierre Ossman <ossman at cendio.se>
> Date: Thu May 19 15:54:08 2016 +0200
>
> memblockq: remove internal "missing" state variable
>
> It was a very confusing state variable that required a lot of
> fiddling. It was also redundant in that it can be computed from
> the other variables, removing any risk of it getting out of sync.
> In the same spirit, make sure "requested" also always contains a
> sane value, even though it may not be used by every caller.
>
> The issue (not receiving any more stream write callback events) can
> be triggered by:
>
> ./src/pacat -r --latency-msec=4 | ./src/pacat --latency-msec=4
>
OK, the regression is that in the cleanup commit above, the semantics
of memblockq 'missing' length calculations has changed.
Namely, bq->requested is now used as a _cach_ for caclculating the
now removed bq->missing:
size_t pa_memblockq_pop_missing(pa_memblockq *bq) {
...
/* Note that write_index might be before read_index, which means
* that we should ask for extra data to catch up. This also means
* that we cannot call pa_memblockq_length() as it doesn't return
* negative values. */
length = (bq->write_index - bq->read_index) + bq->requested;
^^^^^^^^^^^^^^
if (length > (int64_t)bq->tlength)
return 0;
missing = (int64_t)bq->tlength - length;
...
bq->requested += missing;
...
return missing;
}
But bq->requested has different semantics upon write index change
before and after the same commit:
After the cleanup commit:
static void write_index_changed(pa_memblockq *bq, int64_t old_write_index,
bool account) {
int64_t delta;
delta = bq->write_index - old_write_index;
if (account) {
if (delta > (int64_t)bq->requested)
bq->requested = 0; <== Here is the trigger; bq 'requested'
<== only adjusted in the positive case
else if (delta > 0)
bq->requested -= delta;
}
...
}
But before it:
static void write_index_changed(pa_memblockq *bq, int64_t old_write_index,
bool account) {
int64_t delta;
delta = bq->write_index - old_write_index;
if (account)
bq->requested -= delta; <== bq requested adjusted regardless
<== of sign
else
bq->missing -= delta; <== bq missing is adjusted
<== regardless of 'account'
...
}
Restoring the old semantics of bq->requested to allow negative values
upon write index change fixes the issue. It makes
pa_memblockq_pop_missing() reports non-zero values again, and thus
further stream write callback events to be triggered.
Note: this is still a very partial / preliminary understanding. I'm
investigating further for the root cause / right solution ...
regards,
--
Darwish
http://darwish.chasingpointers.com
