[pulseaudio-discuss] [PATCH v3 11/24] echo-cancel: Start capture at a sane value if we're doing webrtc AGC

[Tanu Kaskinen]

On Tue, 2016-02-02 at 09:59 +0530, Arun Raghavan wrote:
> On Mon, 2016-02-01 at 13:18 +0200, Tanu Kaskinen wrote:
> > On Mon, 2016-01-18 at 13:06 +0530, arun at accosted.net wrote:
> > > From: Arun Raghavan <git at arunraghavan.net>
> > > 
> > > This is required to make sure the capture output has sufficient
> > > energy
> > > for the AGC to do its job.
> > 
> > I think the word "value" in the first line of the commit message
> > should
> > be replaced with "volume". After reading just the commit message, I
> > didn't know what the patch was about.
> > 
> > > ---
> > >  src/modules/echo-cancel/echo-cancel.h |  1 +
> > >  src/modules/echo-cancel/webrtc.cc     | 14 +++++++++++++-
> > >  2 files changed, 14 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/modules/echo-cancel/echo-cancel.h
> > > b/src/modules/echo-cancel/echo-cancel.h
> > > index 142f8ac..b570095 100644
> > > --- a/src/modules/echo-cancel/echo-cancel.h
> > > +++ b/src/modules/echo-cancel/echo-cancel.h
> > > @@ -68,6 +68,7 @@ struct pa_echo_canceller_params {
> > >              pa_sample_spec sample_spec;
> > >              bool agc;
> > >              bool trace;
> > > +            bool first;
> > >          } webrtc;
> > >  #endif
> > >          /* each canceller-specific structure goes here */
> > > diff --git a/src/modules/echo-cancel/webrtc.cc b/src/modules/echo-
> > > cancel/webrtc.cc
> > > index c2585a8..6431651 100644
> > > --- a/src/modules/echo-cancel/webrtc.cc
> > > +++ b/src/modules/echo-cancel/webrtc.cc
> > > @@ -52,6 +52,7 @@ PA_C_DECL_END
> > >  #define DEFAULT_TRACE false
> > >  
> > >  #define WEBRTC_AGC_MAX_VOLUME 255
> > > +#define WEBRTC_AGC_START_VOLUME 85
> > >  
> > >  static const char* const valid_modargs[] = {
> > >      "high_pass_filter",
> > > @@ -297,6 +298,7 @@ bool pa_webrtc_ec_init(pa_core *c,
> > > pa_echo_canceller *ec,
> > >      ec->params.priv.webrtc.sample_spec = *out_ss;
> > >      ec->params.priv.webrtc.blocksize =
> > > (uint64_t)pa_bytes_per_second(out_ss) * BLOCK_SIZE_US /
> > > PA_USEC_PER_SEC;
> > >      *nframes = ec->params.priv.webrtc.blocksize /
> > > pa_frame_size(out_ss);
> > > +    ec->params.priv.webrtc.first = true;
> > >  
> > >      pa_modargs_free(ma);
> > >      return true;
> > > @@ -354,7 +356,17 @@ void pa_webrtc_ec_record(pa_echo_canceller
> > > *ec, const uint8_t *rec, uint8_t *out
> > >      apm->ProcessStream(&out_frame);
> > >  
> > >      if (ec->params.priv.webrtc.agc) {
> > > -        new_volume = apm->gain_control()->stream_analog_level();
> > > +        if (PA_UNLIKELY(ec->params.priv.webrtc.first)) {
> > > +            /* We start at a sane default volume (taken from the
> > > Chromium
> > > +             * condition on the experimental AGC in
> > > audio_processing.h). This is
> > > +             * needed to make sure that there's enough energy in
> > > the capture
> > > +             * signal for the AGC to work */
> > > +            ec->params.priv.webrtc.first = false;
> > > +            new_volume = WEBRTC_AGC_START_VOLUME;
> > 
> > This is done only on initial loading of the module. Does AGC work ok
> > after resuming a suspended source?
> 
> After suspend/resume is not a problem since the source volume won't get
> reset from whatever it was set to before. This is mostly to make sure
> that when the AGC first starts, it is able to actually get going.

Ok. Is the problem that if the initial volume is too low, AGC won't
work, but too high initial volume is fine? What if the user sets the
source volume manually? Does AGC stop working? If not, why is that
different from the initialization phase? (I'm just trying to understand
the problem. If manual volume changes mess up AGC, I don't think that's
something that this patch needs to care about.)

-- 
Tanu