[v2,7/8] sink: move the streams to the default_sink when the sink is unlinked

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

Details

Message ID 20190117065340.18712-8-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 a sink is unlinked, all streams of this sink are moved to
default_sink, this action is implemented in the core rather than
modules now.

Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
 src/modules/module-stream-restore.c | 50 -----------------------------
 src/pulsecore/sink.c                |  3 ++
 2 files changed, 3 insertions(+), 50 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/modules/module-stream-restore.c b/src/modules/module-stream-restore.c
index c7a5f228a..fd3acb5bd 100644
--- a/src/modules/module-stream-restore.c
+++ b/src/modules/module-stream-restore.c
@@ -96,7 +96,6 @@  struct userdata {
         *source_output_new_hook_slot,
         *source_output_fixate_hook_slot,
         *source_put_hook_slot,
-        *sink_unlink_hook_slot,
         *source_unlink_hook_slot,
         *connection_unlink_hook_slot;
     pa_time_event *save_time_event;
@@ -1691,54 +1690,6 @@  static pa_hook_result_t source_put_hook_callback(pa_core *c, pa_source *source,
     return PA_HOOK_OK;
 }
 
-static pa_hook_result_t sink_unlink_hook_callback(pa_core *c, pa_sink *sink, struct userdata *u) {
-    pa_sink_input *si;
-    uint32_t idx;
-
-    pa_assert(c);
-    pa_assert(sink);
-    pa_assert(u);
-    pa_assert(u->on_rescue && u->restore_device);
-
-    /* There's no point in doing anything if the core is shut down anyway */
-    if (c->state == PA_CORE_SHUTDOWN)
-        return PA_HOOK_OK;
-
-    PA_IDXSET_FOREACH(si, sink->inputs, idx) {
-        char *name;
-        struct entry *e;
-
-        if (!si->sink)
-            continue;
-
-        /* Skip this sink input if it is connecting a filter sink to
-         * the master */
-        if (si->origin_sink)
-            continue;
-
-        if (!(name = pa_proplist_get_stream_group(si->proplist, "sink-input", IDENTIFICATION_PROPERTY)))
-            continue;
-
-        if ((e = entry_read(u, name))) {
-
-            if (e->device_valid) {
-                pa_sink *d;
-
-                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);
-            }
-
-            entry_free(e);
-        }
-
-        pa_xfree(name);
-    }
-
-    return PA_HOOK_OK;
-}
-
 static pa_hook_result_t source_unlink_hook_callback(pa_core *c, pa_source *source, struct userdata *u) {
     pa_source_output *so;
     uint32_t idx;
@@ -2420,7 +2371,6 @@  int pa__init(pa_module*m) {
 
     if (restore_device && on_rescue) {
         /* A little bit earlier than module-intended-roles, module-rescue-streams, ... */
-        pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SINK_UNLINK], PA_HOOK_LATE, (pa_hook_cb_t) sink_unlink_hook_callback, u);
         pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SOURCE_UNLINK], PA_HOOK_LATE, (pa_hook_cb_t) source_unlink_hook_callback, u);
     }
 
diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
index cf43a78e8..d7973b7c9 100644
--- a/src/pulsecore/sink.c
+++ b/src/pulsecore/sink.c
@@ -745,6 +745,9 @@  void pa_sink_unlink(pa_sink* s) {
 
     linked = PA_SINK_IS_LINKED(s->state);
 
+    if (linked)
+        pa_sink_move_streams_to_default_sink(s->core, s, false);
+
     if (linked)
         pa_hook_fire(&s->core->hooks[PA_CORE_HOOK_SINK_UNLINK], s);
 

Comments

On 17.01.19 07:53, Hui Wang wrote:
> When a sink is unlinked, all streams of this sink are moved to
> default_sink, this action is implemented in the core rather than
> modules now.
>
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---
>   src/modules/module-stream-restore.c | 50 -----------------------------
>   src/pulsecore/sink.c                |  3 ++
>   2 files changed, 3 insertions(+), 50 deletions(-)
>
> diff --git a/src/modules/module-stream-restore.c b/src/modules/module-stream-restore.c
> index c7a5f228a..fd3acb5bd 100644
> --- a/src/modules/module-stream-restore.c
> +++ b/src/modules/module-stream-restore.c
> @@ -96,7 +96,6 @@ struct userdata {
>           *source_output_new_hook_slot,
>           *source_output_fixate_hook_slot,
>           *source_put_hook_slot,
> -        *sink_unlink_hook_slot,
>           *source_unlink_hook_slot,
>           *connection_unlink_hook_slot;
>       pa_time_event *save_time_event;
> @@ -1691,54 +1690,6 @@ static pa_hook_result_t source_put_hook_callback(pa_core *c, pa_source *source,
>       return PA_HOOK_OK;
>   }
>   
> -static pa_hook_result_t sink_unlink_hook_callback(pa_core *c, pa_sink *sink, struct userdata *u) {
> -    pa_sink_input *si;
> -    uint32_t idx;
> -
> -    pa_assert(c);
> -    pa_assert(sink);
> -    pa_assert(u);
> -    pa_assert(u->on_rescue && u->restore_device);
> -
> -    /* There's no point in doing anything if the core is shut down anyway */
> -    if (c->state == PA_CORE_SHUTDOWN)
> -        return PA_HOOK_OK;
> -
> -    PA_IDXSET_FOREACH(si, sink->inputs, idx) {
> -        char *name;
> -        struct entry *e;
> -
> -        if (!si->sink)
> -            continue;
> -
> -        /* Skip this sink input if it is connecting a filter sink to
> -         * the master */
> -        if (si->origin_sink)
> -            continue;
> -
> -        if (!(name = pa_proplist_get_stream_group(si->proplist, "sink-input", IDENTIFICATION_PROPERTY)))
> -            continue;
> -
> -        if ((e = entry_read(u, name))) {
> -
> -            if (e->device_valid) {
> -                pa_sink *d;
> -
> -                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);
> -            }
> -
> -            entry_free(e);
> -        }
> -
> -        pa_xfree(name);
> -    }
> -
> -    return PA_HOOK_OK;
> -}
> -
>   static pa_hook_result_t source_unlink_hook_callback(pa_core *c, pa_source *source, struct userdata *u) {
>       pa_source_output *so;
>       uint32_t idx;
> @@ -2420,7 +2371,6 @@ int pa__init(pa_module*m) {
>   
>       if (restore_device && on_rescue) {
>           /* A little bit earlier than module-intended-roles, module-rescue-streams, ... */
> -        pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SINK_UNLINK], PA_HOOK_LATE, (pa_hook_cb_t) sink_unlink_hook_callback, u);
>           pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SOURCE_UNLINK], PA_HOOK_LATE, (pa_hook_cb_t) source_unlink_hook_callback, u);
>       }
>   
> diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
> index cf43a78e8..d7973b7c9 100644
> --- a/src/pulsecore/sink.c
> +++ b/src/pulsecore/sink.c
> @@ -745,6 +745,9 @@ void pa_sink_unlink(pa_sink* s) {
>   
>       linked = PA_SINK_IS_LINKED(s->state);
>   
> +    if (linked)
> +        pa_sink_move_streams_to_default_sink(s->core, s, false);
> +

This is called too early. It might be the current default sink that we 
are unlinking.

>       if (linked)
>           pa_hook_fire(&s->core->hooks[PA_CORE_HOOK_SINK_UNLINK], s);
>
On 17.01.19 07:53, Hui Wang wrote:
> When a sink is unlinked, all streams of this sink are moved to
> default_sink, this action is implemented in the core rather than
> modules now.
>
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---
>   src/modules/module-stream-restore.c | 50 -----------------------------
>   src/pulsecore/sink.c                |  3 ++
>   2 files changed, 3 insertions(+), 50 deletions(-)
>
module-rescue-stream also has to be modified to reflect
that change. It should now only rescue source streams.
On 2019/6/30 下午8:15, Georg Chini wrote:
> On 17.01.19 07:53, Hui Wang wrote:
>> When a sink is unlinked, all streams of this sink are moved to
>> default_sink, this action is implemented in the core rather than
>> modules now.
>>
>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
>> ---
>>   src/modules/module-stream-restore.c | 50 -----------------------------
>>   src/pulsecore/sink.c                |  3 ++
>>   2 files changed, 3 insertions(+), 50 deletions(-)
>>
>> diff --git a/src/modules/module-stream-restore.c 
>> b/src/modules/module-stream-restore.c
>> index c7a5f228a..fd3acb5bd 100644
>> --- a/src/modules/module-stream-restore.c
>> +++ b/src/modules/module-stream-restore.c
>> @@ -96,7 +96,6 @@ struct userdata {
>>           *source_output_new_hook_slot,
>>           *source_output_fixate_hook_slot,
>>           *source_put_hook_slot,
>> -        *sink_unlink_hook_slot,
>>           *source_unlink_hook_slot,
>>           *connection_unlink_hook_slot;
>>       pa_time_event *save_time_event;
>> @@ -1691,54 +1690,6 @@ static pa_hook_result_t 
>> source_put_hook_callback(pa_core *c, pa_source *source,
>>       return PA_HOOK_OK;
>>   }
>>   -static pa_hook_result_t sink_unlink_hook_callback(pa_core *c, 
>> pa_sink *sink, struct userdata *u) {
>> -    pa_sink_input *si;
>> -    uint32_t idx;
>> -
>> -    pa_assert(c);
>> -    pa_assert(sink);
>> -    pa_assert(u);
>> -    pa_assert(u->on_rescue && u->restore_device);
>> -
>> -    /* There's no point in doing anything if the core is shut down 
>> anyway */
>> -    if (c->state == PA_CORE_SHUTDOWN)
>> -        return PA_HOOK_OK;
>> -
>> -    PA_IDXSET_FOREACH(si, sink->inputs, idx) {
>> -        char *name;
>> -        struct entry *e;
>> -
>> -        if (!si->sink)
>> -            continue;
>> -
>> -        /* Skip this sink input if it is connecting a filter sink to
>> -         * the master */
>> -        if (si->origin_sink)
>> -            continue;
>> -
>> -        if (!(name = pa_proplist_get_stream_group(si->proplist, 
>> "sink-input", IDENTIFICATION_PROPERTY)))
>> -            continue;
>> -
>> -        if ((e = entry_read(u, name))) {
>> -
>> -            if (e->device_valid) {
>> -                pa_sink *d;
>> -
>> -                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);
>> -            }
>> -
>> -            entry_free(e);
>> -        }
>> -
>> -        pa_xfree(name);
>> -    }
>> -
>> -    return PA_HOOK_OK;
>> -}
>> -
>>   static pa_hook_result_t source_unlink_hook_callback(pa_core *c, 
>> pa_source *source, struct userdata *u) {
>>       pa_source_output *so;
>>       uint32_t idx;
>> @@ -2420,7 +2371,6 @@ int pa__init(pa_module*m) {
>>         if (restore_device && on_rescue) {
>>           /* A little bit earlier than module-intended-roles, 
>> module-rescue-streams, ... */
>> -        pa_module_hook_connect(m, 
>> &m->core->hooks[PA_CORE_HOOK_SINK_UNLINK], PA_HOOK_LATE, 
>> (pa_hook_cb_t) sink_unlink_hook_callback, u);
>>           pa_module_hook_connect(m, 
>> &m->core->hooks[PA_CORE_HOOK_SOURCE_UNLINK], PA_HOOK_LATE, 
>> (pa_hook_cb_t) source_unlink_hook_callback, u);
>>       }
>>   diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
>> index cf43a78e8..d7973b7c9 100644
>> --- a/src/pulsecore/sink.c
>> +++ b/src/pulsecore/sink.c
>> @@ -745,6 +745,9 @@ void pa_sink_unlink(pa_sink* s) {
>>         linked = PA_SINK_IS_LINKED(s->state);
>>   +    if (linked)
>> +        pa_sink_move_streams_to_default_sink(s->core, s, false);
>> +
>
> This is called too early. It might be the current default sink that we 
> are unlinking.

Right, will move it after the:

     pa_core_update_default_sink(s->core);

>
>>       if (linked)
>> pa_hook_fire(&s->core->hooks[PA_CORE_HOOK_SINK_UNLINK], s);
>
>
>
On 30.06.19 14:15, Georg Chini wrote:
> On 17.01.19 07:53, Hui Wang wrote:
>> When a sink is unlinked, all streams of this sink are moved to
>> default_sink, this action is implemented in the core rather than
>> modules now.
>>
>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
>> ---
>>   src/modules/module-stream-restore.c | 50 -----------------------------
>>   src/pulsecore/sink.c                |  3 ++
>>   2 files changed, 3 insertions(+), 50 deletions(-)
>>
>> diff --git a/src/modules/module-stream-restore.c 
>> b/src/modules/module-stream-restore.c
>> index c7a5f228a..fd3acb5bd 100644
>> --- a/src/modules/module-stream-restore.c
>> +++ b/src/modules/module-stream-restore.c
>> @@ -96,7 +96,6 @@ struct userdata {
>>           *source_output_new_hook_slot,
>>           *source_output_fixate_hook_slot,
>>           *source_put_hook_slot,
>> -        *sink_unlink_hook_slot,
>>           *source_unlink_hook_slot,
>>           *connection_unlink_hook_slot;
>>       pa_time_event *save_time_event;
>> @@ -1691,54 +1690,6 @@ static pa_hook_result_t 
>> source_put_hook_callback(pa_core *c, pa_source *source,
>>       return PA_HOOK_OK;
>>   }
>>   -static pa_hook_result_t sink_unlink_hook_callback(pa_core *c, 
>> pa_sink *sink, struct userdata *u) {
>> -    pa_sink_input *si;
>> -    uint32_t idx;
>> -
>> -    pa_assert(c);
>> -    pa_assert(sink);
>> -    pa_assert(u);
>> -    pa_assert(u->on_rescue && u->restore_device);
>> -
>> -    /* There's no point in doing anything if the core is shut down 
>> anyway */
>> -    if (c->state == PA_CORE_SHUTDOWN)
>> -        return PA_HOOK_OK;
>> -
>> -    PA_IDXSET_FOREACH(si, sink->inputs, idx) {
>> -        char *name;
>> -        struct entry *e;
>> -
>> -        if (!si->sink)
>> -            continue;
>> -
>> -        /* Skip this sink input if it is connecting a filter sink to
>> -         * the master */
>> -        if (si->origin_sink)
>> -            continue;
>> -
>> -        if (!(name = pa_proplist_get_stream_group(si->proplist, 
>> "sink-input", IDENTIFICATION_PROPERTY)))
>> -            continue;
>> -
>> -        if ((e = entry_read(u, name))) {
>> -
>> -            if (e->device_valid) {
>> -                pa_sink *d;
>> -
>> -                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);
>> -            }
>> -
>> -            entry_free(e);
>> -        }
>> -
>> -        pa_xfree(name);
>> -    }
>> -
>> -    return PA_HOOK_OK;
>> -}
>> -
>>   static pa_hook_result_t source_unlink_hook_callback(pa_core *c, 
>> pa_source *source, struct userdata *u) {
>>       pa_source_output *so;
>>       uint32_t idx;
>> @@ -2420,7 +2371,6 @@ int pa__init(pa_module*m) {
>>         if (restore_device && on_rescue) {
>>           /* A little bit earlier than module-intended-roles, 
>> module-rescue-streams, ... */
>> -        pa_module_hook_connect(m, 
>> &m->core->hooks[PA_CORE_HOOK_SINK_UNLINK], PA_HOOK_LATE, 
>> (pa_hook_cb_t) sink_unlink_hook_callback, u);
>>           pa_module_hook_connect(m, 
>> &m->core->hooks[PA_CORE_HOOK_SOURCE_UNLINK], PA_HOOK_LATE, 
>> (pa_hook_cb_t) source_unlink_hook_callback, u);
>>       }
>>   diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
>> index cf43a78e8..d7973b7c9 100644
>> --- a/src/pulsecore/sink.c
>> +++ b/src/pulsecore/sink.c
>> @@ -745,6 +745,9 @@ void pa_sink_unlink(pa_sink* s) {
>>         linked = PA_SINK_IS_LINKED(s->state);
>>   +    if (linked)
>> +        pa_sink_move_streams_to_default_sink(s->core, s, false);
>> +
>
> This is called too early. It might be the current default sink that we 
> are unlinking.
>
>>       if (linked)
>> pa_hook_fire(&s->core->hooks[PA_CORE_HOOK_SINK_UNLINK], s);
>
>
I wonder if we should not drop this patch from the series. Otherwise I think
it has to be re-worked a bit. If we keep it, part of the logic in 
module-rescue-stream
needs to be copied to the core (and removed from module-rescue-stream).
First, it is a good idea not to start moving sink-inputs around if the 
core is
shutting down anyway and second, simply moving streams to the default
sink may fail. As an example, if you set your default sink to a virtual sink
and the master sink of the virtual sink goes away, you can't move the
sink-input of the virtual sink to the default sink. module-rescue-stream
has find_evacuation_sink() to determine where a sink input should be
moved to. If the patch is re-worked, remember we also need a check for
s->unlink_requested.

Tanu, what would you prefer?
On Sat, 2019-07-13 at 11:03 +0200, Georg Chini wrote:
> On 30.06.19 14:15, Georg Chini wrote:
> > On 17.01.19 07:53, Hui Wang wrote:
> > > When a sink is unlinked, all streams of this sink are moved to
> > > default_sink, this action is implemented in the core rather than
> > > modules now.
> > > 
> > > Signed-off-by: Hui Wang <hui.wang@canonical.com>
> > > ---
> > >   src/modules/module-stream-restore.c | 50 -----------------------------
> > >   src/pulsecore/sink.c                |  3 ++
> > >   2 files changed, 3 insertions(+), 50 deletions(-)
> > > 
> > > diff --git a/src/modules/module-stream-restore.c 
> > > b/src/modules/module-stream-restore.c
> > > index c7a5f228a..fd3acb5bd 100644
> > > --- a/src/modules/module-stream-restore.c
> > > +++ b/src/modules/module-stream-restore.c
> > > @@ -96,7 +96,6 @@ struct userdata {
> > >           *source_output_new_hook_slot,
> > >           *source_output_fixate_hook_slot,
> > >           *source_put_hook_slot,
> > > -        *sink_unlink_hook_slot,
> > >           *source_unlink_hook_slot,
> > >           *connection_unlink_hook_slot;
> > >       pa_time_event *save_time_event;
> > > @@ -1691,54 +1690,6 @@ static pa_hook_result_t 
> > > source_put_hook_callback(pa_core *c, pa_source *source,
> > >       return PA_HOOK_OK;
> > >   }
> > >   -static pa_hook_result_t sink_unlink_hook_callback(pa_core *c, 
> > > pa_sink *sink, struct userdata *u) {
> > > -    pa_sink_input *si;
> > > -    uint32_t idx;
> > > -
> > > -    pa_assert(c);
> > > -    pa_assert(sink);
> > > -    pa_assert(u);
> > > -    pa_assert(u->on_rescue && u->restore_device);
> > > -
> > > -    /* There's no point in doing anything if the core is shut down 
> > > anyway */
> > > -    if (c->state == PA_CORE_SHUTDOWN)
> > > -        return PA_HOOK_OK;
> > > -
> > > -    PA_IDXSET_FOREACH(si, sink->inputs, idx) {
> > > -        char *name;
> > > -        struct entry *e;
> > > -
> > > -        if (!si->sink)
> > > -            continue;
> > > -
> > > -        /* Skip this sink input if it is connecting a filter sink to
> > > -         * the master */
> > > -        if (si->origin_sink)
> > > -            continue;
> > > -
> > > -        if (!(name = pa_proplist_get_stream_group(si->proplist, 
> > > "sink-input", IDENTIFICATION_PROPERTY)))
> > > -            continue;
> > > -
> > > -        if ((e = entry_read(u, name))) {
> > > -
> > > -            if (e->device_valid) {
> > > -                pa_sink *d;
> > > -
> > > -                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);
> > > -            }
> > > -
> > > -            entry_free(e);
> > > -        }
> > > -
> > > -        pa_xfree(name);
> > > -    }
> > > -
> > > -    return PA_HOOK_OK;
> > > -}
> > > -
> > >   static pa_hook_result_t source_unlink_hook_callback(pa_core *c, 
> > > pa_source *source, struct userdata *u) {
> > >       pa_source_output *so;
> > >       uint32_t idx;
> > > @@ -2420,7 +2371,6 @@ int pa__init(pa_module*m) {
> > >         if (restore_device && on_rescue) {
> > >           /* A little bit earlier than module-intended-roles, 
> > > module-rescue-streams, ... */
> > > -        pa_module_hook_connect(m, 
> > > &m->core->hooks[PA_CORE_HOOK_SINK_UNLINK], PA_HOOK_LATE, 
> > > (pa_hook_cb_t) sink_unlink_hook_callback, u);
> > >           pa_module_hook_connect(m, 
> > > &m->core->hooks[PA_CORE_HOOK_SOURCE_UNLINK], PA_HOOK_LATE, 
> > > (pa_hook_cb_t) source_unlink_hook_callback, u);
> > >       }
> > >   diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
> > > index cf43a78e8..d7973b7c9 100644
> > > --- a/src/pulsecore/sink.c
> > > +++ b/src/pulsecore/sink.c
> > > @@ -745,6 +745,9 @@ void pa_sink_unlink(pa_sink* s) {
> > >         linked = PA_SINK_IS_LINKED(s->state);
> > >   +    if (linked)
> > > +        pa_sink_move_streams_to_default_sink(s->core, s, false);
> > > +
> > 
> > This is called too early. It might be the current default sink that we 
> > are unlinking.
> > 
> > >       if (linked)
> > > pa_hook_fire(&s->core->hooks[PA_CORE_HOOK_SINK_UNLINK], s);
> I wonder if we should not drop this patch from the series. Otherwise I think
> it has to be re-worked a bit. If we keep it, part of the logic in 
> module-rescue-stream
> needs to be copied to the core (and removed from module-rescue-stream).
> First, it is a good idea not to start moving sink-inputs around if the 
> core is
> shutting down anyway and second, simply moving streams to the default
> sink may fail. As an example, if you set your default sink to a virtual sink
> and the master sink of the virtual sink goes away, you can't move the
> sink-input of the virtual sink to the default sink. module-rescue-stream
> has find_evacuation_sink() to determine where a sink input should be
> moved to. If the patch is re-worked, remember we also need a check for
> s->unlink_requested.
> 
> Tanu, what would you prefer?

Is the question do I prefer dropping this patch or improving the patch?
I prefer improving the patch, because I think this is an important part
of the policy we want in the core (i.e. keep streams routed to the
default sink when they don't have a preferred sink set or the preferred
sink isn't available).

As far as I can tell, module-rescue-streams is redundant after this
patch (when the implementation is extended to sources as well), so it
should be removed from default.pa and system.pa, and all of its code
should be replaced with a warning that it's unnecessary, doesn't do
anything and is deprecated.

As for what should be copied from module-rescue-streams to the core -
the shutdown check is good, but I'm not sure anything else needs to be
copied. If a sink input can't be moved to the default sink, I think
it's OK to let it be killed rather than moving it to a sink that is
neither the default sink nor the sink input's preferred sink. That's
consistent with the fact that creating a new sink input fails if it
can't use the default sink.

One additional thing I noticed is that the logging in
pa_sink_move_streams_to_default_sink() needs to be improved. Currently
it says "the sink input %u is moving to %s due to default_sink is
changed", but that's not the only possible reason for the move, since
this patch calls the function also when the default sink doesn't
change.
On 15.07.19 14:40, Tanu Kaskinen wrote:
> On Sat, 2019-07-13 at 11:03 +0200, Georg Chini wrote:
>> On 30.06.19 14:15, Georg Chini wrote:
>>> On 17.01.19 07:53, Hui Wang wrote:
>>>> When a sink is unlinked, all streams of this sink are moved to
>>>> default_sink, this action is implemented in the core rather than
>>>> modules now.
>>>>
>>>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
>>>> ---
>>>>    src/modules/module-stream-restore.c | 50 -----------------------------
>>>>    src/pulsecore/sink.c                |  3 ++
>>>>    2 files changed, 3 insertions(+), 50 deletions(-)
>>>>
>>>> diff --git a/src/modules/module-stream-restore.c
>>>> b/src/modules/module-stream-restore.c
>>>> index c7a5f228a..fd3acb5bd 100644
>>>> --- a/src/modules/module-stream-restore.c
>>>> +++ b/src/modules/module-stream-restore.c
>>>> @@ -96,7 +96,6 @@ struct userdata {
>>>>            *source_output_new_hook_slot,
>>>>            *source_output_fixate_hook_slot,
>>>>            *source_put_hook_slot,
>>>> -        *sink_unlink_hook_slot,
>>>>            *source_unlink_hook_slot,
>>>>            *connection_unlink_hook_slot;
>>>>        pa_time_event *save_time_event;
>>>> @@ -1691,54 +1690,6 @@ static pa_hook_result_t
>>>> source_put_hook_callback(pa_core *c, pa_source *source,
>>>>        return PA_HOOK_OK;
>>>>    }
>>>>    -static pa_hook_result_t sink_unlink_hook_callback(pa_core *c,
>>>> pa_sink *sink, struct userdata *u) {
>>>> -    pa_sink_input *si;
>>>> -    uint32_t idx;
>>>> -
>>>> -    pa_assert(c);
>>>> -    pa_assert(sink);
>>>> -    pa_assert(u);
>>>> -    pa_assert(u->on_rescue && u->restore_device);
>>>> -
>>>> -    /* There's no point in doing anything if the core is shut down
>>>> anyway */
>>>> -    if (c->state == PA_CORE_SHUTDOWN)
>>>> -        return PA_HOOK_OK;
>>>> -
>>>> -    PA_IDXSET_FOREACH(si, sink->inputs, idx) {
>>>> -        char *name;
>>>> -        struct entry *e;
>>>> -
>>>> -        if (!si->sink)
>>>> -            continue;
>>>> -
>>>> -        /* Skip this sink input if it is connecting a filter sink to
>>>> -         * the master */
>>>> -        if (si->origin_sink)
>>>> -            continue;
>>>> -
>>>> -        if (!(name = pa_proplist_get_stream_group(si->proplist,
>>>> "sink-input", IDENTIFICATION_PROPERTY)))
>>>> -            continue;
>>>> -
>>>> -        if ((e = entry_read(u, name))) {
>>>> -
>>>> -            if (e->device_valid) {
>>>> -                pa_sink *d;
>>>> -
>>>> -                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);
>>>> -            }
>>>> -
>>>> -            entry_free(e);
>>>> -        }
>>>> -
>>>> -        pa_xfree(name);
>>>> -    }
>>>> -
>>>> -    return PA_HOOK_OK;
>>>> -}
>>>> -
>>>>    static pa_hook_result_t source_unlink_hook_callback(pa_core *c,
>>>> pa_source *source, struct userdata *u) {
>>>>        pa_source_output *so;
>>>>        uint32_t idx;
>>>> @@ -2420,7 +2371,6 @@ int pa__init(pa_module*m) {
>>>>          if (restore_device && on_rescue) {
>>>>            /* A little bit earlier than module-intended-roles,
>>>> module-rescue-streams, ... */
>>>> -        pa_module_hook_connect(m,
>>>> &m->core->hooks[PA_CORE_HOOK_SINK_UNLINK], PA_HOOK_LATE,
>>>> (pa_hook_cb_t) sink_unlink_hook_callback, u);
>>>>            pa_module_hook_connect(m,
>>>> &m->core->hooks[PA_CORE_HOOK_SOURCE_UNLINK], PA_HOOK_LATE,
>>>> (pa_hook_cb_t) source_unlink_hook_callback, u);
>>>>        }
>>>>    diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
>>>> index cf43a78e8..d7973b7c9 100644
>>>> --- a/src/pulsecore/sink.c
>>>> +++ b/src/pulsecore/sink.c
>>>> @@ -745,6 +745,9 @@ void pa_sink_unlink(pa_sink* s) {
>>>>          linked = PA_SINK_IS_LINKED(s->state);
>>>>    +    if (linked)
>>>> +        pa_sink_move_streams_to_default_sink(s->core, s, false);
>>>> +
>>> This is called too early. It might be the current default sink that we
>>> are unlinking.
>>>
>>>>        if (linked)
>>>> pa_hook_fire(&s->core->hooks[PA_CORE_HOOK_SINK_UNLINK], s);
>> I wonder if we should not drop this patch from the series. Otherwise I think
>> it has to be re-worked a bit. If we keep it, part of the logic in
>> module-rescue-stream
>> needs to be copied to the core (and removed from module-rescue-stream).
>> First, it is a good idea not to start moving sink-inputs around if the
>> core is
>> shutting down anyway and second, simply moving streams to the default
>> sink may fail. As an example, if you set your default sink to a virtual sink
>> and the master sink of the virtual sink goes away, you can't move the
>> sink-input of the virtual sink to the default sink. module-rescue-stream
>> has find_evacuation_sink() to determine where a sink input should be
>> moved to. If the patch is re-worked, remember we also need a check for
>> s->unlink_requested.
>>
>> Tanu, what would you prefer?
> Is the question do I prefer dropping this patch or improving the patch?
> I prefer improving the patch, because I think this is an important part
> of the policy we want in the core (i.e. keep streams routed to the
> default sink when they don't have a preferred sink set or the preferred
> sink isn't available).

OK, I would also prefer improving though the patch is slightly
off topic in my opinion because it does not deal with a default
sink transition. That's why I asked if it should be dropped.

>
> As far as I can tell, module-rescue-streams is redundant after this
> patch (when the implementation is extended to sources as well), so it
> should be removed from default.pa and system.pa, and all of its code
> should be replaced with a warning that it's unnecessary, doesn't do
> anything and is deprecated.
Agreed.
>
> As for what should be copied from module-rescue-streams to the core -
> the shutdown check is good, but I'm not sure anything else needs to be
> copied. If a sink input can't be moved to the default sink, I think
> it's OK to let it be killed rather than moving it to a sink that is
> neither the default sink nor the sink input's preferred sink. That's
> consistent with the fact that creating a new sink input fails if it
> can't use the default sink.
I don't understand the last sentence. I would prefer to try
to move a sink-input elsewhere when it cannot move to
the default sink, but I am also OK with killing it.
>
> One additional thing I noticed is that the logging in
> pa_sink_move_streams_to_default_sink() needs to be improved. Currently
> it says "the sink input %u is moving to %s due to default_sink is
> changed", but that's not the only possible reason for the move, since
> this patch calls the function also when the default sink doesn't
> change.
>
On Mon, 2019-07-15 at 21:41 +0200, Georg Chini wrote:
> On 15.07.19 14:40, Tanu Kaskinen wrote:
> > As for what should be copied from module-rescue-streams to the core -
> > the shutdown check is good, but I'm not sure anything else needs to be
> > copied. If a sink input can't be moved to the default sink, I think
> > it's OK to let it be killed rather than moving it to a sink that is
> > neither the default sink nor the sink input's preferred sink. That's
> > consistent with the fact that creating a new sink input fails if it
> > can't use the default sink.
> I don't understand the last sentence. I would prefer to try
> to move a sink-input elsewhere when it cannot move to
> the default sink, but I am also OK with killing it.

When an application creates a stream and doesn't specify a sink,
PulseAudio will try to use the default sink. If that doesn't work, the
stream creation fails instead of PulseAudio trying to find some other
sink that works.

To me it would make sense to be consistent in these two cases: creating
a new stream and rescuing a stream from a sink that is being removed.
If we care so much about keeping a stream alive that killing it is the
absolute last resort, then we probably should also try to find a
working alternative when the default sink doesn't work for a newly
created sink input. However, the simpler solution of just killing the
stream seems like an acceptable policy to me.