Re: [PATCH BlueZ 2/2] player: Allow endpoint config without local endpoint argument

From: Silviu Florian Barbulescu <silviu.barbulescu@nxp.com>
To: luiz.dentz@gmail.com
Cc: andrei.istodorescu@nxp.com, iulia.tanasescu@nxp.com,
	linux-bluetooth@vger.kernel.org, mihai-octavian.urzica@nxp.com,
	silviu.barbulescu@nxp.com, vlad.pruteanu@nxp.com
Subject: Re: [PATCH BlueZ 2/2] player: Allow endpoint config without local endpoint argument
Date: Thu, 21 Mar 2024 13:33:59 +0200	[thread overview]
Message-ID: <20240321113359.12475-1-silviu.barbulescu@nxp.com> (raw)
In-Reply-To: <CABBYNZL-JFqb5wq-k4Ge2zOVPf71E3pVsnu5X-h4LOy1tziNCQ@mail.gmail.com>

Hi Luiz, 

> -----Original Message-----
> From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
> Sent: Wednesday, March 20, 2024 7:33 PM
> To: Silviu Florian Barbulescu <silviu.barbulescu@nxp.com>
> Cc: linux-bluetooth@vger.kernel.org; Mihai-Octavian Urzica <mihai-
> octavian.urzica@nxp.com>; Vlad Pruteanu <vlad.pruteanu@nxp.com>; Andrei
> Istodorescu <andrei.istodorescu@nxp.com>; Iulia Tanasescu
> <iulia.tanasescu@nxp.com>
> Subject: [EXT] Re: [PATCH BlueZ 2/2] player: Allow endpoint config without local
> endpoint argument
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
> 
> 
> Hi Silviu,
> 
> On Wed, Mar 20, 2024 at 2:42PM Silviu Florian Barbulescu
> <silviu.barbulescu@nxp.com> wrote:
> >
> > Update endpoint config command to create local endpoint for broadcast
> > endpoint, this will be used to configure a broadcast source\sink if
> > register endpoint is done from another application like PipeWire
> 
> Sorry but will not gona fly, we don't have any business with endpoints from
> other process in the system, in fact we might actually make it strictly prohibit
> for third-party process to use SetConfiguration with endpoint that do not belong
> their own D-Bus connection, same for MediaTransport, they shall only be
> acquired by the process that created them.
> 
> Also I remember quite clearly stating that we would move away from using
> MediaEndpoint for Broadcast Sink, instead what we shall be doing is
> enumerating BIS as MediaTransport directly so the likes of pipewire can Acquire
> them directly without doing any configuration since they are already configured
> over the air. Broadcast source will probably require a special interface at
> pipewire side, or perhaps a dedicated card which gets configured via some
> configuration file, etc, but we shall not depend on bluetoothctl to configured it,
> that will just make things more complicated than really helping.
> 

We are investigating a solution with the broadcast source configuration
from PipeWire and also removing the MediaEndpoint reliance for the broadcast sink.

> > ---
> >  client/player.c | 80
> > +++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 77 insertions(+), 3 deletions(-)
> >
> > diff --git a/client/player.c b/client/player.c index
> > ab33bfc46..b02a40423 100644
> > --- a/client/player.c
> > +++ b/client/player.c
> > @@ -3662,10 +3662,15 @@ static void endpoint_set_config_bcast(struct
> endpoint_config *cfg)
> >                 config_endpoint_iso_group, cfg);  }
> >
> > +static void endpoint_init_defaults(struct endpoint *ep);
> >  static void cmd_config_endpoint(int argc, char *argv[])  {
> > +       DBusMessageIter iter;
> >         struct endpoint_config *cfg;
> > -       const struct codec_preset *preset;
> > +       const struct codec_preset *preset = NULL;
> > +       const struct capabilities *cap;
> > +       uint8_t codec;
> > +       const char *uuid;
> >
> >         cfg = new0(struct endpoint_config, 1);
> >
> > @@ -3680,8 +3685,75 @@ static void cmd_config_endpoint(int argc, char
> *argv[])
> >         /* Search for the local endpoint */
> >         cfg->ep = endpoint_find(argv[2]);
> >         if (!cfg->ep) {
> > -               bt_shell_printf("Local Endpoint %s not found\n", argv[2]);
> > -               goto fail;
> > +               /* If argc != 3 then argv[2] should be the
> > +                * local endpoint. Return error.
> > +                */
> > +               if (argc != 3) {
> > +                       bt_shell_printf("Local Endpoint %s not found\n",
> > +                                       argv[2]);
> > +                       goto fail;
> > +               }
> > +
> > +               /* If local endpoint is not found verify if this is a
> > +                * broadcast remote endpoint because this can be an
> > +                * endpoint config command with remote endpoint and
> > +                * preset. In this case we have to create its own
> > +                * local endpoint.
> > +                */
> > +               if (!g_dbus_proxy_get_property(cfg->proxy, "UUID", &iter))
> > +                       return;
> > +               dbus_message_iter_get_basic(&iter, &uuid);
> > +
> > +               /* The local endpoint must have the UUID of local pac but
> > +                * the remote endpoint has the UUID of the remote pac, so
> > +                * based on this information we determine the UUID for the
> > +                * local endpoint.
> > +                */
> > +               if (!strcmp(uuid, BAA_SERVICE_UUID))
> > +                       uuid = BCAA_SERVICE_UUID;
> > +               else if (!strcmp(uuid, BCAA_SERVICE_UUID))
> > +                       uuid = BAA_SERVICE_UUID;
> > +               else {
> > +                       /* This should be a local endpoint because is not
> > +                        * a broadcast endpoint.
> > +                        */
> > +                       bt_shell_printf("Local Endpoint %s not found\n",
> > +                                       argv[2]);
> > +                       goto fail;
> > +               }
> > +
> > +               if (!g_dbus_proxy_get_property(cfg->proxy, "Codec", &iter))
> > +                       return;
> > +
> > +               dbus_message_iter_get_basic(&iter, &codec);
> > +
> > +               /* Verify if the local endpoint already exists */
> > +               cfg->ep = endpoint_find(uuid);
> > +               if ((!cfg->ep) || (cfg->ep->codec != codec)) {
> > +                       /* Get capabilities to create the local endpoint */
> > +                       cap = find_capabilities(uuid, codec);
> > +                       if (!cap) {
> > +                               bt_shell_printf(
> > +                                       "Capabilities not found for UUID %s, codec id %d",
> > +                                       uuid, codec);
> > +                               goto fail;
> > +                       }
> > +
> > +                       cfg->ep = endpoint_new(cap);
> > +                       endpoint_init_defaults(cfg->ep);
> > +               }
> > +
> > +               /* Verify if argv[2] parameter is a valid preset */
> > +               preset = preset_find_name(cfg->ep->preset, argv[2]);
> > +               if (!preset) {
> > +                       bt_shell_printf("Preset %s not found\n",
> > +                               argv[2]);
> > +                       /* Free created endpoint */
> > +                       local_endpoints = g_list_remove(local_endpoints,
> > +                                               cfg->ep);
> > +                       endpoint_free(cfg->ep);
> > +                       goto fail;
> > +               }
> >         }
> >
> >         if (argc > 3) {
> > @@ -3690,7 +3762,9 @@ static void cmd_config_endpoint(int argc, char
> *argv[])
> >                         bt_shell_printf("Preset %s not found\n", argv[3]);
> >                         goto fail;
> >                 }
> > +       }
> >
> > +       if (preset) {
> >                 cfg->caps = g_new0(struct iovec, 1);
> >                 /* Copy capabilities */
> >                 util_iov_append(cfg->caps, preset->data.iov_base,
> > --
> > 2.39.2
> >
> 
> 
> --
> Luiz Augusto von Dentz