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

[David Henningsson]

On 11/10/2011 09:34 PM, Tanu Kaskinen wrote:
> Hi,
>
> Some more comments on this last patch. Still not finished, but I've got
> to go to sleep...

I hope I'm not driving you folks too hard :-/

> 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...

Two different path files can have the same name, as "name" refers to the 
"Name" key in the "General" section, not the file name. Does that clear 
things up?

> Since same paths are used for multiple mappings, in the new system there
> clearly will be duplicate path names for one card.

But this is the same path object, used by more than one mapping. So the 
duplicates should not increase.

> 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.

It is a slight optimisation to create it here, this way we only create 
path sets for profiles that are supported.

> 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.

Hmm, and they don't seem to freed correctly either? Needs fixing.

>>       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.

Hmm, good question. I think I just kept it that way to not regress 
anything I didn't think of. If we merge them, I think I at least need to 
check that someone isn't trying to use the same path for input and output.

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic