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
