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

[Tanu Kaskinen]

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.

-- 
Tanu