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

[Tanu Kaskinen]

On Thu, 2013-02-07 at 14:40 +0100, Stefan Huber wrote:
> 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 |   95 ++++++++++++++------------
>  1 file changed, 52 insertions(+), 43 deletions(-)
> 
> diff --git a/src/modules/echo-cancel/module-echo-cancel.c b/src/modules/echo-cancel/module-echo-cancel.c
> index 11ad1de..d6feaa4 100644
> --- a/src/modules/echo-cancel/module-echo-cancel.c
> +++ b/src/modules/echo-cancel/module-echo-cancel.c
> @@ -238,6 +238,8 @@ struct userdata {
>      size_t sink_rem;
>      size_t source_rem;
>  
> +    pa_memchunk silence;
> +
>      pa_atomic_t request_resync;
>  
>      pa_time_event *time_event;
> @@ -820,60 +822,68 @@ 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);
> +        } else {
> +            /* If we run out of play data, we substitute the play bytes with
> +             * silence bytes in order keep the EC implementation with its
> +             * internal buffers running. */
> +
> +            pchunk = u->silence;
> +            pa_memblock_ref(pchunk.memblock);
> +            pchunk.index = 0;
> +            pchunk.length = u->sink_blocksize;
> +        }

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.

-- 
Tanu