[PATCH 1/3] audio/avrcp: Get player playlist details

[PATCH 1/3] audio/avrcp: Get player playlist details

[Bharat Panda]

Support added to read and cache player playlist details after
player registration completes.
---
 profiles/audio/media.c | 164 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 164 insertions(+)

diff --git a/profiles/audio/media.c b/profiles/audio/media.c
index 69070bf..1b5246d 100644
--- a/profiles/audio/media.c
+++ b/profiles/audio/media.c
@@ -58,6 +58,7 @@
 #define MEDIA_INTERFACE "org.bluez.Media1"
 #define MEDIA_ENDPOINT_INTERFACE "org.bluez.MediaEndpoint1"
 #define MEDIA_PLAYER_INTERFACE "org.mpris.MediaPlayer2.Player"
+#define MEDIA_PLAYER_PLAYLIST_INTERFACE "org.mpris.MediaPlayer2.Playlists"
 
 #define REQUEST_TIMEOUT (3 * 1000)		/* 3 seconds */
 
@@ -92,9 +93,19 @@ struct media_endpoint {
 	GSList			*transports;
 };
 
+struct player_request {
+	struct media_player	*mp;
+	DBusMessage		*msg;
+	DBusPendingCall		*call;
+	GDestroyNotify		destroy;
+	void			*user_data;
+};
+
 struct media_player {
 	struct media_adapter	*adapter;
 	struct avrcp_player	*player;
+	GSList			*playlists;
+	GSList			*requests;
 	char			*sender;	/* Player DBus bus id */
 	char			*path;		/* Player object path */
 	GHashTable		*settings;	/* Player settings */
@@ -105,6 +116,7 @@ struct media_player {
 	char			*status;
 	uint32_t		position;
 	uint32_t		duration;
+	uint32_t		total_items;
 	uint8_t			volume;
 	GTimer			*timer;
 	bool			play;
@@ -115,6 +127,13 @@ struct media_player {
 	char			*name;
 };
 
+struct media_playlist {
+	char		*name;
+	char		*id;
+	char		*icon;
+	uint8_t		data[0];
+} __attribute__ ((packed));
+
 static GSList *adapters = NULL;
 
 static void endpoint_request_free(struct endpoint_request *request)
@@ -966,6 +985,7 @@ static void media_player_free(gpointer data)
 	g_free(mp->path);
 	g_free(mp->status);
 	g_free(mp->name);
+	g_free(mp->playlists);
 	g_free(mp);
 }
 
@@ -1271,6 +1291,147 @@ static bool previous(void *user_data)
 	return media_player_send(mp, "Previous");
 }
 
+static void playlist_reply(DBusPendingCall *call, void *user_data)
+{
+	struct player_request *request = user_data;
+	struct media_player *mp = request->mp;
+	DBusMessage *reply;
+	DBusMessageIter array_iter, entry, struct_iter;
+	DBusError derr;
+	int ctype;
+
+	reply = dbus_pending_call_steal_reply(call);
+
+	dbus_error_init(&derr);
+
+	if (dbus_set_error_from_message(&derr, reply)) {
+		error("Error: GetPlayLists method %s, %s", derr.name,
+					derr.message);
+		dbus_error_free(&derr);
+		goto done;
+	}
+
+	dbus_message_iter_init(reply, &array_iter);
+	ctype = dbus_message_iter_get_arg_type(&array_iter);
+
+	if (ctype != DBUS_TYPE_ARRAY)
+		goto done;
+
+	dbus_message_iter_recurse(&array_iter, &struct_iter);
+
+	while ((ctype = dbus_message_iter_get_arg_type(&struct_iter)) !=
+				DBUS_TYPE_INVALID) {
+		struct media_playlist *playlist;
+
+		if (ctype != DBUS_TYPE_STRUCT) {
+			error("Invalid type");
+			goto done;
+		}
+
+		playlist = g_new0(struct media_playlist, 1);
+
+		dbus_message_iter_recurse(&struct_iter, &entry);
+
+		dbus_message_iter_get_basic(&entry, &playlist->id);
+		DBG("Playlists Id %s", playlist->id);
+
+		dbus_message_iter_next(&entry);
+		dbus_message_iter_get_basic(&entry, &playlist->name);
+		DBG("Playlist Name %s", playlist->name);
+
+		dbus_message_iter_next(&entry);
+		dbus_message_iter_get_basic(&entry, &playlist->icon);
+		DBG("Playlist Icon %s", playlist->icon);
+
+		/* TODO: Create Media folder with playlist information */
+
+		mp->playlists = g_slist_append(mp->playlists, playlist);
+
+		g_free(playlist);
+
+		mp->total_items++;
+		dbus_message_iter_next(&struct_iter);
+	}
+
+done:
+	dbus_message_unref(reply);
+
+	mp->requests = g_slist_remove(mp->requests, request);
+
+	if (request->call)
+		dbus_pending_call_unref(request->call);
+
+	if (request->destroy)
+		request->destroy(request->user_data);
+
+	dbus_message_unref(request->msg);
+	g_free(request);
+}
+
+static gboolean media_player_async_call(DBusMessage *msg,
+					struct media_player *mp,
+					GDestroyNotify destroy)
+{
+	struct player_request *request;
+	const char *method = NULL;
+
+	request = g_new0(struct player_request, 1);
+
+	if (!(dbus_connection_send_with_reply(btd_get_dbus_connection(),
+					msg, &request->call, -1))) {
+		error("D-Bus send failed");
+		g_free(request);
+		return FALSE;
+	}
+
+	method = dbus_message_get_member(msg);
+
+	if (g_strcmp0(method, "GetPlaylists") == 0)
+		dbus_pending_call_set_notify(request->call,
+						&playlist_reply, request, NULL);
+
+	request->mp = mp;
+	request->msg = msg;
+	request->destroy = destroy;
+
+	mp->requests = g_slist_append(mp->requests, request);
+
+	DBG("Calling %s: name = %s path = %s", dbus_message_get_member(msg),
+			dbus_message_get_destination(msg),
+			dbus_message_get_path(msg));
+
+	return TRUE;
+}
+static gboolean get_playlists(struct media_player *mp)
+{
+	DBusMessage *msg;
+	gboolean reverse_order = FALSE;
+	uint32_t start_index = 0x0000;
+	uint32_t end_index = 0xFFFF;
+	char *ordering = "Alphabetical";
+	uint32_t max_count = (end_index - start_index);
+
+	msg = dbus_message_new_method_call(mp->sender, mp->path,
+				MEDIA_PLAYER_PLAYLIST_INTERFACE,
+				"GetPlaylists");
+
+	if (msg == NULL) {
+		error("Couldn't allocate D-Bus message");
+		return -ENOMEM;
+	}
+
+	dbus_message_append_args(msg,
+			DBUS_TYPE_UINT32, &start_index,
+			DBUS_TYPE_UINT32, &max_count,
+			DBUS_TYPE_STRING, &ordering,
+			DBUS_TYPE_BOOLEAN, &reverse_order,
+			DBUS_TYPE_INVALID);
+
+	mp->total_items = 0;
+
+	return (media_player_async_call(msg, mp, g_free));
+}
+
 static struct avrcp_player_cb player_cb = {
 	.list_settings = list_settings,
 	.get_setting = get_setting,
@@ -1831,6 +1992,9 @@ static DBusMessage *register_player(DBusConnection *conn, DBusMessage *msg,
 		return btd_error_invalid_args(msg);
 	}
 
+	if (!get_playlists(mp))
+		DBG("Error fetching playlists");
+
 	return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
 }
 
-- 
1.9.1

[PATCH 2/3] audio/avrcp: Handle PlaylistChanged signal

[Bharat Panda]

Added signal handler for "PlaylistChanged" on MediaPlayer2.Playlists
interface.
---
 profiles/audio/media.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/profiles/audio/media.c b/profiles/audio/media.c
index 1b5246d..94fce79 100644
--- a/profiles/audio/media.c
+++ b/profiles/audio/media.c
@@ -113,6 +113,7 @@ struct media_player {
 	guint			watch;
 	guint			properties_watch;
 	guint			seek_watch;
+	guint			playlist_watch;
 	char			*status;
 	uint32_t		position;
 	uint32_t		duration;
@@ -973,6 +974,7 @@ static void media_player_free(gpointer data)
 	g_dbus_remove_watch(conn, mp->watch);
 	g_dbus_remove_watch(conn, mp->properties_watch);
 	g_dbus_remove_watch(conn, mp->seek_watch);
+	g_dbus_remove_watch(conn, mp->playlist_watch);
 
 	if (mp->track)
 		g_hash_table_unref(mp->track);
@@ -1913,6 +1915,76 @@ static gboolean position_changed(DBusConnection *connection, DBusMessage *msg,
 	return TRUE;
 }
 
+static struct media_playlist *find_playlist_by_id(struct media_player *mp,
+						char *playlist_id)
+{
+	GSList *l;
+
+	for (l = mp->playlists; l; l = l->next) {
+		struct media_playlist *playlist = l->data;
+
+		if (g_strcmp0(playlist->id, playlist_id) == 0) {
+			return playlist;
+		}
+	}
+
+	return NULL;
+}
+
+static gboolean playlist_changed(DBusConnection *connection,
+						DBusMessage *msg,
+						void *user_data)
+{
+	struct media_player *mp = user_data;
+	struct media_playlist *playlist;
+	DBusMessageIter iter;
+	DBusMessageIter entry;
+	char *playlist_id;
+	char *name;
+	char *icon;
+
+	dbus_message_iter_init(msg, &iter);
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRUCT)
+		return FALSE;
+
+	dbus_message_iter_recurse(&iter, &entry);
+
+	if (dbus_message_iter_get_arg_type(&entry) !=
+			DBUS_TYPE_OBJECT_PATH)
+		return FALSE;
+
+	dbus_message_iter_get_basic(&entry, &playlist_id);
+
+	dbus_message_iter_next(&entry);
+	if (dbus_message_iter_get_arg_type(&entry) !=
+			DBUS_TYPE_STRING)
+		return FALSE;
+
+	dbus_message_iter_get_basic(&entry, &name);
+
+	dbus_message_iter_next(&entry);
+	if (dbus_message_iter_get_arg_type(&entry) !=
+			DBUS_TYPE_STRING)
+		return FALSE;
+
+	dbus_message_iter_get_basic(&entry, &icon);
+
+	playlist = find_playlist_by_id(mp, playlist_id);
+
+	if (playlist) {
+		playlist->name = g_strdup(name);
+		playlist->icon = g_strdup(icon);
+	} else {
+		playlist = g_new0(struct media_playlist, 1);
+
+		playlist->id = playlist_id;
+		playlist->name = g_strdup(name);
+		playlist->icon = g_strdup(icon);
+	}
+
+	return TRUE;
+}
+
 static struct media_player *media_player_create(struct media_adapter *adapter,
 						const char *sender,
 						const char *path,
@@ -1995,6 +2067,13 @@ static DBusMessage *register_player(DBusConnection *conn, DBusMessage *msg,
 	if (!get_playlists(mp))
 		DBG("Error fetching playlists");
 
+	mp->playlist_watch = g_dbus_add_signal_watch(conn,
+					sender, path,
+					MEDIA_PLAYER_PLAYLIST_INTERFACE,
+					"PlaylistChanged",
+					playlist_changed,
+					mp, NULL);
+
 	return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
 }
 
-- 
1.9.1

[PATCH 3/3] audio/avrcp: Add support for SetBrowsedPlayer

[Bharat Panda]

Support added to handle SetBrowsedPlayer command for TG role.

    AVCTP Browsing: Response: type 0x00 label 13 PID 0x110e
      AVRCP: SetBrowsedPlayer: len 0x000a
       Status: 0x04 (Success)
       UIDCounter: 0x0000 (0)
       Number of Items: 0x00000007 (7)
       CharsetID: 0x006a (UTF-8)
       Folder Depth: 0x00 (0)
---
 profiles/audio/avrcp.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++
 profiles/audio/avrcp.h |  1 +
 profiles/audio/media.c |  8 +++++++
 3 files changed, 72 insertions(+)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 7b60012..c20fbcb 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -207,6 +207,15 @@ struct player_item {
 	char name[0];
 } __attribute__ ((packed));
 
+struct avrcp_set_browsed_player_rsp {
+	uint8_t status;
+	uint16_t uid_counter;
+	uint32_t num_of_items;
+	uint16_t charset_id;
+	uint8_t folder_depth;
+	uint8_t data[0];
+} __attribute__ ((packed));
+
 struct avrcp_server {
 	struct btd_adapter *adapter;
 	uint32_t tg_record_id;
@@ -1873,6 +1882,59 @@ err_metadata:
 	return AVRCP_HEADER_LENGTH + 1;
 }
 
+static void avrcp_handle_set_browsed_player(struct avrcp *session,
+				struct avrcp_browsing_header *pdu,
+				uint8_t transaction)
+{
+	struct avrcp_player *player;
+	struct media_player *mp;
+	struct avrcp_set_browsed_player_rsp *resp;
+	uint16_t len = ntohs(pdu->param_len);
+	uint16_t player_id = 0;
+	uint8_t status;
+
+	if (len < 1) {
+		status = AVRCP_STATUS_INVALID_PARAM;
+		goto failed;
+	}
+
+	player_id = bt_get_be16(&pdu->params[0]);
+	player = find_tg_player(session, player_id);
+
+	if (!player) {
+		status = AVRCP_STATUS_NO_AVAILABLE_PLAYERS;
+		goto failed;
+	}
+
+	if (!(features[7] & 0x08)) {
+		status = AVRCP_STATUS_PLAYER_NOT_BROWSABLE;
+		goto failed;
+	}
+
+	mp = player->user_data;
+	player->browsed = true;
+
+	resp = (struct avrcp_set_browsed_player_rsp *) pdu->params;
+
+	resp->status = AVRCP_STATUS_SUCCESS;
+	resp->uid_counter = htons(player_get_uid_counter(player));
+	resp->num_of_items = htonl(player->cb->get_total_items(mp));
+
+	/*
+	 * MPRIS player returns a flat hierarchy playlist structure,
+	 * where the folder depth will always considered to be 0.
+	 */
+	resp->folder_depth = 0;
+	resp->charset_id = htons(AVRCP_CHARSET_UTF8);
+	pdu->param_len = htons(sizeof(*resp));
+
+	return;
+
+failed:
+	pdu->params[0] = status;
+	pdu->param_len = htons(1);
+}
+
 static void avrcp_handle_media_player_list(struct avrcp *session,
 				struct avrcp_browsing_header *pdu,
 				uint32_t start_item, uint32_t end_item)
@@ -1989,6 +2051,7 @@ static struct browsing_pdu_handler {
 							uint8_t transaction);
 } browsing_handlers[] = {
 		{ AVRCP_GET_FOLDER_ITEMS, avrcp_handle_get_folder_items },
+		{ AVRCP_SET_BROWSED_PLAYER, avrcp_handle_set_browsed_player},
 		{ },
 };
 
diff --git a/profiles/audio/avrcp.h b/profiles/audio/avrcp.h
index 86d310c..c09b910 100644
--- a/profiles/audio/avrcp.h
+++ b/profiles/audio/avrcp.h
@@ -101,6 +101,7 @@ struct avrcp_player_cb {
 	bool (*pause) (void *user_data);
 	bool (*next) (void *user_data);
 	bool (*previous) (void *user_data);
+	uint32_t (*get_total_items) (void *user_data);
 };
 
 int avrcp_set_volume(struct btd_device *dev, uint8_t volume, bool notify);
diff --git a/profiles/audio/media.c b/profiles/audio/media.c
index 94fce79..a4ff4cc 100644
--- a/profiles/audio/media.c
+++ b/profiles/audio/media.c
@@ -1293,6 +1293,13 @@ static bool previous(void *user_data)
 	return media_player_send(mp, "Previous");
 }
 
+static uint32_t get_total_items(void *user_data)
+{
+	struct media_player *mp = user_data;
+
+	return mp->total_items;
+}
+
 static void playlist_reply(DBusPendingCall *call, void *user_data)
 {
 	struct player_request *request = user_data;
@@ -1451,6 +1458,7 @@ static struct avrcp_player_cb player_cb = {
 	.pause = pause,
 	.next = next,
 	.previous = previous,
+	.get_total_items = get_total_items,
 };
 
 static void media_player_exit(DBusConnection *connection, void *user_data)
-- 
1.9.1

Re: [PATCH 1/3] audio/avrcp: Get player playlist details

[Luiz Augusto von Dentz]

Hi Bharat,

On Tue, Oct 20, 2015 at 4:16 PM, Bharat Panda <bharat.panda@samsung.com> wrote:
> Support added to read and cache player playlist details after
> player registration completes.
> ---
>  profiles/audio/media.c | 164 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 164 insertions(+)
>
> diff --git a/profiles/audio/media.c b/profiles/audio/media.c
> index 69070bf..1b5246d 100644
> --- a/profiles/audio/media.c
> +++ b/profiles/audio/media.c
> @@ -58,6 +58,7 @@
>  #define MEDIA_INTERFACE "org.bluez.Media1"
>  #define MEDIA_ENDPOINT_INTERFACE "org.bluez.MediaEndpoint1"
>  #define MEDIA_PLAYER_INTERFACE "org.mpris.MediaPlayer2.Player"
> +#define MEDIA_PLAYER_PLAYLIST_INTERFACE "org.mpris.MediaPlayer2.Playlists"
>
>  #define REQUEST_TIMEOUT (3 * 1000)             /* 3 seconds */
>
> @@ -92,9 +93,19 @@ struct media_endpoint {
>         GSList                  *transports;
>  };
>
> +struct player_request {
> +       struct media_player     *mp;
> +       DBusMessage             *msg;
> +       DBusPendingCall         *call;
> +       GDestroyNotify          destroy;
> +       void                    *user_data;
> +};
> +
>  struct media_player {
>         struct media_adapter    *adapter;
>         struct avrcp_player     *player;
> +       GSList                  *playlists;
> +       GSList                  *requests;
>         char                    *sender;        /* Player DBus bus id */
>         char                    *path;          /* Player object path */
>         GHashTable              *settings;      /* Player settings */
> @@ -105,6 +116,7 @@ struct media_player {
>         char                    *status;
>         uint32_t                position;
>         uint32_t                duration;
> +       uint32_t                total_items;
>         uint8_t                 volume;
>         GTimer                  *timer;
>         bool                    play;
> @@ -115,6 +127,13 @@ struct media_player {
>         char                    *name;
>  };
>
> +struct media_playlist {
> +       char            *name;
> +       char            *id;
> +       char            *icon;
> +       uint8_t         data[0];
> +} __attribute__ ((packed));
> +

Im lost here, why do you need this to packed? Usually we only use
packed for network frames which may not be aligned depending on the
architecture, also what is this data[0] for?

>  static GSList *adapters = NULL;
>
>  static void endpoint_request_free(struct endpoint_request *request)
> @@ -966,6 +985,7 @@ static void media_player_free(gpointer data)
>         g_free(mp->path);
>         g_free(mp->status);
>         g_free(mp->name);
> +       g_free(mp->playlists);

You need to free the playlists here, and probable the requests as
well, actually are you running this with valgrind/gdb? By now I would
expect you to have been always testing with valgrind or gdb to make
sure your changes don't cause leaks which means I have to be super
cautions with your patches.

>         g_free(mp);
>  }
>
> @@ -1271,6 +1291,147 @@ static bool previous(void *user_data)
>         return media_player_send(mp, "Previous");
>  }
>
> +static void playlist_reply(DBusPendingCall *call, void *user_data)
> +{
> +       struct player_request *request = user_data;
> +       struct media_player *mp = request->mp;
> +       DBusMessage *reply;
> +       DBusMessageIter array_iter, entry, struct_iter;
> +       DBusError derr;
> +       int ctype;
> +
> +       reply = dbus_pending_call_steal_reply(call);
> +
> +       dbus_error_init(&derr);
> +
> +       if (dbus_set_error_from_message(&derr, reply)) {
> +               error("Error: GetPlayLists method %s, %s", derr.name,
> +                                       derr.message);
> +               dbus_error_free(&derr);
> +               goto done;
> +       }
> +
> +       dbus_message_iter_init(reply, &array_iter);
> +       ctype = dbus_message_iter_get_arg_type(&array_iter);
> +
> +       if (ctype != DBUS_TYPE_ARRAY)
> +               goto done;
> +
> +       dbus_message_iter_recurse(&array_iter, &struct_iter);
> +
> +       while ((ctype = dbus_message_iter_get_arg_type(&struct_iter)) !=
> +                               DBUS_TYPE_INVALID) {
> +               struct media_playlist *playlist;
> +
> +               if (ctype != DBUS_TYPE_STRUCT) {
> +                       error("Invalid type");
> +                       goto done;
> +               }
> +
> +               playlist = g_new0(struct media_playlist, 1);
> +
> +               dbus_message_iter_recurse(&struct_iter, &entry);
> +
> +               dbus_message_iter_get_basic(&entry, &playlist->id);
> +               DBG("Playlists Id %s", playlist->id);
> +
> +               dbus_message_iter_next(&entry);
> +               dbus_message_iter_get_basic(&entry, &playlist->name);
> +               DBG("Playlist Name %s", playlist->name);
> +
> +               dbus_message_iter_next(&entry);
> +               dbus_message_iter_get_basic(&entry, &playlist->icon);
> +               DBG("Playlist Icon %s", playlist->icon);
> +
> +               /* TODO: Create Media folder with playlist information */
> +
> +               mp->playlists = g_slist_append(mp->playlists, playlist);
> +
> +               g_free(playlist);
> +
> +               mp->total_items++;
> +               dbus_message_iter_next(&struct_iter);
> +       }
> +
> +done:
> +       dbus_message_unref(reply);
> +
> +       mp->requests = g_slist_remove(mp->requests, request);
> +
> +       if (request->call)
> +               dbus_pending_call_unref(request->call);
> +
> +       if (request->destroy)
> +               request->destroy(request->user_data);
> +
> +       dbus_message_unref(request->msg);
> +       g_free(request);

It probably make sense to have a free function for the lines above
e.g. player_request_free so that you can reuse when cleaning up.

> +}
> +
> +static gboolean media_player_async_call(DBusMessage *msg,
> +                                       struct media_player *mp,
> +                                       GDestroyNotify destroy)
> +{
> +       struct player_request *request;
> +       const char *method = NULL;
> +
> +       request = g_new0(struct player_request, 1);
> +
> +       if (!(dbus_connection_send_with_reply(btd_get_dbus_connection(),
> +                                       msg, &request->call, -1))) {
> +               error("D-Bus send failed");
> +               g_free(request);
> +               return FALSE;
> +       }
> +
> +       method = dbus_message_get_member(msg);
> +
> +       if (g_strcmp0(method, "GetPlaylists") == 0)
> +               dbus_pending_call_set_notify(request->call,
> +                                               &playlist_reply, request, NULL);

Pass the reply callback in the function parameter like
media_endpoint_async_call is doing.

> +
> +       request->mp = mp;
> +       request->msg = msg;
> +       request->destroy = destroy;
> +
> +       mp->requests = g_slist_append(mp->requests, request);
> +
> +       DBG("Calling %s: name = %s path = %s", dbus_message_get_member(msg),
> +                       dbus_message_get_destination(msg),
> +                       dbus_message_get_path(msg));
> +
> +       return TRUE;
> +}
> +static gboolean get_playlists(struct media_player *mp)
> +{
> +       DBusMessage *msg;
> +       gboolean reverse_order = FALSE;
> +       uint32_t start_index = 0x0000;
> +       uint32_t end_index = 0xFFFF;
> +       char *ordering = "Alphabetical";
> +       uint32_t max_count = (end_index - start_index);
> +
> +       msg = dbus_message_new_method_call(mp->sender, mp->path,
> +                               MEDIA_PLAYER_PLAYLIST_INTERFACE,
> +                               "GetPlaylists");
> +
> +       if (msg == NULL) {
> +               error("Couldn't allocate D-Bus message");
> +               return -ENOMEM;
> +       }
> +
> +       dbus_message_append_args(msg,
> +                       DBUS_TYPE_UINT32, &start_index,
> +                       DBUS_TYPE_UINT32, &max_count,
> +                       DBUS_TYPE_STRING, &ordering,
> +                       DBUS_TYPE_BOOLEAN, &reverse_order,
> +                       DBUS_TYPE_INVALID);
> +
> +       mp->total_items = 0;
> +
> +       return (media_player_async_call(msg, mp, g_free));

Why the extra parenthesis? Im pretty sure checkpatch would complain about this.

> +}
> +
>  static struct avrcp_player_cb player_cb = {
>         .list_settings = list_settings,
>         .get_setting = get_setting,
> @@ -1831,6 +1992,9 @@ static DBusMessage *register_player(DBusConnection *conn, DBusMessage *msg,
>                 return btd_error_invalid_args(msg);
>         }
>
> +       if (!get_playlists(mp))
> +               DBG("Error fetching playlists");
> +
>         return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
>  }
>
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Luiz Augusto von Dentz

RE: [PATCH 2/3] audio/avrcp: Handle PlaylistChanged signal

[Bharat Bhusan Panda]

ping

> -----Original Message-----
> From: linux-bluetooth-owner@vger.kernel.org [mailto:linux-bluetooth-
> owner@vger.kernel.org] On Behalf Of Bharat Panda
> Sent: Tuesday, October 20, 2015 6:46 PM
> To: linux-bluetooth@vger.kernel.org
> Cc: cpgs@samsung.com; Bharat Panda
> Subject: [PATCH 2/3] audio/avrcp: Handle PlaylistChanged signal
> 
> Added signal handler for "PlaylistChanged" on MediaPlayer2.Playlists
> interface.
> ---
>  profiles/audio/media.c | 79
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/profiles/audio/media.c b/profiles/audio/media.c index
> 1b5246d..94fce79 100644
> --- a/profiles/audio/media.c
> +++ b/profiles/audio/media.c
> @@ -113,6 +113,7 @@ struct media_player {
>  	guint			watch;
>  	guint			properties_watch;
>  	guint			seek_watch;
> +	guint			playlist_watch;
>  	char			*status;
>  	uint32_t		position;
>  	uint32_t		duration;
> @@ -973,6 +974,7 @@ static void media_player_free(gpointer data)
>  	g_dbus_remove_watch(conn, mp->watch);
>  	g_dbus_remove_watch(conn, mp->properties_watch);
>  	g_dbus_remove_watch(conn, mp->seek_watch);
> +	g_dbus_remove_watch(conn, mp->playlist_watch);
> 
>  	if (mp->track)
>  		g_hash_table_unref(mp->track);
> @@ -1913,6 +1915,76 @@ static gboolean
> position_changed(DBusConnection *connection, DBusMessage *msg,
>  	return TRUE;
>  }
> 
> +static struct media_playlist *find_playlist_by_id(struct media_player
*mp,
> +						char *playlist_id)
> +{
> +	GSList *l;
> +
> +	for (l = mp->playlists; l; l = l->next) {
> +		struct media_playlist *playlist = l->data;
> +
> +		if (g_strcmp0(playlist->id, playlist_id) == 0) {
> +			return playlist;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +static gboolean playlist_changed(DBusConnection *connection,
> +						DBusMessage *msg,
> +						void *user_data)
> +{
> +	struct media_player *mp = user_data;
> +	struct media_playlist *playlist;
> +	DBusMessageIter iter;
> +	DBusMessageIter entry;
> +	char *playlist_id;
> +	char *name;
> +	char *icon;
> +
> +	dbus_message_iter_init(msg, &iter);
> +	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRUCT)
> +		return FALSE;
> +
> +	dbus_message_iter_recurse(&iter, &entry);
> +
> +	if (dbus_message_iter_get_arg_type(&entry) !=
> +			DBUS_TYPE_OBJECT_PATH)
> +		return FALSE;
> +
> +	dbus_message_iter_get_basic(&entry, &playlist_id);
> +
> +	dbus_message_iter_next(&entry);
> +	if (dbus_message_iter_get_arg_type(&entry) !=
> +			DBUS_TYPE_STRING)
> +		return FALSE;
> +
> +	dbus_message_iter_get_basic(&entry, &name);
> +
> +	dbus_message_iter_next(&entry);
> +	if (dbus_message_iter_get_arg_type(&entry) !=
> +			DBUS_TYPE_STRING)
> +		return FALSE;
> +
> +	dbus_message_iter_get_basic(&entry, &icon);
> +
> +	playlist = find_playlist_by_id(mp, playlist_id);
> +
> +	if (playlist) {
> +		playlist->name = g_strdup(name);
> +		playlist->icon = g_strdup(icon);
> +	} else {
> +		playlist = g_new0(struct media_playlist, 1);
> +
> +		playlist->id = playlist_id;
> +		playlist->name = g_strdup(name);
> +		playlist->icon = g_strdup(icon);
> +	}
> +
> +	return TRUE;
> +}
> +
>  static struct media_player *media_player_create(struct media_adapter
> *adapter,
>  						const char *sender,
>  						const char *path,
> @@ -1995,6 +2067,13 @@ static DBusMessage
> *register_player(DBusConnection *conn, DBusMessage *msg,
>  	if (!get_playlists(mp))
>  		DBG("Error fetching playlists");
> 
> +	mp->playlist_watch = g_dbus_add_signal_watch(conn,
> +					sender, path,
> +
> 	MEDIA_PLAYER_PLAYLIST_INTERFACE,
> +					"PlaylistChanged",
> +					playlist_changed,
> +					mp, NULL);
> +
>  	return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);  }
> 
> --
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth"
in
> the body of a message to majordomo@vger.kernel.org More majordomo
> info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/3] audio/avrcp: Add support for SetBrowsedPlayer

[Bharat Bhusan Panda]

ping

> -----Original Message-----
> From: Bharat Panda [mailto:bharat.panda@samsung.com]
> Sent: Tuesday, October 20, 2015 6:46 PM
> To: linux-bluetooth@vger.kernel.org
> Cc: cpgs@samsung.com; Bharat Panda
> Subject: [PATCH 3/3] audio/avrcp: Add support for SetBrowsedPlayer
> 
> Support added to handle SetBrowsedPlayer command for TG role.
> 
>     AVCTP Browsing: Response: type 0x00 label 13 PID 0x110e
>       AVRCP: SetBrowsedPlayer: len 0x000a
>        Status: 0x04 (Success)
>        UIDCounter: 0x0000 (0)
>        Number of Items: 0x00000007 (7)
>        CharsetID: 0x006a (UTF-8)
>        Folder Depth: 0x00 (0)
> ---
>  profiles/audio/avrcp.c | 63
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  profiles/audio/avrcp.h |  1 +
>  profiles/audio/media.c |  8 +++++++
>  3 files changed, 72 insertions(+)
> 
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c index
> 7b60012..c20fbcb 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -207,6 +207,15 @@ struct player_item {
>  	char name[0];
>  } __attribute__ ((packed));
> 
> +struct avrcp_set_browsed_player_rsp {
> +	uint8_t status;
> +	uint16_t uid_counter;
> +	uint32_t num_of_items;
> +	uint16_t charset_id;
> +	uint8_t folder_depth;
> +	uint8_t data[0];
> +} __attribute__ ((packed));
> +
>  struct avrcp_server {
>  	struct btd_adapter *adapter;
>  	uint32_t tg_record_id;
> @@ -1873,6 +1882,59 @@ err_metadata:
>  	return AVRCP_HEADER_LENGTH + 1;
>  }
> 
> +static void avrcp_handle_set_browsed_player(struct avrcp *session,
> +				struct avrcp_browsing_header *pdu,
> +				uint8_t transaction)
> +{
> +	struct avrcp_player *player;
> +	struct media_player *mp;
> +	struct avrcp_set_browsed_player_rsp *resp;
> +	uint16_t len = ntohs(pdu->param_len);
> +	uint16_t player_id = 0;
> +	uint8_t status;
> +
> +	if (len < 1) {
> +		status = AVRCP_STATUS_INVALID_PARAM;
> +		goto failed;
> +	}
> +
> +	player_id = bt_get_be16(&pdu->params[0]);
> +	player = find_tg_player(session, player_id);
> +
> +	if (!player) {
> +		status = AVRCP_STATUS_NO_AVAILABLE_PLAYERS;
> +		goto failed;
> +	}
> +
> +	if (!(features[7] & 0x08)) {
> +		status = AVRCP_STATUS_PLAYER_NOT_BROWSABLE;
> +		goto failed;
> +	}
> +
> +	mp = player->user_data;
> +	player->browsed = true;
> +
> +	resp = (struct avrcp_set_browsed_player_rsp *) pdu->params;
> +
> +	resp->status = AVRCP_STATUS_SUCCESS;
> +	resp->uid_counter = htons(player_get_uid_counter(player));
> +	resp->num_of_items = htonl(player->cb->get_total_items(mp));
> +
> +	/*
> +	 * MPRIS player returns a flat hierarchy playlist structure,
> +	 * where the folder depth will always considered to be 0.
> +	 */
> +	resp->folder_depth = 0;
> +	resp->charset_id = htons(AVRCP_CHARSET_UTF8);
> +	pdu->param_len = htons(sizeof(*resp));
> +
> +	return;
> +
> +failed:
> +	pdu->params[0] = status;
> +	pdu->param_len = htons(1);
> +}
> +
>  static void avrcp_handle_media_player_list(struct avrcp *session,
>  				struct avrcp_browsing_header *pdu,
>  				uint32_t start_item, uint32_t end_item) @@
> -1989,6 +2051,7 @@ static struct browsing_pdu_handler {
>  							uint8_t
transaction);
>  } browsing_handlers[] = {
>  		{ AVRCP_GET_FOLDER_ITEMS,
> avrcp_handle_get_folder_items },
> +		{ AVRCP_SET_BROWSED_PLAYER,
> avrcp_handle_set_browsed_player},
>  		{ },
>  };
> 
> diff --git a/profiles/audio/avrcp.h b/profiles/audio/avrcp.h index
> 86d310c..c09b910 100644
> --- a/profiles/audio/avrcp.h
> +++ b/profiles/audio/avrcp.h
> @@ -101,6 +101,7 @@ struct avrcp_player_cb {
>  	bool (*pause) (void *user_data);
>  	bool (*next) (void *user_data);
>  	bool (*previous) (void *user_data);
> +	uint32_t (*get_total_items) (void *user_data);
>  };
> 
>  int avrcp_set_volume(struct btd_device *dev, uint8_t volume, bool
notify);
> diff --git a/profiles/audio/media.c b/profiles/audio/media.c index
> 94fce79..a4ff4cc 100644
> --- a/profiles/audio/media.c
> +++ b/profiles/audio/media.c
> @@ -1293,6 +1293,13 @@ static bool previous(void *user_data)
>  	return media_player_send(mp, "Previous");  }
> 
> +static uint32_t get_total_items(void *user_data) {
> +	struct media_player *mp = user_data;
> +
> +	return mp->total_items;
> +}
> +
>  static void playlist_reply(DBusPendingCall *call, void *user_data)  {
>  	struct player_request *request = user_data; @@ -1451,6 +1458,7
> @@ static struct avrcp_player_cb player_cb = {
>  	.pause = pause,
>  	.next = next,
>  	.previous = previous,
> +	.get_total_items = get_total_items,
>  };
> 
>  static void media_player_exit(DBusConnection *connection, void
> *user_data)
> --
> 1.9.1

Re: [PATCH 2/3] audio/avrcp: Handle PlaylistChanged signal

[Luiz Augusto von Dentz]

Hi Bharat,

On Fri, Nov 27, 2015 at 1:21 PM, Bharat Bhusan Panda
<bharat.panda@samsung.com> wrote:
> ping
>
>> -----Original Message-----
>> From: linux-bluetooth-owner@vger.kernel.org [mailto:linux-bluetooth-
>> owner@vger.kernel.org] On Behalf Of Bharat Panda
>> Sent: Tuesday, October 20, 2015 6:46 PM
>> To: linux-bluetooth@vger.kernel.org
>> Cc: cpgs@samsung.com; Bharat Panda
>> Subject: [PATCH 2/3] audio/avrcp: Handle PlaylistChanged signal
>>
>> Added signal handler for "PlaylistChanged" on MediaPlayer2.Playlists
>> interface.
>> ---
>>  profiles/audio/media.c | 79
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 79 insertions(+)
>>
>> diff --git a/profiles/audio/media.c b/profiles/audio/media.c index
>> 1b5246d..94fce79 100644
>> --- a/profiles/audio/media.c
>> +++ b/profiles/audio/media.c
>> @@ -113,6 +113,7 @@ struct media_player {
>>       guint                   watch;
>>       guint                   properties_watch;
>>       guint                   seek_watch;
>> +     guint                   playlist_watch;
>>       char                    *status;
>>       uint32_t                position;
>>       uint32_t                duration;
>> @@ -973,6 +974,7 @@ static void media_player_free(gpointer data)
>>       g_dbus_remove_watch(conn, mp->watch);
>>       g_dbus_remove_watch(conn, mp->properties_watch);
>>       g_dbus_remove_watch(conn, mp->seek_watch);
>> +     g_dbus_remove_watch(conn, mp->playlist_watch);
>>
>>       if (mp->track)
>>               g_hash_table_unref(mp->track);
>> @@ -1913,6 +1915,76 @@ static gboolean
>> position_changed(DBusConnection *connection, DBusMessage *msg,
>>       return TRUE;
>>  }
>>
>> +static struct media_playlist *find_playlist_by_id(struct media_player
> *mp,
>> +                                             char *playlist_id)
>> +{
>> +     GSList *l;
>> +
>> +     for (l = mp->playlists; l; l = l->next) {
>> +             struct media_playlist *playlist = l->data;
>> +
>> +             if (g_strcmp0(playlist->id, playlist_id) == 0) {
>> +                     return playlist;
>> +             }
>> +     }
>> +
>> +     return NULL;
>> +}
>> +
>> +static gboolean playlist_changed(DBusConnection *connection,
>> +                                             DBusMessage *msg,
>> +                                             void *user_data)
>> +{
>> +     struct media_player *mp = user_data;
>> +     struct media_playlist *playlist;
>> +     DBusMessageIter iter;
>> +     DBusMessageIter entry;
>> +     char *playlist_id;
>> +     char *name;
>> +     char *icon;
>> +
>> +     dbus_message_iter_init(msg, &iter);
>> +     if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRUCT)
>> +             return FALSE;
>> +
>> +     dbus_message_iter_recurse(&iter, &entry);
>> +
>> +     if (dbus_message_iter_get_arg_type(&entry) !=
>> +                     DBUS_TYPE_OBJECT_PATH)
>> +             return FALSE;
>> +
>> +     dbus_message_iter_get_basic(&entry, &playlist_id);
>> +
>> +     dbus_message_iter_next(&entry);
>> +     if (dbus_message_iter_get_arg_type(&entry) !=
>> +                     DBUS_TYPE_STRING)
>> +             return FALSE;
>> +
>> +     dbus_message_iter_get_basic(&entry, &name);
>> +
>> +     dbus_message_iter_next(&entry);
>> +     if (dbus_message_iter_get_arg_type(&entry) !=
>> +                     DBUS_TYPE_STRING)
>> +             return FALSE;
>> +
>> +     dbus_message_iter_get_basic(&entry, &icon);
>> +
>> +     playlist = find_playlist_by_id(mp, playlist_id);
>> +
>> +     if (playlist) {
>> +             playlist->name = g_strdup(name);
>> +             playlist->icon = g_strdup(icon);
>> +     } else {
>> +             playlist = g_new0(struct media_playlist, 1);
>> +
>> +             playlist->id = playlist_id;
>> +             playlist->name = g_strdup(name);
>> +             playlist->icon = g_strdup(icon);
>> +     }

Change this to if (!playlist) { new0.. the you can have name and icon
set after that since playlist would be set one way or the other.

>> +     return TRUE;
>> +}
>> +
>>  static struct media_player *media_player_create(struct media_adapter
>> *adapter,
>>                                               const char *sender,
>>                                               const char *path,
>> @@ -1995,6 +2067,13 @@ static DBusMessage
>> *register_player(DBusConnection *conn, DBusMessage *msg,
>>       if (!get_playlists(mp))
>>               DBG("Error fetching playlists");
>>
>> +     mp->playlist_watch = g_dbus_add_signal_watch(conn,
>> +                                     sender, path,
>> +
>>       MEDIA_PLAYER_PLAYLIST_INTERFACE,
>> +                                     "PlaylistChanged",
>> +                                     playlist_changed,
>> +                                     mp, NULL);
>> +
>>       return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);  }
>>
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth"
> in
>> the body of a message to majordomo@vger.kernel.org More majordomo
>> info at  http://vger.kernel.org/majordomo-info.html
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Luiz Augusto von Dentz

Re: [PATCH 3/3] audio/avrcp: Add support for SetBrowsedPlayer

[Luiz Augusto von Dentz]

Hi Bharat,

On Tue, Oct 20, 2015 at 4:16 PM, Bharat Panda <bharat.panda@samsung.com> wrote:
> Support added to handle SetBrowsedPlayer command for TG role.
>
>     AVCTP Browsing: Response: type 0x00 label 13 PID 0x110e
>       AVRCP: SetBrowsedPlayer: len 0x000a
>        Status: 0x04 (Success)
>        UIDCounter: 0x0000 (0)
>        Number of Items: 0x00000007 (7)
>        CharsetID: 0x006a (UTF-8)
>        Folder Depth: 0x00 (0)
> ---

That patch seems good, but now looking at the decoding it seems we got
AVCTP to do the opposite of what other protocols are doing, so it
should be decode value (raw value) not the other way around. If you
have some time could you please try to fix the this?

>  profiles/audio/avrcp.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  profiles/audio/avrcp.h |  1 +
>  profiles/audio/media.c |  8 +++++++
>  3 files changed, 72 insertions(+)
>
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> index 7b60012..c20fbcb 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -207,6 +207,15 @@ struct player_item {
>         char name[0];
>  } __attribute__ ((packed));
>
> +struct avrcp_set_browsed_player_rsp {
> +       uint8_t status;
> +       uint16_t uid_counter;
> +       uint32_t num_of_items;
> +       uint16_t charset_id;
> +       uint8_t folder_depth;
> +       uint8_t data[0];
> +} __attribute__ ((packed));
> +
>  struct avrcp_server {
>         struct btd_adapter *adapter;
>         uint32_t tg_record_id;
> @@ -1873,6 +1882,59 @@ err_metadata:
>         return AVRCP_HEADER_LENGTH + 1;
>  }
>
> +static void avrcp_handle_set_browsed_player(struct avrcp *session,
> +                               struct avrcp_browsing_header *pdu,
> +                               uint8_t transaction)
> +{
> +       struct avrcp_player *player;
> +       struct media_player *mp;
> +       struct avrcp_set_browsed_player_rsp *resp;
> +       uint16_t len = ntohs(pdu->param_len);
> +       uint16_t player_id = 0;
> +       uint8_t status;
> +
> +       if (len < 1) {
> +               status = AVRCP_STATUS_INVALID_PARAM;
> +               goto failed;
> +       }
> +
> +       player_id = bt_get_be16(&pdu->params[0]);
> +       player = find_tg_player(session, player_id);
> +
> +       if (!player) {
> +               status = AVRCP_STATUS_NO_AVAILABLE_PLAYERS;
> +               goto failed;
> +       }
> +
> +       if (!(features[7] & 0x08)) {
> +               status = AVRCP_STATUS_PLAYER_NOT_BROWSABLE;
> +               goto failed;
> +       }
> +
> +       mp = player->user_data;
> +       player->browsed = true;
> +
> +       resp = (struct avrcp_set_browsed_player_rsp *) pdu->params;
> +
> +       resp->status = AVRCP_STATUS_SUCCESS;
> +       resp->uid_counter = htons(player_get_uid_counter(player));
> +       resp->num_of_items = htonl(player->cb->get_total_items(mp));
> +
> +       /*
> +        * MPRIS player returns a flat hierarchy playlist structure,
> +        * where the folder depth will always considered to be 0.
> +        */
> +       resp->folder_depth = 0;
> +       resp->charset_id = htons(AVRCP_CHARSET_UTF8);
> +       pdu->param_len = htons(sizeof(*resp));
> +
> +       return;
> +
> +failed:
> +       pdu->params[0] = status;
> +       pdu->param_len = htons(1);
> +}
> +
>  static void avrcp_handle_media_player_list(struct avrcp *session,
>                                 struct avrcp_browsing_header *pdu,
>                                 uint32_t start_item, uint32_t end_item)
> @@ -1989,6 +2051,7 @@ static struct browsing_pdu_handler {
>                                                         uint8_t transaction);
>  } browsing_handlers[] = {
>                 { AVRCP_GET_FOLDER_ITEMS, avrcp_handle_get_folder_items },
> +               { AVRCP_SET_BROWSED_PLAYER, avrcp_handle_set_browsed_player},
>                 { },
>  };
>
> diff --git a/profiles/audio/avrcp.h b/profiles/audio/avrcp.h
> index 86d310c..c09b910 100644
> --- a/profiles/audio/avrcp.h
> +++ b/profiles/audio/avrcp.h
> @@ -101,6 +101,7 @@ struct avrcp_player_cb {
>         bool (*pause) (void *user_data);
>         bool (*next) (void *user_data);
>         bool (*previous) (void *user_data);
> +       uint32_t (*get_total_items) (void *user_data);
>  };
>
>  int avrcp_set_volume(struct btd_device *dev, uint8_t volume, bool notify);
> diff --git a/profiles/audio/media.c b/profiles/audio/media.c
> index 94fce79..a4ff4cc 100644
> --- a/profiles/audio/media.c
> +++ b/profiles/audio/media.c
> @@ -1293,6 +1293,13 @@ static bool previous(void *user_data)
>         return media_player_send(mp, "Previous");
>  }
>
> +static uint32_t get_total_items(void *user_data)
> +{
> +       struct media_player *mp = user_data;
> +
> +       return mp->total_items;
> +}
> +
>  static void playlist_reply(DBusPendingCall *call, void *user_data)
>  {
>         struct player_request *request = user_data;
> @@ -1451,6 +1458,7 @@ static struct avrcp_player_cb player_cb = {
>         .pause = pause,
>         .next = next,
>         .previous = previous,
> +       .get_total_items = get_total_items,
>  };
>
>  static void media_player_exit(DBusConnection *connection, void *user_data)
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Luiz Augusto von Dentz