[pulseaudio-discuss] [PATCH v2 7/8] sink: move the streams to the default_sink when the sink is unlinked

[Georg Chini]

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