Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200805100342.bluez.v3.1.I6076fdf5621a5ce59b7307967a8c997638c1d1c8@changeid>
In-Reply-To: <20200805100342.bluez.v3.1.I6076fdf5621a5ce59b7307967a8c997638c1d1c8@changeid>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Wed, 5 Aug 2020 12:50:41 -0700
Message-ID: <CABBYNZ+=ZjPe147Jr8vEVCRixjFoq9xMYWObOLYHErGF3e5DCg@mail.gmail.com>
Subject: Re: [bluez PATCH v3] avrcp: include all player settings in notif event
To:     Howard Chung <howardchung@google.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Archie Pusaka <apusaka@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

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.

-- 
Luiz Augusto von Dentz