On 09/24/2012 03:33 PM, Tanu Kaskinen wrote:
> 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?
Fixed in v2.
>
>>>> + 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.
I was confusing profiles and mappings. Good finding. Fixed in v2.
>
>>>> +
>>>> 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.
Well, I still think
for (i)
for (j)
if (a[i] == b[j]) {
three();
line();
block();
}
...looks better than
for (i)
for (j)
if (a[i] == b[j]) {
three();
line();
block();
}
}
}
...but maybe that's just me.
>>>> + 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.
I don't know if we have a generic opinion on whether we prefer NULL
idxsets or empty idxsets? Or determine on a case-by-case basis?
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
