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: <20200708121928.bluez.v2.1.I6076fdf5621a5ce59b7307967a8c997638c1d1c8@changeid>
 <CAJQfnxEBjHMAO=kTqRBExgS40g7=OJbv-NOTE1V=PM=K1sdGcw@mail.gmail.com>
 <CABBYNZ+dZsOGQZh6R3wGg3qPUep9tz7BzsDRH4__mbHKcL8P+g@mail.gmail.com>
 <CAJQfnxGvvfSHS8ZdYcBPN6Me8E4r76YkkDubnB71r_8MXsDSsQ@mail.gmail.com>
 <CABBYNZ+7JsBAb3s2m0PFnrcnDdSisFLyYEq6rp96fDNNw5zx8Q@mail.gmail.com> <CAJQfnxGZvrJOj-UDcxMQqZ8Qj9SVT+umHZYyswPRefLPnLDeow@mail.gmail.com>
In-Reply-To: <CAJQfnxGZvrJOj-UDcxMQqZ8Qj9SVT+umHZYyswPRefLPnLDeow@mail.gmail.com>
From:   Yun-hao Chung <howardchung@google.com>
Date:   Wed, 5 Aug 2020 10:09:20 +0800
Message-ID: <CAPHZWUf3Vem96AmmM6BN71=SfCSvUeKfNfd4cbHy+uq=TQOCKg@mail.gmail.com>
Subject: Re: [bluez PATCH v2] avrcp: include all player settings in notif event
To:     Archie Pusaka <apusaka@google.com>
Cc:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
        BlueZ <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

I've rebased and submitted as patch v3, please check.

Thanks,
Howard

On Wed, Aug 5, 2020 at 8:31 AM Archie Pusaka <apusaka@google.com> wrote:
>
> Got it, we will rebase to master and resubmit.
>
> Regards,
> Archie
>
> On Wed, 5 Aug 2020 at 08:28, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Archie,
> >
> > On Tue, Aug 4, 2020 at 10:30 AM Archie Pusaka <apusaka@google.com> wrote:
> > >
> > > Hi Luiz,
> > >
> > > Is this the problem?
> > > https://lore.kernel.org/linux-bluetooth/5f05427c.1c69fb81.f61e.0992@mx.google.com/
> > >
> > > If so, that has been resolved in v2.
> >
> > Found it, but it doesn't seem to apply on top of master:
> >
> > Applying: avrcp: include all player settings in notif event
> > error: patch failed: profiles/audio/avrcp.c:1595
> > error: profiles/audio/avrcp.c: patch does not apply
> > Patch failed at 0001 avrcp: include all player settings in notif event
> >
> > > Regards,
> > > Archie
> > >
> > > On Wed, 5 Aug 2020 at 01:07, Luiz Augusto von Dentz
> > > <luiz.dentz@gmail.com> wrote:
> > > >
> > > > Hi Archie,
> > > >
> > > > On Tue, Aug 4, 2020 at 1:26 AM Archie Pusaka <apusaka@google.com> wrote:
> > > > >
> > > > > Hi Bluez maintainers,
> > > > >
> > > > > Could you take a look at this fix?
> > > > > Thank you!
> > > >
> > > > Has there been any new version? It looks like CI has caught some
> > > > problems with it or that has been resolved by V2?
> > > >
> > > > > 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
> > > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz