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

[Arun Raghavan]

On Mon, 2016-01-25 at 13:01 +0200, Tanu Kaskinen wrote:
> On Mon, 2016-01-25 at 15:03 +0530, Arun Raghavan wrote:
> > On 25 January 2016 at 13:30, Tanu Kaskinen <tanuk at iki.fi> wrote:
> > > On Mon, 2016-01-25 at 09:36 +0530, Arun Raghavan wrote:
> > > > On 24 January 2016 at 22:00, Tanu Kaskinen <tanuk at iki.fi>
> > > > wrote:
> > > > > On Mon, 2016-01-18 at 13:06 +0530, arun at accosted.net wrote:
> > > > > > @@ -253,7 +263,7 @@ void
> > > > > > pa_webrtc_ec_play(pa_echo_canceller *ec, const uint8_t
> > > > > > *play) {
> > > > > >      pa_assert(play_frame.samples_per_channel_ <=
> > > > > > webrtc::AudioFrame::kMaxDataSizeSamples);
> > > > > >      memcpy(play_frame.data_, play, ec-
> > > > > > >params.priv.webrtc.blocksize);
> > > > > > 
> > > > > > -    apm->AnalyzeReverseStream(&play_frame);
> > > > > > +    apm->ProcessReverseStream(&play_frame);
> > > > > 
> > > > > This looks like a potentially unrelated change. Why is this
> > > > > change
> > > > > done?
> > > > 
> > > > It is needed for the intelligibility enhancer to work, but I
> > > > later
> > > > realised we don't actually support this -- unlike
> > > > AnalyzeReverseStream(), ProcessReverseStream() modifies the
> > > > playback
> > > > samples. This is something that needs to be done in the future
> > > > (there's a later commit that disables this that I could not
> > > > squash due
> > > > to some intermediate changes).
> > > 
> > > Does that mean that the intelligibility enhancer feature doesn't
> > > work
> > > at all? In that case this patch should be dropped.
> > 
> > I was thinking of leaving this and the corresponding disable-patch
> > in
> > as documentation of what doesn't work, but Ic an just drop it if
> > that's the preference.
> 
> I'd prefer not merging broken patches. If the echo-cancel module has
> options for all webrtc features, then adding the warning here would
> be
> ok (instead of a separate patch), but if the feature coverage is
> partial anyway, then just drop the broken options.

I'll double-check, but I think I've covered all the options by the end
of the patchset.

-- Arun