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

[Georg Chini]

On 05.07.19 09:56, Tanu Kaskinen wrote:
> 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.

Yes, we should do that. Therefore my suggestion is to add a helper
function to this patch set. For consistency we could exclude the
suspend state for the moment, though I would recommend to include
it. Using the helper function outside this patch set in all places where
we check availability should then be the task of another patch series.
By suspended I guess you mean suspended apart from idle, correct?

>
>> 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.
>
I don't really understand what your suggestion means for the patch set
we are discussing. Do you mean the helper should be added to that
set but in a separate patch? Or do you mean it should not be added
to this set? If it is not added, at least an additional check for
s->unlink_requested is required in patch 7.