On Thu, 2013-02-07 at 14:40 +0100, Stefan Huber wrote:
> Enable advanced AEC methods to use different specs (i.e., number of
> channels) for rec and out stream. A typical application is beam forming
> resp. multi-channel AEC, which takes multiple record channels to produce
> an echo-canceled output stream.
> This commit alters the EC API as follows: the EC's init() used to get
> source and sink's sample spec/channel map. The new interface renamed
> source to rec and sink to play and additionally passes sample spec and
> channel map of the out stream. The new parameter names of init()
> {rec,play,out}_{ss,map} are more intuitive and also resemble to the
> parameter names known from run(). Both rec_{ss,map} and out_{ss,map} are
> initialized as we knew it from source_{ss,map} before being passed to
> init(). The previous EC implementations only require trivial changes,
> i.e., setting rec_{ss,map} to out_{ss,map} at the end of init() in case
> that out_{ss,map} is modified in init().
> ---
> src/modules/echo-cancel/adrian.c | 32 +++++++-----
> src/modules/echo-cancel/echo-cancel.h | 34 +++++++-----
> src/modules/echo-cancel/module-echo-cancel.c | 72 ++++++++++++++------------
> src/modules/echo-cancel/null.c | 17 +++---
> src/modules/echo-cancel/speex.c | 34 ++++++------
> src/modules/echo-cancel/webrtc.cc | 27 +++++-----
> 6 files changed, 123 insertions(+), 93 deletions(-)
Some trivial comments below.
> diff --git a/src/modules/echo-cancel/adrian.c b/src/modules/echo-cancel/adrian.c
> index 91e3b35..813c304 100644
> --- a/src/modules/echo-cancel/adrian.c
> +++ b/src/modules/echo-cancel/adrian.c
> @@ -43,20 +43,24 @@ static const char* const valid_modargs[] = {
> NULL
> };
>
> -static void pa_adrian_ec_fixate_spec(pa_sample_spec *source_ss, pa_channel_map *source_map,
> - pa_sample_spec *sink_ss, pa_channel_map *sink_map)
> +static void pa_adrian_ec_fixate_spec(pa_sample_spec *rec_ss, pa_channel_map *rec_map,
> + pa_sample_spec *play_ss, pa_channel_map *play_map,
> + pa_sample_spec *out_ss, pa_channel_map *out_map)
Indented one space too little.
> {
> - source_ss->format = PA_SAMPLE_S16NE;
> - source_ss->channels = 1;
> - pa_channel_map_init_mono(source_map);
> -
> - *sink_ss = *source_ss;
> - *sink_map = *source_map;
> + out_ss->format = PA_SAMPLE_S16NE;
> + out_ss->channels = 1;
> + pa_channel_map_init_mono(out_map);
> +
> + *play_ss = *out_ss;
> + *play_map = *out_map;
> + *rec_ss = *out_ss;
> + *rec_map = *out_map;
> }
>
> pa_bool_t pa_adrian_ec_init(pa_core *c, pa_echo_canceller *ec,
> - pa_sample_spec *source_ss, pa_channel_map *source_map,
> - pa_sample_spec *sink_ss, pa_channel_map *sink_map,
> + pa_sample_spec *rec_ss, pa_channel_map *rec_map,
> + pa_sample_spec *play_ss, pa_channel_map *play_map,
> + pa_sample_spec *out_ss, pa_channel_map *out_map,
> uint32_t *nframes, const char *args)
Indented one space too little.
> {
> int rate, have_vector = 0;
> @@ -74,13 +78,13 @@ pa_bool_t pa_adrian_ec_init(pa_core *c, pa_echo_canceller *ec,
> goto fail;
> }
>
> - pa_adrian_ec_fixate_spec(source_ss, source_map, sink_ss, sink_map);
> + pa_adrian_ec_fixate_spec(rec_ss, rec_map, play_ss, play_map, out_ss, out_map);
>
> - rate = source_ss->rate;
> + rate = out_ss->rate;
> *nframes = (rate * frame_size_ms) / 1000;
> - ec->params.priv.adrian.blocksize = (*nframes) * pa_frame_size(source_ss);
> + ec->params.priv.adrian.blocksize = (*nframes) * pa_frame_size(out_ss);
>
> - pa_log_debug ("Using nframes %d, blocksize %u, channels %d, rate %d", *nframes, ec->params.priv.adrian.blocksize, source_ss->channels, source_ss->rate);
> + pa_log_debug ("Using nframes %d, blocksize %u, channels %d, rate %d", *nframes, ec->params.priv.adrian.blocksize, out_ss->channels, out_ss->rate);
>
> /* For now we only support SSE */
> if (c->cpu_info.cpu_type == PA_CPU_X86 && (c->cpu_info.flags.x86 & PA_CPU_X86_SSE))
> diff --git a/src/modules/echo-cancel/echo-cancel.h b/src/modules/echo-cancel/echo-cancel.h
> index e7eed30..71091ba 100644
> --- a/src/modules/echo-cancel/echo-cancel.h
> +++ b/src/modules/echo-cancel/echo-cancel.h
> @@ -47,7 +47,7 @@ typedef struct pa_echo_canceller_params pa_echo_canceller_params;
> struct pa_echo_canceller_params {
> union {
> struct {
> - pa_sample_spec source_ss;
> + pa_sample_spec out_ss;
> } null;
> #ifdef HAVE_SPEEX
> struct {
> @@ -85,10 +85,12 @@ struct pa_echo_canceller {
> /* Initialise canceller engine. */
> pa_bool_t (*init) (pa_core *c,
> pa_echo_canceller *ec,
> - pa_sample_spec *source_ss,
> - pa_channel_map *source_map,
> - pa_sample_spec *sink_ss,
> - pa_channel_map *sink_map,
> + pa_sample_spec *rec_ss,
> + pa_channel_map *rec_map,
> + pa_sample_spec *play_ss,
> + pa_channel_map *play_map,
> + pa_sample_spec *out_ss,
> + pa_channel_map *out_map,
> uint32_t *nframes,
> const char *args);
>
> @@ -133,8 +135,9 @@ void pa_echo_canceller_set_capture_volume(pa_echo_canceller *ec, pa_cvolume *v);
>
> /* Null canceller functions */
> pa_bool_t pa_null_ec_init(pa_core *c, pa_echo_canceller *ec,
> - pa_sample_spec *source_ss, pa_channel_map *source_map,
> - pa_sample_spec *sink_ss, pa_channel_map *sink_map,
> + pa_sample_spec *rec_ss, pa_channel_map *rec_map,
> + pa_sample_spec *play_ss, pa_channel_map *play_map,
> + pa_sample_spec *out_ss, pa_channel_map *out_map,
Indented one space too much.
> uint32_t *nframes, const char *args);
> void pa_null_ec_run(pa_echo_canceller *ec, const uint8_t *rec, const uint8_t *play, uint8_t *out);
> void pa_null_ec_done(pa_echo_canceller *ec);
> @@ -142,8 +145,9 @@ void pa_null_ec_done(pa_echo_canceller *ec);
> #ifdef HAVE_SPEEX
> /* Speex canceller functions */
> pa_bool_t pa_speex_ec_init(pa_core *c, pa_echo_canceller *ec,
> - pa_sample_spec *source_ss, pa_channel_map *source_map,
> - pa_sample_spec *sink_ss, pa_channel_map *sink_map,
> + pa_sample_spec *rec_ss, pa_channel_map *rec_map,
> + pa_sample_spec *play_ss, pa_channel_map *play_map,
> + pa_sample_spec *out_ss, pa_channel_map *out_map,
> uint32_t *nframes, const char *args);
> void pa_speex_ec_run(pa_echo_canceller *ec, const uint8_t *rec, const uint8_t *play, uint8_t *out);
> void pa_speex_ec_done(pa_echo_canceller *ec);
> @@ -152,8 +156,9 @@ void pa_speex_ec_done(pa_echo_canceller *ec);
> #ifdef HAVE_ADRIAN_EC
> /* Adrian Andre's echo canceller */
> pa_bool_t pa_adrian_ec_init(pa_core *c, pa_echo_canceller *ec,
> - pa_sample_spec *source_ss, pa_channel_map *source_map,
> - pa_sample_spec *sink_ss, pa_channel_map *sink_map,
> + pa_sample_spec *rec_ss, pa_channel_map *rec_map,
> + pa_sample_spec *play_ss, pa_channel_map *play_map,
> + pa_sample_spec *out_ss, pa_channel_map *out_map,
Indented one space too little.
> uint32_t *nframes, const char *args);
> void pa_adrian_ec_run(pa_echo_canceller *ec, const uint8_t *rec, const uint8_t *play, uint8_t *out);
> void pa_adrian_ec_done(pa_echo_canceller *ec);
> @@ -163,9 +168,10 @@ void pa_adrian_ec_done(pa_echo_canceller *ec);
> /* WebRTC canceller functions */
> PA_C_DECL_BEGIN
> pa_bool_t pa_webrtc_ec_init(pa_core *c, pa_echo_canceller *ec,
> - pa_sample_spec *source_ss, pa_channel_map *source_map,
> - pa_sample_spec *sink_ss, pa_channel_map *sink_map,
> - uint32_t *nframes, const char *args);
> + pa_sample_spec *rec_ss, pa_channel_map *rec_map,
> + pa_sample_spec *play_ss, pa_channel_map *play_map,
> + pa_sample_spec *out_ss, pa_channel_map *out_map,
> + uint32_t *nframes, const char *args);
Indented one space too little.
> void pa_webrtc_ec_play(pa_echo_canceller *ec, const uint8_t *play);
> void pa_webrtc_ec_record(pa_echo_canceller *ec, const uint8_t *rec, uint8_t *out);
> void pa_webrtc_ec_set_drift(pa_echo_canceller *ec, float drift);
> diff --git a/src/modules/echo-cancel/module-echo-cancel.c b/src/modules/echo-cancel/module-echo-cancel.c
> index d6feaa4..a140902 100644
> --- a/src/modules/echo-cancel/module-echo-cancel.c
> +++ b/src/modules/echo-cancel/module-echo-cancel.c
> @@ -211,6 +211,7 @@ struct userdata {
> pa_bool_t save_aec;
>
> pa_echo_canceller *ec;
> + uint32_t source_output_blocksize;
> uint32_t source_blocksize;
> uint32_t sink_blocksize;
>
> @@ -420,7 +421,7 @@ static int source_process_msg_cb(pa_msgobject *o, int code, void *data, int64_t
> /* Add the latency internal to our source output on top */
> pa_bytes_to_usec(pa_memblockq_get_length(u->source_output->thread_info.delay_memblockq), &u->source_output->source->sample_spec) +
> /* and the buffering we do on the source */
> - pa_bytes_to_usec(u->source_blocksize, &u->source_output->source->sample_spec);
> + pa_bytes_to_usec(u->source_output_blocksize, &u->source_output->source->sample_spec);
>
> return 0;
>
> @@ -739,7 +740,7 @@ static void do_push_drift_comp(struct userdata *u) {
> */
> drift = ((float)(plen - u->sink_rem) - (rlen - u->source_rem)) / ((float)(rlen - u->source_rem));
> u->sink_rem = plen % u->sink_blocksize;
> - u->source_rem = rlen % u->source_blocksize;
> + u->source_rem = rlen % u->source_output_blocksize;
>
> /* Now let the canceller work its drift compensation magic */
> u->ec->set_drift(u->ec, drift);
> @@ -772,14 +773,14 @@ static void do_push_drift_comp(struct userdata *u) {
> }
>
> /* And now the capture samples */
> - while (rlen >= u->source_blocksize) {
> - pa_memblockq_peek_fixed_size(u->source_memblockq, u->source_blocksize, &rchunk);
> + while (rlen >= u->source_output_blocksize) {
> + pa_memblockq_peek_fixed_size(u->source_memblockq, u->source_output_blocksize, &rchunk);
>
> rdata = pa_memblock_acquire(rchunk.memblock);
> rdata += rchunk.index;
>
> cchunk.index = 0;
> - cchunk.length = u->source_blocksize;
> + cchunk.length = u->source_output_blocksize;
> cchunk.memblock = pa_memblock_new(u->source->core->mempool, cchunk.length);
> cdata = pa_memblock_acquire(cchunk.memblock);
>
> @@ -787,11 +788,11 @@ static void do_push_drift_comp(struct userdata *u) {
>
> if (u->save_aec) {
> if (u->drift_file)
> - fprintf(u->drift_file, "c %d\n", u->source_blocksize);
> + fprintf(u->drift_file, "c %d\n", u->source_output_blocksize);
> if (u->captured_file)
> - unused = fwrite(rdata, 1, u->source_blocksize, u->captured_file);
> + unused = fwrite(rdata, 1, u->source_output_blocksize, u->captured_file);
> if (u->canceled_file)
> - unused = fwrite(cdata, 1, u->source_blocksize, u->canceled_file);
> + unused = fwrite(cdata, 1, u->source_output_blocksize, u->canceled_file);
> }
>
> pa_memblock_release(cchunk.memblock);
> @@ -802,8 +803,8 @@ static void do_push_drift_comp(struct userdata *u) {
> pa_source_post(u->source, &cchunk);
> pa_memblock_unref(cchunk.memblock);
>
> - pa_memblockq_drop(u->source_memblockq, u->source_blocksize);
> - rlen -= u->source_blocksize;
> + pa_memblockq_drop(u->source_memblockq, u->source_output_blocksize);
> + rlen -= u->source_output_blocksize;
> }
> }
>
> @@ -821,10 +822,9 @@ static void do_push(struct userdata *u) {
> rlen = pa_memblockq_get_length(u->source_memblockq);
> plen = pa_memblockq_get_length(u->sink_memblockq);
>
> - while (rlen >= u->source_blocksize) {
> -
> + while (rlen >= u->source_output_blocksize) {
> /* take fixed block from recorded samples */
> - pa_memblockq_peek_fixed_size(u->source_memblockq, u->source_blocksize, &rchunk);
> + pa_memblockq_peek_fixed_size(u->source_memblockq, u->source_output_blocksize, &rchunk);
>
> if (plen >= u->sink_blocksize) {
> /* take fixed block from played samples */
> @@ -852,7 +852,7 @@ static void do_push(struct userdata *u) {
>
> if (u->save_aec) {
> if (u->captured_file)
> - unused = fwrite(rdata, 1, u->source_blocksize, u->captured_file);
> + unused = fwrite(rdata, 1, u->source_output_blocksize, u->captured_file);
> if (u->played_file)
> unused = fwrite(pdata, 1, u->sink_blocksize, u->played_file);
> }
> @@ -870,9 +870,9 @@ static void do_push(struct userdata *u) {
> pa_memblock_release(rchunk.memblock);
>
> /* drop consumed source samples */
> - pa_memblockq_drop(u->source_memblockq, u->source_blocksize);
> + pa_memblockq_drop(u->source_memblockq, u->source_output_blocksize);
> pa_memblock_unref(rchunk.memblock);
> - rlen -= u->source_blocksize;
> + rlen -= u->source_output_blocksize;
>
> if (plen >= u->sink_blocksize) {
> /* drop consumed sink samples */
> @@ -918,7 +918,7 @@ static void source_output_push_cb(pa_source_output *o, const pa_memchunk *chunk)
> plen = pa_memblockq_get_length(u->sink_memblockq);
>
> /* Let's not do anything else till we have enough data to process */
> - if (rlen < u->source_blocksize)
> + if (rlen < u->source_output_blocksize)
> return;
>
> /* See if we need to drop samples in order to sync */
> @@ -934,7 +934,7 @@ static void source_output_push_cb(pa_source_output *o, const pa_memchunk *chunk)
> * means the only way to try to catch up is drop sink samples and let
> * the canceller cope up with this. */
> to_skip = rlen >= u->source_skip ? u->source_skip : rlen;
> - to_skip -= to_skip % u->source_blocksize;
> + to_skip -= to_skip % u->source_output_blocksize;
>
> if (to_skip) {
> pa_memblockq_peek_fixed_size(u->source_memblockq, to_skip, &rchunk);
> @@ -947,9 +947,9 @@ static void source_output_push_cb(pa_source_output *o, const pa_memchunk *chunk)
> u->source_skip -= to_skip;
> }
>
> - if (rlen && u->source_skip % u->source_blocksize) {
> - u->sink_skip += (uint64_t) (u->source_blocksize - (u->source_skip % u->source_blocksize)) * u->sink_blocksize / u->source_blocksize;
> - u->source_skip -= (u->source_skip % u->source_blocksize);
> + if (rlen && u->source_skip % u->source_output_blocksize) {
> + u->sink_skip += (uint64_t) (u->source_output_blocksize - (u->source_skip % u->source_output_blocksize)) * u->sink_blocksize / u->source_output_blocksize;
> + u->source_skip -= (u->source_skip % u->source_output_blocksize);
> }
> }
>
> @@ -1640,8 +1640,8 @@ fail:
> /* Called from main context. */
> int pa__init(pa_module*m) {
> struct userdata *u;
> - pa_sample_spec source_ss, sink_ss;
> - pa_channel_map source_map, sink_map;
> + pa_sample_spec source_output_ss, source_ss, sink_ss;
> + pa_channel_map source_output_map, source_map, sink_map;
> pa_modargs *ma;
> pa_source *source_master=NULL;
> pa_sink *sink_master=NULL;
> @@ -1740,12 +1740,16 @@ int pa__init(pa_module*m) {
> u->asyncmsgq = pa_asyncmsgq_new(0);
> u->need_realign = TRUE;
>
> + source_output_ss = source_ss;
> + source_output_map = source_map;
> +
> pa_assert(u->ec->init);
> - if (!u->ec->init(u->core, u->ec, &source_ss, &source_map, &sink_ss, &sink_map, &nframes, pa_modargs_get_value(ma, "aec_args", NULL))) {
> + if (!u->ec->init(u->core, u->ec, &source_output_ss, &source_output_map, &sink_ss, &sink_map, &source_ss, &source_map, &nframes, pa_modargs_get_value(ma, "aec_args", NULL))) {
> pa_log("Failed to init AEC engine");
> goto fail;
> }
>
> + u->source_output_blocksize = nframes * pa_frame_size(&source_output_ss);
> u->source_blocksize = nframes * pa_frame_size(&source_ss);
> u->sink_blocksize = nframes * pa_frame_size(&sink_ss);
>
> @@ -1864,8 +1868,8 @@ int pa__init(pa_module*m) {
>
> pa_proplist_sets(source_output_data.proplist, PA_PROP_MEDIA_NAME, "Echo-Cancel Source Stream");
> pa_proplist_sets(source_output_data.proplist, PA_PROP_MEDIA_ROLE, "filter");
> - pa_source_output_new_data_set_sample_spec(&source_output_data, &source_ss);
> - pa_source_output_new_data_set_channel_map(&source_output_data, &source_map);
> + pa_source_output_new_data_set_sample_spec(&source_output_data, &source_output_ss);
> + pa_source_output_new_data_set_channel_map(&source_output_data, &source_output_map);
>
> pa_source_output_new(&u->source_output, m->core, &source_output_data);
> pa_source_output_new_data_done(&source_output_data);
> @@ -1932,7 +1936,7 @@ int pa__init(pa_module*m) {
> pa_sink_input_get_silence(u->sink_input, &u->silence);
>
> u->source_memblockq = pa_memblockq_new("module-echo-cancel source_memblockq", 0, MEMBLOCKQ_MAXLENGTH, 0,
> - &source_ss, 1, 1, 0, &u->silence);
> + &source_output_ss, 1, 1, 0, &u->silence);
> u->sink_memblockq = pa_memblockq_new("module-echo-cancel sink_memblockq", 0, MEMBLOCKQ_MAXLENGTH, 0,
> &sink_ss, 1, 1, 0, &u->silence);
>
> @@ -2076,8 +2080,8 @@ void pa__done(pa_module*m) {
> */
> int main(int argc, char* argv[]) {
> struct userdata u;
> - pa_sample_spec source_ss, sink_ss;
> - pa_channel_map source_map, sink_map;
> + pa_sample_spec source_output_ss, source_ss, sink_ss;
> + pa_channel_map source_output_map, source_map, sink_map;
> pa_modargs *ma = NULL;
> uint8_t *rdata = NULL, *pdata = NULL, *cdata = NULL;
> int unused PA_GCC_UNUSED;
> @@ -2130,11 +2134,15 @@ int main(int argc, char* argv[]) {
> if (init_common(ma, &u, &source_ss, &source_map) < 0)
> goto fail;
>
> - if (!u.ec->init(u.core, u.ec, &source_ss, &source_map, &sink_ss, &sink_map, &nframes,
> + source_output_ss = source_ss;
> + source_output_map = source_map;
> +
> + if (!u.ec->init(u.core, u.ec, &source_output_ss, &source_output_map, &sink_ss, &sink_map, &source_ss, &source_map, &nframes,
> (argc > 5) ? argv[5] : NULL )) {
> pa_log("Failed to init AEC engine");
> goto fail;
> }
> + u.source_output_blocksize = nframes * pa_frame_size(&source_output_ss);
> u.source_blocksize = nframes * pa_frame_size(&source_ss);
> u.sink_blocksize = nframes * pa_frame_size(&sink_ss);
>
> @@ -2152,12 +2160,12 @@ int main(int argc, char* argv[]) {
> }
> }
>
> - rdata = pa_xmalloc(u.source_blocksize);
> + rdata = pa_xmalloc(u.source_output_blocksize);
> pdata = pa_xmalloc(u.sink_blocksize);
> cdata = pa_xmalloc(u.source_blocksize);
>
> if (!u.ec->params.drift_compensation) {
> - while (fread(rdata, u.source_blocksize, 1, u.captured_file) > 0) {
> + while (fread(rdata, u.source_output_blocksize, 1, u.captured_file) > 0) {
> if (fread(pdata, u.sink_blocksize, 1, u.played_file) == 0) {
> perror("Played file ended before captured file");
> goto fail;
> diff --git a/src/modules/echo-cancel/null.c b/src/modules/echo-cancel/null.c
> index 6d9531a..3fb5e8d 100644
> --- a/src/modules/echo-cancel/null.c
> +++ b/src/modules/echo-cancel/null.c
> @@ -26,18 +26,23 @@ PA_C_DECL_BEGIN
> PA_C_DECL_END
>
> pa_bool_t pa_null_ec_init(pa_core *c, pa_echo_canceller *ec,
> - pa_sample_spec *source_ss, pa_channel_map *source_map,
> - pa_sample_spec *sink_ss, pa_channel_map *sink_map,
> + pa_sample_spec *rec_ss, pa_channel_map *rec_map,
> + pa_sample_spec *play_ss, pa_channel_map *play_map,
> + pa_sample_spec *out_ss, pa_channel_map *out_map,
> uint32_t *nframes, const char *args) {
Indented one space too much.
> char strss_source[PA_SAMPLE_SPEC_SNPRINT_MAX];
> char strss_sink[PA_SAMPLE_SPEC_SNPRINT_MAX];
>
> *nframes = 256;
> - ec->params.priv.null.source_ss = *source_ss;
> + ec->params.priv.null.out_ss = *out_ss;
> +
> + /* Isn't required as rec_{ss,map} is initialized to out_{ss,map}
> + *rec_ss = *out_ss;
> + *rec_map = *out_map; */
Either use assertions or just have the code there uncommented (it may
not be a good idea to rely on the caller to initialize the variables in
any particular way, so I'd uncomment the code).
>
> pa_log_debug("null AEC: nframes=%u, sample spec source=%s, sample spec sink=%s", *nframes,
> - pa_sample_spec_snprint(strss_source, sizeof(strss_source), source_ss),
> - pa_sample_spec_snprint(strss_sink, sizeof(strss_sink), sink_ss));
> + pa_sample_spec_snprint(strss_source, sizeof(strss_source), out_ss),
> + pa_sample_spec_snprint(strss_sink, sizeof(strss_sink), play_ss));
Indentation could be increased to match the opening parenthesis.
--
Tanu
