[pulseaudio-discuss] [RFC PATCH 0/4] Audio routing fixes and improvements

[João Paulo Rechi Vita]

These are patches we have been carrying downstream for quite some time
now at Endless (some of them which are a refactor of an earlier work by
Mario Sanchez Prada from ~3.5 years ago), plus a fix for problem I have
worked at recently (patches 3 and 4).

Essentially we want to have active streams (and default sink/source
configuration) moved to new devices on hotplug, but respecting a
priority list. My understanding is that this also what
module-device-manager provides, but it relies on something (a client?)
registering the priority lists, which on Colin's main target scenario
was kmix/phonon (although I must confess I have not looked too deeply
into m-d-m, so some of this may not be super accurate). We don't have
such piece to register priority lists at Endless, which uses the default
GNOME audio UI, and for this simpler, static case, a few changes on
module-switch-on-port-available (patches 1 and 2 of this series) have
been enough. The priorities come from the ALSA paths configuration
files, which we also tweaked a bit to fit our needs (not part of this
series).

In addition to what I mentioned on the previous paragraph, we also want
to have the same functionality working across different cards,
specifically for some laptops with AMD graphics, where all analog ports
are on one ALSA card but the HDMI port is on a separate ALSA card which
only has that port. With the recent default sink / source logic
improvements (thanks Tanu!) we can now have it working through
module-switch-on-connect, as long as we change it to not ignore HDMI
sinks (patch 4). But when testing this case I stumbled upon a problem
where a card can get stuck on an unavailable profile when there is no
other port to switch to, with a non-working sink available, which I'm
addressing with patch 3.

I believe all of these would be beneficial upstream and none of them are
too controversial, thus this series, but I'm open for any suggestions on
other possible approaches to address these problems and to do some
rework to have them suit upstream better.

Thanks in advance for any support.

João Paulo Rechi Vita (4):
card: New API pa_card_profile_has_available_ports
switch-on-port-available: Change criteria for keeping the active
profile
alsa-card: Switch profile when the active one becomes unavailable
switch-on-connect: Do not ignore HDMI sinks

src/modules/alsa/module-alsa-card.c | 54 +++++++++++++++++++
src/modules/module-switch-on-connect.c | 13 +++--
src/modules/module-switch-on-port-available.c | 32 +++++------
src/pulsecore/card.c | 18 +++++++
src/pulsecore/card.h | 3 ++
5 files changed, 95 insertions(+), 25 deletions(-)

[João Paulo Rechi Vita]

New function to check if a card has any ports with a certain
availability for a specific direction.

This was built uppon previous work by Mario Sanchez Prada.
---
src/pulsecore/card.c | 18 ++++++++++++++++++
src/pulsecore/card.h | 3 +++
2 files changed, 21 insertions(+)

diff --git a/src/pulsecore/card.c b/src/pulsecore/card.c
index c785f00a7..a3c732341 100644
--- a/src/pulsecore/card.c
+++ b/src/pulsecore/card.c
@@ -79,6 +79,24 @@ void pa_card_profile_set_available(pa_card_profile *c, pa_available_t available)
pa_hook_fire(&core->hooks[PA_CORE_HOOK_CARD_PROFILE_AVAILABLE_CHANGED], c);
}

+bool pa_card_profile_has_available_ports(pa_card_profile *c, pa_direction_t direction, pa_available_t available) {
+ pa_card *card;
+ pa_device_port *port;
+ void *state;
+
+ pa_assert(c);
+
+ card = c->card;
+ pa_assert(card);
+
+ PA_HASHMAP_FOREACH(port, card->ports, state) {
+ if (pa_hashmap_get(port->profiles, c->name) && port->direction == direction && port->available == available)
+ return true;
+ }
+
+ return false;
+}
+
pa_card_new_data* pa_card_new_data_init(pa_card_new_data *data) {
pa_assert(data);

diff --git a/src/pulsecore/card.h b/src/pulsecore/card.h
index 09284f6ff..50f106c5e 100644
--- a/src/pulsecore/card.h
+++ b/src/pulsecore/card.h
@@ -119,6 +119,9 @@ void pa_card_profile_free(pa_card_profile *c);
/* The profile's available status has changed */
void pa_card_profile_set_available(pa_card_profile *c, pa_available_t available);

+/* Check if card has any ports with a certain availability for a specific direction */
+bool pa_card_profile_has_available_ports(pa_card_profile *c, pa_direction_t direction, pa_available_t available);
+
pa_card_new_data *pa_card_new_data_init(pa_card_new_data *data);
void pa_card_new_data_set_name(pa_card_new_data *data, const char *name);
void pa_card_new_data_set_preferred_port(pa_card_new_data *data, pa_direction_t direction, pa_device_port *port);

[Tanu Kaskinen]

Post by João Paulo Rechi Vita
New function to check if a card has any ports with a certain
availability for a specific direction.
This was built uppon previous work by Mario Sanchez Prada.
---
src/pulsecore/card.c | 18 ++++++++++++++++++
src/pulsecore/card.h | 3 +++
2 files changed, 21 insertions(+)
diff --git a/src/pulsecore/card.c b/src/pulsecore/card.c
index c785f00a7..a3c732341 100644
--- a/src/pulsecore/card.c
+++ b/src/pulsecore/card.c
@@ -79,6 +79,24 @@ void pa_card_profile_set_available(pa_card_profile *c, pa_available_t available)
pa_hook_fire(&core->hooks[PA_CORE_HOOK_CARD_PROFILE_AVAILABLE_CHANGED], c);
}
+bool pa_card_profile_has_available_ports(pa_card_profile *c, pa_direction_t direction, pa_available_t available) {

I think pa_card_profile_has_ports_with_availability() would be a better
name, because the function can return false even if the profile has
available ports, which is confusing with the current name.

Nitpicking: I'd prefer "cp" (or "profile") as the card profile variable
name, because I associate "c" with pa_card, so reading "c->name" caused
slight confusion for me.

Post by João Paulo Rechi Vita
+ pa_card *card;
+ pa_device_port *port;
+ void *state;
+
+ pa_assert(c);
+
+ card = c->card;
+ pa_assert(card);
+
+ PA_HASHMAP_FOREACH(port, card->ports, state) {
+ if (pa_hashmap_get(port->profiles, c->name) && port->direction == direction && port->available == available)
+ return true;
+ }
+
+ return false;
+}
+
pa_card_new_data* pa_card_new_data_init(pa_card_new_data *data) {
pa_assert(data);
diff --git a/src/pulsecore/card.h b/src/pulsecore/card.h
index 09284f6ff..50f106c5e 100644
--- a/src/pulsecore/card.h
+++ b/src/pulsecore/card.h
@@ -119,6 +119,9 @@ void pa_card_profile_free(pa_card_profile *c);
/* The profile's available status has changed */
void pa_card_profile_set_available(pa_card_profile *c, pa_available_t available);
+/* Check if card has any ports with a certain availability for a specific direction */
+bool pa_card_profile_has_available_ports(pa_card_profile *c, pa_direction_t direction, pa_available_t available);
+
pa_card_new_data *pa_card_new_data_init(pa_card_new_data *data);
void pa_card_new_data_set_name(pa_card_new_data *data, const char *name);
void pa_card_new_data_set_preferred_port(pa_card_new_data *data, pa_direction_t direction, pa_device_port *port);

[João Paulo Rechi Vita]

Post by Tanu Kaskinen

Post by João Paulo Rechi Vita
New function to check if a card has any ports with a certain
availability for a specific direction.
This was built uppon previous work by Mario Sanchez Prada.
---
src/pulsecore/card.c | 18 ++++++++++++++++++
src/pulsecore/card.h | 3 +++
2 files changed, 21 insertions(+)
diff --git a/src/pulsecore/card.c b/src/pulsecore/card.c
index c785f00a7..a3c732341 100644
--- a/src/pulsecore/card.c
+++ b/src/pulsecore/card.c
@@ -79,6 +79,24 @@ void pa_card_profile_set_available(pa_card_profile *c, pa_available_t available)
pa_hook_fire(&core->hooks[PA_CORE_HOOK_CARD_PROFILE_AVAILABLE_CHANGED], c);
}
+bool pa_card_profile_has_available_ports(pa_card_profile *c, pa_direction_t direction, pa_available_t available) {

I think pa_card_profile_has_ports_with_availability() would be a better
name, because the function can return false even if the profile has
available ports, which is confusing with the current name.
Nitpicking: I'd prefer "cp" (or "profile") as the card profile variable
name, because I associate "c" with pa_card, so reading "c->name" caused
slight confusion for me.

Agreed, I'm going to resubmit this as a gitlab MR (which IIUC is now
the preferred way for patch submissions and CR).

Thanks,

--
João Paulo Rechi Vita
http://about.me/jprvita

[João Paulo Rechi Vita]

When the active profile of a card becomes unavailable and no other
module changes it to a better profile (i.e. there are no available ports
that module-switch-on-port-available could switch to) the card will be
stuck on an unavailable profile with a non-working sink/source and any
active streams connected to that sink/source will remain connected.

This commit switches to a different profile when the active profile
becomes unavailble, looking for a profile with availability yes or
unknown with the highest priority, and ultimately fall-backing to the
OFF profile.

With this fix a card that only has one port can have the streams
connected to its sink/source moved away by module-rescue-stream when
that port becomes unavailable. This has been seen on machines with AMD
graphics, where the HDMI port lives on a separate ALSA card that only
has that port.
---
src/modules/alsa/module-alsa-card.c | 54 +++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)

diff --git a/src/modules/alsa/module-alsa-card.c b/src/modules/alsa/module-alsa-card.c
index 041d53121..1fc5216fc 100644
--- a/src/modules/alsa/module-alsa-card.c
+++ b/src/modules/alsa/module-alsa-card.c
@@ -735,6 +735,57 @@ static pa_hook_result_t source_output_unlink_hook_callback(pa_core *c, pa_source
return PA_HOOK_OK;
}

+static pa_card_profile *find_best_profile(pa_card *card) {
+ pa_card_profile *profile = NULL;
+ pa_card_profile *best_profile = NULL;
+ void *state;
+
+ pa_assert(card);
+ best_profile = pa_hashmap_get(card->profiles, "off");
+
+ PA_HASHMAP_FOREACH(profile, card->profiles, state) {
+ if (profile->available == PA_AVAILABLE_NO)
+ continue;
+
+ if (!pa_card_profile_has_available_ports(profile, PA_DIRECTION_OUTPUT, PA_AVAILABLE_YES))
+ continue;
+
+ if (profile->priority > best_profile->priority)
+ best_profile = profile;
+ }
+
+ if (pa_safe_streq(best_profile->name, "off")) {
+ PA_HASHMAP_FOREACH(profile, card->profiles, state) {
+ if (profile->available == PA_AVAILABLE_NO)
+ continue;
+
+ if (!pa_card_profile_has_available_ports(profile, PA_DIRECTION_OUTPUT, PA_AVAILABLE_UNKNOWN))
+ continue;
+
+ if (profile->priority > best_profile->priority)
+ best_profile = profile;
+ }
+ }
+
+ return best_profile;
+}
+
+static pa_hook_result_t card_profile_available_changed(pa_core *c, pa_card_profile *profile, struct userdata *u) {
+ pa_card *card;
+
+ pa_assert(profile);
+ pa_assert_se(card = profile->card);
+
+ if (profile->available != PA_AVAILABLE_NO)
+ return PA_HOOK_OK;
+
+ if (!pa_safe_streq(profile->name, card->active_profile->name))
+ return PA_HOOK_OK;
+
+ pa_log_debug("Active profile %s on card %s became unavailable, switching to another profile", profile->name, card->name);
+ return pa_card_set_profile(card, find_best_profile(card), false);
+}
+
int pa__init(pa_module *m) {
pa_card_new_data data;
bool ignore_dB = false;
@@ -912,6 +963,9 @@ int pa__init(pa_module *m) {

init_jacks(u);

+ pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_CARD_PROFILE_AVAILABLE_CHANGED], PA_HOOK_NORMAL,
+ (pa_hook_cb_t) card_profile_available_changed, u);
+
pa_card_choose_initial_profile(u->card);

/* If the "profile" modarg is given, we have to override whatever the usual

[Tanu Kaskinen]

Post by João Paulo Rechi Vita
When the active profile of a card becomes unavailable and no other
module changes it to a better profile (i.e. there are no available ports
that module-switch-on-port-available could switch to) the card will be
stuck on an unavailable profile with a non-working sink/source and any
active streams connected to that sink/source will remain connected.
This commit switches to a different profile when the active profile
becomes unavailble, looking for a profile with availability yes or
unknown with the highest priority, and ultimately fall-backing to the
OFF profile.
With this fix a card that only has one port can have the streams
connected to its sink/source moved away by module-rescue-stream when
that port becomes unavailable. This has been seen on machines with AMD
graphics, where the HDMI port lives on a separate ALSA card that only
has that port.

Sounds good, but have you checked that the profile gets restored back
to something else than off once you plug the device back in? At least
at some point this was an issue, and I think it hasn't been fixed. If
this is still an issue, module-switch-on-port-available needs to be
fixed first to do the profile switch away from off when something gets
plugged in.

[João Paulo Rechi Vita]

Post by Tanu Kaskinen

Post by João Paulo Rechi Vita
When the active profile of a card becomes unavailable and no other
module changes it to a better profile (i.e. there are no available ports
that module-switch-on-port-available could switch to) the card will be
stuck on an unavailable profile with a non-working sink/source and any
active streams connected to that sink/source will remain connected.
This commit switches to a different profile when the active profile
becomes unavailble, looking for a profile with availability yes or
unknown with the highest priority, and ultimately fall-backing to the
OFF profile.
With this fix a card that only has one port can have the streams
connected to its sink/source moved away by module-rescue-stream when
that port becomes unavailable. This has been seen on machines with AMD
graphics, where the HDMI port lives on a separate ALSA card that only
has that port.

Sounds good, but have you checked that the profile gets restored back
to something else than off once you plug the device back in? At least
at some point this was an issue, and I think it hasn't been fixed. If
this is still an issue, module-switch-on-port-available needs to be
fixed first to do the profile switch away from off when something gets
plugged in.

Yes, once something is plugged again on the card its profile is
updated to reflect the new port availability.

I'm going to re-send this as a MR for inclusion.

--
João Paulo Rechi Vita
http://about.me/jprvita

[João Paulo Rechi Vita]

On Wed, Nov 21, 2018 at 11:44 AM João Paulo Rechi Vita

Post by João Paulo Rechi Vita

Post by Tanu Kaskinen

Post by João Paulo Rechi Vita
When the active profile of a card becomes unavailable and no other
module changes it to a better profile (i.e. there are no available ports
that module-switch-on-port-available could switch to) the card will be
stuck on an unavailable profile with a non-working sink/source and any
active streams connected to that sink/source will remain connected.
This commit switches to a different profile when the active profile
becomes unavailble, looking for a profile with availability yes or
unknown with the highest priority, and ultimately fall-backing to the
OFF profile.
With this fix a card that only has one port can have the streams
connected to its sink/source moved away by module-rescue-stream when
that port becomes unavailable. This has been seen on machines with AMD
graphics, where the HDMI port lives on a separate ALSA card that only
has that port.

Sounds good, but have you checked that the profile gets restored back
to something else than off once you plug the device back in? At least
at some point this was an issue, and I think it hasn't been fixed. If
this is still an issue, module-switch-on-port-available needs to be
fixed first to do the profile switch away from off when something gets
plugged in.

Yes, once something is plugged again on the card its profile is
updated to reflect the new port availability.
I'm going to re-send this as a MR for inclusion.

Now on https://gitlab.freedesktop.org/pulseaudio/pulseaudio/merge_requests/34,
in case anyone else is interested.

--
João Paulo Rechi Vita
http://about.me/jprvita

[João Paulo Rechi Vita]

Prefer the active profile only if it has ports with available status YES
on the same direction of the port we are trying to switch to and a
higher priority than whichever profile we would select to switch to a
new port.

This patch removes the code that ensures that no profile switching will
happen when a new port is available if the currently active port's
available state is YES or UNKNOWN, for example when you have the
computer permanently plugged to a HDMI device with audio capabilities
and plug something on the headphones port.

In place of that code it adds new code to prefer the currently active
profile if it has at least one port with available state YES on the same
direction of the new available port and a higher priority than the
profile that would be selected to switch to this new port.

This was built uppon previous work by Mario Sanchez Prada.
---
src/modules/module-switch-on-port-available.c | 32 +++++++------------
1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/src/modules/module-switch-on-port-available.c b/src/modules/module-switch-on-port-available.c
index 385c2f693..d02100c58 100644
--- a/src/modules/module-switch-on-port-available.c
+++ b/src/modules/module-switch-on-port-available.c
@@ -64,8 +64,6 @@ static void card_info_free(struct card_info *info) {

static bool profile_good_for_output(pa_card_profile *profile, pa_device_port *port) {
pa_card *card;
- pa_sink *sink;
- uint32_t idx;

pa_assert(profile);

@@ -83,21 +81,11 @@ static bool profile_good_for_output(pa_card_profile *profile, pa_device_port *po
if (port == card->preferred_output_port)
return true;

- PA_IDXSET_FOREACH(sink, card->sinks, idx) {
- if (!sink->active_port)
- continue;
-
- if ((sink->active_port->available != PA_AVAILABLE_NO) && (sink->active_port->priority >= port->priority))
- return false;
- }
-
return true;
}

static bool profile_good_for_input(pa_card_profile *profile, pa_device_port *port) {
pa_card *card;
- pa_source *source;
- uint32_t idx;

pa_assert(profile);

@@ -115,14 +103,6 @@ static bool profile_good_for_input(pa_card_profile *profile, pa_device_port *por
if (port == card->preferred_input_port)
return true;

- PA_IDXSET_FOREACH(source, card->sources, idx) {
- if (!source->active_port)
- continue;
-
- if ((source->active_port->available != PA_AVAILABLE_NO) && (source->active_port->priority >= port->priority))
- return false;
- }
-
return true;
}

@@ -134,6 +114,13 @@ static int try_to_switch_profile(pa_device_port *port) {
pa_log_debug("Finding best profile for port %s, preferred = %s",
port->name, pa_strnull(port->preferred_profile));

+ /* Prefer the current active profile if it has available ports on the same direction
+ * of the port we are trying to switch to and a higher priority */
+ if (pa_card_profile_has_available_ports(port->card->active_profile, port->direction, PA_AVAILABLE_YES)) {
+ best_profile = port->card->active_profile;
+ best_prio = port->card->active_profile->priority;
+ }
+
PA_HASHMAP_FOREACH(profile, port->profiles, state) {
bool good = false;
const char *name;
@@ -171,6 +158,11 @@ static int try_to_switch_profile(pa_device_port *port) {
return -1;
}

+ if (best_profile == port->card->active_profile) {
+ pa_log_debug("No better profile found");
+ return -1;
+ }
+
if (pa_card_set_profile(port->card, best_profile, false) != 0) {
pa_log_debug("Could not set profile %s", best_profile->name);
return -1;

[Tanu Kaskinen]

Post by João Paulo Rechi Vita
Prefer the active profile only if it has ports with available status YES
on the same direction of the port we are trying to switch to and a
higher priority than whichever profile we would select to switch to a
new port.

I don't really understand this logic. Not all ports with status YES on
a profile are necessarily active, and I don't see why inactive ports
should prevent switching away from the current profile.

Post by João Paulo Rechi Vita
This patch removes the code that ensures that no profile switching will
happen when a new port is available if the currently active port's
available state is YES or UNKNOWN, for example when you have the
computer permanently plugged to a HDMI device with audio capabilities
and plug something on the headphones port.

So you want to switch to the headphones in that case? That's good, but
what if I have headphones plugged in and I plug in a HDMI monitor that
I don't want to use for sound? This patch breaks that use case.

I'm not sure what we should do. Maybe add some HDMI specific code, so
that when HDMI is plugged in, we switch to it only if the HDMI port is
the card's preferred port (which means that the user has previously
manually chosen the HDMI output).

Post by João Paulo Rechi Vita
In place of that code it adds new code to prefer the currently active
profile if it has at least one port with available state YES on the same
direction of the new available port and a higher priority than the
profile that would be selected to switch to this new port.
This was built uppon previous work by Mario Sanchez Prada.
---
src/modules/module-switch-on-port-available.c | 32 +++++++------------
1 file changed, 12 insertions(+), 20 deletions(-)
diff --git a/src/modules/module-switch-on-port-available.c b/src/modules/module-switch-on-port-available.c
index 385c2f693..d02100c58 100644
--- a/src/modules/module-switch-on-port-available.c
+++ b/src/modules/module-switch-on-port-available.c
@@ -64,8 +64,6 @@ static void card_info_free(struct card_info *info) {
static bool profile_good_for_output(pa_card_profile *profile, pa_device_port *port) {
pa_card *card;
- pa_sink *sink;
- uint32_t idx;
pa_assert(profile);
@@ -83,21 +81,11 @@ static bool profile_good_for_output(pa_card_profile *profile, pa_device_port *po
if (port == card->preferred_output_port)
return true;
- PA_IDXSET_FOREACH(sink, card->sinks, idx) {
- if (!sink->active_port)
- continue;
-
- if ((sink->active_port->available != PA_AVAILABLE_NO) && (sink->active_port->priority >= port->priority))
- return false;
- }
-
return true;
}
static bool profile_good_for_input(pa_card_profile *profile, pa_device_port *port) {
pa_card *card;
- pa_source *source;
- uint32_t idx;
pa_assert(profile);
@@ -115,14 +103,6 @@ static bool profile_good_for_input(pa_card_profile *profile, pa_device_port *por
if (port == card->preferred_input_port)
return true;
- PA_IDXSET_FOREACH(source, card->sources, idx) {
- if (!source->active_port)
- continue;
-
- if ((source->active_port->available != PA_AVAILABLE_NO) && (source->active_port->priority >= port->priority))
- return false;
- }
-
return true;
}
@@ -134,6 +114,13 @@ static int try_to_switch_profile(pa_device_port *port) {
pa_log_debug("Finding best profile for port %s, preferred = %s",
port->name, pa_strnull(port->preferred_profile));
+ /* Prefer the current active profile if it has available ports on the same direction
+ * of the port we are trying to switch to and a higher priority */
+ if (pa_card_profile_has_available_ports(port->card->active_profile, port->direction, PA_AVAILABLE_YES)) {
+ best_profile = port->card->active_profile;
+ best_prio = port->card->active_profile->priority;
+ }
+
PA_HASHMAP_FOREACH(profile, port->profiles, state) {
bool good = false;
const char *name;
@@ -171,6 +158,11 @@ static int try_to_switch_profile(pa_device_port *port) {
return -1;
}
+ if (best_profile == port->card->active_profile) {
+ pa_log_debug("No better profile found");
+ return -1;
+ }
+
if (pa_card_set_profile(port->card, best_profile, false) != 0) {
pa_log_debug("Could not set profile %s", best_profile->name);
return -1;

[João Paulo Rechi Vita]

Post by Tanu Kaskinen

Post by João Paulo Rechi Vita
Prefer the active profile only if it has ports with available status YES
on the same direction of the port we are trying to switch to and a
higher priority than whichever profile we would select to switch to a
new port.

I don't really understand this logic. Not all ports with status YES on
a profile are necessarily active, and I don't see why inactive ports
should prevent switching away from the current profile.

Previously if there was an active port with higher priority than the
port we were trying to switch to and availability YES or UNKNOWN, we
would avoid switching away from the active port to the new port. The
idea is to reduce the preference given to the active port to only when
it has higher priority than the port we were trying to switch to and
availability YES only. The rationale is that since a port with
availability UNKNOWN might not really be available, we shouldn't avoid
the port switching in that case.

Post by Tanu Kaskinen

Post by João Paulo Rechi Vita
This patch removes the code that ensures that no profile switching will
happen when a new port is available if the currently active port's
available state is YES or UNKNOWN, for example when you have the
computer permanently plugged to a HDMI device with audio capabilities
and plug something on the headphones port.

So you want to switch to the headphones in that case? That's good, but
what if I have headphones plugged in and I plug in a HDMI monitor that
I don't want to use for sound? This patch breaks that use case.
I'm not sure what we should do. Maybe add some HDMI specific code, so
that when HDMI is plugged in, we switch to it only if the HDMI port is
the card's preferred port (which means that the user has previously
manually chosen the HDMI output).

My idea is that unless a port has been manually selected by the user
at some point in time, it should always follow the priorities defined
in mixer paths files. In both cases you described headphones should
win since the have the higher priority. Notice that the change does
not blindly choose the active profile if it is active and has a YES
port, it simply adds it to the list of profiles that would be
considered, but it still has to have the higher priority by the end of
the loop (which gives a bonus to preferred profile).

So unless I'm missing something or mixing up concepts, despite the
order of connection, if you connect both HDMI and headphones, the
headphones will always be preferred because the have higher priority
(assuming no manual choices were made at any point).

This is consistent with the results I got when testing this, but at
this point is important to say we actually change the ports priorities
downstream to give external devices a higher priority:
https://github.com/endlessm/pulseaudio/commit/40d00e72f74b2177ebead548427debadd6d1c05e.
I believe it should still work the same way for the HDMI+HP case we
are discussing, but I'm now not so sure if we will end up with
Speakers always winning with the upstream priorities. Actually,
probably this commit should have been left for a separate series,
together with the one pointed by the link above. I'll re-organize
things a bit and re-send -- should RFCs continue to be sent to the
list, or should they also be submitted though a gitlab MR?

Thanks for the feedback!

--
João Paulo Rechi Vita
http://about.me/jprvita

[Tanu Kaskinen]

Post by João Paulo Rechi Vita

Post by Tanu Kaskinen

Post by João Paulo Rechi Vita
Prefer the active profile only if it has ports with available status YES
on the same direction of the port we are trying to switch to and a
higher priority than whichever profile we would select to switch to a
new port.

I don't really understand this logic. Not all ports with status YES on
a profile are necessarily active, and I don't see why inactive ports
should prevent switching away from the current profile.

Previously if there was an active port with higher priority than the
port we were trying to switch to and availability YES or UNKNOWN, we
would avoid switching away from the active port to the new port. The
idea is to reduce the preference given to the active port to only when
it has higher priority than the port we were trying to switch to and
availability YES only. The rationale is that since a port with
availability UNKNOWN might not really be available, we shouldn't avoid
the port switching in that case.

I probably didn't make my point clear enough. The patch considers also
ports that are *not active* if they are part of the active profile.
This is the thing that doesn't make sense to me. To me it seems that
only active ports should be considered.

As for ignoring currently active ports with UNKNOWN status - that
doesn't seem like a good idea either. If the port is active, it's very
likely that it's also available, because otherwise the user would have
switched to something else. Is there some real use case where treating
UNKNOWN and YES statuses as the same has caused problems?

Post by João Paulo Rechi Vita

Post by Tanu Kaskinen

Post by João Paulo Rechi Vita
This patch removes the code that ensures that no profile switching will
happen when a new port is available if the currently active port's
available state is YES or UNKNOWN, for example when you have the
computer permanently plugged to a HDMI device with audio capabilities
and plug something on the headphones port.

So you want to switch to the headphones in that case? That's good, but
what if I have headphones plugged in and I plug in a HDMI monitor that
I don't want to use for sound? This patch breaks that use case.
I'm not sure what we should do. Maybe add some HDMI specific code, so
that when HDMI is plugged in, we switch to it only if the HDMI port is
the card's preferred port (which means that the user has previously
manually chosen the HDMI output).

My idea is that unless a port has been manually selected by the user
at some point in time, it should always follow the priorities defined
in mixer paths files. In both cases you described headphones should
win since the have the higher priority. Notice that the change does
not blindly choose the active profile if it is active and has a YES
port, it simply adds it to the list of profiles that would be
considered, but it still has to have the higher priority by the end of
the loop (which gives a bonus to preferred profile).
So unless I'm missing something or mixing up concepts, despite the
order of connection, if you connect both HDMI and headphones, the
headphones will always be preferred because the have higher priority
(assuming no manual choices were made at any point).

My example may have been bad, because I didn't consider that the active
profile with the headphone port may have higher priority than any of
the potential HDMI profiles. However, we shouldn't be comparing profile
priorities anyway when deciding between headphones and HDMI, we should
compare the port priorities. Your patch removed the only place where
port priorities are compared.

A better example about the special nature of HDMI would be S/PDIF
instead of headphones. S/PDIF has lower priority than HDMI, but we
still shouldn't switch from S/PDIF to HDMI automatically if the user
hasn't chosen HDMI before. In my opinion we should never switch to HDMI
automatically before the user has indicated that they want to use HDMI
for audio. We don't know if the HDMI monitor has audio capabilities, or
it may only have headphone output. If we switch to it automatically, in
some cases it will look like audio stopped working altogether, which is
pretty bad.

Regarding your example of an HDMI monitor being used while headphones
are plugged in: I agree that switching to headphones makes sense, but
now that I read the patch again, it seems that this case should have
been working already. The patch removed this code from
profile_good_for_output():

if ((sink->active_port->available != PA_AVAILABLE_NO) && (sink->active_port->priority >= port->priority))
return false;

That code doesn't prevent the switch to headphones, because the
headphone port has higher priority than the HDMI port.

Post by João Paulo Rechi Vita
This is consistent with the results I got when testing this, but at
this point is important to say we actually change the ports priorities
https://github.com/endlessm/pulseaudio/commit/40d00e72f74b2177ebead548427debadd6d1c05e.
I believe it should still work the same way for the HDMI+HP case we
are discussing, but I'm now not so sure if we will end up with
Speakers always winning with the upstream priorities.

Yes, the port priorities are not set up as good as they should.
Speakers shouldn't be the highest-priority port. That problem is
mitigated to a large extent by marking speakers unavailable when
headphones or lineout is plugged in, but that's not really ideal.

The priority order of ports should actually depend on whether they have
jack detection support or not. Speakers should have higher priority
than external ports without jack detection, but lower priority than
external ports with jack detection.

Post by João Paulo Rechi Vita
Actually,
probably this commit should have been left for a separate series,
together with the one pointed by the link above. I'll re-organize
things a bit and re-send -- should RFCs continue to be sent to the
list, or should they also be submitted though a gitlab MR?

I guess the mailing list makes sense if you hope to get comments from a
larger audience. If you just want comments from the maintainers, then
MRs are better. Either is fine for me.

[João Paulo Rechi Vita]

Hello Tanu! Sorry it took me so long to get back on this issue, but I
was busy with some urgent things lately. Please find my reply in-line
below.

Post by Tanu Kaskinen

Post by João Paulo Rechi Vita

Post by Tanu Kaskinen

Post by João Paulo Rechi Vita
Prefer the active profile only if it has ports with available status YES
on the same direction of the port we are trying to switch to and a
higher priority than whichever profile we would select to switch to a
new port.

I don't really understand this logic. Not all ports with status YES on
a profile are necessarily active, and I don't see why inactive ports
should prevent switching away from the current profile.

Previously if there was an active port with higher priority than the
port we were trying to switch to and availability YES or UNKNOWN, we
would avoid switching away from the active port to the new port. The
idea is to reduce the preference given to the active port to only when
it has higher priority than the port we were trying to switch to and
availability YES only. The rationale is that since a port with
availability UNKNOWN might not really be available, we shouldn't avoid
the port switching in that case.

I probably didn't make my point clear enough. The patch considers also
ports that are *not active* if they are part of the active profile.
This is the thing that doesn't make sense to me. To me it seems that
only active ports should be considered.

It looks like I got a bit confused on my previous reply to this -- I
agree with your point here.

Post by Tanu Kaskinen
As for ignoring currently active ports with UNKNOWN status - that
doesn't seem like a good idea either. If the port is active, it's very
likely that it's also available, because otherwise the user would have
switched to something else. Is there some real use case where treating
UNKNOWN and YES statuses as the same has caused problems?

IIRC this was to solve a problem on a machine without internal
speakers and only headphones and HDMI ports, where HDMI availability
was always UNKNOWN despite the cable being connected or not. Since it
was the higher priority port with availability != NO it would be
selected when no headphones were connected. Later when connecting
headphones the audio would not be routed to headphones since the
currently active port had availability UNKNOWN.

I can't reproduce it anymore though, and this commit has been carried
over for a couple of years now, so we can probably ignore it. I'll
drop it downstream and if we find any problems I'll come back with a
better informed proposal :)

Post by Tanu Kaskinen

Post by João Paulo Rechi Vita

Post by Tanu Kaskinen

Post by João Paulo Rechi Vita
This patch removes the code that ensures that no profile switching will
happen when a new port is available if the currently active port's
available state is YES or UNKNOWN, for example when you have the
computer permanently plugged to a HDMI device with audio capabilities
and plug something on the headphones port.

So you want to switch to the headphones in that case? That's good, but
what if I have headphones plugged in and I plug in a HDMI monitor that
I don't want to use for sound? This patch breaks that use case.
I'm not sure what we should do. Maybe add some HDMI specific code, so
that when HDMI is plugged in, we switch to it only if the HDMI port is
the card's preferred port (which means that the user has previously
manually chosen the HDMI output).

My idea is that unless a port has been manually selected by the user
at some point in time, it should always follow the priorities defined
in mixer paths files. In both cases you described headphones should
win since the have the higher priority. Notice that the change does
not blindly choose the active profile if it is active and has a YES
port, it simply adds it to the list of profiles that would be
considered, but it still has to have the higher priority by the end of
the loop (which gives a bonus to preferred profile).
So unless I'm missing something or mixing up concepts, despite the
order of connection, if you connect both HDMI and headphones, the
headphones will always be preferred because the have higher priority
(assuming no manual choices were made at any point).

My example may have been bad, because I didn't consider that the active
profile with the headphone port may have higher priority than any of
the potential HDMI profiles. However, we shouldn't be comparing profile
priorities anyway when deciding between headphones and HDMI, we should
compare the port priorities. Your patch removed the only place where
port priorities are compared.
A better example about the special nature of HDMI would be S/PDIF
instead of headphones. S/PDIF has lower priority than HDMI, but we
still shouldn't switch from S/PDIF to HDMI automatically if the user
hasn't chosen HDMI before. In my opinion we should never switch to HDMI
automatically before the user has indicated that they want to use HDMI
for audio. We don't know if the HDMI monitor has audio capabilities, or
it may only have headphone output. If we switch to it automatically, in
some cases it will look like audio stopped working altogether, which is
pretty bad.
Regarding your example of an HDMI monitor being used while headphones
are plugged in: I agree that switching to headphones makes sense, but
now that I read the patch again, it seems that this case should have
been working already. The patch removed this code from
if ((sink->active_port->available != PA_AVAILABLE_NO) && (sink->active_port->priority >= port->priority))
return false;
That code doesn't prevent the switch to headphones, because the
headphone port has higher priority than the HDMI port.

Yes, I guess this bit has changed since then. This was part of a big
downstream refactor and an attempt to upstream as much of our fixes as
possible, and I let this slip through.

Post by Tanu Kaskinen

Post by João Paulo Rechi Vita
This is consistent with the results I got when testing this, but at
this point is important to say we actually change the ports priorities
https://github.com/endlessm/pulseaudio/commit/40d00e72f74b2177ebead548427debadd6d1c05e.
I believe it should still work the same way for the HDMI+HP case we
are discussing, but I'm now not so sure if we will end up with
Speakers always winning with the upstream priorities.

Yes, the port priorities are not set up as good as they should.
Speakers shouldn't be the highest-priority port. That problem is
mitigated to a large extent by marking speakers unavailable when
headphones or lineout is plugged in, but that's not really ideal.
The priority order of ports should actually depend on whether they have
jack detection support or not. Speakers should have higher priority
than external ports without jack detection, but lower priority than
external ports with jack detection.

Is there a way to specify that in the paths files?

Post by Tanu Kaskinen

Post by João Paulo Rechi Vita
Actually,
probably this commit should have been left for a separate series,
together with the one pointed by the link above. I'll re-organize
things a bit and re-send -- should RFCs continue to be sent to the
list, or should they also be submitted though a gitlab MR?

I guess the mailing list makes sense if you hope to get comments from a
larger audience. If you just want comments from the maintainers, then
MRs are better. Either is fine for me.

Thanks for the throughout review, and sorry for wasting your time with
an unnecessary patch.

--
João Paulo Rechi Vita
http://about.me/jprvita

[Tanu Kaskinen]

Post by João Paulo Rechi Vita
Hello Tanu! Sorry it took me so long to get back on this issue, but I
was busy with some urgent things lately. Please find my reply in-line
below.

No problem!

Post by João Paulo Rechi Vita

Post by Tanu Kaskinen

Post by João Paulo Rechi Vita
This is consistent with the results I got when testing this, but at
this point is important to say we actually change the ports priorities
https://github.com/endlessm/pulseaudio/commit/40d00e72f74b2177ebead548427debadd6d1c05e.
I believe it should still work the same way for the HDMI+HP case we
are discussing, but I'm now not so sure if we will end up with
Speakers always winning with the upstream priorities.

Yes, the port priorities are not set up as good as they should.
Speakers shouldn't be the highest-priority port. That problem is
mitigated to a large extent by marking speakers unavailable when
headphones or lineout is plugged in, but that's not really ideal.
The priority order of ports should actually depend on whether they have
jack detection support or not. Speakers should have higher priority
than external ports without jack detection, but lower priority than
external ports with jack detection.

Is there a way to specify that in the paths files?

No, otherwise this probably would have been fixed already :)

I'm not sure how the fix should be implemented. Maybe the simplest fix
would be to have two priority values for each path. One would be used
when the path has jack detection support and the other when the path
doesn't support jack detection. But perhaps it would be better to not
have very complex policy rules embedded in the path configuration,
because policy decisions don't really belong in the hardware
description.

I'd like to have a lookup table for mapping an output device type to
its priority (and of course another table for input device types).
Currently it's very cumbersome to try to get an overview of how the
different alsa paths relate to each other in terms of priority, and
that's only alsa, trying to figure out how other outputs rank is even
harder. The table that I envision would be generic (not specific to
alsa or any other backend), and as fine-grained as necessary. There
would be entries not only for simple stuff like "headphones",
"speakers" and "hdmi", but distinction would be made between outputs
that support or don't support jack detection, integrated or external
speakers etc., whatever is relevant for building a complete ranking of
all different kinds of outputs that we support (and can distinguish).
The backend (e.g. alsa) would either directly set the device type, or
provide a generic metadata structure that a generic function could use
to map the metadata into the device type identifier (the latter option
seems better, because it probably reduces duplication of work between
the backends, and the metadata could be useful for policy modules).

Making that table reconfigurable by users or policy modules would be
desirable, but the first step would be to just have such table.

[João Paulo Rechi Vita]

HDMI ports are normally present on cards connected to an internal bus,
and module-switch-on-connect should switch to them when a HDMI monitor
is plugged.

This is specially relevant on setups where the HDMI port of a machine is
connected to a different audio card then the analog outputs, which is
the case for machines with AMD graphics cards.
---
src/modules/module-switch-on-connect.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/src/modules/module-switch-on-connect.c b/src/modules/module-switch-on-connect.c
index ebdd6aad0..9077eaec4 100644
--- a/src/modules/module-switch-on-connect.c
+++ b/src/modules/module-switch-on-connect.c
@@ -70,11 +70,14 @@ static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, void*

pa_log_debug("Trying to switch to new sink %s", sink->name);

- /* Don't switch to any internal devices */
- s = pa_proplist_gets(sink->proplist, PA_PROP_DEVICE_BUS);
- if (pa_safe_streq(s, "pci") || pa_safe_streq(s, "isa")) {
- pa_log_debug("Refusing to switch to sink on %s bus", s);
- return PA_HOOK_OK;
+ /* Don't switch to any internal devices except HDMI */
+ s = pa_proplist_gets(sink->proplist, PA_PROP_DEVICE_STRING);
+ if (s && !pa_startswith(s, "hdmi")) {
+ s = pa_proplist_gets(sink->proplist, PA_PROP_DEVICE_BUS);
+ if (pa_safe_streq(s, "pci") || pa_safe_streq(s, "isa")) {
+ pa_log_debug("Refusing to switch to sink on %s bus", s);
+ return PA_HOOK_OK;
+ }
}

/* Ignore virtual sinks if not configured otherwise on the command line */

[Tanu Kaskinen]

Post by João Paulo Rechi Vita
HDMI ports are normally present on cards connected to an internal bus,
and module-switch-on-connect should switch to them when a HDMI monitor
is plugged.
This is specially relevant on setups where the HDMI port of a machine is
connected to a different audio card then the analog outputs, which is
the case for machines with AMD graphics cards.

As I mentioned in an earlier mail, it's not generally a good idea to
switch to HDMI automatically, due to the possible lack of speakers on
the monitor, so do you really want to do this chage? I'm fine with this
patch, though, because if the decision to switch to a HDMI profile has
already been made, it's pretty logical to switch output to it in
module-switch-on-connect.

[João Paulo Rechi Vita]

Post by Tanu Kaskinen

Post by João Paulo Rechi Vita
HDMI ports are normally present on cards connected to an internal bus,
and module-switch-on-connect should switch to them when a HDMI monitor
is plugged.
This is specially relevant on setups where the HDMI port of a machine is
connected to a different audio card then the analog outputs, which is
the case for machines with AMD graphics cards.

As I mentioned in an earlier mail, it's not generally a good idea to
switch to HDMI automatically, due to the possible lack of speakers on
the monitor, so do you really want to do this chage? I'm fine with this
patch, though, because if the decision to switch to a HDMI profile has
already been made, it's pretty logical to switch output to it in
module-switch-on-connect.

I see your point and I understand the implications. We recently
checked the behavior on Windows and it always switches the audio to
HDMI no matter what (even with headphones connected), so that tends to
be the expectation from OEMs. We may change our mind at some point,
but for now we are sticking with that policy downstream. Ideally all
the changes we are making to support this case will be generic enough
that they make sense upstream as well and we only have a small
downstream patch set changing the ports / profiles priorities.

As you mentioned, when this code runs the decision to switch the card
profile to HDMI has already been made. But on setups where one card
has the analog ports and another card has only HDMI ports, the HDMI
card will always switch to a HDMI profile when HDMI is connected, and
(unless I'm missing something) the streams will be moved over to the
HDMI sink.

Let me know if the above comment makes sense, and if you still want to
have this patch upstream, in which case I can re-send it as a MR.

Thanks,

--
João Paulo Rechi Vita
http://about.me/jprvita

[Tanu Kaskinen]

Post by João Paulo Rechi Vita

Post by Tanu Kaskinen

Post by João Paulo Rechi Vita
HDMI ports are normally present on cards connected to an internal bus,
and module-switch-on-connect should switch to them when a HDMI monitor
is plugged.
This is specially relevant on setups where the HDMI port of a machine is
connected to a different audio card then the analog outputs, which is
the case for machines with AMD graphics cards.

As I mentioned in an earlier mail, it's not generally a good idea to
switch to HDMI automatically, due to the possible lack of speakers on
the monitor, so do you really want to do this chage? I'm fine with this
patch, though, because if the decision to switch to a HDMI profile has
already been made, it's pretty logical to switch output to it in
module-switch-on-connect.

I see your point and I understand the implications. We recently
checked the behavior on Windows and it always switches the audio to
HDMI no matter what (even with headphones connected), so that tends to
be the expectation from OEMs. We may change our mind at some point,
but for now we are sticking with that policy downstream. Ideally all
the changes we are making to support this case will be generic enough
that they make sense upstream as well and we only have a small
downstream patch set changing the ports / profiles priorities.
As you mentioned, when this code runs the decision to switch the card
profile to HDMI has already been made. But on setups where one card
has the analog ports and another card has only HDMI ports, the HDMI
card will always switch to a HDMI profile when HDMI is connected, and
(unless I'm missing something) the streams will be moved over to the
HDMI sink.
Let me know if the above comment makes sense, and if you still want to
have this patch upstream, in which case I can re-send it as a MR.

No need to re-send, I pushed this now :)

[João Paulo Rechi Vita]

Post by Tanu Kaskinen

Post by João Paulo Rechi Vita

Post by Tanu Kaskinen

Post by João Paulo Rechi Vita
HDMI ports are normally present on cards connected to an internal bus,
and module-switch-on-connect should switch to them when a HDMI monitor
is plugged.
This is specially relevant on setups where the HDMI port of a machine is
connected to a different audio card then the analog outputs, which is
the case for machines with AMD graphics cards.

As I mentioned in an earlier mail, it's not generally a good idea to
switch to HDMI automatically, due to the possible lack of speakers on
the monitor, so do you really want to do this chage? I'm fine with this
patch, though, because if the decision to switch to a HDMI profile has
already been made, it's pretty logical to switch output to it in
module-switch-on-connect.

I see your point and I understand the implications. We recently
checked the behavior on Windows and it always switches the audio to
HDMI no matter what (even with headphones connected), so that tends to
be the expectation from OEMs. We may change our mind at some point,
but for now we are sticking with that policy downstream. Ideally all
the changes we are making to support this case will be generic enough
that they make sense upstream as well and we only have a small
downstream patch set changing the ports / profiles priorities.
As you mentioned, when this code runs the decision to switch the card
profile to HDMI has already been made. But on setups where one card
has the analog ports and another card has only HDMI ports, the HDMI
card will always switch to a HDMI profile when HDMI is connected, and
(unless I'm missing something) the streams will be moved over to the
HDMI sink.
Let me know if the above comment makes sense, and if you still want to
have this patch upstream, in which case I can re-send it as a MR.

No need to re-send, I pushed this now :)

Thanks!

--
João Paulo Rechi Vita
http://about.me/jprvita