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
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
