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
