[pulseaudio-discuss] [PATCH 03/23] echo-cancel: Fix for webrtc AEC when run from test program

[Tanu Kaskinen]

On Thu, 2013-02-14 at 14:38 +0100, Peter Meerwald wrote:
> On Thu, 14 Feb 2013, Tanu Kaskinen wrote:
> 
> > On Wed, 2013-02-13 at 17:26 +0100, Peter Meerwald wrote:
> > > webrtc crashes with agc enabled when run from test program, i.e.
> > > echo-cancel-test play.raw rec.raw out.raw aec_method=webrtc
> > > 
> > > ==8785== Invalid read of size 8
> > > ==8785==    at 0x40A2F6: pa_echo_canceller_get_capture_volume (module-echo-cancel.c:1593)
> > > ==8785==    by 0x418EA2F: pa_webrtc_ec_record (webrtc.cc:262)
> > > ==8785==    by 0x40C890: main (module-echo-cancel.c:2223)
> > > ==8785==  Address 0x28 is not stack'd, malloc'd or (recently) free'd
> > > 
> > > fix this by checking if ec->msg is set in pa_echo_canceller_get_capture_volume() and
> > > pa_echo_canceller_set_capture_volume() -- the test program has no audio device and
> > > hence no volume control
> > > 
> > > Signed-off-by: Peter Meerwald <pmeerw at pmeerw.net>
> > > ---
> > >  src/modules/echo-cancel/module-echo-cancel.c |    4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/src/modules/echo-cancel/module-echo-cancel.c b/src/modules/echo-cancel/module-echo-cancel.c
> > > index 11ad1de..81c0a54 100644
> > > --- a/src/modules/echo-cancel/module-echo-cancel.c
> > > +++ b/src/modules/echo-cancel/module-echo-cancel.c
> > > @@ -1556,11 +1556,15 @@ static int canceller_process_msg_cb(pa_msgobject *o, int code, void *userdata, i
> > >  
> > >  /* Called by the canceller, so source I/O thread context. */
> > >  void pa_echo_canceller_get_capture_volume(pa_echo_canceller *ec, pa_cvolume *v) {
> > > +    if (!ec->msg)
> > > +        return;
> > >      *v = ec->msg->userdata->thread_info.current_volume;
> > >  }
> > >  
> > >  /* Called by the canceller, so source I/O thread context. */
> > >  void pa_echo_canceller_set_capture_volume(pa_echo_canceller *ec, pa_cvolume *v) {
> > > +    if (!ec->msg)
> > > +        return;
> > >      if (!pa_cvolume_equal(&ec->msg->userdata->thread_info.current_volume, v)) {
> > >          pa_cvolume *vol = pa_xnewdup(pa_cvolume, v, 1);
> > 
> > These functions are only relevant when AGC is enabled. If the test
> > program don't provide an audio device, they clearly don't support AGC,
> > so shouldn't the test program disable AGC?
> 
> I thought about this; in case AGC is enabled, webrtc calls 
> apm->gain_control()->set_analog_level_limits(0, PA_VOLUME_NORM-1)
> and
> apm->gain_control()->Enable(true)
> 
> it makes sense to be able to test these code path even when the 
> information from the audio device is missing

The "if (!ec->msg)" checks look really out-of-place. I don't think
having some more test coverage is a valid justification for those
checks.

The same (or better) coverage can be achieved by mocking the required
volume interface in the test code. That requires setting up the
thread_mq, though, which probably means running the echo canceller code
in a separate thread. Not excessively complicated, but if you don't want
to do that, then I'd prefer just disabling AGC in the test code.

-- 
Tanu