Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-2675-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240320144153.46408-1-silviu.barbulescu@nxp.com> <20240320144153.46408-3-silviu.barbulescu@nxp.com>
In-Reply-To: <20240320144153.46408-3-silviu.barbulescu@nxp.com>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Wed, 20 Mar 2024 17:32:30 +0000
Message-ID: <CABBYNZL-JFqb5wq-k4Ge2zOVPf71E3pVsnu5X-h4LOy1tziNCQ@mail.gmail.com>
Subject: Re: [PATCH BlueZ 2/2] player: Allow endpoint config without local
 endpoint argument
To: Silviu Florian Barbulescu <silviu.barbulescu@nxp.com>
Cc: linux-bluetooth@vger.kernel.org, mihai-octavian.urzica@nxp.com, 
	vlad.pruteanu@nxp.com, andrei.istodorescu@nxp.com, iulia.tanasescu@nxp.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Silviu,

On Wed, Mar 20, 2024 at 2:42=E2=80=AFPM 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.

> ---
>  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 endp=
oint_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 =3D NULL;
> +       const struct capabilities *cap;
> +       uint8_t codec;
> +       const char *uuid;
>
>         cfg =3D new0(struct endpoint_config, 1);
>
> @@ -3680,8 +3685,75 @@ static void cmd_config_endpoint(int argc, char *ar=
gv[])
>         /* Search for the local endpoint */
>         cfg->ep =3D endpoint_find(argv[2]);
>         if (!cfg->ep) {
> -               bt_shell_printf("Local Endpoint %s not found\n", argv[2])=
;
> -               goto fail;
> +               /* If argc !=3D 3 then argv[2] should be the
> +                * local endpoint. Return error.
> +                */
> +               if (argc !=3D 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 th=
e
> +                * local endpoint.
> +                */
> +               if (!strcmp(uuid, BAA_SERVICE_UUID))
> +                       uuid =3D BCAA_SERVICE_UUID;
> +               else if (!strcmp(uuid, BCAA_SERVICE_UUID))
> +                       uuid =3D 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 =3D endpoint_find(uuid);
> +               if ((!cfg->ep) || (cfg->ep->codec !=3D codec)) {
> +                       /* Get capabilities to create the local endpoint =
*/
> +                       cap =3D find_capabilities(uuid, codec);
> +                       if (!cap) {
> +                               bt_shell_printf(
> +                                       "Capabilities not found for UUID =
%s, codec id %d",
> +                                       uuid, codec);
> +                               goto fail;
> +                       }
> +
> +                       cfg->ep =3D endpoint_new(cap);
> +                       endpoint_init_defaults(cfg->ep);
> +               }
> +
> +               /* Verify if argv[2] parameter is a valid preset */
> +               preset =3D 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 =3D 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 *arg=
v[])
>                         bt_shell_printf("Preset %s not found\n", argv[3])=
;
>                         goto fail;
>                 }
> +       }
>
> +       if (preset) {
>                 cfg->caps =3D g_new0(struct iovec, 1);
>                 /* Copy capabilities */
>                 util_iov_append(cfg->caps, preset->data.iov_base,
> --
> 2.39.2
>


--=20
Luiz Augusto von Dentz