[pulseaudio-discuss] [PATCH v4 03/23] echo-cancel: Add support for the webrtc intelligibility enhancer

[Arun Raghavan]

On Wed, 2016-02-24 at 18:57 +0200, Tanu Kaskinen wrote:
> On Mon, 2016-02-22 at 11:39 +0530, Arun Raghavan wrote:
> > On Fri, 2016-02-19 at 15:27 +0200, Tanu Kaskinen wrote:
> > > On Fri, 2016-02-19 at 14:48 +0530, Arun Raghavan wrote:
> > > > On Thu, 2016-02-18 at 14:58 +0200, Tanu Kaskinen wrote:
> > > > > On Wed, 2016-02-17 at 19:46 +0530, arun at accosted.net wrote:
> > > > > > From: Arun Raghavan <git at arunraghavan.net>
> > > > > > 
> > > > > > Just exposing this, disabled by default. It's not used by Chromium
> > > > > > at
> > > > > > the moment.
> > > > > > ---
> > > > > >  src/modules/echo-cancel/webrtc.cc | 14 ++++++++++++--
> > > > > >  1 file changed, 12 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/src/modules/echo-cancel/webrtc.cc b/src/modules/echo-
> > > > > > cancel/webrtc.cc
> > > > > > index ee3075f..ec63e26 100644
> > > > > > --- a/src/modules/echo-cancel/webrtc.cc
> > > > > > +++ b/src/modules/echo-cancel/webrtc.cc
> > > > > > @@ -47,6 +47,7 @@ PA_C_DECL_END
> > > > > >  #define DEFAULT_COMFORT_NOISE true
> > > > > >  #define DEFAULT_DRIFT_COMPENSATION false
> > > > > >  #define DEFAULT_EXTENDED_FILTER false
> > > > > > +#define DEFAULT_INTELLIGIBILITY_ENHANCER false
> > > > > >  
> > > > > >  static const char* const valid_modargs[] = {
> > > > > >      "high_pass_filter",
> > > > > > @@ -58,6 +59,7 @@ static const char* const valid_modargs[] = {
> > > > > >      "comfort_noise",
> > > > > >      "drift_compensation",
> > > > > >      "extended_filter",
> > > > > > +    "intelligibility_enhancer",
> > > > > >      NULL
> > > > > >  };
> > > > > >  
> > > > > > @@ -84,7 +86,7 @@ bool pa_webrtc_ec_init(pa_core *c,
> > > > > > pa_echo_canceller *ec,
> > > > > >      webrtc::AudioProcessing *apm = NULL;
> > > > > >      webrtc::ProcessingConfig pconfig;
> > > > > >      webrtc::Config config;
> > > > > > -    bool hpf, ns, agc, dgc, mobile, cn, ext_filter;
> > > > > > +    bool hpf, ns, agc, dgc, mobile, cn, ext_filter,
> > > > > > intelligibility;
> > > > > >      int rm = -1;
> > > > > >      pa_modargs *ma;
> > > > > >  
> > > > > > @@ -163,8 +165,16 @@ bool pa_webrtc_ec_init(pa_core *c,
> > > > > > pa_echo_canceller *ec,
> > > > > >          goto fail;
> > > > > >      }
> > > > > >  
> > > > > > +    intelligibility = DEFAULT_INTELLIGIBILITY_ENHANCER;
> > > > > > +    if (pa_modargs_get_value_boolean(ma,
> > > > > > "intelligibility_enhancer", &intelligibility) < 0) {
> > > > > > +        pa_log("Failed to parse intelligibility_enhancer value");
> > > > > > +        goto fail;
> > > > > > +    }
> > > > > > +
> > > > > >      if (ext_filter)
> > > > > >          config.Set(new webrtc::ExtendedFilter(true));
> > > > > > +    if (intelligibility)
> > > > > > +        config.Set(new webrtc::Intelligibility(true));
> > > > > 
> > > > > I see you did no changes here despite our earlier discussion. I was
> > > > > hoping that you'd squash patch 19 to this patch - do you mind if I do
> > > > > that?
> > > > 
> > > > I made the change and then found this form slightly easier to read so
> > > > stuck with it. Please go ahead and squash (or I can do it as well if
> > > > you prefer).
> > > 
> > > Sorry for being unclear - I didn't mean that you should have removed
> > > the if statement. You said you preferred that, and we agreed to keep it
> > > that way. I referred to my comment that it would be better to have the
> > > warning already in this patch, instead of adding it in a later fixup
> > > patch.
> > > 
> > > If I move also the FIXME comment to this patch, I will reword it a bit
> > > to make it clear that the FIXME is mainly relevant to the
> > > intelligibility enhancer, and with these changes I think I will change
> > > the patch attribution to myself. If you want to keep yourself as the
> > > author, then please resubmit the patch, but otherwise I have no problem
> > > with doing the rebasing myself.
> > 
> > I can do that, no problem.
> 
> I have now finished reviewing v4. I did some changes locally, hoping
> that I could just push the set once the review is done. There are some
> changes still needed, however, so I made my local changes available
> here: git://people.freedesktop.org/~tanuk/pulseaudio (branch "webrtc")
> 
> My tree includes the changes discussed above, but if you still want to
> replace the patch that is now attributed to me with your own, that's
> fine by me.

I've picked this up now.

-- Arun