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

[Ahmed S. Darwish]

Hi!

On Fri, Dec 30, 2016 at 05:52:36PM +0200, Tanu Kaskinen wrote:
> The intuitive meaning of "missing" would be the difference between
> tlength and the current queue length, and that's how memblockq-test
> assumed pa_memblockq_pop_missing() to define the term "missing", but
> that was an incorrect assumption, causing the last
> pa_memblockq_pop_missing() return value assertion to fail.
> 
> This patch fixes the failing assertion and adds some comments about how
> the "missing" and "requested" variables in memblockq work.
> ---
>  src/tests/memblockq-test.c | 95 ++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 75 insertions(+), 20 deletions(-)
> 
> 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. */
> +

What really bothers me is that memblockq.requested is actually
referenced in just _one_ place: a MEMBLOCKQ_DEBUG printf()! :-(

So memblockq.requested actually serves no value: it's just used
for debugging statements. That makes my earlier notes of requested
needed to be negative also wrong. And thus there's definitely
another reason why the now-reverted 74251f0 commit triggered a
problem when doing large 64K writes on low-latency streams..

I've verified the statement above. Removed memblockq.requested
completely [1] and everything compiled (and worked) fine as
expected; even in the big stream writes case.

So an ACK of course for this patch; its explanations make things
much more comprehesible! Just a note that the case is still open,
and we still don't know what's actually causing problems in the
memblockq code..

[1]

Remove memblockq.requested; it's only used for debugging statements
and makes an already confusing code harder to deduce.

(I'll send an official submission for this soon)

---

diff --git a/src/pulsecore/memblockq.c b/src/pulsecore/memblockq.c
index f660ffaed..145b7a2e9 100644
--- a/src/pulsecore/memblockq.c
+++ b/src/pulsecore/memblockq.c
@@ -53,7 +53,7 @@ struct pa_memblockq {
     bool in_prebuf;
     pa_memchunk silence;
     pa_mcalign *mcalign;
-    int64_t missing, requested;
+    int64_t missing;
     char *name;
     pa_sample_spec sample_spec;
 };
@@ -251,13 +251,11 @@ static void write_index_changed(pa_memblockq *bq, int64_t old_write_index, bool
 
     delta = bq->write_index - old_write_index;
 
-    if (account)
-        bq->requested -= delta;
-    else
+    if (!account)
         bq->missing -= delta;
 
 #ifdef MEMBLOCKQ_DEBUG
-     pa_log_debug("[%s] pushed/seeked %lli: requested counter at %lli, account=%i", bq->name, (long long) delta, (long long) bq->requested, account);
+     pa_log_debug("[%s] pushed/seeked %lli", bq->name, (long long) delta);
 #endif
 }
 
@@ -846,7 +844,6 @@ size_t pa_memblockq_pop_missing(pa_memblockq *bq) {
 
     l = (size_t) bq->missing;
 
-    bq->requested += bq->missing;
     bq->missing = 0;
 
 #ifdef MEMBLOCKQ_DEBUG


-- 
Darwish
http://darwish.chasingpointers.com