Re: [PATCH v2 4/8] core: move sink-inputs conditionally when update default_sink

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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.
> > > 
> > > >         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?

-- 
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




[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux