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
