Re: [pulseaudio-discuss] [PATCH v3 1/2] refactor default sink/source handling
On Mon, 2017-03-13 at 20:34 +0100, Georg Chini wrote: > One more thing before you push your patch: In source_put_cb() > and sink_put_cb() you do not use the return value of > create_dbus_object_for_*(). There should be a (void) before the > calls to avoid warnings about the unused return value. Did you actually see a warning? My compiler doesn't warn about that. (But I'll add the void casts anyway.) -- Tanu https://www.patreon.com/tanuk ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v3 1/2] refactor default sink/source handling
On 13.03.2017 20:04, Tanu Kaskinen wrote: On Mon, 2017-03-13 at 18:45 +0100, Georg Chini wrote: On 13.03.2017 17:45, Tanu Kaskinen wrote: On Mon, 2017-03-13 at 07:57 +0100, Georg Chini wrote: 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. When you say "user default", do you exclude the default set by module- switch-on-connect? When I said "the configured default sink", I meant the default sink that is configured either by the user or module- switch-on-connect, whichever happened last. The two cases have the same priority. "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. It seems like you didn't get my point. To select B instead of A in my example doesn't require the whole history. It only requires remembering the previous configured sink in addition to the current configured sink (again, by "configured sink" I mean both user-configured and configured by module-switch-on-connect). I believe this arrangement would cover all the cases you want covered, plus the example above that your proposal doesn't cover, and it shouldn't be any more difficult to implement (but I still choose not to work on it at this time). Well, I think you did not get my point either. The one-step history based approach would still overwrite the users choice in your example and I think that is the bad thing that happens. But anyway, lets stop the discussion here, it won't lead anywhere. When I talked about the full history, I meant that if I'm going to spend effort on adding history information, I'd prefer having the full history rather than remembering just one step back. But if you want, feel free to implement the one-step solution. It would be an improvement after all, and simpler than the full history solution. As said above, I would choose another approach to ensure that the users choice does not get lost. Maybe I'll do that when your patches are pushed. One more thing before you push your patch: In source_put_cb() and sink_put_cb() you do not use the return value of create_dbus_object_for_*(). There should be a (void) before the calls to avoid warnings about the unused return value. ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v3 1/2] refactor default sink/source handling
On Mon, 2017-03-13 at 18:45 +0100, Georg Chini wrote: > On 13.03.2017 17:45, Tanu Kaskinen wrote: > > On Mon, 2017-03-13 at 07:57 +0100, Georg Chini wrote: > > > 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. When you say "user default", do you exclude the default set by module- switch-on-connect? When I said "the configured default sink", I meant the default sink that is configured either by the user or module- switch-on-connect, whichever happened last. The two cases have the same priority. > > > "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. It seems like you didn't get my point. To select B instead of A in my example doesn't require the whole history. It only requires remembering the previous configured sink in addition to the current configured sink (again, by "configured sink" I mean both user-configured and configured by module-switch-on-connect). I believe this arrangement would cover all the cases you want covered, plus the example above that your proposal doesn't cover, and it shouldn't be any more difficult to implement (but I still choose not to work on it at this time). When I talked about the full history, I meant that if I'm going to spend effort on adding history information, I'd prefer having the full history rather than remembering just one step back. But if you want, feel free to implement the one-step solution. It would be an improvement after all, and simpler than the full history solution. -- Tanu https://www.patreon.com/tanuk ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v3 1/2] refactor default sink/source handling
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 th
Re: [pulseaudio-discuss] [PATCH v3 1/2] refactor default sink/source handling
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 co
Re: [pulseaudio-discuss] [PATCH v3 1/2] refactor default sink/source handling
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 v
Re: [pulseaudio-discuss] [PATCH v3 1/2] refactor default sink/source handling
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.
That change could however be done 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
Re: [pulseaudio-discuss] [PATCH v3 1/2] refactor default sink/source handling
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.
> 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.
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.
> > +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 t
Re: [pulseaudio-discuss] [PATCH v3 1/2] refactor default sink/source handling
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.
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 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.
+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.
+
+if (sink == old_sink)
+return;
+
+core->configured_default_sink = sink;
+pa_log_info("configured_default_sink: %s -> %s",
+old_sink ? old_sink->name : "(unset)", sink ? sink->name :
"(unset)");
+
+pa_core_update_default_sink(core);
+}
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] [PATCH v3 1/2] refactor default sink/source handling
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(). I haven't confirmed it, but I expect that this patch will fix problems in the D-Bus protocol related to default sink handling. The D-Bus protocol used to get confused when the current default sink gets removed. It would incorrectly think that if there's no explicitly configured default sink, then there's no default sink at all. Even worse, when the D-Bus thinks that there's no default sink, it concludes that there are no sinks at all, which made it impossible to configure the default sink via the D-Bus interface. Now that pa_core.default_sink is always set, except when there really aren't any sinks, the D-Bus protocol should behave correctly. BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=99425 --- Changes in v3: - Fixed a crash in module-dbus-protocol: the module would crash if a new sink became immediately the default, because the module assumed that the sink put hook would always be fired before the default sink changed hook. - Changed the default source selection logic so that if we're comparing two monitor sources, the monitored sinks are compared before resorting to checking whether one of the sources is already the default. Changes in v2: - Added a note about the D-Bus protocol bug to the commit message. - Changed the default sink/source selection logic to prefer the current default device when comparing two devices with the same priority. - Removed the redundant call to pa_core_update_default_sink/source() when unlinking the current configured default sink or source. pa_core_set_configured_default_sink/source() will call that function anyway. - Made the comment on pa_core.default_sink/source a bit more verbose. - Dropped the patch that uses the active port's priority as the sink/source priority. The discussion is ongoing about what to do about that, so I'll submit that patch separately later, or a different patch that achieves the same goal. src/modules/dbus/iface-core.c | 114 +-- src/modules/dbus/iface-sample.c | 10 +- src/modules/module-default-device-restore.c | 14 +-- src/modules/module-intended-roles.c | 37 +++--- src/modules/module-rescue-streams.c | 20 ++-- src/modules/module-switch-on-connect.c | 30 ++--- src/pulsecore/cli-command.c | 20 ++-- src/pulsecore/cli-text.c| 12 +- src/pulsecore/core.c| 170 src/pulsecore/core.h| 28 - src/pulsecore/namereg.c | 115 +-- src/pulsecore/namereg.h | 6 - src/pulsecore/protocol-native.c | 19 ++-- src/pulsecore/sink.c| 7 ++ src/pulsecore/source.c | 7 ++ 15 files changed, 370 insertions(+), 239 deletions(-) diff --git a/src/modules/dbus/iface-core.c b/src/modules/dbus/iface-core.c index 508913de1..3f368ab46 100644 --- a/src/modules/dbus/iface-core.c +++ b/src/modules/dbus/iface-core.c @@ -721,7 +721,7 @@ static void handle_set_fallback_sink(DBusConnection *conn, DBusMessage *msg, DBu return; } -pa_namereg_set_default_sink(c->core, pa_dbusiface_device_get_sink(fallback_sink)); +pa_core_set_configured_default_sink(c->co
