[PATCH 0/5] Fixes and refactor avrcp

[PATCH 0/5] Fixes and refactor avrcp

[Lucas De Marchi]

Here are some fixes for avrcp. 0001 fixes a bug due to the recent code move,
0002 fixes the response of one command and the other 3 were already sent
previously.


David Stockwell (3):
  avrcp: use LAST element on media_info_id enum
  avrcp: fix handling of metadata item 0x7
  avrcp: get/set three-byte company-id

Lucas De Marchi (2):
  avrcp: fix missing error code
  avrcp: fix overwrite of number of attributes

 audio/avrcp.c |   72 +++++++++++++++++++++++++++++++++-----------------------
 1 files changed, 42 insertions(+), 30 deletions(-)

-- 
1.7.6.1

[PATCH 1/5] avrcp: fix missing error code

[Lucas De Marchi]

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

diff --git a/audio/avrcp.c b/audio/avrcp.c
index c9eae6e..9b1d797 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -1228,8 +1228,10 @@ static size_t handle_vendordep_pdu(struct avctp *session, uint8_t transaction,
 	pdu->packet_type = 0;
 	pdu->rsvd = 0;
 
-	if (operand_count + 3 < AVRCP_HEADER_LENGTH)
+	if (operand_count + 3 < AVRCP_HEADER_LENGTH) {
+		pdu->params[0] = E_INVALID_COMMAND;
 		goto err_metadata;
+	}
 
 	for (handler = handlers; handler; handler++) {
 		if (handler->pdu_id == pdu->pdu_id)
-- 
1.7.6.1

[PATCH 2/5] avrcp: fix overwrite of number of attributes

[Lucas De Marchi]

The response of GetCurrentPlayerApplicationSettingValue expects the
first operand to be the number of attributes in response. Since we start
with len=0, we were overwriting this number with the value of the first
attribute.

Also use g_memdup instead of g_malloc + memcpy.
---
 audio/avrcp.c |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index 9b1d797..783ba02 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -947,8 +947,7 @@ static uint8_t avrcp_handle_get_current_player_value(struct media_player *mp,
 	 * Save a copy of requested settings because we can override them
 	 * while responding
 	 */
-	settings = g_malloc(pdu->params[0]);
-	memcpy(settings, &pdu->params[1], pdu->params[0]);
+	settings = g_memdup(&pdu->params[1], pdu->params[0]);
 	len = 0;
 
 	/*
@@ -972,16 +971,15 @@ static uint8_t avrcp_handle_get_current_player_value(struct media_player *mp,
 			continue;
 		}
 
-		pdu->params[len] = settings[i];
-		pdu->params[len + 1] = val;
-		len += 2;
+		pdu->params[++len] = settings[i];
+		pdu->params[++len] = val;
 	}
 
 	g_free(settings);
 
 	if (len) {
-		pdu->params[0] = len;
-		pdu->params_len = htons(2 * len + 1);
+		pdu->params[0] = len / 2;
+		pdu->params_len = htons(len + 1);
 
 		return AVC_CTYPE_STABLE;
 	}
-- 
1.7.6.1

[PATCH 3/5] avrcp: use LAST element on media_info_id enum

[Lucas De Marchi]

From: David Stockwell <dstockwell@frequency-one.com>

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

diff --git a/audio/avrcp.c b/audio/avrcp.c
index 783ba02..f7f3924 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -142,6 +142,7 @@ enum media_info_id {
 	MEDIA_INFO_N_TRACKS =		5,
 	MEDIA_INFO_GENRE =		6,
 	MEDIA_INFO_CURRENT_POSITION =	7,
+	MEDIA_INFO_LAST
 };
 
 #if __BYTE_ORDER == __LITTLE_ENDIAN
@@ -890,7 +891,7 @@ static uint8_t avrcp_handle_get_element_attributes(struct media_player *mp,
 		 * Return all available information, at least
 		 * title must be returned.
 		 */
-		for (i = 1; i <= MEDIA_INFO_CURRENT_POSITION; i++) {
+		for (i = 1; i < MEDIA_INFO_LAST; i++) {
 			size = mp_get_media_attribute(mp, i,
 							&pdu->params[pos]);
 
-- 
1.7.6.1

[PATCH 4/5] avrcp: fix handling of metadata item 0x7

[Lucas De Marchi]

From: David Stockwell <dstockwell@frequency-one.com>

Metadata field number 0x7 should be the total playing time of the track
(TrackDuration) in msec, not current position within track.
---
 audio/avrcp.c |   18 ++++++------------
 1 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index f7f3924..835249b 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -141,7 +141,7 @@ enum media_info_id {
 	MEDIA_INFO_TRACK =		4,
 	MEDIA_INFO_N_TRACKS =		5,
 	MEDIA_INFO_GENRE =		6,
-	MEDIA_INFO_CURRENT_POSITION =	7,
+	MEDIA_INFO_PLAYING_TIME =	7,
 	MEDIA_INFO_LAST
 };
 
@@ -690,19 +690,13 @@ static int mp_get_media_attribute(struct media_player *mp,
 		len = strlen(valstr);
 		memcpy(elem->val, valstr, len);
 		break;
-	case MEDIA_INFO_CURRENT_POSITION:
-		if (mi->elapsed != 0xFFFFFFFF) {
-			uint32_t elapsed;
-
-			mp_get_playback_status(mp, NULL, &elapsed, NULL);
-
-			snprintf(valstr, 20, "%u", elapsed);
-			len = strlen(valstr);
-			memcpy(elem->val, valstr, len);
-		} else {
+	case MEDIA_INFO_PLAYING_TIME:
+		if (mi->track_len == 0xFFFFFFFF)
 			return -ENOENT;
-		}
 
+		snprintf(valstr, 20, "%u", mi->track_len);
+		len = strlen(valstr);
+		memcpy(elem->val, valstr, len);
 		break;
 	default:
 		return -EINVAL;
-- 
1.7.6.1

[PATCH 5/5] avrcp: get/set three-byte company-id

[Lucas De Marchi]

From: David Stockwell <dstockwell@frequency-one.com>

---
 audio/avrcp.c |   35 ++++++++++++++++++++++++++---------
 1 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index 835249b..f962946 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -517,6 +517,28 @@ static const char *battery_status_to_str(enum battery_status status)
 	return NULL;
 }
 
+/*
+ * get_company_id:
+ *
+ * Get three-byte Company_ID from incoming AVRCP message
+ */
+static uint32_t get_company_id(const uint8_t cid[3])
+{
+	return cid[0] << 16 | cid[1] << 8 | cid[2];
+}
+
+/*
+ * set_company_id:
+ *
+ * Set three-byte Company_ID into outgoing AVRCP message
+ */
+static void set_company_id(uint8_t cid[3], const uint32_t cid_in)
+{
+	cid[0] = cid_in >> 16;
+	cid[1] = cid_in >> 8;
+	cid[2] = cid_in;
+}
+
 static int avrcp_send_event(struct media_player *mp, uint8_t id, void *data)
 {
 	uint8_t buf[AVRCP_HEADER_LENGTH + 9];
@@ -532,9 +554,7 @@ static int avrcp_send_event(struct media_player *mp, uint8_t id, void *data)
 
 	memset(buf, 0, sizeof(buf));
 
-	pdu->company_id[0] = IEEEID_BTSIG >> 16;
-	pdu->company_id[1] = (IEEEID_BTSIG >> 8) & 0xFF;
-	pdu->company_id[2] = IEEEID_BTSIG & 0xFF;
+	set_company_id(pdu->company_id, IEEEID_BTSIG);
 
 	pdu->pdu_id = AVRCP_REGISTER_NOTIFICATION;
 	pdu->params[0] = id;
@@ -774,9 +794,8 @@ static uint8_t avrcp_handle_get_capabilities(struct media_player *mp,
 	switch (pdu->params[0]) {
 	case CAP_COMPANY_ID:
 		for (i = 0; i < G_N_ELEMENTS(company_ids); i++) {
-			pdu->params[2 + i * 3] = company_ids[i] >> 16;
-			pdu->params[3 + i * 3] = (company_ids[i] >> 8) & 0xFF;
-			pdu->params[4 + i * 3] = company_ids[i] & 0xFF;
+			set_company_id(&pdu->params[2 + i * 3],
+							company_ids[i]);
 		}
 
 		pdu->params_len = htons(2 + (3 * G_N_ELEMENTS(company_ids)));
@@ -1206,9 +1225,7 @@ static size_t handle_vendordep_pdu(struct avctp *session, uint8_t transaction,
 	struct media_player *mp = user_data;
 	struct pdu_handler *handler;
 	struct avrcp_header *pdu = (void *) operands;
-	uint32_t company_id = (pdu->company_id[0] << 16) |
-				(pdu->company_id[1] << 8) |
-				(pdu->company_id[2]);
+	uint32_t company_id = get_company_id(pdu->company_id);
 
 	if (company_id != IEEEID_BTSIG) {
 		*code = AVC_CTYPE_NOT_IMPLEMENTED;
-- 
1.7.6.1

Re: [PATCH 1/5] avrcp: fix missing error code

[Luiz Augusto von Dentz]

Hi Lucas,

On Thu, Sep 15, 2011 at 7:21 AM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> ---
>  audio/avrcp.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index c9eae6e..9b1d797 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -1228,8 +1228,10 @@ static size_t handle_vendordep_pdu(struct avctp *session, uint8_t transaction,
>        pdu->packet_type = 0;
>        pdu->rsvd = 0;
>
> -       if (operand_count + 3 < AVRCP_HEADER_LENGTH)
> +       if (operand_count + 3 < AVRCP_HEADER_LENGTH) {
> +               pdu->params[0] = E_INVALID_COMMAND;
>                goto err_metadata;
> +       }
>
>        for (handler = handlers; handler; handler++) {
>                if (handler->pdu_id == pdu->pdu_id)
> --
> 1.7.6.1
>
> --

Ack.

-- 
Luiz Augusto von Dentz

Re: [PATCH 2/5] avrcp: fix overwrite of number of attributes

[Luiz Augusto von Dentz]

Hi Lucas,

On Thu, Sep 15, 2011 at 7:21 AM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> The response of GetCurrentPlayerApplicationSettingValue expects the
> first operand to be the number of attributes in response. Since we start
> with len=0, we were overwriting this number with the value of the first
> attribute.
>
> Also use g_memdup instead of g_malloc + memcpy.
> ---
>  audio/avrcp.c |   12 +++++-------
>  1 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index 9b1d797..783ba02 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -947,8 +947,7 @@ static uint8_t avrcp_handle_get_current_player_value(struct media_player *mp,
>         * Save a copy of requested settings because we can override them
>         * while responding
>         */
> -       settings = g_malloc(pdu->params[0]);
> -       memcpy(settings, &pdu->params[1], pdu->params[0]);
> +       settings = g_memdup(&pdu->params[1], pdu->params[0]);
>        len = 0;
>
>        /*
> @@ -972,16 +971,15 @@ static uint8_t avrcp_handle_get_current_player_value(struct media_player *mp,
>                        continue;
>                }
>
> -               pdu->params[len] = settings[i];
> -               pdu->params[len + 1] = val;
> -               len += 2;
> +               pdu->params[++len] = settings[i];
> +               pdu->params[++len] = val;
>        }
>
>        g_free(settings);
>
>        if (len) {
> -               pdu->params[0] = len;
> -               pdu->params_len = htons(2 * len + 1);
> +               pdu->params[0] = len / 2;
> +               pdu->params_len = htons(len + 1);
>
>                return AVC_CTYPE_STABLE;
>        }
> --
> 1.7.6.1
>
> --

Ack.

-- 
Luiz Augusto von Dentz

Re: [PATCH 3/5] avrcp: use LAST element on media_info_id enum

[Luiz Augusto von Dentz]

Hi Lucas, David,

On Thu, Sep 15, 2011 at 7:21 AM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> From: David Stockwell <dstockwell@frequency-one.com>
>
> ---
>  audio/avrcp.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index 783ba02..f7f3924 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -142,6 +142,7 @@ enum media_info_id {
>        MEDIA_INFO_N_TRACKS =           5,
>        MEDIA_INFO_GENRE =              6,
>        MEDIA_INFO_CURRENT_POSITION =   7,
> +       MEDIA_INFO_LAST
>  };
>
>  #if __BYTE_ORDER == __LITTLE_ENDIAN
> @@ -890,7 +891,7 @@ static uint8_t avrcp_handle_get_element_attributes(struct media_player *mp,
>                 * Return all available information, at least
>                 * title must be returned.
>                 */
> -               for (i = 1; i <= MEDIA_INFO_CURRENT_POSITION; i++) {
> +               for (i = 1; i < MEDIA_INFO_LAST; i++) {
>                        size = mp_get_media_attribute(mp, i,
>                                                        &pdu->params[pos]);
>
> --
> 1.7.6.1

Ack.


-- 
Luiz Augusto von Dentz

Re: [PATCH 4/5] avrcp: fix handling of metadata item 0x7

[Luiz Augusto von Dentz]

Hi Lucas and David,

On Thu, Sep 15, 2011 at 7:21 AM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> From: David Stockwell <dstockwell@frequency-one.com>
>
> Metadata field number 0x7 should be the total playing time of the track
> (TrackDuration) in msec, not current position within track.
> ---
>  audio/avrcp.c |   18 ++++++------------
>  1 files changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index f7f3924..835249b 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -141,7 +141,7 @@ enum media_info_id {
>        MEDIA_INFO_TRACK =              4,
>        MEDIA_INFO_N_TRACKS =           5,
>        MEDIA_INFO_GENRE =              6,
> -       MEDIA_INFO_CURRENT_POSITION =   7,
> +       MEDIA_INFO_PLAYING_TIME =       7,
>        MEDIA_INFO_LAST
>  };
>
> @@ -690,19 +690,13 @@ static int mp_get_media_attribute(struct media_player *mp,
>                len = strlen(valstr);
>                memcpy(elem->val, valstr, len);
>                break;
> -       case MEDIA_INFO_CURRENT_POSITION:
> -               if (mi->elapsed != 0xFFFFFFFF) {
> -                       uint32_t elapsed;
> -
> -                       mp_get_playback_status(mp, NULL, &elapsed, NULL);
> -
> -                       snprintf(valstr, 20, "%u", elapsed);
> -                       len = strlen(valstr);
> -                       memcpy(elem->val, valstr, len);
> -               } else {
> +       case MEDIA_INFO_PLAYING_TIME:
> +               if (mi->track_len == 0xFFFFFFFF)
>                        return -ENOENT;
> -               }
>
> +               snprintf(valstr, 20, "%u", mi->track_len);
> +               len = strlen(valstr);
> +               memcpy(elem->val, valstr, len);
>                break;
>        default:
>                return -EINVAL;
> --
> 1.7.6.1

Ack.

-- 
Luiz Augusto von Dentz

Re: [PATCH 5/5] avrcp: get/set three-byte company-id

[Luiz Augusto von Dentz]

Hi Lucas and David,

On Thu, Sep 15, 2011 at 7:21 AM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> From: David Stockwell <dstockwell@frequency-one.com>
>
> ---
>  audio/avrcp.c |   35 ++++++++++++++++++++++++++---------
>  1 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index 835249b..f962946 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -517,6 +517,28 @@ static const char *battery_status_to_str(enum battery_status status)
>        return NULL;
>  }
>
> +/*
> + * get_company_id:
> + *
> + * Get three-byte Company_ID from incoming AVRCP message
> + */
> +static uint32_t get_company_id(const uint8_t cid[3])
> +{
> +       return cid[0] << 16 | cid[1] << 8 | cid[2];
> +}
> +
> +/*
> + * set_company_id:
> + *
> + * Set three-byte Company_ID into outgoing AVRCP message
> + */
> +static void set_company_id(uint8_t cid[3], const uint32_t cid_in)
> +{
> +       cid[0] = cid_in >> 16;
> +       cid[1] = cid_in >> 8;
> +       cid[2] = cid_in;
> +}
> +
>  static int avrcp_send_event(struct media_player *mp, uint8_t id, void *data)
>  {
>        uint8_t buf[AVRCP_HEADER_LENGTH + 9];
> @@ -532,9 +554,7 @@ static int avrcp_send_event(struct media_player *mp, uint8_t id, void *data)
>
>        memset(buf, 0, sizeof(buf));
>
> -       pdu->company_id[0] = IEEEID_BTSIG >> 16;
> -       pdu->company_id[1] = (IEEEID_BTSIG >> 8) & 0xFF;
> -       pdu->company_id[2] = IEEEID_BTSIG & 0xFF;
> +       set_company_id(pdu->company_id, IEEEID_BTSIG);
>
>        pdu->pdu_id = AVRCP_REGISTER_NOTIFICATION;
>        pdu->params[0] = id;
> @@ -774,9 +794,8 @@ static uint8_t avrcp_handle_get_capabilities(struct media_player *mp,
>        switch (pdu->params[0]) {
>        case CAP_COMPANY_ID:
>                for (i = 0; i < G_N_ELEMENTS(company_ids); i++) {
> -                       pdu->params[2 + i * 3] = company_ids[i] >> 16;
> -                       pdu->params[3 + i * 3] = (company_ids[i] >> 8) & 0xFF;
> -                       pdu->params[4 + i * 3] = company_ids[i] & 0xFF;
> +                       set_company_id(&pdu->params[2 + i * 3],
> +                                                       company_ids[i]);
>                }
>
>                pdu->params_len = htons(2 + (3 * G_N_ELEMENTS(company_ids)));
> @@ -1206,9 +1225,7 @@ static size_t handle_vendordep_pdu(struct avctp *session, uint8_t transaction,
>        struct media_player *mp = user_data;
>        struct pdu_handler *handler;
>        struct avrcp_header *pdu = (void *) operands;
> -       uint32_t company_id = (pdu->company_id[0] << 16) |
> -                               (pdu->company_id[1] << 8) |
> -                               (pdu->company_id[2]);
> +       uint32_t company_id = get_company_id(pdu->company_id);
>
>        if (company_id != IEEEID_BTSIG) {
>                *code = AVC_CTYPE_NOT_IMPLEMENTED;
> --
> 1.7.6.1

Ack.

-- 
Luiz Augusto von Dentz

Re: [PATCH 0/5] Fixes and refactor avrcp

[Johan Hedberg]

Hi,

On Thu, Sep 15, 2011, Lucas De Marchi wrote:
> Here are some fixes for avrcp. 0001 fixes a bug due to the recent code move,
> 0002 fixes the response of one command and the other 3 were already sent
> previously.
> 
> 
> David Stockwell (3):
>   avrcp: use LAST element on media_info_id enum
>   avrcp: fix handling of metadata item 0x7
>   avrcp: get/set three-byte company-id
> 
> Lucas De Marchi (2):
>   avrcp: fix missing error code
>   avrcp: fix overwrite of number of attributes
> 
>  audio/avrcp.c |   72 +++++++++++++++++++++++++++++++++-----------------------
>  1 files changed, 42 insertions(+), 30 deletions(-)

All five patches applied. Thanks.

Johan