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
