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

[Tanu Kaskinen]

On Sun, 2012-09-23 at 19:27 +0200, David Henningsson wrote:
> 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.

Ok. Maybe a comment about this too?

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

Let's say that we have a broken output. First it's tried with a profile
that only contains that output. Then it's tried with all working inputs
(broken inputs will be filtered out at this point). We could skip all
combination profiles if the output is found broken.

> >> +
> >>       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();
>    }
> }

Ok, I don't have a problem with this example. But the code in your patch
has different structure: instead of a simple match_found() call, the
inner code contains a three-line block.

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

Right, sorry for not paying better attention.

> >> +                        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?

Fair enough. I don't like the check, because it implies that it's
possible that output_mappings is empty, which is not true, but I see how
this can prevent a bug in the future (not a severe bug, because only a
minor optimization would be skipped, but a bug anyway). I think it would
be best to always create the output_mappings idxset so that the code
doesn't have to worry about it being NULL. If you agree, I'll file a
bug, since the issue is separate from this patch and I don't plan to
write the patch right away, nor ask you to do so.

-- 
Tanu