[1/4] change bool save_sink to char *preferred_sink

Submitted by Hui Wang on Nov. 5, 2018, 1:47 a.m.

Details

Message ID 20181105014716.5613-2-hui.wang@canonical.com
State New
Series "The tentative fix of the issue #185"
Headers show

Commit Message

Hui Wang Nov. 5, 2018, 1:47 a.m.
And don't move the stream in the module-stream-restore anymore,
And the preferred_sink is only set when user is calling the
move_to() and the module-stream-restore maintains the saving and
deleting of preferred_sink.

If the target of move_to() is default_sink, preferred_sink will be
cleared and the entry->device will be cleared too from database.

Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
 src/modules/module-device-manager.c    |  2 +-
 src/modules/module-intended-roles.c    |  2 +-
 src/modules/module-stream-restore.c    | 47 +++++++++++++++++---------
 src/modules/module-switch-on-connect.c |  2 +-
 src/pulsecore/sink-input.c             | 25 +++++++++++---
 src/pulsecore/sink-input.h             |  7 ++--
 6 files changed, 59 insertions(+), 26 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/modules/module-device-manager.c b/src/modules/module-device-manager.c
index 15fdaaaa2..36e71584e 100644
--- a/src/modules/module-device-manager.c
+++ b/src/modules/module-device-manager.c
@@ -657,7 +657,7 @@  static void route_sink_input(struct userdata *u, pa_sink_input *si) {
     pa_assert(u->do_routing);
 
     /* Don't override user or application routing requests. */
-    if (si->save_sink || si->sink_requested_by_application)
+    if (si->preferred_sink != NULL || si->sink_requested_by_application)
         return;
 
     /* Skip this if it is already in the process of being moved anyway */
diff --git a/src/modules/module-intended-roles.c b/src/modules/module-intended-roles.c
index adee51c20..98e4c241f 100644
--- a/src/modules/module-intended-roles.c
+++ b/src/modules/module-intended-roles.c
@@ -175,7 +175,7 @@  static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, struct
         if (si->sink == sink)
             continue;
 
-        if (si->save_sink)
+        if (si->preferred_sink != NULL)
             continue;
 
         /* Skip this if it is already in the process of being moved
diff --git a/src/modules/module-stream-restore.c b/src/modules/module-stream-restore.c
index 228e9e447..276957c25 100644
--- a/src/modules/module-stream-restore.c
+++ b/src/modules/module-stream-restore.c
@@ -1311,19 +1311,29 @@  static void subscribe_callback(pa_core *c, pa_subscription_event_type_t t, uint3
             mute_updated = !created_new_entry && (!old->muted_valid || entry->muted != old->muted);
         }
 
-        if (sink_input->save_sink) {
+        if (sink_input->preferred_sink != NULL || !created_new_entry) {
             pa_xfree(entry->device);
-            entry->device = pa_xstrdup(sink_input->sink->name);
-            entry->device_valid = true;
 
-            device_updated = !created_new_entry && (!old->device_valid || !pa_streq(entry->device, old->device));
-            if (sink_input->sink->card) {
+	    if (sink_input->preferred_sink != NULL) {
+		entry->device = pa_xstrdup(sink_input->preferred_sink);
+		entry->device_valid = true;
+	    } else {
+		entry->device = NULL;
+		entry->device_valid = false;
+	    }
+
+            device_updated = !created_new_entry && (!old->device_valid || !entry->device_valid || !pa_streq(entry->device, old->device));
+
+	    if (entry->device_valid == false) {
+                pa_xfree(entry->card);
+                entry->card = NULL;
+                entry->card_valid = false;
+	    } else if (sink_input->sink->card) {
                 pa_xfree(entry->card);
                 entry->card = pa_xstrdup(sink_input->sink->card->name);
                 entry->card_valid = true;
             }
         }
-
     } else {
         pa_source_output *source_output;
 
@@ -1641,7 +1651,7 @@  static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, struct
         if (si->sink == sink)
             continue;
 
-        if (si->save_sink)
+        if (si->preferred_sink != NULL)
             continue;
 
         /* Skip this if it is already in the process of being moved
@@ -1665,7 +1675,7 @@  static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, struct
 
         if ((e = entry_read(u, name))) {
             if (e->device_valid && pa_streq(e->device, sink->name))
-                pa_sink_input_move_to(si, sink, true);
+		si->preferred_sink = pa_xstrdup(sink->name);
 
             entry_free(e);
         }
@@ -1764,8 +1774,10 @@  static pa_hook_result_t sink_unlink_hook_callback(pa_core *c, pa_sink *sink, str
 
                 if ((d = pa_namereg_get(c, e->device, PA_NAMEREG_SINK)) &&
                     d != sink &&
-                    PA_SINK_IS_LINKED(d->state))
-                    pa_sink_input_move_to(si, d, true);
+                    PA_SINK_IS_LINKED(d->state)) {
+		    pa_xfree(si->preferred_sink);
+		    si->preferred_sink = pa_xstrdup(d->name);
+		}
             }
 
             entry_free(e);
@@ -1942,12 +1954,13 @@  static void entry_apply(struct userdata *u, const char *name, struct entry *e) {
 
         if (u->restore_device) {
             if (!e->device_valid) {
-                if (si->save_sink) {
+                if (si->preferred_sink != NULL) {
                     pa_log_info("Ensuring device is not saved for stream %s.", name);
                     /* If the device is not valid we should make sure the
                        save flag is cleared as the user may have specifically
                        removed the sink element from the rule. */
-                    si->save_sink = false;
+		    pa_xfree(si->preferred_sink);
+                    si->preferred_sink = NULL;
                     /* This is cheating a bit. The sink input itself has not changed
                        but the rules governing its routing have, so we fire this event
                        such that other routing modules (e.g. module-device-manager)
@@ -1956,7 +1969,8 @@  static void entry_apply(struct userdata *u, const char *name, struct entry *e) {
                 }
             } else if ((s = pa_namereg_get(u->core, e->device, PA_NAMEREG_SINK))) {
                 pa_log_info("Restoring device for stream %s.", name);
-                pa_sink_input_move_to(si, s, true);
+		pa_xfree(si->preferred_sink);
+		si->preferred_sink = pa_xstrdup(s->name);
             }
         }
     }
@@ -2176,9 +2190,10 @@  static int extension_cb(pa_native_protocol *p, pa_module *m, pa_native_connectio
 
                 entry->muted = muted;
                 entry->muted_valid = true;
-
-                entry->device = pa_xstrdup(device);
-                entry->device_valid = device && !!entry->device[0];
+		if (device && !pa_streq(device, m->core->default_sink->name) && !pa_streq(device, m->core->default_source->name)) {
+		    entry->device = pa_xstrdup(device);
+		    entry->device_valid = device && !!entry->device[0];
+		}
 
                 if (entry->device_valid && !pa_namereg_is_valid_name(entry->device)) {
                     entry_free(entry);
diff --git a/src/modules/module-switch-on-connect.c b/src/modules/module-switch-on-connect.c
index ebdd6aad0..faa0f289f 100644
--- a/src/modules/module-switch-on-connect.c
+++ b/src/modules/module-switch-on-connect.c
@@ -112,7 +112,7 @@  static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, void*
     }
 
     PA_IDXSET_FOREACH(i, old_default_sink->inputs, idx) {
-        if (i->save_sink || !PA_SINK_INPUT_IS_LINKED(i->state))
+        if (i->preferred_sink != NULL || !PA_SINK_INPUT_IS_LINKED(i->state))
             continue;
 
         if (pa_sink_input_move_to(i, sink, false) < 0)
diff --git a/src/pulsecore/sink-input.c b/src/pulsecore/sink-input.c
index 312ec4a97..1031051a7 100644
--- a/src/pulsecore/sink-input.c
+++ b/src/pulsecore/sink-input.c
@@ -190,7 +190,8 @@  bool pa_sink_input_new_data_set_sink(pa_sink_input_new_data *data, pa_sink *s, b
     if (!data->req_formats) {
         /* We're not working with the extended API */
         data->sink = s;
-        data->save_sink = save;
+	if (save)
+            data->preferred_sink = pa_xstrdup(s->name);
         data->sink_requested_by_application = requested_by_application;
     } else {
         /* Extended API: let's see if this sink supports the formats the client can provide */
@@ -199,7 +200,8 @@  bool pa_sink_input_new_data_set_sink(pa_sink_input_new_data *data, pa_sink *s, b
         if (formats && !pa_idxset_isempty(formats)) {
             /* Sink supports at least one of the requested formats */
             data->sink = s;
-            data->save_sink = save;
+	    if (save)
+		data->preferred_sink = pa_xstrdup(s->name);
             data->sink_requested_by_application = requested_by_application;
             if (data->nego_formats)
                 pa_idxset_free(data->nego_formats, (pa_free_cb_t) pa_format_info_free);
@@ -226,7 +228,7 @@  bool pa_sink_input_new_data_set_formats(pa_sink_input_new_data *data, pa_idxset
 
     if (data->sink) {
         /* Trigger format negotiation */
-        return pa_sink_input_new_data_set_sink(data, data->sink, data->save_sink, data->sink_requested_by_application);
+        return pa_sink_input_new_data_set_sink(data, data->sink, false, data->sink_requested_by_application);
     }
 
     return true;
@@ -519,7 +521,7 @@  int pa_sink_input_new(
     pa_cvolume_reset(&i->real_ratio, i->sample_spec.channels);
     i->volume_writable = data->volume_writable;
     i->save_volume = data->save_volume;
-    i->save_sink = data->save_sink;
+    i->preferred_sink = pa_xstrdup(data->preferred_sink);
     i->save_muted = data->save_muted;
 
     i->muted = data->muted;
@@ -777,6 +779,9 @@  static void sink_input_free(pa_object *o) {
     if (i->volume_factor_sink_items)
         pa_hashmap_free(i->volume_factor_sink_items);
 
+    if (i->preferred_sink)
+	pa_xfree(i->preferred_sink);
+
     pa_xfree(i->driver);
     pa_xfree(i);
 }
@@ -1914,7 +1919,17 @@  int pa_sink_input_finish_move(pa_sink_input *i, pa_sink *dest, bool save) {
         i->moving(i, dest);
 
     i->sink = dest;
-    i->save_sink = save;
+
+    /* save == true, means user is calling the move_to() and want to
+       save the preferred_sinke if the sink is not default_sink */
+    if (save) {
+	pa_xfree(i->preferred_sink);
+	if (dest == dest->core->default_sink)
+	    i->preferred_sink = NULL;
+	else
+	    i->preferred_sink = pa_xstrdup(dest->name);
+    }
+
     pa_idxset_put(dest->inputs, pa_sink_input_ref(i), NULL);
 
     PA_HASHMAP_FOREACH(v, i->volume_factor_sink_items, state)
diff --git a/src/pulsecore/sink-input.h b/src/pulsecore/sink-input.h
index 9e90291ca..537cec9a1 100644
--- a/src/pulsecore/sink-input.h
+++ b/src/pulsecore/sink-input.h
@@ -123,7 +123,8 @@  struct pa_sink_input {
      * set is worth remembering, i.e. was explicitly chosen by the
      * user and not automatically. module-stream-restore looks for
      * this.*/
-    bool save_sink:1, save_volume:1, save_muted:1;
+    char *preferred_sink;
+    bool save_volume:1, save_muted:1;
 
     pa_resample_method_t requested_resample_method, actual_resample_method;
 
@@ -300,6 +301,8 @@  typedef struct pa_sink_input_new_data {
     pa_idxset *req_formats;
     pa_idxset *nego_formats;
 
+    char *preferred_sink;
+
     pa_cvolume volume;
     bool muted:1;
     pa_hashmap *volume_factor_items, *volume_factor_sink_items;
@@ -314,7 +317,7 @@  typedef struct pa_sink_input_new_data {
 
     bool volume_writable:1;
 
-    bool save_sink:1, save_volume:1, save_muted:1;
+    bool save_volume:1, save_muted:1;
 } pa_sink_input_new_data;
 
 pa_sink_input_new_data* pa_sink_input_new_data_init(pa_sink_input_new_data *data);

Comments

Tanu Kaskinen Dec. 12, 2018, 1:39 p.m.
Thanks for working on this! Sorry for slow review, I hope I'll be much
quicker to comment on subsequent iterations.

On Mon, 2018-11-05 at 09:47 +0800, Hui Wang wrote:
> And don't move the stream in the module-stream-restore anymore,
> And the preferred_sink is only set when user is calling the
> move_to() and the module-stream-restore maintains the saving and
> deleting of preferred_sink.
> 
> If the target of move_to() is default_sink, preferred_sink will be
> cleared and the entry->device will be cleared too from database.

Can you split this so that the first patch only changes save_sink to
preferred_sink, without any changes in behaviour? That is, put the
"don't move the stream in the module-stream-restore" and "if the target
of move_to() is default_sink" logic into separate patches.

Also replace tabs with spaces.

> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---
>  src/modules/module-device-manager.c    |  2 +-
>  src/modules/module-intended-roles.c    |  2 +-
>  src/modules/module-stream-restore.c    | 47 +++++++++++++++++---------
>  src/modules/module-switch-on-connect.c |  2 +-
>  src/pulsecore/sink-input.c             | 25 +++++++++++---
>  src/pulsecore/sink-input.h             |  7 ++--
>  6 files changed, 59 insertions(+), 26 deletions(-)
> 
> diff --git a/src/modules/module-device-manager.c b/src/modules/module-device-manager.c
> index 15fdaaaa2..36e71584e 100644
> --- a/src/modules/module-device-manager.c
> +++ b/src/modules/module-device-manager.c
> @@ -657,7 +657,7 @@ static void route_sink_input(struct userdata *u, pa_sink_input *si) {
>      pa_assert(u->do_routing);
>  
>      /* Don't override user or application routing requests. */
> -    if (si->save_sink || si->sink_requested_by_application)
> +    if (si->preferred_sink != NULL || si->sink_requested_by_application)

Just checking whether si->preferred_sink is set isn't enough - you need
to also check that the input is currently routed to the preferred sink.
Maybe we should have some helper, since this is a common thing to check
- perhaps pa_sink_input_is_routed_to_preferred_sink(). That's quite a
long name, though, so it's not that much better than just using
pa_safe_streq(si->sink->name, si->preferred_sink). I think the
pa_safe_streq() approach is good enough.

>          return;
>  
>      /* Skip this if it is already in the process of being moved anyway */
> diff --git a/src/modules/module-intended-roles.c b/src/modules/module-intended-roles.c
> index adee51c20..98e4c241f 100644
> --- a/src/modules/module-intended-roles.c
> +++ b/src/modules/module-intended-roles.c
> @@ -175,7 +175,7 @@ static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, struct
>          if (si->sink == sink)
>              continue;
>  
> -        if (si->save_sink)
> +        if (si->preferred_sink != NULL)

pa_safe_streq(si->sink->name, si->preferred_sink)

>              continue;
>  
>          /* Skip this if it is already in the process of being moved
> diff --git a/src/modules/module-stream-restore.c b/src/modules/module-stream-restore.c
> index 228e9e447..276957c25 100644
> --- a/src/modules/module-stream-restore.c
> +++ b/src/modules/module-stream-restore.c
> @@ -1311,19 +1311,29 @@ static void subscribe_callback(pa_core *c, pa_subscription_event_type_t t, uint3
>              mute_updated = !created_new_entry && (!old->muted_valid || entry->muted != old->muted);
>          }
>  
> -        if (sink_input->save_sink) {
> +        if (sink_input->preferred_sink != NULL || !created_new_entry) {
>              pa_xfree(entry->device);
> -            entry->device = pa_xstrdup(sink_input->sink->name);
> -            entry->device_valid = true;
>  
> -            device_updated = !created_new_entry && (!old->device_valid || !pa_streq(entry->device, old->device));
> -            if (sink_input->sink->card) {
> +	    if (sink_input->preferred_sink != NULL) {
> +		entry->device = pa_xstrdup(sink_input->preferred_sink);
> +		entry->device_valid = true;
> +	    } else {
> +		entry->device = NULL;
> +		entry->device_valid = false;
> +	    }
> +
> +            device_updated = !created_new_entry && (!old->device_valid || !entry->device_valid || !pa_streq(entry->device, old->device));

This sets device_updated to true if both old->device_valid and entry-
>device_valid are false. device_updated should be set to false in that
case.

> +
> +	    if (entry->device_valid == false) {
> +                pa_xfree(entry->card);
> +                entry->card = NULL;
> +                entry->card_valid = false;
> +	    } else if (sink_input->sink->card) {
>                  pa_xfree(entry->card);
>                  entry->card = pa_xstrdup(sink_input->sink->card->name);
>                  entry->card_valid = true;
>              }
>          }
> -
>      } else {
>          pa_source_output *source_output;
>  
> @@ -1641,7 +1651,7 @@ static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, struct
>          if (si->sink == sink)
>              continue;
>  
> -        if (si->save_sink)
> +        if (si->preferred_sink != NULL)
>              continue;

The purpose of this hook callback is to move streams whose preferred
sink is the one that just got created. Such streams have si-
>preferred_sink set to a non-NULL value, so this if condition is not
good.

With the old code si->save_sink indicated that the stream was already
routed to the preferred sink. To replicate that you would have to check
whether si->sink->name matches si->preferred_sink, but I don't think
that's very useful. This condition can be simply dropped. If the stream
is already correctly routed, then pa_sink_input_move_to() will do
nothing.

>  
>          /* Skip this if it is already in the process of being moved
> @@ -1665,7 +1675,7 @@ static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, struct
>  
>          if ((e = entry_read(u, name))) {
>              if (e->device_valid && pa_streq(e->device, sink->name))
> -                pa_sink_input_move_to(si, sink, true);
> +		si->preferred_sink = pa_xstrdup(sink->name);

I don't understand what this change is trying to achieve. The database
should always be in sync with with the preferred_sink variable of all
sink inputs, so it's not possible that device_valid is true while si-
>preferred_sink is NULL. If the goal is to avoid moving streams in
module-stream-restore, this move has to be done by the core when a new
sink is created, but you didn't add such code (and when that code is
added, it should be in a separate patch). At this point I don't think
any changes are needed here.

>  
>              entry_free(e);
>          }
> @@ -1764,8 +1774,10 @@ static pa_hook_result_t sink_unlink_hook_callback(pa_core *c, pa_sink *sink, str
>  
>                  if ((d = pa_namereg_get(c, e->device, PA_NAMEREG_SINK)) &&
>                      d != sink &&
> -                    PA_SINK_IS_LINKED(d->state))
> -                    pa_sink_input_move_to(si, d, true);
> +                    PA_SINK_IS_LINKED(d->state)) {
> +		    pa_xfree(si->preferred_sink);
> +		    si->preferred_sink = pa_xstrdup(d->name);
> +		}

Same as above: the database is always in sync with si->preferred_sink,
so si->preferred_sink doesn't need updating. Also this patch should
keep the pa_sink_input_move_to() call. A later patch should add code to
the core so that streams are rescued to their preferred sink when their
current sink goes away. At that point his whole unlink callback can be
removed. For now this code needs no changing.

>              }
>  
>              entry_free(e);
> @@ -1942,12 +1954,13 @@ static void entry_apply(struct userdata *u, const char *name, struct entry *e) {
>  
>          if (u->restore_device) {
>              if (!e->device_valid) {
> -                if (si->save_sink) {
> +                if (si->preferred_sink != NULL) {
>                      pa_log_info("Ensuring device is not saved for stream %s.", name);
>                      /* If the device is not valid we should make sure the
>                         save flag is cleared as the user may have specifically
>                         removed the sink element from the rule. */
> -                    si->save_sink = false;
> +		    pa_xfree(si->preferred_sink);
> +                    si->preferred_sink = NULL;

I think we should add a pa_sink_input_set_preferred_sink() function.
Modifying the pa_sink_input struct outside sink-input.c isn't nice from
encapsulation point of view. pa_sink_input_set_preferred_sink() can
also call pa_subscription_post(), so the pa_subscription_post() call
below can be removed.

pa_sink_input_set_preferred_sink() should also move the stream to the
current default sink, but it's probably best to do that in a later
patch so that the first patch doesn't cause any changes in behaviour.

>                      /* This is cheating a bit. The sink input itself has not changed
>                         but the rules governing its routing have, so we fire this event
>                         such that other routing modules (e.g. module-device-manager)
> @@ -1956,7 +1969,8 @@ static void entry_apply(struct userdata *u, const char *name, struct entry *e) {
>                  }
>              } else if ((s = pa_namereg_get(u->core, e->device, PA_NAMEREG_SINK))) {
>                  pa_log_info("Restoring device for stream %s.", name);
> -                pa_sink_input_move_to(si, s, true);
> +		pa_xfree(si->preferred_sink);
> +		si->preferred_sink = pa_xstrdup(s->name);

This should also be replaced with a call to
pa_sink_input_set_preferred_sink(), and
pa_sink_input_set_preferred_sink() should move the stream to the new
preferred sink. (In this case the move should be done already in the
first patch, because that matches the old behaviour.)

>              }
>          }
>      }
> @@ -2176,9 +2190,10 @@ static int extension_cb(pa_native_protocol *p, pa_module *m, pa_native_connectio
>  
>                  entry->muted = muted;
>                  entry->muted_valid = true;
> -
> -                entry->device = pa_xstrdup(device);
> -                entry->device_valid = device && !!entry->device[0];
> +		if (device && !pa_streq(device, m->core->default_sink->name) && !pa_streq(device, m->core->default_source->name)) {
> +		    entry->device = pa_xstrdup(device);
> +		    entry->device_valid = device && !!entry->device[0];
> +		}

What's the goal here? The client tries to change an entry in the
stream-restore database, why should that change be ignored if the
current default sink happens to be the same as the new device? Maybe
you intended to set entry->device to NULL in this case. But I don't
think that's necessary either - if the client wants to unset the
device, it can just give NULL as the device name. I don't think you
need to change anything here.

By the way, m->core->default_sink can be NULL, so that would have to be
checked if this code was kept.

>  
>                  if (entry->device_valid && !pa_namereg_is_valid_name(entry->device)) {
>                      entry_free(entry);
> diff --git a/src/modules/module-switch-on-connect.c b/src/modules/module-switch-on-connect.c
> index ebdd6aad0..faa0f289f 100644
> --- a/src/modules/module-switch-on-connect.c
> +++ b/src/modules/module-switch-on-connect.c
> @@ -112,7 +112,7 @@ static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, void*
>      }
>  
>      PA_IDXSET_FOREACH(i, old_default_sink->inputs, idx) {
> -        if (i->save_sink || !PA_SINK_INPUT_IS_LINKED(i->state))
> +        if (i->preferred_sink != NULL || !PA_SINK_INPUT_IS_LINKED(i->state))

pa_safe_streq(i->sink->name, i->preferred_sink)

>              continue;
>  
>          if (pa_sink_input_move_to(i, sink, false) < 0)
> diff --git a/src/pulsecore/sink-input.c b/src/pulsecore/sink-input.c
> index 312ec4a97..1031051a7 100644
> --- a/src/pulsecore/sink-input.c
> +++ b/src/pulsecore/sink-input.c
> @@ -190,7 +190,8 @@ bool pa_sink_input_new_data_set_sink(pa_sink_input_new_data *data, pa_sink *s, b
>      if (!data->req_formats) {
>          /* We're not working with the extended API */
>          data->sink = s;
> -        data->save_sink = save;
> +	if (save)
> +            data->preferred_sink = pa_xstrdup(s->name);
>          data->sink_requested_by_application = requested_by_application;
>      } else {
>          /* Extended API: let's see if this sink supports the formats the client can provide */
> @@ -199,7 +200,8 @@ bool pa_sink_input_new_data_set_sink(pa_sink_input_new_data *data, pa_sink *s, b
>          if (formats && !pa_idxset_isempty(formats)) {
>              /* Sink supports at least one of the requested formats */
>              data->sink = s;
> -            data->save_sink = save;
> +	    if (save)
> +		data->preferred_sink = pa_xstrdup(s->name);
>              data->sink_requested_by_application = requested_by_application;
>              if (data->nego_formats)
>                  pa_idxset_free(data->nego_formats, (pa_free_cb_t) pa_format_info_free);
> @@ -226,7 +228,7 @@ bool pa_sink_input_new_data_set_formats(pa_sink_input_new_data *data, pa_idxset
>  
>      if (data->sink) {
>          /* Trigger format negotiation */
> -        return pa_sink_input_new_data_set_sink(data, data->sink, data->save_sink, data->sink_requested_by_application);
> +        return pa_sink_input_new_data_set_sink(data, data->sink, false, data->sink_requested_by_application);

Rather than unconditionally setting save to false, we need to check if
data->preferred_sink was set previously.

>      }
>  
>      return true;
> @@ -519,7 +521,7 @@ int pa_sink_input_new(
>      pa_cvolume_reset(&i->real_ratio, i->sample_spec.channels);
>      i->volume_writable = data->volume_writable;
>      i->save_volume = data->save_volume;
> -    i->save_sink = data->save_sink;
> +    i->preferred_sink = pa_xstrdup(data->preferred_sink);
>      i->save_muted = data->save_muted;
>  
>      i->muted = data->muted;
> @@ -777,6 +779,9 @@ static void sink_input_free(pa_object *o) {
>      if (i->volume_factor_sink_items)
>          pa_hashmap_free(i->volume_factor_sink_items);
>  
> +    if (i->preferred_sink)
> +	pa_xfree(i->preferred_sink);
> +
>      pa_xfree(i->driver);
>      pa_xfree(i);
>  }
> @@ -1914,7 +1919,17 @@ int pa_sink_input_finish_move(pa_sink_input *i, pa_sink *dest, bool save) {
>          i->moving(i, dest);
>  
>      i->sink = dest;
> -    i->save_sink = save;
> +
> +    /* save == true, means user is calling the move_to() and want to
> +       save the preferred_sinke if the sink is not default_sink */
> +    if (save) {
> +	pa_xfree(i->preferred_sink);
> +	if (dest == dest->core->default_sink)
> +	    i->preferred_sink = NULL;
> +	else
> +	    i->preferred_sink = pa_xstrdup(dest->name);
> +    }
> +
>      pa_idxset_put(dest->inputs, pa_sink_input_ref(i), NULL);
>  
>      PA_HASHMAP_FOREACH(v, i->volume_factor_sink_items, state)
> diff --git a/src/pulsecore/sink-input.h b/src/pulsecore/sink-input.h
> index 9e90291ca..537cec9a1 100644
> --- a/src/pulsecore/sink-input.h
> +++ b/src/pulsecore/sink-input.h
> @@ -123,7 +123,8 @@ struct pa_sink_input {
>       * set is worth remembering, i.e. was explicitly chosen by the
>       * user and not automatically. module-stream-restore looks for
>       * this.*/
> -    bool save_sink:1, save_volume:1, save_muted:1;
> +    char *preferred_sink;
> +    bool save_volume:1, save_muted:1;

The comment above these lines is a bit illogical after this change, so
the comment needs to be updated as well. The preferred_sink deserves an
in-depth explanation about how it's used, so don't bundle that with
save_volume and save_muted.
Hui Wang Dec. 15, 2018, 8:47 a.m.
On 2018/12/12 下午9:39, Tanu Kaskinen wrote:
> Thanks for working on this! Sorry for slow review, I hope I'll be much
> quicker to comment on subsequent iterations.
>
> On Mon, 2018-11-05 at 09:47 +0800, Hui Wang wrote:
>> And don't move the stream in the module-stream-restore anymore,
>> And the preferred_sink is only set when user is calling the
>> move_to() and the module-stream-restore maintains the saving and
>> deleting of preferred_sink.
>>
>> If the target of move_to() is default_sink, preferred_sink will be
>> cleared and the entry->device will be cleared too from database.
> Can you split this so that the first patch only changes save_sink to
> preferred_sink, without any changes in behaviour? That is, put the
> "don't move the stream in the module-stream-restore" and "if the target
> of move_to() is default_sink" logic into separate patches.
>
> Also replace tabs with spaces.
OK, got it, will addressed all comments.
>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
[...]
>>           }
>>       }
>> @@ -2176,9 +2190,10 @@ static int extension_cb(pa_native_protocol *p, pa_module *m, pa_native_connectio
>>   
>>                   entry->muted = muted;
>>                   entry->muted_valid = true;
>> -
>> -                entry->device = pa_xstrdup(device);
>> -                entry->device_valid = device && !!entry->device[0];
>> +		if (device && !pa_streq(device, m->core->default_sink->name) && !pa_streq(device, m->core->default_source->name)) {
>> +		    entry->device = pa_xstrdup(device);
>> +		    entry->device_valid = device && !!entry->device[0];
>> +		}
> What's the goal here? The client tries to change an entry in the
> stream-restore database, why should that change be ignored if the
> current default sink happens to be the same as the new device? Maybe
> you intended to set entry->device to NULL in this case. But I don't
> think that's necessary either - if the client wants to unset the
> device, it can just give NULL as the device name. I don't think you
> need to change anything here.
>
> By the way, m->core->default_sink can be NULL, so that would have to be
> checked if this code was kept.

Actually I didn't change this part first, but I remember the stream bond 
did not work as expected, after changed as above, it worked as expected.

Supposing sink0 is hdmi, and is playing a music over sink0, sink1 is 
speaker,  after I unplugged the hdmi cable, the music is switched to 
speaker,  but music->preferred_sink is still sink0-hdmi, after I plug 
the hdmi cable again, the music should be switched back to sink0-hdmi.  
I remember after I unplugged the cable, a user-space app call 
extension_cb to set the music->preferred_sink to be sink1-speaker 
(default_sink), then the music can't be switched back to hdmi anymore.

I will test it again, if it is really not needed, I will drop these code.


Thanks,

Hui.


>>   
>>                   if (entry->device_valid && !pa_namereg_is_valid_name(entry->device)) {
>>                       entry_free(entry);
>>
Tanu Kaskinen Dec. 22, 2018, 7:06 p.m.
On Sat, 2018-12-15 at 16:47 +0800, Hui Wang wrote:
> On 2018/12/12 下午9:39, Tanu Kaskinen wrote:
> > Thanks for working on this! Sorry for slow review, I hope I'll be much
> > quicker to comment on subsequent iterations.
> > 
> > On Mon, 2018-11-05 at 09:47 +0800, Hui Wang wrote:
> > > And don't move the stream in the module-stream-restore anymore,
> > > And the preferred_sink is only set when user is calling the
> > > move_to() and the module-stream-restore maintains the saving and
> > > deleting of preferred_sink.
> > > 
> > > If the target of move_to() is default_sink, preferred_sink will be
> > > cleared and the entry->device will be cleared too from database.
> > Can you split this so that the first patch only changes save_sink to
> > preferred_sink, without any changes in behaviour? That is, put the
> > "don't move the stream in the module-stream-restore" and "if the target
> > of move_to() is default_sink" logic into separate patches.
> > 
> > Also replace tabs with spaces.
> OK, got it, will addressed all comments.
> > > Signed-off-by: Hui Wang <hui.wang@canonical.com>
> [...]
> > >           }
> > >       }
> > > @@ -2176,9 +2190,10 @@ static int extension_cb(pa_native_protocol *p, pa_module *m, pa_native_connectio
> > >   
> > >                   entry->muted = muted;
> > >                   entry->muted_valid = true;
> > > -
> > > -                entry->device = pa_xstrdup(device);
> > > -                entry->device_valid = device && !!entry->device[0];
> > > +		if (device && !pa_streq(device, m->core->default_sink->name) && !pa_streq(device, m->core->default_source->name)) {
> > > +		    entry->device = pa_xstrdup(device);
> > > +		    entry->device_valid = device && !!entry->device[0];
> > > +		}
> > What's the goal here? The client tries to change an entry in the
> > stream-restore database, why should that change be ignored if the
> > current default sink happens to be the same as the new device? Maybe
> > you intended to set entry->device to NULL in this case. But I don't
> > think that's necessary either - if the client wants to unset the
> > device, it can just give NULL as the device name. I don't think you
> > need to change anything here.
> > 
> > By the way, m->core->default_sink can be NULL, so that would have to be
> > checked if this code was kept.
> 
> Actually I didn't change this part first, but I remember the stream bond 
> did not work as expected, after changed as above, it worked as expected.
> 
> Supposing sink0 is hdmi, and is playing a music over sink0, sink1 is 
> speaker,  after I unplugged the hdmi cable, the music is switched to 
> speaker,  but music->preferred_sink is still sink0-hdmi, after I plug 
> the hdmi cable again, the music should be switched back to sink0-hdmi.  
> I remember after I unplugged the cable, a user-space app call 
> extension_cb to set the music->preferred_sink to be sink1-speaker 
> (default_sink), then the music can't be switched back to hdmi anymore.
> 
> I will test it again, if it is really not needed, I will drop these code.

I would guess that it's gnome-control-center that sets the device to
speakers when you select speakers as the output. gnome-control-center
updates the routing in all stream-restore entries. Once these patches
are merged (and once a new release is made), gnome-control-center can
be fixed to not mess with the stream-restore database any more. I
believe the current gnome-control-center behaviour is a workaround for
the fact that PulseAudio hasn't so far automatically moved streams when
the default sink changes.
Hui Wang Dec. 26, 2018, 1:52 a.m.
On 2018/12/23 上午3:06, Tanu Kaskinen wrote:
> On Sat, 2018-12-15 at 16:47 +0800, Hui Wang wrote:
>> On 2018/12/12 下午9:39, Tanu Kaskinen wrote:
>>> Thanks for working on this! Sorry for slow review, I hope I'll be much
>>> quicker to comment on subsequent iterations.
>>>
>>> On Mon, 2018-11-05 at 09:47 +0800, Hui Wang wrote:
>>>> And don't move the stream in the module-stream-restore anymore,
>>>> And the preferred_sink is only set when user is calling the
>>>> move_to() and the module-stream-restore maintains the saving and
>>>> deleting of preferred_sink.
>>>>
>>>> If the target of move_to() is default_sink, preferred_sink will be
>>>> cleared and the entry->device will be cleared too from database.
>>> Can you split this so that the first patch only changes save_sink to
>>> preferred_sink, without any changes in behaviour? That is, put the
>>> "don't move the stream in the module-stream-restore" and "if the target
>>> of move_to() is default_sink" logic into separate patches.
>>>
>>> Also replace tabs with spaces.
>> OK, got it, will addressed all comments.
>>>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
>> [...]
>>>>            }
>>>>        }
>>>> @@ -2176,9 +2190,10 @@ static int extension_cb(pa_native_protocol *p, pa_module *m, pa_native_connectio
>>>>    
>>>>                    entry->muted = muted;
>>>>                    entry->muted_valid = true;
>>>> -
>>>> -                entry->device = pa_xstrdup(device);
>>>> -                entry->device_valid = device && !!entry->device[0];
>>>> +		if (device && !pa_streq(device, m->core->default_sink->name) && !pa_streq(device, m->core->default_source->name)) {
>>>> +		    entry->device = pa_xstrdup(device);
>>>> +		    entry->device_valid = device && !!entry->device[0];
>>>> +		}
>>> What's the goal here? The client tries to change an entry in the
>>> stream-restore database, why should that change be ignored if the
>>> current default sink happens to be the same as the new device? Maybe
>>> you intended to set entry->device to NULL in this case. But I don't
>>> think that's necessary either - if the client wants to unset the
>>> device, it can just give NULL as the device name. I don't think you
>>> need to change anything here.
>>>
>>> By the way, m->core->default_sink can be NULL, so that would have to be
>>> checked if this code was kept.
>> Actually I didn't change this part first, but I remember the stream bond
>> did not work as expected, after changed as above, it worked as expected.
>>
>> Supposing sink0 is hdmi, and is playing a music over sink0, sink1 is
>> speaker,  after I unplugged the hdmi cable, the music is switched to
>> speaker,  but music->preferred_sink is still sink0-hdmi, after I plug
>> the hdmi cable again, the music should be switched back to sink0-hdmi.
>> I remember after I unplugged the cable, a user-space app call
>> extension_cb to set the music->preferred_sink to be sink1-speaker
>> (default_sink), then the music can't be switched back to hdmi anymore.
>>
>> I will test it again, if it is really not needed, I will drop these code.
> I would guess that it's gnome-control-center that sets the device to
> speakers when you select speakers as the output. gnome-control-center
> updates the routing in all stream-restore entries. Once these patches
> are merged (and once a new release is made), gnome-control-center can
> be fixed to not mess with the stream-restore database any more. I
> believe the current gnome-control-center behaviour is a workaround for
> the fact that PulseAudio hasn't so far automatically moved streams when
> the default sink changes.
Yes, it is gnome-control-center. OK, will drop this part.