On Sun, 2019-06-30 at 13:52 +0200, 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@xxxxxxxxxxxxx>
> > ---
> > 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.
I'm against adding that kind of extra complexity without a very good
justification. module-switch-on-connect should become nearly obsolete
anyway after this patch set has been merged.
> >
> > 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.
The definition of an unavailable sink is that its active port is
unavailable. I don't know what kind of sinks you're thinking about
here, maybe virtual sinks that are on top of an unavailable hardware
sink? There are many places where we check the availability of a sink
this way, and I don't think it makes sense to be different here.
> > +
> > + 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. */
>
>
--
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
