[PATCH BlueZ 1/4] audio/avrcp: Fix possible crash when current player is removed

[PATCH BlueZ 1/4] audio/avrcp: Fix possible crash when current player is removed

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

If current player is removed a new player should be assigned.

Note: In normal condition this should never happen since player 0 works
as a place holder but there have been some cases where addressed player
changed don't match with any player returned by GetFolderList which
cause a new player to be created and the old one to be destroyed.
---
 profiles/audio/avrcp.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 24deac5..a3ed16a 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -3320,10 +3320,15 @@ static void player_remove(gpointer data)
 
 	for (l = player->sessions; l; l = l->next) {
 		struct avrcp *session = l->data;
+		struct avrcp_data *controller = session->controller;
 
-		session->controller->players = g_slist_remove(
-						session->controller->players,
-						player);
+		controller->players = g_slist_remove(controller->players,
+								player);
+
+		/* Check if current player is being removed */
+		if (controller->player == player)
+			controller->player = g_slist_nth_data(
+							controller->players, 0);
 	}
 
 	player_destroy(player);
@@ -3374,9 +3379,6 @@ static gboolean avrcp_get_media_player_list_rsp(struct avctp *conn,
 		i += len;
 	}
 
-	if (g_slist_find(removed, session->controller->player))
-		session->controller->player = NULL;
-
 	g_slist_free_full(removed, player_remove);
 
 	return FALSE;
-- 
2.4.3

[PATCH BlueZ 2/4] doc/media-api: Add Player property to MediaControl1

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Player property is use to inform the current addressed player which is
necessary in case more than one player is available.

This is also remove the deprecated status of MediaControl1 and instead
deprecated just its methods which similar functionality have been moved
to MediaPlayer1.
---
 doc/media-api.txt | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/doc/media-api.txt b/doc/media-api.txt
index b17a151..b5ad2db 100644
--- a/doc/media-api.txt
+++ b/doc/media-api.txt
@@ -71,43 +71,43 @@ Media Control hierarchy
 =======================
 
 Service		org.bluez
-Interface	org.bluez.MediaControl1 [Deprecated]
+Interface	org.bluez.MediaControl1
 Object path	[variable prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX
 
-Methods		void Play()
+Methods		void Play() [Deprecated]
 
 			Resume playback.
 
-		void Pause()
+		void Pause() [Deprecated]
 
 			Pause playback.
 
-		void Stop()
+		void Stop() [Deprecated]
 
 			Stop playback.
 
-		void Next()
+		void Next() [Deprecated]
 
 			Next item.
 
-		void Previous()
+		void Previous() [Deprecated]
 
 			Previous item.
 
-		void VolumeUp()
+		void VolumeUp() [Deprecated]
 
 			Adjust remote volume one step up
 
-		void VolumeDown()
+		void VolumeDown() [Deprecated]
 
 			Adjust remote volume one step down
 
-		void FastForward()
+		void FastForward() [Deprecated]
 
 			Fast forward playback, this action is only stopped
 			when another method in this interface is called.
 
-		void Rewind()
+		void Rewind() [Deprecated]
 
 			Rewind playback, this action is only stopped
 			when another method in this interface is called.
@@ -116,6 +116,10 @@ Properties
 
 		boolean Connected [readonly]
 
+		object Player [readonly, optional]
+
+			Addressed Player object path.
+
 
 MediaPlayer1 hierarchy
 ======================
-- 
2.4.3

[PATCH BlueZ 3/4] audio/control: Mark all methods of MediaControl1 as deprecated

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

MediaControl1 is deprecated according to its documentation.
---
 profiles/audio/control.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/profiles/audio/control.c b/profiles/audio/control.c
index f4656d8..d109d1f 100644
--- a/profiles/audio/control.c
+++ b/profiles/audio/control.c
@@ -216,15 +216,17 @@ static gboolean control_property_get_connected(
 }
 
 static const GDBusMethodTable control_methods[] = {
-	{ GDBUS_METHOD("Play", NULL, NULL, control_play) },
-	{ GDBUS_METHOD("Pause", NULL, NULL, control_pause) },
-	{ GDBUS_METHOD("Stop", NULL, NULL, control_stop) },
-	{ GDBUS_METHOD("Next", NULL, NULL, control_next) },
-	{ GDBUS_METHOD("Previous", NULL, NULL, control_previous) },
-	{ GDBUS_METHOD("VolumeUp", NULL, NULL, control_volume_up) },
-	{ GDBUS_METHOD("VolumeDown", NULL, NULL, control_volume_down) },
-	{ GDBUS_METHOD("FastForward", NULL, NULL, control_fast_forward) },
-	{ GDBUS_METHOD("Rewind", NULL, NULL, control_rewind) },
+	{ GDBUS_DEPRECATED_METHOD("Play", NULL, NULL, control_play) },
+	{ GDBUS_DEPRECATED_METHOD("Pause", NULL, NULL, control_pause) },
+	{ GDBUS_DEPRECATED_METHOD("Stop", NULL, NULL, control_stop) },
+	{ GDBUS_DEPRECATED_METHOD("Next", NULL, NULL, control_next) },
+	{ GDBUS_DEPRECATED_METHOD("Previous", NULL, NULL, control_previous) },
+	{ GDBUS_DEPRECATED_METHOD("VolumeUp", NULL, NULL, control_volume_up) },
+	{ GDBUS_DEPRECATED_METHOD("VolumeDown", NULL, NULL,
+							control_volume_down) },
+	{ GDBUS_DEPRECATED_METHOD("FastForward", NULL, NULL,
+							control_fast_forward) },
+	{ GDBUS_DEPRECATED_METHOD("Rewind", NULL, NULL, control_rewind) },
 	{ }
 };
 
-- 
2.4.3

[PATCH BlueZ 4/4] audio/control: Add Player property to MediaControl1

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This adds Player property to MediaControl1 interface which contains the
object path of the addressed player.
---
 profiles/audio/avrcp.c   | 17 +++++++++++++----
 profiles/audio/control.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++
 profiles/audio/control.h |  2 ++
 profiles/audio/player.c  |  5 +++++
 profiles/audio/player.h  |  1 +
 5 files changed, 70 insertions(+), 4 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index a3ed16a..44f8929 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -3191,6 +3191,15 @@ static const struct media_player_callback ct_cbs = {
 	.total_items = ct_get_total_numberofitems,
 };
 
+static void set_ct_player(struct avrcp *session, struct avrcp_player *player)
+{
+	struct btd_service *service;
+
+	session->controller->player = player;
+	service = btd_device_get_service(session->dev, AVRCP_TARGET_UUID);
+	control_set_player(service, media_player_get_path(player->user_data));
+}
+
 static struct avrcp_player *create_ct_player(struct avrcp *session,
 								uint16_t id)
 {
@@ -3212,7 +3221,7 @@ static struct avrcp_player *create_ct_player(struct avrcp *session,
 	player->destroy = (GDestroyNotify) media_player_destroy;
 
 	if (session->controller->player == NULL)
-		session->controller->player = player;
+		set_ct_player(session, player);
 
 	session->controller->players = g_slist_prepend(
 						session->controller->players,
@@ -3327,8 +3336,8 @@ static void player_remove(gpointer data)
 
 		/* Check if current player is being removed */
 		if (controller->player == player)
-			controller->player = g_slist_nth_data(
-							controller->players, 0);
+			set_ct_player(session, g_slist_nth_data(
+						controller->players, 0));
 	}
 
 	player_destroy(player);
@@ -3503,7 +3512,7 @@ static void avrcp_addressed_player_changed(struct avrcp *session,
 	}
 
 	player->uid_counter = get_be16(&pdu->params[3]);
-	session->controller->player = player;
+	set_ct_player(session, player);
 
 	if (player->features != NULL)
 		return;
diff --git a/profiles/audio/control.c b/profiles/audio/control.c
index d109d1f..5c48882 100644
--- a/profiles/audio/control.c
+++ b/profiles/audio/control.c
@@ -59,6 +59,7 @@
 
 #include "avctp.h"
 #include "control.h"
+#include "player.h"
 
 static GSList *devices = NULL;
 
@@ -68,6 +69,8 @@ struct control {
 	struct btd_service *target;
 	struct btd_service *remote;
 	unsigned int avctp_id;
+	const char *player;
+	GSList *players;
 };
 
 static void state_changed(struct btd_device *dev, avctp_state_t old_state,
@@ -81,9 +84,12 @@ static void state_changed(struct btd_device *dev, avctp_state_t old_state,
 	switch (new_state) {
 	case AVCTP_STATE_DISCONNECTED:
 		control->session = NULL;
+		control->player = NULL;
 
 		g_dbus_emit_property_changed(conn, path,
 					AUDIO_CONTROL_INTERFACE, "Connected");
+		g_dbus_emit_property_changed(conn, path,
+					AUDIO_CONTROL_INTERFACE, "Player");
 
 		break;
 	case AVCTP_STATE_CONNECTING:
@@ -215,6 +221,28 @@ static gboolean control_property_get_connected(
 	return TRUE;
 }
 
+static gboolean control_player_exists(const GDBusPropertyTable *property,
+								void *data)
+{
+	struct control *control = data;
+
+	return control->player != NULL;
+}
+
+static gboolean control_get_player(const GDBusPropertyTable *property,
+					DBusMessageIter *iter, void *data)
+{
+	struct control *control = data;
+
+	if (!control->player)
+		return FALSE;
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_OBJECT_PATH,
+							&control->player);
+
+	return TRUE;
+}
+
 static const GDBusMethodTable control_methods[] = {
 	{ GDBUS_DEPRECATED_METHOD("Play", NULL, NULL, control_play) },
 	{ GDBUS_DEPRECATED_METHOD("Pause", NULL, NULL, control_pause) },
@@ -232,6 +260,7 @@ static const GDBusMethodTable control_methods[] = {
 
 static const GDBusPropertyTable control_properties[] = {
 	{ "Connected", "b", control_property_get_connected },
+	{ "Player", "o", control_get_player, NULL, control_player_exists },
 	{ }
 };
 
@@ -254,6 +283,7 @@ static void path_unregister(void *data)
 		btd_service_unref(control->remote);
 
 	devices = g_slist_remove(devices, control);
+	g_slist_free(control->players);
 	g_free(control);
 }
 
@@ -340,3 +370,22 @@ int control_init_remote(struct btd_service *service)
 
 	return 0;
 }
+
+int control_set_player(struct btd_service *service, const char *path)
+{
+	struct control *control = btd_service_get_user_data(service);
+
+	if (!control->session)
+		return -ENOTCONN;
+
+	if (g_strcmp0(control->player, path) == 0)
+		return -EALREADY;
+
+	control->player = path;
+
+	g_dbus_emit_property_changed(btd_get_dbus_connection(),
+					device_get_path(control->dev),
+					AUDIO_CONTROL_INTERFACE, "Player");
+
+	return 0;
+}
diff --git a/profiles/audio/control.h b/profiles/audio/control.h
index 4bda896..aab2631 100644
--- a/profiles/audio/control.h
+++ b/profiles/audio/control.h
@@ -32,3 +32,5 @@ void control_unregister(struct btd_service *service);
 
 int control_connect(struct btd_service *service);
 int control_disconnect(struct btd_service *service);
+
+int control_set_player(struct btd_service *service, const char *path);
diff --git a/profiles/audio/player.c b/profiles/audio/player.c
index 147bcbf..4736396 100644
--- a/profiles/audio/player.c
+++ b/profiles/audio/player.c
@@ -1193,6 +1193,11 @@ struct media_player *media_player_controller_create(const char *path,
 	return mp;
 }
 
+const char *media_player_get_path(struct media_player *mp)
+{
+	return mp->path;
+}
+
 void media_player_set_duration(struct media_player *mp, uint32_t duration)
 {
 	char *value, *curval;
diff --git a/profiles/audio/player.h b/profiles/audio/player.h
index 0871904..4ad8bfe 100644
--- a/profiles/audio/player.h
+++ b/profiles/audio/player.h
@@ -70,6 +70,7 @@ struct media_player_callback {
 
 struct media_player *media_player_controller_create(const char *path,
 								uint16_t id);
+const char *media_player_get_path(struct media_player *mp);
 void media_player_destroy(struct media_player *mp);
 void media_player_set_duration(struct media_player *mp, uint32_t duration);
 void media_player_set_position(struct media_player *mp, uint32_t position);
-- 
2.4.3

Re: [PATCH BlueZ 1/4] audio/avrcp: Fix possible crash when current player is removed

[Luiz Augusto von Dentz]

Hi,

On Tue, Sep 29, 2015 at 4:45 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> If current player is removed a new player should be assigned.
>
> Note: In normal condition this should never happen since player 0 works
> as a place holder but there have been some cases where addressed player
> changed don't match with any player returned by GetFolderList which
> cause a new player to be created and the old one to be destroyed.
> ---
>  profiles/audio/avrcp.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> index 24deac5..a3ed16a 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -3320,10 +3320,15 @@ static void player_remove(gpointer data)
>
>         for (l = player->sessions; l; l = l->next) {
>                 struct avrcp *session = l->data;
> +               struct avrcp_data *controller = session->controller;
>
> -               session->controller->players = g_slist_remove(
> -                                               session->controller->players,
> -                                               player);
> +               controller->players = g_slist_remove(controller->players,
> +                                                               player);
> +
> +               /* Check if current player is being removed */
> +               if (controller->player == player)
> +                       controller->player = g_slist_nth_data(
> +                                                       controller->players, 0);
>         }
>
>         player_destroy(player);
> @@ -3374,9 +3379,6 @@ static gboolean avrcp_get_media_player_list_rsp(struct avctp *conn,
>                 i += len;
>         }
>
> -       if (g_slist_find(removed, session->controller->player))
> -               session->controller->player = NULL;
> -
>         g_slist_free_full(removed, player_remove);
>
>         return FALSE;
> --
> 2.4.3

Applied.


-- 
Luiz Augusto von Dentz