[pulseaudio-discuss] [PATCH v2 4/8] core: move sink-inputs conditionally when update default_sink
Georg Chini
georg at chini.tk
Sat Jul 13 09:31:30 UTC 2019
On 13.07.19 10:46, Georg Chini wrote:
> On 30.06.19 13:52, Georg Chini wrote:
>> On 17.01.19 07:53, Hui Wang wrote:
>>> When the default sink changes, the streams from the old default sink
>>> should be moved to the new default sink, unless the preferred_sink
>>> string is set to the old default sink and the active port of the old
>>> default sink is not unavailable
>>>
>>> Signed-off-by: Hui Wang <hui.wang at canonical.com>
>>> ---
>>> src/modules/dbus/iface-core.c | 2 +-
>>> src/modules/module-default-device-restore.c | 2 +-
>>> src/modules/module-switch-on-connect.c | 27 ++--------------
>>> src/pulsecore/cli-command.c | 2 +-
>>> src/pulsecore/core.c | 10 ++++--
>>> src/pulsecore/core.h | 4 +--
>>> src/pulsecore/device-port.c | 2 +-
>>> src/pulsecore/protocol-native.c | 2 +-
>>> src/pulsecore/sink.c | 35
>>> +++++++++++++++++++--
>>> src/pulsecore/sink.h | 6 ++++
>>> 10 files changed, 54 insertions(+), 38 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 f0cb29a7c..3ceac8b4f 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;
>>> @@ -88,7 +85,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;
>>> }
>>> @@ -103,28 +100,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 (pa_safe_streq(i->sink->name, i->preferred_sink) ||
>>> !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);
>>
>> I wonder if we could use something like
>> pa_core_set_temporary_default_sink() here
>> and have a variable core->temporary_default_sink that has even higher
>> priority
>> than core->configured_default_sink in the default sink selection
>> process. The temporary
>> default sink could be reset when the sink vanishes again or replaced
>> when another new
>> sink turns up. What module-switch-on-connect does is to temporarily
>> override the default
>> sink that the user configured. If we save this in another variable we
>> would not overwrite
>> the user configured default sink by a sink that is not expected to be
>> present the next
>> time PA is started. But that would be just nice to have.
>>
>>> 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..084a9f54b 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 pa_core_set_configured_default_sink(pa_core *core, const
>>> char *sink) {
>>> +void pa_core_set_configured_default_sink(pa_core *core, const char
>>> *sink, bool from_user) {
>>> char *old_sink;
>>> pa_assert(core);
>>> @@ -241,7 +241,7 @@ void pa_core_set_configured_default_sink(pa_core
>>> *core, const char *sink) {
>>> pa_log_info("configured_default_sink: %s -> %s",
>>> old_sink ? old_sink : "(unset)", sink ? sink :
>>> "(unset)");
>>> - pa_core_update_default_sink(core);
>>> + pa_core_update_default_sink(core, from_user);
>>> finish:
>>> pa_xfree(old_sink);
>>> @@ -306,7 +306,7 @@ static int compare_sinks(pa_sink *a, pa_sink *b) {
>>> return 0;
>>> }
>>> -void pa_core_update_default_sink(pa_core *core) {
>>> +void pa_core_update_default_sink(pa_core *core, bool from_user) {
>>> pa_sink *best = NULL;
>>> pa_sink *sink;
>>> uint32_t idx;
>>> @@ -343,6 +343,10 @@ void pa_core_update_default_sink(pa_core *core) {
>>> pa_subscription_post(core, PA_SUBSCRIPTION_EVENT_SERVER |
>>> PA_SUBSCRIPTION_EVENT_CHANGE, PA_INVALID_INDEX);
>>> pa_hook_fire(&core->hooks[PA_CORE_HOOK_DEFAULT_SINK_CHANGED],
>>> core->default_sink);
>>> +
>>> + /* try to move the streams from old_default_sink to the new
>>> default_sink conditionally */
>>> + if (old_default_sink)
>>> + pa_sink_move_streams_to_default_sink(core,
>>> old_default_sink, from_user);
>>> }
>>> /* a < b -> return -1
>>> diff --git a/src/pulsecore/core.h b/src/pulsecore/core.h
>>> index 38622f618..82573f001 100644
>>> --- a/src/pulsecore/core.h
>>> +++ b/src/pulsecore/core.h
>>> @@ -243,7 +243,7 @@ enum {
>>> pa_core* pa_core_new(pa_mainloop_api *m, bool shared, bool
>>> enable_memfd, size_t shm_size);
>>> -void pa_core_set_configured_default_sink(pa_core *core, const
>>> char *sink);
>>> +void pa_core_set_configured_default_sink(pa_core *core, const char
>>> *sink, bool from_user);
>>> void pa_core_set_configured_default_source(pa_core *core, const
>>> char *source);
>>> /* These should be called whenever something changes that may
>>> affect the
>>> @@ -255,7 +255,7 @@ void
>>> pa_core_set_configured_default_source(pa_core *core, const char
>>> *source);
>>> * pa_core_update_default_source() internally, so it's sufficient
>>> to only call
>>> * pa_core_update_default_sink() when something happens that
>>> affects the sink
>>> * ordering. */
>>> -void pa_core_update_default_sink(pa_core *core);
>>> +void pa_core_update_default_sink(pa_core *core, bool from_user);
>>> void pa_core_update_default_source(pa_core *core);
>>> void pa_core_set_exit_idle_time(pa_core *core, int time);
>>> diff --git a/src/pulsecore/device-port.c b/src/pulsecore/device-port.c
>>> index fb1277215..464c3f8a2 100644
>>> --- a/src/pulsecore/device-port.c
>>> +++ b/src/pulsecore/device-port.c
>>> @@ -96,7 +96,7 @@ void pa_device_port_set_available(pa_device_port
>>> *p, pa_available_t status) {
>>> * default sink/source, so port availability changes may
>>> affect the
>>> * default sink/source choice. */
>>> if (p->direction == PA_DIRECTION_OUTPUT)
>>> - pa_core_update_default_sink(p->core);
>>> + pa_core_update_default_sink(p->core, false);
>>> else
>>> pa_core_update_default_source(p->core);
>>> diff --git a/src/pulsecore/protocol-native.c
>>> b/src/pulsecore/protocol-native.c
>>> index 09e5aa3d5..7c6e5fb06 100644
>>> --- a/src/pulsecore/protocol-native.c
>>> +++ b/src/pulsecore/protocol-native.c
>>> @@ -4354,7 +4354,7 @@ static void
>>> command_set_default_sink_or_source(pa_pdispatch *pd, uint32_t comman
>>> sink = pa_namereg_get(c->protocol->core, s, PA_NAMEREG_SINK);
>>> CHECK_VALIDITY(c->pstream, sink, tag, PA_ERR_NOENTITY);
>>> - pa_core_set_configured_default_sink(c->protocol->core, sink->name);
>>> + pa_core_set_configured_default_sink(c->protocol->core, sink->name,
>>> true);
>>> }
>>> pa_pstream_send_simple_ack(c->pstream, tag);
>>> diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
>>> index d9851ce59..2d77188fd 100644
>>> --- a/src/pulsecore/sink.c
>>> +++ b/src/pulsecore/sink.c
>>> @@ -721,7 +721,7 @@ void pa_sink_put(pa_sink* s) {
>>> /* This function must be called after the
>>> PA_CORE_HOOK_SINK_PUT hook,
>>> * because module-switch-on-connect needs to know the old
>>> default sink */
>>> - pa_core_update_default_sink(s->core);
>>> + pa_core_update_default_sink(s->core, false);
>>> }
>>> /* Called from main context */
>>> @@ -750,7 +750,7 @@ void pa_sink_unlink(pa_sink* s) {
>>> pa_namereg_unregister(s->core, s->name);
>>> pa_idxset_remove_by_data(s->core->sinks, s, NULL);
>>> - pa_core_update_default_sink(s->core);
>>> + pa_core_update_default_sink(s->core, false);
>>> if (s->card)
>>> pa_idxset_remove_by_data(s->card->sinks, s, NULL);
>>> @@ -3416,7 +3416,7 @@ int pa_sink_set_port(pa_sink *s, const char
>>> *name, bool save) {
>>> pa_sink_set_port_latency_offset(s,
>>> s->active_port->latency_offset);
>>> /* The active port affects the default sink selection. */
>>> - pa_core_update_default_sink(s->core);
>>> + pa_core_update_default_sink(s->core, false);
>>> pa_hook_fire(&s->core->hooks[PA_CORE_HOOK_SINK_PORT_CHANGED], s);
>>> @@ -3922,3 +3922,32 @@ 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_to_default_sink(pa_core *core, pa_sink
>>> *old_sink, bool from_user) {
>>> + pa_sink_input *i;
>>> + uint32_t idx;
>>> + bool old_sink_is_unavailable;
>> Does this not give a warning about using uninitialized variables?
>>> +
>>> + pa_assert(core);
>>> + pa_assert(old_sink);
>>> +
>>> + if (old_sink == core->default_sink)
>>> + return;
>>> +
>>> + if (old_sink->active_port && old_sink->active_port->available
>>> == PA_AVAILABLE_NO)
>>> + old_sink_is_unavailable = true;
>> This is not sufficient to determine if the old sink is still
>> available. There are sinks
>> that do not have ports (null-sink, virtual sinks). I think you will
>> need another boolean
>> argument to the function which indicates availability of the old sink.
>>> +
>>> + 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 (pa_safe_streq(old_sink->name, i->preferred_sink) &&
>>> !old_sink_is_unavailable)
>>> + continue;
>>> +
>>> + pa_log_info("The sink input %u \"%s\" is moving to %s
>>> due to default_sink is changed.",
>>> + i->index,
>>> pa_strnull(pa_proplist_gets(i->proplist, PA_PROP_APPLICATION_NAME)),
>>> core->default_sink->name);
>>> + pa_sink_input_move_to(i, core->default_sink, from_user);
>>> + }
>>> + }
>>> +}
>>> diff --git a/src/pulsecore/sink.h b/src/pulsecore/sink.h
>>> index b9dd64f6f..b67d579dd 100644
>>> --- a/src/pulsecore/sink.h
>>> +++ b/src/pulsecore/sink.h
>>> @@ -558,6 +558,12 @@ int64_t
>>> pa_sink_get_latency_within_thread(pa_sink *s, bool allow_negative);
>>> * s->reference_volume and fires change notifications. */
>>> void pa_sink_set_reference_volume_direct(pa_sink *s, const
>>> pa_cvolume *volume);
>>> +/* When the default_sink is changed or the active_port of a sink
>>> is changed to
>>> + * PA_AVAILABLE_NO, this function is called to move the streams of
>>> the old
>>> + * default_sink or the sink with active_port equals PA_AVAILABLE_NO
>>> to the
>>> + * current default_sink conditionally*/
>>> +void pa_sink_move_streams_to_default_sink(pa_core *core, pa_sink
>>> *old_sink, bool from_user);
>>> +
>>> /* Verify that we called in IO context (aka 'thread context), or that
>>> * the sink is not yet set up, i.e. the thread not set up yet. See
>>> * pa_assert_io_context() in thread-mq.h for more information. */
>
>
> To summarize the outcome of Tanu's and my discussion, the result
> is that this patch remains unchanged apart from initializing
> old_sink_is_unavailable before use.
>
I just realized there is one more change necessary. You should
check if the sink input may move to the new default sink with
pa_sink_input_may_move_to() before executing the move and
if it may not move, keep the sink input on the old sink.
Again the example of a virtual sink that you set up on top of the
default sink. If you now configure the virtual sink as default, its
sink input should not move.
More information about the pulseaudio-discuss
mailing list