2011-09-15 04:21:32

by Lucas De Marchi

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

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

2011-09-27 08:34:22

by Johan Hedberg

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

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

2011-09-16 07:51:37

by Luiz Augusto von Dentz

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

Hi Lucas and David,

On Thu, Sep 15, 2011 at 7:21 AM, Lucas De Marchi
<[email protected]> wrote:
> From: David Stockwell <[email protected]>
>
> ---
> ?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

2011-09-16 07:50:58

by Luiz Augusto von Dentz

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

Hi Lucas and David,

On Thu, Sep 15, 2011 at 7:21 AM, Lucas De Marchi
<[email protected]> wrote:
> From: David Stockwell <[email protected]>
>
> 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

2011-09-16 07:49:40

by Luiz Augusto von Dentz

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

Hi Lucas, David,

On Thu, Sep 15, 2011 at 7:21 AM, Lucas De Marchi
<[email protected]> wrote:
> From: David Stockwell <[email protected]>
>
> ---
> ?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

2011-09-16 07:47:47

by Luiz Augusto von Dentz

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

Hi Lucas,

On Thu, Sep 15, 2011 at 7:21 AM, Lucas De Marchi
<[email protected]> 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

2011-09-16 07:39:32

by Luiz Augusto von Dentz

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

Hi Lucas,

On Thu, Sep 15, 2011 at 7:21 AM, Lucas De Marchi
<[email protected]> 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

2011-09-15 04:21:37

by Lucas De Marchi

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

From: David Stockwell <[email protected]>

---
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

2011-09-15 04:21:36

by Lucas De Marchi

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

From: David Stockwell <[email protected]>

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

2011-09-15 04:21:35

by Lucas De Marchi

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

From: David Stockwell <[email protected]>

---
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

2011-09-15 04:21:34

by Lucas De Marchi

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

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

2011-09-15 04:21:33

by Lucas De Marchi

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

---
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