[pulseaudio-discuss] [PATCH v2 5/9] Remove refcounting from ports, sinks and sources
Tanu Kaskinen
tanuk at iki.fi
Wed Feb 20 10:24:00 PST 2013
The refcounting is not completely removed from sinks and sources,
because they are still subclasses of pa_object, which implies
refcounting, but the only ones that have a reference to a sink or
source are the sink and source implementors, so in practice there's no
recounting being used. (Actually, pa_asyncmsgq also holds a reference
to a sink or source when the IO thread sends a message to the sink or
source. To avoid problems with this, the messages have to be
dispatched using pa_thread_mq_done() after shutting down the IO
thread, but before unreffing the sink or source. The current code base
doesn't have problems with this.)
This should fix an issue reported by Luiz von Dentz: freeing the
profiles hashmap of pa_device_port accessed already freed memory,
because the hashmap uses the string stored at pa_card_profile.name
as the key, and the profiles are freed when cards are freed. Card
ports might not be freed at that time, because they are (were)
refcounted.
I first tried to fix this by removing the profiles from
pa_device_port.profiles when the profiles are removed, but there are
more things that would have to be updated too on a port when it's not
connected to a card anymore, notably the card pointer. It looked like
the card pointer could not be safely set to NULL without a significant
amount of work, and since I dislike refcounting, I decided to spend
the effort on removing the refcounting from ports instead. That
required removing it also from sinks and sources.
Ports are now owned solely by cards. Anyone who holds a pointer to
a port should use the card unlink hook to get rid of that pointer
before the port disappears. (The current code base shouldn't have any
issues with this.) Similarly, sinks and sources are now owned solely
by their implementers, so the unlink hooks are essential when dealing
with sinks and sources too.
A note about the bluetooth "SCO over PCM" mode: now that the SCO sink
and source aren't referenced anymore, module-bluetooth-device can end
up holding stale pointers in case the SCO sink or source disappears.
I decided not to fix this, because the "SCO over PCM" mode has never
worked properly if the SCO sink or source can disappear at runtime. On
the systems that I know are using the "SCO over PCM" mode, the SCO
devices stay loaded all the time. If the stale pointers are an
unacceptable bug, I can try to fix it.
---
src/modules/alsa/alsa-mixer.c | 4 +---
src/modules/alsa/alsa-ucm.c | 4 +---
src/modules/bluetooth/module-bluetooth-device.c | 12 ++++++------
src/modules/dbus/iface-device.c | 13 +++++--------
src/modules/module-suspend-on-idle.c | 9 ++-------
src/pulsecore/card.c | 18 +++++++++++++-----
src/pulsecore/device-port.c | 20 +++-----------------
src/pulsecore/device-port.h | 5 +----
src/pulsecore/sink.c | 20 ++------------------
src/pulsecore/source.c | 4 ++--
10 files changed, 36 insertions(+), 73 deletions(-)
diff --git a/src/modules/alsa/alsa-mixer.c b/src/modules/alsa/alsa-mixer.c
index 137c9eb..22dc39e 100644
--- a/src/modules/alsa/alsa-mixer.c
+++ b/src/modules/alsa/alsa-mixer.c
@@ -4476,10 +4476,8 @@ static pa_device_port* device_port_alsa_init(pa_hashmap *ports,
if (cp)
pa_hashmap_put(p->profiles, cp->name, cp);
- if (extra) {
+ if (extra)
pa_hashmap_put(extra, p->name, p);
- pa_device_port_ref(p);
- }
return p;
}
diff --git a/src/modules/alsa/alsa-ucm.c b/src/modules/alsa/alsa-ucm.c
index f69ee89..6e37ae2 100644
--- a/src/modules/alsa/alsa-ucm.c
+++ b/src/modules/alsa/alsa-ucm.c
@@ -713,10 +713,8 @@ static void ucm_add_port_combination(
pa_hashmap_put(port->profiles, cp->name, cp);
}
- if (hash) {
+ if (hash)
pa_hashmap_put(hash, port->name, port);
- pa_device_port_ref(port);
- }
}
static int ucm_port_contains(const char *port_name, const char *dev_name, bool is_sink) {
diff --git a/src/modules/bluetooth/module-bluetooth-device.c b/src/modules/bluetooth/module-bluetooth-device.c
index e04780b..3b729bb 100644
--- a/src/modules/bluetooth/module-bluetooth-device.c
+++ b/src/modules/bluetooth/module-bluetooth-device.c
@@ -1518,13 +1518,11 @@ static void connect_ports(struct userdata *u, void *sink_or_source_new_data, pa_
pa_assert_se(port = pa_hashmap_get(u->card->ports, "bluetooth-output"));
pa_assert_se(pa_hashmap_put(sink_new_data->ports, port->name, port) >= 0);
- pa_device_port_ref(port);
} else {
pa_source_new_data *source_new_data = sink_or_source_new_data;
pa_assert_se(port = pa_hashmap_get(u->card->ports, "bluetooth-input"));
pa_assert_se(pa_hashmap_put(source_new_data->ports, port->name, port) >= 0);
- pa_device_port_ref(port);
}
}
@@ -1917,7 +1915,9 @@ static void stop_thread(struct userdata *u) {
pa_xfree(k);
}
- pa_sink_unref(u->sink);
+ if (!USE_SCO_OVER_PCM(u))
+ pa_sink_unref(u->sink);
+
u->sink = NULL;
}
@@ -1928,7 +1928,9 @@ static void stop_thread(struct userdata *u) {
pa_xfree(k);
}
- pa_source_unref(u->source);
+ if (!USE_SCO_OVER_PCM(u))
+ pa_source_unref(u->source);
+
u->source = NULL;
}
@@ -1967,8 +1969,6 @@ static int start_thread(struct userdata *u) {
return -1;
}
- pa_sink_ref(u->sink);
- pa_source_ref(u->source);
/* FIXME: monitor stream_fd error */
return 0;
}
diff --git a/src/modules/dbus/iface-device.c b/src/modules/dbus/iface-device.c
index 888d407..bc8255f 100644
--- a/src/modules/dbus/iface-device.c
+++ b/src/modules/dbus/iface-device.c
@@ -1208,7 +1208,7 @@ pa_dbusiface_device *pa_dbusiface_device_new_sink(pa_dbusiface_core *core, pa_si
d = pa_xnew0(pa_dbusiface_device, 1);
d->core = core;
- d->sink = pa_sink_ref(sink);
+ d->sink = sink;
d->type = PA_DEVICE_TYPE_SINK;
d->path = pa_sprintf_malloc("%s/%s%u", PA_DBUS_CORE_OBJECT_PATH, SINK_OBJECT_NAME, sink->index);
d->volume = *pa_sink_get_volume(sink, FALSE);
@@ -1242,7 +1242,7 @@ pa_dbusiface_device *pa_dbusiface_device_new_source(pa_dbusiface_core *core, pa_
d = pa_xnew0(pa_dbusiface_device, 1);
d->core = core;
- d->source = pa_source_ref(source);
+ d->source = source;
d->type = PA_DEVICE_TYPE_SOURCE;
d->path = pa_sprintf_malloc("%s/%s%u", PA_DBUS_CORE_OBJECT_PATH, SOURCE_OBJECT_NAME, source->index);
d->volume = *pa_source_get_volume(source, FALSE);
@@ -1271,14 +1271,11 @@ void pa_dbusiface_device_free(pa_dbusiface_device *d) {
pa_assert_se(pa_dbus_protocol_remove_interface(d->dbus_protocol, d->path, device_interface_info.name) >= 0);
- if (d->type == PA_DEVICE_TYPE_SINK) {
+ if (d->type == PA_DEVICE_TYPE_SINK)
pa_assert_se(pa_dbus_protocol_remove_interface(d->dbus_protocol, d->path, sink_interface_info.name) >= 0);
- pa_sink_unref(d->sink);
-
- } else {
+ else
pa_assert_se(pa_dbus_protocol_remove_interface(d->dbus_protocol, d->path, source_interface_info.name) >= 0);
- pa_source_unref(d->source);
- }
+
pa_hashmap_free(d->ports, (pa_free_cb_t) pa_dbusiface_device_port_free);
pa_proplist_free(d->proplist);
pa_dbus_protocol_unref(d->dbus_protocol);
diff --git a/src/modules/module-suspend-on-idle.c b/src/modules/module-suspend-on-idle.c
index 02a6b91..2325308 100644
--- a/src/modules/module-suspend-on-idle.c
+++ b/src/modules/module-suspend-on-idle.c
@@ -344,8 +344,8 @@ static pa_hook_result_t device_new_hook_cb(pa_core *c, pa_object *o, struct user
d = pa_xnew(struct device_info, 1);
d->userdata = u;
- d->source = source ? pa_source_ref(source) : NULL;
- d->sink = sink ? pa_sink_ref(sink) : NULL;
+ d->source = source;
+ d->sink = sink;
d->time_event = pa_core_rttime_new(c, PA_USEC_INVALID, timeout_cb, d);
pa_hashmap_put(u->device_infos, o, d);
@@ -359,11 +359,6 @@ static pa_hook_result_t device_new_hook_cb(pa_core *c, pa_object *o, struct user
static void device_info_free(struct device_info *d) {
pa_assert(d);
- if (d->source)
- pa_source_unref(d->source);
- if (d->sink)
- pa_sink_unref(d->sink);
-
d->userdata->core->mainloop->time_free(d->time_event);
pa_xfree(d);
diff --git a/src/pulsecore/card.c b/src/pulsecore/card.c
index 6149d2b..ae81fcb 100644
--- a/src/pulsecore/card.c
+++ b/src/pulsecore/card.c
@@ -141,17 +141,20 @@ void pa_card_new_data_set_profile(pa_card_new_data *data, const char *profile) {
}
void pa_card_new_data_done(pa_card_new_data *data) {
-
pa_assert(data);
pa_proplist_free(data->proplist);
+ /* Free ports before profiles, because ports have references to profiles
+ * but profiles don't have references to ports. (Profiles might some day
+ * have references to ports, in which case this becomes a bit more
+ * complicated.) */
+ if (data->ports)
+ pa_hashmap_free(data->ports, (pa_free_cb_t) pa_device_port_free);
+
if (data->profiles)
pa_hashmap_free(data->profiles, (pa_free_cb_t) pa_card_profile_free);
- if (data->ports)
- pa_hashmap_free(data->ports, (pa_free_cb_t) pa_device_port_unref);
-
pa_xfree(data->name);
pa_xfree(data->active_profile);
}
@@ -258,7 +261,12 @@ void pa_card_free(pa_card *c) {
pa_assert(pa_idxset_isempty(c->sources));
pa_idxset_free(c->sources, NULL);
- pa_hashmap_free(c->ports, (pa_free_cb_t) pa_device_port_unref);
+ /* Free ports before profiles, because ports have references to profiles
+ * but profiles don't have references to ports. (Profiles might some day
+ * have references to ports, in which case this becomes a bit more
+ * complicated.) */
+ if (c->ports)
+ pa_hashmap_free(c->ports, (pa_free_cb_t) pa_device_port_free);
if (c->profiles)
pa_hashmap_free(c->profiles, (pa_free_cb_t) pa_card_profile_free);
diff --git a/src/pulsecore/device-port.c b/src/pulsecore/device-port.c
index f16de3a..3934c9c 100644
--- a/src/pulsecore/device-port.c
+++ b/src/pulsecore/device-port.c
@@ -24,10 +24,7 @@
#include "device-port.h"
#include <pulsecore/card.h>
-PA_DEFINE_PUBLIC_CLASS(pa_device_port, pa_object);
-
-void pa_device_port_set_available(pa_device_port *p, pa_available_t status)
-{
+void pa_device_port_set_available(pa_device_port *p, pa_available_t status) {
pa_core *core;
pa_assert(p);
@@ -48,11 +45,8 @@ void pa_device_port_set_available(pa_device_port *p, pa_available_t status)
pa_hook_fire(&core->hooks[PA_CORE_HOOK_PORT_AVAILABLE_CHANGED], p);
}
-static void device_port_free(pa_object *o) {
- pa_device_port *p = PA_DEVICE_PORT(o);
-
+void pa_device_port_free(pa_device_port *p) {
pa_assert(p);
- pa_assert(pa_device_port_refcnt(p) == 0);
if (p->proplist)
pa_proplist_free(p->proplist);
@@ -65,25 +59,17 @@ static void device_port_free(pa_object *o) {
pa_xfree(p);
}
-
pa_device_port *pa_device_port_new(pa_core *c, const char *name, const char *description, size_t extra) {
pa_device_port *p;
pa_assert(name);
- p = PA_DEVICE_PORT(pa_object_new_internal(PA_ALIGN(sizeof(pa_device_port)) + extra, pa_device_port_type_id, pa_device_port_check_type));
- p->parent.free = device_port_free;
-
+ p = pa_xmalloc0(PA_ALIGN(sizeof(pa_device_port)) + extra);
p->name = pa_xstrdup(name);
p->description = pa_xstrdup(description);
p->core = c;
- p->card = NULL;
- p->priority = 0;
p->available = PA_AVAILABLE_UNKNOWN;
p->profiles = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
- p->is_input = FALSE;
- p->is_output = FALSE;
- p->latency_offset = 0;
p->proplist = pa_proplist_new();
return p;
diff --git a/src/pulsecore/device-port.h b/src/pulsecore/device-port.h
index c0c00cf..673f67f 100644
--- a/src/pulsecore/device-port.h
+++ b/src/pulsecore/device-port.h
@@ -39,7 +39,6 @@ typedef struct pa_device_port pa_device_port;
#include <pulsecore/card.h>
struct pa_device_port {
- pa_object parent; /* Needed for reference counting */
pa_core *core;
pa_card *card;
@@ -58,12 +57,10 @@ struct pa_device_port {
/* .. followed by some implementation specific data */
};
-PA_DECLARE_PUBLIC_CLASS(pa_device_port);
-#define PA_DEVICE_PORT(s) (pa_device_port_cast(s))
-
#define PA_DEVICE_PORT_DATA(d) ((void*) ((uint8_t*) d + PA_ALIGN(sizeof(pa_device_port))))
pa_device_port *pa_device_port_new(pa_core *c, const char *name, const char *description, size_t extra);
+void pa_device_port_free(pa_device_port *p);
/* The port's available status has changed */
void pa_device_port_set_available(pa_device_port *p, pa_available_t available);
diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
index 6ebe956..1c7e48c 100644
--- a/src/pulsecore/sink.c
+++ b/src/pulsecore/sink.c
@@ -142,7 +142,7 @@ void pa_sink_new_data_done(pa_sink_new_data *data) {
pa_proplist_free(data->proplist);
if (data->ports)
- pa_hashmap_free(data->ports, (pa_free_cb_t) pa_device_port_unref);
+ pa_hashmap_free(data->ports, data->card ? NULL : (pa_free_cb_t) pa_device_port_free);
pa_xfree(data->name);
pa_xfree(data->active_port);
@@ -743,7 +743,7 @@ static void sink_free(pa_object *o) {
pa_proplist_free(s->proplist);
if (s->ports)
- pa_hashmap_free(s->ports, (pa_free_cb_t) pa_device_port_unref);
+ pa_hashmap_free(s->ports, NULL);
pa_xfree(s);
}
@@ -1110,8 +1110,6 @@ void pa_sink_render(pa_sink*s, size_t length, pa_memchunk *result) {
return;
}
- pa_sink_ref(s);
-
if (length <= 0)
length = pa_frame_align(MIX_BUFFER_LENGTH, &s->sample_spec);
@@ -1169,8 +1167,6 @@ void pa_sink_render(pa_sink*s, size_t length, pa_memchunk *result) {
}
inputs_drop(s, info, n, result);
-
- pa_sink_unref(s);
}
/* Called from IO thread context */
@@ -1195,8 +1191,6 @@ void pa_sink_render_into(pa_sink*s, pa_memchunk *target) {
return;
}
- pa_sink_ref(s);
-
length = target->length;
block_size_max = pa_mempool_block_size_max(s->core->mempool);
if (length > block_size_max)
@@ -1254,8 +1248,6 @@ void pa_sink_render_into(pa_sink*s, pa_memchunk *target) {
}
inputs_drop(s, info, n, target);
-
- pa_sink_unref(s);
}
/* Called from IO thread context */
@@ -1279,8 +1271,6 @@ void pa_sink_render_into_full(pa_sink *s, pa_memchunk *target) {
return;
}
- pa_sink_ref(s);
-
l = target->length;
d = 0;
while (l > 0) {
@@ -1293,8 +1283,6 @@ void pa_sink_render_into_full(pa_sink *s, pa_memchunk *target) {
d += chunk.length;
l -= chunk.length;
}
-
- pa_sink_unref(s);
}
/* Called from IO thread context */
@@ -1309,8 +1297,6 @@ void pa_sink_render_full(pa_sink *s, size_t length, pa_memchunk *result) {
pa_assert(!s->thread_info.rewind_requested);
pa_assert(s->thread_info.rewind_nbytes == 0);
- pa_sink_ref(s);
-
pa_sink_render(s, length, result);
if (result->length < length) {
@@ -1326,8 +1312,6 @@ void pa_sink_render_full(pa_sink *s, size_t length, pa_memchunk *result) {
result->length = length;
}
-
- pa_sink_unref(s);
}
/* Called from main thread */
diff --git a/src/pulsecore/source.c b/src/pulsecore/source.c
index 4e4cd67..c35f19a 100644
--- a/src/pulsecore/source.c
+++ b/src/pulsecore/source.c
@@ -133,7 +133,7 @@ void pa_source_new_data_done(pa_source_new_data *data) {
pa_proplist_free(data->proplist);
if (data->ports)
- pa_hashmap_free(data->ports, (pa_free_cb_t) pa_device_port_unref);
+ pa_hashmap_free(data->ports, data->card ? NULL : (pa_free_cb_t) pa_device_port_free);
pa_xfree(data->name);
pa_xfree(data->active_port);
@@ -672,7 +672,7 @@ static void source_free(pa_object *o) {
pa_proplist_free(s->proplist);
if (s->ports)
- pa_hashmap_free(s->ports, (pa_free_cb_t) pa_device_port_unref);
+ pa_hashmap_free(s->ports, NULL);
pa_xfree(s);
}
--
1.7.10.4
More information about the pulseaudio-discuss
mailing list