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

[Georg Chini]

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.

>
>> @@ -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).
See also the commit message.

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