[pulseaudio-discuss] [PATCH 4/7] card: move profile selection after pa_card_new()

Tanu Kaskinen tanuk at iki.fi
Fri Oct 23 03:56:43 PDT 2015


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 introducing pa_card_put() and
moving the profile selection code there.

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 CARD_NEW hook is used when applying the initial profile policy, so
that was moved to pa_card_put(). That required changing the hook data
from pa_card_new_data to pa_card. module-card-restore now uses
pa_card_set_profile() instead of pa_card_new_data_set_profile(). That
required adding a state variable to pa_card, because
pa_card_set_profile() needs to distinguish between setting the initial
profile and setting the profile in other situations.

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.
---
 src/modules/alsa/module-alsa-card.c          | 19 ++++---
 src/modules/bluetooth/module-bluez4-device.c | 18 +++---
 src/modules/bluetooth/module-bluez5-device.c |  1 +
 src/modules/macosx/module-coreaudio-device.c |  1 +
 src/modules/module-card-restore.c            | 24 ++++----
 src/pulsecore/card.c                         | 83 +++++++++++++++-------------
 src/pulsecore/card.h                         |  7 +++
 7 files changed, 87 insertions(+), 66 deletions(-)

diff --git a/src/modules/alsa/module-alsa-card.c b/src/modules/alsa/module-alsa-card.c
index 0943a17..ad463ec 100644
--- a/src/modules/alsa/module-alsa-card.c
+++ b/src/modules/alsa/module-alsa-card.c
@@ -768,15 +768,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);
 
@@ -787,6 +778,16 @@ int pa__init(pa_module *m) {
     u->card->set_profile = card_set_profile;
 
     init_jacks(u);
+    pa_card_put(u->card);
+
+    if ((profile = pa_modargs_get_value(u->modargs, "profile", NULL))) {
+        u->card->active_profile = pa_hashmap_get(u->card->profiles, profile);
+        if (!u->card->active_profile) {
+            pa_log("No such profile: %s", profile);
+            goto fail;
+        }
+    }
+
     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 41b436f..b9e2335 100644
--- a/src/modules/bluetooth/module-bluez4-device.c
+++ b/src/modules/bluetooth/module-bluez4-device.c
@@ -2304,15 +2304,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);
-            return -1;
-        }
-    }
-
     u->card = pa_card_new(u->core, &data);
     pa_card_new_data_done(&data);
 
@@ -2323,6 +2314,15 @@ static int add_card(struct userdata *u) {
 
     u->card->userdata = u;
     u->card->set_profile = card_set_profile;
+    pa_card_put(u->card);
+
+    if ((default_profile = pa_modargs_get_value(u->modargs, "profile", NULL))) {
+        u->card->active_profile = pa_hashmap_get(u->card->profiles, default_profile);
+        if (!u->card->active_profile) {
+            pa_log("Profile '%s' not valid or not supported by device.", default_profile);
+            return -1;
+        }
+    }
 
     d = PA_CARD_PROFILE_DATA(u->card->active_profile);
 
diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
index 19ce2b7..bf81384 100644
--- a/src/modules/bluetooth/module-bluez5-device.c
+++ b/src/modules/bluetooth/module-bluez5-device.c
@@ -1959,6 +1959,7 @@ static int add_card(struct userdata *u) {
 
     u->card->userdata = u;
     u->card->set_profile = set_profile_cb;
+    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 cbf1f27..e100f7a 100644
--- a/src/modules/macosx/module-coreaudio-device.c
+++ b/src/modules/macosx/module-coreaudio-device.c
@@ -765,6 +765,7 @@ 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_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 3725d30..90bac30 100644
--- a/src/modules/module-card-restore.c
+++ b/src/modules/module-card-restore.c
@@ -481,34 +481,38 @@ static pa_hook_result_t port_offset_change_callback(pa_core *c, pa_device_port *
     return PA_HOOK_OK;
 }
 
-static pa_hook_result_t card_new_hook_callback(pa_core *c, pa_card_new_data *new_data, struct userdata *u) {
+static pa_hook_result_t card_new_hook_callback(pa_core *c, pa_card *card, struct userdata *u) {
     struct entry *e;
     void *state;
     pa_device_port *p;
     struct port_info *p_info;
 
-    pa_assert(new_data);
+    pa_assert(c);
+    pa_assert(card);
+    pa_assert(u);
 
-    if (!(e = entry_read(u, new_data->name)))
+    if (!(e = entry_read(u, card->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;
+        pa_card_profile *profile;
 
+        profile = pa_hashmap_get(card->profiles, e->profile);
+        if (profile) {
+            pa_card_set_profile(card, profile, true);
+            pa_log_info("Restored profile '%s' for card %s.", card->active_profile->name, card->name);
         } else
-            pa_log_debug("Not restoring profile for card %s, because already set.", new_data->name);
+            pa_log_debug("Tried to restore profile %s for card %s, but the card doesn't have such profile.",
+                         e->profile, card->name);
     }
 
     /* Always restore the latency offsets because their
      * initial value is always 0 */
 
-    pa_log_info("Restoring port latency offsets for card %s.", new_data->name);
+    pa_log_info("Restoring port latency offsets for card %s.", card->name);
 
     PA_HASHMAP_FOREACH(p_info, e->ports, state)
-        if ((p = pa_hashmap_get(new_data->ports, p_info->name)))
+        if ((p = pa_hashmap_get(card->ports, p_info->name)))
             p->latency_offset = p_info->offset;
 
     entry_free(e);
diff --git a/src/pulsecore/card.c b/src/pulsecore/card.c
index 4ef7900..67471cc 100644
--- a/src/pulsecore/card.c
+++ b/src/pulsecore/card.c
@@ -139,6 +139,7 @@ pa_card *pa_card_new(pa_core *core, pa_card_new_data *data) {
     pa_assert(!pa_hashmap_isempty(data->profiles));
 
     c = pa_xnew0(pa_card, 1);
+    c->state = PA_CARD_STATE_INIT;
 
     if (!(name = pa_namereg_register(core, data->name, PA_NAMEREG_CARD, c, data->namereg_fail))) {
         pa_xfree(c);
@@ -147,12 +148,6 @@ pa_card *pa_card_new(pa_core *core, pa_card_new_data *data) {
 
     pa_card_new_data_set_name(data, name);
 
-    if (pa_hook_fire(&core->hooks[PA_CORE_HOOK_CARD_NEW], data) < 0) {
-        pa_xfree(c);
-        pa_namereg_unregister(core, name);
-        return NULL;
-    }
-
     c->core = core;
     c->name = pa_xstrdup(data->name);
     c->proplist = pa_proplist_copy(data->proplist);
@@ -175,38 +170,43 @@ pa_card *pa_card_new(pa_core *core, pa_card_new_data *data) {
     PA_HASHMAP_FOREACH(port, c->ports, state)
         port->card = c;
 
-    if (data->active_profile)
-        if ((c->active_profile = pa_hashmap_get(c->profiles, data->active_profile)))
-            c->save_profile = data->save_profile;
-
-    if (!c->active_profile) {
-        PA_HASHMAP_FOREACH(profile, c->profiles, state) {
-            if (profile->available == PA_AVAILABLE_NO)
-                continue;
-
-            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;
-        }
-        pa_assert(c->active_profile);
-    }
-
     pa_device_init_description(c->proplist, c);
     pa_device_init_icon(c->proplist, true);
     pa_device_init_intended_roles(c->proplist);
 
-    pa_assert_se(pa_idxset_put(core->cards, c, &c->index) >= 0);
+    return c;
+}
+
+void pa_card_put(pa_card *card) {
+    pa_card_profile *profile;
+    void *state;
 
-    pa_log_info("Created %u \"%s\"", c->index, c->name);
-    pa_subscription_post(core, PA_SUBSCRIPTION_EVENT_CARD|PA_SUBSCRIPTION_EVENT_NEW, c->index);
+    pa_assert(card);
 
-    pa_hook_fire(&core->hooks[PA_CORE_HOOK_CARD_PUT], c);
-    return c;
+    PA_HASHMAP_FOREACH(profile, card->profiles, state) {
+        if (profile->available == PA_AVAILABLE_NO)
+            continue;
+
+        if (!card->active_profile || profile->priority > card->active_profile->priority)
+            card->active_profile = profile;
+    }
+
+    /* If all profiles are unavailable, then we still need to pick one */
+    if (!card->active_profile) {
+        PA_HASHMAP_FOREACH(profile, card->profiles, state)
+            if (!card->active_profile || profile->priority > card->active_profile->priority)
+                card->active_profile = profile;
+    }
+    pa_assert(card->active_profile);
+
+    pa_hook_fire(&card->core->hooks[PA_CORE_HOOK_CARD_NEW], card);
+
+    pa_assert_se(pa_idxset_put(card->core->cards, card, &card->index) >= 0);
+    card->state = PA_CARD_STATE_LINKED;
+
+    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) {
@@ -273,17 +273,24 @@ 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->state != PA_CARD_STATE_INIT && (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;
 
-    pa_hook_fire(&c->core->hooks[PA_CORE_HOOK_CARD_PROFILE_CHANGED], c);
+    if (c->state != PA_CARD_STATE_INIT) {
+        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 1c33958..dbbc1c2 100644
--- a/src/pulsecore/card.h
+++ b/src/pulsecore/card.h
@@ -37,6 +37,11 @@ typedef enum pa_available {
 #include <pulsecore/module.h>
 #include <pulsecore/idxset.h>
 
+typedef enum pa_card_state {
+    PA_CARD_STATE_INIT,
+    PA_CARD_STATE_LINKED,
+} pa_card_state_t;
+
 typedef struct pa_card_profile {
     pa_card *card;
     char *name;
@@ -61,6 +66,7 @@ typedef struct pa_card_profile {
 
 struct pa_card {
     uint32_t index;
+    pa_card_state_t state;
     pa_core *core;
 
     char *name;
@@ -115,6 +121,7 @@ void pa_card_new_data_set_profile(pa_card_new_data *data, const char *profile);
 void pa_card_new_data_done(pa_card_new_data *data);
 
 pa_card *pa_card_new(pa_core *c, pa_card_new_data *data);
+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);
-- 
2.6.1



More information about the pulseaudio-discuss mailing list