[pulseaudio-discuss] [PATCH v3 19/24] echo-cancel: webrtc canceller supports different in/out channel counts

[Tanu Kaskinen]

On Mon, 2016-01-18 at 13:06 +0530, arun at accosted.net wrote:
> From: Arun Raghavan <git at arunraghavan.net>
> 
> Needed for upcoming beamforming code.
> ---
>  src/modules/echo-cancel/echo-cancel.h |  2 +-
>  src/modules/echo-cancel/webrtc.cc     | 14 ++++++++------
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/src/modules/echo-cancel/echo-cancel.h b/src/modules/echo-cancel/echo-cancel.h
> index 4693516..613f7e3 100644
> --- a/src/modules/echo-cancel/echo-cancel.h
> +++ b/src/modules/echo-cancel/echo-cancel.h
> @@ -65,7 +65,7 @@ struct pa_echo_canceller_params {
>               * to C++ linkage. apm is a pointer to an AudioProcessing object */
>              void *apm;
>              int32_t blocksize; /* in frames */
> -            pa_sample_spec rec_ss, play_ss;
> +            pa_sample_spec rec_ss, play_ss, out_ss;
>              bool agc;
>              bool trace;
>              bool first;
> diff --git a/src/modules/echo-cancel/webrtc.cc b/src/modules/echo-cancel/webrtc.cc
> index 5741f45..5f00286 100644
> --- a/src/modules/echo-cancel/webrtc.cc
> +++ b/src/modules/echo-cancel/webrtc.cc
> @@ -333,6 +333,7 @@ bool pa_webrtc_ec_init(pa_core *c, pa_echo_canceller *ec,
>      ec->params.webrtc.apm = apm;
>      ec->params.webrtc.rec_ss = *rec_ss;
>      ec->params.webrtc.play_ss = *play_ss;
> +    ec->params.webrtc.out_ss = *out_ss;
>      ec->params.webrtc.blocksize =
>          (uint64_t) (pa_bytes_per_second(out_ss) / pa_frame_size(out_ss)) * BLOCK_SIZE_US / PA_USEC_PER_SEC;
>      *nframes = ec->params.webrtc.blocksize;
> @@ -372,17 +373,18 @@ 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) {
>      webrtc::AudioProcessing *apm = (webrtc::AudioProcessing*)ec->params.webrtc.apm;
>      webrtc::AudioFrame out_frame;
> -    const pa_sample_spec *ss = &ec->params.webrtc.rec_ss;
> +    const pa_sample_spec *rec_ss = &ec->params.webrtc.rec_ss;
> +    const pa_sample_spec *out_ss = &ec->params.webrtc.out_ss;
>      pa_cvolume v;
>      int old_volume, new_volume;
>  
> -    out_frame.num_channels_ = ss->channels;
> -    out_frame.sample_rate_hz_ = ss->rate;
> +    out_frame.num_channels_ = rec_ss->channels;
> +    out_frame.sample_rate_hz_ = rec_ss->rate;
>      out_frame.interleaved_ = true;
>      out_frame.samples_per_channel_ = ec->params.webrtc.blocksize;
>  
>      pa_assert(out_frame.samples_per_channel_ <= webrtc::AudioFrame::kMaxDataSizeSamples);

Not directly related to this patch, but this assertion seems wrong. We
compare kMaxDataSizeSamples to the number of frames, but
kMaxDataSizeSamples is calculated as frames * channels.

Also, kMaxDataSizeSamples assumes that the maximum sample spec is 16-
bit, stereo, 32 kHz, and that the maximum buffer size is 60 ms.
However, we seem to allow using 48 kHz, and I didn't find any
limitations for the channel count. We seem to use 10 ms buffers, so
that gives some headroom. webrtc_ec_fixate_spec() should check all
this, and make sure that our parameters don't exceed
kMaxDataSizeSamples.

The changes in this patch all look good.

-- 
Tanu