[pulseaudio-discuss] [PATCH v2 3/6] card: move profile selection after pa_card_new()
Alexander E. Patrakov
patrakov at gmail.com
Sun Jun 12 13:33:45 UTC 2016
10.06.2016 22:55, Tanu Kaskinen пишет:
> I want module-alsa-card to set the availability of unavailable
> profiles before the initial card profile gets selected, so that the
> selection logic can use correct availability information.
> module-alsa-card initializes the jack state after calling
> pa_card_new(), however, and the profile selection happens in
> pa_card_new(). This patch solves that by moving parts of pa_card_new()
> to pa_card_choose_initial_profile() and pa_card_put().
>
> pa_card_choose_initial_profile() applies the profile selection policy,
> so module-alsa-card can first call pa_card_new(), then initialize the
> jack state, and then call pa_card_choose_initial_profile(). After that
> module-alsa-card can still override the profile selection policy, in
> case module-alsa-card was loaded with the "profile" argument. Finally,
> pa_card_put() finalizes the card creation.
>
> An alternative solution would have been to move the jack
> initialization to happen before pa_card_new() and use pa_card_new_data
> instead of pa_card in the jack initialization code, but I disliked
> that idea (I want to get rid of the "new data" pattern eventually).
>
> The order in which the initial profile policy is applied is reversed
> in this patch. Previously the first one to set it won, now the last
> one to set it wins. I think this is better, because if you have N
> parties that want to set the profile, we avoid checking N times
> whether someone else has already set the profile.
Looks OK to me, but I would like an extra confirmation whether in the
UCM case the sequence of mixer accesses is the same before and after the
patch.
--
Alexander E. Patrakov
> ---
> src/modules/alsa/module-alsa-card.c | 31 ++++++---
> src/modules/bluetooth/module-bluez4-device.c | 31 +++++----
> src/modules/bluetooth/module-bluez5-device.c | 2 +
> src/modules/macosx/module-coreaudio-device.c | 2 +
> src/modules/module-card-restore.c | 36 +++++++---
> src/pulsecore/card.c | 99 +++++++++++++++++-----------
> src/pulsecore/card.h | 9 +++
> src/pulsecore/core.h | 1 +
> 8 files changed, 143 insertions(+), 68 deletions(-)
>
> diff --git a/src/modules/alsa/module-alsa-card.c b/src/modules/alsa/module-alsa-card.c
> index 1ab2ea2..1976230 100644
> --- a/src/modules/alsa/module-alsa-card.c
> +++ b/src/modules/alsa/module-alsa-card.c
> @@ -671,7 +671,7 @@ int pa__init(pa_module *m) {
> struct userdata *u;
> pa_reserve_wrapper *reserve = NULL;
> const char *description;
> - const char *profile = NULL;
> + const char *profile_str = NULL;
> char *fn = NULL;
> bool namereg_fail = false;
>
> @@ -799,15 +799,6 @@ int pa__init(pa_module *m) {
> goto fail;
> }
>
> - if ((profile = pa_modargs_get_value(u->modargs, "profile", NULL))) {
> - if (pa_hashmap_get(data.profiles, profile))
> - pa_card_new_data_set_profile(&data, profile);
> - else {
> - pa_log("No such profile: %s", profile);
> - goto fail;
> - }
> - }
> -
> u->card = pa_card_new(m->core, &data);
> pa_card_new_data_done(&data);
>
> @@ -821,6 +812,26 @@ int pa__init(pa_module *m) {
> (pa_hook_cb_t) card_suspend_changed, u);
>
> init_jacks(u);
> +
> + pa_card_choose_initial_profile(u->card);
> +
> + /* If the "profile" modarg is given, we have to override whatever the usual
> + * policy chose in pa_card_choose_initial_profile(). */
> + profile_str = pa_modargs_get_value(u->modargs, "profile", NULL);
> + if (profile_str) {
> + pa_card_profile *profile;
> +
> + profile = pa_hashmap_get(u->card->profiles, profile_str);
> + if (!profile) {
> + pa_log("No such profile: %s", profile_str);
> + goto fail;
> + }
> +
> + pa_card_set_profile(u->card, profile, false);
> + }
> +
> + pa_card_put(u->card);
> +
> init_profile(u);
> init_eld_ctls(u);
>
> diff --git a/src/modules/bluetooth/module-bluez4-device.c b/src/modules/bluetooth/module-bluez4-device.c
> index a2de525..13fb7ab 100644
> --- a/src/modules/bluetooth/module-bluez4-device.c
> +++ b/src/modules/bluetooth/module-bluez4-device.c
> @@ -2238,7 +2238,7 @@ static int add_card(struct userdata *u) {
> pa_bluez4_profile_t *d;
> pa_bluez4_form_factor_t ff;
> char *n;
> - const char *default_profile;
> + const char *profile_str;
> const pa_bluez4_device *device;
> const pa_bluez4_uuid *uuid;
>
> @@ -2298,16 +2298,6 @@ static int add_card(struct userdata *u) {
> *d = PA_BLUEZ4_PROFILE_OFF;
> pa_hashmap_put(data.profiles, p->name, p);
>
> - if ((default_profile = pa_modargs_get_value(u->modargs, "profile", NULL))) {
> - if (pa_hashmap_get(data.profiles, default_profile))
> - pa_card_new_data_set_profile(&data, default_profile);
> - else {
> - pa_log("Profile '%s' not valid or not supported by device.", default_profile);
> - pa_card_new_data_done(&data);
> - return -1;
> - }
> - }
> -
> u->card = pa_card_new(u->core, &data);
> pa_card_new_data_done(&data);
>
> @@ -2319,6 +2309,25 @@ static int add_card(struct userdata *u) {
> u->card->userdata = u;
> u->card->set_profile = card_set_profile;
>
> + pa_card_choose_initial_profile(u->card);
> +
> + /* If the "profile" modarg is given, we have to override whatever the usual
> + * policy chose in pa_card_choose_initial_profile(). */
> + profile_str = pa_modargs_get_value(u->modargs, "profile", NULL);
> + if (profile_str) {
> + pa_card_profile *profile;
> +
> + profile = pa_hashmap_get(u->card->profiles, profile_str);
> + if (!profile) {
> + pa_log("No such profile: %s", profile_str);
> + return -1;
> + }
> +
> + pa_card_set_profile(u->card, profile, false);
> + }
> +
> + pa_card_put(u->card);
> +
> d = PA_CARD_PROFILE_DATA(u->card->active_profile);
>
> if (*d != PA_BLUEZ4_PROFILE_OFF && (!device->transports[*d] ||
> diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
> index 84e6d55..498d0e1 100644
> --- a/src/modules/bluetooth/module-bluez5-device.c
> +++ b/src/modules/bluetooth/module-bluez5-device.c
> @@ -1953,6 +1953,8 @@ static int add_card(struct userdata *u) {
>
> u->card->userdata = u;
> u->card->set_profile = set_profile_cb;
> + pa_card_choose_initial_profile(u->card);
> + pa_card_put(u->card);
>
> p = PA_CARD_PROFILE_DATA(u->card->active_profile);
> u->profile = *p;
> diff --git a/src/modules/macosx/module-coreaudio-device.c b/src/modules/macosx/module-coreaudio-device.c
> index 6c9e55d..d91c656 100644
> --- a/src/modules/macosx/module-coreaudio-device.c
> +++ b/src/modules/macosx/module-coreaudio-device.c
> @@ -821,6 +821,8 @@ int pa__init(pa_module *m) {
> pa_card_new_data_done(&card_new_data);
> u->card->userdata = u;
> u->card->set_profile = card_set_profile;
> + pa_card_choose_initial_profile(u->card);
> + pa_card_put(u->card);
>
> u->rtpoll = pa_rtpoll_new();
> pa_thread_mq_init(&u->thread_mq, m->core->mainloop, u->rtpoll);
> diff --git a/src/modules/module-card-restore.c b/src/modules/module-card-restore.c
> index 7545aa5..718a0dd 100644
> --- a/src/modules/module-card-restore.c
> +++ b/src/modules/module-card-restore.c
> @@ -551,16 +551,6 @@ static pa_hook_result_t card_new_hook_callback(pa_core *c, pa_card_new_data *new
> if (!(e = entry_read(u, new_data->name)))
> return PA_HOOK_OK;
>
> - if (e->profile[0]) {
> - if (!new_data->active_profile) {
> - pa_card_new_data_set_profile(new_data, e->profile);
> - pa_log_info("Restored profile '%s' for card %s.", new_data->active_profile, new_data->name);
> - new_data->save_profile = true;
> -
> - } else
> - pa_log_debug("Not restoring profile for card %s, because already set.", new_data->name);
> - }
> -
> /* Always restore the latency offsets because their
> * initial value is always 0 */
>
> @@ -590,6 +580,30 @@ static pa_hook_result_t card_new_hook_callback(pa_core *c, pa_card_new_data *new
> return PA_HOOK_OK;
> }
>
> +static pa_hook_result_t card_choose_initial_profile_callback(pa_core *core, pa_card *card, struct userdata *u) {
> + struct entry *e;
> +
> + if (!(e = entry_read(u, card->name)))
> + return PA_HOOK_OK;
> +
> + if (e->profile[0]) {
> + pa_card_profile *profile;
> +
> + profile = pa_hashmap_get(card->profiles, e->profile);
> + if (profile) {
> + pa_log_info("Restoring profile '%s' for card %s.", card->active_profile->name, card->name);
> + pa_card_set_profile(card, profile, true);
> + } else {
> + pa_log_debug("Tried to restore profile %s for card %s, but the card doesn't have such profile.",
> + e->profile, card->name);
> + }
> + }
> +
> + entry_free(e);
> +
> + return PA_HOOK_OK;
> +}
> +
> static pa_hook_result_t card_preferred_port_changed_callback(pa_core *core, pa_card_preferred_port_changed_hook_data *data,
> struct userdata *u) {
> struct entry *e;
> @@ -634,6 +648,8 @@ int pa__init(pa_module*m) {
> u->module = m;
>
> pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_CARD_NEW], PA_HOOK_EARLY, (pa_hook_cb_t) card_new_hook_callback, u);
> + pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_CARD_CHOOSE_INITIAL_PROFILE], PA_HOOK_NORMAL,
> + (pa_hook_cb_t) card_choose_initial_profile_callback, u);
> pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_CARD_PUT], PA_HOOK_NORMAL, (pa_hook_cb_t) card_put_hook_callback, u);
> pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_CARD_PREFERRED_PORT_CHANGED], PA_HOOK_NORMAL, (pa_hook_cb_t) card_preferred_port_changed_callback, u);
> pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_CARD_PROFILE_CHANGED], PA_HOOK_NORMAL, (pa_hook_cb_t) card_profile_changed_callback, u);
> diff --git a/src/pulsecore/card.c b/src/pulsecore/card.c
> index 0ac70b9..a0c3d93 100644
> --- a/src/pulsecore/card.c
> +++ b/src/pulsecore/card.c
> @@ -176,38 +176,56 @@ pa_card *pa_card_new(pa_core *core, pa_card_new_data *data) {
> c->preferred_input_port = data->preferred_input_port;
> c->preferred_output_port = data->preferred_output_port;
>
> - if (data->active_profile)
> - if ((c->active_profile = pa_hashmap_get(c->profiles, data->active_profile)))
> - c->save_profile = data->save_profile;
> + pa_device_init_description(c->proplist, c);
> + pa_device_init_icon(c->proplist, true);
> + pa_device_init_intended_roles(c->proplist);
>
> - if (!c->active_profile) {
> - PA_HASHMAP_FOREACH(profile, c->profiles, state) {
> - if (profile->available == PA_AVAILABLE_NO)
> - continue;
> + return c;
> +}
>
> - if (!c->active_profile || profile->priority > c->active_profile->priority)
> - c->active_profile = profile;
> - }
> - /* If all profiles are not available, then we still need to pick one */
> - if (!c->active_profile) {
> - PA_HASHMAP_FOREACH(profile, c->profiles, state)
> - if (!c->active_profile || profile->priority > c->active_profile->priority)
> - c->active_profile = profile;
> +void pa_card_choose_initial_profile(pa_card *card) {
> + pa_card_profile *profile;
> + void *state;
> + pa_card_profile *best = NULL;
> +
> + pa_assert(card);
> +
> + /* By default, pick the highest priority profile that is not unavailable,
> + * or if all profiles are unavailable, pick the profile with the highest
> + * priority regardless of its availability. */
> +
> + PA_HASHMAP_FOREACH(profile, card->profiles, state) {
> + if (profile->available == PA_AVAILABLE_NO)
> + continue;
> +
> + if (!best || profile->priority > best->priority)
> + best = profile;
> + }
> +
> + if (!best) {
> + PA_HASHMAP_FOREACH(profile, card->profiles, state) {
> + if (!best || profile->priority > best->priority)
> + best = profile;
> }
> - pa_assert(c->active_profile);
> }
> + pa_assert(best);
>
> - pa_device_init_description(c->proplist, c);
> - pa_device_init_icon(c->proplist, true);
> - pa_device_init_intended_roles(c->proplist);
> + card->active_profile = best;
> + card->save_profile = false;
>
> - pa_assert_se(pa_idxset_put(core->cards, c, &c->index) >= 0);
> + /* Let policy modules override the default. */
> + pa_hook_fire(&card->core->hooks[PA_CORE_HOOK_CARD_CHOOSE_INITIAL_PROFILE], card);
> +}
>
> - pa_log_info("Created %u \"%s\"", c->index, c->name);
> - pa_subscription_post(core, PA_SUBSCRIPTION_EVENT_CARD|PA_SUBSCRIPTION_EVENT_NEW, c->index);
> +void pa_card_put(pa_card *card) {
> + pa_assert(card);
>
> - pa_hook_fire(&core->hooks[PA_CORE_HOOK_CARD_PUT], c);
> - return c;
> + pa_assert_se(pa_idxset_put(card->core->cards, card, &card->index) >= 0);
> + card->linked = true;
> +
> + pa_log_info("Created %u \"%s\"", card->index, card->name);
> + pa_hook_fire(&card->core->hooks[PA_CORE_HOOK_CARD_PUT], card);
> + pa_subscription_post(card->core, PA_SUBSCRIPTION_EVENT_CARD|PA_SUBSCRIPTION_EVENT_NEW, card->index);
> }
>
> void pa_card_free(pa_card *c) {
> @@ -218,15 +236,15 @@ void pa_card_free(pa_card *c) {
>
> core = c->core;
>
> - pa_hook_fire(&core->hooks[PA_CORE_HOOK_CARD_UNLINK], c);
> -
> - pa_namereg_unregister(core, c->name);
> -
> - pa_idxset_remove_by_data(c->core->cards, c, NULL);
> + if (c->linked) {
> + pa_hook_fire(&core->hooks[PA_CORE_HOOK_CARD_UNLINK], c);
>
> - pa_log_info("Freed %u \"%s\"", c->index, c->name);
> + pa_idxset_remove_by_data(c->core->cards, c, NULL);
> + pa_log_info("Freed %u \"%s\"", c->index, c->name);
> + pa_subscription_post(c->core, PA_SUBSCRIPTION_EVENT_CARD|PA_SUBSCRIPTION_EVENT_REMOVE, c->index);
> + }
>
> - pa_subscription_post(c->core, PA_SUBSCRIPTION_EVENT_CARD|PA_SUBSCRIPTION_EVENT_REMOVE, c->index);
> + pa_namereg_unregister(core, c->name);
>
> pa_assert(pa_idxset_isempty(c->sinks));
> pa_idxset_free(c->sinks, NULL);
> @@ -298,20 +316,27 @@ int pa_card_set_profile(pa_card *c, pa_card_profile *profile, bool save) {
> return 0;
> }
>
> - if ((r = c->set_profile(c, profile)) < 0)
> + /* If we're setting the initial profile, we shouldn't call set_profile(),
> + * because the implementations don't expect that (for historical reasons).
> + * We should just set c->active_profile, and the implementations will
> + * properly set up that profile after pa_card_put() has returned. It would
> + * be probably good to change this so that also the initial profile can be
> + * set up in set_profile(), but if set_profile() fails, that would need
> + * some better handling than what we do here currently. */
> + if (c->linked && (r = c->set_profile(c, profile)) < 0)
> return r;
>
> - pa_subscription_post(c->core, PA_SUBSCRIPTION_EVENT_CARD|PA_SUBSCRIPTION_EVENT_CHANGE, c->index);
> -
> - pa_log_info("Changed profile of card %u \"%s\" to %s", c->index, c->name, profile->name);
> -
> c->active_profile = profile;
> c->save_profile = save;
>
> if (save)
> update_port_preferred_profile(c);
>
> - pa_hook_fire(&c->core->hooks[PA_CORE_HOOK_CARD_PROFILE_CHANGED], c);
> + if (c->linked) {
> + pa_log_info("Changed profile of card %u \"%s\" to %s", c->index, c->name, profile->name);
> + pa_hook_fire(&c->core->hooks[PA_CORE_HOOK_CARD_PROFILE_CHANGED], c);
> + pa_subscription_post(c->core, PA_SUBSCRIPTION_EVENT_CARD|PA_SUBSCRIPTION_EVENT_CHANGE, c->index);
> + }
>
> return 0;
> }
> diff --git a/src/pulsecore/card.h b/src/pulsecore/card.h
> index d4970e3..fd1fe0a 100644
> --- a/src/pulsecore/card.h
> +++ b/src/pulsecore/card.h
> @@ -86,6 +86,8 @@ struct pa_card {
>
> pa_suspend_cause_t suspend_cause;
>
> + bool linked;
> +
> void *userdata;
>
> int (*set_profile)(pa_card *c, pa_card_profile *profile);
> @@ -128,6 +130,13 @@ void pa_card_new_data_set_preferred_port(pa_card_new_data *data, pa_direction_t
> void pa_card_new_data_done(pa_card_new_data *data);
>
> pa_card *pa_card_new(pa_core *c, pa_card_new_data *data);
> +
> +/* Select the initial card profile according to the configured policies. This
> + * must be called between pa_card_new() and pa_card_put(), after the port and
> + * profile availablities have been initialized. */
> +void pa_card_choose_initial_profile(pa_card *card);
> +
> +void pa_card_put(pa_card *c);
> void pa_card_free(pa_card *c);
>
> void pa_card_add_profile(pa_card *c, pa_card_profile *profile);
> diff --git a/src/pulsecore/core.h b/src/pulsecore/core.h
> index 00d7f2f..802111b 100644
> --- a/src/pulsecore/core.h
> +++ b/src/pulsecore/core.h
> @@ -118,6 +118,7 @@ typedef enum pa_core_hook {
> PA_CORE_HOOK_CLIENT_PROPLIST_CHANGED,
> PA_CORE_HOOK_CLIENT_SEND_EVENT,
> PA_CORE_HOOK_CARD_NEW,
> + PA_CORE_HOOK_CARD_CHOOSE_INITIAL_PROFILE,
> PA_CORE_HOOK_CARD_PUT,
> PA_CORE_HOOK_CARD_UNLINK,
> PA_CORE_HOOK_CARD_PREFERRED_PORT_CHANGED,
>
More information about the pulseaudio-discuss
mailing list