From: Archie Pusaka <apusaka@google.com>
To: Howard Chung <howardchung@google.com>
Cc: BlueZ <linux-bluetooth@vger.kernel.org>,
Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
Archie Pusaka <apusaka@chromium.org>
Subject: Re: [bluez PATCH v2] avrcp: include all player settings in notif event
Date: Tue, 4 Aug 2020 16:26:03 +0800 [thread overview]
Message-ID: <CAJQfnxEBjHMAO=kTqRBExgS40g7=OJbv-NOTE1V=PM=K1sdGcw@mail.gmail.com> (raw)
In-Reply-To: <20200708121928.bluez.v2.1.I6076fdf5621a5ce59b7307967a8c997638c1d1c8@changeid>
Hi Bluez maintainers,
Could you take a look at this fix?
Thank you!
Regards,
Archie
On Wed, 8 Jul 2020 at 12:19, 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 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 e2428250e..a4de7530e 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -369,6 +369,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)
> {
> @@ -743,6 +744,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)
> {
> @@ -751,8 +781,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;
> @@ -791,19 +819,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;
> @@ -1595,7 +1611,6 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> struct btd_device *dev = session->dev;
> uint16_t len = ntohs(pdu->params_len);
> uint64_t uid;
> - GList *settings;
>
> /*
> * 1 byte for EventID, 4 bytes for Playback interval but the latest
> @@ -1626,29 +1641,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.27.0.383.g050319c2ae-goog
>
next prev parent reply other threads:[~2020-08-04 8:26 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-08 4:19 [bluez PATCH v2] avrcp: include all player settings in notif event Howard Chung
2020-08-04 8:26 ` Archie Pusaka [this message]
2020-08-04 17:07 ` Luiz Augusto von Dentz
2020-08-04 17:30 ` Archie Pusaka
2020-08-05 0:27 ` Luiz Augusto von Dentz
2020-08-05 0:30 ` Archie Pusaka
2020-08-05 2:09 ` Yun-hao Chung
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAJQfnxEBjHMAO=kTqRBExgS40g7=OJbv-NOTE1V=PM=K1sdGcw@mail.gmail.com' \
--to=apusaka@google.com \
--cc=apusaka@chromium.org \
--cc=howardchung@google.com \
--cc=linux-bluetooth@vger.kernel.org \
--cc=luiz.dentz@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).