[pulseaudio-discuss] [PATCH] bluetooth: Add ports to the bluetooth sink/source

[poljarinho at gmail.com]

On Tue, Jun 05, 2012 at 01:36:58PM +0200, David Henningsson wrote:
> On 06/04/2012 07:35 PM, poljar wrote:
> >The bluetooth device should have ports so we can attach a latency to
> >the ports.
> 
> I think it is good that we create ports for more types of devices,
> but I don't see how that correlates to different latencies...?
> 

This is part of my GSOC project [1].

> >Every profile (a2dp, hsp...) has his own set of ports depending on the
> >number of sinks and sources it provides.
> >---
> >  src/modules/bluetooth/module-bluetooth-device.c |  124 +++++++++++++++++++++++
> >  1 file changed, 124 insertions(+)
> >
> >diff --git a/src/modules/bluetooth/module-bluetooth-device.c b/src/modules/bluetooth/module-bluetooth-device.c
> >index 68c4efc..4901ef9 100644
> >--- a/src/modules/bluetooth/module-bluetooth-device.c
> >+++ b/src/modules/bluetooth/module-bluetooth-device.c
> >@@ -2073,6 +2073,61 @@ static pa_hook_result_t source_state_changed_cb(pa_core *c, pa_source *s, struct
> >      return PA_HOOK_OK;
> >  }
> >
> >+static void connect_ports(struct userdata *u, void *sink_or_source_new_data, pa_direction_t direction) {
> >+    union {
> >+        pa_sink_new_data *sink_new_data;
> >+        pa_source_new_data *source_new_data;
> >+    } data;
> 
> Using unions this way is error-prone IMO - just use two different
> pointers "sink_new_data" and "source_new_data", no point in
> encapsulating them into "data".
> 

This was a suggestion by Tanu, I don't know how and why this should be
error-prone.

> >+    pa_device_port *port;
> >+
> >+    if (direction == PA_DIRECTION_OUTPUT) {
> >+        data.sink_new_data = sink_or_source_new_data;
> >+        data.sink_new_data->ports = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
> >+    } else {
> >+        data.source_new_data = sink_or_source_new_data;
> >+        data.source_new_data->ports = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
> >+    }
> >+
> >+    switch (u->profile) {
> >+        case PROFILE_A2DP:
> >+            pa_assert_se(port = pa_hashmap_get(u->card->ports, "a2dp-output"));
> >+            pa_assert_se(pa_hashmap_put(data.sink_new_data->ports, port->name, port)>= 0);
> >+            pa_device_port_ref(port);
> >+            break;
> 
> This might be something I'm not seeing here, but you're ref-ing the
> port here, but where is it unref-ed so it can be freed properly?
> 

The unref-ing should be done when the pa_device_port_hashmap_free() is
called. If I'm not mistaken that is done in sink.c automatically when we
create and free a sink.

Thanks for the review.

[1] http://www.google-melange.com/gsoc/project/google/gsoc2012/poljar/13001