[pulseaudio-discuss] [PATCH] stream: Return error in case a client peeks to early
If there is no silence memblock and no data, pa_memblockq_peek can
return NULL. In this case, do not crash on an assertion in
pa_memblock_acquire, but instead return a proper error to the client.
BugLink: http://bugs.launchpad.net/bugs/1058200
Signed-off-by: David Henningsson
---
src/pulse/stream.c |2 ++
1 file changed, 2 insertions(+)
diff --git a/src/pulse/stream.c b/src/pulse/stream.c
index 2b6d306..9bb0995 100644
--- a/src/pulse/stream.c
+++ b/src/pulse/stream.c
@@ -1598,6 +1598,8 @@ int pa_stream_peek(pa_stream *s, const void **data,
size_t *length) {
return 0;
}
+PA_CHECK_VALIDITY(s->context, s->peek_memchunk.memblock != NULL,
PA_ERR_NODATA);
+
s->peek_data = pa_memblock_acquire(s->peek_memchunk.memblock);
}
--
1.7.9.5
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [RFC PATCH] alsa-mixer: Cache failure to open inputs
On 09/25/2012 06:46 PM, Tanu Kaskinen wrote:
On Mon, 2012-09-24 at 17:02 +0200, David Henningsson wrote:
On 09/24/2012 03:33 PM, Tanu Kaskinen wrote:
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.
I certainly prefer the latter, though this is not a very important thing
for me. But I'm now left wondering what you thought we agreed earlier -
unfortunately I don't have the logs, but I think we agreed to disallow
some cases of omitting the braces. If you think that the first example
would still be allowed, I don't know what case would be left to
disallow.
I don't recall for sure, but I think I was imagining multiline
statements, e g
if (i)
this_is_a_very_long_statement(with_multiple, parameters,
that_maybe_are_functions(themselves));
vs
if (i) {
this_is_a_very_long_statement(with_multiple, parameters,
that_maybe_are_functions(themselves));
}
in which case the latter is okay. Anyway, if it's not important, it's
just another trap to fall in when you try to fix somebody's real problem.
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?
I don't think there has been any rule so far, or if there has been, it
has been to favor NULL. I would definitely choose to always initialize
idxsets and hashmaps if there's no particular reason to distinguish
between NULL and an empty container. (I have even sent some patches that
ensure that certain idxsets or hashmaps are always non-NULL, but I don't
remember if they have been reviewed yet.) It makes the code simpler when
you don't have to worry about the container being NULL.
Another option could be to make pa_hashmap_* to treat NULL pointers as
empty containers. I don't know if that's better though.
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] [PATCH v4] alsa-mixer: Cache failure to open inputs/output mappings
I was hoping this would improve bootup speed, but it doesn't seem
to do so here, at least not much. But at least it reduces the logs
a little.
Signed-off-by: David Henningsson
---
Please ignore the just sent v3. It was identical to v2, sorry.
src/modules/alsa/alsa-mixer.c | 54 ++---
1 file changed, 50 insertions(+), 4 deletions(-)
diff --git a/src/modules/alsa/alsa-mixer.c b/src/modules/alsa/alsa-mixer.c
index 8072fbb..ea24880 100644
--- a/src/modules/alsa/alsa-mixer.c
+++ b/src/modules/alsa/alsa-mixer.c
@@ -3920,6 +3920,16 @@ static void profile_set_add_auto(pa_alsa_profile_set
*ps) {
pa_assert(ps);
+/* The order is important here:
+ 1) try single inputs and outputs before trying their
+ combination, because if the half-duplex test failed, we don't have
+ to try full duplex.
+ 2) try the output right before the input combinations with
+ that output, because then the output_pcm is not closed between tests.
+*/
+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 +3937,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 +4301,7 @@ void pa_alsa_profile_set_probe(
void *state;
pa_alsa_profile *p, *last = NULL;
pa_alsa_mapping *m;
+pa_hashmap *broken_inputs, *broken_outputs;
pa_assert(ps);
pa_assert(dev_id);
@@ -4301,18 +4310,43 @@ 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);
+broken_outputs = pa_hashmap_new(pa_idxset_trivial_hash_func,
pa_idxset_trivial_compare_func);
+
PA_HASHMAP_FOREACH(p, ps->profiles, state) {
uint32_t idx;
/* Skip if this is already marked that it is supported (i.e. from the
config file) */
if (!p->supported) {
-pa_log_debug("Looking at profile %s", p->name);
profile_finalize_probing(last, p);
p->supported = TRUE;
+if (p->output_mappings) {
+PA_IDXSET_FOREACH(m, p->output_mappings, idx) {
+if (pa_hashmap_get(broken_outputs, m) == m) {
+pa_log_debug("Skipping profile %s - will not be able
to open output:%s", p->name, m->name);
+p->supported = FALSE;
+break;
+}
+}
+}
+
+if (p->input_mappings && p->supported) {
+PA_IDXSET_FOREACH(m, p->input_mappings, idx) {
+if (pa_hashmap_get(broken_inputs, m) == m) {
+pa_log_debug("Skipping profile %s - will not be able
to open input:%s", p->name, m->name);
+p->supported = FALSE;
+break;
+}
+}
+}
+
+if (p->supported)
+pa_log_debug("Looking at profile %s", p->name);
+
/* Check if we can open all new ones */
-if (p->output_mappings)
+if (p->output_mappings && p->supported)
PA_IDXSET_FOREACH(m, p->output_mappings, idx) {
if (m->output_pcm)
@@ -4324,6 +4358,11 @@ void pa_alsa_profile_set_probe(
default_n_fragments,
default_fragment_size_msec))) {
p->supported = FALSE;
+if (pa_idxset_size(p->output_mappings) == 1 &&
+((!p->input_mappings) ||
pa_idxset_size(p->input_mappings) == 0)) {
+pa_log_debug("Caching failure to open output:%s",
m->name);
+pa_hashmap_put(broken_outputs, m, m);
+}
break;
}
}
@@ -4340,6 +4379,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)) {
+pa_log_debug("Caching failure to open input:%s",
m->name);
+pa_hashmap_put(broken_inputs, m, m);
+}
break;
[pulseaudio-discuss] [PATCH v3] alsa-mixer: Cache failure to open inputs/output mappings
I was hoping this would improve bootup speed, but it doesn't seem
to do so here, at least not much. But at least it reduces the logs
a little.
Signed-off-by: David Henningsson
---
src/modules/alsa/alsa-mixer.c | 43 +
1 file changed, 39 insertions(+), 4 deletions(-)
diff --git a/src/modules/alsa/alsa-mixer.c b/src/modules/alsa/alsa-mixer.c
index 8072fbb..b6ee1a4 100644
--- a/src/modules/alsa/alsa-mixer.c
+++ b/src/modules/alsa/alsa-mixer.c
@@ -3920,6 +3920,16 @@ static void profile_set_add_auto(pa_alsa_profile_set
*ps) {
pa_assert(ps);
+/* The order is important here:
+ 1) try single inputs and outputs before trying their
+ combination, because if the half-duplex test failed, we don't have
+ to try full duplex.
+ 2) try the output right before the input combinations with
+ that output, because then the output_pcm is not closed between tests.
+*/
+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 +3937,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) {
@@ -4290,9 +4298,10 @@ void pa_alsa_profile_set_probe(
unsigned default_n_fragments,
unsigned default_fragment_size_msec) {
-void *state;
+void *state, *state2;
pa_alsa_profile *p, *last = NULL;
pa_alsa_mapping *m;
+pa_hashmap *broken_mappings;
pa_assert(ps);
pa_assert(dev_id);
@@ -4301,6 +4310,8 @@ void pa_alsa_profile_set_probe(
if (ps->probed)
return;
+broken_mappings = pa_hashmap_new(pa_idxset_trivial_hash_func,
pa_idxset_trivial_compare_func);
+
PA_HASHMAP_FOREACH(p, ps->profiles, state) {
uint32_t idx;
@@ -4311,8 +4322,21 @@ void pa_alsa_profile_set_probe(
profile_finalize_probing(last, p);
p->supported = TRUE;
+PA_HASHMAP_FOREACH(m, broken_mappings, state2) {
+const char* inout = NULL;
+if (p->output_mappings &&
pa_idxset_get_by_data(p->output_mappings, m, NULL) == m)
+inout = "output";
+else if (p->input_mappings &&
pa_idxset_get_by_data(p->input_mappings, m, NULL) == m)
+inout = "input";
+if (inout) {
+pa_log_debug("Skipping - will not be able to open %s:%s",
inout, m->name);
+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)
@@ -4324,6 +4348,11 @@ void pa_alsa_profile_set_probe(
default_n_fragments,
default_fragment_size_msec))) {
p->supported = FALSE;
+if (pa_idxset_size(p->output_mappings) == 1 &&
+((!p->input_mappings) ||
pa_idxset_size(p->input_mappings) == 0)) {
+pa_log_debug("Caching failure to open output:%s",
m->name);
+pa_hashmap_put(broken_mappings, m, m);
+}
break;
}
}
@@ -4340,6 +4369,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)) {
+pa_log_debug("Caching failure to open input:%s",
m->name);
+pa_hashmap_put(broken_mappings, m, m);
+}
break;
}
}
@@ -4370,6 +4404,7 @@ void pa_alsa_profile_set_probe(
paths_drop_unsupported(ps->input_paths);
paths_drop_unsupported(ps->output_paths);
+pa_hashmap_free(broken_mappings, NULL, NULL);
ps->probed = TRUE;
}
--
1.7.9.5
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] ARM NEON patches, status
Hello, > I have submitted v3 of my ARM NEON patches July 24 > (http://lists.freedesktop.org/archives/pulseaudio-discuss/2012-July/014226.html) > but not received any feedback any comment on this? thanks, regards, p. -- Peter Meerwald +43-664-218 (mobile) ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v0 01/16] sink, source: Fix missing pa_asyncmsgq_ref()
Hi Tanu, On Sun, Sep 30, 2012 at 9:55 AM, Tanu Kaskinen wrote: > On Fri, 2012-09-28 at 17:45 +0200, Mikel Astiz wrote: >> From: Mikel Astiz >> >> Sinks and sources hold a pointer to asyncmsgq, so they should use the >> reference counting mechanism to make sure this queue is not freed. > > I don't really want to apply this. This shouldn't be needed, so if this > makes some bug unreproducible, it's a case of hiding a programming > error, not really fixing it. As a side comment, note that the rest of the patches in this patchset do not depend on this one, as explained in the cover letter. > > Explanation why this shouldn't be needed: > > The pa_sink.asyncmsgq variable should only be used by the main thread, > and only when the sink is linked. pa_sink.asyncmsgq always points to > pa_thread_mq's inq. pa_thread_mq.inq is created in pa_thread_mq_init() > and unreffed in pa_thread_mq_done(). pa_thread_mq_done() is only called > after the "hardware sink" (the one implementing the IO thread) has been > unlinked. Therefore, at least the hardware sink is fine without any This was exactly the condition that was not met during my testing with the bluetooth modules. As checked in patch v0 11/16, a potential error in start_thread() could lead to this inconsistent state where the sinks/sources were not unlinked (until I realized patch v0 07/16 was needed). This was obviously a bug, but such reference counting would have come handy to debug the problem. > extra pa_asyncmsgq_ref() calls. What about filter sinks then? Can they > be linked while the hardware sink is not, therefore maybe allowing the > filter sink's asyncmsgq to be used while the underlying pa_thread_mq is > gone? No. The filter sinks are unlinked at the same time when the > hardware sink is unlinked. So, pa_sink.asyncmsgq will always have a > reference when it's needed. No need to call pa_asyncmsgq_ref() in > pa_sink_set_asyncmsgq(). What about checking the reference counter pa_thread_mq_done()? If I got this right, we would expect that the reference count is 1 at that point, so we could add an assertion. If there is some sink/source that hasn't been unlinked, the assertion would fail. Would this check be valid for the rest of the code using pa_asyncmsgq_ref()? Cheers, Mikel ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
