[PATCH 1/3] avrcp: Fix response ctype of AbortContinuingResponse

[PATCH 1/3] avrcp: Fix response ctype of AbortContinuingResponse

[Michal Labedzki]

Request ctype of "AbortContinuingResponse" is CONTROL, so response should be
ACCEPTED instead of STABLE. This affect PTS Test Case for TP/RCR/BV-04-C.
---
 audio/avrcp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index c9ec314..4573133 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -986,7 +986,7 @@ static uint8_t avrcp_handle_abort_continuing(struct avrcp_player *player,
 	player_abort_pending_pdu(player);
 	pdu->params_len = 0;
 
-	return AVC_CTYPE_STABLE;
+	return AVC_CTYPE_ACCEPTED;
 
 err:
 	pdu->params_len = htons(1);
-- 
on behalf of ST-Ericsson

[PATCH 2/3] avrcp: Add error code for rejected vendor command

[Michal Labedzki]

Make correct response for any vendor commands when there are not any
registered player. According to the specification responses should be
REJECTED with error code.
---
 audio/avctp.c |    3 +++
 audio/avctp.h |    5 +++++
 audio/avrcp.c |   19 +++++++++++++++++++
 3 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/audio/avctp.c b/audio/avctp.c
index 5bd5db1..38a2d20 100644
--- a/audio/avctp.c
+++ b/audio/avctp.c
@@ -465,6 +465,9 @@ static gboolean session_cb(GIOChannel *chan, GIOCondition cond,
 	if (!handler) {
 		DBG("handler not found for 0x%02x", avc->opcode);
 		avc->code = AVC_CTYPE_REJECTED;
+
+		packet_size += handle_vendor_reject(session, avctp->transaction, &code,
+					&subunit, operands, operand_count, NULL);
 		goto done;
 	}
 
diff --git a/audio/avctp.h b/audio/avctp.h
index 9727485..a1109a9 100644
--- a/audio/avctp.h
+++ b/audio/avctp.h
@@ -97,3 +97,8 @@ int avctp_send_passthrough(struct avctp *session, uint8_t op);
 int avctp_send_vendordep(struct avctp *session, uint8_t transaction,
 				uint8_t code, uint8_t subunit,
 				uint8_t *operands, size_t operand_count);
+
+size_t handle_vendor_reject(struct avctp *session, uint8_t transaction,
+					uint8_t *code, uint8_t *subunit,
+					uint8_t *operands, size_t operand_count,
+					void *user_data);
\ No newline at end of file
diff --git a/audio/avrcp.c b/audio/avrcp.c
index 4573133..2d29d56 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -862,6 +862,7 @@ static uint8_t avrcp_handle_get_play_status(struct avrcp_player *player,
 	position = player->cb->get_position(player->user_data);
 	pduration = player->cb->get_metadata(AVRCP_MEDIA_ATTRIBUTE_DURATION,
 							player->user_data);
+
 	if (pduration != NULL)
 		duration = htonl(GPOINTER_TO_UINT(pduration));
 	else
@@ -1092,6 +1093,24 @@ err_metadata:
 	return AVRCP_HEADER_LENGTH + 1;
 }
 
+size_t handle_vendor_reject(struct avctp *session, uint8_t transaction,
+					uint8_t *code, uint8_t *subunit,
+					uint8_t *operands, size_t operand_count,
+					void *user_data)
+{
+    struct avrcp_header *pdu = (void *) operands;
+    uint32_t company_id = get_company_id(pdu->company_id);
+
+    *code = AVC_CTYPE_REJECTED;
+    pdu->params_len = htons(1);
+    pdu->params[0] = E_INTERNAL;
+
+    DBG("rejecting AVRCP PDU 0x%02X, company 0x%06X len 0x%04X",
+            pdu->pdu_id, company_id, pdu->params_len);
+
+    return AVRCP_HEADER_LENGTH + 1;
+}
+
 static struct avrcp_server *find_server(GSList *list, const bdaddr_t *src)
 {
 	for (; list; list = list->next) {
-- 
on behalf of ST-Ericsson

Re: [PATCH 2/3] avrcp: Add error code for rejected vendor command

[Luiz Augusto von Dentz]

Hi Michal,

On Thu, Mar 1, 2012 at 4:45 PM, Michal Labedzki
<michal.labedzki@tieto.com> wrote:
> Make correct response for any vendor commands when there are not any
> registered player. According to the specification responses should be
> REJECTED with error code.

Please quote the spec whenever possible, actually what spec does this come from?


-- 
Luiz Augusto von Dentz

[PATCH 2/3] avrcp: Add error code for rejected vendor command

[Michal Labedzki]

On Thu, Mar 01, 2012 at 05:34:19PM +0200, Luiz Augusto von Dentz wrote:
> Hi Michal,
> 
> On Thu, Mar 1, 2012 at 4:45 PM, Michal Labedzki
> <michal.labedzki@tieto.com> wrote:
> > Make correct response for any vendor commands when there are not any
> > registered player. According to the specification responses should be
> > REJECTED with error code.
> 
> Please quote the spec whenever possible, actually what spec does this come from?
> 
> 
> -- 
> Luiz Augusto von Dentz

Hi Luiz,                                                                                                                                             
                                                                                                                                                     
Here you are:                                                                                                                                        
"For error response PDU the response parameter is always the error code independent                                                                  
of the response format defined for ACCEPTED PDU response for the corresponding                                                                       
PDU command."                                                                                                                                        
(source: AVRCP 1.3 specification, pages 34,56 or AVRCP 1.4 specification, pages 33,93)                                                               
                                                                                                                                                     
All other error responses already have error code in parameter list.

Re: [PATCH 2/3] avrcp: Add error code for rejected vendor command

[Lucas De Marchi]

On Fri, Mar 2, 2012 at 6:39 AM, Michal Labedzki
<michal.labedzki@tieto.com> wrote:
> On Thu, Mar 01, 2012 at 05:34:19PM +0200, Luiz Augusto von Dentz wrote:
>> Hi Michal,
>>
>> On Thu, Mar 1, 2012 at 4:45 PM, Michal Labedzki
>> <michal.labedzki@tieto.com> wrote:
>> > Make correct response for any vendor commands when there are not any
>> > registered player. According to the specification responses should be
>> > REJECTED with error code.
>>
>> Please quote the spec whenever possible, actually what spec does this come from?
>>
>>
>> --
>> Luiz Augusto von Dentz
>
> Hi Luiz,
>
> Here you are:
> "For error response PDU the response parameter is always the error code independent
> of the response format defined for ACCEPTED PDU response for the corresponding
> PDU command."
> (source: AVRCP 1.3 specification, pages 34,56 or AVRCP 1.4 specification, pages 33,93)

Ack.

Could you put this in the commit message?

Lucas De Marchi

[PATCH v3] avrcp: Add error code for rejected vendor command

[Michal Labedzki]

Fix response for any vendor commands when there are no players
registered. According to the specification responses should be
REJECTED with error code.

"For error response PDU the response parameter is always the
error code independent of the response format defined for
ACCEPTED PDU response for the corresponding PDU command."
(source: section 4.7.9 of AVRCP 1.3 specification)
---
 audio/avctp.c |    4 +++-
 audio/avrcp.c |   15 +++++++++++++++
 audio/avrcp.h |    2 ++
 3 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/audio/avctp.c b/audio/avctp.c
index 5bd5db1..dfad9fd 100644
--- a/audio/avctp.c
+++ b/audio/avctp.c
@@ -53,6 +53,7 @@
 #include "manager.h"
 #include "device.h"
 #include "avctp.h"
+#include "avrcp.h"
 
 #define QUIRK_NO_RELEASE 1 << 0
 
@@ -464,7 +465,8 @@ static gboolean session_cb(GIOChannel *chan, GIOCondition cond,
 	handler = find_handler(handlers, avc->opcode);
 	if (!handler) {
 		DBG("handler not found for 0x%02x", avc->opcode);
-		avc->code = AVC_CTYPE_REJECTED;
+		packet_size += avrcp_handle_vendor_reject(&code, operands);
+		avc->code = code;
 		goto done;
 	}
 
diff --git a/audio/avrcp.c b/audio/avrcp.c
index 4573133..8eba046 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -1092,6 +1092,21 @@ err_metadata:
 	return AVRCP_HEADER_LENGTH + 1;
 }
 
+size_t avrcp_handle_vendor_reject(uint8_t *code, uint8_t *operands)
+{
+    struct avrcp_header *pdu = (void *) operands;
+    uint32_t company_id = get_company_id(pdu->company_id);
+
+    *code = AVC_CTYPE_REJECTED;
+    pdu->params_len = htons(1);
+    pdu->params[0] = E_INTERNAL;
+
+    DBG("rejecting AVRCP PDU 0x%02X, company 0x%06X len 0x%04X",
+            pdu->pdu_id, company_id, pdu->params_len);
+
+    return AVRCP_HEADER_LENGTH + 1;
+}
+
 static struct avrcp_server *find_server(GSList *list, const bdaddr_t *src)
 {
 	for (; list; list = list->next) {
diff --git a/audio/avrcp.h b/audio/avrcp.h
index fb64f3b..8a09546 100644
--- a/audio/avrcp.h
+++ b/audio/avrcp.h
@@ -98,3 +98,5 @@ struct avrcp_player *avrcp_register_player(const bdaddr_t *src,
 void avrcp_unregister_player(struct avrcp_player *player);
 
 int avrcp_player_event(struct avrcp_player *player, uint8_t id, void *data);
+
+size_t avrcp_handle_vendor_reject(uint8_t *code, uint8_t *operands);
-- 
on behalf of ST-Ericsson

Re: [PATCH v3] avrcp: Add error code for rejected vendor command

[Johan Hedberg]

Hi Michal,

On Wed, Mar 07, 2012, Michal Labedzki wrote:
> Fix response for any vendor commands when there are no players
> registered. According to the specification responses should be
> REJECTED with error code.
> 
> "For error response PDU the response parameter is always the
> error code independent of the response format defined for
> ACCEPTED PDU response for the corresponding PDU command."
> (source: section 4.7.9 of AVRCP 1.3 specification)
> ---
>  audio/avctp.c |    4 +++-
>  audio/avrcp.c |   15 +++++++++++++++
>  audio/avrcp.h |    2 ++
>  3 files changed, 20 insertions(+), 1 deletions(-)

Patch 1/3 as well as this one have been applied. Thanks.

Johan

[PATCH 3/3] avrcp: Treat position as unknown when duration is unavailable

[Michal Labedzki]

According to the specification when duration and position are not
supported then 0xFFFFFFFF should be returned. This patch avoid random
position on stream where duration is not available.
---
 audio/avrcp.c |   10 ++++++----
 audio/media.c |    3 ++-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index 2d29d56..e141030 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -859,16 +859,18 @@ static uint8_t avrcp_handle_get_play_status(struct avrcp_player *player,
 		return AVC_CTYPE_REJECTED;
 	}
 
-	position = player->cb->get_position(player->user_data);
 	pduration = player->cb->get_metadata(AVRCP_MEDIA_ATTRIBUTE_DURATION,
 							player->user_data);
 
-	if (pduration != NULL)
+	if (pduration != NULL) {
 		duration = htonl(GPOINTER_TO_UINT(pduration));
-	else
+		position = htonl(player->cb->get_position(player->user_data));
+	} else {
 		duration = htonl(UINT32_MAX);
+		position = htonl(UINT32_MAX);
+	}
 
-	position = htonl(position);
+	DBG("position=%u duration=%u", ntohl(position), ntohl(duration));
 
 	memcpy(&pdu->params[0], &duration, 4);
 	memcpy(&pdu->params[4], &position, 4);
diff --git a/audio/media.c b/audio/media.c
index c0fd0c3..4887753 100644
--- a/audio/media.c
+++ b/audio/media.c
@@ -1569,7 +1569,8 @@ static struct media_player *media_player_create(struct media_adapter *adapter,
 	mp->sender = g_strdup(sender);
 	mp->path = g_strdup(path);
 	mp->timer = g_timer_new();
-
+	mp->position = UINT32_MAX;
+	mp->status = AVRCP_PLAY_STATUS_STOPPED;
 	mp->watch = g_dbus_add_disconnect_watch(adapter->conn, sender,
 						media_player_exit, mp,
 						NULL);
-- 
on behalf of ST-Ericsson

Re: [PATCH 3/3] avrcp: Treat position as unknown when duration is unavailable

[Luiz Augusto von Dentz]

Hi Michal,

On Thu, Mar 1, 2012 at 4:45 PM, Michal Labedzki
<michal.labedzki@tieto.com> wrote:
> According to the specification when duration and position are not
> supported then 0xFFFFFFFF should be returned. This patch avoid random
> position on stream where duration is not available.

Again you should quote the spec. Btw is really random or 0?

> ---
>  audio/avrcp.c |   10 ++++++----
>  audio/media.c |    3 ++-
>  2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index 2d29d56..e141030 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -859,16 +859,18 @@ static uint8_t avrcp_handle_get_play_status(struct avrcp_player *player,
>                return AVC_CTYPE_REJECTED;
>        }
>
> -       position = player->cb->get_position(player->user_data);
>        pduration = player->cb->get_metadata(AVRCP_MEDIA_ATTRIBUTE_DURATION,
>                                                        player->user_data);
>
> -       if (pduration != NULL)
> +       if (pduration != NULL) {
>                duration = htonl(GPOINTER_TO_UINT(pduration));
> -       else
> +               position = htonl(player->cb->get_position(player->user_data));
> +       } else {
>                duration = htonl(UINT32_MAX);
> +               position = htonl(UINT32_MAX);
> +       }

So the position cannot be known without the duration? What about live streams?

-- 
Luiz Augusto von Dentz

Re: [PATCH 3/3] avrcp: Treat position as unknown when duration is unavailable

[Lucas De Marchi]

Hi Michal

On Thu, Mar 1, 2012 at 11:45 AM, Michal Labedzki
<michal.labedzki@tieto.com> wrote:
> According to the specification when duration and position are not
> supported then 0xFFFFFFFF should be returned. This patch avoid random
> position on stream where duration is not available.
> ---
>  audio/avrcp.c |   10 ++++++----
>  audio/media.c |    3 ++-
>  2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index 2d29d56..e141030 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -859,16 +859,18 @@ static uint8_t avrcp_handle_get_play_status(struct avrcp_player *player,
>                return AVC_CTYPE_REJECTED;
>        }
>
> -       position = player->cb->get_position(player->user_data);
>        pduration = player->cb->get_metadata(AVRCP_MEDIA_ATTRIBUTE_DURATION,
>                                                        player->user_data);
>
> -       if (pduration != NULL)
> +       if (pduration != NULL) {
>                duration = htonl(GPOINTER_TO_UINT(pduration));
> -       else
> +               position = htonl(player->cb->get_position(player->user_data));

Duration and position are independent. Only duration accepts
UINT32_MAX when it's not available. Position is always available
because we handle it inside bluetoothd.


Regards,
Lucas De Marchi

Re: [PATCH 3/3] avrcp: Treat position as unknown when duration is unavailable

[Michal Labedzki]

Yes, you are right. I was thinging about radio cases, but for now I do not need this change.
Abandoned.

---

-- 
on behalf of ST-Ericsson

Re: [PATCH 1/3] avrcp: Fix response ctype of AbortContinuingResponse

[Luiz Augusto von Dentz]

Hi Michal,

On Thu, Mar 1, 2012 at 4:45 PM, Michal Labedzki
<michal.labedzki@tieto.com> wrote:
> Request ctype of "AbortContinuingResponse" is CONTROL, so response should be
> ACCEPTED instead of STABLE. This affect PTS Test Case for TP/RCR/BV-04-C.
> ---
>  audio/avrcp.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index c9ec314..4573133 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -986,7 +986,7 @@ static uint8_t avrcp_handle_abort_continuing(struct avrcp_player *player,
>        player_abort_pending_pdu(player);
>        pdu->params_len = 0;
>
> -       return AVC_CTYPE_STABLE;
> +       return AVC_CTYPE_ACCEPTED;
>
>  err:
>        pdu->params_len = htons(1);
> --
> on behalf of ST-Ericsson
>
> --

Lucas can you take a look and ack this, it should be very straight forward.


-- 
Luiz Augusto von Dentz

Re: [PATCH 1/3] avrcp: Fix response ctype of AbortContinuingResponse

[Lucas De Marchi]

On Thu, Mar 1, 2012 at 11:45 AM, Michal Labedzki
<michal.labedzki@tieto.com> wrote:
> Request ctype of "AbortContinuingResponse" is CONTROL, so response should be
> ACCEPTED instead of STABLE. This affect PTS Test Case for TP/RCR/BV-04-C.
> ---
>  audio/avrcp.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index c9ec314..4573133 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -986,7 +986,7 @@ static uint8_t avrcp_handle_abort_continuing(struct avrcp_player *player,
>        player_abort_pending_pdu(player);
>        pdu->params_len = 0;
>
> -       return AVC_CTYPE_STABLE;
> +       return AVC_CTYPE_ACCEPTED;

What version of PTS you are running? It seems like PTS was accepting
it  before. Last time I checked it was passing exactly this test, but
searching PTS bugs I found this:

https://www.bluetooth.org/pts/issues/view_issue.cfm?id=8043 (Created:
10-Jun-11     by: Bradford Ayres     ID: 38385)


So you have my ACK here.


Lucas De Marchi