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

[Georg Chini]

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
- port availability
- s->unlink_requested
- suspend state (any reason apart from SUSPEND_IDLE)

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?