[pulseaudio-discuss] [PATCH 2/4] move stream after default sink is changed.

[Tanu Kaskinen]

On Mon, 2018-11-05 at 09:47 +0800, Hui Wang wrote:
> When default sink changes, the stream should be moved to new
> default_sink too.

Except if the stream's preferred sink is the old default sink.

> If it is user to call change default function,
> all stream will move to new default sink unconditionally; if it
> is not, will check if stream binds to old_default_sink and the
> active_port staus of old_default_sink, then it will move the
> stream conditionally.

Why does it matter if the default sink changed due to user request or
some other reason? I don't think streams should be moved
unconditionally when the user changes the default sink.

I think it would be logical to not care about the port unavailability
in this patch, because there's a separate patch for handling that.

> diff --git a/src/modules/module-switch-on-connect.c b/src/modules/module-switch-on-connect.c
> index faa0f289f..4f01c701f 100644
> --- a/src/modules/module-switch-on-connect.c
> +++ b/src/modules/module-switch-on-connect.c

> @@ -100,29 +97,8 @@ static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, void*
>              return PA_HOOK_OK;
>          }
>  
> -    old_default_sink = c->default_sink;
> -
>      /* Actually do the switch to the new sink */

This comment isn't very helpful any more (not sure how helpful it was
before either), I'd remove it.

> -    pa_core_set_configured_default_sink(c, sink->name);
> -
> -    /* Now move all old inputs over */
> -    if (pa_idxset_size(old_default_sink->inputs) <= 0) {
> -        pa_log_debug("No sink inputs to move away.");
> -        return PA_HOOK_OK;
> -    }
> -
> -    PA_IDXSET_FOREACH(i, old_default_sink->inputs, idx) {
> -        if (i->preferred_sink != NULL || !PA_SINK_INPUT_IS_LINKED(i->state))
> -            continue;
> -
> -        if (pa_sink_input_move_to(i, sink, false) < 0)
> -            pa_log_info("Failed to move sink input %u \"%s\" to %s.", i->index,
> -                        pa_strnull(pa_proplist_gets(i->proplist, PA_PROP_APPLICATION_NAME)), sink->name);
> -        else
> -            pa_log_info("Successfully moved sink input %u \"%s\" to %s.", i->index,
> -                        pa_strnull(pa_proplist_gets(i->proplist, PA_PROP_APPLICATION_NAME)), sink->name);
> -    }
> -
> +    pa_core_set_configured_default_sink(c, sink->name, false);
>      return PA_HOOK_OK;
>  }

> diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
> index 566367f9e..63a3456e7 100644
> --- a/src/pulsecore/sink.c
> +++ b/src/pulsecore/sink.c

> @@ -3886,3 +3886,36 @@ void pa_sink_set_reference_volume_direct(pa_sink *s, const pa_cvolume *volume) {
>      pa_subscription_post(s->core, PA_SUBSCRIPTION_EVENT_SINK|PA_SUBSCRIPTION_EVENT_CHANGE, s->index);
>      pa_hook_fire(&s->core->hooks[PA_CORE_HOOK_SINK_VOLUME_CHANGED], s);
>  }
> +
> +void pa_sink_move_streams_from_oldsink_to_newsink(pa_sink *old_sink, pa_sink *new_sink, bool from_user) {

I think "pa_sink_move_streams_to_default_sink" would be a better name
for the function. It would also be good to have a comment explaining
when the function is used and which streams are not moved (that could
go to the header file).

> +    pa_sink_input *i;
> +    uint32_t idx;
> +    pa_device_port *o_active_p;
> +
> +    if (old_sink == new_sink)
> +	return;
> +
> +    if (old_sink == NULL)
> +	return;

It doesn't make sense to call this function with NULL, so this check
should be an assertion instead.

> +
> +    o_active_p = old_sink->active_port;

Note that not all sinks have ports, so this can be NULL.

Since we only care about the availability of the active port, I'd add a
"old_sink_is_unavailable" boolean to be used inside the loop.

> +    if (pa_idxset_size(old_sink->inputs) > 0) {
> +	    PA_IDXSET_FOREACH(i, old_sink->inputs, idx) {
> +		if (!PA_SINK_INPUT_IS_LINKED(i->state))
> +		    continue;
> +
> +		if (!from_user && i->preferred_sink != NULL && pa_safe_streq(old_sink->name, i->preferred_sink))

No need to check if i->preferred_sink is NULL, because pa_safe_streq()
checks that anyway.

> +		    if (o_active_p->available != PA_AVAILABLE_NO)
> +			continue;
> +
> +		if (pa_sink_input_move_to(i, new_sink, from_user) < 0)
> +		    pa_log_info("Failed to move sink input %u \"%s\" to %s.", i->index,
> +				pa_strnull(pa_proplist_gets(i->proplist, PA_PROP_APPLICATION_NAME)), new_sink->name);
> +		else
> +		    pa_log_info("Successfully moved sink input %u \"%s\" to %s.", i->index,
> +				pa_strnull(pa_proplist_gets(i->proplist, PA_PROP_APPLICATION_NAME)), new_sink->name);

Logging the reason for moving the stream before calling
pa_sink_input_move_to() would be very useful.

Here's a suggestion you don't need to implement, but you can if you
want: it would be nice to have good enough logging in
pa_sink_input_move_finish() so that callers don't all need to do their
own logging about the move result. This should be done in a separate
patch.

-- 
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk