[PATCH BlueZ 1/3] audio/media: Fix notifying settings changed incorrectly

[PATCH BlueZ 1/3] audio/media: Fix notifying settings changed incorrectly

[Luiz Augusto von Dentz]

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

We should notify only the setting that has changed not all of them.
---
 profiles/audio/avrcp.c | 30 +++++++++++++-----------------
 profiles/audio/avrcp.h |  3 ++-
 profiles/audio/media.c |  7 +------
 3 files changed, 16 insertions(+), 24 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 730f061..f0554fe 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -612,13 +612,15 @@ static int play_status_to_val(const char *status)
 	return -EINVAL;
 }
 
-void avrcp_player_event(struct avrcp_player *player, uint8_t id, void *data)
+void avrcp_player_event(struct avrcp_player *player, uint8_t id,
+							const void *data)
 {
 	uint8_t buf[AVRCP_HEADER_LENGTH + 9];
 	struct avrcp_header *pdu = (void *) buf;
 	uint16_t size;
 	GSList *l;
-	GList *settings;
+	int attr;
+	int val;
 
 	if (player->sessions == NULL)
 		return;
@@ -649,24 +651,18 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id, void *data)
 		break;
 	case AVRCP_EVENT_SETTINGS_CHANGED:
 		size = 2;
-		settings = data;
-		pdu->params[1] = g_list_length(settings);
-		for (; settings; settings = settings->next) {
-			const char *key = settings->data;
-			int attr;
-			int val;
+		pdu->params[1] = 1;
 
-			attr = attr_to_val(key);
-			if (attr < 0)
-				continue;
+		attr = attr_to_val(data);
+		if (attr < 0)
+			return;
 
-			val = player_get_setting(player, attr);
-			if (val < 0)
-				continue;
+		val = player_get_setting(player, attr);
+		if (val < 0)
+			return;
 
-			pdu->params[++size] = attr;
-			pdu->params[++size] = val;
-		}
+		pdu->params[++size] = attr;
+		pdu->params[++size] = val;
 		break;
 	default:
 		error("Unknown event %u", id);
diff --git a/profiles/audio/avrcp.h b/profiles/audio/avrcp.h
index 3b1963f..6a435e6 100644
--- a/profiles/audio/avrcp.h
+++ b/profiles/audio/avrcp.h
@@ -115,7 +115,8 @@ struct avrcp_player *avrcp_register_player(struct btd_adapter *adapter,
 						GDestroyNotify destroy);
 void avrcp_unregister_player(struct avrcp_player *player);
 
-void avrcp_player_event(struct avrcp_player *player, uint8_t id, void *data);
+void avrcp_player_event(struct avrcp_player *player, uint8_t id,
+							const void *data);
 
 
 size_t avrcp_handle_vendor_reject(uint8_t *code, uint8_t *operands);
diff --git a/profiles/audio/media.c b/profiles/audio/media.c
index eb5ea81..45dfe53 100644
--- a/profiles/audio/media.c
+++ b/profiles/audio/media.c
@@ -1470,7 +1470,6 @@ static gboolean set_property(struct media_player *mp, const char *key,
 							const char *value)
 {
 	const char *curval;
-	GList *settings;
 
 	curval = g_hash_table_lookup(mp->settings, key);
 	if (g_strcmp0(curval, value) == 0)
@@ -1480,11 +1479,7 @@ static gboolean set_property(struct media_player *mp, const char *key,
 
 	g_hash_table_replace(mp->settings, g_strdup(key), g_strdup(value));
 
-	settings = list_settings(mp);
-
-	avrcp_player_event(mp->player, AVRCP_EVENT_SETTINGS_CHANGED, settings);
-
-	g_list_free(settings);
+	avrcp_player_event(mp->player, AVRCP_EVENT_SETTINGS_CHANGED, key);
 
 	return TRUE;
 }
-- 
1.8.1.4

[PATCH BlueZ 2/3] audio/AVRCP: Fix invalid response to RegisterNotification

[Luiz Augusto von Dentz]

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

The response to RegisterNotification for event settings changed was
not setting the initial length properly which cause the code to send
malformed/invalid PDUs.
---
 profiles/audio/avrcp.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index f0554fe..f028da9 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -1450,20 +1450,25 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
 		len = 1;
 		break;
 	case AVRCP_EVENT_SETTINGS_CHANGED:
+		len = 1;
 		settings = player_list_settings(player);
 
-		pdu->params[++len] = g_list_length(settings);
+		pdu->params[len++] = g_list_length(settings);
 		for (; settings; settings = settings->next) {
 			const char *key = settings->data;
-			uint8_t attr = attr_to_val(key);
+			int attr;
 			int val;
 
+			attr = attr_to_val(key);
+			if (attr < 0)
+				continue;
+
 			val = player_get_setting(player, attr);
 			if (val < 0)
 				continue;
 
-			pdu->params[++len] = attr;
-			pdu->params[++len] = val;
+			pdu->params[len++] = attr;
+			pdu->params[len++] = val;
 		}
 
 		break;
-- 
1.8.1.4

[PATCH BlueZ 3/3] audio/media: Fix setting player settings

[Luiz Augusto von Dentz]

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

The value has to be converted to MPRIS setting otherwise the player won't
recognize it and will probably discard the change.
---
 profiles/audio/avrcp.c |  4 ++--
 profiles/audio/media.c | 54 +++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 49 insertions(+), 9 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index f028da9..ffc6415 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -661,8 +661,8 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id,
 		if (val < 0)
 			return;
 
-		pdu->params[++size] = attr;
-		pdu->params[++size] = val;
+		pdu->params[size++] = attr;
+		pdu->params[size++] = val;
 		break;
 	default:
 		error("Unknown event %u", id);
diff --git a/profiles/audio/media.c b/profiles/audio/media.c
index 45dfe53..d4d82cf 100644
--- a/profiles/audio/media.c
+++ b/profiles/audio/media.c
@@ -1007,12 +1007,54 @@ static const char *get_setting(const char *key, void *user_data)
 	return g_hash_table_lookup(mp->settings, key);
 }
 
+static void set_shuffle_setting(DBusMessageIter *iter, const char *value)
+{
+	const char *key = "Shuffle";
+	dbus_bool_t val;
+	DBusMessageIter var;
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING, &key);
+	dbus_message_iter_open_container(iter, DBUS_TYPE_VARIANT,
+						DBUS_TYPE_BOOLEAN_AS_STRING,
+						&var);
+	val = strcasecmp(value, "off") != 0;
+	dbus_message_iter_append_basic(&var, DBUS_TYPE_BOOLEAN, &val);
+	dbus_message_iter_close_container(iter, &var);
+}
+
+static const char *repeat_to_loop_status(const char *value)
+{
+	if (strcasecmp(value, "off") == 0)
+		return "None";
+	else if (strcasecmp(value, "singletrack") == 0)
+		return "Track";
+	else if (strcasecmp(value, "alltracks") == 0)
+		return "Playlist";
+
+	return NULL;
+}
+
+static void set_repeat_setting(DBusMessageIter *iter, const char *value)
+{
+	const char *key = "LoopStatus";
+	const char *val;
+	DBusMessageIter var;
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING, &key);
+	dbus_message_iter_open_container(iter, DBUS_TYPE_VARIANT,
+						DBUS_TYPE_STRING_AS_STRING,
+						&var);
+	val = repeat_to_loop_status(value);
+	dbus_message_iter_append_basic(&var, DBUS_TYPE_STRING, &val);
+	dbus_message_iter_close_container(iter, &var);
+}
+
 static int set_setting(const char *key, const char *value, void *user_data)
 {
 	struct media_player *mp = user_data;
 	const char *iface = MEDIA_PLAYER_INTERFACE;
 	DBusMessage *msg;
-	DBusMessageIter iter, var;
+	DBusMessageIter iter;
 
 	DBG("%s = %s", key, value);
 
@@ -1028,13 +1070,11 @@ static int set_setting(const char *key, const char *value, void *user_data)
 
 	dbus_message_iter_init_append(msg, &iter);
 	dbus_message_iter_append_basic(&iter, DBUS_TYPE_STRING, &iface);
-	dbus_message_iter_append_basic(&iter, DBUS_TYPE_STRING, &key);
 
-	dbus_message_iter_open_container(&iter, DBUS_TYPE_VARIANT,
-						DBUS_TYPE_STRING_AS_STRING,
-						&var);
-	dbus_message_iter_append_basic(&var, DBUS_TYPE_STRING, &value);
-	dbus_message_iter_close_container(&iter, &var);
+	if (strcasecmp(key, "Shuffle") == 0)
+		set_shuffle_setting(&iter, value);
+	else if (strcasecmp(key, "Repeat") == 0)
+		set_repeat_setting(&iter, value);
 
 	g_dbus_send_message(btd_get_dbus_connection(), msg);
 
-- 
1.8.1.4

Re: [PATCH BlueZ 1/3] audio/media: Fix notifying settings changed incorrectly

[Johan Hedberg]

Hi Luiz,

On Tue, Jun 18, 2013, Luiz Augusto von Dentz wrote:
> We should notify only the setting that has changed not all of them.
> ---
>  profiles/audio/avrcp.c | 30 +++++++++++++-----------------
>  profiles/audio/avrcp.h |  3 ++-
>  profiles/audio/media.c |  7 +------
>  3 files changed, 16 insertions(+), 24 deletions(-)

All three patches have been applied. Thanks.

Johan

[PATCH BlueZ 1/3] audio/media: Fix notifying settings changed incorrectly

[Luiz Augusto von Dentz]

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

We should notify only the setting that has changed not all of them.
---
 profiles/audio/media.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/profiles/audio/media.c b/profiles/audio/media.c
index eb5ea81..69139a7 100644
--- a/profiles/audio/media.c
+++ b/profiles/audio/media.c
@@ -1480,7 +1480,7 @@ static gboolean set_property(struct media_player *mp, const char *key,
 
 	g_hash_table_replace(mp->settings, g_strdup(key), g_strdup(value));
 
-	settings = list_settings(mp);
+	settings = g_list_prepend(NULL, (char *) key);
 
 	avrcp_player_event(mp->player, AVRCP_EVENT_SETTINGS_CHANGED, settings);
 
-- 
1.8.1.4

Re: [PATCH BlueZ 1/3] audio/media: Fix notifying settings changed incorrectly

[Marcel Holtmann]

Hi Luiz,

> We should notify only the setting that has changed not all of them.
> ---
> profiles/audio/media.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/profiles/audio/media.c b/profiles/audio/media.c
> index eb5ea81..69139a7 100644
> --- a/profiles/audio/media.c
> +++ b/profiles/audio/media.c
> @@ -1480,7 +1480,7 @@ static gboolean set_property(struct media_player *mp, const char *key,
> 
> 	g_hash_table_replace(mp->settings, g_strdup(key), g_strdup(value));
> 
> -	settings = list_settings(mp);
> +	settings = g_list_prepend(NULL, (char *) key);

this is dangerous. Can you guarantee that the memory it points to is not used outside of this function. Seriously, making a comment here if this is safe is a good idea. If it is not safe, then it needs to be fixed.

We are using const function arguments for pointers to clearly indicate that the lifetime of that pointer is really only guaranteed inside the function.

Regards

Marcel