[pulseaudio-discuss] [PATCH v3 22/24] echo-cancel: Use webrtc's deinterleaved API

[Tanu Kaskinen]

On Wed, 2016-02-17 at 18:40 +0530, Arun Raghavan wrote:
> On Wed, 2016-02-17 at 18:36 +0530, Arun Raghavan wrote:
> > On Tue, 2016-02-16 at 14:20 +0200, Tanu Kaskinen wrote:
> > > On Mon, 2016-01-18 at 13:06 +0530, arun at accosted.net wrote:
> > > > +    /* FIXME: we need to be able to modify playback samples */
> > > 
> > > This comment should say also why we need to be able to modify them
> > > (and
> > > I also don't understand what's preventing us from doing so now).
> > 
> > Reworded this as:
> > 
> > /* FIXME: we need to be able to modify playback samples, which we can't
> >  * currently do. This is because module-echo-cancel processes playback
> >  * frames in the source thread, and just stores playback chunks as they
> >  * pass through the sink. */

Mmmh, that still doesn't answer the question why we need to be able to
modify playback samples. What processing do we want to do that we can't
currently do?

> > > > @@ -573,4 +575,9 @@ void pa_webrtc_ec_done(pa_echo_canceller *ec)
> > > > {
> > > >          delete (webrtc::AudioProcessing*)ec->params.webrtc.apm;
> > > >          ec->params.webrtc.apm = NULL;
> > > >      }
> > > > +
> > > > +    for (i = 0; i < ec->params.webrtc.rec_ss.channels; i++)
> > > > +        pa_xfree(ec->params.webrtc.rec_buffer[i]);
> > > > +    for (i = 0; i < ec->params.webrtc.play_ss.channels; i++)
> > > > +        pa_xfree(ec->params.webrtc.play_buffer[i]);
> > > 
> > > I think it's not guaranteed that rec_ss and play_ss are initialized
> > > at
> > > this point. You should check that rec_buffer and play_buffer are
> > > non-
> > > NULL before accessing them.
> > 
> > There should not be a path where rec_buffer and play_buffer are not
> > initialised before done, but I'll add that check in case this becomes
> > possible in the future.
> 
> Shouldn't be necessary, actually. pa_xfree() is NULL-safe.

The problem I thought there to be was that the arrays themselves were
dynamically allocated and possibly NULL, but that's not the case, so
never mind :)

-- 
Tanu