[pulseaudio-discuss] [PATCH v9] loopback: Correct corking logic during sink or source move

[Tanu Kaskinen]

On Fri, 2017-03-17 at 22:50 +0200, Tanu Kaskinen wrote:
> On Mon, 2017-02-27 at 18:17 +0100, Georg Chini wrote:
> > The corking logic of module-loopback was incorrectly implemented. If you suspended
> > the source, the sink input would be corked. When then the sink was suspended because
> > of being idle, the source output was also corked. If you moved then away from the
> > suspended source, module-loopback would not start the stream again, because sink
> > input and source output were both corked. The same applied if the sink was suspended.
> > 
> > This patch corrects this behavior. It also uncorks sink input or source output if the
> > destination source or sink during a move is suspended because it is idle. This avoids
> > unnecessary interruptions of the stream, which makes the latency reports used to
> > correct the initial latency more reliable.
> > 
> > The patch also takes profile switches into account, where sink and source become invalid
> > at the same time. In this case, corking or uncorking must be delayed until the appropriate
> > attach callback.
> > 
> > ---
> >  src/modules/module-loopback.c | 98 +++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 81 insertions(+), 17 deletions(-)
> > @@ -480,6 +487,15 @@ static void source_output_attach_cb(pa_source_output *o) {
> >              o->source->thread_info.rtpoll,
> >              PA_RTPOLL_LATE,
> >              u->asyncmsgq);
> > +
> > +    /* Delayed state schange if necessary */
> > +    if (u->input_thread_info.delayed_source_output_state_change_required) {
> > +        u->input_thread_info.delayed_source_output_state_change_required = false;
> > +        if (u->input_thread_info.delayed_source_output_should_cork)
> > +            pa_source_output_set_state_within_thread(o, PA_SOURCE_OUTPUT_CORKED);
> > +        else
> > +            pa_source_output_set_state_within_thread(o, PA_SOURCE_OUTPUT_RUNNING);
> > +    }
> 
> pa_source_output_cork() does a bunch of things that
> pa_source_output_set_state_within_thread() doesn't do, so it looks like
> you can't replace the former with the latter.
> 
> Did you look into modifying pa_source_output_cork() so that it could be
> called from the moving() callback? I had a look, and it seems that
> there shouldn't be big problems with dealing with the case where o-
> >source is NULL. Remember to update module-suspend-on-idle to deal with
> that case in its PA_SOURCE_OUTPUT_STATE_CHANGED hook callback.

I didn't check the state_change() callbacks, except in module-echo-
cancel. "git grep \\bstate_change\\b" shows that there are quite a few
modules that use those callbacks, and those need to be reviewed to make
sure they can deal with a source output that is moving. Also, it can
cause some trouble if you call the callback from the main thread (at
least module-echo-cancel has an assertion that will fail).

If all the callbacks are similar to the ones in module-echo-cancel,
though, then there's not much harm in not calling those callbacks at
all, because module-echo-cancel's callbacks won't do anything else than
log a debug message.

-- 
Tanu

https://www.patreon.com/tanuk