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@xxxxxxxxxxxxx>
> > > ---
> > > 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.
--
Tanu
https://www.patreon.com/tanuk
https://liberapay.com/tanuk
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
