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

[Georg Chini]

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