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

[Tanu Kaskinen]

On Mon, 2013-02-18 at 14:02 +0100, Stefan Huber wrote:
> 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(-)

I made some changes, details below. I've applied the patch. If you could
test that things still works ok, that would be awesome.

> 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);

I believe the right function would actually be pa_memblockq_seek(),
because pa_memblock_flush_write() removes all history. The history data
is important in case of rewinds.

The exact call now looks like this:
pa_memblockq_seek(u->sink_memblockq, u->sink_blocksize - plen, PA_SEEK_RELATIVE, 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);

The peeked data has to be dropped always, not only when plen >=
u->sink_blocksize.

-- 
Tanu