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

[Georg Chini]

On 05.07.19 09:41, Tanu Kaskinen wrote:
> On Tue, 2019-07-02 at 09:08 +0200, Georg Chini wrote:
>> On 02.07.19 08:43, Tanu Kaskinen wrote:
>>> On Mon, 2019-07-01 at 08:03 +0200, 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
>>>>>>>
>>>>>>>
>>>>>>>      
>>>>>>>      static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, void* userdata) {
>>>>>>> -    pa_sink_input *i;
>>>>>>> -    uint32_t idx;
>>>>>>> -    pa_sink *old_default_sink;
>>>>>>>          const char *s;
>>>>>>>          struct userdata *u = userdata;
>>>>>>>      
>>>>>>> @@ -88,7 +85,7 @@ static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, void*
>>>>>>>      
>>>>>>>          /* No default sink, nothing to move away, just set the new default */
>>>>>>>          if (!c->default_sink) {
>>>>>>> -        pa_core_set_configured_default_sink(c, sink->name);
>>>>>>> +        pa_core_set_configured_default_sink(c, sink->name, false);
>>>>>>>              return PA_HOOK_OK;
>>>>>>>          }
>>>>>>>      
>>>>>>> @@ -103,28 +100,8 @@ static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, void*
>>>>>>>                  return PA_HOOK_OK;
>>>>>>>              }
>>>>>>>      
>>>>>>> -    old_default_sink = c->default_sink;
>>>>>>> -
>>>>>>>          /* Actually do the switch to the new sink */
>>>>>>> -    pa_core_set_configured_default_sink(c, sink->name);
>>>>>>> -
>>>>>>> -    /* Now move all old inputs over */
>>>>>>> -    if (pa_idxset_size(old_default_sink->inputs) <= 0) {
>>>>>>> -        pa_log_debug("No sink inputs to move away.");
>>>>>>> -        return PA_HOOK_OK;
>>>>>>> -    }
>>>>>>> -
>>>>>>> -    PA_IDXSET_FOREACH(i, old_default_sink->inputs, idx) {
>>>>>>> -        if (pa_safe_streq(i->sink->name, i->preferred_sink) || !PA_SINK_INPUT_IS_LINKED(i->state))
>>>>>>> -            continue;
>>>>>>> -
>>>>>>> -        if (pa_sink_input_move_to(i, sink, false) < 0)
>>>>>>> -            pa_log_info("Failed to move sink input %u \"%s\" to %s.", i->index,
>>>>>>> -                        pa_strnull(pa_proplist_gets(i->proplist, PA_PROP_APPLICATION_NAME)), sink->name);
>>>>>>> -        else
>>>>>>> -            pa_log_info("Successfully moved sink input %u \"%s\" to %s.", i->index,
>>>>>>> -                        pa_strnull(pa_proplist_gets(i->proplist, PA_PROP_APPLICATION_NAME)), sink->name);
>>>>>>> -    }
>>>>>>> +    pa_core_set_configured_default_sink(c, sink->name, false);
>>>>>> I wonder if we could use something like
>>>>>> pa_core_set_temporary_default_sink() here
>>>>>> and have a variable core->temporary_default_sink that has even higher
>>>>>> priority
>>>>>> than core->configured_default_sink in the default sink selection
>>>>>> process. The temporary
>>>>>> default sink could be reset when the sink vanishes again or replaced
>>>>>> when another new
>>>>>> sink turns up. What module-switch-on-connect does is to temporarily
>>>>>> override the default
>>>>>> sink that the user configured. If we save this in another variable we
>>>>>> would not overwrite
>>>>>> the user configured default sink by a sink that is not expected to be
>>>>>> present the next
>>>>>> time PA is started. But that would be just nice to have.
>>>>> I'm against adding that kind of extra complexity without a very good
>>>>> justification. module-switch-on-connect should become nearly obsolete
>>>>> anyway after this patch set has been merged.
>>>> We already talked about this some time ago and you agreed
>>>> that this would be useful. Maybe you remember that discussion.
>>> I remember that we discussed my initial attempt at implementing what
>>> this patch set does. I probably should go back to that discussion to
>>> find out why I agreed that we should add a separate "temporary default
>>> sink" variable.
>> You did not exactly agree to adding a temporary_default_sink
>> variable. You agreed however (after some discussion) that
>> what module-switch-on-connect does is a temporary override
>> of the configured default sink and that there should be a way
>> to restore the original default sink. (The user may have set
>> another than the highest priority sink as default, see also
>> below).
>>
>> It seems quite simple to add such a variable and replace it
>> when a new sink turns up or set it to NULL if the current
>> temporary default sink is removed. This would then make
>> it really easy to implement the module-switch-on-connect
>> functionality in the core at some later point.
> I'm concerned about the complexity of answering the question "how is
> the default sink determined". The fact that "default sink" and
> "configured default sink" are different things is already bothersome.

Why do you find this bothersome? I think it is pretty obvious
that the two are not necessarily identical.

> Your point about restoring the old configured default sink is
> definitely valid, although that problem seems to be not specific to
> module-switch-on-connect: if there are 3 sinks, the user can't specify
> which of them is the second most preferred sink in case the configured
> default sink goes away. Having a priority list of sinks based on manual
> configuration history might be a better way to solve this (although I'm
> afraid it's not that simple if there are sink with multiple ports).

I remember you proposed some kind of history, but I personally
don't think a long history is required. Just going back to the last
sink if I temporarily overwrote the default sink seems fully
sufficient from a user perspective.

But I see you don't want something like that (at least not now), so
I guess we forget my initial suggestion. It will probably work correctly
anyway, because the user will have the configured default sink set.
See also below.

>
>>>> Consider you add your BT speakers and module-switch-on-connect
>>>> makes them the default sink. When you remove them, there is
>>>> no guarantee that the default sink will revert to what the user
>>>> configured before. Instead PA will choose the sink which it thinks
>>>> is best. Also, if you remove the device after a shutdown, PA will
>>>> still keep it as the configured default sink and the user will loose
>>>> what was configured.
>>>>
>>>> So in my opinion we should distinguish if the server changed the
>>>> default sink or the user did, though as already said above, it is
>>>> not mandatory but only nice to have.
>>>>
>>>> I do not see why module-switch-on-connect would become obsolete
>>>> - it switches the default sink to a newly appeared sink and there is
>>>> nothing in these patches which does that (unless the newly appeared
>>>> sink is the configured default sink).
>>> When a new sink appears, pa_core_update_default_sink() is called. Since
>>> PulseAudio 11.1, bluetooth and USB sinks have higher priority than the
>>> internal sound card, so pa_core_update_default_sink() will switch to
>>> the BT speakers in your example. The main benefit of module-switch-on-
>>> connect was that it moved existing streams to the new sink, but after
>>> this patch set that's handled by the core. Therefore there's much less
>>> need for module-switch-on-connect.
>>>
>> This is true, but only relevant if there is no configured default
>> sink. If the configured default sink is set, there will be no switch
>> to a newly appearing sink because the configured default sink
>> is prioritized over all other sinks. In this case you still need
>> module-switch-on-connect.
> My estimation is that this is very rarely a problem.

Mh, my estimation is, that this will be the normal case. At some
point the user will set a default sink manually, and from that point
on, automatic switching will no longer work. The configured default
sink is never unset once it is set.

>
> If you have manually selected an external sound card as the default
> sink and you then plug in another external sound card, then you may or
> may not want to automatically switch to the new sound card. If you
> fiddle a lot with two external sound cards and you always want to use
> the last one plugged in, then module-switch-on-connect is still useful,
> but in this case it doesn't really matter that your old default device
> choice is forgotten, because PulseAudio will anyway prefer the
> remaining external sound card.
This topic is not about forgetting the last sink, but about switching
to a new sink at all. Once you manually set the default sink, it will
never switch automatically without module-switch-on-connect,
because the default sink selection process prefers the configured
default sink over all other sinks.
>
> If you have an external sound card and you have manually selected the
> internal sound card as the default sink, then module-switch-on-connect
> seems like a bad idea, because you probably want to keep the internal
> sound card as the default even after plugging out the external sound
> card and plugging it back in. If your preference has changed, then it's
> expected that you have to restate your preference by manually selecting
> the external sound card.
>