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 <[email protected]>
---
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
_______________________________________________
pulseaudio-discuss mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
