[pulseaudio-discuss] [PATCH v3 1/2] refactor default sink/source handling

[Georg Chini]

On 13.03.2017 18:45, Georg Chini wrote:
> On 13.03.2017 17:45, Tanu Kaskinen wrote:
>> On Mon, 2017-03-13 at 07:57 +0100, Georg Chini wrote:
>>> On 12.03.2017 23:07, Tanu Kaskinen wrote:
>>>> On Sun, 2017-03-12 at 16:45 +0100, Georg Chini wrote:
>>>>> On 16.02.2017 11:09, Tanu Kaskinen wrote:
>>>>>
>>>>> "Refactor" is the wrong word, you are changing the logic.
>>>>> Maybe redesign is a better verb.
>>>> Indeed. I think I'll change it to "improve" ("redesign" feels a bit
>>>> strong).
>>>>
>>>>>> Currently the default sink policy is simple: either the user has
>>>>>> configured it explicitly, in which case we always use that as the
>>>>>> default, or we pick the sink with the highest priority. The sink
>>>>>> priorities are currently static, so there's no need to worry about
>>>>>> updating the default sink when sink priorities change.
>>>>>>
>>>>>> I intend to make things a bit more complex: if the active port of 
>>>>>> a sink
>>>>>> is unavailable, the sink should not be the default sink, and I 
>>>>>> also want
>>>>>> to make sink priorities dependent on the active port, so changing 
>>>>>> the
>>>>>> port should cause re-evaluation of which sink to choose as the 
>>>>>> default.
>>>>>> Currently the default sink choice is done only when someone calls
>>>>>> pa_namereg_get_default_sink(), and change notifications are only 
>>>>>> sent
>>>>>> when a sink is created or destroyed. That makes it hard to add 
>>>>>> new rules
>>>>>> to the default sink selection policy.
>>>>>>
>>>>>> This patch moves the default sink selection to
>>>>>> pa_core_update_default_sink(), which is called whenever something
>>>>>> happens that can affect the default sink choice. That function 
>>>>>> needs to
>>>>>> know the previous choice in order to send change notifications as
>>>>>> appropriate, but previously pa_core.default_sink was only set 
>>>>>> when the
>>>>>> user had configured it explicitly. Now pa_core.default_sink is 
>>>>>> always
>>>>>> set (unless there are no sinks at all), so 
>>>>>> pa_core_update_default_sink()
>>>>>> can use that to get the previous choice. The user configuration 
>>>>>> is saved
>>>>>> in a new variable, pa_core.configured_default_sink.
>>>>>>
>>>>>> pa_namereg_get_default_sink() is now unnecessary, because
>>>>>> pa_core.default_sink can be used directly to get the
>>>>>> currently-considered-best sink. pa_namereg_set_default_sink() is
>>>>>> replaced by pa_core_set_configured_default_sink().
>>>>> It seems a bit strange to me, that module-default-device-restore and
>>>>> module-switch-on-connect set the configured_default_* variables, 
>>>>> because
>>>>> these should be the user configured values.
>>>>> If a user has configured his default sink/source,
>>>>> module-switch-on-connect will change
>>>>> it if some new device (for example a bluetooth headset) turns up. 
>>>>> This
>>>>> is then saved
>>>>> as the new default sink/source, so the original user setting is 
>>>>> lost and
>>>>> on the next start
>>>>> of pulseaudio, it will use the (possibly not existent) device as the
>>>>> default.
>>>>> It also means, that if there is no user configured value, it suddenly
>>>>> will be there
>>>>> because module-default-device-restore saves the current default as 
>>>>> the
>>>>> user default.
>>>> I don't think this is a regression. I tried to make as few behavioural
>>>> changes as possible. I'm aware that module-default-device-restore 
>>>> needs
>>>> to be improved, which requires changes in the core too. Basically, I
>>>> think configured_default_sink should be the sink name (a string) so
>>>> that it can remain set even when the configured sink is not present.
>>> Yes, it is no regression. But anyway, while you are improving it, I
>>> would use a string for the user configured sink as you say and also
>>> save the user configured value and the current value. When pulseaudio
>>> is restarted you can check if the last used default sink is still 
>>> available
>>> and if not use the user default.
>> Why would you want to save and restore the current default sink if it's
>> not configured by the user nor set by module-switch-on-connect? We want
>> to always use the configured default sink when it's available. If it's
>> not available, we'll fall back to using the sink priority based
>> algorithm.
>
> I was thinking of the case where module-switch-on-connect has
> chosen a default sink which is still available after a restart (for
> example USB) or the case where the default sink was chosen by
> priority and you have added a device with higher priority between
> two pulseaudio sessions. But always using the user default after
> restart is also OK for me.
>
>>
>>> That change could however be done in a separate patch.
>> I'm willing to change the code to use the sink name for the
>> configured_default_sink variable. Do you prefer to review that as a
>> separate patch on top of this one (I'd prefer a separate patch too, but
>> you can decide)?
>
> You can do it in a separate patch.
>
>>
>>>>> I would use two variables, user_configured_default_* and
>>>>> system_configured_default_*
>>>>> and let module-default-device-restore save both values. When 
>>>>> loading the
>>>>> configuration,
>>>>> the module can check if the system configured default still exists 
>>>>> and
>>>>> else revert to
>>>>> the user setting. Module-switch-on-connect would use the system_ 
>>>>> variables.
>>>>> The system_configured_default_* variables would take the role of 
>>>>> the current
>>>>> configured_default_* variables and the user_configured_default_*
>>>>> variables would
>>>>> only get copied into them if the sink/source is valid.
>>>>>
>>>>> This would also mean, that the user can configure any value for the
>>>>> default, if the
>>>>> sink or source does not exist, it won't be used. The user configured
>>>>> value will
>>>>> also survive any temporary changes done by module-switch-on-connect.
>>>> I acknowledge that it feels a bit weird to have module-switch-on-
>>>> connect set a variable that's supposed to be set by the user, but
>>>> again, this is not a regression, and I'm not convinced that there is
>>>> any better alternative.
>>> It is not a regression, but it is done wrong in the current code.
>>> I proposed the alternative to have two variables so that the user
>>> setting will not be forgotten.
>>>
>>>> Regarding your proposal, you didn't specify in detail how the system
>>>> and user default devices are supposed to be prioritized. When a device
>>>> is plugged in, we certainly want to override the user's previous
>>>> configuration, which would suggest prioritizing the system default 
>>>> over
>>>> the user default. But if the user then manually selects some other
>>>> device as the default, that should override the system default. If the
>>>> rule is "whichever was set last wins", then we lose no information by
>>>> folding the two variables into a single default device variable.
>>> "Whichever was set last wins" looks good to me. With the current code
>>> you definitely lose the information what the user configured, that is
>>> why it bothers me that the value is overwritten. Consider the following
>>> case:
>>> You have a system with sinks A and B where A has higher priority and
>>> the user has chosen B as default.
>>> If you now connect a new device, the default sink will change to it.
>>> If you disconnect the device again, the system will go to sink A and 
>>> not
>>> to B as the user configured, because the user setting has been
>>> overwritten.
>>> I think that is definitely wrong behavior and needs fixing. (What's 
>>> even
>>> worse, if you have module-default-device-restore loaded, it will 
>>> present
>>> sink A as the user choice if pulseaudio is restarted at this point.)
>> Okay, I see your point. Having two variables means that we remember a
>> bit more of the history. However, if we're going to have two variables
>> for that purpose, it's better to have "configured_default_sink" and
>> "previous_configured_default_sink". Consider this case:
>>
>> 1. The user sets sink A as the default sink.
>> 2. Sink B is plugged in, it becomes the default.
>> 3. Sink C is plugged in, it becomes the default.
>> 4. Sink C is plugged out.
>>
>> With your proposal sink A would become the new default, but I think
>> sink B would be a better choice.
>
> OK, it would be nice to have the whole history, but falling back to the
> user configured default is in any case better than what is done now.
> Also, module-default-device-restore would be able to really save the
> user default instead of some sink that happens to be default because
> of priority or because of module-switch-on-connect.
>
> And it should in fact be very easy to implement, basically you can leave
> it as it is and introduce an additional user_configured_default variable.
> This variable is only set from the various API's and if the user 
> configured
> sink/source is valid, it is just copied into your current 
> configured_default
> variable.
> module-default-device-restore can then save only the user default (or
> better the user default plus the current default).

I forgot one point: In your selection scheme you look first if there is
your current configured_default and after that you look for the user
default. This should always work:
If the user configured a valid default, your default will be the same if
module-switch-on-connect did not chose another value. If your
configured default is different and goes away, the next choice is the
user default (which can at that point be copied into your 
configured_default).
And only after that, the default will be chosen based on priority.