[pulseaudio-discuss] [RFC PATCH] alsa-mixer: Cache failure to open inputs

[David Henningsson]

On 09/23/2012 07:09 PM, Tanu Kaskinen wrote:
> On Fri, 2012-09-21 at 10:37 +0200, David Henningsson wrote:
>> We spend most of our startup time probing soundcards, so I was hoping this
>> would improve bootup speed, but it doesn't seem to do so here.
>> Do we want this anyway? At least it reduces the logs a little.
>>
>> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
>> ---
>>   src/modules/alsa/alsa-mixer.c |   25 ++++++++++++++++++++++---
>>   1 file changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/modules/alsa/alsa-mixer.c b/src/modules/alsa/alsa-mixer.c
>> index 8072fbb..1cf502d 100644
>> --- a/src/modules/alsa/alsa-mixer.c
>> +++ b/src/modules/alsa/alsa-mixer.c
>> @@ -3920,6 +3920,10 @@ static void profile_set_add_auto(pa_alsa_profile_set *ps) {
>>
>>       pa_assert(ps);
>>
>> +    /* Try input only first, for caching */
>
> This comment could be more explicit in explaining that for the caching
> to be useful, we depend on the fact that pa_hashmap iteration happens in
> the order in which the profiles were created. This reordering seemed
> nonsensical before I checked how the iteration is implemented.

Ok, I could add more explanation in the comment.

> I think it would make sense to also create all the output-only profiles
> before the combinations.

It is actually an optimisation to have it as it is - the output_pcm is 
less likely to be closed between (m, NULL) and (m, n) this way.

>> +    PA_HASHMAP_FOREACH(n, ps->mappings, n_state)
>> +        profile_set_add_auto_pair(ps, NULL, n);
>> +
>>       PA_HASHMAP_FOREACH(m, ps->mappings, m_state) {
>>           profile_set_add_auto_pair(ps, m, NULL);
>>
>> @@ -3927,8 +3931,6 @@ static void profile_set_add_auto(pa_alsa_profile_set *ps) {
>>               profile_set_add_auto_pair(ps, m, n);
>>       }
>>
>> -    PA_HASHMAP_FOREACH(n, ps->mappings, n_state)
>> -        profile_set_add_auto_pair(ps, NULL, n);
>>   }
>>
>>   static int profile_verify(pa_alsa_profile *p) {
>> @@ -4293,6 +4295,7 @@ void pa_alsa_profile_set_probe(
>>       void *state;
>>       pa_alsa_profile *p, *last = NULL;
>>       pa_alsa_mapping *m;
>> +    pa_hashmap *broken_inputs;
>>
>>       pa_assert(ps);
>>       pa_assert(dev_id);
>> @@ -4301,6 +4304,8 @@ void pa_alsa_profile_set_probe(
>>       if (ps->probed)
>>           return;
>>
>> +    broken_inputs = pa_hashmap_new(pa_idxset_trivial_hash_func, pa_idxset_trivial_compare_func);
>
> Wouldn't it make sense to have broken_outputs too?

Given the current ordering in the above function, I would say no.

>> +
>>       PA_HASHMAP_FOREACH(p, ps->profiles, state) {
>>           uint32_t idx;
>>
>> @@ -4311,8 +4316,16 @@ void pa_alsa_profile_set_probe(
>>               profile_finalize_probing(last, p);
>>               p->supported = TRUE;
>>
>> +            if (p->input_mappings)
>> +                PA_IDXSET_FOREACH(m, p->input_mappings, idx)
>
> I see that it's still not codified in the coding style document, but
> didn't we agree a while ago in IRC that for multiline block contents
> braces should be always used, even if they are not strictly necessary?

When we discussed it on IRC, it didn't occur to me that it would strike 
against simple nested loops. Can I take it back? I think

for (i)
   for (j)
      if (a[i] == b[j])
        match_found();

...looks much better than

for (i) {
   for (j) {
      if (a[i] == b[j])
        match_found();
   }
}

>> +                    if (pa_hashmap_get(broken_inputs, m) == m) {
>> +                        pa_log_debug("Skipping - will not be able to open input:%s", m->name);
>
> A space would be nice before %s.

the profiles are made up using e g
output:hdmi-stereo+input:analog-stereo, so this reflects the 
"input:analog-stereo" construct we already use for profile naming.

>> +                        p->supported = FALSE;
>> +                        break;
>> +                    }
>> +
>>               /* Check if we can open all new ones */
>> -            if (p->output_mappings)
>> +            if (p->supported && p->output_mappings)
>>                   PA_IDXSET_FOREACH(m, p->output_mappings, idx) {
>>
>>                       if (m->output_pcm)
>> @@ -4340,6 +4353,11 @@ void pa_alsa_profile_set_probe(
>>                                                             default_n_fragments,
>>                                                             default_fragment_size_msec))) {
>>                           p->supported = FALSE;
>> +                        if (pa_idxset_size(p->input_mappings) == 1 &&
>> +                            ((!p->output_mappings) || pa_idxset_size(p->output_mappings) == 0)) {
>
> As far as I can see, p->output_mappings can never have zero size.

Ok. Well, better safe than sorry?


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