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

Mon Mar 13 16:45:54 UTC 2017

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.

> 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)?

> > > 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.

I'm not volunteering to implement this, at least any time soon. And if
I will at some point work on this, I think I would prefer to go
straight to managing the full history with a priority list: remember
all sinks that have ever been configured as the default sink, and pick
the first one that is available. When a sink is configured as the
default sink, it moves to the top of the list.

> > 
> > > > +void pa_core_set_configured_default_sink(pa_core *core, pa_sink *sink) {
> > > > +    pa_sink *old_sink;
> > > > +
> > > > +    pa_assert(core);
> > > > +
> > > > +    old_sink = core->configured_default_sink;
> > > 
> > > Why do you use a new variable here? If you move the assignment after
> > > the pa_log_info() the variable is not necessary. This also applies to a few
> > > other functions.
> > 
> > I find the extra variable to make the code a bit clearer. I don't feel
> > very strongly about this, though, so if it bothers you, I can change
> > it.
> 
> I don't really see how it makes things clearer, but I also don't mind 
> keeping
> it if you prefer.

Okay, I'll keep the code as it is.

-- 
Tanu

https://www.patreon.com/tanuk