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