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

[Tanu Kaskinen]

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.

> @@ -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)?

> +
> +    } 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.

-- 
Tanu

https://www.patreon.com/tanuk