[pulseaudio-discuss] [PATCHv2 1/3] echo-cancel: Do not bypass EC implementation when play stream is empty

[Stefan Huber]

On Sat 16.02.13  22:42, Tanu Kaskinen wrote:
> If plen < u->sink_blocksize && plen > 0, then you leave some data in
> sink_memblockq which will be passed to the canceller when sink_memblockq
> next time contains enough data. This most likely causes glitches.
> 
> I think you should always call pa_memblockq_peek_fixed_size(). If the
> queue doesn't contain enough data, that function will give you whatever
> is in the queue and generate silence for the rest. I think the queue is
> then left in a state where the read index is ahead of the write index,
> which will cause that when data is next time written, some of it goes
> to /dev/null. That's probably not what you want, so if plen <
> u->sink_blocksize, then call pa_memblock_flush_write() after
> pa_memblockq_peek_fixed_size() (I'm pretty sure the "account" parameter
> of pa_memblockq_flush_write() should be true).
> 
> If you do what I suggested, this is probably not relevant, but I'll
> point out it anyway: the code doesn't seem to ensure that
> u->silence.memblock length is always at least u->sink_blocksize.

Actually, I first pursued your idea of using the ability of
pa_memblockq_peek_fixed_size() to fill up the buffer with silence bytes.
However, there was a problem with pa_memblockq_peek_fixed_size()
returning -1 due to pre-buffering. This, in turn, caused pchunk not to
be filled with silence bytes and the code failed.

As I do not see the rationale behind enabling pre-buffering for
u->sink_memblockq I have deactivated it. Another advantage of the
current solution is that the newly introduced 'silence' member of
userdata is now gone again.

To sum up, the new solution that you suggested is clearly the cleaner
alternative.

-- >8 --

When the play stream from the EC sink has not enough data available then
the EC implementation is currently bypassed by directly forwarding the
record bytes to the EC source. Since EC implementations maintain their
own buffers and cause certain latencies, a bypass leads to glitches as
the out stream stream jumps forth and back in time. Furthermore, some
EC implementations may also apply noise reduction or other sound
enhancing techniques, which are therefore bypassed, too.

Fix this by passing silence bytes to the EC implementation if the play
stream runs empty. Hence, this patch keeps the EC implementation running
even if the play stream has no data available.
---
 src/modules/echo-cancel/module-echo-cancel.c |   85 +++++++++++++-------------
 1 file changed, 42 insertions(+), 43 deletions(-)

diff --git a/src/modules/echo-cancel/module-echo-cancel.c b/src/modules/echo-cancel/module-echo-cancel.c
index 9d94a83..0e80954 100644
--- a/src/modules/echo-cancel/module-echo-cancel.c
+++ b/src/modules/echo-cancel/module-echo-cancel.c
@@ -820,60 +820,59 @@ static void do_push(struct userdata *u) {
     plen = pa_memblockq_get_length(u->sink_memblockq);
 
     while (rlen >= u->source_blocksize) {
-        /* take fixed block from recorded samples */
-        pa_memblockq_peek_fixed_size(u->source_memblockq, u->source_blocksize, &rchunk);
 
-        if (plen >= u->sink_blocksize) {
-            /* take fixed block from played samples */
-            pa_memblockq_peek_fixed_size(u->sink_memblockq, u->sink_blocksize, &pchunk);
-
-            rdata = pa_memblock_acquire(rchunk.memblock);
-            rdata += rchunk.index;
-            pdata = pa_memblock_acquire(pchunk.memblock);
-            pdata += pchunk.index;
-
-            cchunk.index = 0;
-            cchunk.length = u->source_blocksize;
-            cchunk.memblock = pa_memblock_new(u->source->core->mempool, cchunk.length);
-            cdata = pa_memblock_acquire(cchunk.memblock);
-
-            if (u->save_aec) {
-                if (u->captured_file)
-                    unused = fwrite(rdata, 1, u->source_blocksize, u->captured_file);
-                if (u->played_file)
-                    unused = fwrite(pdata, 1, u->sink_blocksize, u->played_file);
-            }
+        /* take fixed blocks from recorded and played samples */
+        pa_memblockq_peek_fixed_size(u->source_memblockq, u->source_blocksize, &rchunk);
+        pa_memblockq_peek_fixed_size(u->sink_memblockq, u->sink_blocksize, &pchunk);
 
-            /* perform echo cancellation */
-            u->ec->run(u->ec, rdata, pdata, cdata);
+        /* we ran out of played data and pchunk has been filled with silence bytes */
+        if (plen < u->sink_blocksize)
+            pa_memblockq_flush_write(u->sink_memblockq, TRUE);
 
-            if (u->save_aec) {
-                if (u->canceled_file)
-                    unused = fwrite(cdata, 1, u->source_blocksize, u->canceled_file);
-            }
+        rdata = pa_memblock_acquire(rchunk.memblock);
+        rdata += rchunk.index;
+        pdata = pa_memblock_acquire(pchunk.memblock);
+        pdata += pchunk.index;
 
-            pa_memblock_release(cchunk.memblock);
-            pa_memblock_release(pchunk.memblock);
-            pa_memblock_release(rchunk.memblock);
+        cchunk.index = 0;
+        cchunk.length = u->source_blocksize;
+        cchunk.memblock = pa_memblock_new(u->source->core->mempool, cchunk.length);
+        cdata = pa_memblock_acquire(cchunk.memblock);
 
-            /* drop consumed sink samples */
-            pa_memblockq_drop(u->sink_memblockq, u->sink_blocksize);
-            pa_memblock_unref(pchunk.memblock);
+        if (u->save_aec) {
+            if (u->captured_file)
+                unused = fwrite(rdata, 1, u->source_blocksize, u->captured_file);
+            if (u->played_file)
+                unused = fwrite(pdata, 1, u->sink_blocksize, u->played_file);
+        }
 
-            pa_memblock_unref(rchunk.memblock);
-            /* the filtered samples now become the samples from our
-             * source */
-            rchunk = cchunk;
+        /* perform echo cancellation */
+        u->ec->run(u->ec, rdata, pdata, cdata);
 
-            plen -= u->sink_blocksize;
+        if (u->save_aec) {
+            if (u->canceled_file)
+                unused = fwrite(cdata, 1, u->source_blocksize, u->canceled_file);
         }
 
-        /* forward the (echo-canceled) data to the virtual source */
-        pa_source_post(u->source, &rchunk);
-        pa_memblock_unref(rchunk.memblock);
+        pa_memblock_release(cchunk.memblock);
+        pa_memblock_release(pchunk.memblock);
+        pa_memblock_release(rchunk.memblock);
 
+        /* drop consumed source samples */
         pa_memblockq_drop(u->source_memblockq, u->source_blocksize);
+        pa_memblock_unref(rchunk.memblock);
         rlen -= u->source_blocksize;
+
+        if (plen >= u->sink_blocksize) {
+            /* drop consumed sink samples */
+            pa_memblockq_drop(u->sink_memblockq, u->sink_blocksize);
+            plen -= u->sink_blocksize;
+        }
+        pa_memblock_unref(pchunk.memblock);
+
+        /* forward the (echo-canceled) data to the virtual source */
+        pa_source_post(u->source, &cchunk);
+        pa_memblock_unref(cchunk.memblock);
     }
 }
 
@@ -1940,7 +1939,7 @@ int pa__init(pa_module*m) {
     u->source_memblockq = pa_memblockq_new("module-echo-cancel source_memblockq", 0, MEMBLOCKQ_MAXLENGTH, 0,
         &source_ss, 1, 1, 0, &silence);
     u->sink_memblockq = pa_memblockq_new("module-echo-cancel sink_memblockq", 0, MEMBLOCKQ_MAXLENGTH, 0,
-        &sink_ss, 1, 1, 0, &silence);
+        &sink_ss, 0, 1, 0, &silence);
 
     pa_memblock_unref(silence.memblock);
 
-- 
1.7.9.5