[pulseaudio-discuss] [PATCH] Don't settle on a card profile if it doesn't have any available port
David Henningsson
david.henningsson at canonical.com
Mon Mar 30 05:36:18 PDT 2015
On 2015-03-20 17:32, Mario Sanchez Prada wrote:
> When creating a new card and selecting a new profile we need to check
> whether there's a port available for that profile before settling on it,
> or we might end up with a useless profile when we might have others that
> would be a better choice.
>
> In order to achieve this, we need to make sure that port availability
> based on the state of the actual alsa jacks for a path is properly
> initialized before creating the card, so that we can make the right
> decision once its created, by checking the card's port availability.
>
> This patch avoids scenarios like starting with the analog-stereo profile
> selected when there are neither built-in speakers nor an external
> headset connected, even when the device is connected to an external
> screen with audio capabilities through HDMI (so it could use the
> available hdmi-output port from the the hdmi-stereo profile).
In general, this looks good. See some review comments below.
>
> https://bugs.freedesktop.org/show_bug.cgi?id=87002
> ---
> src/modules/alsa/alsa-mixer.c | 4 ++
> src/modules/alsa/alsa-util.c | 52 +++++++++++++++++++++++
> src/modules/alsa/alsa-util.h | 2 +
> src/modules/alsa/module-alsa-card.c | 60 ++++-----------------------
> src/modules/module-switch-on-port-available.c | 59 ++++++++++++++++++++++++++
> 5 files changed, 125 insertions(+), 52 deletions(-)
>
> diff --git a/src/modules/alsa/alsa-mixer.c b/src/modules/alsa/alsa-mixer.c
> index 2fe2ae4..75c0f34 100644
> --- a/src/modules/alsa/alsa-mixer.c
> +++ b/src/modules/alsa/alsa-mixer.c
> @@ -4636,12 +4636,16 @@ static pa_device_port* device_port_alsa_init(pa_hashmap *ports, /* card ports */
> if (!p) {
> pa_alsa_port_data *data;
> pa_device_port_new_data port_data;
> + pa_available_t pa = PA_AVAILABLE_UNKNOWN;
Unnecessary assignment - pa is always set further down.
> pa_device_port_new_data_init(&port_data);
> pa_device_port_new_data_set_name(&port_data, name);
> pa_device_port_new_data_set_description(&port_data, description);
> pa_device_port_new_data_set_direction(&port_data, path->direction == PA_ALSA_DIRECTION_OUTPUT ? PA_DIRECTION_OUTPUT : PA_DIRECTION_INPUT);
>
> + pa = pa_alsa_availability_from_jacks(path->jacks, NULL, NULL);
> + pa_device_port_new_data_set_available(&port_data, pa);
> +
> p = pa_device_port_new(core, &port_data, sizeof(pa_alsa_port_data));
> pa_device_port_new_data_done(&port_data);
> pa_assert(p);
> diff --git a/src/modules/alsa/alsa-util.c b/src/modules/alsa/alsa-util.c
> index bb79e71..8ac1ef3 100644
> --- a/src/modules/alsa/alsa-util.c
> +++ b/src/modules/alsa/alsa-util.c
> @@ -1702,3 +1702,55 @@ int pa_alsa_get_hdmi_eld(snd_hctl_elem_t *elem, pa_hdmi_eld *eld) {
>
> return 0;
> }
> +
> +pa_available_t pa_alsa_availability_from_jacks(pa_alsa_jack *jacks_llist, pa_device_port *port, pa_card *card) {
> + pa_alsa_jack *jack;
> + pa_available_t pa = PA_AVAILABLE_UNKNOWN;
> + pa_device_port *p;
> +
> + PA_LLIST_FOREACH(jack, jacks_llist) {
> + pa_available_t cpa;
> +
> + /* If a port has NOT been provided, it means we will trust the list
> + * of jacks passed to be the ones we are interested in checking, so
> + * get the actual port from the jack only when a port has been passed. */
> + if (port) {
This is a refactoring from u->use_ucm to "if (card)". It works but I
found it confusing at first. It's because ucm jacks don't have paths, so
we have to go through the card instead.
Maybe we could comment this a bit better, like putting this above the
function:
/*
* port - can be null, in which case all jacks in the list will be
* checked.
* card - null for non-UCM scenarios, where we can use the
* jack->path->port pointer. Set this to the right card only for UCM
* jacks.
*/
> + if (card)
> + p = pa_hashmap_get(card->ports, jack->name);
> + else {
> + if (jack->path)
> + p = jack->path->port;
> + else
> + continue;
> + }
> +
> + if (p != port)
> + continue;
> + }
> +
> + cpa = jack->plugged_in ? jack->state_plugged : jack->state_unplugged;
> +
> + if (cpa == PA_AVAILABLE_NO) {
Nitpick: this is probably not your fault, but indentation of this
function is inconsistent.
> + /* If a plugged-in jack causes the availability to go to NO, it
> + * should override all other availability information (like a
> + * blacklist) so set and bail */
> + if (jack->plugged_in) {
> + pa = cpa;
> + break;
> + }
> +
> + /* If the current availablility is unknown go the more precise no,
> + * but otherwise don't change state */
> + if (pa == PA_AVAILABLE_UNKNOWN)
> + pa = cpa;
> + } else if (cpa == PA_AVAILABLE_YES) {
> + /* Output is available through at least one jack, so go to that
> + * level of availability. We still need to continue iterating through
> + * the jacks in case a jack is plugged in that forces the state to no
> + */
> + pa = cpa;
> + }
> + }
> +
> + return pa;
> +}
> diff --git a/src/modules/alsa/alsa-util.h b/src/modules/alsa/alsa-util.h
> index 8345a0b..dc4b210 100644
> --- a/src/modules/alsa/alsa-util.h
> +++ b/src/modules/alsa/alsa-util.h
> @@ -151,4 +151,6 @@ struct pa_hdmi_eld {
>
> int pa_alsa_get_hdmi_eld(snd_hctl_elem_t *elem, pa_hdmi_eld *eld);
>
> +pa_available_t pa_alsa_availability_from_jacks(pa_alsa_jack *jacks_llist, pa_device_port *port, pa_card *card);
> +
> #endif
> diff --git a/src/modules/alsa/module-alsa-card.c b/src/modules/alsa/module-alsa-card.c
> index a7fec04..4032f3b 100644
> --- a/src/modules/alsa/module-alsa-card.c
> +++ b/src/modules/alsa/module-alsa-card.c
> @@ -310,54 +310,6 @@ static void init_profile(struct userdata *u) {
> am->source = pa_alsa_source_new(u->module, u->modargs, __FILE__, u->card, am);
> }
>
> -static void report_port_state(pa_device_port *p, struct userdata *u) {
> - void *state;
> - pa_alsa_jack *jack;
> - pa_available_t pa = PA_AVAILABLE_UNKNOWN;
> - pa_device_port *port;
> -
> - PA_HASHMAP_FOREACH(jack, u->jacks, state) {
> - pa_available_t cpa;
> -
> - if (u->use_ucm)
> - port = pa_hashmap_get(u->card->ports, jack->name);
> - else {
> - if (jack->path)
> - port = jack->path->port;
> - else
> - continue;
> - }
> -
> - if (p != port)
> - continue;
> -
> - cpa = jack->plugged_in ? jack->state_plugged : jack->state_unplugged;
> -
> - if (cpa == PA_AVAILABLE_NO) {
> - /* If a plugged-in jack causes the availability to go to NO, it
> - * should override all other availability information (like a
> - * blacklist) so set and bail */
> - if (jack->plugged_in) {
> - pa = cpa;
> - break;
> - }
> -
> - /* If the current availablility is unknown go the more precise no,
> - * but otherwise don't change state */
> - if (pa == PA_AVAILABLE_UNKNOWN)
> - pa = cpa;
> - } else if (cpa == PA_AVAILABLE_YES) {
> - /* Output is available through at least one jack, so go to that
> - * level of availability. We still need to continue iterating through
> - * the jacks in case a jack is plugged in that forces the state to no
> - */
> - pa = cpa;
> - }
> - }
> -
> - pa_device_port_set_available(p, pa);
> -}
> -
> static int report_jack_state(snd_mixer_elem_t *melem, unsigned int mask) {
> struct userdata *u = snd_mixer_elem_get_callback_private(melem);
> snd_hctl_elem_t *elem = snd_mixer_elem_get_private(melem);
> @@ -388,13 +340,16 @@ static int report_jack_state(snd_mixer_elem_t *melem, unsigned int mask) {
> if (u->use_ucm) {
> pa_assert(u->card->ports);
> port = pa_hashmap_get(u->card->ports, jack->name);
> - pa_assert(port);
> }
> else {
> pa_assert(jack->path);
> - pa_assert_se(port = jack->path->port);
> + port = jack->path->port;
> + }
Inconsistent indentation.
> + if (port) {
> + pa_card *card = u->use_ucm ? u->card : NULL;
> + pa_available_t pa = pa_alsa_availability_from_jacks(u->jacks, port, card);
> + pa_device_port_set_available(port, pa);
> }
> - report_port_state(port, u);
> }
> return 0;
> }
> @@ -738,6 +693,8 @@ int pa__init(pa_module *m) {
> if ((description = pa_proplist_gets(data.proplist, PA_PROP_DEVICE_DESCRIPTION)))
> pa_reserve_wrapper_set_application_device_name(reserve, description);
>
> + init_jacks(u);
> +
> add_profiles(u, data.profiles, data.ports);
>
> if (pa_hashmap_isempty(data.profiles)) {
> @@ -766,7 +723,6 @@ int pa__init(pa_module *m) {
> u->card->userdata = u;
> u->card->set_profile = card_set_profile;
>
> - init_jacks(u);
> init_profile(u);
> init_eld_ctls(u);
>
> diff --git a/src/modules/module-switch-on-port-available.c b/src/modules/module-switch-on-port-available.c
> index 5a6b401..b44819e 100644
> --- a/src/modules/module-switch-on-port-available.c
> +++ b/src/modules/module-switch-on-port-available.c
> @@ -30,6 +30,7 @@
>
> struct userdata {
> pa_hook_slot *available_slot;
> + pa_hook_slot *card_put_slot;
> pa_hook_slot *sink_new_slot;
> pa_hook_slot *source_new_slot;
> };
> @@ -246,6 +247,60 @@ static pa_device_port *new_sink_source(pa_hashmap *ports, const char *name) {
> return p;
> }
>
> +static bool profile_contains_available_ports(pa_card_profile *profile) {
> + pa_card *card;
> + pa_device_port *port;
> + void *state;
> +
> + pa_assert(profile);
> +
> + card = profile->card;
> + pa_assert(card);
> +
> + PA_HASHMAP_FOREACH(port, card->ports, state) {
> + if (pa_hashmap_get(port->profiles, profile->name)
> + && port->available != PA_AVAILABLE_NO)
> + return true;
> + }
> +
> + return false;
> +}
> +
> +static pa_card_profile *find_best_profile_with_available_ports(pa_card *card) {
> + pa_card_profile *profile = NULL;
> + pa_card_profile *best_profile = NULL;
> + void *state;
> +
> + pa_assert(card);
> +
> + PA_HASHMAP_FOREACH(profile, card->profiles, state) {
> + if (profile->available == PA_AVAILABLE_NO)
> + continue;
> +
> + if (!profile_contains_available_ports(profile))
> + continue;
> +
> + if (!best_profile || profile->priority > best_profile->priority)
> + best_profile = profile;
> + }
> +
> + return best_profile;
> +}
> +
> +static pa_hook_result_t card_put_hook_callback(pa_core *c, pa_card *card, struct userdata *u) {
> + pa_card_profile *alt_profile;
> +
> + if (profile_contains_available_ports(card->active_profile))
> + return PA_HOOK_OK;
We had some discussion about moving this to the card_new hook instead. I
still think that would be a good idea (but feel free to prove me wrong).
You'd need to change the above to:
if (card->active_profile &&
profile_contains_available_ports(card->active_profile))
return PA_HOOK_OK;
...because card->active_profile isn't always set at that point (but can
be, in case of module-card-restore).
Also set save_profile to false when you're overwriting
module-card-restore's suggestion. (Hmm, maybe I need to do the same for
save_port in sink/source_new callbacks actually!)
> +
> + /* Try to avoid situations where we could settle on a profile when
> + there are not available ports that could be actually used. */
> + if (alt_profile = find_best_profile_with_available_ports(card))
> + card->active_profile = alt_profile;
> +
> + return PA_HOOK_OK;
> +}
> +
> static pa_hook_result_t sink_new_hook_callback(pa_core *c, pa_sink_new_data *new_data, struct userdata *u) {
>
> pa_device_port *p = new_sink_source(new_data->ports, new_data->active_port);
> @@ -276,6 +331,8 @@ int pa__init(pa_module*m) {
> m->userdata = u = pa_xnew(struct userdata, 1);
>
> /* Make sure we are after module-device-restore, so we can overwrite that suggestion if necessary */
> + u->card_put_slot = pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_CARD_PUT],
> + PA_HOOK_NORMAL, (pa_hook_cb_t) card_put_hook_callback, u);
> u->sink_new_slot = pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_SINK_NEW],
> PA_HOOK_NORMAL, (pa_hook_cb_t) sink_new_hook_callback, u);
> u->source_new_slot = pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_SOURCE_NEW],
> @@ -298,6 +355,8 @@ void pa__done(pa_module*m) {
>
> if (u->available_slot)
> pa_hook_slot_free(u->available_slot);
> + if (u->card_put_slot)
> + pa_hook_slot_free(u->card_put_slot);
> if (u->sink_new_slot)
> pa_hook_slot_free(u->sink_new_slot);
> if (u->source_new_slot)
>
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
More information about the pulseaudio-discuss
mailing list