Some comments below. I don't have time right now to go through it all,
but I'll continue later. You might have to wait until Monday for the
rest, though, if Colin or Arun doesn't do the review before me.
--
Tanu
On Thu, 2011-10-27 at 16:45 +0200, David Henningsson wrote:
> To be able to add ports to all profiles, we need to probe all
> profiles at startup. To speed this up, we now have a cache of
> probes paths which is owned by the profile set. Since paths
> are now owned by the profile set, the path set must now have
> a hashmap of paths instead of a linked list.
>
> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
> ---
> src/modules/alsa/alsa-mixer.c | 342 +++++++++++++++++++++--------------
> src/modules/alsa/alsa-mixer.h | 17 +-
> src/modules/alsa/alsa-sink.c | 27 +--
> src/modules/alsa/alsa-source.c | 25 +--
> src/modules/alsa/module-alsa-card.c | 20 ++-
> 5 files changed, 247 insertions(+), 184 deletions(-)
>
> diff --git a/src/modules/alsa/alsa-mixer.c b/src/modules/alsa/alsa-mixer.c
> index 151eef5..9a5c066 100644
> --- a/src/modules/alsa/alsa-mixer.c
> +++ b/src/modules/alsa/alsa-mixer.c
> @@ -561,13 +561,10 @@ void pa_alsa_path_free(pa_alsa_path *p) {
> }
>
> void pa_alsa_path_set_free(pa_alsa_path_set *ps) {
> - pa_alsa_path *p;
> pa_assert(ps);
>
> - while ((p = ps->paths)) {
> - PA_LLIST_REMOVE(pa_alsa_path, ps->paths, p);
> - pa_alsa_path_free(p);
> - }
> + if (ps->paths)
> + pa_hashmap_free(ps->paths, NULL, NULL);
>
> pa_xfree(ps);
> }
> @@ -2575,7 +2572,7 @@ int pa_alsa_path_probe(pa_alsa_path *p, snd_mixer_t *m, pa_bool_t ignore_dB) {
> pa_assert(m);
>
> if (p->probed)
> - return 0;
> + return p->supported ? 0 : -1;
>
> pa_zero(min_dB);
> pa_zero(max_dB);
> @@ -2585,6 +2582,7 @@ int pa_alsa_path_probe(pa_alsa_path *p, snd_mixer_t *m, pa_bool_t ignore_dB) {
> PA_LLIST_FOREACH(e, p->elements) {
> if (element_probe(e, m) < 0) {
> p->supported = FALSE;
> + p->probed = TRUE;
> pa_log_debug("Probe of element '%s' failed.", e->alsa_name);
> return -1;
> }
> @@ -2641,6 +2639,7 @@ int pa_alsa_path_probe(pa_alsa_path *p, snd_mixer_t *m, pa_bool_t ignore_dB) {
>
> if (p->has_req_any && !p->req_any_present) {
> p->supported = FALSE;
> + p->probed = TRUE;
So pa_alsa_path_probe() should always set p->probed to TRUE. Instead of
setting it three times, I think it would be better to set it immediately
after the "if (p->probed)" check in the beginning.
> pa_log_debug("Skipping path '%s', none of required-any elements preset.", p->name);
> return -1;
> }
> @@ -2767,12 +2766,13 @@ void pa_alsa_path_set_callback(pa_alsa_path *p, snd_mixer_t *m, snd_mixer_elem_c
>
> void pa_alsa_path_set_set_callback(pa_alsa_path_set *ps, snd_mixer_t *m, snd_mixer_elem_callback_t cb, void *userdata) {
> pa_alsa_path *p;
> + void *state;
>
> pa_assert(ps);
> pa_assert(m);
> pa_assert(cb);
>
> - PA_LLIST_FOREACH(p, ps->paths)
> + PA_HASHMAP_FOREACH(p, ps->paths, state)
> pa_alsa_path_set_callback(p, m, cb, userdata);
> }
>
> @@ -2780,7 +2780,8 @@ pa_alsa_path_set *pa_alsa_path_set_new(pa_alsa_mapping *m, pa_alsa_direction_t d
> pa_alsa_path_set *ps;
> char **pn = NULL, **en = NULL, **ie;
> pa_alsa_decibel_fix *db_fix;
> - void *state;
> + void *state, *state2;
> + pa_hashmap *cache;
>
> pa_assert(m);
> pa_assert(m->profile_set);
> @@ -2792,19 +2793,24 @@ pa_alsa_path_set *pa_alsa_path_set_new(pa_alsa_mapping *m, pa_alsa_direction_t d
>
> ps = pa_xnew0(pa_alsa_path_set, 1);
> ps->direction = direction;
> + ps->paths = pa_hashmap_new(pa_idxset_trivial_hash_func, pa_idxset_trivial_compare_func);
>
> - if (direction == PA_ALSA_DIRECTION_OUTPUT)
> + if (direction == PA_ALSA_DIRECTION_OUTPUT) {
> pn = m->output_path_names;
> - else if (direction == PA_ALSA_DIRECTION_INPUT)
> + cache = m->profile_set->output_paths;
> + }
> + else if (direction == PA_ALSA_DIRECTION_INPUT) {
> pn = m->input_path_names;
> + cache = m->profile_set->input_paths;
> + }
>
> if (pn) {
> char **in;
>
> for (in = pn; *in; in++) {
> - pa_alsa_path *p;
> + pa_alsa_path *p = NULL;
> pa_bool_t duplicate = FALSE;
> - char **kn, *fn;
> + char **kn;
>
> for (kn = pn; kn < in; kn++)
> if (pa_streq(*kn, *in)) {
> @@ -2815,15 +2821,18 @@ pa_alsa_path_set *pa_alsa_path_set_new(pa_alsa_mapping *m, pa_alsa_direction_t d
> if (duplicate)
> continue;
>
> - fn = pa_sprintf_malloc("%s.conf", *in);
> -
> - if ((p = pa_alsa_path_new(paths_dir, fn, direction))) {
> - p->path_set = ps;
> - PA_LLIST_INSERT_AFTER(pa_alsa_path, ps->paths, ps->last_path, p);
> - ps->last_path = p;
> + p = pa_hashmap_get(cache, *in);
> + if (!p) {
> + char *fn = pa_sprintf_malloc("%s.conf", *in);
> + p = pa_alsa_path_new(paths_dir, fn, direction);
> + pa_xfree(fn);
> + if (p)
> + pa_hashmap_put(cache, *in, p);
> }
> + pa_assert(pa_hashmap_get(cache, *in) == p);
> + if (p)
> + pa_hashmap_put(ps->paths, p, p);
I have not thought this through, but would it make sense to use the path
name as the key to ps->paths, and if there are collisions, do the path
renaming here instead of using the path_set_make_paths_unique() function
later?
>
> - pa_xfree(fn);
> }
>
> goto finish;
> @@ -2844,7 +2853,6 @@ pa_alsa_path_set *pa_alsa_path_set_new(pa_alsa_mapping *m, pa_alsa_direction_t d
> pa_alsa_path *p;
>
> p = pa_alsa_path_synthesize(*ie, direction);
> - p->path_set = ps;
>
> /* Mark all other passed elements for require-absent */
> for (je = en; *je; je++) {
> @@ -2864,8 +2872,7 @@ pa_alsa_path_set *pa_alsa_path_set_new(pa_alsa_mapping *m, pa_alsa_direction_t d
> p->last_element = e;
> }
>
> - PA_LLIST_INSERT_AFTER(pa_alsa_path, ps->paths, ps->last_path, p);
> - ps->last_path = p;
> + pa_hashmap_put(ps->paths, *ie, p);
> }
>
> finish:
> @@ -2873,7 +2880,7 @@ finish:
> PA_HASHMAP_FOREACH(db_fix, m->profile_set->decibel_fixes, state) {
> pa_alsa_path *p;
>
> - PA_LLIST_FOREACH(p, ps->paths) {
> + PA_HASHMAP_FOREACH(p, ps->paths, state2) {
> pa_alsa_element *e;
>
> PA_LLIST_FOREACH(e, p->elements) {
> @@ -2895,6 +2902,7 @@ finish:
>
> void pa_alsa_path_set_dump(pa_alsa_path_set *ps) {
> pa_alsa_path *p;
> + void *state;
> pa_assert(ps);
>
> pa_log_debug("Path Set %p, direction=%i, probed=%s",
> @@ -2902,7 +2910,7 @@ void pa_alsa_path_set_dump(pa_alsa_path_set *ps) {
> ps->direction,
> pa_yes_no(ps->probed));
>
> - PA_LLIST_FOREACH(p, ps->paths)
> + PA_HASHMAP_FOREACH(p, ps->paths, state)
> pa_alsa_path_dump(p);
> }
>
> @@ -3048,20 +3056,21 @@ static pa_bool_t element_is_subset(pa_alsa_element *a, pa_alsa_element *b, snd_m
> }
>
> static void path_set_condense(pa_alsa_path_set *ps, snd_mixer_t *m) {
> - pa_alsa_path *p, *np;
> + pa_alsa_path *p;
> + void *state;
>
> pa_assert(ps);
> pa_assert(m);
>
> /* If we only have one path, then don't bother */
> - if (!ps->paths || !ps->paths->next)
> + if (pa_hashmap_size(ps->paths) < 2)
> return;
>
> - for (p = ps->paths; p; p = np) {
> + PA_HASHMAP_FOREACH(p, ps->paths, state) {
> pa_alsa_path *p2;
> - np = p->next;
> + void *state2;
>
> - PA_LLIST_FOREACH(p2, ps->paths) {
> + PA_HASHMAP_FOREACH(p2, ps->paths, state2) {
> pa_alsa_element *ea, *eb;
> pa_bool_t is_subset = TRUE;
>
> @@ -3090,24 +3099,33 @@ static void path_set_condense(pa_alsa_path_set *ps, snd_mixer_t *m) {
>
> if (is_subset) {
> pa_log_debug("Removing path '%s' as it is a subset of '%s'.", p->name, p2->name);
> - PA_LLIST_REMOVE(pa_alsa_path, ps->paths, p);
> - pa_alsa_path_free(p);
> + pa_hashmap_remove(ps->paths, p);
What if some path is removed from all path sets? Does it still exist in
the profile set, and if so, will a client-visible port be created for
it?
> break;
> }
> }
> }
> }
>
> +static pa_alsa_path* path_set_find_path_by_name(pa_alsa_path_set *ps, const char* name, pa_alsa_path *ignore)
> +{
> + pa_alsa_path* p;
> + void *state;
> +
> + PA_HASHMAP_FOREACH(p, ps->paths, state)
> + if (p != ignore && pa_streq(p->name, name))
> + return p;
> + return NULL;
> +}
> +
> static void path_set_make_paths_unique(pa_alsa_path_set *ps) {
> pa_alsa_path *p, *q;
> + void *state, *state2;
>
> - PA_LLIST_FOREACH(p, ps->paths) {
> + PA_HASHMAP_FOREACH(p, ps->paths, state) {
> unsigned i;
> char *m;
>
> - for (q = p->next; q; q = q->next)
> - if (pa_streq(q->name, p->name))
> - break;
> + q = path_set_find_path_by_name(ps, p->name, p);
>
> if (!q)
> continue;
> @@ -3115,7 +3133,8 @@ static void path_set_make_paths_unique(pa_alsa_path_set *ps) {
> m = pa_xstrdup(p->name);
>
> /* OK, this name is not unique, hence let's rename */
> - for (i = 1, q = p; q; q = q->next) {
> + i = 1;
> + PA_HASHMAP_FOREACH(q, ps->paths, state2) {
> char *nn, *nd;
>
> if (!pa_streq(q->name, m))
> @@ -3136,34 +3155,6 @@ static void path_set_make_paths_unique(pa_alsa_path_set *ps) {
> }
> }
>
> -void pa_alsa_path_set_probe(pa_alsa_path_set *ps, snd_mixer_t *m, pa_bool_t ignore_dB) {
> - pa_alsa_path *p, *n;
> -
> - pa_assert(ps);
> -
> - if (ps->probed)
> - return;
> -
> - for (p = ps->paths; p; p = n) {
> - n = p->next;
> -
> - if (pa_alsa_path_probe(p, m, ignore_dB) < 0) {
> - PA_LLIST_REMOVE(pa_alsa_path, ps->paths, p);
> - pa_alsa_path_free(p);
> - }
> - }
> -
> - pa_log_debug("Found mixer paths (before tidying):");
> - pa_alsa_path_set_dump(ps);
> -
> - path_set_condense(ps, m);
> - path_set_make_paths_unique(ps);
> - ps->probed = TRUE;
> -
> - pa_log_debug("Available mixer paths (after tidying):");
> - pa_alsa_path_set_dump(ps);
> -}
> -
> static void mapping_free(pa_alsa_mapping *m) {
> pa_assert(m);
>
> @@ -3203,6 +3194,24 @@ static void profile_free(pa_alsa_profile *p) {
> void pa_alsa_profile_set_free(pa_alsa_profile_set *ps) {
> pa_assert(ps);
>
> + if (ps->input_paths) {
> + pa_alsa_path *p;
> +
> + while ((p = pa_hashmap_steal_first(ps->input_paths)))
> + pa_alsa_path_free(p);
> +
> + pa_hashmap_free(ps->input_paths, NULL, NULL);
> + }
> +
> + if (ps->output_paths) {
> + pa_alsa_path *p;
> +
> + while ((p = pa_hashmap_steal_first(ps->output_paths)))
> + pa_alsa_path_free(p);
> +
> + pa_hashmap_free(ps->output_paths, NULL, NULL);
> + }
> +
> if (ps->profiles) {
> pa_alsa_profile *p;
>
> @@ -3688,6 +3697,53 @@ fail:
> return -1;
> }
I'll have to stop now. To be continued...
