Re: [PATCH BlueZ v3 1/3] audio/avrcp: Guard SetAbsoluteVolume without target behind config value

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Luiz,

I'm still clueless why this reply didn't reach my inbox.  I thought it
was going into ignore-land again (sorry) but randomly found your replies
on the BlueZ mailing list.  The bot message didn't make it through
either, but it did notify me about "audio/transport: Propagate errors
from avrcp_set_volume to DBus" being applied.

On 2023-03-14 16:16:55, Luiz Augusto von Dentz wrote:
> Hi Marijn,
> 
> On Fri, Mar 10, 2023 at 4:39 PM Marijn Suijten
> <marijn.suijten@xxxxxxxxxxxxxx> wrote:
> >
> > Commit 179ccb936 ("avrcp: Set volume if volume changed event is
> > registered") introduced a catch that allows SetAbsoluteVolume to be sent
> > to a remote device that does _not_ implement the AVRCP TG profile.  This
> > is strange as the TG role is required to be able to send commands to the
> > peer, but the commit must have been applied to the tree for a reason.
> >
> > We discussed in [1] that workarounds for dubious peers and software
> > stacks should be guarded behind a config entry in main.conf, so this
> > starts out by introducing a new [AVRCP] category to to it that will
> > later be extended with other workarounds.
> >
> > [1]: https://marc.info/?l=linux-bluetooth&m=163519566912788&w=2
> > ---
> >  profiles/audio/avrcp.c | 12 +++++++++---
> >  src/btd.h              |  5 +++++
> >  src/main.c             | 13 +++++++++++++
> >  src/main.conf          |  6 ++++++
> >  4 files changed, 33 insertions(+), 3 deletions(-)
> >
> > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > index 80f34c7a7..5e6322916 100644
> > --- a/profiles/audio/avrcp.c
> > +++ b/profiles/audio/avrcp.c
> > @@ -48,6 +48,7 @@
> >  #include "src/dbus-common.h"
> >  #include "src/shared/timeout.h"
> >  #include "src/shared/util.h"
> > +#include "src/btd.h"
> >
> >  #include "avctp.h"
> >  #include "avrcp.h"
> > @@ -4577,9 +4578,14 @@ int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
> >                                                                 &volume);
> >         }
> >
> > -       if (!session->controller && !avrcp_event_registered(session,
> > -                                       AVRCP_EVENT_VOLUME_CHANGED))
> > -               return -ENOTSUP;
> > +       if (btd_opts.avrcp.set_absolute_volume_without_target) {
> > +               if (!session->controller && !avrcp_event_registered(session,
> > +                                               AVRCP_EVENT_VOLUME_CHANGED))
> > +                       return -ENOTSUP;
> > +       } else {
> > +               if (!session->controller || session->controller->version < 0x0104)
> > +                       return -ENOTSUP;
> > +       }
> >
> >         memset(buf, 0, sizeof(buf));
> >
> > diff --git a/src/btd.h b/src/btd.h
> > index 42cffcde4..31c04a990 100644
> > --- a/src/btd.h
> > +++ b/src/btd.h
> > @@ -97,6 +97,10 @@ struct btd_avdtp_opts {
> >         uint8_t  stream_mode;
> >  };
> >
> > +struct btd_avrcp_opts {
> > +       gboolean set_absolute_volume_without_target;
> > +};
> > +
> >  struct btd_advmon_opts {
> >         uint8_t         rssi_sampling_period;
> >  };
> > @@ -136,6 +140,7 @@ struct btd_opts {
> >         enum mps_mode_t mps;
> >
> >         struct btd_avdtp_opts avdtp;
> > +       struct btd_avrcp_opts avrcp;
> >
> >         uint8_t         key_size;
> >
> > diff --git a/src/main.c b/src/main.c
> > index 99d9c508f..92f74e381 100644
> > --- a/src/main.c
> > +++ b/src/main.c
> > @@ -152,6 +152,11 @@ static const char *avdtp_options[] = {
> >         NULL
> >  };
> >
> > +static const char *avrcp_options[] = {
> > +       "SetAbsoluteVolumeWithoutTarget",
> > +       NULL
> > +};
> > +
> >  static const char *advmon_options[] = {
> >         "RSSISamplingPeriod",
> >         NULL
> > @@ -167,6 +172,7 @@ static const struct group_table {
> >         { "Policy",     policy_options },
> >         { "GATT",       gatt_options },
> >         { "AVDTP",      avdtp_options },
> > +       { "AVRCP",      avrcp_options },
> >         { "AdvMon",     advmon_options },
> >         { }
> >  };
> > @@ -975,6 +981,13 @@ static void parse_config(GKeyFile *config)
> >                 g_free(str);
> >         }
> >
> > +       boolean = g_key_file_get_boolean(config, "AVRCP",
> > +                                               "SetAbsoluteVolumeWithoutTarget", &err);
> > +       if (err)
> > +               g_clear_error(&err);
> > +       else
> > +               btd_opts.avrcp.set_absolute_volume_without_target = boolean;
> > +
> >         val = g_key_file_get_integer(config, "AdvMon", "RSSISamplingPeriod",
> >                                                                         &err);
> >         if (err) {
> > diff --git a/src/main.conf b/src/main.conf
> > index f187c9aaa..ca00ed03e 100644
> > --- a/src/main.conf
> > +++ b/src/main.conf
> > @@ -271,6 +271,12 @@
> >  # streaming: Use L2CAP Streaming Mode
> >  #StreamMode = basic
> >
> > +[AVRCP]
> > +# Allow SetAbsoluteVolume calls to a peer device that
> > +# does not advertise the AVRCP remote control target
> > +# profile.
> > +#SetAbsoluteVolumeWithoutTarget = false
> 
> Let's do just VolumeWithoutTarget and we should probably mention that
> it would ignore the version as well.

Sure, sounds good.  We can also invert the condition to validate the
version in the event that ->controller is non-NULL, even if
VolumeWithoutTarget is set.

- Marijn

> 
> > +
> >  [Policy]
> >  #
> >  # The ReconnectUUIDs defines the set of remote services that should try
> > --
> > 2.39.2
> >
> 
> 
> -- 
> Luiz Augusto von Dentz



[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux