Skip to contentSkip to conversation

Add support for the source interface to audio IPC.

PATCH In review João Paulo Rechi Vita 3 participants 9 messages Aug 16, 2009 Bluetooth drivers

lore
Share & tools
lore

João Paulo Rechi Vita

author
#0 lore raw

PATCH 1/1 Add support for the source interface to audio IPC.

---
 audio/unix.c |   55 +++++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 41 insertions(+), 14 deletions(-)
audio/unix.c +41 -14
index 3fad129..23e2382 100644
@@ -168,21 +168,28 @@ static void unix_ipc_error(struct unix_client *client, uint8_t name, int err)
 
 	rsp->posix_errno = err;
 
+	debug("sending error %s(%d)", strerror(err), err);
 	unix_ipc_sendmsg(client, &rsp->h);
 }
 
 static service_type_t select_service(struct audio_device *dev, const char *interface)
 {
 	if (!interface) {
-		if (dev->sink && avdtp_is_connected(&dev->src, &dev->dst))
+		if (dev->source && avdtp_is_connected(&dev->src, &dev->dst))
+			return TYPE_SOURCE;
+		else if (dev->sink && avdtp_is_connected(&dev->src, &dev->dst))
 			return TYPE_SINK;
 		else if (dev->headset && headset_is_active(dev))
 			return TYPE_HEADSET;
+		else if (dev->source)
+			return TYPE_SOURCE;
 		else if (dev->sink)
 			return TYPE_SINK;
 		else if (dev->headset)
 			return TYPE_HEADSET;
-	} else if (!strcmp(interface, AUDIO_SINK_INTERFACE) && dev->sink)
+	} else if (!strcmp(interface, AUDIO_SOURCE_INTERFACE) && dev->source)
+		return TYPE_SOURCE;
+	else if (!strcmp(interface, AUDIO_SINK_INTERFACE) && dev->sink)
 		return TYPE_SINK;
 	else if (!strcmp(interface, AUDIO_HEADSET_INTERFACE) && dev->headset)
 		return TYPE_HEADSET;

@@ -487,6 +494,7 @@ static void print_sbc(struct sbc_codec_cap *sbc)
 static int a2dp_append_codec(struct bt_get_capabilities_rsp *rsp,
 				struct avdtp_service_capability *cap,
 				uint8_t seid,
+				uint8_t type,
 				uint8_t configured,
 				uint8_t lock)
 {

@@ -518,7 +526,10 @@ static int a2dp_append_codec(struct bt_get_capabilities_rsp *rsp,
 		sbc->max_bitpool = sbc_cap->max_bitpool;
 
 		print_sbc(sbc_cap);
-		codec->type = BT_A2DP_SBC_SINK;
+		if (type == AVDTP_SEP_TYPE_SINK)
+			codec->type = BT_A2DP_SBC_SINK;
+		else if (type == AVDTP_SEP_TYPE_SOURCE)
+			codec->type = BT_A2DP_SBC_SOURCE;
 	} else if (codec_cap->media_codec_type == A2DP_CODEC_MPEG12) {
 		struct mpeg_codec_cap *mpeg_cap = (void *) codec_cap;
 		mpeg_capabilities_t *mpeg = (void *) codec;

@@ -536,7 +547,10 @@ static int a2dp_append_codec(struct bt_get_capabilities_rsp *rsp,
 		mpeg->bitrate = mpeg_cap->bitrate;
 
 		print_mpeg12(mpeg_cap);
-		codec->type = BT_A2DP_MPEG12_SINK;
+		if (type == AVDTP_SEP_TYPE_SINK)
+			codec->type = BT_A2DP_MPEG12_SINK;
+		else if (type == AVDTP_SEP_TYPE_SOURCE)
+			codec->type = BT_A2DP_MPEG12_SOURCE;
 	} else {
 		size_t codec_length, type_length, total_length;
 

@@ -553,7 +567,10 @@ static int a2dp_append_codec(struct bt_get_capabilities_rsp *rsp,
 		memcpy(codec->data, &codec_cap->media_codec_type, type_length);
 		memcpy(codec->data + type_length, codec_cap->data,
 			codec_length);
-		codec->type = BT_A2DP_UNKNOWN_SINK;
+		if (type == AVDTP_SEP_TYPE_SINK)
+			codec->type = BT_A2DP_UNKNOWN_SINK;
+		else if (type == AVDTP_SEP_TYPE_SOURCE)
+			codec->type = BT_A2DP_UNKNOWN_SOURCE;
 	}
 
 	codec->seid = seid;

@@ -606,7 +623,8 @@ static void a2dp_discovery_complete(struct avdtp *session, GSList *seps,
 
 		type = avdtp_get_type(rsep);
 
-		if (type != AVDTP_SEP_TYPE_SINK)
+		if (type != AVDTP_SEP_TYPE_SINK &&
+			type != AVDTP_SEP_TYPE_SOURCE)
 			continue;
 
 		cap = avdtp_get_codec(rsep);

@@ -641,7 +659,7 @@ static void a2dp_discovery_complete(struct avdtp *session, GSList *seps,
 		if (sep && a2dp_sep_get_lock(sep))
 			lock = BT_WRITE_LOCK;
 
-		a2dp_append_codec(rsp, cap, seid, configured, lock);
+		a2dp_append_codec(rsp, cap, seid, type, configured, lock);
 	}
 
 	unix_ipc_sendmsg(client, &rsp->h);

@@ -819,6 +837,7 @@ static void start_discovery(struct audio_device *dev, struct unix_client *client
 
 	switch (client->type) {
 	case TYPE_SINK:
+	case TYPE_SOURCE:
 		a2dp = &client->d.a2dp;
 
 		if (!a2dp->session)

@@ -1252,10 +1271,12 @@ static void handle_getcapabilities_req(struct unix_client *client,
 		dev = manager_find_device(req->object, &src, &dst,
 					client->interface, FALSE);
 
-	if (!dev && req->transport == BT_CAPABILITIES_TRANSPORT_SCO) {
+	if (!dev) {
 		g_free(client->interface);
-		client->interface = g_strdup(AUDIO_GATEWAY_INTERFACE);
-
+		if (req->transport == BT_CAPABILITIES_TRANSPORT_SCO)
+			client->interface = g_strdup(AUDIO_GATEWAY_INTERFACE);
+		else if (req->transport == BT_CAPABILITIES_TRANSPORT_A2DP)
+			client->interface = g_strdup(AUDIO_SOURCE_INTERFACE);
 		dev = manager_find_device(req->object, &src, &dst,
 				client->interface, TRUE);
 		if (!dev && (req->flags & BT_FLAG_AUTOCONNECT))

@@ -1305,8 +1326,10 @@ static int handle_sco_open(struct unix_client *client, struct bt_open_req *req)
 static int handle_a2dp_open(struct unix_client *client, struct bt_open_req *req)
 {
 	if (!client->interface)
+		/* FIXME: are we treating a sink or a source? */
 		client->interface = g_strdup(AUDIO_SINK_INTERFACE);
-	else if (!g_str_equal(client->interface, AUDIO_SINK_INTERFACE))
+	else if (!g_str_equal(client->interface, AUDIO_SINK_INTERFACE) &&
+			!g_str_equal(client->interface, AUDIO_SOURCE_INTERFACE))
 		return -EIO;
 
 	debug("open a2dp - object=%s source=%s destination=%s lock=%s%s",

@@ -1398,8 +1421,10 @@ static int handle_a2dp_transport(struct unix_client *client,
 	struct mpeg_codec_cap mpeg_cap;
 
 	if (!client->interface)
+		/* FIXME: are we treating a sink or a source? */
 		client->interface = g_strdup(AUDIO_SINK_INTERFACE);
-	else if (!g_str_equal(client->interface, AUDIO_SINK_INTERFACE))
+	else if (!g_str_equal(client->interface, AUDIO_SINK_INTERFACE) &&
+			!g_str_equal(client->interface, AUDIO_SOURCE_INTERFACE))
 		return -EIO;
 
 	if (client->caps) {

@@ -1413,7 +1438,8 @@ static int handle_a2dp_transport(struct unix_client *client,
 
 	client->caps = g_slist_append(client->caps, media_transport);
 
-	if (req->codec.type == BT_A2DP_MPEG12_SINK) {
+	if (req->codec.type == BT_A2DP_MPEG12_SINK ||
+		req->codec.type == BT_A2DP_MPEG12_SOURCE) {
 		mpeg_capabilities_t *mpeg = (void *) &req->codec;
 
 		memset(&mpeg_cap, 0, sizeof(mpeg_cap));

@@ -1431,7 +1457,8 @@ static int handle_a2dp_transport(struct unix_client *client,
 							sizeof(mpeg_cap));
 
 		print_mpeg12(&mpeg_cap);
-	} else if (req->codec.type == BT_A2DP_SBC_SINK) {
+	} else if (req->codec.type == BT_A2DP_SBC_SINK ||
+			req->codec.type == BT_A2DP_SBC_SOURCE) {
 		sbc_capabilities_t *sbc = (void *) &req->codec;
 
 		memset(&sbc_cap, 0, sizeof(sbc_cap));
Signature · 1 line
1.6.0.4
author
#1 lore raw

PATCH 1/1 Small optimization on audio IPC.

Check if we find the device before trying to define an interface, so if we fail the if comparison plus the strdup are not necessary.

---
 audio/unix.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)
audio/unix.c +3 -4
index 23e2382..e0307e0 100644
@@ -1257,14 +1257,13 @@ static void handle_getcapabilities_req(struct unix_client *client,
 	str2ba(req->source, &src);
 	str2ba(req->destination, &dst);
 
+	if (!manager_find_device(req->object, &src, &dst, NULL, FALSE))
+		goto failed;
+
 	if (req->transport == BT_CAPABILITIES_TRANSPORT_SCO)
 		client->interface = g_strdup(AUDIO_HEADSET_INTERFACE);
 	else if (req->transport == BT_CAPABILITIES_TRANSPORT_A2DP)
 		client->interface = g_strdup(AUDIO_SINK_INTERFACE);
-
-	if (!manager_find_device(req->object, &src, &dst, NULL, FALSE))
-		goto failed;
-
 	dev = manager_find_device(req->object, &src, &dst, client->interface,
 				TRUE);
 	if (!dev && (req->flags & BT_FLAG_AUTOCONNECT))
Signature · 1 line
1.6.0.4
#2 lore raw
In reply to João Paulo Rechi Vita

Hi,

There are a few issues with this patch:

audio/unix.c static void unix_ipc_error(struct unix_client *client, uint8_t name, int err) · line 181

Quoted 6 lines · João Paulo Rechi Vita
audio/unix.c +3 -1
 	if (!interface) {
-		if (dev->sink && avdtp_is_connected(&dev->src, &dev->dst))
+		if (dev->source && avdtp_is_connected(&dev->src, &dev->dst))
+			return TYPE_SOURCE;
+		else if (dev->sink && avdtp_is_connected(&dev->src, &dev->dst))
 			return TYPE_SINK;

I think the dev->source check should be in an "else if" clause while the dev->sink check should be first. I.e. if a device happens to support both roles of A2DP we default to dev->sink.

audio/unix.c static void unix_ipc_error(struct unix_client *client, uint8_t name, int err) · line 187

Quoted 4 lines
audio/unix.c +2 -0
+		else if (dev->source)
+			return TYPE_SOURCE;
 		else if (dev->sink)
 			return TYPE_SINK;

Same here. dev->sink check before dev->source.

audio/unix.c static int a2dp_append_codec(struct bt_get_capabilities_rsp *rsp, · line 532

Quoted 5 lines
audio/unix.c +4 -1
-		codec->type = BT_A2DP_SBC_SINK;
+		if (type == AVDTP_SEP_TYPE_SINK)
+			codec->type = BT_A2DP_SBC_SINK;
+		else if (type == AVDTP_SEP_TYPE_SOURCE)
+			codec->type = BT_A2DP_SBC_SOURCE;

What about the "else" part? If type is neither you should probably bail out of the function with -EINVAL.

audio/unix.c static int a2dp_append_codec(struct bt_get_capabilities_rsp *rsp, · line 553

Quoted 6 lines
audio/unix.c +4 -1
 		print_mpeg12(mpeg_cap);
-		codec->type = BT_A2DP_MPEG12_SINK;
+		if (type == AVDTP_SEP_TYPE_SINK)
+			codec->type = BT_A2DP_MPEG12_SINK;
+		else if (type == AVDTP_SEP_TYPE_SOURCE)
+			codec->type = BT_A2DP_MPEG12_SOURCE;

Same here.

audio/unix.c static int a2dp_append_codec(struct bt_get_capabilities_rsp *rsp, · line 573

Quoted 4 lines
audio/unix.c +4 -0
+		if (type == AVDTP_SEP_TYPE_SINK)
+			codec->type = BT_A2DP_UNKNOWN_SINK;
+		else if (type == AVDTP_SEP_TYPE_SOURCE)
+			codec->type = BT_A2DP_UNKNOWN_SOURCE;

And again..

audio/unix.c static void a2dp_discovery_complete(struct avdtp *session, GSList *seps, · line 628

Quoted 11 lines
audio/unix.c +2 -1
 	}
 
 	codec->seid = seid;

@@ -606,7 +623,8 @@ static void a2dp_discovery_complete(struct avdtp *session, GSList *seps,
 
 		type = avdtp_get_type(rsep);
 
-		if (type != AVDTP_SEP_TYPE_SINK)
+		if (type != AVDTP_SEP_TYPE_SINK &&
+			type != AVDTP_SEP_TYPE_SOURCE)
 			continue;

Incorrect indentation for the continuation line of the if clause. It should at least two tabs from the original to clearly separate it from the "continue;" line.

audio/unix.c static void handle_getcapabilities_req(struct unix_client *client, · line 1279

Quoted 8 lines
audio/unix.c +5 -2
+	if (!dev) {
 		g_free(client->interface);
-		client->interface = g_strdup(AUDIO_GATEWAY_INTERFACE);
-
+		if (req->transport == BT_CAPABILITIES_TRANSPORT_SCO)
+			client->interface = g_strdup(AUDIO_GATEWAY_INTERFACE);
+		else if (req->transport == BT_CAPABILITIES_TRANSPORT_A2DP)
+			client->interface = g_strdup(AUDIO_SOURCE_INTERFACE);

Again a missing "else" clause but this time the consequences are even more serious. If req->transport doesn't match either of those values you'll leave client->interface pointing to free'd memory which will probably cause access to free'd memory or a double-free later. So at the very least you should set client->interface to NULL here and maybe also return an error from the function.

Johan

maintainer
#3 lore raw

Hi,

2009/8/16 Johan Hedberg <[email protected]>:

Quoted 12 lines

Hi,

There are a few issues with this patch:

Quoted 6 lines · João Paulo Rechi Vita

      if (!interface) {
-             if (dev->sink && avdtp_is_connected(&dev->src, &dev->dst))
+             if (dev->source && avdtp_is_connected(&dev->src, &dev->dst))
+                     return TYPE_SOURCE;
+             else if (dev->sink && avdtp_is_connected(&dev->src, &dev->dst)) return TYPE_SINK;

Quoted 3 lines

I think the dev->source check should be in an "else if" clause while the dev->sink check should be first. I.e. if a device happens to support both roles of A2DP we default to dev->sink.

Good catches, perhaps we should consider doing avdtp_is_connected check before checking for sink/source pointers. What do you think?

Signature · 2 lines
Luiz Augusto von Dentz
Engenheiro de Computação
#4 lore raw

Hi Luiz,

Quoted 8 lines · Luiz Augusto von Dentz
Quoted 8 lines · João Paulo Rechi Vita
Quoted 6 lines

      if (!interface) {
-             if (dev->sink && avdtp_is_connected(&dev->src, &dev->dst))
+             if (dev->source && avdtp_is_connected(&dev->src, &dev->dst))
+                     return TYPE_SOURCE;
+             else if (dev->sink && avdtp_is_connected(&dev->src, &dev->dst)) return TYPE_SINK;

Quoted 6 lines
Quoted 3 lines

I think the dev->source check should be in an "else if" clause while the dev->sink check should be first. I.e. if a device happens to support both roles of A2DP we default to dev->sink.

Good catches, perhaps we should consider doing avdtp_is_connected check before checking for sink/source pointers. What do you think?

What exactly do you mean? Could you perhaps give an example code snippet of how you'd like the code to look like?

Johan

maintainer
#5 lore raw

Hi Johan,

2009/8/16 Johan Hedberg <[email protected]>:

Quoted 15 lines

Hi Luiz,

Quoted 12 lines · Luiz Augusto von Dentz
Quoted 11 lines · João Paulo Rechi Vita
Quoted 6 lines

      if (!interface) {
-             if (dev->sink && avdtp_is_connected(&dev->src, &dev->dst))
+             if (dev->source && avdtp_is_connected(&dev->src, &dev->dst))
+                     return TYPE_SOURCE;
+             else if (dev->sink && avdtp_is_connected(&dev->src, &dev->dst)) return TYPE_SINK;

I think the dev->source check should be in an "else if" clause while the dev->sink check should be first. I.e. if a device happens to support both roles of A2DP we default to dev->sink.

Quoted 8 lines
Quoted 2 lines

Good catches, perhaps we should consider doing avdtp_is_connected check before checking for sink/source pointers. What do you think?

What exactly do you mean? Could you perhaps give an example code snippet of how you'd like the code to look like?

Johan

Something like this:

if (avdtp_is_connected(...)) {
if (dev->sink) return TYPE_SINK;
else if (dev->source) return TYPE_SOURCE;
else...
}

Signature · 2 lines
Luiz Augusto von Dentz
Engenheiro de Computação
#6 lore raw

Hi Luiz,

Quoted 20 lines · Luiz Augusto von Dentz
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
Quoted 6 lines

      if (!interface) {
-             if (dev->sink && avdtp_is_connected(&dev->src, &dev->dst))
+             if (dev->source && avdtp_is_connected(&dev->src, &dev->dst))
+                     return TYPE_SOURCE;
+             else if (dev->sink && avdtp_is_connected(&dev->src, &dev->dst)) return TYPE_SINK;

I think the dev->source check should be in an "else if" clause while the dev->sink check should be first. I.e. if a device happens to support both roles of A2DP we default to dev->sink.

Quoted 8 lines
Quoted 2 lines

Good catches, perhaps we should consider doing avdtp_is_connected check before checking for sink/source pointers. What do you think?

What exactly do you mean? Could you perhaps give an example code snippet of how you'd like the code to look like?

Johan

Quoted 7 lines

Something like this:

if (avdtp_is_connected(...)) {
if (dev->sink) return TYPE_SINK;
else if (dev->source) return TYPE_SOURCE;
else...
}

Sure, but take a look at the bigger context, i.e. the whole select_service function. You need to be careful not to change it's current behavior e.g. in the case that avdtp_is_connected returns TRUE but both dev->sink and dev->source are NULL (not sure how that could happen but your change would change the logic for that case).

Johan

#7 lore raw
In reply to Johan Hedberg

Hi,

Quoted 1 line · Johan Hedberg

There are a few issues with this patch:

I went ahead and fixed the issues myself so we can proceed with the 4.48 release. The patches are now pushed upstream.

Johan

author
#8 lore raw

2009/8/16 Johan Hedberg <[email protected]>:

Quoted 10 lines

Hi,

Quoted 1 line · Johan Hedberg

There are a few issues with this patch:

I went ahead and fixed the issues myself so we can proceed with the 4.48 release. The patches are now pushed upstream.

Johan

Sorry for the late reply, didn't check email after very early in the morning today (GMT-3) because of Mom's birthday.

Thanks for the review and the fixes!

Signature · 5 lines
João Paulo Rechi Vita
MSc Computer Science Student
Computer Engineer
IC / Unicamp
http://jprvita.wordpress.com/