All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] Request/Abort Continuing and some minor things
@ 2011-10-14 21:28 Lucas De Marchi
  2011-10-14 21:28 ` [PATCH 1/5] AVRCP: Use track's UID in event notification Lucas De Marchi
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Lucas De Marchi @ 2011-10-14 21:28 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Lucas De Marchi

Hi all,

Patch 1 allows TG to correctly inform CT whether there's a track selected.
Second patch is an easy event to add, track reached start. Third is a fix to
RequestPlayStatus.

Last 2 patches are the ones more important. They cover the Request/abort
continuing responses. In my previous implementation (that doesn't apply
anymore) I was generating all the PDUs in the first GetElementAttributes
request, putting them in a queue and sending them upon receipt of
RequestContinuing. Another approach (requested by Luiz) is to save the list of
attributes we need to send, the position within the last attribute we sent and
the pdu id. Then we can generate new PDUs only if CT asks us to continue.

I implemented the later, hoping it would be simpler than the former. However
it's not, but there are some benefits:
i) We don't allocate "lots" of space to keep pending pdus
ii) The first request to GetElementAttributes is much quicker this way since we
generate only one PDU.

Lucas De Marchi (5):
  AVRCP: Use track's UID in event notification
  AVRCP: implement TRACK-REACHED-START event
  AVRCP: respond with UINT32_MAX if duration is not available
  AVRCP: Implement AbortContinuingResponse PDU
  AVRCP: Implement RequestContinuingResponse PDU

 audio/avrcp.c     |  301 ++++++++++++++++++++++++++++++++++++++++-------------
 audio/avrcp.h     |    3 +
 audio/media.c     |   22 ++++-
 doc/media-api.txt |    4 +-
 4 files changed, 256 insertions(+), 74 deletions(-)

-- 
1.7.7


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/5] AVRCP: Use track's UID in event notification
  2011-10-14 21:28 [PATCH 0/5] Request/Abort Continuing and some minor things Lucas De Marchi
@ 2011-10-14 21:28 ` Lucas De Marchi
  2011-10-17  7:35   ` Luiz Augusto von Dentz
  2011-10-14 21:28 ` [PATCH 2/5] AVRCP: implement TRACK-REACHED-START event Lucas De Marchi
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Lucas De Marchi @ 2011-10-14 21:28 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Lucas De Marchi

When registering for TRACK_CHANGED event, CT expects the track's UID in
the INTERIM and CHANGED responses. According to AVRCP 1.3 spec, there
are only 2 possible values:
	1) 0: there's a track selected
	2) UINT32_MAX: there's no track selected

AVRCP 1.4 reserves the value UINT64_MAX for the second case. Since this
later value is the one used in certification process for best IOP
we return UINT64_MAX instead of the former.

This implementation allows to pass PTS test TP/NFY/BV-05-C.
---
 audio/avrcp.c |   16 +++++-----------
 audio/avrcp.h |    1 +
 audio/media.c |   17 ++++++++++++++++-
 3 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index ac7c8d4..076fdf4 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -363,18 +363,11 @@ int avrcp_player_event(struct avrcp_player *player, uint8_t id, void *data)
 		pdu->params[1] = *((uint8_t *)data);
 
 		break;
-	case AVRCP_EVENT_TRACK_CHANGED: {
+	case AVRCP_EVENT_TRACK_CHANGED:
 		size = 9;
-
-		/*
-		 * AVRCP 1.3 supports only one track identifier: PLAYING
-		 * (0x0). When 1.4 version is added, this shall be changed to
-		 * contain the identifier of the track.
-		 */
-		memset(&pdu->params[1], 0, 8);
+		memcpy(&pdu->params[1], data, sizeof(uint64_t));
 
 		break;
-	}
 	default:
 		error("Unknown event %u", id);
 		return -EINVAL;
@@ -828,6 +821,7 @@ static uint8_t avrcp_handle_register_notification(struct avrcp_player *player,
 						uint8_t transaction)
 {
 	uint16_t len = ntohs(pdu->params_len);
+	uint64_t uid;
 
 	/*
 	 * 1 byte for EventID, 4 bytes for Playback interval but the latest
@@ -845,8 +839,8 @@ static uint8_t avrcp_handle_register_notification(struct avrcp_player *player,
 		break;
 	case AVRCP_EVENT_TRACK_CHANGED:
 		len = 9;
-
-		memset(&pdu->params[1], 0, 8);
+		uid = player->cb->get_uid(player->user_data);
+		memcpy(&pdu->params[1], &uid, sizeof(uint64_t));
 
 		break;
 	default:
diff --git a/audio/avrcp.h b/audio/avrcp.h
index 360a80a..8cf95a4 100644
--- a/audio/avrcp.h
+++ b/audio/avrcp.h
@@ -75,6 +75,7 @@
 struct avrcp_player_cb {
 	int (*get_setting) (uint8_t attr, void *user_data);
 	int (*set_setting) (uint8_t attr, uint8_t value, void *user_data);
+	uint64_t (*get_uid) (void *user_data);
 	void *(*get_metadata) (uint32_t id, void *user_data);
 	GList *(*list_metadata) (void *user_data);
 	uint8_t (*get_status) (void *user_data);
diff --git a/audio/media.c b/audio/media.c
index 519cafe..9ef393b 100644
--- a/audio/media.c
+++ b/audio/media.c
@@ -1168,6 +1168,18 @@ static GList *list_metadata(void *user_data)
 	return g_hash_table_get_keys(mp->track);
 }
 
+static uint64_t get_uid(void *user_data)
+{
+	struct media_player *mp = user_data;
+
+	DBG("%p", mp->track);
+
+	if (mp->track == NULL)
+		return UINT64_MAX;
+
+	return 0;
+}
+
 static void *get_metadata(uint32_t id, void *user_data)
 {
 	struct media_player *mp = user_data;
@@ -1220,6 +1232,7 @@ static struct avrcp_player_cb player_cb = {
 	.get_setting = get_setting,
 	.set_setting = set_setting,
 	.list_metadata = list_metadata,
+	.get_uid = get_uid,
 	.get_metadata = get_metadata,
 	.get_position = get_position,
 	.get_status = get_status
@@ -1369,6 +1382,7 @@ static gboolean parse_player_metadata(struct media_player *mp,
 	GHashTable *track;
 	int ctype;
 	gboolean title = FALSE;
+	uint64_t uid;
 
 	ctype = dbus_message_iter_get_arg_type(iter);
 	if (ctype != DBUS_TYPE_ARRAY)
@@ -1466,8 +1480,9 @@ static gboolean parse_player_metadata(struct media_player *mp,
 	mp->track = track;
 	mp->position = 0;
 	g_timer_start(mp->timer);
+	uid = get_uid(mp);
 
-	avrcp_player_event(mp->player, AVRCP_EVENT_TRACK_CHANGED, NULL);
+	avrcp_player_event(mp->player, AVRCP_EVENT_TRACK_CHANGED, &uid);
 
 	return TRUE;
 
-- 
1.7.7


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/5] AVRCP: implement TRACK-REACHED-START event
  2011-10-14 21:28 [PATCH 0/5] Request/Abort Continuing and some minor things Lucas De Marchi
  2011-10-14 21:28 ` [PATCH 1/5] AVRCP: Use track's UID in event notification Lucas De Marchi
@ 2011-10-14 21:28 ` Lucas De Marchi
  2011-10-17  7:40   ` Luiz Augusto von Dentz
  2011-10-14 21:28 ` [PATCH 3/5] AVRCP: respond with UINT32_MAX if duration is not available Lucas De Marchi
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Lucas De Marchi @ 2011-10-14 21:28 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Lucas De Marchi

---
 audio/avrcp.c |   13 ++++++++++---
 audio/avrcp.h |    2 ++
 audio/media.c |    5 +++++
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index 076fdf4..8b48e83 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -136,7 +136,7 @@ struct avrcp_player {
 
 	unsigned int handler;
 	uint16_t registered_events;
-	uint8_t transaction_events[AVRCP_EVENT_TRACK_CHANGED + 1];
+	uint8_t transaction_events[AVRCP_EVENT_LAST + 1];
 
 	struct avrcp_player_cb *cb;
 	void *user_data;
@@ -368,6 +368,9 @@ int avrcp_player_event(struct avrcp_player *player, uint8_t id, void *data)
 		memcpy(&pdu->params[1], data, sizeof(uint64_t));
 
 		break;
+	case AVRCP_EVENT_TRACK_REACHED_START:
+		size = 1;
+		break;
 	default:
 		error("Unknown event %u", id);
 		return -EINVAL;
@@ -504,10 +507,11 @@ static uint8_t avrcp_handle_get_capabilities(struct avrcp_player *player,
 
 		return AVC_CTYPE_STABLE;
 	case CAP_EVENTS_SUPPORTED:
-		pdu->params_len = htons(4);
-		pdu->params[1] = 2;
+		pdu->params_len = htons(5);
+		pdu->params[1] = 3;
 		pdu->params[2] = AVRCP_EVENT_STATUS_CHANGED;
 		pdu->params[3] = AVRCP_EVENT_TRACK_CHANGED;
+		pdu->params[4] = AVRCP_EVENT_TRACK_REACHED_START;
 
 		return AVC_CTYPE_STABLE;
 	}
@@ -843,6 +847,9 @@ static uint8_t avrcp_handle_register_notification(struct avrcp_player *player,
 		memcpy(&pdu->params[1], &uid, sizeof(uint64_t));
 
 		break;
+	case AVRCP_EVENT_TRACK_REACHED_START:
+		len =1;
+		break;
 	default:
 		/* All other events are not supported yet */
 		goto err;
diff --git a/audio/avrcp.h b/audio/avrcp.h
index 8cf95a4..c798658 100644
--- a/audio/avrcp.h
+++ b/audio/avrcp.h
@@ -71,6 +71,8 @@
 /* Notification events */
 #define AVRCP_EVENT_STATUS_CHANGED	0x01
 #define AVRCP_EVENT_TRACK_CHANGED	0x02
+#define AVRCP_EVENT_TRACK_REACHED_START	0x04
+#define AVRCP_EVENT_LAST		AVRCP_EVENT_TRACK_REACHED_START
 
 struct avrcp_player_cb {
 	int (*get_setting) (uint8_t attr, void *user_data);
diff --git a/audio/media.c b/audio/media.c
index 9ef393b..a7647ea 100644
--- a/audio/media.c
+++ b/audio/media.c
@@ -1289,6 +1289,11 @@ static gboolean set_position(struct media_player *mp, DBusMessageIter *iter)
 	mp->position = value;
 	g_timer_start(mp->timer);
 
+	if (!mp->position) {
+		avrcp_player_event(mp->player,
+					AVRCP_EVENT_TRACK_REACHED_START, NULL);
+	}
+
 	return TRUE;
 }
 
-- 
1.7.7


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/5] AVRCP: respond with UINT32_MAX if duration is not available
  2011-10-14 21:28 [PATCH 0/5] Request/Abort Continuing and some minor things Lucas De Marchi
  2011-10-14 21:28 ` [PATCH 1/5] AVRCP: Use track's UID in event notification Lucas De Marchi
  2011-10-14 21:28 ` [PATCH 2/5] AVRCP: implement TRACK-REACHED-START event Lucas De Marchi
@ 2011-10-14 21:28 ` Lucas De Marchi
  2011-10-17  7:57   ` Luiz Augusto von Dentz
  2011-10-14 21:28 ` [PATCH 4/5] AVRCP: Implement AbortContinuingResponse PDU Lucas De Marchi
  2011-10-14 21:28 ` [PATCH 5/5] AVRCP: Implement RequestContinuingResponse PDU Lucas De Marchi
  4 siblings, 1 reply; 13+ messages in thread
From: Lucas De Marchi @ 2011-10-14 21:28 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Lucas De Marchi

Section 5.4.1 of AVRCP 1.3 spec says:

	If TG does not support SongLength And SongPosition on TG, then TG shall
	return 0xFFFFFFFF.

SongPosition is always available, but song length depends on user to
provied it.
---
 audio/avrcp.c     |    2 +-
 doc/media-api.txt |    4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index 8b48e83..1b55e33 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -808,7 +808,7 @@ static uint8_t avrcp_handle_get_play_status(struct avrcp_player *player,
 						AVRCP_MEDIA_ATTRIBUTE_DURATION,
 						player->user_data));
 
-	duration = htonl(duration);
+	duration = duration ? htonl(duration) : UINT32_MAX;
 	position = htonl(position);
 
 	memcpy(&pdu->params[0], &duration, 4);
diff --git a/doc/media-api.txt b/doc/media-api.txt
index b8dcdbd..204f33e 100644
--- a/doc/media-api.txt
+++ b/doc/media-api.txt
@@ -114,7 +114,9 @@ Methods		void RegisterEndpoint(object endpoint, dict properties)
 
 				uint32 Duration:
 
-					Track duration in milliseconds
+					Track duration in milliseconds. If it's
+					set to 0 or left blank, it will be
+					treated as not available.
 
 			Possible Errors: org.bluez.Error.InvalidArguments
 					 org.bluez.Error.NotSupported
-- 
1.7.7


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/5] AVRCP: Implement AbortContinuingResponse PDU
  2011-10-14 21:28 [PATCH 0/5] Request/Abort Continuing and some minor things Lucas De Marchi
                   ` (2 preceding siblings ...)
  2011-10-14 21:28 ` [PATCH 3/5] AVRCP: respond with UINT32_MAX if duration is not available Lucas De Marchi
@ 2011-10-14 21:28 ` Lucas De Marchi
  2011-10-17  8:05   ` Luiz Augusto von Dentz
  2011-10-14 21:28 ` [PATCH 5/5] AVRCP: Implement RequestContinuingResponse PDU Lucas De Marchi
  4 siblings, 1 reply; 13+ messages in thread
From: Lucas De Marchi @ 2011-10-14 21:28 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Lucas De Marchi

---
 audio/avrcp.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index 1b55e33..1babb94 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -77,6 +77,7 @@
 #define AVRCP_GET_ELEMENT_ATTRIBUTES	0x20
 #define AVRCP_GET_PLAY_STATUS		0x30
 #define AVRCP_REGISTER_NOTIFICATION	0x31
+#define AVRCP_ABORT_CONTINUING		0x41
 
 /* Capabilities for AVRCP_GET_CAPABILITIES pdu */
 #define CAP_COMPANY_ID		0x02
@@ -129,6 +130,12 @@ struct avrcp_server {
 	struct avrcp_player *active_player;
 };
 
+struct pending_pdu {
+	uint8_t pdu_id;
+	GList *attr_ids;
+	unsigned int last_idx;
+};
+
 struct avrcp_player {
 	struct avrcp_server *server;
 	struct avctp *session;
@@ -137,6 +144,7 @@ struct avrcp_player {
 	unsigned int handler;
 	uint16_t registered_events;
 	uint8_t transaction_events[AVRCP_EVENT_LAST + 1];
+	struct pending_pdu *pending_pdu;
 
 	struct avrcp_player_cb *cb;
 	void *user_data;
@@ -462,6 +470,18 @@ done:
 	return sizeof(struct media_info_elem) + len;
 }
 
+static gboolean player_abort_pending_pdu(struct avrcp_player *player)
+{
+	if (player->pending_pdu == NULL)
+		return FALSE;
+
+	g_list_free(player->pending_pdu->attr_ids);
+	g_free(player->pending_pdu);
+	player->pending_pdu = NULL;
+
+	return TRUE;
+}
+
 static int player_set_attribute(struct avrcp_player *player,
 						uint8_t attr, uint8_t val)
 {
@@ -869,6 +889,33 @@ err:
 	return AVC_CTYPE_REJECTED;
 }
 
+static uint8_t avrcp_handle_abort_continuing(struct avrcp_player *player,
+						struct avrcp_header *pdu,
+						uint8_t transaction)
+{
+	uint16_t len = ntohs(pdu->params_len);
+	struct pending_pdu *pending;
+
+	if (len != 1 || player->pending_pdu == NULL)
+		goto err;
+
+	pending = player->pending_pdu;
+
+	if (pending->pdu_id != pdu->params[0])
+		goto err;
+
+	player_abort_pending_pdu(player);
+	pdu->params_len = 0;
+
+	return AVC_CTYPE_STABLE;
+
+err:
+	pdu->params_len = htons(1);
+	pdu->params[0] = E_INVALID_PARAM;
+	return AVC_CTYPE_REJECTED;
+}
+
+
 static struct pdu_handler {
 	uint8_t pdu_id;
 	uint8_t code;
@@ -900,6 +947,8 @@ static struct pdu_handler {
 					avrcp_handle_get_play_status },
 		{ AVRCP_REGISTER_NOTIFICATION, AVC_CTYPE_NOTIFY,
 					avrcp_handle_register_notification },
+		{ AVRCP_ABORT_CONTINUING, AVC_CTYPE_CONTROL,
+					avrcp_handle_abort_continuing },
 		{ },
 };
 
@@ -947,6 +996,10 @@ static size_t handle_vendordep_pdu(struct avctp *session, uint8_t transaction,
 
 	*code = handler->func(player, pdu, transaction);
 
+	if (*code != AVC_CTYPE_REJECTED &&
+				pdu->pdu_id != AVRCP_ABORT_CONTINUING)
+		player_abort_pending_pdu(player);
+
 	return AVRCP_HEADER_LENGTH + ntohs(pdu->params_len);
 
 err_metadata:
@@ -1100,6 +1153,8 @@ static void player_destroy(gpointer data)
 	if (player->destroy)
 		player->destroy(player->user_data);
 
+	player_abort_pending_pdu(player);
+
 	if (player->handler)
 		avctp_unregister_pdu_handler(player->handler);
 
-- 
1.7.7


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 5/5] AVRCP: Implement RequestContinuingResponse PDU
  2011-10-14 21:28 [PATCH 0/5] Request/Abort Continuing and some minor things Lucas De Marchi
                   ` (3 preceding siblings ...)
  2011-10-14 21:28 ` [PATCH 4/5] AVRCP: Implement AbortContinuingResponse PDU Lucas De Marchi
@ 2011-10-14 21:28 ` Lucas De Marchi
  2011-10-17  9:41   ` Luiz Augusto von Dentz
  4 siblings, 1 reply; 13+ messages in thread
From: Lucas De Marchi @ 2011-10-14 21:28 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Lucas De Marchi

---
 audio/avrcp.c |  215 ++++++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 158 insertions(+), 57 deletions(-)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index 1babb94..3c1b752 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -64,6 +64,12 @@
 #define E_PARAM_NOT_FOUND	0x02
 #define E_INTERNAL		0x03
 
+/* Packet types */
+#define AVRCP_PACKET_TYPE_SINGLE	0x00
+#define AVRCP_PACKET_TYPE_START		0x01
+#define AVRCP_PACKET_TYPE_CONTINUING	0x02
+#define AVRCP_PACKET_TYPE_END		0x03
+
 /* PDU types for metadata transfer */
 #define AVRCP_GET_CAPABILITIES		0x10
 #define AVRCP_LIST_PLAYER_ATTRIBUTES	0X11
@@ -77,6 +83,7 @@
 #define AVRCP_GET_ELEMENT_ATTRIBUTES	0x20
 #define AVRCP_GET_PLAY_STATUS		0x30
 #define AVRCP_REGISTER_NOTIFICATION	0x31
+#define AVRCP_REQUEST_CONTINUING	0x40
 #define AVRCP_ABORT_CONTINUING		0x41
 
 /* Capabilities for AVRCP_GET_CAPABILITIES pdu */
@@ -399,75 +406,119 @@ int avrcp_player_event(struct avrcp_player *player, uint8_t id, void *data)
 }
 
 /*
- * Copy media_info field to a buffer, intended to be used in a response to
- * GetElementAttributes message.
- *
- * It assumes there's enough space in the buffer and on success it returns the
- * size written.
+ * Write media attribute to buffer, taking care of writing the header when
+ * necessary, not writing more than @param maxlen and starting string from
+ * @param last_idx.
  *
- * If @param id is not valid, -EINVAL is returned. If there's no space left on
- * the buffer -ENOBUFS is returned.
+ * If there isn't enough space in the buffer even for the header, it returns
+ * -ENOBUFS. Otherwise it returns the size written to buffer and the position
+ *  within the media attribute in @param last_idx.
  */
-static int player_get_media_attribute(struct avrcp_player *player,
+static int player_write_media_attribute(struct avrcp_player *player,
 						uint32_t id, uint8_t *buf,
-						uint16_t maxlen)
+						uint16_t maxlen,
+						unsigned int *last_idx)
 {
-	struct media_info_elem {
+	struct media_attribute_header {
 		uint32_t id;
 		uint16_t charset;
 		uint16_t len;
-		uint8_t val[];
 	};
-	struct media_info_elem *elem = (void *)buf;
+	uint8_t *val;
+	struct media_attribute_header *hdr;
 	uint16_t len;
 	char valstr[20];
 	void *value;
 
-	if (maxlen < sizeof(struct media_info_elem))
-		return -ENOBUFS;
+	if (*last_idx == 0) {
+		/*
+		 * Write header first, but bail out if even that
+		 * doesn't fit in PDU.
+		 */
+		if (maxlen < sizeof(*hdr))
+			return -ENOBUFS;
 
-	/* Subtract the size of elem header from the available space */
-	maxlen -= sizeof(struct media_info_elem);
+		maxlen -= sizeof(*hdr);
+		hdr = (void *) buf;
+		hdr->id = htonl(id);
+		hdr->charset = htons(0x6A); /* Always use UTF-8 */
+		val = ((uint8_t *) hdr) + sizeof(*hdr);
+	} else {
+		hdr = NULL;
+		val = buf;
+	}
 
 	DBG("Get media attribute: %u", id);
 
-	if (id == AVRCP_MEDIA_ATTRIBUTE_ILLEGAL ||
-			id > AVRCP_MEDIA_ATTRIBUTE_LAST)
-		return -ENOENT;
-
 	value = player->cb->get_metadata(id, player->user_data);
 	if (value == NULL) {
-		len = 0;
-		goto done;
+		hdr->len = 0;
+		return sizeof(*hdr);
 	}
 
 	switch (id) {
-	case AVRCP_MEDIA_ATTRIBUTE_TITLE:
-	case AVRCP_MEDIA_ATTRIBUTE_ARTIST:
-	case AVRCP_MEDIA_ATTRIBUTE_ALBUM:
-	case AVRCP_MEDIA_ATTRIBUTE_GENRE:
-		len = strlen((char *) value);
-		if (len > maxlen)
-			return -ENOBUFS;
-		memcpy(elem->val, value, len);
-		break;
 	case AVRCP_MEDIA_ATTRIBUTE_TRACK:
 	case AVRCP_MEDIA_ATTRIBUTE_N_TRACKS:
 	case AVRCP_MEDIA_ATTRIBUTE_DURATION:
 		snprintf(valstr, 20, "%u", GPOINTER_TO_UINT(value));
-		len = strlen(valstr);
-		if (len > maxlen)
-			return -ENOBUFS;
-		memcpy(elem->val, valstr, len);
+		value = valstr;
 		break;
 	}
 
-done:
-	elem->id = htonl(id);
-	elem->charset = htons(0x6A); /* Always use UTF-8 */
-	elem->len = htons(len);
+	len = strlen(value);
+	if (hdr != NULL)
+		hdr->len = htons(len);
+
+	value = ((char *) value) + *last_idx;
+	len -= *last_idx;
+
+	if (len > maxlen) {
+		*last_idx += maxlen;
+		len = maxlen;
+	} else {
+		*last_idx = 0;
+	}
+
+	memcpy(val, value, len);
+
+	return val - buf + len;
+}
 
-	return sizeof(struct media_info_elem) + len;
+static GList *player_fill_media_attribute(struct avrcp_player *player,
+					GList *attr_ids, uint8_t *buf,
+					uint16_t *pos, unsigned int *last_idx)
+{
+	GList *l;
+
+	for (l = attr_ids; l != NULL; l = g_list_delete_link(l, l)) {
+		uint32_t attr = GPOINTER_TO_UINT(l->data);
+		int size;
+
+		size = player_write_media_attribute(player, attr, &buf[*pos],
+							AVRCP_PDU_MTU - *pos,
+							last_idx);
+		if (size < 0)
+			break;
+
+		*pos += size;
+
+		if (*last_idx)
+			break;
+	}
+
+	return l;
+}
+
+static struct pending_pdu *pending_pdu_new(uint8_t pdu_id, GList *attr_ids,
+							unsigned int last_idx)
+{
+	struct pending_pdu *pending = g_new(struct pending_pdu, 1);
+
+	pending->pdu_id = pdu_id;
+	pending->attr_ids = attr_ids;
+	pending->last_idx = last_idx;
+
+	return pending;
 }
 
 static gboolean player_abort_pending_pdu(struct avrcp_player *player)
@@ -611,8 +662,8 @@ static uint8_t avrcp_handle_get_element_attributes(struct avrcp_player *player,
 	uint64_t *identifier = (uint64_t *) &pdu->params[0];
 	uint16_t pos;
 	uint8_t nattr;
-	int size;
-	GList *l, *attr_ids;
+	GList *attr_ids;
+	unsigned int last_idx;
 
 	if (len < 9 || *identifier != 0)
 		goto err;
@@ -628,12 +679,20 @@ static uint8_t avrcp_handle_get_element_attributes(struct avrcp_player *player,
 		 * title must be returned if there's a track selected.
 		 */
 		attr_ids = player->cb->list_metadata(player->user_data);
+		len = g_list_length(attr_ids);
 	} else {
 		unsigned int i;
 		uint32_t *attr = (uint32_t *) &pdu->params[9];
 
-		for (i = 0, attr_ids = NULL; i < nattr; i++, attr++) {
+		for (i = 0, len = 0, attr_ids = NULL; i < nattr; i++, attr++) {
 			uint32_t id = ntohl(bt_get_unaligned(attr));
+
+			/* Don't add invalid attributes */
+			if (id == AVRCP_MEDIA_ATTRIBUTE_ILLEGAL ||
+					id > AVRCP_MEDIA_ATTRIBUTE_LAST)
+				continue;
+
+			len++;
 			attr_ids = g_list_prepend(attr_ids,
 							GUINT_TO_POINTER(id));
 		}
@@ -641,24 +700,21 @@ static uint8_t avrcp_handle_get_element_attributes(struct avrcp_player *player,
 		attr_ids = g_list_reverse(attr_ids);
 	}
 
-	for (l = attr_ids, len = 0, pos = 1; l != NULL; l = l->next) {
-		uint32_t attr = GPOINTER_TO_UINT(l->data);
+	if (!len)
+		goto err;
 
-		size = player_get_media_attribute(player, attr,
-							&pdu->params[pos],
-							AVRCP_PDU_MTU - pos);
+	player_abort_pending_pdu(player);
+	pos = 1;
+	last_idx = 0;
+	attr_ids = player_fill_media_attribute(player, attr_ids, pdu->params,
+							&pos, &last_idx);
 
-		if (size >= 0) {
-			len++;
-			pos += size;
-		}
+	if (attr_ids != NULL) {
+		player->pending_pdu = pending_pdu_new(pdu->pdu_id, attr_ids,
+								last_idx);
+		pdu->packet_type = AVRCP_PACKET_TYPE_START;
 	}
 
-	g_list_free(attr_ids);
-
-	if (!len)
-		goto err;
-
 	pdu->params[0] = len;
 	pdu->params_len = htons(pos);
 
@@ -889,6 +945,46 @@ err:
 	return AVC_CTYPE_REJECTED;
 }
 
+static uint8_t avrcp_handle_request_continuing(struct avrcp_player *player,
+						struct avrcp_header *pdu,
+						uint8_t transaction)
+{
+	uint16_t len = ntohs(pdu->params_len);
+	struct pending_pdu *pending;
+
+	if (len != 1 || player->pending_pdu == NULL)
+		goto err;
+
+	pending = player->pending_pdu;
+
+	if (pending->pdu_id != pdu->params[0])
+		goto err;
+
+
+	len = 0;
+	pending->attr_ids = player_fill_media_attribute(player,
+							pending->attr_ids,
+							pdu->params, &len,
+							&pending->last_idx);
+	pdu->pdu_id = pending->pdu_id;
+
+	if (pending->attr_ids == NULL) {
+		g_free(player->pending_pdu);
+		player->pending_pdu = NULL;
+		pdu->packet_type = AVRCP_PACKET_TYPE_END;
+	} else {
+		pdu->packet_type = AVRCP_PACKET_TYPE_CONTINUING;
+	}
+
+	pdu->params_len = htons(len);
+
+	return AVC_CTYPE_STABLE;
+err:
+	pdu->params_len = htons(1);
+	pdu->params[0] = E_INVALID_PARAM;
+	return AVC_CTYPE_REJECTED;
+}
+
 static uint8_t avrcp_handle_abort_continuing(struct avrcp_player *player,
 						struct avrcp_header *pdu,
 						uint8_t transaction)
@@ -947,8 +1043,11 @@ static struct pdu_handler {
 					avrcp_handle_get_play_status },
 		{ AVRCP_REGISTER_NOTIFICATION, AVC_CTYPE_NOTIFY,
 					avrcp_handle_register_notification },
+		{ AVRCP_REQUEST_CONTINUING, AVC_CTYPE_CONTROL,
+					avrcp_handle_request_continuing },
 		{ AVRCP_ABORT_CONTINUING, AVC_CTYPE_CONTROL,
 					avrcp_handle_abort_continuing },
+
 		{ },
 };
 
@@ -997,6 +1096,8 @@ static size_t handle_vendordep_pdu(struct avctp *session, uint8_t transaction,
 	*code = handler->func(player, pdu, transaction);
 
 	if (*code != AVC_CTYPE_REJECTED &&
+				pdu->pdu_id != AVRCP_GET_ELEMENT_ATTRIBUTES &&
+				pdu->pdu_id != AVRCP_REQUEST_CONTINUING &&
 				pdu->pdu_id != AVRCP_ABORT_CONTINUING)
 		player_abort_pending_pdu(player);
 
-- 
1.7.7


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] AVRCP: Use track's UID in event notification
  2011-10-14 21:28 ` [PATCH 1/5] AVRCP: Use track's UID in event notification Lucas De Marchi
@ 2011-10-17  7:35   ` Luiz Augusto von Dentz
  0 siblings, 0 replies; 13+ messages in thread
From: Luiz Augusto von Dentz @ 2011-10-17  7:35 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: linux-bluetooth

Hi Lucas,

On Sat, Oct 15, 2011 at 12:28 AM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> When registering for TRACK_CHANGED event, CT expects the track's UID in
> the INTERIM and CHANGED responses. According to AVRCP 1.3 spec, there
> are only 2 possible values:
>        1) 0: there's a track selected
>        2) UINT32_MAX: there's no track selected
>
> AVRCP 1.4 reserves the value UINT64_MAX for the second case. Since this
> later value is the one used in certification process for best IOP
> we return UINT64_MAX instead of the former.
>
> This implementation allows to pass PTS test TP/NFY/BV-05-C.
> ---
>  audio/avrcp.c |   16 +++++-----------
>  audio/avrcp.h |    1 +
>  audio/media.c |   17 ++++++++++++++++-
>  3 files changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index ac7c8d4..076fdf4 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -363,18 +363,11 @@ int avrcp_player_event(struct avrcp_player *player, uint8_t id, void *data)
>                pdu->params[1] = *((uint8_t *)data);
>
>                break;
> -       case AVRCP_EVENT_TRACK_CHANGED: {
> +       case AVRCP_EVENT_TRACK_CHANGED:
>                size = 9;
> -
> -               /*
> -                * AVRCP 1.3 supports only one track identifier: PLAYING
> -                * (0x0). When 1.4 version is added, this shall be changed to
> -                * contain the identifier of the track.
> -                */
> -               memset(&pdu->params[1], 0, 8);
> +               memcpy(&pdu->params[1], data, sizeof(uint64_t));
>
>                break;
> -       }
>        default:
>                error("Unknown event %u", id);
>                return -EINVAL;
> @@ -828,6 +821,7 @@ static uint8_t avrcp_handle_register_notification(struct avrcp_player *player,
>                                                uint8_t transaction)
>  {
>        uint16_t len = ntohs(pdu->params_len);
> +       uint64_t uid;
>
>        /*
>         * 1 byte for EventID, 4 bytes for Playback interval but the latest
> @@ -845,8 +839,8 @@ static uint8_t avrcp_handle_register_notification(struct avrcp_player *player,
>                break;
>        case AVRCP_EVENT_TRACK_CHANGED:
>                len = 9;
> -
> -               memset(&pdu->params[1], 0, 8);
> +               uid = player->cb->get_uid(player->user_data);
> +               memcpy(&pdu->params[1], &uid, sizeof(uint64_t));
>
>                break;
>        default:
> diff --git a/audio/avrcp.h b/audio/avrcp.h
> index 360a80a..8cf95a4 100644
> --- a/audio/avrcp.h
> +++ b/audio/avrcp.h
> @@ -75,6 +75,7 @@
>  struct avrcp_player_cb {
>        int (*get_setting) (uint8_t attr, void *user_data);
>        int (*set_setting) (uint8_t attr, uint8_t value, void *user_data);
> +       uint64_t (*get_uid) (void *user_data);
>        void *(*get_metadata) (uint32_t id, void *user_data);
>        GList *(*list_metadata) (void *user_data);
>        uint8_t (*get_status) (void *user_data);
> diff --git a/audio/media.c b/audio/media.c
> index 519cafe..9ef393b 100644
> --- a/audio/media.c
> +++ b/audio/media.c
> @@ -1168,6 +1168,18 @@ static GList *list_metadata(void *user_data)
>        return g_hash_table_get_keys(mp->track);
>  }
>
> +static uint64_t get_uid(void *user_data)
> +{
> +       struct media_player *mp = user_data;
> +
> +       DBG("%p", mp->track);
> +
> +       if (mp->track == NULL)
> +               return UINT64_MAX;
> +
> +       return 0;
> +}
> +
>  static void *get_metadata(uint32_t id, void *user_data)
>  {
>        struct media_player *mp = user_data;
> @@ -1220,6 +1232,7 @@ static struct avrcp_player_cb player_cb = {
>        .get_setting = get_setting,
>        .set_setting = set_setting,
>        .list_metadata = list_metadata,
> +       .get_uid = get_uid,
>        .get_metadata = get_metadata,
>        .get_position = get_position,
>        .get_status = get_status
> @@ -1369,6 +1382,7 @@ static gboolean parse_player_metadata(struct media_player *mp,
>        GHashTable *track;
>        int ctype;
>        gboolean title = FALSE;
> +       uint64_t uid;
>
>        ctype = dbus_message_iter_get_arg_type(iter);
>        if (ctype != DBUS_TYPE_ARRAY)
> @@ -1466,8 +1480,9 @@ static gboolean parse_player_metadata(struct media_player *mp,
>        mp->track = track;
>        mp->position = 0;
>        g_timer_start(mp->timer);
> +       uid = get_uid(mp);
>
> -       avrcp_player_event(mp->player, AVRCP_EVENT_TRACK_CHANGED, NULL);
> +       avrcp_player_event(mp->player, AVRCP_EVENT_TRACK_CHANGED, &uid);
>
>        return TRUE;
>
> --
> 1.7.7
>

Ack.

-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/5] AVRCP: implement TRACK-REACHED-START event
  2011-10-14 21:28 ` [PATCH 2/5] AVRCP: implement TRACK-REACHED-START event Lucas De Marchi
@ 2011-10-17  7:40   ` Luiz Augusto von Dentz
  0 siblings, 0 replies; 13+ messages in thread
From: Luiz Augusto von Dentz @ 2011-10-17  7:40 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: linux-bluetooth

Hi Lucas,

On Sat, Oct 15, 2011 at 12:28 AM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> ---
>  audio/avrcp.c |   13 ++++++++++---
>  audio/avrcp.h |    2 ++
>  audio/media.c |    5 +++++
>  3 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index 076fdf4..8b48e83 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -136,7 +136,7 @@ struct avrcp_player {
>
>        unsigned int handler;
>        uint16_t registered_events;
> -       uint8_t transaction_events[AVRCP_EVENT_TRACK_CHANGED + 1];
> +       uint8_t transaction_events[AVRCP_EVENT_LAST + 1];
>
>        struct avrcp_player_cb *cb;
>        void *user_data;
> @@ -368,6 +368,9 @@ int avrcp_player_event(struct avrcp_player *player, uint8_t id, void *data)
>                memcpy(&pdu->params[1], data, sizeof(uint64_t));
>
>                break;
> +       case AVRCP_EVENT_TRACK_REACHED_START:
> +               size = 1;
> +               break;
>        default:
>                error("Unknown event %u", id);
>                return -EINVAL;
> @@ -504,10 +507,11 @@ static uint8_t avrcp_handle_get_capabilities(struct avrcp_player *player,
>
>                return AVC_CTYPE_STABLE;
>        case CAP_EVENTS_SUPPORTED:
> -               pdu->params_len = htons(4);
> -               pdu->params[1] = 2;
> +               pdu->params_len = htons(5);
> +               pdu->params[1] = 3;
>                pdu->params[2] = AVRCP_EVENT_STATUS_CHANGED;
>                pdu->params[3] = AVRCP_EVENT_TRACK_CHANGED;
> +               pdu->params[4] = AVRCP_EVENT_TRACK_REACHED_START;
>
>                return AVC_CTYPE_STABLE;
>        }
> @@ -843,6 +847,9 @@ static uint8_t avrcp_handle_register_notification(struct avrcp_player *player,
>                memcpy(&pdu->params[1], &uid, sizeof(uint64_t));
>
>                break;
> +       case AVRCP_EVENT_TRACK_REACHED_START:
> +               len =1;
> +               break;
>        default:
>                /* All other events are not supported yet */
>                goto err;
> diff --git a/audio/avrcp.h b/audio/avrcp.h
> index 8cf95a4..c798658 100644
> --- a/audio/avrcp.h
> +++ b/audio/avrcp.h
> @@ -71,6 +71,8 @@
>  /* Notification events */
>  #define AVRCP_EVENT_STATUS_CHANGED     0x01
>  #define AVRCP_EVENT_TRACK_CHANGED      0x02
> +#define AVRCP_EVENT_TRACK_REACHED_START        0x04
> +#define AVRCP_EVENT_LAST               AVRCP_EVENT_TRACK_REACHED_START
>
>  struct avrcp_player_cb {
>        int (*get_setting) (uint8_t attr, void *user_data);
> diff --git a/audio/media.c b/audio/media.c
> index 9ef393b..a7647ea 100644
> --- a/audio/media.c
> +++ b/audio/media.c
> @@ -1289,6 +1289,11 @@ static gboolean set_position(struct media_player *mp, DBusMessageIter *iter)
>        mp->position = value;
>        g_timer_start(mp->timer);
>
> +       if (!mp->position) {
> +               avrcp_player_event(mp->player,
> +                                       AVRCP_EVENT_TRACK_REACHED_START, NULL);
> +       }
> +

Ack.

-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/5] AVRCP: respond with UINT32_MAX if duration is not available
  2011-10-14 21:28 ` [PATCH 3/5] AVRCP: respond with UINT32_MAX if duration is not available Lucas De Marchi
@ 2011-10-17  7:57   ` Luiz Augusto von Dentz
  2011-10-17 12:13     ` Lucas De Marchi
  0 siblings, 1 reply; 13+ messages in thread
From: Luiz Augusto von Dentz @ 2011-10-17  7:57 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: linux-bluetooth

Hi Lucas,

On Sat, Oct 15, 2011 at 12:28 AM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> Section 5.4.1 of AVRCP 1.3 spec says:
>
>        If TG does not support SongLength And SongPosition on TG, then TG shall
>        return 0xFFFFFFFF.
>
> SongPosition is always available, but song length depends on user to
> provied it.
> ---
>  audio/avrcp.c     |    2 +-
>  doc/media-api.txt |    4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index 8b48e83..1b55e33 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -808,7 +808,7 @@ static uint8_t avrcp_handle_get_play_status(struct avrcp_player *player,
>                                                AVRCP_MEDIA_ATTRIBUTE_DURATION,
>                                                player->user_data));
>
> -       duration = htonl(duration);
> +       duration = duration ? htonl(duration) : UINT32_MAX;
>        position = htonl(position);

I would prefer doing this on RegisterPlayer/parse_player_metadata as
we do with title, so if is not present then we set UINT32_MAX.

>        memcpy(&pdu->params[0], &duration, 4);
> diff --git a/doc/media-api.txt b/doc/media-api.txt
> index b8dcdbd..204f33e 100644
> --- a/doc/media-api.txt
> +++ b/doc/media-api.txt
> @@ -114,7 +114,9 @@ Methods             void RegisterEndpoint(object endpoint, dict properties)
>
>                                uint32 Duration:
>
> -                                       Track duration in milliseconds
> +                                       Track duration in milliseconds. If it's
> +                                       set to 0 or left blank, it will be
> +                                       treated as not available.

0 might be a valid duration so I would just treat as not available if
Duration is not part of the metadata or if the value is actually
UINT32_MAX which can happen if the duration exceeds 32 bits.


-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/5] AVRCP: Implement AbortContinuingResponse PDU
  2011-10-14 21:28 ` [PATCH 4/5] AVRCP: Implement AbortContinuingResponse PDU Lucas De Marchi
@ 2011-10-17  8:05   ` Luiz Augusto von Dentz
  0 siblings, 0 replies; 13+ messages in thread
From: Luiz Augusto von Dentz @ 2011-10-17  8:05 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: linux-bluetooth

Hi Lucas,

On Sat, Oct 15, 2011 at 12:28 AM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> ---
>  audio/avrcp.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 55 insertions(+), 0 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index 1b55e33..1babb94 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -77,6 +77,7 @@
>  #define AVRCP_GET_ELEMENT_ATTRIBUTES   0x20
>  #define AVRCP_GET_PLAY_STATUS          0x30
>  #define AVRCP_REGISTER_NOTIFICATION    0x31
> +#define AVRCP_ABORT_CONTINUING         0x41
>
>  /* Capabilities for AVRCP_GET_CAPABILITIES pdu */
>  #define CAP_COMPANY_ID         0x02
> @@ -129,6 +130,12 @@ struct avrcp_server {
>        struct avrcp_player *active_player;
>  };
>
> +struct pending_pdu {
> +       uint8_t pdu_id;
> +       GList *attr_ids;
> +       unsigned int last_idx;
> +};
> +
>  struct avrcp_player {
>        struct avrcp_server *server;
>        struct avctp *session;
> @@ -137,6 +144,7 @@ struct avrcp_player {
>        unsigned int handler;
>        uint16_t registered_events;
>        uint8_t transaction_events[AVRCP_EVENT_LAST + 1];
> +       struct pending_pdu *pending_pdu;
>
>        struct avrcp_player_cb *cb;
>        void *user_data;
> @@ -462,6 +470,18 @@ done:
>        return sizeof(struct media_info_elem) + len;
>  }
>
> +static gboolean player_abort_pending_pdu(struct avrcp_player *player)
> +{
> +       if (player->pending_pdu == NULL)
> +               return FALSE;
> +
> +       g_list_free(player->pending_pdu->attr_ids);
> +       g_free(player->pending_pdu);
> +       player->pending_pdu = NULL;
> +
> +       return TRUE;
> +}
> +
>  static int player_set_attribute(struct avrcp_player *player,
>                                                uint8_t attr, uint8_t val)
>  {
> @@ -869,6 +889,33 @@ err:
>        return AVC_CTYPE_REJECTED;
>  }
>
> +static uint8_t avrcp_handle_abort_continuing(struct avrcp_player *player,
> +                                               struct avrcp_header *pdu,
> +                                               uint8_t transaction)
> +{
> +       uint16_t len = ntohs(pdu->params_len);
> +       struct pending_pdu *pending;
> +
> +       if (len != 1 || player->pending_pdu == NULL)
> +               goto err;
> +
> +       pending = player->pending_pdu;
> +
> +       if (pending->pdu_id != pdu->params[0])
> +               goto err;
> +
> +       player_abort_pending_pdu(player);
> +       pdu->params_len = 0;
> +
> +       return AVC_CTYPE_STABLE;
> +
> +err:
> +       pdu->params_len = htons(1);
> +       pdu->params[0] = E_INVALID_PARAM;
> +       return AVC_CTYPE_REJECTED;
> +}
> +
> +
>  static struct pdu_handler {
>        uint8_t pdu_id;
>        uint8_t code;
> @@ -900,6 +947,8 @@ static struct pdu_handler {
>                                        avrcp_handle_get_play_status },
>                { AVRCP_REGISTER_NOTIFICATION, AVC_CTYPE_NOTIFY,
>                                        avrcp_handle_register_notification },
> +               { AVRCP_ABORT_CONTINUING, AVC_CTYPE_CONTROL,
> +                                       avrcp_handle_abort_continuing },
>                { },
>  };
>
> @@ -947,6 +996,10 @@ static size_t handle_vendordep_pdu(struct avctp *session, uint8_t transaction,
>
>        *code = handler->func(player, pdu, transaction);
>
> +       if (*code != AVC_CTYPE_REJECTED &&
> +                               pdu->pdu_id != AVRCP_ABORT_CONTINUING)
> +               player_abort_pending_pdu(player);
> +
>        return AVRCP_HEADER_LENGTH + ntohs(pdu->params_len);
>
>  err_metadata:
> @@ -1100,6 +1153,8 @@ static void player_destroy(gpointer data)
>        if (player->destroy)
>                player->destroy(player->user_data);
>
> +       player_abort_pending_pdu(player);
> +
>        if (player->handler)
>                avctp_unregister_pdu_handler(player->handler);
>
> --
> 1.7.7

Ack.

-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 5/5] AVRCP: Implement RequestContinuingResponse PDU
  2011-10-14 21:28 ` [PATCH 5/5] AVRCP: Implement RequestContinuingResponse PDU Lucas De Marchi
@ 2011-10-17  9:41   ` Luiz Augusto von Dentz
  2011-10-17 12:27     ` Lucas De Marchi
  0 siblings, 1 reply; 13+ messages in thread
From: Luiz Augusto von Dentz @ 2011-10-17  9:41 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: linux-bluetooth

Hi Lucas,

On Sat, Oct 15, 2011 at 12:28 AM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> ---
>  audio/avrcp.c |  215 ++++++++++++++++++++++++++++++++++++++++++---------------
>  1 files changed, 158 insertions(+), 57 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index 1babb94..3c1b752 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -64,6 +64,12 @@
>  #define E_PARAM_NOT_FOUND      0x02
>  #define E_INTERNAL             0x03
>
> +/* Packet types */
> +#define AVRCP_PACKET_TYPE_SINGLE       0x00
> +#define AVRCP_PACKET_TYPE_START                0x01
> +#define AVRCP_PACKET_TYPE_CONTINUING   0x02
> +#define AVRCP_PACKET_TYPE_END          0x03
> +
>  /* PDU types for metadata transfer */
>  #define AVRCP_GET_CAPABILITIES         0x10
>  #define AVRCP_LIST_PLAYER_ATTRIBUTES   0X11
> @@ -77,6 +83,7 @@
>  #define AVRCP_GET_ELEMENT_ATTRIBUTES   0x20
>  #define AVRCP_GET_PLAY_STATUS          0x30
>  #define AVRCP_REGISTER_NOTIFICATION    0x31
> +#define AVRCP_REQUEST_CONTINUING       0x40
>  #define AVRCP_ABORT_CONTINUING         0x41
>
>  /* Capabilities for AVRCP_GET_CAPABILITIES pdu */
> @@ -399,75 +406,119 @@ int avrcp_player_event(struct avrcp_player *player, uint8_t id, void *data)
>  }
>
>  /*
> - * Copy media_info field to a buffer, intended to be used in a response to
> - * GetElementAttributes message.
> - *
> - * It assumes there's enough space in the buffer and on success it returns the
> - * size written.
> + * Write media attribute to buffer, taking care of writing the header when
> + * necessary, not writing more than @param maxlen and starting string from
> + * @param last_idx.
>  *
> - * If @param id is not valid, -EINVAL is returned. If there's no space left on
> - * the buffer -ENOBUFS is returned.
> + * If there isn't enough space in the buffer even for the header, it returns
> + * -ENOBUFS. Otherwise it returns the size written to buffer and the position
> + *  within the media attribute in @param last_idx.
>  */
> -static int player_get_media_attribute(struct avrcp_player *player,
> +static int player_write_media_attribute(struct avrcp_player *player,
>                                                uint32_t id, uint8_t *buf,
> -                                               uint16_t maxlen)
> +                                               uint16_t maxlen,
> +                                               unsigned int *last_idx)
>  {
> -       struct media_info_elem {
> +       struct media_attribute_header {
>                uint32_t id;
>                uint16_t charset;
>                uint16_t len;
> -               uint8_t val[];
>        };
> -       struct media_info_elem *elem = (void *)buf;
> +       uint8_t *val;
> +       struct media_attribute_header *hdr;
>        uint16_t len;
>        char valstr[20];
>        void *value;
>
> -       if (maxlen < sizeof(struct media_info_elem))
> -               return -ENOBUFS;
> +       if (*last_idx == 0) {
> +               /*
> +                * Write header first, but bail out if even that
> +                * doesn't fit in PDU.
> +                */
> +               if (maxlen < sizeof(*hdr))
> +                       return -ENOBUFS;
>
> -       /* Subtract the size of elem header from the available space */
> -       maxlen -= sizeof(struct media_info_elem);
> +               maxlen -= sizeof(*hdr);
> +               hdr = (void *) buf;
> +               hdr->id = htonl(id);
> +               hdr->charset = htons(0x6A); /* Always use UTF-8 */
> +               val = ((uint8_t *) hdr) + sizeof(*hdr);
> +       } else {
> +               hdr = NULL;
> +               val = buf;
> +       }
>
>        DBG("Get media attribute: %u", id);
>
> -       if (id == AVRCP_MEDIA_ATTRIBUTE_ILLEGAL ||
> -                       id > AVRCP_MEDIA_ATTRIBUTE_LAST)
> -               return -ENOENT;
> -
>        value = player->cb->get_metadata(id, player->user_data);
>        if (value == NULL) {
> -               len = 0;
> -               goto done;
> +               hdr->len = 0;
> +               return sizeof(*hdr);
>        }
>
>        switch (id) {
> -       case AVRCP_MEDIA_ATTRIBUTE_TITLE:
> -       case AVRCP_MEDIA_ATTRIBUTE_ARTIST:
> -       case AVRCP_MEDIA_ATTRIBUTE_ALBUM:
> -       case AVRCP_MEDIA_ATTRIBUTE_GENRE:
> -               len = strlen((char *) value);
> -               if (len > maxlen)
> -                       return -ENOBUFS;
> -               memcpy(elem->val, value, len);
> -               break;
>        case AVRCP_MEDIA_ATTRIBUTE_TRACK:
>        case AVRCP_MEDIA_ATTRIBUTE_N_TRACKS:
>        case AVRCP_MEDIA_ATTRIBUTE_DURATION:
>                snprintf(valstr, 20, "%u", GPOINTER_TO_UINT(value));
> -               len = strlen(valstr);
> -               if (len > maxlen)
> -                       return -ENOBUFS;
> -               memcpy(elem->val, valstr, len);
> +               value = valstr;
>                break;
>        }
>
> -done:
> -       elem->id = htonl(id);
> -       elem->charset = htons(0x6A); /* Always use UTF-8 */
> -       elem->len = htons(len);
> +       len = strlen(value);
> +       if (hdr != NULL)
> +               hdr->len = htons(len);
> +
> +       value = ((char *) value) + *last_idx;
> +       len -= *last_idx;
> +
> +       if (len > maxlen) {
> +               *last_idx += maxlen;
> +               len = maxlen;
> +       } else {
> +               *last_idx = 0;
> +       }
> +
> +       memcpy(val, value, len);
> +
> +       return val - buf + len;

So the reason you have val - buf is to track if header was written or
not, right? In that case I would suggest using something like hdr ?
sizeof(*hdr) + len : len or perhaps moving the header filling to
player_fill_media_attribute might simplify this, so you only have to
keep track of the written value in player_write_media_attribute.

> +}
>
> -       return sizeof(struct media_info_elem) + len;
> +static GList *player_fill_media_attribute(struct avrcp_player *player,
> +                                       GList *attr_ids, uint8_t *buf,
> +                                       uint16_t *pos, unsigned int *last_idx)
> +{
> +       GList *l;
> +
> +       for (l = attr_ids; l != NULL; l = g_list_delete_link(l, l)) {
> +               uint32_t attr = GPOINTER_TO_UINT(l->data);
> +               int size;
> +
> +               size = player_write_media_attribute(player, attr, &buf[*pos],
> +                                                       AVRCP_PDU_MTU - *pos,
> +                                                       last_idx);
> +               if (size < 0)
> +                       break;
> +
> +               *pos += size;
> +
> +               if (*last_idx)
> +                       break;
> +       }
> +
> +       return l;
> +}
> +
> -               for (i = 0, attr_ids = NULL; i < nattr; i++, attr++) {
> +               for (i = 0, len = 0, attr_ids = NULL; i < nattr; i++, attr++) {
>                        uint32_t id = ntohl(bt_get_unaligned(attr));
> +
> +                       /* Don't add invalid attributes */
> +                       if (id == AVRCP_MEDIA_ATTRIBUTE_ILLEGAL ||
> +                                       id > AVRCP_MEDIA_ATTRIBUTE_LAST)
> +                               continue;
> +
> +                       len++;
>                        attr_ids = g_list_prepend(attr_ids,
>                                                        GUINT_TO_POINTER(id));
>                }

This sounds like a different bug, I mean not checking for invalid
attributes, if it was introduced in the previous patch can you please
fix it in place?

> @@ -641,24 +700,21 @@ static uint8_t avrcp_handle_get_element_attributes(struct avrcp_player *player,
>                attr_ids = g_list_reverse(attr_ids);
>        }
>
> -       for (l = attr_ids, len = 0, pos = 1; l != NULL; l = l->next) {
> -               uint32_t attr = GPOINTER_TO_UINT(l->data);
> +       if (!len)
> +               goto err;
>
> -               size = player_get_media_attribute(player, attr,
> -                                                       &pdu->params[pos],
> -                                                       AVRCP_PDU_MTU - pos);
> +       player_abort_pending_pdu(player);
> +       pos = 1;
> +       last_idx = 0;
> +       attr_ids = player_fill_media_attribute(player, attr_ids, pdu->params,
> +                                                       &pos, &last_idx);
>
> -               if (size >= 0) {
> -                       len++;
> -                       pos += size;
> -               }
> +       if (attr_ids != NULL) {
> +               player->pending_pdu = pending_pdu_new(pdu->pdu_id, attr_ids,
> +                                                               last_idx);
> +               pdu->packet_type = AVRCP_PACKET_TYPE_START;
>        }
>
> -       g_list_free(attr_ids);
> -
> -       if (!len)
> -               goto err;
> -
>        pdu->params[0] = len;
>        pdu->params_len = htons(pos);
>
> @@ -889,6 +945,46 @@ err:
>        return AVC_CTYPE_REJECTED;
>  }
>
> +static uint8_t avrcp_handle_request_continuing(struct avrcp_player *player,
> +                                               struct avrcp_header *pdu,
> +                                               uint8_t transaction)
> +{
> +       uint16_t len = ntohs(pdu->params_len);
> +       struct pending_pdu *pending;
> +
> +       if (len != 1 || player->pending_pdu == NULL)
> +               goto err;
> +
> +       pending = player->pending_pdu;
> +
> +       if (pending->pdu_id != pdu->params[0])
> +               goto err;
> +
> +
> +       len = 0;
> +       pending->attr_ids = player_fill_media_attribute(player,
> +                                                       pending->attr_ids,
> +                                                       pdu->params, &len,
> +                                                       &pending->last_idx);
> +       pdu->pdu_id = pending->pdu_id;
> +
> +       if (pending->attr_ids == NULL) {
> +               g_free(player->pending_pdu);
> +               player->pending_pdu = NULL;
> +               pdu->packet_type = AVRCP_PACKET_TYPE_END;
> +       } else {
> +               pdu->packet_type = AVRCP_PACKET_TYPE_CONTINUING;
> +       }
> +
> +       pdu->params_len = htons(len);
> +
> +       return AVC_CTYPE_STABLE;
> +err:
> +       pdu->params_len = htons(1);
> +       pdu->params[0] = E_INVALID_PARAM;
> +       return AVC_CTYPE_REJECTED;
> +}
> +
>  static uint8_t avrcp_handle_abort_continuing(struct avrcp_player *player,
>                                                struct avrcp_header *pdu,
>                                                uint8_t transaction)
> @@ -947,8 +1043,11 @@ static struct pdu_handler {
>                                        avrcp_handle_get_play_status },
>                { AVRCP_REGISTER_NOTIFICATION, AVC_CTYPE_NOTIFY,
>                                        avrcp_handle_register_notification },
> +               { AVRCP_REQUEST_CONTINUING, AVC_CTYPE_CONTROL,
> +                                       avrcp_handle_request_continuing },
>                { AVRCP_ABORT_CONTINUING, AVC_CTYPE_CONTROL,
>                                        avrcp_handle_abort_continuing },
> +

Empty line above is not really needed.


-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/5] AVRCP: respond with UINT32_MAX if duration is not available
  2011-10-17  7:57   ` Luiz Augusto von Dentz
@ 2011-10-17 12:13     ` Lucas De Marchi
  0 siblings, 0 replies; 13+ messages in thread
From: Lucas De Marchi @ 2011-10-17 12:13 UTC (permalink / raw)
  To: Luiz Augusto von Dentz; +Cc: linux-bluetooth

Hi Luiz,

On Mon, Oct 17, 2011 at 5:57 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Lucas,
>
> On Sat, Oct 15, 2011 at 12:28 AM, Lucas De Marchi
> <lucas.demarchi@profusion.mobi> wrote:
>> Section 5.4.1 of AVRCP 1.3 spec says:
>>
>>        If TG does not support SongLength And SongPosition on TG, then TG shall
>>        return 0xFFFFFFFF.
>>
>> SongPosition is always available, but song length depends on user to
>> provied it.
>> ---
>>  audio/avrcp.c     |    2 +-
>>  doc/media-api.txt |    4 +++-
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/audio/avrcp.c b/audio/avrcp.c
>> index 8b48e83..1b55e33 100644
>> --- a/audio/avrcp.c
>> +++ b/audio/avrcp.c
>> @@ -808,7 +808,7 @@ static uint8_t avrcp_handle_get_play_status(struct avrcp_player *player,
>>                                                AVRCP_MEDIA_ATTRIBUTE_DURATION,
>>                                                player->user_data));
>>
>> -       duration = htonl(duration);
>> +       duration = duration ? htonl(duration) : UINT32_MAX;
>>        position = htonl(position);
>
> I would prefer doing this on RegisterPlayer/parse_player_metadata as
> we do with title, so if is not present then we set UINT32_MAX.

Humn... I don't know if this is good. 'Duration' is replied on both
GetElementAttributes and GetPlayStatus commands and they have
different ways to inform about a unavailable attribute. While in the
former we should use zero-length string, the later uses UINT32_MAX.


>
>>        memcpy(&pdu->params[0], &duration, 4);
>> diff --git a/doc/media-api.txt b/doc/media-api.txt
>> index b8dcdbd..204f33e 100644
>> --- a/doc/media-api.txt
>> +++ b/doc/media-api.txt
>> @@ -114,7 +114,9 @@ Methods             void RegisterEndpoint(object endpoint, dict properties)
>>
>>                                uint32 Duration:
>>
>> -                                       Track duration in milliseconds
>> +                                       Track duration in milliseconds. If it's
>> +                                       set to 0 or left blank, it will be
>> +                                       treated as not available.
>
> 0 might be a valid duration so I would just treat as not available if
> Duration is not part of the metadata or if the value is actually
> UINT32_MAX which can happen if the duration exceeds 32 bits.

Ok.

Lucas De Marchi

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 5/5] AVRCP: Implement RequestContinuingResponse PDU
  2011-10-17  9:41   ` Luiz Augusto von Dentz
@ 2011-10-17 12:27     ` Lucas De Marchi
  0 siblings, 0 replies; 13+ messages in thread
From: Lucas De Marchi @ 2011-10-17 12:27 UTC (permalink / raw)
  To: Luiz Augusto von Dentz; +Cc: linux-bluetooth

Hi Luiz,

On Mon, Oct 17, 2011 at 7:41 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Lucas,
>
> On Sat, Oct 15, 2011 at 12:28 AM, Lucas De Marchi
> <lucas.demarchi@profusion.mobi> wrote:
>> ---
>>  audio/avrcp.c |  215 ++++++++++++++++++++++++++++++++++++++++++---------------
>>  1 files changed, 158 insertions(+), 57 deletions(-)
>>
>> diff --git a/audio/avrcp.c b/audio/avrcp.c
>> index 1babb94..3c1b752 100644
>> --- a/audio/avrcp.c
>> +++ b/audio/avrcp.c
>> @@ -64,6 +64,12 @@
>>  #define E_PARAM_NOT_FOUND      0x02
>>  #define E_INTERNAL             0x03
>>
>> +/* Packet types */
>> +#define AVRCP_PACKET_TYPE_SINGLE       0x00
>> +#define AVRCP_PACKET_TYPE_START                0x01
>> +#define AVRCP_PACKET_TYPE_CONTINUING   0x02
>> +#define AVRCP_PACKET_TYPE_END          0x03
>> +
>>  /* PDU types for metadata transfer */
>>  #define AVRCP_GET_CAPABILITIES         0x10
>>  #define AVRCP_LIST_PLAYER_ATTRIBUTES   0X11
>> @@ -77,6 +83,7 @@
>>  #define AVRCP_GET_ELEMENT_ATTRIBUTES   0x20
>>  #define AVRCP_GET_PLAY_STATUS          0x30
>>  #define AVRCP_REGISTER_NOTIFICATION    0x31
>> +#define AVRCP_REQUEST_CONTINUING       0x40
>>  #define AVRCP_ABORT_CONTINUING         0x41
>>
>>  /* Capabilities for AVRCP_GET_CAPABILITIES pdu */
>> @@ -399,75 +406,119 @@ int avrcp_player_event(struct avrcp_player *player, uint8_t id, void *data)
>>  }
>>
>>  /*
>> - * Copy media_info field to a buffer, intended to be used in a response to
>> - * GetElementAttributes message.
>> - *
>> - * It assumes there's enough space in the buffer and on success it returns the
>> - * size written.
>> + * Write media attribute to buffer, taking care of writing the header when
>> + * necessary, not writing more than @param maxlen and starting string from
>> + * @param last_idx.
>>  *
>> - * If @param id is not valid, -EINVAL is returned. If there's no space left on
>> - * the buffer -ENOBUFS is returned.
>> + * If there isn't enough space in the buffer even for the header, it returns
>> + * -ENOBUFS. Otherwise it returns the size written to buffer and the position
>> + *  within the media attribute in @param last_idx.
>>  */
>> -static int player_get_media_attribute(struct avrcp_player *player,
>> +static int player_write_media_attribute(struct avrcp_player *player,
>>                                                uint32_t id, uint8_t *buf,
>> -                                               uint16_t maxlen)
>> +                                               uint16_t maxlen,
>> +                                               unsigned int *last_idx)
>>  {
>> -       struct media_info_elem {
>> +       struct media_attribute_header {
>>                uint32_t id;
>>                uint16_t charset;
>>                uint16_t len;
>> -               uint8_t val[];
>>        };
>> -       struct media_info_elem *elem = (void *)buf;
>> +       uint8_t *val;
>> +       struct media_attribute_header *hdr;
>>        uint16_t len;
>>        char valstr[20];
>>        void *value;
>>
>> -       if (maxlen < sizeof(struct media_info_elem))
>> -               return -ENOBUFS;
>> +       if (*last_idx == 0) {
>> +               /*
>> +                * Write header first, but bail out if even that
>> +                * doesn't fit in PDU.
>> +                */
>> +               if (maxlen < sizeof(*hdr))
>> +                       return -ENOBUFS;
>>
>> -       /* Subtract the size of elem header from the available space */
>> -       maxlen -= sizeof(struct media_info_elem);
>> +               maxlen -= sizeof(*hdr);
>> +               hdr = (void *) buf;
>> +               hdr->id = htonl(id);
>> +               hdr->charset = htons(0x6A); /* Always use UTF-8 */
>> +               val = ((uint8_t *) hdr) + sizeof(*hdr);
>> +       } else {
>> +               hdr = NULL;
>> +               val = buf;
>> +       }
>>
>>        DBG("Get media attribute: %u", id);
>>
>> -       if (id == AVRCP_MEDIA_ATTRIBUTE_ILLEGAL ||
>> -                       id > AVRCP_MEDIA_ATTRIBUTE_LAST)
>> -               return -ENOENT;
>> -
>>        value = player->cb->get_metadata(id, player->user_data);
>>        if (value == NULL) {
>> -               len = 0;
>> -               goto done;
>> +               hdr->len = 0;
>> +               return sizeof(*hdr);
>>        }
>>
>>        switch (id) {
>> -       case AVRCP_MEDIA_ATTRIBUTE_TITLE:
>> -       case AVRCP_MEDIA_ATTRIBUTE_ARTIST:
>> -       case AVRCP_MEDIA_ATTRIBUTE_ALBUM:
>> -       case AVRCP_MEDIA_ATTRIBUTE_GENRE:
>> -               len = strlen((char *) value);
>> -               if (len > maxlen)
>> -                       return -ENOBUFS;
>> -               memcpy(elem->val, value, len);
>> -               break;
>>        case AVRCP_MEDIA_ATTRIBUTE_TRACK:
>>        case AVRCP_MEDIA_ATTRIBUTE_N_TRACKS:
>>        case AVRCP_MEDIA_ATTRIBUTE_DURATION:
>>                snprintf(valstr, 20, "%u", GPOINTER_TO_UINT(value));
>> -               len = strlen(valstr);
>> -               if (len > maxlen)
>> -                       return -ENOBUFS;
>> -               memcpy(elem->val, valstr, len);
>> +               value = valstr;
>>                break;
>>        }
>>
>> -done:
>> -       elem->id = htonl(id);
>> -       elem->charset = htons(0x6A); /* Always use UTF-8 */
>> -       elem->len = htons(len);
>> +       len = strlen(value);
>> +       if (hdr != NULL)
>> +               hdr->len = htons(len);
>> +
>> +       value = ((char *) value) + *last_idx;
>> +       len -= *last_idx;
>> +
>> +       if (len > maxlen) {
>> +               *last_idx += maxlen;
>> +               len = maxlen;
>> +       } else {
>> +               *last_idx = 0;
>> +       }
>> +
>> +       memcpy(val, value, len);
>> +
>> +       return val - buf + len;
>
> So the reason you have val - buf is to track if header was written or
> not, right?

Correct.

> In that case I would suggest using something like hdr ?
> sizeof(*hdr) + len : len or perhaps moving the header filling to
> player_fill_media_attribute might simplify this, so you only have to
> keep track of the written value in player_write_media_attribute.

Yeah, I think moving the header filling to player_fill_media_attribute
is a better idea.


>> +}
>>
>> -       return sizeof(struct media_info_elem) + len;
>> +static GList *player_fill_media_attribute(struct avrcp_player *player,
>> +                                       GList *attr_ids, uint8_t *buf,
>> +                                       uint16_t *pos, unsigned int *last_idx)
>> +{
>> +       GList *l;
>> +
>> +       for (l = attr_ids; l != NULL; l = g_list_delete_link(l, l)) {
>> +               uint32_t attr = GPOINTER_TO_UINT(l->data);
>> +               int size;
>> +
>> +               size = player_write_media_attribute(player, attr, &buf[*pos],
>> +                                                       AVRCP_PDU_MTU - *pos,
>> +                                                       last_idx);
>> +               if (size < 0)
>> +                       break;
>> +
>> +               *pos += size;
>> +
>> +               if (*last_idx)
>> +                       break;
>> +       }
>> +
>> +       return l;
>> +}
>> +
>> -               for (i = 0, attr_ids = NULL; i < nattr; i++, attr++) {
>> +               for (i = 0, len = 0, attr_ids = NULL; i < nattr; i++, attr++) {
>>                        uint32_t id = ntohl(bt_get_unaligned(attr));
>> +
>> +                       /* Don't add invalid attributes */
>> +                       if (id == AVRCP_MEDIA_ATTRIBUTE_ILLEGAL ||
>> +                                       id > AVRCP_MEDIA_ATTRIBUTE_LAST)
>> +                               continue;
>> +
>> +                       len++;
>>                        attr_ids = g_list_prepend(attr_ids,
>>                                                        GUINT_TO_POINTER(id));
>>                }
>
> This sounds like a different bug, I mean not checking for invalid
> attributes, if it was introduced in the previous patch can you please
> fix it in place?

It's not a bug. In order to to save the list of pending attributes, I
have to make sure only valid attributes are in the list because I have
to answer in the first PDU the number of attributes I'll be
responding. Without pdu-continuing, this check could be where it was
before.



>
>> @@ -641,24 +700,21 @@ static uint8_t avrcp_handle_get_element_attributes(struct avrcp_player *player,
>>                attr_ids = g_list_reverse(attr_ids);
>>        }
>>
>> -       for (l = attr_ids, len = 0, pos = 1; l != NULL; l = l->next) {
>> -               uint32_t attr = GPOINTER_TO_UINT(l->data);
>> +       if (!len)
>> +               goto err;
>>
>> -               size = player_get_media_attribute(player, attr,
>> -                                                       &pdu->params[pos],
>> -                                                       AVRCP_PDU_MTU - pos);
>> +       player_abort_pending_pdu(player);
>> +       pos = 1;
>> +       last_idx = 0;
>> +       attr_ids = player_fill_media_attribute(player, attr_ids, pdu->params,
>> +                                                       &pos, &last_idx);
>>
>> -               if (size >= 0) {
>> -                       len++;
>> -                       pos += size;
>> -               }
>> +       if (attr_ids != NULL) {
>> +               player->pending_pdu = pending_pdu_new(pdu->pdu_id, attr_ids,
>> +                                                               last_idx);
>> +               pdu->packet_type = AVRCP_PACKET_TYPE_START;
>>        }
>>
>> -       g_list_free(attr_ids);
>> -
>> -       if (!len)
>> -               goto err;
>> -
>>        pdu->params[0] = len;
>>        pdu->params_len = htons(pos);
>>
>> @@ -889,6 +945,46 @@ err:
>>        return AVC_CTYPE_REJECTED;
>>  }
>>
>> +static uint8_t avrcp_handle_request_continuing(struct avrcp_player *player,
>> +                                               struct avrcp_header *pdu,
>> +                                               uint8_t transaction)
>> +{
>> +       uint16_t len = ntohs(pdu->params_len);
>> +       struct pending_pdu *pending;
>> +
>> +       if (len != 1 || player->pending_pdu == NULL)
>> +               goto err;
>> +
>> +       pending = player->pending_pdu;
>> +
>> +       if (pending->pdu_id != pdu->params[0])
>> +               goto err;
>> +
>> +
>> +       len = 0;
>> +       pending->attr_ids = player_fill_media_attribute(player,
>> +                                                       pending->attr_ids,
>> +                                                       pdu->params, &len,
>> +                                                       &pending->last_idx);
>> +       pdu->pdu_id = pending->pdu_id;
>> +
>> +       if (pending->attr_ids == NULL) {
>> +               g_free(player->pending_pdu);
>> +               player->pending_pdu = NULL;
>> +               pdu->packet_type = AVRCP_PACKET_TYPE_END;
>> +       } else {
>> +               pdu->packet_type = AVRCP_PACKET_TYPE_CONTINUING;
>> +       }
>> +
>> +       pdu->params_len = htons(len);
>> +
>> +       return AVC_CTYPE_STABLE;
>> +err:
>> +       pdu->params_len = htons(1);
>> +       pdu->params[0] = E_INVALID_PARAM;
>> +       return AVC_CTYPE_REJECTED;
>> +}
>> +
>>  static uint8_t avrcp_handle_abort_continuing(struct avrcp_player *player,
>>                                                struct avrcp_header *pdu,
>>                                                uint8_t transaction)
>> @@ -947,8 +1043,11 @@ static struct pdu_handler {
>>                                        avrcp_handle_get_play_status },
>>                { AVRCP_REGISTER_NOTIFICATION, AVC_CTYPE_NOTIFY,
>>                                        avrcp_handle_register_notification },
>> +               { AVRCP_REQUEST_CONTINUING, AVC_CTYPE_CONTROL,
>> +                                       avrcp_handle_request_continuing },
>>                { AVRCP_ABORT_CONTINUING, AVC_CTYPE_CONTROL,
>>                                        avrcp_handle_abort_continuing },
>> +
>
> Empty line above is not really needed.

ok


Lucas De Marchi

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2011-10-17 12:27 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-10-14 21:28 [PATCH 0/5] Request/Abort Continuing and some minor things Lucas De Marchi
2011-10-14 21:28 ` [PATCH 1/5] AVRCP: Use track's UID in event notification Lucas De Marchi
2011-10-17  7:35   ` Luiz Augusto von Dentz
2011-10-14 21:28 ` [PATCH 2/5] AVRCP: implement TRACK-REACHED-START event Lucas De Marchi
2011-10-17  7:40   ` Luiz Augusto von Dentz
2011-10-14 21:28 ` [PATCH 3/5] AVRCP: respond with UINT32_MAX if duration is not available Lucas De Marchi
2011-10-17  7:57   ` Luiz Augusto von Dentz
2011-10-17 12:13     ` Lucas De Marchi
2011-10-14 21:28 ` [PATCH 4/5] AVRCP: Implement AbortContinuingResponse PDU Lucas De Marchi
2011-10-17  8:05   ` Luiz Augusto von Dentz
2011-10-14 21:28 ` [PATCH 5/5] AVRCP: Implement RequestContinuingResponse PDU Lucas De Marchi
2011-10-17  9:41   ` Luiz Augusto von Dentz
2011-10-17 12:27     ` Lucas De Marchi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.