[PATCH 0/3] track-changed notification

[PATCH 0/3] track-changed notification

[Lucas De Marchi]

While implementing the feature in the last patch, I noticed the track-changed
notification was not working anymore due to recent split of audio/control.c.

First two patches fix the issues and 3rd patch puts an UID in media_info. AVRCP
1.3 allows only two values:
	- 0x0: track playing
	- UINT64_MAX: no track playing/selected.

AVRCP 1.4 properly uses this field though, so I'm already preparing the ground
for it.

Lucas De Marchi (3):
  AVRCP: fix changed notification
  AVRCP: fix missing bytes on notification
  AVRCP: allow track to be un-selected

 audio/avrcp.c |   40 +++++++++++++++++++++-------------------
 1 files changed, 21 insertions(+), 19 deletions(-)

-- 
1.7.6.4

[PATCH 1/3] AVRCP: fix changed notification

[Lucas De Marchi]

We sure want to send notifications only when section is not NULL.
Otherwise we either crash or do not send the expected notification.
---
 audio/avrcp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index ac9a107..e5b51db 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -549,7 +549,7 @@ static int avrcp_send_event(struct media_player *mp, uint8_t id, void *data)
 	uint16_t size;
 	int err;
 
-	if (mp->session)
+	if (mp->session == NULL)
 		return -ENOTCONN;
 
 	if (!(mp->registered_events & (1 << id)))
-- 
1.7.6.4

Re: [PATCH 1/3] AVRCP: fix changed notification

[Lucas De Marchi]

Hi Luiz,

On Thu, Sep 29, 2011 at 2:07 PM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> We sure want to send notifications only when section is not NULL.
> Otherwise we either crash or do not send the expected notification.
> ---
>  audio/avrcp.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index ac9a107..e5b51db 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -549,7 +549,7 @@ static int avrcp_send_event(struct media_player *mp, uint8_t id, void *data)
>        uint16_t size;
>        int err;
>
> -       if (mp->session)
> +       if (mp->session == NULL)
>                return -ENOTCONN;

This fixes the issue with recent code move, but thinking about it see
the following scenario:

1) CT connects to TG and registers itself for events
2) CT disconnects
3) CT connects again, but does not register for receiving track-change
notification
4) Track changes and we send the track-change notification


It seems like (4) is wrong and what we really need to do is to
unregister the events when we are disconnected. What do you think?



Lucas De Marchi

Re: [PATCH 1/3] AVRCP: fix changed notification

[Luiz Augusto von Dentz]

Hi Lucas,

On Thu, Sep 29, 2011 at 9:01 PM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> Hi Luiz,
>
> On Thu, Sep 29, 2011 at 2:07 PM, Lucas De Marchi
> <lucas.demarchi@profusion.mobi> wrote:
>> We sure want to send notifications only when section is not NULL.
>> Otherwise we either crash or do not send the expected notification.
>> ---
>>  audio/avrcp.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/audio/avrcp.c b/audio/avrcp.c
>> index ac9a107..e5b51db 100644
>> --- a/audio/avrcp.c
>> +++ b/audio/avrcp.c
>> @@ -549,7 +549,7 @@ static int avrcp_send_event(struct media_player *mp, uint8_t id, void *data)
>>        uint16_t size;
>>        int err;
>>
>> -       if (mp->session)
>> +       if (mp->session == NULL)
>>                return -ENOTCONN;

That is a bug, ack.

> This fixes the issue with recent code move, but thinking about it see
> the following scenario:
>
> 1) CT connects to TG and registers itself for events
> 2) CT disconnects
> 3) CT connects again, but does not register for receiving track-change
> notification
> 4) Track changes and we send the track-change notification
>
>
> It seems like (4) is wrong and what we really need to do is to
> unregister the events when we are disconnected. What do you think?

Yep, we need to unregister when disconnected, but I think it would be
better to have something else to register the events like
avrcp_session which represents the connection so when we disconnect it
would be freed and events automatically unregistered.

Im also working on moving the MediaPlayer to adapter object, as an
agent, so we can support multiple players, properly track them and
have the 1.3 record only when there are players registered.

-- 
Luiz Augusto von Dentz

Re: [PATCH 1/3] AVRCP: fix changed notification

[Lucas De Marchi]

Hi Luiz,

On Thu, Sep 29, 2011 at 4:14 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Lucas,
>
> On Thu, Sep 29, 2011 at 9:01 PM, Lucas De Marchi
> <lucas.demarchi@profusion.mobi> wrote:
>> Hi Luiz,
>>
>> On Thu, Sep 29, 2011 at 2:07 PM, Lucas De Marchi
>> <lucas.demarchi@profusion.mobi> wrote:
>>> We sure want to send notifications only when section is not NULL.
>>> Otherwise we either crash or do not send the expected notification.
>>> ---
>>>  audio/avrcp.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/audio/avrcp.c b/audio/avrcp.c
>>> index ac9a107..e5b51db 100644
>>> --- a/audio/avrcp.c
>>> +++ b/audio/avrcp.c
>>> @@ -549,7 +549,7 @@ static int avrcp_send_event(struct media_player *mp, uint8_t id, void *data)
>>>        uint16_t size;
>>>        int err;
>>>
>>> -       if (mp->session)
>>> +       if (mp->session == NULL)
>>>                return -ENOTCONN;
>
> That is a bug, ack.
>
>> This fixes the issue with recent code move, but thinking about it see
>> the following scenario:
>>
>> 1) CT connects to TG and registers itself for events
>> 2) CT disconnects
>> 3) CT connects again, but does not register for receiving track-change
>> notification
>> 4) Track changes and we send the track-change notification
>>
>>
>> It seems like (4) is wrong and what we really need to do is to
>> unregister the events when we are disconnected. What do you think?
>
> Yep, we need to unregister when disconnected, but I think it would be
> better to have something else to register the events like
> avrcp_session which represents the connection so when we disconnect it
> would be freed and events automatically unregistered.
>
> Im also working on moving the MediaPlayer to adapter object, as an
> agent, so we can support multiple players, properly track them and

Yep, moving to adapter is a necessary thing. In the end it didn't make
much sense having it on per device.
Don't know how you are thinking about the agent - is it used only to
register/track the media players?

> have the 1.3 record only when there are players registered.

Since you are talking about multiple players, I assume you are talking
about AVRCP 1.4.

Don't know if this is a good idea. When we have 1.4, I think it would
be better to have 1.4 record published and 0 media players than
changing it after a connection is already established. How do you deal
with the case of previously connected devices?

>From section 6.9 of AVRCP 1.4 spec:

"If no players are available the TG shall return an error response
with the error
code No Available Players. Note that available means that a player can
be accessed via
AVRCP with no user interaction locally on the TG. It does not imply
that the media
player application must be currently running."

And from 5.9:

"A device that supports the media source role may contain zero or more
media players.
For the purposes of this document a media player is defined as an
entity which can fulfill
the requirements of this specification. Whether there is a one to one
mapping between
media player entities as presented over AVRCP and applications on the device is
implementation dependant."


Lucas De Marchi

Re: [PATCH 1/3] AVRCP: fix changed notification

[Luiz Augusto von Dentz]

Hi Lucas,

On Thu, Sep 29, 2011 at 11:17 PM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> Hi Luiz,
>
> On Thu, Sep 29, 2011 at 4:14 PM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Lucas,
>>
>> On Thu, Sep 29, 2011 at 9:01 PM, Lucas De Marchi
>> <lucas.demarchi@profusion.mobi> wrote:
>>> Hi Luiz,
>>>
>>> On Thu, Sep 29, 2011 at 2:07 PM, Lucas De Marchi
>>> <lucas.demarchi@profusion.mobi> wrote:
>>>> We sure want to send notifications only when section is not NULL.
>>>> Otherwise we either crash or do not send the expected notification.
>>>> ---
>>>>  audio/avrcp.c |    2 +-
>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/audio/avrcp.c b/audio/avrcp.c
>>>> index ac9a107..e5b51db 100644
>>>> --- a/audio/avrcp.c
>>>> +++ b/audio/avrcp.c
>>>> @@ -549,7 +549,7 @@ static int avrcp_send_event(struct media_player *mp, uint8_t id, void *data)
>>>>        uint16_t size;
>>>>        int err;
>>>>
>>>> -       if (mp->session)
>>>> +       if (mp->session == NULL)
>>>>                return -ENOTCONN;
>>
>> That is a bug, ack.
>>
>>> This fixes the issue with recent code move, but thinking about it see
>>> the following scenario:
>>>
>>> 1) CT connects to TG and registers itself for events
>>> 2) CT disconnects
>>> 3) CT connects again, but does not register for receiving track-change
>>> notification
>>> 4) Track changes and we send the track-change notification
>>>
>>>
>>> It seems like (4) is wrong and what we really need to do is to
>>> unregister the events when we are disconnected. What do you think?
>>
>> Yep, we need to unregister when disconnected, but I think it would be
>> better to have something else to register the events like
>> avrcp_session which represents the connection so when we disconnect it
>> would be freed and events automatically unregistered.
>>
>> Im also working on moving the MediaPlayer to adapter object, as an
>> agent, so we can support multiple players, properly track them and
>
> Yep, moving to adapter is a necessary thing. In the end it didn't make
> much sense having it on per device.
> Don't know how you are thinking about the agent - is it used only to
> register/track the media players?

The thing is that if we don't do via agent we have to track only by
bus id which means there could be only one player per connection but I
don't fell this is flexible enough because there could be systems
where a single process want to register multiple player e.g. mpris
agent.

>> have the 1.3 record only when there are players registered.
>
> Since you are talking about multiple players, I assume you are talking
> about AVRCP 1.4.
>
> Don't know if this is a good idea. When we have 1.4, I think it would
> be better to have 1.4 record published and 0 media players than
> changing it after a connection is already established. How do you deal
> with the case of previously connected devices?
>
> From section 6.9 of AVRCP 1.4 spec:
>
> "If no players are available the TG shall return an error response
> with the error
> code No Available Players. Note that available means that a player can
> be accessed via
> AVRCP with no user interaction locally on the TG. It does not imply
> that the media
> player application must be currently running."

Obviously we don't need to follow 100% what the spec says, while we
can emulate the player we just cannot emulate metadata, player
settings and status, so to me this statements after the No Available
Player are pretty clueless. But you have a point that we can just
return  No Available Player and be good with it so the record dont
need to change at run time (this can actually be a problem).

> And from 5.9:
>
> "A device that supports the media source role may contain zero or more
> media players.
> For the purposes of this document a media player is defined as an
> entity which can fulfill
> the requirements of this specification. Whether there is a one to one
> mapping between
> media player entities as presented over AVRCP and applications on the device is
> implementation dependant."

Im not sure if anything else than one to one mapping make sense here,
otherwise we have to implement a player inside bluetoothd and always
restore the settings when a real player connects, IMO that is a very
difficult to synchronize, so I would avoid doing anything when there
is no player active.

-- 
Luiz Augusto von Dentz

Re: [PATCH 1/3] AVRCP: fix changed notification

[Johan Hedberg]

Hi Lucas,

On Thu, Sep 29, 2011, Lucas De Marchi wrote:
> We sure want to send notifications only when section is not NULL.
> Otherwise we either crash or do not send the expected notification.
> ---
>  audio/avrcp.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

Thanks. This patch has been applied.

Johan

[PATCH 2/3] AVRCP: fix missing bytes on notification

[Lucas De Marchi]

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

diff --git a/audio/avrcp.c b/audio/avrcp.c
index e5b51db..6e8b8ff 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -591,7 +591,7 @@ static int avrcp_send_event(struct media_player *mp, uint8_t id, void *data)
 
 	err = avctp_send_vendordep(mp->session, mp->transaction_events[id],
 					AVC_CTYPE_CHANGED, AVC_SUBUNIT_PANEL,
-					buf, size);
+					buf, size + AVRCP_HEADER_LENGTH);
 	if (err < 0)
 		return err;
 
-- 
1.7.6.4

Re: [PATCH 2/3] AVRCP: fix missing bytes on notification

[Luiz Augusto von Dentz]

Hi Lucas,

On Thu, Sep 29, 2011 at 8:07 PM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> ---
>  audio/avrcp.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index e5b51db..6e8b8ff 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -591,7 +591,7 @@ static int avrcp_send_event(struct media_player *mp, uint8_t id, void *data)
>
>        err = avctp_send_vendordep(mp->session, mp->transaction_events[id],
>                                        AVC_CTYPE_CHANGED, AVC_SUBUNIT_PANEL,
> -                                       buf, size);
> +                                       buf, size + AVRCP_HEADER_LENGTH);
>        if (err < 0)
>                return err;
>
> --
> 1.7.6.4

Ack.

-- 
Luiz Augusto von Dentz

Re: [PATCH 2/3] AVRCP: fix missing bytes on notification

[Johan Hedberg]

Hi Lucas,

On Thu, Sep 29, 2011, Lucas De Marchi wrote:
> ---
>  audio/avrcp.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

This patch has also been applied. Thanks.

Johan

[PATCH 3/3] AVRCP: allow track to be un-selected

[Lucas De Marchi]

As of now there was no way to tell BlueZ that no track is selected. Now
if client send us a ChangeTrack() with empty metadata, it means there's no
track currently selected/playing. This is necessary in order to pass
TP/NFY/BV-04-C that expects us to respond with:
	- 0x0: a track is playing
	- UINT64_MAX: there's no track playing
---
 audio/avrcp.c |   36 +++++++++++++++++++-----------------
 1 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index 6e8b8ff..8fb23e0 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -191,6 +191,7 @@ struct media_info {
 	uint32_t track;
 	uint32_t track_len;
 	uint32_t elapsed;
+	uint64_t uid;
 };
 
 struct media_player {
@@ -570,18 +571,11 @@ static int avrcp_send_event(struct media_player *mp, 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], (uint64_t *) data, sizeof(uint64_t));
 
 		break;
-	}
 	default:
 		error("Unknown event %u", id);
 		return -EINVAL;
@@ -753,6 +747,9 @@ static int mp_get_attribute(struct media_player *mp, uint8_t attr)
 static void mp_set_media_attributes(struct media_player *mp,
 							struct media_info *mi)
 {
+	if (mp->mi.uid == UINT64_MAX && mi->uid == UINT64_MAX)
+		return;
+
 	g_free(mp->mi.title);
 	mp->mi.title = g_strdup(mi->title);
 
@@ -768,6 +765,7 @@ static void mp_set_media_attributes(struct media_player *mp,
 	mp->mi.ntracks = mi->ntracks;
 	mp->mi.track = mi->track;
 	mp->mi.track_len = mi->track_len;
+	mp->mi.uid = mi->uid;
 
 	/*
 	 * elapsed is special. Whenever the track changes, we reset it to 0,
@@ -782,7 +780,7 @@ static void mp_set_media_attributes(struct media_player *mp,
 			mi->title, mi->artist, mi->album, mi->genre,
 			mi->ntracks, mi->track, mi->track_len);
 
-	avrcp_send_event(mp, AVRCP_EVENT_TRACK_CHANGED, NULL);
+	avrcp_send_event(mp, AVRCP_EVENT_TRACK_CHANGED, &mi->uid);
 }
 
 static uint8_t avrcp_handle_get_capabilities(struct media_player *mp,
@@ -1166,8 +1164,7 @@ static uint8_t avrcp_handle_register_notification(struct media_player *mp,
 		break;
 	case AVRCP_EVENT_TRACK_CHANGED:
 		len = 9;
-
-		memset(&pdu->params[1], 0, 8);
+		memcpy(&pdu->params[1], &mp->mi.uid, sizeof(uint64_t));
 
 		break;
 	default:
@@ -1320,6 +1317,7 @@ static void media_info_init(struct media_info *mi)
 	 */
 	mi->track_len = 0xFFFFFFFF;
 	mi->elapsed = 0xFFFFFFFF;
+	mi->uid = UINT64_MAX;
 }
 
 gboolean avrcp_connect(struct audio_device *dev)
@@ -1518,6 +1516,7 @@ static gboolean media_info_parse(DBusMessageIter *iter, struct media_info *mi)
 	DBusMessageIter dict;
 	DBusMessageIter var;
 	int ctype;
+	int nattr;
 
 	ctype = dbus_message_iter_get_arg_type(iter);
 	if (ctype != DBUS_TYPE_ARRAY)
@@ -1526,8 +1525,8 @@ static gboolean media_info_parse(DBusMessageIter *iter, struct media_info *mi)
 	media_info_init(mi);
 	dbus_message_iter_recurse(iter, &dict);
 
-	while ((ctype = dbus_message_iter_get_arg_type(&dict)) !=
-							DBUS_TYPE_INVALID) {
+	for(nattr = 0; (ctype = dbus_message_iter_get_arg_type(&dict)) !=
+						DBUS_TYPE_INVALID; nattr++) {
 		DBusMessageIter entry;
 		const char *key;
 
@@ -1595,8 +1594,12 @@ static gboolean media_info_parse(DBusMessageIter *iter, struct media_info *mi)
 		dbus_message_iter_next(&dict);
 	}
 
-	if (mi->title == NULL)
-		return FALSE;
+	if (nattr > 0) {
+		if (mi->title == NULL)
+			return FALSE;
+
+		mi->uid = 0;
+	}
 
 	return TRUE;
 }
@@ -1609,7 +1612,6 @@ static DBusMessage *mp_change_track(DBusConnection *conn,
 	DBusMessageIter iter;
 	struct media_info mi;
 
-
 	dbus_message_iter_init(msg, &iter);
 	if (!media_info_parse(&iter, &mi))
 		return btd_error_invalid_args(msg);
-- 
1.7.6.4

Re: [PATCH 3/3] AVRCP: allow track to be un-selected

[Luiz Augusto von Dentz]

Hi Lucas,

On Thu, Sep 29, 2011 at 8:07 PM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> As of now there was no way to tell BlueZ that no track is selected. Now
> if client send us a ChangeTrack() with empty metadata, it means there's no
> track currently selected/playing. This is necessary in order to pass
> TP/NFY/BV-04-C that expects us to respond with:
>        - 0x0: a track is playing
>        - UINT64_MAX: there's no track playing
> ---
>  audio/avrcp.c |   36 +++++++++++++++++++-----------------
>  1 files changed, 19 insertions(+), 17 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index 6e8b8ff..8fb23e0 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -191,6 +191,7 @@ struct media_info {
>        uint32_t track;
>        uint32_t track_len;
>        uint32_t elapsed;
> +       uint64_t uid;
>  };
>
>  struct media_player {
> @@ -570,18 +571,11 @@ static int avrcp_send_event(struct media_player *mp, 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], (uint64_t *) data, sizeof(uint64_t));
>
>                break;
> -       }
>        default:
>                error("Unknown event %u", id);
>                return -EINVAL;
> @@ -753,6 +747,9 @@ static int mp_get_attribute(struct media_player *mp, uint8_t attr)
>  static void mp_set_media_attributes(struct media_player *mp,
>                                                        struct media_info *mi)
>  {
> +       if (mp->mi.uid == UINT64_MAX && mi->uid == UINT64_MAX)
> +               return;
> +
>        g_free(mp->mi.title);
>        mp->mi.title = g_strdup(mi->title);
>
> @@ -768,6 +765,7 @@ static void mp_set_media_attributes(struct media_player *mp,
>        mp->mi.ntracks = mi->ntracks;
>        mp->mi.track = mi->track;
>        mp->mi.track_len = mi->track_len;
> +       mp->mi.uid = mi->uid;
>
>        /*
>         * elapsed is special. Whenever the track changes, we reset it to 0,
> @@ -782,7 +780,7 @@ static void mp_set_media_attributes(struct media_player *mp,
>                        mi->title, mi->artist, mi->album, mi->genre,
>                        mi->ntracks, mi->track, mi->track_len);
>
> -       avrcp_send_event(mp, AVRCP_EVENT_TRACK_CHANGED, NULL);
> +       avrcp_send_event(mp, AVRCP_EVENT_TRACK_CHANGED, &mi->uid);
>  }
>
>  static uint8_t avrcp_handle_get_capabilities(struct media_player *mp,
> @@ -1166,8 +1164,7 @@ static uint8_t avrcp_handle_register_notification(struct media_player *mp,
>                break;
>        case AVRCP_EVENT_TRACK_CHANGED:
>                len = 9;
> -
> -               memset(&pdu->params[1], 0, 8);
> +               memcpy(&pdu->params[1], &mp->mi.uid, sizeof(uint64_t));
>
>                break;
>        default:
> @@ -1320,6 +1317,7 @@ static void media_info_init(struct media_info *mi)
>         */
>        mi->track_len = 0xFFFFFFFF;
>        mi->elapsed = 0xFFFFFFFF;
> +       mi->uid = UINT64_MAX;
>  }
>
>  gboolean avrcp_connect(struct audio_device *dev)
> @@ -1518,6 +1516,7 @@ static gboolean media_info_parse(DBusMessageIter *iter, struct media_info *mi)
>        DBusMessageIter dict;
>        DBusMessageIter var;
>        int ctype;
> +       int nattr;
>
>        ctype = dbus_message_iter_get_arg_type(iter);
>        if (ctype != DBUS_TYPE_ARRAY)
> @@ -1526,8 +1525,8 @@ static gboolean media_info_parse(DBusMessageIter *iter, struct media_info *mi)
>        media_info_init(mi);
>        dbus_message_iter_recurse(iter, &dict);
>
> -       while ((ctype = dbus_message_iter_get_arg_type(&dict)) !=
> -                                                       DBUS_TYPE_INVALID) {
> +       for(nattr = 0; (ctype = dbus_message_iter_get_arg_type(&dict)) !=
> +                                               DBUS_TYPE_INVALID; nattr++) {
>                DBusMessageIter entry;
>                const char *key;
>
> @@ -1595,8 +1594,12 @@ static gboolean media_info_parse(DBusMessageIter *iter, struct media_info *mi)
>                dbus_message_iter_next(&dict);
>        }
>
> -       if (mi->title == NULL)
> -               return FALSE;
> +       if (nattr > 0) {
> +               if (mi->title == NULL)
> +                       return FALSE;
> +
> +               mi->uid = 0;
> +       }
>
>        return TRUE;
>  }
> @@ -1609,7 +1612,6 @@ static DBusMessage *mp_change_track(DBusConnection *conn,
>        DBusMessageIter iter;
>        struct media_info mi;
>
> -
>        dbus_message_iter_init(msg, &iter);
>        if (!media_info_parse(&iter, &mi))
>                return btd_error_invalid_args(msg);
> --
> 1.7.6.4

I have this more or less covered in mine yet to be published changes
to have player registration per adapter, so I would like to ask you to
hold this a bit until I finish to avoid to many conflicts.

-- 
Luiz Augusto von Dentz

Re: [PATCH 3/3] AVRCP: allow track to be un-selected

[Lucas De Marchi]

Hi Luiz,

On Thu, Sep 29, 2011 at 4:31 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Lucas,
>
> On Thu, Sep 29, 2011 at 8:07 PM, Lucas De Marchi
> <lucas.demarchi@profusion.mobi> wrote:
>> As of now there was no way to tell BlueZ that no track is selected. Now
>> if client send us a ChangeTrack() with empty metadata, it means there's no
>> track currently selected/playing. This is necessary in order to pass
>> TP/NFY/BV-04-C that expects us to respond with:
>>        - 0x0: a track is playing
>>        - UINT64_MAX: there's no track playing
>> ---
>>  audio/avrcp.c |   36 +++++++++++++++++++-----------------
>>  1 files changed, 19 insertions(+), 17 deletions(-)
>>
>> diff --git a/audio/avrcp.c b/audio/avrcp.c
>> index 6e8b8ff..8fb23e0 100644
>> --- a/audio/avrcp.c
>> +++ b/audio/avrcp.c
>> @@ -191,6 +191,7 @@ struct media_info {
>>        uint32_t track;
>>        uint32_t track_len;
>>        uint32_t elapsed;
>> +       uint64_t uid;
>>  };
>>
>>  struct media_player {
>> @@ -570,18 +571,11 @@ static int avrcp_send_event(struct media_player *mp, 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], (uint64_t *) data, sizeof(uint64_t));
>>
>>                break;
>> -       }
>>        default:
>>                error("Unknown event %u", id);
>>                return -EINVAL;
>> @@ -753,6 +747,9 @@ static int mp_get_attribute(struct media_player *mp, uint8_t attr)
>>  static void mp_set_media_attributes(struct media_player *mp,
>>                                                        struct media_info *mi)
>>  {
>> +       if (mp->mi.uid == UINT64_MAX && mi->uid == UINT64_MAX)
>> +               return;
>> +
>>        g_free(mp->mi.title);
>>        mp->mi.title = g_strdup(mi->title);
>>
>> @@ -768,6 +765,7 @@ static void mp_set_media_attributes(struct media_player *mp,
>>        mp->mi.ntracks = mi->ntracks;
>>        mp->mi.track = mi->track;
>>        mp->mi.track_len = mi->track_len;
>> +       mp->mi.uid = mi->uid;
>>
>>        /*
>>         * elapsed is special. Whenever the track changes, we reset it to 0,
>> @@ -782,7 +780,7 @@ static void mp_set_media_attributes(struct media_player *mp,
>>                        mi->title, mi->artist, mi->album, mi->genre,
>>                        mi->ntracks, mi->track, mi->track_len);
>>
>> -       avrcp_send_event(mp, AVRCP_EVENT_TRACK_CHANGED, NULL);
>> +       avrcp_send_event(mp, AVRCP_EVENT_TRACK_CHANGED, &mi->uid);
>>  }
>>
>>  static uint8_t avrcp_handle_get_capabilities(struct media_player *mp,
>> @@ -1166,8 +1164,7 @@ static uint8_t avrcp_handle_register_notification(struct media_player *mp,
>>                break;
>>        case AVRCP_EVENT_TRACK_CHANGED:
>>                len = 9;
>> -
>> -               memset(&pdu->params[1], 0, 8);
>> +               memcpy(&pdu->params[1], &mp->mi.uid, sizeof(uint64_t));
>>
>>                break;
>>        default:
>> @@ -1320,6 +1317,7 @@ static void media_info_init(struct media_info *mi)
>>         */
>>        mi->track_len = 0xFFFFFFFF;
>>        mi->elapsed = 0xFFFFFFFF;
>> +       mi->uid = UINT64_MAX;
>>  }
>>
>>  gboolean avrcp_connect(struct audio_device *dev)
>> @@ -1518,6 +1516,7 @@ static gboolean media_info_parse(DBusMessageIter *iter, struct media_info *mi)
>>        DBusMessageIter dict;
>>        DBusMessageIter var;
>>        int ctype;
>> +       int nattr;
>>
>>        ctype = dbus_message_iter_get_arg_type(iter);
>>        if (ctype != DBUS_TYPE_ARRAY)
>> @@ -1526,8 +1525,8 @@ static gboolean media_info_parse(DBusMessageIter *iter, struct media_info *mi)
>>        media_info_init(mi);
>>        dbus_message_iter_recurse(iter, &dict);
>>
>> -       while ((ctype = dbus_message_iter_get_arg_type(&dict)) !=
>> -                                                       DBUS_TYPE_INVALID) {
>> +       for(nattr = 0; (ctype = dbus_message_iter_get_arg_type(&dict)) !=
>> +                                               DBUS_TYPE_INVALID; nattr++) {
>>                DBusMessageIter entry;
>>                const char *key;
>>
>> @@ -1595,8 +1594,12 @@ static gboolean media_info_parse(DBusMessageIter *iter, struct media_info *mi)
>>                dbus_message_iter_next(&dict);
>>        }
>>
>> -       if (mi->title == NULL)
>> -               return FALSE;
>> +       if (nattr > 0) {
>> +               if (mi->title == NULL)
>> +                       return FALSE;
>> +
>> +               mi->uid = 0;
>> +       }
>>
>>        return TRUE;
>>  }
>> @@ -1609,7 +1612,6 @@ static DBusMessage *mp_change_track(DBusConnection *conn,
>>        DBusMessageIter iter;
>>        struct media_info mi;
>>
>> -
>>        dbus_message_iter_init(msg, &iter);
>>        if (!media_info_parse(&iter, &mi))
>>                return btd_error_invalid_args(msg);
>> --
>> 1.7.6.4
>
> I have this more or less covered in mine yet to be published changes
> to have player registration per adapter, so I would like to ask you to
> hold this a bit until I finish to avoid to many conflicts.

It's not a nice thing asking to wait for unpublished code. Do you have
an ETA? The code above is a very basic thing, necessary for people
doing AVRCP 1.3 certification.

I already have other patches queued here:
 - TRACK_REACHED_END event;
 - TRACK_REACHED_START event;
 - PDU_CONTINUING / PDU_ABORT (this was already sent once, but I will
refactor it as requested)

So there are already conflicts and they will continue to exist if we
don't sync what we are doing.



Lucas De Marchi

Re: [PATCH 3/3] AVRCP: allow track to be un-selected

[Luiz Augusto von Dentz]

Hi Lucas,

On Thu, Sep 29, 2011 at 10:54 PM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> Hi Luiz,
>
>> I have this more or less covered in mine yet to be published changes
>> to have player registration per adapter, so I would like to ask you to
>> hold this a bit until I finish to avoid to many conflicts.
>
> It's not a nice thing asking to wait for unpublished code. Do you have
> an ETA? The code above is a very basic thing, necessary for people
> doing AVRCP 1.3 certification.

This should be ready by next week to be tested at UPF, also I would
suggest not doing any official qualification until we do proper IOP
testing with this code, first because it may pass qualification but
may not work properly with many platforms, second there is still no
release that contains this code and third we are still doing
modification to its API.

> I already have other patches queued here:
>  - TRACK_REACHED_END event;
>  - TRACK_REACHED_START event;
>  - PDU_CONTINUING / PDU_ABORT (this was already sent once, but I will
> refactor it as requested)
>
> So there are already conflicts and they will continue to exist if we
> don't sync what we are doing.

Im sorry if this is a bad timing , but I really want to run IOP with
an interface per adapter and not the one used by test-player. In my
experience the API definition is almost always more important than the
protocol itself, it is much easier to fix the latter when a problem is
detected than having to break API if that doesn't fulfill some
requirements.

-- 
Luiz Augusto von Dentz