code.ossystems Code Review - openembedded-core.git/blob

   1 From cc41c8a3149ef04d4aa2db3d15032605a5504658 Mon Sep 17 00:00:00 2001
   2 From: Tanu Kaskinen <tanuk@iki.fi>
   3 Date: Fri, 23 Oct 2015 12:59:53 +0300
   4 Subject: [PATCH 3/4] card: move profile selection after pa_card_new()
   5
   6 I want module-alsa-card to set the availability of unavailable
   7 profiles before the initial card profile gets selected, so that the
   8 selection logic can use correct availability information.
   9 module-alsa-card initializes the jack state after calling
  10 pa_card_new(), however, and the profile selection happens in
  11 pa_card_new(). This patch solves that by introducing pa_card_put() and
  12 moving the profile selection code there.
  13
  14 An alternative solution would have been to move the jack
  15 initialization to happen before pa_card_new() and use pa_card_new_data
  16 instead of pa_card in the jack initialization code, but I disliked
  17 that idea (I want to get rid of the "new data" pattern eventually).
  18
  19 The CARD_NEW hook is used when applying the initial profile policy, so
  20 that was moved to pa_card_put(). That required changing the hook data
  21 from pa_card_new_data to pa_card. module-card-restore now uses
  22 pa_card_set_profile() instead of pa_card_new_data_set_profile(). That
  23 required adding a state variable to pa_card, because
  24 pa_card_set_profile() needs to distinguish between setting the initial
  25 profile and setting the profile in other situations.
  26
  27 The order in which the initial profile policy is applied is reversed
  28 in this patch. Previously the first one to set it won, now the last
  29 one to set it wins. I think this is better, because if you have N
  30 parties that want to set the profile, we avoid checking N times
  31 whether someone else has already set the profile.
  32
  33 http://bugzilla.yoctoproject.org/show_bug.cgi?id=8448
  34
  35 Upstream-Status: Submitted [http://lists.freedesktop.org/archives/pulseaudio-discuss/2015-October/024614.html]
  36 Signed-off-by: Jussi Kukkonen <jussi.kukkonen@intel.com>
  37 ---
  38  src/modules/alsa/module-alsa-card.c          | 19 +++---
  39  src/modules/bluetooth/module-bluez4-device.c | 18 +++---
  40  src/modules/bluetooth/module-bluez5-device.c |  1 +
  41  src/modules/macosx/module-coreaudio-device.c |  1 +
  42  src/modules/module-card-restore.c            | 24 ++++----
  43  src/pulsecore/card.c                         | 86 +++++++++++++++-------------
  44  src/pulsecore/card.h                         |  7 +++
  45  7 files changed, 87 insertions(+), 69 deletions(-)
  46
  47 diff --git a/src/modules/alsa/module-alsa-card.c b/src/modules/alsa/module-alsa-card.c
  48 index 32f517e..5b39654 100644
  49 --- a/src/modules/alsa/module-alsa-card.c
  50 +++ b/src/modules/alsa/module-alsa-card.c
  51 @@ -754,15 +754,6 @@ int pa__init(pa_module *m) {
  52          goto fail;
  53      }
  54
  55 -    if ((profile = pa_modargs_get_value(u->modargs, "profile", NULL))) {
  56 -        if (pa_hashmap_get(data.profiles, profile))
  57 -            pa_card_new_data_set_profile(&data, profile);
  58 -        else {
  59 -            pa_log("No such profile: %s", profile);
  60 -            goto fail;
  61 -        }
  62 -    }
  63 -
  64      u->card = pa_card_new(m->core, &data);
  65      pa_card_new_data_done(&data);
  66
  67 @@ -773,6 +764,16 @@ int pa__init(pa_module *m) {
  68      u->card->set_profile = card_set_profile;
  69
  70      init_jacks(u);
  71 +    pa_card_put(u->card);
  72 +
  73 +    if ((profile = pa_modargs_get_value(u->modargs, "profile", NULL))) {
  74 +        u->card->active_profile = pa_hashmap_get(u->card->profiles, profile);
  75 +        if (!u->card->active_profile) {
  76 +            pa_log("No such profile: %s", profile);
  77 +            goto fail;
  78 +        }
  79 +    }
  80 +
  81      init_profile(u);
  82      init_eld_ctls(u);
  83
  84 diff --git a/src/modules/bluetooth/module-bluez4-device.c b/src/modules/bluetooth/module-bluez4-device.c
  85 index 94e6988..5efc5dc 100644
  86 --- a/src/modules/bluetooth/module-bluez4-device.c
  87 +++ b/src/modules/bluetooth/module-bluez4-device.c
  88 @@ -2307,15 +2307,6 @@ static int add_card(struct userdata *u) {
  89      *d = PA_BLUEZ4_PROFILE_OFF;
  90      pa_hashmap_put(data.profiles, p->name, p);
  91
  92 -    if ((default_profile = pa_modargs_get_value(u->modargs, "profile", NULL))) {
  93 -        if (pa_hashmap_get(data.profiles, default_profile))
  94 -            pa_card_new_data_set_profile(&data, default_profile);
  95 -        else {
  96 -            pa_log("Profile '%s' not valid or not supported by device.", default_profile);
  97 -            return -1;
  98 -        }
  99 -    }
 100 -
 101      u->card = pa_card_new(u->core, &data);
 102      pa_card_new_data_done(&data);
 103
 104 @@ -2326,6 +2317,15 @@ static int add_card(struct userdata *u) {
 105
 106      u->card->userdata = u;
 107      u->card->set_profile = card_set_profile;
 108 +    pa_card_put(u->card);
 109 +
 110 +    if ((default_profile = pa_modargs_get_value(u->modargs, "profile", NULL))) {
 111 +        u->card->active_profile = pa_hashmap_get(u->card->profiles, default_profile);
 112 +        if (!u->card->active_profile) {
 113 +            pa_log("Profile '%s' not valid or not supported by device.", default_profile);
 114 +            return -1;
 115 +        }
 116 +    }
 117
 118      d = PA_CARD_PROFILE_DATA(u->card->active_profile);
 119
 120 diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
 121 index 3321785..0081a21 100644
 122 --- a/src/modules/bluetooth/module-bluez5-device.c
 123 +++ b/src/modules/bluetooth/module-bluez5-device.c
 124 @@ -1959,6 +1959,7 @@ static int add_card(struct userdata *u) {
 125
 126      u->card->userdata = u;
 127      u->card->set_profile = set_profile_cb;
 128 +    pa_card_put(u->card);
 129
 130      p = PA_CARD_PROFILE_DATA(u->card->active_profile);
 131      u->profile = *p;
 132 diff --git a/src/modules/macosx/module-coreaudio-device.c b/src/modules/macosx/module-coreaudio-device.c
 133 index 4bbb5d5..41f151f 100644
 134 --- a/src/modules/macosx/module-coreaudio-device.c
 135 +++ b/src/modules/macosx/module-coreaudio-device.c
 136 @@ -764,6 +764,7 @@ int pa__init(pa_module *m) {
 137      pa_card_new_data_done(&card_new_data);
 138      u->card->userdata = u;
 139      u->card->set_profile = card_set_profile;
 140 +    pa_card_put(u->card);
 141
 142      u->rtpoll = pa_rtpoll_new();
 143      pa_thread_mq_init(&u->thread_mq, m->core->mainloop, u->rtpoll);
 144 diff --git a/src/modules/module-card-restore.c b/src/modules/module-card-restore.c
 145 index baa2f4f..0501ac8 100644
 146 --- a/src/modules/module-card-restore.c
 147 +++ b/src/modules/module-card-restore.c
 148 @@ -485,34 +485,38 @@ static pa_hook_result_t port_offset_change_callback(pa_core *c, pa_device_port *
 149      return PA_HOOK_OK;
 150  }
 151
 152 -static pa_hook_result_t card_new_hook_callback(pa_core *c, pa_card_new_data *new_data, struct userdata *u) {
 153 +static pa_hook_result_t card_new_hook_callback(pa_core *c, pa_card *card, struct userdata *u) {
 154      struct entry *e;
 155      void *state;
 156      pa_device_port *p;
 157      struct port_info *p_info;
 158
 159 -    pa_assert(new_data);
 160 +    pa_assert(c);
 161 +    pa_assert(card);
 162 +    pa_assert(u);
 163
 164 -    if (!(e = entry_read(u, new_data->name)))
 165 +    if (!(e = entry_read(u, card->name)))
 166          return PA_HOOK_OK;
 167
 168      if (e->profile[0]) {
 169 -        if (!new_data->active_profile) {
 170 -            pa_card_new_data_set_profile(new_data, e->profile);
 171 -            pa_log_info("Restored profile '%s' for card %s.", new_data->active_profile, new_data->name);
 172 -            new_data->save_profile = true;
 173 +        pa_card_profile *profile;
 174
 175 +        profile = pa_hashmap_get(card->profiles, e->profile);
 176 +        if (profile) {
 177 +            pa_card_set_profile(card, profile, true);
 178 +            pa_log_info("Restored profile '%s' for card %s.", card->active_profile->name, card->name);
 179          } else
 180 -            pa_log_debug("Not restoring profile for card %s, because already set.", new_data->name);
 181 +            pa_log_debug("Tried to restore profile %s for card %s, but the card doesn't have such profile.",
 182 +                         e->profile, card->name);
 183      }
 184
 185      /* Always restore the latency offsets because their
 186       * initial value is always 0 */
 187
 188 -    pa_log_info("Restoring port latency offsets for card %s.", new_data->name);
 189 +    pa_log_info("Restoring port latency offsets for card %s.", card->name);
 190
 191      PA_HASHMAP_FOREACH(p_info, e->ports, state)
 192 -        if ((p = pa_hashmap_get(new_data->ports, p_info->name)))
 193 +        if ((p = pa_hashmap_get(card->ports, p_info->name)))
 194              p->latency_offset = p_info->offset;
 195
 196      entry_free(e);
 197 diff --git a/src/pulsecore/card.c b/src/pulsecore/card.c
 198 index cc4c784..1b7f71b 100644
 199 --- a/src/pulsecore/card.c
 200 +++ b/src/pulsecore/card.c
 201 @@ -151,6 +151,7 @@ pa_card *pa_card_new(pa_core *core, pa_card_new_data *data) {
 202      pa_assert(!pa_hashmap_isempty(data->profiles));
 203
 204      c = pa_xnew(pa_card, 1);
 205 +    c->state = PA_CARD_STATE_INIT;
 206
 207      if (!(name = pa_namereg_register(core, data->name, PA_NAMEREG_CARD, c, data->namereg_fail))) {
 208          pa_xfree(c);
 209 @@ -159,12 +160,6 @@ pa_card *pa_card_new(pa_core *core, pa_card_new_data *data) {
 210
 211      pa_card_new_data_set_name(data, name);
 212
 213 -    if (pa_hook_fire(&core->hooks[PA_CORE_HOOK_CARD_NEW], data) < 0) {
 214 -        pa_xfree(c);
 215 -        pa_namereg_unregister(core, name);
 216 -        return NULL;
 217 -    }
 218 -
 219      c->core = core;
 220      c->name = pa_xstrdup(data->name);
 221      c->proplist = pa_proplist_copy(data->proplist);
 222 @@ -187,30 +182,6 @@ pa_card *pa_card_new(pa_core *core, pa_card_new_data *data) {
 223      PA_HASHMAP_FOREACH(port, c->ports, state)
 224          port->card = c;
 225
 226 -    c->active_profile = NULL;
 227 -    c->save_profile = false;
 228 -
 229 -    if (data->active_profile)
 230 -        if ((c->active_profile = pa_hashmap_get(c->profiles, data->active_profile)))
 231 -            c->save_profile = data->save_profile;
 232 -
 233 -    if (!c->active_profile) {
 234 -        PA_HASHMAP_FOREACH(profile, c->profiles, state) {
 235 -            if (profile->available == PA_AVAILABLE_NO)
 236 -                continue;
 237 -
 238 -            if (!c->active_profile || profile->priority > c->active_profile->priority)
 239 -                c->active_profile = profile;
 240 -        }
 241 -        /* If all profiles are not available, then we still need to pick one */
 242 -        if (!c->active_profile) {
 243 -            PA_HASHMAP_FOREACH(profile, c->profiles, state)
 244 -                if (!c->active_profile || profile->priority > c->active_profile->priority)
 245 -                    c->active_profile = profile;
 246 -        }
 247 -        pa_assert(c->active_profile);
 248 -    }
 249 -
 250      c->userdata = NULL;
 251      c->set_profile = NULL;
 252      c->active_profile = NULL;
 253 @@ -219,13 +190,39 @@ pa_card *pa_card_new(pa_core *core, pa_card_new_data *data) {
 254      pa_device_init_icon(c->proplist, true);
 255      pa_device_init_intended_roles(c->proplist);
 256
 257 -    pa_assert_se(pa_idxset_put(core->cards, c, &c->index) >= 0);
 258 +    return c;
 259 +}
 260
 261 -    pa_log_info("Created %u \"%s\"", c->index, c->name);
 262 -    pa_subscription_post(core, PA_SUBSCRIPTION_EVENT_CARD|PA_SUBSCRIPTION_EVENT_NEW, c->index);
 263 +void pa_card_put(pa_card *card) {
 264 +    pa_card_profile *profile;
 265 +    void *state;
 266
 267 -    pa_hook_fire(&core->hooks[PA_CORE_HOOK_CARD_PUT], c);
 268 -    return c;
 269 +    pa_assert(card);
 270 +
 271 +    PA_HASHMAP_FOREACH(profile, card->profiles, state) {
 272 +        if (profile->available == PA_AVAILABLE_NO)
 273 +            continue;
 274 +
 275 +        if (!card->active_profile || profile->priority > card->active_profile->priority)
 276 +            card->active_profile = profile;
 277 +    }
 278 +
 279 +    /* If all profiles are unavailable, then we still need to pick one */
 280 +    if (!card->active_profile) {
 281 +        PA_HASHMAP_FOREACH(profile, card->profiles, state)
 282 +            if (!card->active_profile || profile->priority > card->active_profile->priority)
 283 +                card->active_profile = profile;
 284 +    }
 285 +    pa_assert(card->active_profile);
 286 +
 287 +    pa_hook_fire(&card->core->hooks[PA_CORE_HOOK_CARD_NEW], card);
 288 +
 289 +    pa_assert_se(pa_idxset_put(card->core->cards, card, &card->index) >= 0);
 290 +    card->state = PA_CARD_STATE_LINKED;
 291 +
 292 +    pa_log_info("Created %u \"%s\"", card->index, card->name);
 293 +    pa_hook_fire(&card->core->hooks[PA_CORE_HOOK_CARD_PUT], card);
 294 +    pa_subscription_post(card->core, PA_SUBSCRIPTION_EVENT_CARD|PA_SUBSCRIPTION_EVENT_NEW, card->index);
 295  }
 296
 297  void pa_card_free(pa_card *c) {
 298 @@ -292,17 +289,24 @@ int pa_card_set_profile(pa_card *c, pa_card_profile *profile, bool save) {
 299          return 0;
 300      }
 301
 302 -    if ((r = c->set_profile(c, profile)) < 0)
 303 +    /* If we're setting the initial profile, we shouldn't call set_profile(),
 304 +     * because the implementations don't expect that (for historical reasons).
 305 +     * We should just set c->active_profile, and the implementations will
 306 +     * properly set up that profile after pa_card_put() has returned. It would
 307 +     * be probably good to change this so that also the initial profile can be
 308 +     * set up in set_profile(), but if set_profile() fails, that would need
 309 +     * some better handling than what we do here currently. */
 310 +    if (c->state != PA_CARD_STATE_INIT && (r = c->set_profile(c, profile)) < 0)
 311          return r;
 312
 313 -    pa_subscription_post(c->core, PA_SUBSCRIPTION_EVENT_CARD|PA_SUBSCRIPTION_EVENT_CHANGE, c->index);
 314 -
 315 -    pa_log_info("Changed profile of card %u \"%s\" to %s", c->index, c->name, profile->name);
 316 -
 317      c->active_profile = profile;
 318      c->save_profile = save;
 319
 320 -    pa_hook_fire(&c->core->hooks[PA_CORE_HOOK_CARD_PROFILE_CHANGED], c);
 321 +    if (c->state != PA_CARD_STATE_INIT) {
 322 +        pa_log_info("Changed profile of card %u \"%s\" to %s", c->index, c->name, profile->name);
 323 +        pa_hook_fire(&c->core->hooks[PA_CORE_HOOK_CARD_PROFILE_CHANGED], c);
 324 +        pa_subscription_post(c->core, PA_SUBSCRIPTION_EVENT_CARD|PA_SUBSCRIPTION_EVENT_CHANGE, c->index);
 325 +    }
 326
 327      return 0;
 328  }
 329 diff --git a/src/pulsecore/card.h b/src/pulsecore/card.h
 330 index 1c33958..dbbc1c2 100644
 331 --- a/src/pulsecore/card.h
 332 +++ b/src/pulsecore/card.h
 333 @@ -37,6 +37,11 @@ typedef enum pa_available {
 334  #include <pulsecore/module.h>
 335  #include <pulsecore/idxset.h>
 336
 337 +typedef enum pa_card_state {
 338 +    PA_CARD_STATE_INIT,
 339 +    PA_CARD_STATE_LINKED,
 340 +} pa_card_state_t;
 341 +
 342  typedef struct pa_card_profile {
 343      pa_card *card;
 344      char *name;
 345 @@ -61,6 +66,7 @@ typedef struct pa_card_profile {
 346
 347  struct pa_card {
 348      uint32_t index;
 349 +    pa_card_state_t state;
 350      pa_core *core;
 351
 352      char *name;
 353 @@ -115,6 +121,7 @@ void pa_card_new_data_set_profile(pa_card_new_data *data, const char *profile);
 354  void pa_card_new_data_done(pa_card_new_data *data);
 355
 356  pa_card *pa_card_new(pa_core *c, pa_card_new_data *data);
 357 +void pa_card_put(pa_card *c);
 358  void pa_card_free(pa_card *c);
 359
 360  void pa_card_add_profile(pa_card *c, pa_card_profile *profile);
 361 --
 362 2.1.4
 363