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

Commit Message

Silviu Florian Barbulescu March 20, 2024, 2:41 p.m. UTC

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
---
 client/player.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 77 insertions(+), 3 deletions(-)

Comments

Luiz Augusto von Dentz March 20, 2024, 5:32 p.m. UTC | #1

Hi Silviu,

On Wed, Mar 20, 2024 at 2:42 PM Silviu Florian Barbulescu
<[e-mail]> 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 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
>

Silviu Florian Barbulescu March 21, 2024, 11:33 a.m. UTC | #2

Hi Luiz, 

> -----Original Message-----
> From: Luiz Augusto von Dentz <[e-mail]>
> Sent: Wednesday, March 20, 2024 7:33 PM
> To: Silviu Florian Barbulescu <[e-mail]>
> Cc: [e-mail]; Mihai-Octavian Urzica <mihai-
> [e-mail]>; Vlad Pruteanu <[e-mail]>; Andrei
> Istodorescu <[e-mail]>; Iulia Tanasescu
> <[e-mail]>
> 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
> <[e-mail]> 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

Patch

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,