[pulseaudio-discuss] [PATCH] echo-cancel: Fix assert with webrtc's built-in drift compensation

[Arun Raghavan]

On Mon, 13 Mar 2017, at 08:06 PM, Tanu Kaskinen wrote:
> On Mon, 2017-03-13 at 16:07 +0530, Arun Raghavan wrote:
> > The webrtc canceller requires that the set_stream_drift_samples() method
> > be called before every call of ProcessStream(). We do kind of leak this
> > into the generic bits of module-echo-cancel, but this should not be
> > harmful in the general case either.
> >
> > Relatedly, we also require that this happen when there are no sink
> > samples being sent, so we just drop the special-case check for whether
> > the sink is running to do the drift compensation calculations.
> 
> It's not clear to me from the commit message which assertion fails.
> 
> Did the crash happen because set_drift() wasn't called before
> ProcessStream(), because we didn't call do_push_drift_comp() when the
> sink isn't running? If so, why did you move the set_drift() call? Was
> that a fix for some other bug?

It's actually a fix for two things:

1. We are calling do_push_drift_comp() when the sink isn't running,
which calls ProcessStream() without setting the drift

2. We are calling ProcessStream() in a loop, but not setting the drift
each time (the code seems to have changed to require a call each time).

I will update the message to reflect this.

> > @@ -947,7 +945,7 @@ static void source_output_push_cb(pa_source_output *o, const pa_memchunk *chunk)
> >      }
> >  
> >      /* process and push out samples, do drift compensation only if the sink is actually running */
> > -    if (u->ec->params.drift_compensation && u->sink->thread_info.state == PA_SINK_RUNNING)
> > +    if (u->ec->params.drift_compensation)
> >          do_push_drift_comp(u);
> >      else
> >          do_push(u);
> 
> It looks like the comment should be updated too.

Indeed.

Following up with an updated patch.

-- Arun