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

   1 From 13e85dd1763e99d21a60323671b9a5df08bdae75 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 Signed-off-by: Jussi Kukkonen <jussi.kukkonen@intel.com>
  36
  37 Rebased on 8.0.
  38
  39 Upstream-Status: Denied [The patch set needs some work to be accepted.
  40 The review thread:
  41 http://thread.gmane.org/gmane.comp.audio.pulseaudio.general/24301]
  42
  43 Signed-off-by: Tanu Kaskinen <tanuk@iki.fi>
  44 ---
  45  src/modules/alsa/module-alsa-card.c          | 19 +++----
  46  src/modules/bluetooth/module-bluez4-device.c | 18 +++----
  47  src/modules/bluetooth/module-bluez5-device.c |  1 +
  48  src/modules/macosx/module-coreaudio-device.c |  1 +
  49  src/modules/module-card-restore.c            | 24 +++++----
  50  src/pulsecore/card.c                         | 81 +++++++++++++++-------------
  51  src/pulsecore/card.h                         |  7 +++
  52  7 files changed, 86 insertions(+), 65 deletions(-)
  53
  54 diff --git a/src/modules/alsa/module-alsa-card.c b/src/modules/alsa/module-alsa-card.c
  55 index 9e8cde2..fe240f0 100644
  56 --- a/src/modules/alsa/module-alsa-card.c
  57 +++ b/src/modules/alsa/module-alsa-card.c
  58 @@ -770,15 +770,6 @@ int pa__init(pa_module *m) {
  59          goto fail;
  60      }
  61
  62 -    if ((profile = pa_modargs_get_value(u->modargs, "profile", NULL))) {
  63 -        if (pa_hashmap_get(data.profiles, profile))
  64 -            pa_card_new_data_set_profile(&data, profile);
  65 -        else {
  66 -            pa_log("No such profile: %s", profile);
  67 -            goto fail;
  68 -        }
  69 -    }
  70 -
  71      u->card = pa_card_new(m->core, &data);
  72      pa_card_new_data_done(&data);
  73
  74 @@ -789,6 +780,16 @@ int pa__init(pa_module *m) {
  75      u->card->set_profile = card_set_profile;
  76
  77      init_jacks(u);
  78 +    pa_card_put(u->card);
  79 +
  80 +    if ((profile = pa_modargs_get_value(u->modargs, "profile", NULL))) {
  81 +        u->card->active_profile = pa_hashmap_get(u->card->profiles, profile);
  82 +        if (!u->card->active_profile) {
  83 +            pa_log("No such profile: %s", profile);
  84 +            goto fail;
  85 +        }
  86 +    }
  87 +
  88      init_profile(u);
  89      init_eld_ctls(u);
  90
  91 diff --git a/src/modules/bluetooth/module-bluez4-device.c b/src/modules/bluetooth/module-bluez4-device.c
  92 index dd18217..5d0d3db 100644
  93 --- a/src/modules/bluetooth/module-bluez4-device.c
  94 +++ b/src/modules/bluetooth/module-bluez4-device.c
  95 @@ -2304,15 +2304,6 @@ static int add_card(struct userdata *u) {
  96      *d = PA_BLUEZ4_PROFILE_OFF;
  97      pa_hashmap_put(data.profiles, p->name, p);
  98
  99 -    if ((default_profile = pa_modargs_get_value(u->modargs, "profile", NULL))) {
 100 -        if (pa_hashmap_get(data.profiles, default_profile))
 101 -            pa_card_new_data_set_profile(&data, default_profile);
 102 -        else {
 103 -            pa_log("Profile '%s' not valid or not supported by device.", default_profile);
 104 -            return -1;
 105 -        }
 106 -    }
 107 -
 108      u->card = pa_card_new(u->core, &data);
 109      pa_card_new_data_done(&data);
 110
 111 @@ -2323,6 +2314,15 @@ static int add_card(struct userdata *u) {
 112
 113      u->card->userdata = u;
 114      u->card->set_profile = card_set_profile;
 115 +    pa_card_put(u->card);
 116 +
 117 +    if ((default_profile = pa_modargs_get_value(u->modargs, "profile", NULL))) {
 118 +        u->card->active_profile = pa_hashmap_get(u->card->profiles, default_profile);
 119 +        if (!u->card->active_profile) {
 120 +            pa_log("Profile '%s' not valid or not supported by device.", default_profile);
 121 +            return -1;
 122 +        }
 123 +    }
 124
 125      d = PA_CARD_PROFILE_DATA(u->card->active_profile);
 126
 127 diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
 128 index b015c67..7b90a31 100644
 129 --- a/src/modules/bluetooth/module-bluez5-device.c
 130 +++ b/src/modules/bluetooth/module-bluez5-device.c
 131 @@ -1959,6 +1959,7 @@ static int add_card(struct userdata *u) {
 132
 133      u->card->userdata = u;
 134      u->card->set_profile = set_profile_cb;
 135 +    pa_card_put(u->card);
 136
 137      p = PA_CARD_PROFILE_DATA(u->card->active_profile);
 138      u->profile = *p;
 139 diff --git a/src/modules/macosx/module-coreaudio-device.c b/src/modules/macosx/module-coreaudio-device.c
 140 index 0c92d42..7190ee9 100644
 141 --- a/src/modules/macosx/module-coreaudio-device.c
 142 +++ b/src/modules/macosx/module-coreaudio-device.c
 143 @@ -807,6 +807,7 @@ int pa__init(pa_module *m) {
 144      pa_card_new_data_done(&card_new_data);
 145      u->card->userdata = u;
 146      u->card->set_profile = card_set_profile;
 147 +    pa_card_put(u->card);
 148
 149      u->rtpoll = pa_rtpoll_new();
 150      pa_thread_mq_init(&u->thread_mq, m->core->mainloop, u->rtpoll);
 151 diff --git a/src/modules/module-card-restore.c b/src/modules/module-card-restore.c
 152 index f906843..dce6674 100644
 153 --- a/src/modules/module-card-restore.c
 154 +++ b/src/modules/module-card-restore.c
 155 @@ -515,34 +515,38 @@ static pa_hook_result_t port_offset_change_callback(pa_core *c, pa_device_port *
 156      return PA_HOOK_OK;
 157  }
 158
 159 -static pa_hook_result_t card_new_hook_callback(pa_core *c, pa_card_new_data *new_data, struct userdata *u) {
 160 +static pa_hook_result_t card_new_hook_callback(pa_core *c, pa_card *card, struct userdata *u) {
 161      struct entry *e;
 162      void *state;
 163      pa_device_port *p;
 164      struct port_info *p_info;
 165
 166 -    pa_assert(new_data);
 167 +    pa_assert(c);
 168 +    pa_assert(card);
 169 +    pa_assert(u);
 170
 171 -    if (!(e = entry_read(u, new_data->name)))
 172 +    if (!(e = entry_read(u, card->name)))
 173          return PA_HOOK_OK;
 174
 175      if (e->profile[0]) {
 176 -        if (!new_data->active_profile) {
 177 -            pa_card_new_data_set_profile(new_data, e->profile);
 178 -            pa_log_info("Restored profile '%s' for card %s.", new_data->active_profile, new_data->name);
 179 -            new_data->save_profile = true;
 180 +        pa_card_profile *profile;
 181
 182 +        profile = pa_hashmap_get(card->profiles, e->profile);
 183 +        if (profile) {
 184 +            pa_card_set_profile(card, profile, true);
 185 +            pa_log_info("Restored profile '%s' for card %s.", card->active_profile->name, card->name);
 186          } else
 187 -            pa_log_debug("Not restoring profile for card %s, because already set.", new_data->name);
 188 +            pa_log_debug("Tried to restore profile %s for card %s, but the card doesn't have such profile.",
 189 +                         e->profile, card->name);
 190      }
 191
 192      /* Always restore the latency offsets because their
 193       * initial value is always 0 */
 194
 195 -    pa_log_info("Restoring port latency offsets for card %s.", new_data->name);
 196 +    pa_log_info("Restoring port latency offsets for card %s.", card->name);
 197
 198      PA_HASHMAP_FOREACH(p_info, e->ports, state)
 199 -        if ((p = pa_hashmap_get(new_data->ports, p_info->name))) {
 200 +        if ((p = pa_hashmap_get(card->ports, p_info->name))) {
 201              p->latency_offset = p_info->offset;
 202              if (!p->preferred_profile && p_info->profile)
 203                  pa_device_port_set_preferred_profile(p, p_info->profile);
 204 diff --git a/src/pulsecore/card.c b/src/pulsecore/card.c
 205 index f92ac87..1a6e705 100644
 206 --- a/src/pulsecore/card.c
 207 +++ b/src/pulsecore/card.c
 208 @@ -148,6 +148,7 @@ pa_card *pa_card_new(pa_core *core, pa_card_new_data *data) {
 209      pa_assert(!pa_hashmap_isempty(data->profiles));
 210
 211      c = pa_xnew0(pa_card, 1);
 212 +    c->state = PA_CARD_STATE_INIT;
 213
 214      if (!(name = pa_namereg_register(core, data->name, PA_NAMEREG_CARD, c, data->namereg_fail))) {
 215          pa_xfree(c);
 216 @@ -156,12 +157,6 @@ pa_card *pa_card_new(pa_core *core, pa_card_new_data *data) {
 217
 218      pa_card_new_data_set_name(data, name);
 219
 220 -    if (pa_hook_fire(&core->hooks[PA_CORE_HOOK_CARD_NEW], data) < 0) {
 221 -        pa_xfree(c);
 222 -        pa_namereg_unregister(core, name);
 223 -        return NULL;
 224 -    }
 225 -
 226      c->core = core;
 227      c->name = pa_xstrdup(data->name);
 228      c->proplist = pa_proplist_copy(data->proplist);
 229 @@ -184,38 +179,43 @@ pa_card *pa_card_new(pa_core *core, pa_card_new_data *data) {
 230      PA_HASHMAP_FOREACH(port, c->ports, state)
 231          port->card = c;
 232
 233 -    if (data->active_profile)
 234 -        if ((c->active_profile = pa_hashmap_get(c->profiles, data->active_profile)))
 235 -            c->save_profile = data->save_profile;
 236 +    pa_device_init_description(c->proplist, c);
 237 +    pa_device_init_icon(c->proplist, true);
 238 +    pa_device_init_intended_roles(c->proplist);
 239
 240 -    if (!c->active_profile) {
 241 -        PA_HASHMAP_FOREACH(profile, c->profiles, state) {
 242 -            if (profile->available == PA_AVAILABLE_NO)
 243 -                continue;
 244 +    return c;
 245 +}
 246
 247 -            if (!c->active_profile || profile->priority > c->active_profile->priority)
 248 -                c->active_profile = profile;
 249 -        }
 250 -        /* If all profiles are not available, then we still need to pick one */
 251 -        if (!c->active_profile) {
 252 -            PA_HASHMAP_FOREACH(profile, c->profiles, state)
 253 -                if (!c->active_profile || profile->priority > c->active_profile->priority)
 254 -                    c->active_profile = profile;
 255 -        }
 256 -        pa_assert(c->active_profile);
 257 +void pa_card_put(pa_card *card) {
 258 +    pa_card_profile *profile;
 259 +    void *state;
 260 +
 261 +    pa_assert(card);
 262 +
 263 +    PA_HASHMAP_FOREACH(profile, card->profiles, state) {
 264 +        if (profile->available == PA_AVAILABLE_NO)
 265 +            continue;
 266 +
 267 +        if (!card->active_profile || profile->priority > card->active_profile->priority)
 268 +            card->active_profile = profile;
 269      }
 270
 271 -    pa_device_init_description(c->proplist, c);
 272 -    pa_device_init_icon(c->proplist, true);
 273 -    pa_device_init_intended_roles(c->proplist);
 274 +    /* If all profiles are unavailable, then we still need to pick one */
 275 +    if (!card->active_profile) {
 276 +        PA_HASHMAP_FOREACH(profile, card->profiles, state)
 277 +            if (!card->active_profile || profile->priority > card->active_profile->priority)
 278 +                card->active_profile = profile;
 279 +    }
 280 +    pa_assert(card->active_profile);
 281
 282 -    pa_assert_se(pa_idxset_put(core->cards, c, &c->index) >= 0);
 283 +    pa_hook_fire(&card->core->hooks[PA_CORE_HOOK_CARD_NEW], card);
 284
 285 -    pa_log_info("Created %u \"%s\"", c->index, c->name);
 286 -    pa_subscription_post(core, PA_SUBSCRIPTION_EVENT_CARD|PA_SUBSCRIPTION_EVENT_NEW, c->index);
 287 +    pa_assert_se(pa_idxset_put(card->core->cards, card, &card->index) >= 0);
 288 +    card->state = PA_CARD_STATE_LINKED;
 289
 290 -    pa_hook_fire(&core->hooks[PA_CORE_HOOK_CARD_PUT], c);
 291 -    return c;
 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 @@ -306,20 +306,27 @@ 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      if (save)
 321          update_port_preferred_profile(c);
 322
 323 -    pa_hook_fire(&c->core->hooks[PA_CORE_HOOK_CARD_PROFILE_CHANGED], c);
 324 +    if (c->state != PA_CARD_STATE_INIT) {
 325 +        pa_log_info("Changed profile of card %u \"%s\" to %s", c->index, c->name, profile->name);
 326 +        pa_hook_fire(&c->core->hooks[PA_CORE_HOOK_CARD_PROFILE_CHANGED], c);
 327 +        pa_subscription_post(c->core, PA_SUBSCRIPTION_EVENT_CARD|PA_SUBSCRIPTION_EVENT_CHANGE, c->index);
 328 +    }
 329
 330      return 0;
 331  }
 332 diff --git a/src/pulsecore/card.h b/src/pulsecore/card.h
 333 index fff9057..a944301 100644
 334 --- a/src/pulsecore/card.h
 335 +++ b/src/pulsecore/card.h
 336 @@ -34,6 +34,11 @@ typedef enum pa_available {
 337      PA_AVAILABLE_YES = 2,
 338  } pa_available_t;
 339
 340 +typedef enum pa_card_state {
 341 +    PA_CARD_STATE_INIT,
 342 +    PA_CARD_STATE_LINKED,
 343 +} pa_card_state_t;
 344 +
 345  struct pa_card_profile {
 346      pa_card *card;
 347      char *name;
 348 @@ -66,6 +71,7 @@ struct pa_card_profile {
 349
 350  struct pa_card {
 351      uint32_t index;
 352 +    pa_card_state_t state;
 353      pa_core *core;
 354
 355      char *name;
 356 @@ -120,6 +126,7 @@ void pa_card_new_data_set_profile(pa_card_new_data *data, const char *profile);
 357  void pa_card_new_data_done(pa_card_new_data *data);
 358
 359  pa_card *pa_card_new(pa_core *c, pa_card_new_data *data);
 360 +void pa_card_put(pa_card *c);
 361  void pa_card_free(pa_card *c);
 362
 363  void pa_card_add_profile(pa_card *c, pa_card_profile *profile);
 364 --
 365 2.7.0
 366