[pulseaudio-discuss] [PATCH v2 4/8] core: move sink-inputs conditionally when update default_sink
Georg Chini
georg at chini.tk
Mon Jul 15 19:28:35 UTC 2019
On 15.07.19 14:42, Tanu Kaskinen wrote:
> On Sat, 2019-07-13 at 11:31 +0200, Georg Chini wrote:
>> 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.
> Isn't this unnecessary, since pa_move_sink_input_to() will check
> pa_sink_input_may_move_to() anyway?
>
Yes, nothing bad will happen, you are right. But we still should
check it here I think, at least to avoid a false log message.
More information about the pulseaudio-discuss
mailing list