[pulseaudio-commits] 3 commits - src/modules src/pulsecore
Tanu Kaskinen
tanuk at kemper.freedesktop.org
Mon Dec 19 23:32:37 UTC 2016
src/modules/bluetooth/module-bluetooth-policy.c | 6 ++--
src/modules/module-suspend-on-idle.c | 34 ++++++++++++------------
src/pulsecore/sink-input.c | 10 +++----
src/pulsecore/sink.c | 7 +++-
src/pulsecore/sink.h | 9 +++++-
src/pulsecore/source-output.c | 10 +++----
src/pulsecore/source.c | 5 ++-
src/pulsecore/source.h | 6 +++-
8 files changed, 52 insertions(+), 35 deletions(-)
New commits:
commit 74ff1153425c927e6dfd198cbf1e0d6edfbfa873
Author: Tanu Kaskinen <tanuk at iki.fi>
Date: Wed Oct 12 17:20:41 2016 +0300
sink-input, source-output: set sink/source to NULL before the "unlink post" hook
At the time the "unlink post" hook is fired, the stream is not any more
connected to its old device, so it makes sense to reset the sink/source
pointer to NULL before firing the hook. If this is not done, the pointer
may become stale during the "unlink post" hook, because
module-bluetooth-policy does a card profile change in its "unlink post"
callback, so even if the pointer is valid when module-bluetooth-policy's
callback is called, it will be invalid in subsequent callbacks.
diff --git a/src/pulsecore/sink-input.c b/src/pulsecore/sink-input.c
index 0dda204..c248cd8 100644
--- a/src/pulsecore/sink-input.c
+++ b/src/pulsecore/sink-input.c
@@ -697,11 +697,6 @@ void pa_sink_input_unlink(pa_sink_input *i) {
reset_callbacks(i);
- if (linked) {
- pa_subscription_post(i->core, PA_SUBSCRIPTION_EVENT_SINK_INPUT|PA_SUBSCRIPTION_EVENT_REMOVE, i->index);
- pa_hook_fire(&i->core->hooks[PA_CORE_HOOK_SINK_INPUT_UNLINK_POST], i);
- }
-
if (i->sink) {
if (PA_SINK_IS_LINKED(pa_sink_get_state(i->sink)))
pa_sink_update_status(i->sink);
@@ -709,6 +704,11 @@ void pa_sink_input_unlink(pa_sink_input *i) {
i->sink = NULL;
}
+ if (linked) {
+ pa_subscription_post(i->core, PA_SUBSCRIPTION_EVENT_SINK_INPUT|PA_SUBSCRIPTION_EVENT_REMOVE, i->index);
+ pa_hook_fire(&i->core->hooks[PA_CORE_HOOK_SINK_INPUT_UNLINK_POST], i);
+ }
+
pa_core_maybe_vacuum(i->core);
pa_sink_input_unref(i);
diff --git a/src/pulsecore/source-output.c b/src/pulsecore/source-output.c
index 35ef1c5..3c421ad 100644
--- a/src/pulsecore/source-output.c
+++ b/src/pulsecore/source-output.c
@@ -591,11 +591,6 @@ void pa_source_output_unlink(pa_source_output*o) {
reset_callbacks(o);
- if (linked) {
- pa_subscription_post(o->core, PA_SUBSCRIPTION_EVENT_SOURCE_OUTPUT|PA_SUBSCRIPTION_EVENT_REMOVE, o->index);
- pa_hook_fire(&o->core->hooks[PA_CORE_HOOK_SOURCE_OUTPUT_UNLINK_POST], o);
- }
-
if (o->source) {
if (PA_SOURCE_IS_LINKED(pa_source_get_state(o->source)))
pa_source_update_status(o->source);
@@ -603,6 +598,11 @@ void pa_source_output_unlink(pa_source_output*o) {
o->source = NULL;
}
+ if (linked) {
+ pa_subscription_post(o->core, PA_SUBSCRIPTION_EVENT_SOURCE_OUTPUT|PA_SUBSCRIPTION_EVENT_REMOVE, o->index);
+ pa_hook_fire(&o->core->hooks[PA_CORE_HOOK_SOURCE_OUTPUT_UNLINK_POST], o);
+ }
+
pa_core_maybe_vacuum(o->core);
pa_source_output_unref(o);
commit c3393d27a5b18bca3e052d54a734d5cfb9e0aea0
Author: Tanu Kaskinen <tanuk at iki.fi>
Date: Wed Oct 12 17:20:40 2016 +0300
suspend-on-idle: use earlier (safer) hooks for stream unlink notifications
In the "unlink post" hook it's not guaranteed that the stream's old
device exists any more, so let's use the "unlink" hook that is safer.
For example, module-bluetooth-policy does a card profile change in the
source-output "unlink post" hook, which invalidates the source-output's
source pointer.
When the "unlink" hook is fired, the stream is still linked to its
device, which affects the return values of the check_suspend()
functions. The unlinked streams should be ignored by the check_suspend()
functions, so I had to add extra parameters to those functions.
diff --git a/src/modules/module-suspend-on-idle.c b/src/modules/module-suspend-on-idle.c
index f7620db..a284f85 100644
--- a/src/modules/module-suspend-on-idle.c
+++ b/src/modules/module-suspend-on-idle.c
@@ -67,13 +67,13 @@ static void timeout_cb(pa_mainloop_api*a, pa_time_event* e, const struct timeval
d->userdata->core->mainloop->time_restart(d->time_event, NULL);
- if (d->sink && pa_sink_check_suspend(d->sink) <= 0 && !(d->sink->suspend_cause & PA_SUSPEND_IDLE)) {
+ if (d->sink && pa_sink_check_suspend(d->sink, NULL, NULL) <= 0 && !(d->sink->suspend_cause & PA_SUSPEND_IDLE)) {
pa_log_info("Sink %s idle for too long, suspending ...", d->sink->name);
pa_sink_suspend(d->sink, true, PA_SUSPEND_IDLE);
pa_core_maybe_vacuum(d->userdata->core);
}
- if (d->source && pa_source_check_suspend(d->source) <= 0 && !(d->source->suspend_cause & PA_SUSPEND_IDLE)) {
+ if (d->source && pa_source_check_suspend(d->source, NULL) <= 0 && !(d->source->suspend_cause & PA_SUSPEND_IDLE)) {
pa_log_info("Source %s idle for too long, suspending ...", d->source->name);
pa_source_suspend(d->source, true, PA_SUSPEND_IDLE);
pa_core_maybe_vacuum(d->userdata->core);
@@ -125,7 +125,7 @@ static pa_hook_result_t sink_input_fixate_hook_cb(pa_core *c, pa_sink_input_new_
if ((d = pa_hashmap_get(u->device_infos, data->sink))) {
resume(d);
- if (pa_sink_check_suspend(d->sink) <= 0)
+ if (pa_sink_check_suspend(d->sink, NULL, NULL) <= 0)
restart(d);
}
@@ -147,12 +147,12 @@ static pa_hook_result_t source_output_fixate_hook_cb(pa_core *c, pa_source_outpu
if (d) {
resume(d);
if (d->source) {
- if (pa_source_check_suspend(d->source) <= 0)
+ if (pa_source_check_suspend(d->source, NULL) <= 0)
restart(d);
} else {
/* The source output is connected to a monitor source. */
pa_assert(d->sink);
- if (pa_sink_check_suspend(d->sink) <= 0)
+ if (pa_sink_check_suspend(d->sink, NULL, NULL) <= 0)
restart(d);
}
}
@@ -168,7 +168,7 @@ static pa_hook_result_t sink_input_unlink_hook_cb(pa_core *c, pa_sink_input *s,
if (!s->sink)
return PA_HOOK_OK;
- if (pa_sink_check_suspend(s->sink) <= 0) {
+ if (pa_sink_check_suspend(s->sink, s, NULL) <= 0) {
struct device_info *d;
if ((d = pa_hashmap_get(u->device_infos, s->sink)))
restart(d);
@@ -188,10 +188,10 @@ static pa_hook_result_t source_output_unlink_hook_cb(pa_core *c, pa_source_outpu
return PA_HOOK_OK;
if (s->source->monitor_of) {
- if (pa_sink_check_suspend(s->source->monitor_of) <= 0)
+ if (pa_sink_check_suspend(s->source->monitor_of, NULL, s) <= 0)
d = pa_hashmap_get(u->device_infos, s->source->monitor_of);
} else {
- if (pa_source_check_suspend(s->source) <= 0)
+ if (pa_source_check_suspend(s->source, s) <= 0)
d = pa_hashmap_get(u->device_infos, s->source);
}
@@ -208,7 +208,7 @@ static pa_hook_result_t sink_input_move_start_hook_cb(pa_core *c, pa_sink_input
pa_sink_input_assert_ref(s);
pa_assert(u);
- if (pa_sink_check_suspend(s->sink) <= 1)
+ if (pa_sink_check_suspend(s->sink, s, NULL) <= 0)
if ((d = pa_hashmap_get(u->device_infos, s->sink)))
restart(d);
@@ -241,10 +241,10 @@ static pa_hook_result_t source_output_move_start_hook_cb(pa_core *c, pa_source_o
pa_assert(u);
if (s->source->monitor_of) {
- if (pa_sink_check_suspend(s->source->monitor_of) <= 1)
+ if (pa_sink_check_suspend(s->source->monitor_of, NULL, s) <= 0)
d = pa_hashmap_get(u->device_infos, s->source->monitor_of);
} else {
- if (pa_source_check_suspend(s->source) <= 1)
+ if (pa_source_check_suspend(s->source, s) <= 0)
d = pa_hashmap_get(u->device_infos, s->source);
}
@@ -354,8 +354,8 @@ static pa_hook_result_t device_new_hook_cb(pa_core *c, pa_object *o, struct user
pa_hashmap_put(u->device_infos, o, d);
- if ((d->sink && pa_sink_check_suspend(d->sink) <= 0) ||
- (d->source && pa_source_check_suspend(d->source) <= 0))
+ if ((d->sink && pa_sink_check_suspend(d->sink, NULL, NULL) <= 0) ||
+ (d->source && pa_source_check_suspend(d->source, NULL) <= 0))
restart(d);
return PA_HOOK_OK;
@@ -398,7 +398,7 @@ static pa_hook_result_t device_state_changed_hook_cb(pa_core *c, pa_object *o, s
pa_sink *s = PA_SINK(o);
pa_sink_state_t state = pa_sink_get_state(s);
- if (pa_sink_check_suspend(s) <= 0)
+ if (pa_sink_check_suspend(s, NULL, NULL) <= 0)
if (PA_SINK_IS_OPENED(state))
restart(d);
@@ -406,7 +406,7 @@ static pa_hook_result_t device_state_changed_hook_cb(pa_core *c, pa_object *o, s
pa_source *s = PA_SOURCE(o);
pa_source_state_t state = pa_source_get_state(s);
- if (pa_source_check_suspend(s) <= 0)
+ if (pa_source_check_suspend(s, NULL) <= 0)
if (PA_SOURCE_IS_OPENED(state))
restart(d);
}
@@ -454,8 +454,8 @@ int pa__init(pa_module*m) {
pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SINK_INPUT_FIXATE], PA_HOOK_NORMAL, (pa_hook_cb_t) sink_input_fixate_hook_cb, u);
pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SOURCE_OUTPUT_FIXATE], PA_HOOK_NORMAL, (pa_hook_cb_t) source_output_fixate_hook_cb, u);
- pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SINK_INPUT_UNLINK_POST], PA_HOOK_NORMAL, (pa_hook_cb_t) sink_input_unlink_hook_cb, u);
- pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SOURCE_OUTPUT_UNLINK_POST], PA_HOOK_NORMAL, (pa_hook_cb_t) source_output_unlink_hook_cb, u);
+ pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SINK_INPUT_UNLINK], PA_HOOK_NORMAL, (pa_hook_cb_t) sink_input_unlink_hook_cb, u);
+ pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SOURCE_OUTPUT_UNLINK], PA_HOOK_NORMAL, (pa_hook_cb_t) source_output_unlink_hook_cb, u);
pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SINK_INPUT_MOVE_START], PA_HOOK_NORMAL, (pa_hook_cb_t) sink_input_move_start_hook_cb, u);
pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SOURCE_OUTPUT_MOVE_START], PA_HOOK_NORMAL, (pa_hook_cb_t) source_output_move_start_hook_cb, u);
pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SINK_INPUT_MOVE_FINISH], PA_HOOK_NORMAL, (pa_hook_cb_t) sink_input_move_finish_hook_cb, u);
diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
index 5c6a9c6..a745f09 100644
--- a/src/pulsecore/sink.c
+++ b/src/pulsecore/sink.c
@@ -2381,7 +2381,7 @@ unsigned pa_sink_used_by(pa_sink *s) {
}
/* Called from main thread */
-unsigned pa_sink_check_suspend(pa_sink *s) {
+unsigned pa_sink_check_suspend(pa_sink *s, pa_sink_input *ignore_input, pa_source_output *ignore_output) {
unsigned ret;
pa_sink_input *i;
uint32_t idx;
@@ -2397,6 +2397,9 @@ unsigned pa_sink_check_suspend(pa_sink *s) {
PA_IDXSET_FOREACH(i, s->inputs, idx) {
pa_sink_input_state_t st;
+ if (i == ignore_input)
+ continue;
+
st = pa_sink_input_get_state(i);
/* We do not assert here. It is perfectly valid for a sink input to
@@ -2417,7 +2420,7 @@ unsigned pa_sink_check_suspend(pa_sink *s) {
}
if (s->monitor_source)
- ret += pa_source_check_suspend(s->monitor_source);
+ ret += pa_source_check_suspend(s->monitor_source, ignore_output);
return ret;
}
diff --git a/src/pulsecore/sink.h b/src/pulsecore/sink.h
index c549869..bc2a200 100644
--- a/src/pulsecore/sink.h
+++ b/src/pulsecore/sink.h
@@ -466,7 +466,14 @@ void pa_sink_set_mixer_dirty(pa_sink *s, bool is_dirty);
unsigned pa_sink_linked_by(pa_sink *s); /* Number of connected streams */
unsigned pa_sink_used_by(pa_sink *s); /* Number of connected streams which are not corked */
-unsigned pa_sink_check_suspend(pa_sink *s); /* Returns how many streams are active that don't allow suspensions */
+
+/* Returns how many streams are active that don't allow suspensions. If
+ * "ignore_input" or "ignore_output" is non-NULL, that stream is not included
+ * in the count (the returned count includes the value from
+ * pa_source_check_suspend(), which is called for the monitor source, so that's
+ * why "ignore_output" may be relevant). */
+unsigned pa_sink_check_suspend(pa_sink *s, pa_sink_input *ignore_input, pa_source_output *ignore_output);
+
#define pa_sink_get_state(s) ((s)->state)
/* Moves all inputs away, and stores them in pa_queue */
diff --git a/src/pulsecore/source.c b/src/pulsecore/source.c
index 84f8428..4615702 100644
--- a/src/pulsecore/source.c
+++ b/src/pulsecore/source.c
@@ -1943,7 +1943,7 @@ unsigned pa_source_used_by(pa_source *s) {
}
/* Called from main thread */
-unsigned pa_source_check_suspend(pa_source *s) {
+unsigned pa_source_check_suspend(pa_source *s, pa_source_output *ignore) {
unsigned ret;
pa_source_output *o;
uint32_t idx;
@@ -1959,6 +1959,9 @@ unsigned pa_source_check_suspend(pa_source *s) {
PA_IDXSET_FOREACH(o, s->outputs, idx) {
pa_source_output_state_t st;
+ if (o == ignore)
+ continue;
+
st = pa_source_output_get_state(o);
/* We do not assert here. It is perfectly valid for a source output to
diff --git a/src/pulsecore/source.h b/src/pulsecore/source.h
index 3a6b5c3..e5b9333 100644
--- a/src/pulsecore/source.h
+++ b/src/pulsecore/source.h
@@ -400,7 +400,11 @@ int pa_source_update_rate(pa_source *s, uint32_t rate, bool passthrough);
unsigned pa_source_linked_by(pa_source *s); /* Number of connected streams */
unsigned pa_source_used_by(pa_source *s); /* Number of connected streams that are not corked */
-unsigned pa_source_check_suspend(pa_source *s); /* Returns how many streams are active that don't allow suspensions */
+
+/* Returns how many streams are active that don't allow suspensions. If
+ * "ignore" is non-NULL, that stream is not included in the count. */
+unsigned pa_source_check_suspend(pa_source *s, pa_source_output *ignore);
+
#define pa_source_get_state(s) ((pa_source_state_t) (s)->state)
/* Moves all inputs away, and stores them in pa_queue */
commit 2250dbfd6968bd9ce295fc7bef8595b2c6ef6a44
Author: Tanu Kaskinen <tanuk at iki.fi>
Date: Wed Oct 12 17:20:39 2016 +0300
bluetooth-policy: do A2DP profile restoring a bit later
This fixes a crash that happens if the bluetooth headset is the only
non-monitor source in the system and the last "phone" stream dies.
When the stream dies, the native protocol calls pa_source_output_unlink()
and would call pa_source_output_unref() next, but without this patch,
things happen during the unlinking, and the unreffing ends up being
performed on a stream that is already freed.
pa_source_output_unlink() fires the "unlink" hook before doing anything
else. module-bluetooth-policy then switches the headset profile from HSP
to A2DP within that hook. The HSP source gets removed, and at this point
the dying stream is still connected to it, and needs to be rescued.
Rescuing fails, because there are no other sources in the system, so the
stream gets killed. The native protocol has a kill callback, which again
calls pa_source_output_unlink() and pa_source_output_unref(). This is
the point where the native protocol drops its own reference to the
stream, but another unref call is waiting to be executed once we return
from the original unlink call.
I first tried to avoid the double unreffing by making it safe to do
unlinking recursively, but I found out that there's code that assumes
that once unlink() returns, unlinking has actually occurred (a
reasonable assumption), and at least with my implementation this was not
guaranteed. I now think that we must avoid situations where unlinking
happens recursively. It's just too hairy to deal with. This patch moves
the bluetooth profile switch to happen at a time when the dead stream
isn't any more connected to the source, so it doesn't have to be
rescued or killed.
BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=97906
diff --git a/src/modules/bluetooth/module-bluetooth-policy.c b/src/modules/bluetooth/module-bluetooth-policy.c
index e62a114..df702cc 100644
--- a/src/modules/bluetooth/module-bluetooth-policy.c
+++ b/src/modules/bluetooth/module-bluetooth-policy.c
@@ -267,8 +267,8 @@ static pa_hook_result_t source_output_unlink_hook_callback(pa_core *c, pa_source
if (ignore_output(source_output))
return PA_HOOK_OK;
- /* If there are still some source outputs do nothing (count is with *this* source_output, so +1) */
- if (source_output_count(c) > 1)
+ /* If there are still some source outputs do nothing. */
+ if (source_output_count(c) > 0)
return PA_HOOK_OK;
switch_profile_all(c->cards, true, userdata);
@@ -439,7 +439,7 @@ int pa__init(pa_module *m) {
u->source_output_put_slot = pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_SOURCE_OUTPUT_PUT], PA_HOOK_NORMAL,
(pa_hook_cb_t) source_output_put_hook_callback, u);
- u->source_output_unlink_slot = pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_SOURCE_OUTPUT_UNLINK], PA_HOOK_NORMAL,
+ u->source_output_unlink_slot = pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_SOURCE_OUTPUT_UNLINK_POST], PA_HOOK_NORMAL,
(pa_hook_cb_t) source_output_unlink_hook_callback, u);
u->card_init_profile_slot = pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_CARD_CHOOSE_INITIAL_PROFILE], PA_HOOK_NORMAL,
More information about the pulseaudio-commits
mailing list