[pulseaudio-discuss] [PATCH 6/6] Alsa: add card ports and path probe cache

[Tanu Kaskinen]

Hi,

Some more comments on this last patch. Still not finished, but I've got
to go to sleep...

I got a bit confused about the path naming, or more specifically about
the uniqueness of the path names. I didn't have time to double check,
but it seemed like in the old system there was no reason to have the
path_set_make_paths_unique() function at all, because it looked like the
path names would be unique anyway (within one path set). I'm pretty sure
that I've missed something...

Since same paths are used for multiple mappings, in the new system there
clearly will be duplicate path names for one card. But if the only
reason for that is that different mappings reuse the same path
configuration files, I think it's not a good idea to just append a
number at the end of the path description that will be shown in UIs. I
think the path descriptions could incorporate the mapping description or
something, instead of using just numbers to tell the paths apart... But
I didn't have the time to double check this, so I'm probably missing
something.

On Thu, 2011-10-27 at 16:45 +0200, David Henningsson wrote:
> +static void mapping_paths_probe(pa_alsa_mapping *m, pa_alsa_profile *profile,
> +                                pa_alsa_direction_t direction) {
> +
> +    pa_alsa_path *p;
> +    void *state;
> +    snd_pcm_t *pcm_handle;
> +    pa_alsa_path_set *ps;
> +    snd_mixer_t *mixer_handle;
> +
> +    if (direction == PA_ALSA_DIRECTION_OUTPUT) {
> +        if (m->output_path_set)
> +            return; /* Already probed */
> +        m->output_path_set = ps = pa_alsa_path_set_new(m, direction, NULL); /* FIXME: Handle paths_dir */

The path set should be created already at configuration parsing time.
There shouldn't be need to create anything at probe time - I think the
purpose of probing is just to remove those paths that are not available.

Maybe doing the path set initialization sooner would also help with
paths_dir... or maybe not, I don't know. I didn't do the work to figure
out where exactly the path set should be created.

> @@ -248,6 +241,8 @@ struct pa_alsa_mapping {
>      /* Temporarily used during probing */
>      snd_pcm_t *input_pcm;
>      snd_pcm_t *output_pcm;
> +    pa_alsa_path_set *input_path_set;
> +    pa_alsa_path_set *output_path_set;

These appear to be used after probing also, so they shouldn't be under
the "Temporarily used during probing" comment.

>  
>      pa_sink *sink;
>      pa_source *source;
> @@ -289,8 +284,11 @@ struct pa_alsa_profile_set {
>      pa_hashmap *mappings;
>      pa_hashmap *profiles;
>      pa_hashmap *decibel_fixes;
> +    pa_hashmap *input_paths;
> +    pa_hashmap *output_paths;

Are separate hashmaps really needed for input and output? I didn't
notice that at least this patch would have any need to handle them
separately. Separate hashmaps make it possible to have an input path and
an output path with the same name, and I wouldn't like to allow that
unless there's a good reason to do so.

-- 
Tanu