diff mbox series

[bluez,v3] avrcp: include all player settings in notif event

Message ID 20200805100342.bluez.v3.1.I6076fdf5621a5ce59b7307967a8c997638c1d1c8@changeid (mailing list archive)
State New, archived
Headers show
Series [bluez,v3] avrcp: include all player settings in notif event | expand

Commit Message

Howard Chung Aug. 5, 2020, 2:05 a.m. UTC 
According to AVRCP 1.6.2 spec section 6.7.2 table 6.39, all player
application settings should be returned to the CT and let CT to
determine which settings have changed. Currently bluez only returns
the changed attribute instead. This patch also addresses a potential
issue on which the number of application settings mismatches with
the actual number returned.

Reviewed-by: Archie Pusaka <apusaka@chromium.org>
---

Changes in v3:
- Rebase to master

Changes in v2:
- Fixed unused variables

 profiles/audio/avrcp.c | 71 +++++++++++++++++++-----------------------
 1 file changed, 32 insertions(+), 39 deletions(-)

Comments

Luiz Augusto von Dentz Aug. 5, 2020, 7:50 p.m. UTC | #1 
Hi Howard,

On Tue, Aug 4, 2020 at 7:06 PM Howard Chung <howardchung@google.com> wrote:
>
> According to AVRCP 1.6.2 spec section 6.7.2 table 6.39, all player
> application settings should be returned to the CT and let CT to
> determine which settings have changed. Currently bluez only returns
> the changed attribute instead. This patch also addresses a potential
> issue on which the number of application settings mismatches with
> the actual number returned.
>
> Reviewed-by: Archie Pusaka <apusaka@chromium.org>
> ---
>
> Changes in v3:
> - Rebase to master
>
> Changes in v2:
> - Fixed unused variables
>
>  profiles/audio/avrcp.c | 71 +++++++++++++++++++-----------------------
>  1 file changed, 32 insertions(+), 39 deletions(-)
>
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> index 4e7ff75c0..5d0256c52 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -370,6 +370,7 @@ static uint32_t company_ids[] = {
>  };
>
>  static void avrcp_register_notification(struct avrcp *session, uint8_t event);
> +static GList *player_list_settings(struct avrcp_player *player);
>
>  static sdp_record_t *avrcp_ct_record(void)
>  {
> @@ -744,6 +745,35 @@ static int play_status_to_val(const char *status)
>         return -EINVAL;
>  }
>
> +static uint16_t player_settings_changed(struct avrcp_player *player,
> +                                               struct avrcp_header *pdu)
> +{
> +       GList *settings = player_list_settings(player);
> +       int size = 2;
> +
> +       for (; settings; settings = settings->next) {
> +               const char *key = settings->data;
> +               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[size++] = attr;
> +               pdu->params[size++] = val;
> +       }
> +
> +       g_list_free(settings);
> +
> +       pdu->params[1] = (size - 2) >> 1;
> +       return size;
> +}
> +
>  void avrcp_player_event(struct avrcp_player *player, uint8_t id,
>                                                         const void *data)
>  {
> @@ -752,8 +782,6 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id,
>         uint8_t code;
>         uint16_t size;
>         GSList *l;
> -       int attr;
> -       int val;
>
>         if (player->sessions == NULL)
>                 return;
> @@ -792,19 +820,7 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id,
>                 size = 1;
>                 break;
>         case AVRCP_EVENT_SETTINGS_CHANGED:
> -               size = 2;
> -               pdu->params[1] = 1;
> -
> -               attr = attr_to_val(data);
> -               if (attr < 0)
> -                       return;
> -
> -               val = player_get_setting(player, attr);
> -               if (val < 0)
> -                       return;
> -
> -               pdu->params[size++] = attr;
> -               pdu->params[size++] = val;
> +               size = player_settings_changed(player, pdu);
>                 break;
>         case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
>                 size = 5;
> @@ -1597,7 +1613,6 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
>         uint16_t len = ntohs(pdu->params_len);
>         uint64_t uid;
>         int8_t volume;
> -       GList *settings;
>
>         /*
>          * 1 byte for EventID, 4 bytes for Playback interval but the latest
> @@ -1628,29 +1643,7 @@ 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);
> -               for (; settings; settings = settings->next) {
> -                       const char *key = settings->data;
> -                       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;
> -               }
> -
> -               g_list_free(settings);
> -
> +               len = player_settings_changed(player, pdu);
>                 break;
>         case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
>                 len = 5;
> --
> 2.28.0.163.g6104cc2f0b6-goog

Applied, thanks.
diff mbox series

Patch

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 4e7ff75c0..5d0256c52 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -370,6 +370,7 @@  static uint32_t company_ids[] = {
 };
 
 static void avrcp_register_notification(struct avrcp *session, uint8_t event);
+static GList *player_list_settings(struct avrcp_player *player);
 
 static sdp_record_t *avrcp_ct_record(void)
 {
@@ -744,6 +745,35 @@  static int play_status_to_val(const char *status)
 	return -EINVAL;
 }
 
+static uint16_t player_settings_changed(struct avrcp_player *player,
+						struct avrcp_header *pdu)
+{
+	GList *settings = player_list_settings(player);
+	int size = 2;
+
+	for (; settings; settings = settings->next) {
+		const char *key = settings->data;
+		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[size++] = attr;
+		pdu->params[size++] = val;
+	}
+
+	g_list_free(settings);
+
+	pdu->params[1] = (size - 2) >> 1;
+	return size;
+}
+
 void avrcp_player_event(struct avrcp_player *player, uint8_t id,
 							const void *data)
 {
@@ -752,8 +782,6 @@  void avrcp_player_event(struct avrcp_player *player, uint8_t id,
 	uint8_t code;
 	uint16_t size;
 	GSList *l;
-	int attr;
-	int val;
 
 	if (player->sessions == NULL)
 		return;
@@ -792,19 +820,7 @@  void avrcp_player_event(struct avrcp_player *player, uint8_t id,
 		size = 1;
 		break;
 	case AVRCP_EVENT_SETTINGS_CHANGED:
-		size = 2;
-		pdu->params[1] = 1;
-
-		attr = attr_to_val(data);
-		if (attr < 0)
-			return;
-
-		val = player_get_setting(player, attr);
-		if (val < 0)
-			return;
-
-		pdu->params[size++] = attr;
-		pdu->params[size++] = val;
+		size = player_settings_changed(player, pdu);
 		break;
 	case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
 		size = 5;
@@ -1597,7 +1613,6 @@  static uint8_t avrcp_handle_register_notification(struct avrcp *session,
 	uint16_t len = ntohs(pdu->params_len);
 	uint64_t uid;
 	int8_t volume;
-	GList *settings;
 
 	/*
 	 * 1 byte for EventID, 4 bytes for Playback interval but the latest
@@ -1628,29 +1643,7 @@  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);
-		for (; settings; settings = settings->next) {
-			const char *key = settings->data;
-			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;
-		}
-
-		g_list_free(settings);
-
+		len = player_settings_changed(player, pdu);
 		break;
 	case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
 		len = 5;