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

[Tanu Kaskinen]

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. */
+
+    /* length + missing + requested = tlength
+     * 0      + 100     + 0         = 100 */
+
     ck_assert_int_eq(pa_memblockq_pop_missing(bq), tlength);
 
-    /* add 20 bytes of data */
+    /* length + missing + requested = tlength
+     * 0      + 0       + 100       = 100 */
+
     for (int i = 0; i != 2; ++i)
         ck_assert_int_eq(pa_memblockq_push(bq, &data), 0);
     check_queue_invariants(bq);
 
-    /* no new missing data is reported */
+    /* length + missing + requested = tlength
+     * 20     + 0       + 80        = 100 */
+
     ck_assert_int_eq(pa_memblockq_pop_missing(bq), 0);
 
-    /* fill up to 100 bytes of data */
+    /* length + missing + requested = tlength
+     * 20     + 0       + 80        = 100 */
+
     for (int i = 0; i != 8; ++i)
         ck_assert_int_eq(pa_memblockq_push(bq, &data), 0);
     check_queue_invariants(bq);
 
-    /* queue fill level is at target level now */
+    /* length + missing + requested = tlength
+     * 100    + 0       + 0         = 100 */
+
     ck_assert_int_eq(pa_memblockq_pop_missing(bq), 0);
 
-    /* pop 40 bytes of data, down to 60 bytes fill level */
+    /* length + missing + requested = tlength
+     * 100    + 0       + 0         = 100 */
+
     ck_assert_int_eq(pa_memblockq_peek_fixed_size(bq, 40, &chunk), 0);
     pa_memblockq_drop(bq, 40);
     ck_assert_int_eq(chunk.length - chunk.index, 40);
     pa_memblock_unref(chunk.memblock);
     check_queue_invariants(bq);
 
-    /* queue fill level is 40 bytes under target length
-     * This is less than minreq, so no missing data is reported */
+    /* length + missing + requested = tlength
+     * 60     + 40      + 0         = 100 */
+
+    /* 40 bytes are missing, but that's less than minreq, so 0 is reported */
     ck_assert_int_eq(pa_memblockq_pop_missing(bq), 0);
 
-    /* add 30 bytes of data, up to 90 bytes fill level */
+    /* length + missing + requested = tlength
+     * 60     + 40      + 0         = 100 */
+
+    /* Now we push 30 bytes even though it was not requested, so the requested
+     * counter goes negative! */
     for (int i = 0; i != 3; ++i)
         ck_assert_int_eq(pa_memblockq_push(bq, &data), 0);
     check_queue_invariants(bq);
 
-    /* queue fill level is 10 bytes under target length
-     * This is less than minreq, so no missing data is reported. */
+    /* length + missing + requested = tlength
+     * 90     + 40      + -30       = 100 */
+
+    /* missing < minreq, so nothing is reported missing. */
     ck_assert_int_eq(pa_memblockq_pop_missing(bq), 0);
 
-    /* pop 20 bytes of data, down to 70 bytes of data */
+    /* length + missing + requested = tlength
+     * 90     + 40      + -30       = 100 */
+
     ck_assert_int_eq(pa_memblockq_peek_fixed_size(bq, 20, &chunk), 0);
     pa_memblockq_drop(bq, 20);
     ck_assert_int_eq(chunk.length - chunk.index, 20);
     pa_memblock_unref(chunk.memblock);
     check_queue_invariants(bq);
 
-    /* queue fill level is 30 bytes under target length
-     * This is less than minreq, so no missing data is reported */
+    /* length + missing + requested = tlength
+     * 70     + 60      + -30       = 100 */
+
+    /* missing < minreq, so nothing is reported missing. */
     ck_assert_int_eq(pa_memblockq_pop_missing(bq), 0);
 
-    /* add 50 bytes of data, up to 120 bytes fill level */
+    /* length + missing + requested = tlength
+     * 70     + 60      + -30       = 100 */
+
+    /* We push more data again even though it was not requested, so the
+     * requested counter goes further into the negative range. */
     for (int i = 0; i != 5; ++i)
         ck_assert_int_eq(pa_memblockq_push(bq, &data), 0);
     check_queue_invariants(bq);
 
-    /* queue fill level is above target level, so no missing data is reported. */
+    /* length + missing + requested = tlength
+     * 120    + 60      + -80       = 100 */
+
+    /* missing < minreq, so nothing is reported missing. */
     ck_assert_int_eq(pa_memblockq_pop_missing(bq), 0);
 
-    /* pop 20 bytes of data, down the target level */
+    /* length + missing + requested = tlength
+     * 120    + 60      + -80       = 100 */
+
     ck_assert_int_eq(pa_memblockq_peek_fixed_size(bq, 20, &chunk), 0);
     pa_memblockq_drop(bq, 20);
     ck_assert_int_eq(chunk.length - chunk.index, 20);
     pa_memblock_unref(chunk.memblock);
     check_queue_invariants(bq);
 
-    /* queue fill level is at target level now
-     * No missing data should be reported. */
-    ck_assert_int_eq(pa_memblockq_pop_missing(bq), 0);
+    /* length + missing + requested = tlength
+     * 100    + 80      + -80       = 100 */
+
+    /* missing has now reached the minreq threshold */
+    ck_assert_int_eq(pa_memblockq_pop_missing(bq), 80);
+
+    /* length + missing + requested = tlength
+     * 100    + 0       + 0         = 100 */
 
     /* cleanup */
     pa_memblockq_free(bq);
-- 
2.10.2