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

[Tanu Kaskinen]

On Mon, 2017-03-20 at 21:15 +0100, Georg Chini wrote:
> On 17.03.2017 21:50, 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.
> 
> That's wrong. pa_source_output_cork()  only calls source_output_set_state()
> and does nothing else. The same applies to sink_input_cork().
> 
> > 
> > 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 don't think it is worth the effort. As far as I know module-loopback 
> is the
> only place where this will ever happen (and only in the very special case
> where sink and source are switched at the same time due to a profile 
> change),
> so working around the problem by delaying the cork/uncork seems sufficient.
> If you feel it is really necessary, I can however work on it.

It would be nice to fix pa_source_output_cork() to support this use
case, but I'm ok with a workaround. I think the comments should be
clear about it being a workaround for brokenness in
pa_source_output_cork(), though.

> > > @@ -598,11 +624,20 @@ static void source_output_suspend_cb(pa_source_output *o, bool suspended) {
> > >           else
> > >               u->output_thread_info.push_called = false;
> > >   
> > > -    } else
> > > +        /* Do not cork the sink input if the source is suspended
> > > +         * because the sink was suspended and the source output
> > > +         * corked previously */
> > > +        if (pa_source_output_get_state(u->source_output) != PA_SOURCE_OUTPUT_CORKED)
> > > +            pa_sink_input_cork(u->sink_input, true);
> > 
> > I don't fully understand why the check is done before corking. Is it to
> > avoid corking the sink input if the source is idle-suspended? If so,
> > would it work if you changed the condition to checking the source's
> > suspend cause (that is, cork the sink input iff the source is idle-
> > suspended)?
> 
> This is needed to avoid corking both, sink input and source output. Once
> both are corked, nothing will uncork them again. The sequence is the 
> following:
> 
> 1) The sink is suspended by the user.
> 2) Due to this the source output will be corked.
> 3) If nothing else uses the source it will become idle-suspended after a 
> while.
> 4) This will call the suspend callback which then corks the sink input.
> 
> Then both are corked and module-loopback will effectively stop working, at
> least when you move the sink input away from the suspended sink. The sink
> input will not be uncorked because the source output is corked (and vice 
> versa).

To me the code looks like the sink input will be uncorked in this
situation. You say that it won't be uncorked "because the source output
is corked", but the code in sink_input_moving_cb() doesn't care about
the current source output state. The source output will be uncorked if
the new sink is not user-suspended, and that will make the source to
resume, and that will make the sink input uncork.

However, if the sink is both idle-suspended and user-suspended,
removing the user suspension won't restart the loopback, because there
are no notifications about changes in the suspend cause if the device
stays suspended.

This is annoyingly difficult to reason about. Isn't it so that if both
devices are running or idle-suspended, we should uncork both streams,
and in all other cases we should cork both streams? So there could be a
function like this:

void update_cork_statuses() {
    bool cork = false;

    if (sink->suspended && sink->suspend_cause != IDLE)
        cork = true;

    if (source->suspended && source->suspend_cause != IDLE)
        cork = true;

    pa_sink_input_cork(sink_input, cork);
    pa_source_output_cork(source_output, cork);
}

And then call that function whenever a stream moves or a device changes
its suspend status. I think that would make things a lot simpler to
understand. That still doesn't solve the problem of not getting a
notification when e.g. the sink suspend cause changes from idle+user to
just idle. Solving that problem will require adding a new notification
or changing the existing notifications to make the information
available.

> > > +
> > > +    } else {
> > >           /* Get effective source latency on unsuspend */
> > >           update_effective_source_latency(u, u->source_output->source, u->sink_input->sink);
> > >   
> > > -    pa_sink_input_cork(u->sink_input, suspended);
> > > +        /* Only uncork the sink input, if the sink is valid */
> > > +        if (u->sink_input->sink)
> > > +            pa_sink_input_cork(u->sink_input, false);
> > 
> > If the sink is not valid, what will trigger uncorking when the sink
> > becomes valid?
> > 
> > If it's possible to make the uncorking unconditional here, that would
> > make this simpler.
> > 
> 
> The problem is the same here, if the sink does not exist, uncorking will 
> fail.
> When the sink is invalid, it means it is moving. Uncorking will then be 
> triggered
> by the moving callback.

Okay. I think the comment should explain that the uncorking will be
done in the moving callback.

-- 
Tanu

https://www.patreon.com/tanuk