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

Submitted by Hui Wang on Jan. 17, 2019, 6:53 a.m.

Details

Message ID 20190117065340.18712-5-hui.wang@canonical.com
State New
Headers show
Series "Change the bool sink_save to char *preferred_sink" ( rev: 1 ) in PulseAudio

Not browsing as part of any series.

Commit Message

Hui Wang Jan. 17, 2019, 6:53 a.m.
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@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(-)

Patch hide | download patch | download mbox

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);
 
     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;
+
+    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;
+
+    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. */

Comments

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@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. */
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@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.

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. */
> 
>
On 01.07.19 07:08, Tanu Kaskinen wrote:
> 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@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.
> 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.

We already talked about this some time ago and you agreed
that this would be useful. Maybe you remember that discussion.
Consider you add your BT speakers and module-switch-on-connect
makes them the default sink. When you remove them, there is
no guarantee that the default sink will revert to what the user
configured before. Instead PA will choose the sink which it thinks
is best. Also, if you remove the device after a shutdown, PA will
still keep it as the configured default sink and the user will loose
what was configured.

So in my opinion we should distinguish if the server changed the
default sink or the user did, though as already said above, it is
not mandatory but only nice to have.

I do not see why module-switch-on-connect would become obsolete
- it switches the default sink to a newly appeared sink and there is
nothing in these patches which does that (unless the newly appeared
sink is the configured default sink).


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

I don't understand. Virtual sinks don't have ports. So checking only
sinks that have an active port excludes all sinks that don't have
ports like the null-sink and virtual sinks. Following your definition
above, those sinks are never available.
On 01.07.19 08:03, Georg Chini wrote:
> On 01.07.19 07:08, Tanu Kaskinen wrote:
>> 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



>>>>    +
>>>> +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.
>
> I don't understand. Virtual sinks don't have ports. So checking only
> sinks that have an active port excludes all sinks that don't have
> ports like the null-sink and virtual sinks. Following your definition
> above, those sinks are never available.
>
>
Checking the code, only alsa, bluetooth and raop sinks define ports and
the "many places" you are referring to are compare_sinks() and
compare_sources() in core.c and a check in module-switch-on-connect,
which is used to determine the availability of the default sink.
On Mon, 2019-07-01 at 08:48 +0200, Georg Chini wrote:
> On 01.07.19 08:03, Georg Chini wrote:
> > On 01.07.19 07:08, Tanu Kaskinen wrote:
> > > 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
> 
> 
> > > > >    +
> > > > > +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.
> > 
> > I don't understand. Virtual sinks don't have ports. So checking only
> > sinks that have an active port excludes all sinks that don't have
> > ports like the null-sink and virtual sinks. Following your definition
> > above, those sinks are never available.

You have it reversed: following my definition above, those sinks are
always available.

> Checking the code, only alsa, bluetooth and raop sinks define ports and
> the "many places" you are referring to are compare_sinks() and
> compare_sources() in core.c and a check in module-switch-on-connect,
> which is used to determine the availability of the default sink.

At least module-stream-restore checks device availability too (although
probably not anymore after this patch set, because module-stream-
restore won't do the routing directly anymore, it will just restore the
preferred_sink variable, which can be done regardless of the sink
availability).

Extending the hardware sink availability to filter sinks probably makes
sense, but I think that's a topic for a separate patch (or patches).

You suggested an extra flag for pa_sink_move_streams_to_default_sink(),
which seems unnecessary to me. The function can in any case figure out
the availability by itself (possibly using a helper function
pa_sink_is_available() that can handle filter sinks too).
On Mon, 2019-07-01 at 08:03 +0200, Georg Chini wrote:
> On 01.07.19 07:08, Tanu Kaskinen wrote:
> > 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@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.
> > 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.
> 
> We already talked about this some time ago and you agreed
> that this would be useful. Maybe you remember that discussion.

I remember that we discussed my initial attempt at implementing what
this patch set does. I probably should go back to that discussion to
find out why I agreed that we should add a separate "temporary default
sink" variable.

> Consider you add your BT speakers and module-switch-on-connect
> makes them the default sink. When you remove them, there is
> no guarantee that the default sink will revert to what the user
> configured before. Instead PA will choose the sink which it thinks
> is best. Also, if you remove the device after a shutdown, PA will
> still keep it as the configured default sink and the user will loose
> what was configured.
> 
> So in my opinion we should distinguish if the server changed the
> default sink or the user did, though as already said above, it is
> not mandatory but only nice to have.
> 
> I do not see why module-switch-on-connect would become obsolete
> - it switches the default sink to a newly appeared sink and there is
> nothing in these patches which does that (unless the newly appeared
> sink is the configured default sink).

When a new sink appears, pa_core_update_default_sink() is called. Since
PulseAudio 11.1, bluetooth and USB sinks have higher priority than the
internal sound card, so pa_core_update_default_sink() will switch to
the BT speakers in your example. The main benefit of module-switch-on-
connect was that it moved existing streams to the new sink, but after
this patch set that's handled by the core. Therefore there's much less
need for module-switch-on-connect.
On 02.07.19 08:43, Tanu Kaskinen wrote:
> On Mon, 2019-07-01 at 08:03 +0200, Georg Chini wrote:
>> On 01.07.19 07:08, Tanu Kaskinen wrote:
>>> 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@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.
>>> 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.
>> We already talked about this some time ago and you agreed
>> that this would be useful. Maybe you remember that discussion.
> I remember that we discussed my initial attempt at implementing what
> this patch set does. I probably should go back to that discussion to
> find out why I agreed that we should add a separate "temporary default
> sink" variable.

You did not exactly agree to adding a temporary_default_sink
variable. You agreed however (after some discussion) that
what module-switch-on-connect does is a temporary override
of the configured default sink and that there should be a way
to restore the original default sink. (The user may have set
another than the highest priority sink as default, see also
below).

It seems quite simple to add such a variable and replace it
when a new sink turns up or set it to NULL if the current
temporary default sink is removed. This would then make
it really easy to implement the module-switch-on-connect
functionality in the core at some later point.

>
>> Consider you add your BT speakers and module-switch-on-connect
>> makes them the default sink. When you remove them, there is
>> no guarantee that the default sink will revert to what the user
>> configured before. Instead PA will choose the sink which it thinks
>> is best. Also, if you remove the device after a shutdown, PA will
>> still keep it as the configured default sink and the user will loose
>> what was configured.
>>
>> So in my opinion we should distinguish if the server changed the
>> default sink or the user did, though as already said above, it is
>> not mandatory but only nice to have.
>>
>> I do not see why module-switch-on-connect would become obsolete
>> - it switches the default sink to a newly appeared sink and there is
>> nothing in these patches which does that (unless the newly appeared
>> sink is the configured default sink).
> When a new sink appears, pa_core_update_default_sink() is called. Since
> PulseAudio 11.1, bluetooth and USB sinks have higher priority than the
> internal sound card, so pa_core_update_default_sink() will switch to
> the BT speakers in your example. The main benefit of module-switch-on-
> connect was that it moved existing streams to the new sink, but after
> this patch set that's handled by the core. Therefore there's much less
> need for module-switch-on-connect.
>
This is true, but only relevant if there is no configured default
sink. If the configured default sink is set, there will be no switch
to a newly appearing sink because the configured default sink
is prioritized over all other sinks. In this case you still need
module-switch-on-connect.
On 02.07.19 06:49, Tanu Kaskinen wrote:
> On Mon, 2019-07-01 at 08:48 +0200, Georg Chini wrote:
>> On 01.07.19 08:03, Georg Chini wrote:
>>> On 01.07.19 07:08, Tanu Kaskinen wrote:
>>>> 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
>>
>>>>>>     +
>>>>>> +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.
>>> I don't understand. Virtual sinks don't have ports. So checking only
>>> sinks that have an active port excludes all sinks that don't have
>>> ports like the null-sink and virtual sinks. Following your definition
>>> above, those sinks are never available.
> You have it reversed: following my definition above, those sinks are
> always available.
Right, sorry. (I seem to be reading things wrong quite often
the last few weeks ... I'll do my best to be more thorough in
the future.)
>
>> Checking the code, only alsa, bluetooth and raop sinks define ports and
>> the "many places" you are referring to are compare_sinks() and
>> compare_sources() in core.c and a check in module-switch-on-connect,
>> which is used to determine the availability of the default sink.
> At least module-stream-restore checks device availability too (although
> probably not anymore after this patch set, because module-stream-
> restore won't do the routing directly anymore, it will just restore the
> preferred_sink variable, which can be done regardless of the sink
> availability).
>
> Extending the hardware sink availability to filter sinks probably makes
> sense, but I think that's a topic for a separate patch (or patches).
>
> You suggested an extra flag for pa_sink_move_streams_to_default_sink(),
> which seems unnecessary to me. The function can in any case figure out
> the availability by itself (possibly using a helper function
> pa_sink_is_available() that can handle filter sinks too).
>
I think some additional check is still needed at least for 
s->unlink_requested
because when a sink is going to be unlinked and the function is called from
pa_sink_unlink() in patch 7, there may be nothing else that indicates 
that the
sink is going to be removed. When I proposed an additional argument, I did
not see that pa_sink_unlink() already sets a flag that can be used.

So I would suggest that the helper function checks on
- link state
- port availability
- s->unlink_requested
- suspend state (any reason apart from SUSPEND_IDLE)

Using the helper function in other places (and extending it if I forgot 
something
in the list above) could then be done in a separate patch. Are you OK 
with that?
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@canonical.com>
> ---
...
> +
> +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;
> +
> +    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;
> +
> +    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);
> +        }
> +    }
> +}

If the last sink is in the process of getting removed,  old_sink would 
be the previous
default sink and core->default_sink would be NULL. Is it possible, that 
in this situation
there are still inputs on the sink? Maybe the function should check if 
core->default_sink
is not NULL, just to make sure.
On Tue, 2019-07-02 at 09:08 +0200, Georg Chini wrote:
> On 02.07.19 08:43, Tanu Kaskinen wrote:
> > On Mon, 2019-07-01 at 08:03 +0200, Georg Chini wrote:
> > > On 01.07.19 07:08, Tanu Kaskinen wrote:
> > > > 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@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.
> > > > 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.
> > > We already talked about this some time ago and you agreed
> > > that this would be useful. Maybe you remember that discussion.
> > I remember that we discussed my initial attempt at implementing what
> > this patch set does. I probably should go back to that discussion to
> > find out why I agreed that we should add a separate "temporary default
> > sink" variable.
> 
> You did not exactly agree to adding a temporary_default_sink
> variable. You agreed however (after some discussion) that
> what module-switch-on-connect does is a temporary override
> of the configured default sink and that there should be a way
> to restore the original default sink. (The user may have set
> another than the highest priority sink as default, see also
> below).
> 
> It seems quite simple to add such a variable and replace it
> when a new sink turns up or set it to NULL if the current
> temporary default sink is removed. This would then make
> it really easy to implement the module-switch-on-connect
> functionality in the core at some later point.

I'm concerned about the complexity of answering the question "how is
the default sink determined". The fact that "default sink" and
"configured default sink" are different things is already bothersome.
Your point about restoring the old configured default sink is
definitely valid, although that problem seems to be not specific to
module-switch-on-connect: if there are 3 sinks, the user can't specify
which of them is the second most preferred sink in case the configured
default sink goes away. Having a priority list of sinks based on manual
configuration history might be a better way to solve this (although I'm
afraid it's not that simple if there are sink with multiple ports).

> > > Consider you add your BT speakers and module-switch-on-connect
> > > makes them the default sink. When you remove them, there is
> > > no guarantee that the default sink will revert to what the user
> > > configured before. Instead PA will choose the sink which it thinks
> > > is best. Also, if you remove the device after a shutdown, PA will
> > > still keep it as the configured default sink and the user will loose
> > > what was configured.
> > > 
> > > So in my opinion we should distinguish if the server changed the
> > > default sink or the user did, though as already said above, it is
> > > not mandatory but only nice to have.
> > > 
> > > I do not see why module-switch-on-connect would become obsolete
> > > - it switches the default sink to a newly appeared sink and there is
> > > nothing in these patches which does that (unless the newly appeared
> > > sink is the configured default sink).
> > When a new sink appears, pa_core_update_default_sink() is called. Since
> > PulseAudio 11.1, bluetooth and USB sinks have higher priority than the
> > internal sound card, so pa_core_update_default_sink() will switch to
> > the BT speakers in your example. The main benefit of module-switch-on-
> > connect was that it moved existing streams to the new sink, but after
> > this patch set that's handled by the core. Therefore there's much less
> > need for module-switch-on-connect.
> > 
> This is true, but only relevant if there is no configured default
> sink. If the configured default sink is set, there will be no switch
> to a newly appearing sink because the configured default sink
> is prioritized over all other sinks. In this case you still need
> module-switch-on-connect.

My estimation is that this is very rarely a problem.

If you have manually selected an external sound card as the default
sink and you then plug in another external sound card, then you may or
may not want to automatically switch to the new sound card. If you
fiddle a lot with two external sound cards and you always want to use
the last one plugged in, then module-switch-on-connect is still useful,
but in this case it doesn't really matter that your old default device
choice is forgotten, because PulseAudio will anyway prefer the
remaining external sound card.

If you have an external sound card and you have manually selected the
internal sound card as the default sink, then module-switch-on-connect
seems like a bad idea, because you probably want to keep the internal
sound card as the default even after plugging out the external sound
card and plugging it back in. If your preference has changed, then it's
expected that you have to restate your preference by manually selecting
the external sound card.
On Tue, 2019-07-02 at 10:25 +0200, Georg Chini wrote:
> On 02.07.19 06:49, Tanu Kaskinen wrote:
> > On Mon, 2019-07-01 at 08:48 +0200, Georg Chini wrote:
> > > On 01.07.19 08:03, Georg Chini wrote:
> > > > On 01.07.19 07:08, Tanu Kaskinen wrote:
> > > > > 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
> > > > > > >     +
> > > > > > > +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.
> > > > I don't understand. Virtual sinks don't have ports. So checking only
> > > > sinks that have an active port excludes all sinks that don't have
> > > > ports like the null-sink and virtual sinks. Following your definition
> > > > above, those sinks are never available.
> > You have it reversed: following my definition above, those sinks are
> > always available.
> Right, sorry. (I seem to be reading things wrong quite often
> the last few weeks ... I'll do my best to be more thorough in
> the future.)
> > > Checking the code, only alsa, bluetooth and raop sinks define ports and
> > > the "many places" you are referring to are compare_sinks() and
> > > compare_sources() in core.c and a check in module-switch-on-connect,
> > > which is used to determine the availability of the default sink.
> > At least module-stream-restore checks device availability too (although
> > probably not anymore after this patch set, because module-stream-
> > restore won't do the routing directly anymore, it will just restore the
> > preferred_sink variable, which can be done regardless of the sink
> > availability).
> > 
> > Extending the hardware sink availability to filter sinks probably makes
> > sense, but I think that's a topic for a separate patch (or patches).
> > 
> > You suggested an extra flag for pa_sink_move_streams_to_default_sink(),
> > which seems unnecessary to me. The function can in any case figure out
> > the availability by itself (possibly using a helper function
> > pa_sink_is_available() that can handle filter sinks too).
> > 
> I think some additional check is still needed at least for 
> s->unlink_requested
> because when a sink is going to be unlinked and the function is called from
> pa_sink_unlink() in patch 7, there may be nothing else that indicates 
> that the
> sink is going to be removed. When I proposed an additional argument, I did
> not see that pa_sink_unlink() already sets a flag that can be used.
> 
> So I would suggest that the helper function checks on
> - link state

I don't expect this to be necessary, although it can't hurt either.

> - port availability
> - s->unlink_requested

If this is indeed used from pa_sink_unlink() to rescue streams, then
checking this seems appropriate (and can't hurt anyway).

> - suspend state (any reason apart from SUSPEND_IDLE)

This doesn't seem appropriate, unless we start avoiding suspended sinks
in the same way we avoid unavailable sinks. That would mean that we
would never use a suspended sink as the default sink. That would
probably make sense actually, so I'm just pointing out that we need to
be consistent: if we avoid suspended sinks in one situation, we should
avoid them in all situations.

> Using the helper function in other places (and extending it if I forgot 
> something
> in the list above) could then be done in a separate patch. Are you OK 
> with that?

Yes, I think a helper function can be added, and if it's added, it's
added in a separate patch.
On 05.07.19 09:41, Tanu Kaskinen wrote:
> On Tue, 2019-07-02 at 09:08 +0200, Georg Chini wrote:
>> On 02.07.19 08:43, Tanu Kaskinen wrote:
>>> On Mon, 2019-07-01 at 08:03 +0200, Georg Chini wrote:
>>>> On 01.07.19 07:08, Tanu Kaskinen wrote:
>>>>> 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
>>>>>>>
>>>>>>>
>>>>>>>      
>>>>>>>      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.
>>>> We already talked about this some time ago and you agreed
>>>> that this would be useful. Maybe you remember that discussion.
>>> I remember that we discussed my initial attempt at implementing what
>>> this patch set does. I probably should go back to that discussion to
>>> find out why I agreed that we should add a separate "temporary default
>>> sink" variable.
>> You did not exactly agree to adding a temporary_default_sink
>> variable. You agreed however (after some discussion) that
>> what module-switch-on-connect does is a temporary override
>> of the configured default sink and that there should be a way
>> to restore the original default sink. (The user may have set
>> another than the highest priority sink as default, see also
>> below).
>>
>> It seems quite simple to add such a variable and replace it
>> when a new sink turns up or set it to NULL if the current
>> temporary default sink is removed. This would then make
>> it really easy to implement the module-switch-on-connect
>> functionality in the core at some later point.
> I'm concerned about the complexity of answering the question "how is
> the default sink determined". The fact that "default sink" and
> "configured default sink" are different things is already bothersome.

Why do you find this bothersome? I think it is pretty obvious
that the two are not necessarily identical.

> Your point about restoring the old configured default sink is
> definitely valid, although that problem seems to be not specific to
> module-switch-on-connect: if there are 3 sinks, the user can't specify
> which of them is the second most preferred sink in case the configured
> default sink goes away. Having a priority list of sinks based on manual
> configuration history might be a better way to solve this (although I'm
> afraid it's not that simple if there are sink with multiple ports).

I remember you proposed some kind of history, but I personally
don't think a long history is required. Just going back to the last
sink if I temporarily overwrote the default sink seems fully
sufficient from a user perspective.

But I see you don't want something like that (at least not now), so
I guess we forget my initial suggestion. It will probably work correctly
anyway, because the user will have the configured default sink set.
See also below.

>
>>>> Consider you add your BT speakers and module-switch-on-connect
>>>> makes them the default sink. When you remove them, there is
>>>> no guarantee that the default sink will revert to what the user
>>>> configured before. Instead PA will choose the sink which it thinks
>>>> is best. Also, if you remove the device after a shutdown, PA will
>>>> still keep it as the configured default sink and the user will loose
>>>> what was configured.
>>>>
>>>> So in my opinion we should distinguish if the server changed the
>>>> default sink or the user did, though as already said above, it is
>>>> not mandatory but only nice to have.
>>>>
>>>> I do not see why module-switch-on-connect would become obsolete
>>>> - it switches the default sink to a newly appeared sink and there is
>>>> nothing in these patches which does that (unless the newly appeared
>>>> sink is the configured default sink).
>>> When a new sink appears, pa_core_update_default_sink() is called. Since
>>> PulseAudio 11.1, bluetooth and USB sinks have higher priority than the
>>> internal sound card, so pa_core_update_default_sink() will switch to
>>> the BT speakers in your example. The main benefit of module-switch-on-
>>> connect was that it moved existing streams to the new sink, but after
>>> this patch set that's handled by the core. Therefore there's much less
>>> need for module-switch-on-connect.
>>>
>> This is true, but only relevant if there is no configured default
>> sink. If the configured default sink is set, there will be no switch
>> to a newly appearing sink because the configured default sink
>> is prioritized over all other sinks. In this case you still need
>> module-switch-on-connect.
> My estimation is that this is very rarely a problem.

Mh, my estimation is, that this will be the normal case. At some
point the user will set a default sink manually, and from that point
on, automatic switching will no longer work. The configured default
sink is never unset once it is set.

>
> If you have manually selected an external sound card as the default
> sink and you then plug in another external sound card, then you may or
> may not want to automatically switch to the new sound card. If you
> fiddle a lot with two external sound cards and you always want to use
> the last one plugged in, then module-switch-on-connect is still useful,
> but in this case it doesn't really matter that your old default device
> choice is forgotten, because PulseAudio will anyway prefer the
> remaining external sound card.
This topic is not about forgetting the last sink, but about switching
to a new sink at all. Once you manually set the default sink, it will
never switch automatically without module-switch-on-connect,
because the default sink selection process prefers the configured
default sink over all other sinks.
>
> If you have an external sound card and you have manually selected the
> internal sound card as the default sink, then module-switch-on-connect
> seems like a bad idea, because you probably want to keep the internal
> sound card as the default even after plugging out the external sound
> card and plugging it back in. If your preference has changed, then it's
> expected that you have to restate your preference by manually selecting
> the external sound card.
>
On 05.07.19 09:56, Tanu Kaskinen wrote:
> On Tue, 2019-07-02 at 10:25 +0200, Georg Chini wrote:
>> On 02.07.19 06:49, Tanu Kaskinen wrote:
>>> On Mon, 2019-07-01 at 08:48 +0200, Georg Chini wrote:
>>>> On 01.07.19 08:03, Georg Chini wrote:
>>>>> On 01.07.19 07:08, Tanu Kaskinen wrote:
>>>>>> 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
>>>>>>>>      +
>>>>>>>> +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.
>>>>> I don't understand. Virtual sinks don't have ports. So checking only
>>>>> sinks that have an active port excludes all sinks that don't have
>>>>> ports like the null-sink and virtual sinks. Following your definition
>>>>> above, those sinks are never available.
>>> You have it reversed: following my definition above, those sinks are
>>> always available.
>> Right, sorry. (I seem to be reading things wrong quite often
>> the last few weeks ... I'll do my best to be more thorough in
>> the future.)
>>>> Checking the code, only alsa, bluetooth and raop sinks define ports and
>>>> the "many places" you are referring to are compare_sinks() and
>>>> compare_sources() in core.c and a check in module-switch-on-connect,
>>>> which is used to determine the availability of the default sink.
>>> At least module-stream-restore checks device availability too (although
>>> probably not anymore after this patch set, because module-stream-
>>> restore won't do the routing directly anymore, it will just restore the
>>> preferred_sink variable, which can be done regardless of the sink
>>> availability).
>>>
>>> Extending the hardware sink availability to filter sinks probably makes
>>> sense, but I think that's a topic for a separate patch (or patches).
>>>
>>> You suggested an extra flag for pa_sink_move_streams_to_default_sink(),
>>> which seems unnecessary to me. The function can in any case figure out
>>> the availability by itself (possibly using a helper function
>>> pa_sink_is_available() that can handle filter sinks too).
>>>
>> I think some additional check is still needed at least for
>> s->unlink_requested
>> because when a sink is going to be unlinked and the function is called from
>> pa_sink_unlink() in patch 7, there may be nothing else that indicates
>> that the
>> sink is going to be removed. When I proposed an additional argument, I did
>> not see that pa_sink_unlink() already sets a flag that can be used.
>>
>> So I would suggest that the helper function checks on
>> - link state
> I don't expect this to be necessary, although it can't hurt either.
>
>> - port availability
>> - s->unlink_requested
> If this is indeed used from pa_sink_unlink() to rescue streams, then
> checking this seems appropriate (and can't hurt anyway).
>
>> - suspend state (any reason apart from SUSPEND_IDLE)
> This doesn't seem appropriate, unless we start avoiding suspended sinks
> in the same way we avoid unavailable sinks. That would mean that we
> would never use a suspended sink as the default sink. That would
> probably make sense actually, so I'm just pointing out that we need to
> be consistent: if we avoid suspended sinks in one situation, we should
> avoid them in all situations.

Yes, we should do that. Therefore my suggestion is to add a helper
function to this patch set. For consistency we could exclude the
suspend state for the moment, though I would recommend to include
it. Using the helper function outside this patch set in all places where
we check availability should then be the task of another patch series.
By suspended I guess you mean suspended apart from idle, correct?

>
>> Using the helper function in other places (and extending it if I forgot
>> something
>> in the list above) could then be done in a separate patch. Are you OK
>> with that?
> Yes, I think a helper function can be added, and if it's added, it's
> added in a separate patch.
>
I don't really understand what your suggestion means for the patch set
we are discussing. Do you mean the helper should be added to that
set but in a separate patch? Or do you mean it should not be added
to this set? If it is not added, at least an additional check for
s->unlink_requested is required in patch 7.
On 05.07.19 10:57, Georg Chini wrote:
> On 05.07.19 09:41, Tanu Kaskinen wrote:
>> On Tue, 2019-07-02 at 09:08 +0200, Georg Chini wrote:
>>> On 02.07.19 08:43, Tanu Kaskinen wrote:
>>>> On Mon, 2019-07-01 at 08:03 +0200, Georg Chini wrote:
>>>>> On 01.07.19 07:08, Tanu Kaskinen wrote:
>>>>>> 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
>>>>>>>>
>>>>>>>>
>>>>>>>>           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.
>>>>> We already talked about this some time ago and you agreed
>>>>> that this would be useful. Maybe you remember that discussion.
>>>> I remember that we discussed my initial attempt at implementing what
>>>> this patch set does. I probably should go back to that discussion to
>>>> find out why I agreed that we should add a separate "temporary default
>>>> sink" variable.
>>> You did not exactly agree to adding a temporary_default_sink
>>> variable. You agreed however (after some discussion) that
>>> what module-switch-on-connect does is a temporary override
>>> of the configured default sink and that there should be a way
>>> to restore the original default sink. (The user may have set
>>> another than the highest priority sink as default, see also
>>> below).
>>>
>>> It seems quite simple to add such a variable and replace it
>>> when a new sink turns up or set it to NULL if the current
>>> temporary default sink is removed. This would then make
>>> it really easy to implement the module-switch-on-connect
>>> functionality in the core at some later point.
>> I'm concerned about the complexity of answering the question "how is
>> the default sink determined". The fact that "default sink" and
>> "configured default sink" are different things is already bothersome.
>
> Why do you find this bothersome? I think it is pretty obvious
> that the two are not necessarily identical.
>
>> Your point about restoring the old configured default sink is
>> definitely valid, although that problem seems to be not specific to
>> module-switch-on-connect: if there are 3 sinks, the user can't specify
>> which of them is the second most preferred sink in case the configured
>> default sink goes away. Having a priority list of sinks based on manual
>> configuration history might be a better way to solve this (although I'm
>> afraid it's not that simple if there are sink with multiple ports).
>
> I remember you proposed some kind of history, but I personally
> don't think a long history is required. Just going back to the last
> sink if I temporarily overwrote the default sink seems fully
> sufficient from a user perspective.
>
> But I see you don't want something like that (at least not now), so
> I guess we forget my initial suggestion. It will probably work correctly
> anyway, because the user will have the configured default sink set.
> See also below.
Thinking again, it won't work correctly because we overwrite the configured
default sink here. This means when disconnecting the device we have lost
the user configured sink and go back to what PA thinks is best. So in all
cases where the user had configured a low priority sink as default, we will
move to the wrong sink.
On Fri, 2019-07-05 at 10:57 +0200, Georg Chini wrote:
> On 05.07.19 09:41, Tanu Kaskinen wrote:
> > On Tue, 2019-07-02 at 09:08 +0200, Georg Chini wrote:
> > > On 02.07.19 08:43, Tanu Kaskinen wrote:
> > > > On Mon, 2019-07-01 at 08:03 +0200, Georg Chini wrote:
> > > > > On 01.07.19 07:08, Tanu Kaskinen wrote:
> > > > > > 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
> > > > > > > > 
> > > > > > > > 
> > > > > > > >      
> > > > > > > >      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.
> > > > > We already talked about this some time ago and you agreed
> > > > > that this would be useful. Maybe you remember that discussion.
> > > > I remember that we discussed my initial attempt at implementing what
> > > > this patch set does. I probably should go back to that discussion to
> > > > find out why I agreed that we should add a separate "temporary default
> > > > sink" variable.
> > > You did not exactly agree to adding a temporary_default_sink
> > > variable. You agreed however (after some discussion) that
> > > what module-switch-on-connect does is a temporary override
> > > of the configured default sink and that there should be a way
> > > to restore the original default sink. (The user may have set
> > > another than the highest priority sink as default, see also
> > > below).
> > > 
> > > It seems quite simple to add such a variable and replace it
> > > when a new sink turns up or set it to NULL if the current
> > > temporary default sink is removed. This would then make
> > > it really easy to implement the module-switch-on-connect
> > > functionality in the core at some later point.
> > I'm concerned about the complexity of answering the question "how is
> > the default sink determined". The fact that "default sink" and
> > "configured default sink" are different things is already bothersome.
> 
> Why do you find this bothersome? I think it is pretty obvious
> that the two are not necessarily identical.

I meant that when thinking from the user's perspective, it would be
nice if it would be straightforward how the default sink is determined.
But maybe I'll just have to give up on the hope on that, since I think
having default sink history is worth having, and that's not really any
less complicated than what you propose, so I don't think the argument I
made here has much weight.

> > Your point about restoring the old configured default sink is
> > definitely valid, although that problem seems to be not specific to
> > module-switch-on-connect: if there are 3 sinks, the user can't specify
> > which of them is the second most preferred sink in case the configured
> > default sink goes away. Having a priority list of sinks based on manual
> > configuration history might be a better way to solve this (although I'm
> > afraid it's not that simple if there are sink with multiple ports).
> 
> I remember you proposed some kind of history, but I personally
> don't think a long history is required. Just going back to the last
> sink if I temporarily overwrote the default sink seems fully
> sufficient from a user perspective.
> 
> But I see you don't want something like that (at least not now), so
> I guess we forget my initial suggestion. It will probably work correctly
> anyway, because the user will have the configured default sink set.
> See also below.
> 
> > > > > Consider you add your BT speakers and module-switch-on-connect
> > > > > makes them the default sink. When you remove them, there is
> > > > > no guarantee that the default sink will revert to what the user
> > > > > configured before. Instead PA will choose the sink which it thinks
> > > > > is best. Also, if you remove the device after a shutdown, PA will
> > > > > still keep it as the configured default sink and the user will loose
> > > > > what was configured.
> > > > > 
> > > > > So in my opinion we should distinguish if the server changed the
> > > > > default sink or the user did, though as already said above, it is
> > > > > not mandatory but only nice to have.
> > > > > 
> > > > > I do not see why module-switch-on-connect would become obsolete
> > > > > - it switches the default sink to a newly appeared sink and there is
> > > > > nothing in these patches which does that (unless the newly appeared
> > > > > sink is the configured default sink).
> > > > When a new sink appears, pa_core_update_default_sink() is called. Since
> > > > PulseAudio 11.1, bluetooth and USB sinks have higher priority than the
> > > > internal sound card, so pa_core_update_default_sink() will switch to
> > > > the BT speakers in your example. The main benefit of module-switch-on-
> > > > connect was that it moved existing streams to the new sink, but after
> > > > this patch set that's handled by the core. Therefore there's much less
> > > > need for module-switch-on-connect.
> > > > 
> > > This is true, but only relevant if there is no configured default
> > > sink. If the configured default sink is set, there will be no switch
> > > to a newly appearing sink because the configured default sink
> > > is prioritized over all other sinks. In this case you still need
> > > module-switch-on-connect.
> > My estimation is that this is very rarely a problem.
> 
> Mh, my estimation is, that this will be the normal case. At some
> point the user will set a default sink manually, and from that point
> on, automatic switching will no longer work. The configured default
> sink is never unset once it is set.

Yes, sure, but I tried to explain why this is rarely a problem. (In
another thread you described a valid use case where this becomes a
problem, but I don't think that's a "normal case".)

> > If you have manually selected an external sound card as the default
> > sink and you then plug in another external sound card, then you may or
> > may not want to automatically switch to the new sound card. If you
> > fiddle a lot with two external sound cards and you always want to use
> > the last one plugged in, then module-switch-on-connect is still useful,
> > but in this case it doesn't really matter that your old default device
> > choice is forgotten, because PulseAudio will anyway prefer the
> > remaining external sound card.
> This topic is not about forgetting the last sink, but about switching
> to a new sink at all. Once you manually set the default sink, it will
> never switch automatically without module-switch-on-connect,
> because the default sink selection process prefers the configured
> default sink over all other sinks.

You're assuming that we always want to switch to a newly plugged in
device. Let's say that you have just one USB device, no other external
devices. If you for some reason set the internal sound card as the
default device, I think it's a good thing that we don't automatically
switch to that USB device when it's plugged in again. Therefore I think
it's a good thing that we don't load module-switch-on-connect by
default.

If you in this situation plug in another external device, it would
probably make sense to switch to it, but I don't what the full policy
would look like that would allow this without breaking anything. I
believe it's a good compromise to stick to the current configured
default sink until the user changes it or the device goes away.

> > If you have an external sound card and you have manually selected the
> > internal sound card as the default sink, then module-switch-on-connect
> > seems like a bad idea, because you probably want to keep the internal
> > sound card as the default even after plugging out the external sound
> > card and plugging it back in. If your preference has changed, then it's
> > expected that you have to restate your preference by manually selecting
> > the external sound card.
> >
On Fri, 2019-07-05 at 11:14 +0200, Georg Chini wrote:
> On 05.07.19 09:56, Tanu Kaskinen wrote:
> > On Tue, 2019-07-02 at 10:25 +0200, Georg Chini wrote:
> > > On 02.07.19 06:49, Tanu Kaskinen wrote:
> > > > On Mon, 2019-07-01 at 08:48 +0200, Georg Chini wrote:
> > > > > On 01.07.19 08:03, Georg Chini wrote:
> > > > > > On 01.07.19 07:08, Tanu Kaskinen wrote:
> > > > > > > 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
> > > > > > > > >      +
> > > > > > > > > +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.
> > > > > > I don't understand. Virtual sinks don't have ports. So checking only
> > > > > > sinks that have an active port excludes all sinks that don't have
> > > > > > ports like the null-sink and virtual sinks. Following your definition
> > > > > > above, those sinks are never available.
> > > > You have it reversed: following my definition above, those sinks are
> > > > always available.
> > > Right, sorry. (I seem to be reading things wrong quite often
> > > the last few weeks ... I'll do my best to be more thorough in
> > > the future.)
> > > > > Checking the code, only alsa, bluetooth and raop sinks define ports and
> > > > > the "many places" you are referring to are compare_sinks() and
> > > > > compare_sources() in core.c and a check in module-switch-on-connect,
> > > > > which is used to determine the availability of the default sink.
> > > > At least module-stream-restore checks device availability too (although
> > > > probably not anymore after this patch set, because module-stream-
> > > > restore won't do the routing directly anymore, it will just restore the
> > > > preferred_sink variable, which can be done regardless of the sink
> > > > availability).
> > > > 
> > > > Extending the hardware sink availability to filter sinks probably makes
> > > > sense, but I think that's a topic for a separate patch (or patches).
> > > > 
> > > > You suggested an extra flag for pa_sink_move_streams_to_default_sink(),
> > > > which seems unnecessary to me. The function can in any case figure out
> > > > the availability by itself (possibly using a helper function
> > > > pa_sink_is_available() that can handle filter sinks too).
> > > > 
> > > I think some additional check is still needed at least for
> > > s->unlink_requested
> > > because when a sink is going to be unlinked and the function is called from
> > > pa_sink_unlink() in patch 7, there may be nothing else that indicates
> > > that the
> > > sink is going to be removed. When I proposed an additional argument, I did
> > > not see that pa_sink_unlink() already sets a flag that can be used.
> > > 
> > > So I would suggest that the helper function checks on
> > > - link state
> > I don't expect this to be necessary, although it can't hurt either.
> > 
> > > - port availability
> > > - s->unlink_requested
> > If this is indeed used from pa_sink_unlink() to rescue streams, then
> > checking this seems appropriate (and can't hurt anyway).
> > 
> > > - suspend state (any reason apart from SUSPEND_IDLE)
> > This doesn't seem appropriate, unless we start avoiding suspended sinks
> > in the same way we avoid unavailable sinks. That would mean that we
> > would never use a suspended sink as the default sink. That would
> > probably make sense actually, so I'm just pointing out that we need to
> > be consistent: if we avoid suspended sinks in one situation, we should
> > avoid them in all situations.
> 
> Yes, we should do that. Therefore my suggestion is to add a helper
> function to this patch set. For consistency we could exclude the
> suspend state for the moment, though I would recommend to include
> it. Using the helper function outside this patch set in all places where
> we check availability should then be the task of another patch series.
> By suspended I guess you mean suspended apart from idle, correct?

Yes, that's what I mean.

> > > Using the helper function in other places (and extending it if I forgot
> > > something
> > > in the list above) could then be done in a separate patch. Are you OK
> > > with that?
> > Yes, I think a helper function can be added, and if it's added, it's
> > added in a separate patch.
> > 
> I don't really understand what your suggestion means for the patch set
> we are discussing. Do you mean the helper should be added to that
> set but in a separate patch? Or do you mean it should not be added
> to this set? If it is not added, at least an additional check for
> s->unlink_requested is required in patch 7.

My suggestion is that the helper function is not added in this patch
set. To me it makes more sense to add the function at the same time
when the rest of the code base is updated to use that function.

Ack for adding the s->unlink_request check in this patch.
On 10.07.19 16:03, Tanu Kaskinen wrote:
> On Fri, 2019-07-05 at 10:57 +0200, Georg Chini wrote:
>> On 05.07.19 09:41, Tanu Kaskinen wrote:
>>> On Tue, 2019-07-02 at 09:08 +0200, Georg Chini wrote:
>>>> On 02.07.19 08:43, Tanu Kaskinen wrote:
>>>>> On Mon, 2019-07-01 at 08:03 +0200, Georg Chini wrote:
>>>>>> On 01.07.19 07:08, Tanu Kaskinen wrote:
>>>>>> When a new sink appears, pa_core_update_default_sink() is called. Since
>>>>>> PulseAudio 11.1, bluetooth and USB sinks have higher priority than the
>>>>>> internal sound card, so pa_core_update_default_sink() will switch to
>>>>>> the BT speakers in your example. The main benefit of module-switch-on-
>>>>>> connect was that it moved existing streams to the new sink, but after
>>>>>> this patch set that's handled by the core. Therefore there's much less
>>>>>> need for module-switch-on-connect.
>>>>>>
>>>> This is true, but only relevant if there is no configured default
>>>> sink. If the configured default sink is set, there will be no switch
>>>> to a newly appearing sink because the configured default sink
>>>> is prioritized over all other sinks. In this case you still need
>>>> module-switch-on-connect.
>>> My estimation is that this is very rarely a problem.
>> Mh, my estimation is, that this will be the normal case. At some
>> point the user will set a default sink manually, and from that point
>> on, automatic switching will no longer work. The configured default
>> sink is never unset once it is set.
> Yes, sure, but I tried to explain why this is rarely a problem. (In
> another thread you described a valid use case where this becomes a
> problem, but I don't think that's a "normal case".)
>
>>> If you have manually selected an external sound card as the default
>>> sink and you then plug in another external sound card, then you may or
>>> may not want to automatically switch to the new sound card. If you
>>> fiddle a lot with two external sound cards and you always want to use
>>> the last one plugged in, then module-switch-on-connect is still useful,
>>> but in this case it doesn't really matter that your old default device
>>> choice is forgotten, because PulseAudio will anyway prefer the
>>> remaining external sound card.
>> This topic is not about forgetting the last sink, but about switching
>> to a new sink at all. Once you manually set the default sink, it will
>> never switch automatically without module-switch-on-connect,
>> because the default sink selection process prefers the configured
>> default sink over all other sinks.
> You're assuming that we always want to switch to a newly plugged in
> device. Let's say that you have just one USB device, no other external
> devices. If you for some reason set the internal sound card as the
> default device, I think it's a good thing that we don't automatically
> switch to that USB device when it's plugged in again. Therefore I think
> it's a good thing that we don't load module-switch-on-connect by
> default.
>
> If you in this situation plug in another external device, it would
> probably make sense to switch to it, but I don't what the full policy
> would look like that would allow this without breaking anything. I
> believe it's a good compromise to stick to the current configured
> default sink until the user changes it or the device goes away.
>
You said you will write another mail tomorrow, so I skip most
of your comments, but while reading your mails I realized
what is really behind my dislike of the current switching logic:

I believe it is a bad idea to make switching to new sinks
dependent on the configured default sink. Switching to
new sinks is something a user should be able to turn on
and off separately. Behavior could be something like that:

If switching to new sinks is turned off and a new sink turns up,
we should never switch to it, even if it has higher priority and
even if the configured default sink is not set. The only exception
would be if the configured default sink is set and the sink that
turns up is the configured default sink.

If switching to new sinks is turned on, we always switch to a
new device. The configured default sink is the one we always
return to when the last added device is removed. This means
if two devices have been added, we would already go back to
the configured default when the second device is removed. If
no default is configured, we would use the highest priority sink
to return to.
On Wed, 2019-07-10 at 22:03 +0200, Georg Chini wrote:
> On 10.07.19 16:03, Tanu Kaskinen wrote:
> > On Fri, 2019-07-05 at 10:57 +0200, Georg Chini wrote:
> > > On 05.07.19 09:41, Tanu Kaskinen wrote:
> > > > On Tue, 2019-07-02 at 09:08 +0200, Georg Chini wrote:
> > > > > On 02.07.19 08:43, Tanu Kaskinen wrote:
> > > > > > On Mon, 2019-07-01 at 08:03 +0200, Georg Chini wrote:
> > > > > > > On 01.07.19 07:08, Tanu Kaskinen wrote:
> > > > > > > When a new sink appears, pa_core_update_default_sink() is called. Since
> > > > > > > PulseAudio 11.1, bluetooth and USB sinks have higher priority than the
> > > > > > > internal sound card, so pa_core_update_default_sink() will switch to
> > > > > > > the BT speakers in your example. The main benefit of module-switch-on-
> > > > > > > connect was that it moved existing streams to the new sink, but after
> > > > > > > this patch set that's handled by the core. Therefore there's much less
> > > > > > > need for module-switch-on-connect.
> > > > > > > 
> > > > > This is true, but only relevant if there is no configured default
> > > > > sink. If the configured default sink is set, there will be no switch
> > > > > to a newly appearing sink because the configured default sink
> > > > > is prioritized over all other sinks. In this case you still need
> > > > > module-switch-on-connect.
> > > > My estimation is that this is very rarely a problem.
> > > Mh, my estimation is, that this will be the normal case. At some
> > > point the user will set a default sink manually, and from that point
> > > on, automatic switching will no longer work. The configured default
> > > sink is never unset once it is set.
> > Yes, sure, but I tried to explain why this is rarely a problem. (In
> > another thread you described a valid use case where this becomes a
> > problem, but I don't think that's a "normal case".)
> > 
> > > > If you have manually selected an external sound card as the default
> > > > sink and you then plug in another external sound card, then you may or
> > > > may not want to automatically switch to the new sound card. If you
> > > > fiddle a lot with two external sound cards and you always want to use
> > > > the last one plugged in, then module-switch-on-connect is still useful,
> > > > but in this case it doesn't really matter that your old default device
> > > > choice is forgotten, because PulseAudio will anyway prefer the
> > > > remaining external sound card.
> > > This topic is not about forgetting the last sink, but about switching
> > > to a new sink at all. Once you manually set the default sink, it will
> > > never switch automatically without module-switch-on-connect,
> > > because the default sink selection process prefers the configured
> > > default sink over all other sinks.
> > You're assuming that we always want to switch to a newly plugged in
> > device. Let's say that you have just one USB device, no other external
> > devices. If you for some reason set the internal sound card as the
> > default device, I think it's a good thing that we don't automatically
> > switch to that USB device when it's plugged in again. Therefore I think
> > it's a good thing that we don't load module-switch-on-connect by
> > default.
> > 
> > If you in this situation plug in another external device, it would
> > probably make sense to switch to it, but I don't what the full policy
> > would look like that would allow this without breaking anything. I
> > believe it's a good compromise to stick to the current configured
> > default sink until the user changes it or the device goes away.
> > 
> You said you will write another mail tomorrow, so I skip most
> of your comments, but while reading your mails I realized
> what is really behind my dislike of the current switching logic:
> 
> I believe it is a bad idea to make switching to new sinks
> dependent on the configured default sink. Switching to
> new sinks is something a user should be able to turn on
> and off separately. Behavior could be something like that:
> 
> If switching to new sinks is turned off and a new sink turns up,
> we should never switch to it, even if it has higher priority and
> even if the configured default sink is not set. The only exception
> would be if the configured default sink is set and the sink that
> turns up is the configured default sink.
> 
> If switching to new sinks is turned on, we always switch to a
> new device. The configured default sink is the one we always
> return to when the last added device is removed. This means
> if two devices have been added, we would already go back to
> the configured default when the second device is removed. If
> no default is configured, we would use the highest priority sink
> to return to.

As I said in IRC, I changed my mind about always switching to new
devices regardless of the current configured default sink. I now think
that's a good idea. As you've said, if the user ever configures the
default sink, PulseAudio will from then on never switch to plugged in
devices. I first thought that's OK, because we should respect the
user's explicit preference if the configured default device remains
available, and if it isn't available, then automatic switching to the
plugged in device will work fine.

But if we implement default device history as I've suggested, and the
internal sound card has ever been configured as the default and is
therefore in the history database, it will always be preferred over
never-before-seen plugged in devices, which is bad. In this situation
automatic switching truly is entirely disabled, without the user
requesting such policy change and without a way to re-enable the
policy.

You suggested above that if automatic switching to plugged in devices
is disabled, we shouldn't switch even if there's no configured default
sink. That has the problem that if you plug in a USB device but keep
using the internal sound card, PulseAudio will change the routing after
reboot, because PulseAudio has to choose some sink as the default sink,
and it will choose the USB sink, because it has higher priority.
Changing the default sink on reboot seems bad enough that we should
keep switching to higher priority sinks even if automatic switching to
new devices is otherwise disabled.

I don't know if you have some plans to enable module-switch-on-connect
(or similar policy) by default, but if you do, note the following
complications:

 * We should only switch to newly plugged in hardware, not to new
virtual sinks or sinks that are created as part of a profile change.
I'm not sure about network sinks (tunnels, raop). At least if the
network sinks are created via automatic discovery, they shouldn't be
automatically switched to. Also, we shouldn't switch to HDMI
automatically.

 * We should treat plugging in headphones to an internal sound card the
same way as plugging in USB headphones. That is, we should make the
internal sound card the default sink. This can perhaps be implemented
later, though (or even earlier, by modifying module-switch-on-connect
without loading it by default).
On 12.07.19 13:13, Tanu Kaskinen wrote:
> On Wed, 2019-07-10 at 22:03 +0200, Georg Chini wrote:
>> On 10.07.19 16:03, Tanu Kaskinen wrote:
>>> On Fri, 2019-07-05 at 10:57 +0200, Georg Chini wrote:
>>>> On 05.07.19 09:41, Tanu Kaskinen wrote:
>>>>> On Tue, 2019-07-02 at 09:08 +0200, Georg Chini wrote:
>>>>>> On 02.07.19 08:43, Tanu Kaskinen wrote:
>>>>>>> On Mon, 2019-07-01 at 08:03 +0200, Georg Chini wrote:
>>>>>>>> On 01.07.19 07:08, Tanu Kaskinen wrote:
>>>>>>>> When a new sink appears, pa_core_update_default_sink() is called. Since
>>>>>>>> PulseAudio 11.1, bluetooth and USB sinks have higher priority than the
>>>>>>>> internal sound card, so pa_core_update_default_sink() will switch to
>>>>>>>> the BT speakers in your example. The main benefit of module-switch-on-
>>>>>>>> connect was that it moved existing streams to the new sink, but after
>>>>>>>> this patch set that's handled by the core. Therefore there's much less
>>>>>>>> need for module-switch-on-connect.
>>>>>>>>
>>>>>> This is true, but only relevant if there is no configured default
>>>>>> sink. If the configured default sink is set, there will be no switch
>>>>>> to a newly appearing sink because the configured default sink
>>>>>> is prioritized over all other sinks. In this case you still need
>>>>>> module-switch-on-connect.
>>>>> My estimation is that this is very rarely a problem.
>>>> Mh, my estimation is, that this will be the normal case. At some
>>>> point the user will set a default sink manually, and from that point
>>>> on, automatic switching will no longer work. The configured default
>>>> sink is never unset once it is set.
>>> Yes, sure, but I tried to explain why this is rarely a problem. (In
>>> another thread you described a valid use case where this becomes a
>>> problem, but I don't think that's a "normal case".)
>>>
>>>>> If you have manually selected an external sound card as the default
>>>>> sink and you then plug in another external sound card, then you may or
>>>>> may not want to automatically switch to the new sound card. If you
>>>>> fiddle a lot with two external sound cards and you always want to use
>>>>> the last one plugged in, then module-switch-on-connect is still useful,
>>>>> but in this case it doesn't really matter that your old default device
>>>>> choice is forgotten, because PulseAudio will anyway prefer the
>>>>> remaining external sound card.
>>>> This topic is not about forgetting the last sink, but about switching
>>>> to a new sink at all. Once you manually set the default sink, it will
>>>> never switch automatically without module-switch-on-connect,
>>>> because the default sink selection process prefers the configured
>>>> default sink over all other sinks.
>>> You're assuming that we always want to switch to a newly plugged in
>>> device. Let's say that you have just one USB device, no other external
>>> devices. If you for some reason set the internal sound card as the
>>> default device, I think it's a good thing that we don't automatically
>>> switch to that USB device when it's plugged in again. Therefore I think
>>> it's a good thing that we don't load module-switch-on-connect by
>>> default.
>>>
>>> If you in this situation plug in another external device, it would
>>> probably make sense to switch to it, but I don't what the full policy
>>> would look like that would allow this without breaking anything. I
>>> believe it's a good compromise to stick to the current configured
>>> default sink until the user changes it or the device goes away.
>>>
>> You said you will write another mail tomorrow, so I skip most
>> of your comments, but while reading your mails I realized
>> what is really behind my dislike of the current switching logic:
>>
>> I believe it is a bad idea to make switching to new sinks
>> dependent on the configured default sink. Switching to
>> new sinks is something a user should be able to turn on
>> and off separately. Behavior could be something like that:
>>
>> If switching to new sinks is turned off and a new sink turns up,
>> we should never switch to it, even if it has higher priority and
>> even if the configured default sink is not set. The only exception
>> would be if the configured default sink is set and the sink that
>> turns up is the configured default sink.
>>
>> If switching to new sinks is turned on, we always switch to a
>> new device. The configured default sink is the one we always
>> return to when the last added device is removed. This means
>> if two devices have been added, we would already go back to
>> the configured default when the second device is removed. If
>> no default is configured, we would use the highest priority sink
>> to return to.
> As I said in IRC, I changed my mind about always switching to new
> devices regardless of the current configured default sink. I now think
> that's a good idea. As you've said, if the user ever configures the
> default sink, PulseAudio will from then on never switch to plugged in
> devices. I first thought that's OK, because we should respect the
> user's explicit preference if the configured default device remains
> available, and if it isn't available, then automatic switching to the
> plugged in device will work fine.
>
> But if we implement default device history as I've suggested, and the
> internal sound card has ever been configured as the default and is
> therefore in the history database, it will always be preferred over
> never-before-seen plugged in devices, which is bad. In this situation
> automatic switching truly is entirely disabled, without the user
> requesting such policy change and without a way to re-enable the
> policy.

I still can't see much benefit of a default sink history. Why would
I care if I had some USB device or my friend's Bluetooth speakers
made default at some time? It may never be connected again.
And if you have a history, how would you prioritize it? Simply by
time? If yes, would you then allow for multiple occurrences of
the same sink? A one-shot history for a temporary override is
necessary so that the configured default sink does not get lost,
but everything more is in my opinion luxury (though I would not
mind if somebody implemented that).

Which brings me back to the patch set where this discussion
originated. I guess you still want that module-switch-on-connect
uses pa_core_set_configured_default_sink()? Because that
was my original complaint.

>
> You suggested above that if automatic switching to plugged in devices
> is disabled, we shouldn't switch even if there's no configured default
> sink. That has the problem that if you plug in a USB device but keep
> using the internal sound card, PulseAudio will change the routing after
> reboot, because PulseAudio has to choose some sink as the default sink,
> and it will choose the USB sink, because it has higher priority.

That's why I said the exception is, if you have a configured
default sink, it will switch to it. The configured default sink
is exactly for those situations where you want a specific
sink even if higher priority sinks may be available.

If the user chooses not to configure the default sink and not
to use switching, the user will get what was configured, which
is not changing the sink when something is plugged but using
the highest priority sink as default after a reboot.

So I can't see what is bad about your case above.

Apart from that, there is module-default-device-restore, which
could save the current default sink together with the configured
default sink and restore the previous default sink, if the configured
default sink is not set or not available after a reboot.

> Changing the default sink on reboot seems bad enough that we should
> keep switching to higher priority sinks even if automatic switching to
> new devices is otherwise disabled.

 From my perspective it either it can be disabled or it can't. I do not
understand what you mean with "automatic switching is otherwise
disabled" in that context. Only switching to lower priority sinks is
disabled? That does not make much sense to me. I think when
switching is disabled it should mean we only switch to the configured
default sink.

If we now always switch to new devices (how do we do that without
overwriting the configured default sink each time?), I think we need
a way to disable it. As you said yourself, there are many setups where
switching is not wanted. Let me take a laptop as example. You will
probably want that it switches from the internal card to the sound
card of the docking station when you connect it. But you don't
want that it switches to some (higher priority) USB headset that
you plug in only to make a phone call.
So you could set your configured default sink to the docking
station and otherwise disable switching. Together with the
modification of module-default-device-restore you would have
a reliable setup where the USB head set will only become the
default sink if you remove the machine from the docking station
while the head set is connected (regardless if the system is on
or off at the time of removal).

The use case with switching would be the one I described in
another mail, where you permanently have an USB head set
connected but will not want it to become default.

One of my major concerns with coupling switching and the
configured default sink is that users will not understand the
consequences of clicking the little green tick-mark in pavucontrol.
Instead, they will just wonder why switching sometimes works and
sometimes not. From a user perspective I do not see any
obvious connection between setting the sink I normally want
to use and the ability to switch to another device automatically.

Probably all routing schemes have their benefits and drawbacks,
but what I outlined above is at least easy to understand and works
in many different situations.

>
> I don't know if you have some plans to enable module-switch-on-connect
> (or similar policy) by default, but if you do, note the following
> complications:
>
>   * We should only switch to newly plugged in hardware, not to new
> virtual sinks or sinks that are created as part of a profile change.
> I'm not sure about network sinks (tunnels, raop). At least if the
> network sinks are created via automatic discovery, they shouldn't be
> automatically switched to. Also, we shouldn't switch to HDMI
> automatically.
>
>   * We should treat plugging in headphones to an internal sound card the
> same way as plugging in USB headphones. That is, we should make the
> internal sound card the default sink. This can perhaps be implemented
> later, though (or even earlier, by modifying module-switch-on-connect
> without loading it by default).
>
I do not have any plans concerning module-switch-on-connect.
In my opinion it would be better to move the functionality to the
core and have an option in daemon.conf to enable/disable
switching. Having half of the logic in the core and the other half
in module-switch-on-connect seems wrong. But I have enough
patches pending, so I think it does not make sense to add even
more infrastructure changes to the list, especially as it seems
we cannot really agree on the best approach. Actually I would like
to hear more opinions on that matter because the topic allows
multiple solutions and we should implement what most people
find logical and useful.
On Fri, 2019-07-12 at 20:52 +0200, Georg Chini wrote:
> On 12.07.19 13:13, Tanu Kaskinen wrote:
> > On Wed, 2019-07-10 at 22:03 +0200, Georg Chini wrote:
> > > On 10.07.19 16:03, Tanu Kaskinen wrote:
> > > > On Fri, 2019-07-05 at 10:57 +0200, Georg Chini wrote:
> > > > > On 05.07.19 09:41, Tanu Kaskinen wrote:
> > > > > > On Tue, 2019-07-02 at 09:08 +0200, Georg Chini wrote:
> > > > > > > On 02.07.19 08:43, Tanu Kaskinen wrote:
> > > > > > > > On Mon, 2019-07-01 at 08:03 +0200, Georg Chini wrote:
> > > > > > > > > On 01.07.19 07:08, Tanu Kaskinen wrote:
> > > > > > > > > When a new sink appears, pa_core_update_default_sink() is called. Since
> > > > > > > > > PulseAudio 11.1, bluetooth and USB sinks have higher priority than the
> > > > > > > > > internal sound card, so pa_core_update_default_sink() will switch to
> > > > > > > > > the BT speakers in your example. The main benefit of module-switch-on-
> > > > > > > > > connect was that it moved existing streams to the new sink, but after
> > > > > > > > > this patch set that's handled by the core. Therefore there's much less
> > > > > > > > > need for module-switch-on-connect.
> > > > > > > > > 
> > > > > > > This is true, but only relevant if there is no configured default
> > > > > > > sink. If the configured default sink is set, there will be no switch
> > > > > > > to a newly appearing sink because the configured default sink
> > > > > > > is prioritized over all other sinks. In this case you still need
> > > > > > > module-switch-on-connect.
> > > > > > My estimation is that this is very rarely a problem.
> > > > > Mh, my estimation is, that this will be the normal case. At some
> > > > > point the user will set a default sink manually, and from that point
> > > > > on, automatic switching will no longer work. The configured default
> > > > > sink is never unset once it is set.
> > > > Yes, sure, but I tried to explain why this is rarely a problem. (In
> > > > another thread you described a valid use case where this becomes a
> > > > problem, but I don't think that's a "normal case".)
> > > > 
> > > > > > If you have manually selected an external sound card as the default
> > > > > > sink and you then plug in another external sound card, then you may or
> > > > > > may not want to automatically switch to the new sound card. If you
> > > > > > fiddle a lot with two external sound cards and you always want to use
> > > > > > the last one plugged in, then module-switch-on-connect is still useful,
> > > > > > but in this case it doesn't really matter that your old default device
> > > > > > choice is forgotten, because PulseAudio will anyway prefer the
> > > > > > remaining external sound card.
> > > > > This topic is not about forgetting the last sink, but about switching
> > > > > to a new sink at all. Once you manually set the default sink, it will
> > > > > never switch automatically without module-switch-on-connect,
> > > > > because the default sink selection process prefers the configured
> > > > > default sink over all other sinks.
> > > > You're assuming that we always want to switch to a newly plugged in
> > > > device. Let's say that you have just one USB device, no other external
> > > > devices. If you for some reason set the internal sound card as the
> > > > default device, I think it's a good thing that we don't automatically
> > > > switch to that USB device when it's plugged in again. Therefore I think
> > > > it's a good thing that we don't load module-switch-on-connect by
> > > > default.
> > > > 
> > > > If you in this situation plug in another external device, it would
> > > > probably make sense to switch to it, but I don't what the full policy
> > > > would look like that would allow this without breaking anything. I
> > > > believe it's a good compromise to stick to the current configured
> > > > default sink until the user changes it or the device goes away.
> > > > 
> > > You said you will write another mail tomorrow, so I skip most
> > > of your comments, but while reading your mails I realized
> > > what is really behind my dislike of the current switching logic:
> > > 
> > > I believe it is a bad idea to make switching to new sinks
> > > dependent on the configured default sink. Switching to
> > > new sinks is something a user should be able to turn on
> > > and off separately. Behavior could be something like that:
> > > 
> > > If switching to new sinks is turned off and a new sink turns up,
> > > we should never switch to it, even if it has higher priority and
> > > even if the configured default sink is not set. The only exception
> > > would be if the configured default sink is set and the sink that
> > > turns up is the configured default sink.
> > > 
> > > If switching to new sinks is turned on, we always switch to a
> > > new device. The configured default sink is the one we always
> > > return to when the last added device is removed. This means
> > > if two devices have been added, we would already go back to
> > > the configured default when the second device is removed. If
> > > no default is configured, we would use the highest priority sink
> > > to return to.
> > As I said in IRC, I changed my mind about always switching to new
> > devices regardless of the current configured default sink. I now think
> > that's a good idea. As you've said, if the user ever configures the
> > default sink, PulseAudio will from then on never switch to plugged in
> > devices. I first thought that's OK, because we should respect the
> > user's explicit preference if the configured default device remains
> > available, and if it isn't available, then automatic switching to the
> > plugged in device will work fine.
> > 
> > But if we implement default device history as I've suggested, and the
> > internal sound card has ever been configured as the default and is
> > therefore in the history database, it will always be preferred over
> > never-before-seen plugged in devices, which is bad. In this situation
> > automatic switching truly is entirely disabled, without the user
> > requesting such policy change and without a way to re-enable the
> > policy.
> 
> I still can't see much benefit of a default sink history. Why would
> I care if I had some USB device or my friend's Bluetooth speakers
> made default at some time? It may never be connected again.
> And if you have a history, how would you prioritize it? Simply by
> time? If yes, would you then allow for multiple occurrences of
> the same sink?

I wouldn't allow for multiple occurrences of the same sink. It would be
a priority list. Every time a sink is configured as the default, it
would become the highest-priority sink.

> A one-shot history for a temporary override is
> necessary so that the configured default sink does not get lost,
> but everything more is in my opinion luxury (though I would not
> mind if somebody implemented that).

Yes, maintaining the history isn't very important if module-switch-on-
connect uses a temporary override instead of setting the configured
default sink (which is now fine by me).

> Which brings me back to the patch set where this discussion
> originated. I guess you still want that module-switch-on-connect
> uses pa_core_set_configured_default_sink()? Because that
> was my original complaint.

Changing how module-switch-on-connect seems off topic for this patch
set, so yes, I would prefer to keep using
pa_core_set_configured_default_sink() in this patch set. However,
introducing a temporary override mechanism in a separate patch set is
fine by me.

> > You suggested above that if automatic switching to plugged in devices
> > is disabled, we shouldn't switch even if there's no configured default
> > sink. That has the problem that if you plug in a USB device but keep
> > using the internal sound card, PulseAudio will change the routing after
> > reboot, because PulseAudio has to choose some sink as the default sink,
> > and it will choose the USB sink, because it has higher priority.
> 
> That's why I said the exception is, if you have a configured
> default sink, it will switch to it. The configured default sink
> is exactly for those situations where you want a specific
> sink even if higher priority sinks may be available.
> 
> If the user chooses not to configure the default sink and not
> to use switching, the user will get what was configured, which
> is not changing the sink when something is plugged but using
> the highest priority sink as default after a reboot.
> 
> So I can't see what is bad about your case above.

You don't think it's bad that rebooting changes the routing even when
there's no change in what hardware is available? In my opinion that's
very bad behaviour.

> Apart from that, there is module-default-device-restore, which
> could save the current default sink together with the configured
> default sink and restore the previous default sink, if the configured
> default sink is not set or not available after a reboot.

That sounds workable. I'm starting to like your idea of not switcing to
higher priority sinks when they become available if automatic switching
to new devices is disabled.

> > Changing the default sink on reboot seems bad enough that we should
> > keep switching to higher priority sinks even if automatic switching to
> > new devices is otherwise disabled.
> 
>  From my perspective it either it can be disabled or it can't. I do not
> understand what you mean with "automatic switching is otherwise
> disabled" in that context. Only switching to lower priority sinks is
> disabled?

Yes, that's what I meant.

> That does not make much sense to me. I think when
> switching is disabled it should mean we only switch to the configured
> default sink.
> 
> If we now always switch to new devices (how do we do that without
> overwriting the configured default sink each time?), I think we need
> a way to disable it. As you said yourself, there are many setups where
> switching is not wanted. Let me take a laptop as example. You will
> probably want that it switches from the internal card to the sound
> card of the docking station when you connect it. But you don't
> want that it switches to some (higher priority) USB headset that
> you plug in only to make a phone call.
> So you could set your configured default sink to the docking
> station and otherwise disable switching. Together with the
> modification of module-default-device-restore you would have
> a reliable setup where the USB head set will only become the
> default sink if you remove the machine from the docking station
> while the head set is connected (regardless if the system is on
> or off at the time of removal).
> 
> The use case with switching would be the one I described in
> another mail, where you permanently have an USB head set
> connected but will not want it to become default.
> 
> One of my major concerns with coupling switching and the
> configured default sink is that users will not understand the
> consequences of clicking the little green tick-mark in pavucontrol.
> Instead, they will just wonder why switching sometimes works and
> sometimes not. From a user perspective I do not see any
> obvious connection between setting the sink I normally want
> to use and the ability to switch to another device automatically.
> 
> Probably all routing schemes have their benefits and drawbacks,
> but what I outlined above is at least easy to understand and works
> in many different situations.
> 
> > I don't know if you have some plans to enable module-switch-on-connect
> > (or similar policy) by default, but if you do, note the following
> > complications:
> > 
> >   * We should only switch to newly plugged in hardware, not to new
> > virtual sinks or sinks that are created as part of a profile change.
> > I'm not sure about network sinks (tunnels, raop). At least if the
> > network sinks are created via automatic discovery, they shouldn't be
> > automatically switched to. Also, we shouldn't switch to HDMI
> > automatically.
> > 
> >   * We should treat plugging in headphones to an internal sound card the
> > same way as plugging in USB headphones. That is, we should make the
> > internal sound card the default sink. This can perhaps be implemented
> > later, though (or even earlier, by modifying module-switch-on-connect
> > without loading it by default).
> > 
> I do not have any plans concerning module-switch-on-connect.
> In my opinion it would be better to move the functionality to the
> core and have an option in daemon.conf to enable/disable
> switching.

I would be fine with that. (In general I don't see much value in having
modules that are built and installed unconditionally. Modules are
mainly useful for things that have external dependencies that we don't
want the core to depend on. But so far we've been very strict in what
can go into the core, and I don't know if everybody would like changing
the tradition.)

> Having half of the logic in the core and the other half
> in module-switch-on-connect seems wrong.

Yes. On the other hand, forcing policy in the core isn't nice if
someone wants to implement a custom policy system (people have been
known to do such thing). It could make sense to have a comprehensive
default policy implementation in the core, but allow it to be switched
out.

> But I have enough
> patches pending, so I think it does not make sense to add even
> more infrastructure changes to the list, especially as it seems
> we cannot really agree on the best approach.

Actually I pretty much agree with your proposal now. But you're right
that writing more and more patches while the old ones are still pending
review isn't necessarily a good idea. Spending time on making the
review queue shorter rather than longer would be great ;)
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@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.
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@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.
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@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?
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@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.