[pulseaudio-discuss] [PATCH v2 4/8] core: move sink-inputs conditionally when update default_sink

[Tanu Kaskinen]

On Tue, 2019-07-02 at 10:25 +0200, Georg Chini wrote:
> On 02.07.19 06:49, Tanu Kaskinen wrote:
> > On Mon, 2019-07-01 at 08:48 +0200, Georg Chini wrote:
> > > On 01.07.19 08:03, Georg Chini wrote:
> > > > On 01.07.19 07:08, Tanu Kaskinen wrote:
> > > > > On Sun, 2019-06-30 at 13:52 +0200, Georg Chini wrote:
> > > > > > On 17.01.19 07:53, Hui Wang wrote:
> > > > > > > When the default sink changes, the streams from the old default sink
> > > > > > > should be moved to the new default sink, unless the preferred_sink
> > > > > > > string is set to the old default sink and the active port of the old
> > > > > > > default sink is not unavailable
> > > > > > >     +
> > > > > > > +void pa_sink_move_streams_to_default_sink(pa_core *core, pa_sink
> > > > > > > *old_sink, bool from_user) {
> > > > > > > +    pa_sink_input *i;
> > > > > > > +    uint32_t idx;
> > > > > > > +    bool old_sink_is_unavailable;
> > > > > > Does this not give a warning about using uninitialized variables?
> > > > > > > +
> > > > > > > +    pa_assert(core);
> > > > > > > +    pa_assert(old_sink);
> > > > > > > +
> > > > > > > +    if (old_sink == core->default_sink)
> > > > > > > +        return;
> > > > > > > +
> > > > > > > +    if (old_sink->active_port && old_sink->active_port->available
> > > > > > > == PA_AVAILABLE_NO)
> > > > > > > +        old_sink_is_unavailable = true;
> > > > > > This is not sufficient to determine if the old sink is still available.
> > > > > > There are sinks
> > > > > > that do not have ports (null-sink, virtual sinks). I think you will
> > > > > > need
> > > > > > another boolean
> > > > > > argument to the function which indicates availability of the old sink.
> > > > > The definition of an unavailable sink is that its active port is
> > > > > unavailable. I don't know what kind of sinks you're thinking about
> > > > > here, maybe virtual sinks that are on top of an unavailable hardware
> > > > > sink? There are many places where we check the availability of a sink
> > > > > this way, and I don't think it makes sense to be different here.
> > > > I don't understand. Virtual sinks don't have ports. So checking only
> > > > sinks that have an active port excludes all sinks that don't have
> > > > ports like the null-sink and virtual sinks. Following your definition
> > > > above, those sinks are never available.
> > You have it reversed: following my definition above, those sinks are
> > always available.
> Right, sorry. (I seem to be reading things wrong quite often
> the last few weeks ... I'll do my best to be more thorough in
> the future.)
> > > Checking the code, only alsa, bluetooth and raop sinks define ports and
> > > the "many places" you are referring to are compare_sinks() and
> > > compare_sources() in core.c and a check in module-switch-on-connect,
> > > which is used to determine the availability of the default sink.
> > At least module-stream-restore checks device availability too (although
> > probably not anymore after this patch set, because module-stream-
> > restore won't do the routing directly anymore, it will just restore the
> > preferred_sink variable, which can be done regardless of the sink
> > availability).
> > 
> > Extending the hardware sink availability to filter sinks probably makes
> > sense, but I think that's a topic for a separate patch (or patches).
> > 
> > You suggested an extra flag for pa_sink_move_streams_to_default_sink(),
> > which seems unnecessary to me. The function can in any case figure out
> > the availability by itself (possibly using a helper function
> > pa_sink_is_available() that can handle filter sinks too).
> > 
> I think some additional check is still needed at least for 
> s->unlink_requested
> because when a sink is going to be unlinked and the function is called from
> pa_sink_unlink() in patch 7, there may be nothing else that indicates 
> that the
> sink is going to be removed. When I proposed an additional argument, I did
> not see that pa_sink_unlink() already sets a flag that can be used.
> 
> So I would suggest that the helper function checks on
> - link state

I don't expect this to be necessary, although it can't hurt either.

> - port availability
> - s->unlink_requested

If this is indeed used from pa_sink_unlink() to rescue streams, then
checking this seems appropriate (and can't hurt anyway).

> - suspend state (any reason apart from SUSPEND_IDLE)

This doesn't seem appropriate, unless we start avoiding suspended sinks
in the same way we avoid unavailable sinks. That would mean that we
would never use a suspended sink as the default sink. That would
probably make sense actually, so I'm just pointing out that we need to
be consistent: if we avoid suspended sinks in one situation, we should
avoid them in all situations.

> Using the helper function in other places (and extending it if I forgot 
> something
> in the list above) could then be done in a separate patch. Are you OK 
> with that?

Yes, I think a helper function can be added, and if it's added, it's
added in a separate patch.

-- 
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk