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

[Georg Chini]

On 05.07.19 10:57, Georg Chini wrote:
> 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.
Thinking again, it won't work correctly because we overwrite the configured
default sink here. This means when disconnecting the device we have lost
the user configured sink and go back to what PA thinks is best. So in all
cases where the user had configured a low priority sink as default, we will
move to the wrong sink.