[pulseaudio-discuss] [PATCH 2/2] memblockq-test: fix incorrect assumption of pa_memblockq_pop_missing() behaviour

[Tanu Kaskinen]

On Mon, 2017-01-02 at 10:19 +0100, Pierre Ossman wrote:
> On 31/12/16 17:26, Ahmed S. Darwish wrote:
> > On Fri, Dec 30, 2016 at 05:52:36PM +0200, Tanu Kaskinen wrote:
> > > diff --git a/src/tests/memblockq-test.c b/src/tests/memblockq-test.c
> > > index fc83d99..2a9b88a 100644
> > > --- a/src/tests/memblockq-test.c
> > > +++ b/src/tests/memblockq-test.c
> > > @@ -421,74 +421,129 @@ START_TEST (memblockq_test_pop_missing) {
> > >      bq = pa_memblockq_new("test memblockq", idx, maxlength, tlength, &ss, prebuf, minreq, maxrewind, &silence);
> > >      fail_unless(bq != NULL);
> > > 
> > > -    /* initially, the whole target length of bytes is missing */
> > > +    /* The following equation regarding the internal variables of a memblockq
> > > +     * is always true:
> > > +     *
> > > +     *   length + missing + requested = tlength
> > > +     *
> > > +     * "length" is the current memblockq length (write index minus read index)
> > > +     * and "tlength" is the target length. The intuitive meaning of "missing"
> > > +     * would be the difference between tlength and length, but actually
> > > +     * "missing" and "requested" together constitute the amount that is missing
> > > +     * from the queue. Writing to the queue decrements "requested" and reading
> > > +     * from the queue increments "missing". pa_memblockq_pop_missing() resets
> > > +     * "missing" to zero, returns the old "missing" value and adds the
> > > +     * equivalent amount to "requested".
> > > +     *
> > > +     * This test has comments between each step documenting the assumed state
> > > +     * of those internal variables. */
> > > +
> 
> How about a rename to fit their use instead? I find negative requested 
> to be absolutely horrible.
> 
> > What really bothers me is that memblockq.requested is actually
> > referenced in just _one_ place: a MEMBLOCKQ_DEBUG printf()! :-(
> > 
> 
> Great, so that would nuke it. And it means that we only have "missing" 
> to deal with and the fact that there can be more "missing" than there is 
> room in the buffer.
> 
> How about calling it "consumed"? Because that seems to be what it is, 
> the amount of data consumed from the buffer. And in that case it is more 
> natural that it might exceed available space if you fail to pop it 
> regularly.

"Consumed" has some problems too. The intuitive interpretation for a
variable named like that would be the amount that has been read from
the queue, but that would be the same thing as the read index. Since we
reset the variable every now and then, the meaning changes to "amount
read from the queue since the last reset". But even that's not always
accurate: when the queue is created, the variable doesn't start at
zero, it starts at tlength before anything has been consumed. I agree
that "consumed" would be a bit better name than "missing", though.

> > > Currently the server sends REQUEST
> > > commands only when "missing" becomes positive, however, which doesn't
> > > work when the buffer is filled beyond tlength, because the client
> > > expects REQUEST commands also during the time when the server already
> > > has enough data.
> 
> That sounds like a bug to be honest. Why can't we wait until the buffer 
> drains a bit?

Perhaps the protocol could have been designed so that the REQUEST
commands are only sent when the server really needs more data, but it's
too late to change the protocol now (or well, it could be changed, but
we'd still have to support the old protocol too).

-- 
Tanu

https://www.patreon.com/tanuk