[pulseaudio-discuss] [PATCH v2] pacat: Write to stream in frame-sized chunks

[Ahmed S. Darwish]

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