Re: [PATCH BlueZ] audio/avrcp: Always update transport volume regardless of player

[Luiz Augusto von Dentz <luiz.dentz@gmail.com>]

Hi Marijn,

On Mon, Jul 13, 2020 at 5:05 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Marijn,
>
> On Sat, Jul 11, 2020 at 4:50 AM Marijn Suijten <marijns95@gmail.com> wrote:
> >
> > `Volume` is a special property that not only exists on players but also
> > on the transport (see org.bluez.MediaTransport1). A player is not
> > attached when the controller does not support FEATURE_CATEGORY_1, which
> > is common on headphones without media browsing capabilities.
> >
> > On such audio devices (headphones, in-ears and the like) Absolute Volume
> > is not available unless an external player is registered
> > (org.bluez.Media1.RegisterPlayer) and the device sends a volume event
> > back after that to set a2dp->volume in transport.c to a valid value
> > (causing volume_exists to finally return true).
> >
> > This [1] mail thread denoting the same issue has a solution to at least
> > request capabilities from the controller, but the proposed player object
> > is not created on category 2 devices. Any notifications received on
> > AVRCP_EVENT_VOLUME_CHANGED (avrcp_volume_changed) that is subsequently
> > registered, or handling the result of avrcp_set_volume in
> > avrcp_handle_set_volume will be ignored unless said player is present.
> >
> > This issue is not addressed by adding a fake player but instead dealing
> > with the fact that volume is "special" and available on the transport
> > regardless of the existence of a player. This is confirmed in
> > avrcp_get_capabilities_resp as well which requires a player to register
> > any event except AVRCP_EVENT_VOLUME_CHANGED.
> >
> > The applied solution moves media_transport_update_device_volume out of
> > the player and into avrcp_volume_changed/avrcp_handle_set_volume where
> > it is unconditionally called. These functions are the only users of
> > avrcp_player->set_volume.
> >
> > Note that the volume member of media_player is never used which seems a
> > result of updating from org.bluez.MediaPlayer1 to
> > org.mpris.MediaPlayer2.Player in
> > 15e421737ccc4696ed567edcc24d178aedb47854, where the volume property [2]
> > is left out.
>
> This is actually on purpose since the volume notification indicates
> the volume at the device side we don't want to change the player
> volume as well as it would most likely result in duplicating the
> volume change both at sink and source, note that the likes of handling
> the transport like pulseaudio would already notify the device volume
> level change without actually applying any change to the volume
> locally.
>
> > [1]: https://marc.info/?l=linux-bluetooth&m=145337574806153
> > [2]: https://specifications.freedesktop.org/mpris-spec/2.2/Player_Interface.html#Property:Volume
> >
> > ---
> > Hi,
> >
> > This is is a resend from an earlier mail that didn't comply with the
> > contributor guidelines. Seeing that the topic of AVRCP volume is brought
> > up recently it is about time to repair it and hereby send it again.
> >
> > I still have an incomplete patch lying around that synchronizes Volume
> > on org.mpris.MediaPlayer2.Player back when this patch was written 6
> > months ago. It'll require some time to get back in to it and finalize
> > it, let me know if that's desired.
> >
> > - Marijn Suijten
> >
> >  profiles/audio/avrcp.c | 12 ++++++++----
> >  profiles/audio/media.c | 16 ----------------
> >  2 files changed, 8 insertions(+), 20 deletions(-)
> >
> > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > index e2428250e..8370c8a44 100644
> > --- a/profiles/audio/avrcp.c
> > +++ b/profiles/audio/avrcp.c
> > @@ -3625,12 +3625,13 @@ static void avrcp_volume_changed(struct avrcp *session,
> >         struct avrcp_player *player = target_get_player(session);
> >         uint8_t volume;
> >
> > -       if (!player)
> > -               return;
> > -
> >         volume = pdu->params[1] & 0x7F;
> >
> > -       player->cb->set_volume(volume, session->dev, player->user_data);
> > +       /* Always update the transport volume, which is separate from the player */
> > +       media_transport_update_device_volume(session->dev, volume);
> > +
> > +       if (player)
> > +               player->cb->set_volume(volume, session->dev, player->user_data);
> >  }
> >
> >  static void avrcp_status_changed(struct avrcp *session,
> > @@ -4378,6 +4379,9 @@ static gboolean avrcp_handle_set_volume(struct avctp *conn, uint8_t code,
> >
> >         volume = pdu->params[0] & 0x7F;
> >
> > +       /* Always update the transport volume, which is separate from the player */
> > +       media_transport_update_device_volume(session->dev, volume);
> > +
> >         if (player != NULL)
> >                 player->cb->set_volume(volume, session->dev, player->user_data);
> >
> > diff --git a/profiles/audio/media.c b/profiles/audio/media.c
> > index 993ecb3b3..a0173fdd4 100644
> > --- a/profiles/audio/media.c
> > +++ b/profiles/audio/media.c
> > @@ -1202,27 +1202,11 @@ static uint32_t get_duration(void *user_data)
> >  static void set_volume(uint8_t volume, struct btd_device *dev, void *user_data)
> >  {
> >         struct media_player *mp = user_data;
> > -       GSList *l;
> >
> >         if (mp->volume == volume)
> >                 return;
> >
> >         mp->volume = volume;
> > -
> > -       for (l = mp->adapter->endpoints; l; l = l->next) {
> > -               struct media_endpoint *endpoint = l->data;
> > -               struct media_transport *transport;
> > -
> > -               /* Volume is A2DP only */
> > -               if (endpoint->sep == NULL)
> > -                       continue;
> > -
> > -               transport = find_device_transport(endpoint, dev);
> > -               if (transport == NULL)
> > -                       continue;
> > -
> > -               media_transport_update_volume(transport, volume);
> > -       }
> >  }
> >
> >  static bool media_player_send(struct media_player *mp, const char *name)
> > --
> > 2.27.0
> >

Applied, Thanks. Note that I did fix some coding style problems.

-- 
Luiz Augusto von Dentz