Re: [pulseaudio-discuss] [PATCH 2/4] move stream after default sink is changed.
On 2018/12/23 上午2:57, Tanu Kaskinen wrote: On Sat, 2018-12-15 at 14:48 +0800, Hui Wang wrote: On 2018/12/13 下午5:16, Tanu Kaskinen wrote: 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. Supposing the sink0 is hdmi, sink1 is analog-speaker, and a music is playing on sink0 (default_sink is sink0), if users select the analog-speaker from UI (gnome-sound-setting), the sink1 is default_sink now, and the music should be played on sink1. If the streams don't move to new default_sink unconditionally, I have no idea how to let music be played on sink1 here. I don't see what the problem is - streams should by default move to the new default sink, but if the user has at some point manually moved a stream to sink0, making sink0 the preferred sink, then that stream shouldn't be moved automatically. If the user doesn't want to keep that old setting any more, they can move the stream manually to sink1, and since sink1 is the default sink at this point, the preferred_sink will be unset, so that stream will from then on behave like all other streams. OK, got it. thanks. ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH 2/4] move stream after default sink is changed.
On Sat, 2018-12-15 at 14:48 +0800, Hui Wang wrote: > On 2018/12/13 下午5:16, Tanu Kaskinen wrote: > > 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. > > Supposing the sink0 is hdmi, sink1 is analog-speaker, and a music is > playing on sink0 (default_sink is sink0), if users select the > analog-speaker from UI (gnome-sound-setting), the sink1 is default_sink > now, and the music should be played on sink1. If the streams don't move > to new default_sink unconditionally, I have no idea how to let music be > played on sink1 here. I don't see what the problem is - streams should by default move to the new default sink, but if the user has at some point manually moved a stream to sink0, making sink0 the preferred sink, then that stream shouldn't be moved automatically. If the user doesn't want to keep that old setting any more, they can move the stream manually to sink1, and since sink1 is the default sink at this point, the preferred_sink will be unset, so that stream will from then on behave like all other streams. -- 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
Re: [pulseaudio-discuss] [PATCH 2/4] move stream after default sink is changed.
On 2018/12/13 下午5:16, Tanu Kaskinen wrote:
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.
Supposing the sink0 is hdmi, sink1 is analog-speaker, and a music is
playing on sink0 (default_sink is sink0), if users select the
analog-speaker from UI (gnome-sound-setting), the sink1 is default_sink
now, and the music should be played on sink1. If the streams don't move
to new default_sink unconditionally, I have no idea how to let music be
played on sink1 here.
For the rest comments, I will fix them as suggested.
Thanks.
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(>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,
+
Re: [pulseaudio-discuss] [PATCH 2/4] move stream after default sink is changed.
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(>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
[pulseaudio-discuss] [PATCH 2/4] move stream after default sink is changed.
When default sink changes, the stream should be moved to new
default_sink too. 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.
Signed-off-by: Hui Wang
---
src/modules/dbus/iface-core.c | 2 +-
src/modules/module-default-device-restore.c | 2 +-
src/modules/module-switch-on-connect.c | 28 ++-
src/pulsecore/cli-command.c | 2 +-
src/pulsecore/core.c| 9 +++--
src/pulsecore/core.h| 4 +--
src/pulsecore/device-port.c | 2 +-
src/pulsecore/protocol-native.c | 2 +-
src/pulsecore/sink.c| 39 +++--
src/pulsecore/sink.h| 1 +
10 files changed, 52 insertions(+), 39 deletions(-)
diff --git a/src/modules/dbus/iface-core.c b/src/modules/dbus/iface-core.c
index 5229c0467..9763480d2 100644
--- a/src/modules/dbus/iface-core.c
+++ b/src/modules/dbus/iface-core.c
@@ -721,7 +721,7 @@ static void handle_set_fallback_sink(DBusConnection *conn,
DBusMessage *msg, DBu
return;
}
-pa_core_set_configured_default_sink(c->core,
pa_dbusiface_device_get_sink(fallback_sink)->name);
+pa_core_set_configured_default_sink(c->core,
pa_dbusiface_device_get_sink(fallback_sink)->name, true);
pa_dbus_send_empty_reply(conn, msg);
}
diff --git a/src/modules/module-default-device-restore.c
b/src/modules/module-default-device-restore.c
index c4dbad99f..33e74c071 100644
--- a/src/modules/module-default-device-restore.c
+++ b/src/modules/module-default-device-restore.c
@@ -69,7 +69,7 @@ static void load(struct userdata *u) {
pa_log_warn("Invalid sink name: %s", ln);
else {
pa_log_info("Restoring default sink '%s'.", ln);
-pa_core_set_configured_default_sink(u->core, ln);
+pa_core_set_configured_default_sink(u->core, ln, false);
}
} else if (errno != ENOENT)
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
@@ -54,9 +54,6 @@ struct userdata {
};
static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink,
void* userdata) {
-pa_sink_input *i;
-uint32_t idx;
-pa_sink *old_default_sink;
const char *s;
struct userdata *u = userdata;
@@ -85,7 +82,7 @@ static pa_hook_result_t sink_put_hook_callback(pa_core *c,
pa_sink *sink, void*
/* No default sink, nothing to move away, just set the new default */
if (!c->default_sink) {
-pa_core_set_configured_default_sink(c, sink->name);
+pa_core_set_configured_default_sink(c, sink->name, false);
return PA_HOOK_OK;
}
@@ -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 */
-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/cli-command.c b/src/pulsecore/cli-command.c
index 5205349bd..cc7addaa1 100644
--- a/src/pulsecore/cli-command.c
+++ b/src/pulsecore/cli-command.c
@@ -1036,7 +1036,7 @@ static int pa_cli_command_sink_default(pa_core *c,
pa_tokenizer *t, pa_strbuf *b
}
if ((s = pa_namereg_get(c, n, PA_NAMEREG_SINK)))
-pa_core_set_configured_default_sink(c, s->name);
+pa_core_set_configured_default_sink(c, s->name, true);
else
pa_strbuf_printf(buf, "Sink %s does not exist.\n", n);
diff --git a/src/pulsecore/core.c b/src/pulsecore/core.c
index cc4a6f38b..7d85e1788 100644
--- a/src/pulsecore/core.c
+++ b/src/pulsecore/core.c
@@ -226,7 +226,7 @@ static void core_free(pa_object *o) {
pa_xfree(c);
}
-void
