by Luiz Augusto von Dentz

[permalink] [raw]

Subject: Re: [PATCH BlueZ v3 5/6] client/player: Add support for Company ID, Vendor ID

Hi Abhay,

On Thu, Dec 29, 2022 at 7:58 AM Abhay Maheta
<[email protected]> wrote:
>
> This adds support for Company ID and Vendor Codec ID in BAP profile.
> This also adds handling of Vendor Specific Coding format for BAP
> Profile.
>
> Now it allows to enter zero codec capabilities.
> In order to register zero codec capabilities, 0 shall be
> entered when prompted.
>
> [bluetooth]# endpoint.register 00002bc9-0000-1000-8000-00805f9b34fb 0xff
> [/local/endpoint/ep2] Enter Capabilities: 0
> [/local/endpoint/ep2] Enter Company ID & Vendor ID: 0xaabbccdd

Lets do this a little differently:

<UUID> <codec[:company]> [capabilities...]
endpoint.register 00002bc9-0000-1000-8000-00805f9b34fb 0xccdd:0xaabb

So you can detect if the user has entered a tuble by parsing ':', if
it does you assume the Codec to be 0xff and use the tuple entered as
Vendor otherwise you must omit the Vendor, which means you need to
have a exists callback that returns cid && vid.

> [/local/endpoint/ep2] Auto Accept (yes/no): y

I don't think it makes any sense to Auto Accept in case of vendor
codecs since we don't have presets for them, except if you want to
default to custom preset which the user can change.

> [/local/endpoint/ep2] CIG (auto/value): a
> [/local/endpoint/ep2] CIS (auto/value): a
> Endpoint /local/endpoint/ep2 registered
> ---
> client/player.c | 131 +++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 114 insertions(+), 17 deletions(-)
>
> diff --git a/client/player.c b/client/player.c
> index eba233329..ff1ff209c 100644
> --- a/client/player.c
> +++ b/client/player.c
> @@ -66,6 +66,8 @@ struct endpoint {
> char *path;
> char *uuid;
> uint8_t codec;
> + uint16_t cid;
> + uint16_t vid;
> struct iovec *caps;
> bool auto_accept;
> bool acquiring;
> @@ -85,6 +87,7 @@ static GList *endpoints = NULL;
> static GList *local_endpoints = NULL;
> static GList *transports = NULL;
> static struct queue *ios = NULL;
> +static bool is_cid_available = FALSE;
>
> struct transport {
> GDBusProxy *proxy;
> @@ -1815,7 +1818,8 @@ static void endpoint_free(void *data)
> struct endpoint *ep = data;
>
> if (ep->caps) {
> - g_free(ep->caps->iov_base);
> + if (ep->caps->iov_base)
> + g_free(ep->caps->iov_base);
> g_free(ep->caps);
> }
>
> @@ -1865,10 +1869,32 @@ static gboolean endpoint_get_capabilities(const GDBusPropertyTable *property,
> return TRUE;
> }
>
> +static gboolean endpoint_get_cid(const GDBusPropertyTable *property,
> + DBusMessageIter *iter, void *data)
> +{
> + struct endpoint *ep = data;
> +
> + dbus_message_iter_append_basic(iter, DBUS_TYPE_UINT16, &ep->cid);
> +
> + return TRUE;
> +}
> +
> +static gboolean endpoint_get_vid(const GDBusPropertyTable *property,
> + DBusMessageIter *iter, void *data)
> +{
> + struct endpoint *ep = data;
> +
> + dbus_message_iter_append_basic(iter, DBUS_TYPE_UINT16, &ep->vid);
> +
> + return TRUE;
> +}
> +
> static const GDBusPropertyTable endpoint_properties[] = {
> { "UUID", "s", endpoint_get_uuid, NULL, NULL },
> { "Codec", "y", endpoint_get_codec, NULL, NULL },
> { "Capabilities", "ay", endpoint_get_capabilities, NULL, NULL },
> + { "CompanyID", "q", endpoint_get_cid, NULL, NULL },
> + { "VendorCodecID", "q", endpoint_get_vid, NULL, NULL },
> { }
> };
>
> @@ -1886,12 +1912,20 @@ static void register_endpoint_setup(DBusMessageIter *iter, void *user_data)
>
> g_dbus_dict_append_entry(&dict, "Codec", DBUS_TYPE_BYTE, &ep->codec);
>
> - g_dbus_dict_append_basic_array(&dict, DBUS_TYPE_STRING, &key,
> + g_dbus_dict_append_entry(&dict, "CompanyID", DBUS_TYPE_UINT16,
> + &ep->cid);
> +
> + g_dbus_dict_append_entry(&dict, "VendorCodecID", DBUS_TYPE_UINT16,
> + &ep->vid);
> +
> + if (ep->caps->iov_len) {
> + g_dbus_dict_append_basic_array(&dict, DBUS_TYPE_STRING, &key,
> DBUS_TYPE_BYTE, &ep->caps->iov_base,
> ep->caps->iov_len);
>
> - bt_shell_printf("Capabilities:\n");
> - bt_shell_hexdump(ep->caps->iov_base, ep->caps->iov_len);
> + bt_shell_printf("Capabilities:\n");
> + bt_shell_hexdump(ep->caps->iov_base, ep->caps->iov_len);
> + }
>
> dbus_message_iter_close_container(iter, &dict);
> }
> @@ -1950,6 +1984,21 @@ fail:
>
> }
>
> +static void get_cid_vid(const char *input, uint32_t *id)
> +{
> + char *endptr = NULL;
> + int value;
> +
> + value = strtol(input, &endptr, 0);
> +
> + if (!endptr || (char)*endptr != (char)'\0' || value > UINT32_MAX) {
> + bt_shell_printf("Invalid argument: %s\n", input);
> + return bt_shell_noninteractive_quit(EXIT_FAILURE);
> + }
> +
> + *id = (uint32_t)value;
> +}
> +
> static void endpoint_cis(const char *input, void *user_data)
> {
> struct endpoint *ep = user_data;
> @@ -2010,19 +2059,55 @@ static void endpoint_auto_accept(const char *input, void *user_data)
> bt_shell_prompt_input(ep->path, "CIG (auto/value):", endpoint_cig, ep);
> }
>
> +static void endpoint_set_id(const char *input, void *user_data)
> +{
> + struct endpoint *ep = user_data;
> + uint32_t val;
> +
> + get_cid_vid(input, &val);
> + ep->cid = (uint16_t)(val & 0x0000ffff);
> + ep->vid = (uint16_t)((val & 0xffff0000) >> 16);
> +
> + bt_shell_prompt_input(ep->path, "Auto Accept (yes/no):",
> + endpoint_auto_accept, ep);
> +}
> +
> +static void endpoint_set_parameters(struct endpoint *ep)
> +{
> + if (!(strcasecmp(ep->uuid, PAC_SINK_UUID)) ||
> + !(strcasecmp(ep->uuid, PAC_SOURCE_UUID))) {
> + if ((ep->codec == 0xff) && (is_cid_available == FALSE))
> + bt_shell_prompt_input(ep->path,
> + "Enter Company ID & Vendor ID:",
> + endpoint_set_id, ep);
> + else
> + bt_shell_prompt_input(ep->path, "Auto Accept (yes/no):",
> + endpoint_auto_accept, ep);
> + } else
> + bt_shell_prompt_input(ep->path, "Auto Accept (yes/no):",
> + endpoint_auto_accept, ep);
> +}
> +
> static void endpoint_set_capabilities(const char *input, void *user_data)
> {
> struct endpoint *ep = user_data;
>
> - if (ep->caps)
> + if (ep->caps && ep->caps->iov_base) {
> g_free(ep->caps->iov_base);
> - else
> + ep->caps = g_new0(struct iovec, 1);
> + } else
> ep->caps = g_new0(struct iovec, 1);
>
> ep->caps->iov_base = str2bytearray((char *) input, &ep->caps->iov_len);
>
> - bt_shell_prompt_input(ep->path, "Auto Accept (yes/no):",
> - endpoint_auto_accept, ep);
> + if (ep->caps->iov_len == 0x01 &&
> + (*(uint8_t *)(ep->caps->iov_base)) == 0x00) {
> + g_free(ep->caps->iov_base);
> + ep->caps->iov_base = NULL;
> + ep->caps->iov_len = 0x00;
> + }
> +
> + endpoint_set_parameters(ep);
> }
>
> static char *uuid_generator(const char *text, int state)
> @@ -2073,13 +2158,13 @@ static void cmd_register_endpoint(int argc, char *argv[])
> ep = g_new0(struct endpoint, 1);
> ep->uuid = g_strdup(argv[1]);
> ep->codec = strtol(argv[2], &endptr, 0);
> + ep->cid = 0x0000;
> + ep->vid = 0x0000;
> ep->path = g_strdup_printf("%s/ep%u", BLUEZ_MEDIA_ENDPOINT_PATH,
> g_list_length(local_endpoints));
> local_endpoints = g_list_append(local_endpoints, ep);
>
> - if (argc > 3)
> - endpoint_set_capabilities(argv[3], ep);
> - else {
> + if (argc == 3) {
> const struct capabilities *cap;
>
> cap = find_capabilities(ep->uuid, ep->codec);
> @@ -2089,13 +2174,25 @@ static void cmd_register_endpoint(int argc, char *argv[])
>
> /* Copy capabilities */
> iov_append(&ep->caps, cap->data.iov_base,
> - cap->data.iov_len);
> + cap->data.iov_len);
>
> - bt_shell_prompt_input(ep->path, "Auto Accept (yes/no):",
> - endpoint_auto_accept, ep);
> + endpoint_set_parameters(ep);
> } else
> - bt_shell_prompt_input(ep->path, "Enter capabilities:",
> - endpoint_set_capabilities, ep);
> + bt_shell_prompt_input(ep->path, "Enter Capabilities:",
> + endpoint_set_capabilities, ep);
> + } else if (argc == 4) {
> + endpoint_set_capabilities(argv[3], ep);
> + } else if (argc == 5) {
> + uint32_t val = 0;
> +
> + get_cid_vid(argv[4], &val);
> + ep->cid = (uint16_t)(val & 0x0000ffff);
> + ep->vid = (uint16_t)((val & 0xffff0000) >> 16);
> + is_cid_available = TRUE;
> +
> + endpoint_set_capabilities(argv[3], ep);
> +
> + is_cid_available = FALSE;
> }
> }
>
> @@ -2638,7 +2735,7 @@ static const struct bt_shell_menu endpoint_menu = {
> { "show", "<endpoint>", cmd_show_endpoint,
> "Endpoint information",
> endpoint_generator },
> - { "register", "<UUID> <codec> [capabilities...]",
> + { "register", "<UUID> <codec> [capabilities...] [Company ID]",
> cmd_register_endpoint,
> "Register Endpoint",
> uuid_generator },
> --
> 2.25.1
>

--
Luiz Augusto von Dentz

2023-01-04 16:43:48

by Abhay Maheta

[permalink] [raw]

Subject: RE: [PATCH BlueZ v3 2/6] shared/bap: Add support to set stream metadata

Hi Luiz,

I agree. Shall continue using "bt_bap_steam_metadata" API.

Thanks & Regards,
Abhay

-----Original Message-----
From: Luiz Augusto von Dentz <[email protected]>
Sent: 04 January 2023 05:55
To: Maheta, Abhay <[email protected]>
Cc: [email protected]; Abhay Maheta <[email protected]>
Subject: Re: [PATCH BlueZ v3 2/6] shared/bap: Add support to set stream metadata

Hi Abhay,

On Thu, Dec 29, 2022 at 7:57 AM Abhay Maheta <[email protected]> wrote:
>
> This adds new API to set stream metadata.
> ---
> src/shared/bap.c | 30 +++++++++++++++++++++++-------
> src/shared/bap.h | 2 ++
> 2 files changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/src/shared/bap.c b/src/shared/bap.c index
> 0cafb75e6..b3c65283e 100644
> --- a/src/shared/bap.c
> +++ b/src/shared/bap.c
> @@ -344,8 +344,10 @@ static void pac_foreach(void *data, void
> *user_data)
>
> p = util_iov_push(iov, sizeof(*p));
> p->codec.id = pac->codec.id;
> - p->codec.cid = pac->codec.cid;
> - p->codec.vid = pac->codec.vid;
> + if (p->codec.id == 0xff) {
> + p->codec.cid = cpu_to_le16(pac->codec.cid);
> + p->codec.vid = cpu_to_le16(pac->codec.vid);
> + }
>
> if (pac->data) {
> p->cc_len = pac->data->iov_len; @@ -2773,7 +2775,7 @@
> static void bap_parse_pacs(struct bt_bap *bap, uint8_t type,
> struct bt_pac *p;
> struct bt_ltv *cc;
> struct bt_pac_metadata *meta;
> - struct iovec data, metadata;
> + struct iovec data, *metadata = NULL;
>
> p = util_iov_pull_mem(&iov, sizeof(*p));
> if (!p) {
> @@ -2802,8 +2804,11 @@ static void bap_parse_pacs(struct bt_bap *bap, uint8_t type,
> data.iov_len = p->cc_len;
> data.iov_base = cc;
>
> - metadata.iov_len = meta->len;
> - metadata.iov_base = meta->data;
> + if (meta->len) {
> + metadata = new0(struct iovec, 1);
> + metadata->iov_len = meta->len;
> + metadata->iov_base = meta->data;
> + }
>
> util_iov_pull_mem(&iov, meta->len);
>
> @@ -2813,12 +2818,14 @@ static void bap_parse_pacs(struct bt_bap *bap, uint8_t type,
> /* Check if there is already a PAC record for the codec */
> pac = bap_pac_find(bap->rdb, type, &p->codec);
> if (pac) {
> - bap_pac_merge(pac, &data, &metadata);
> + bap_pac_merge(pac, &data, metadata);
> + free(metadata);
> continue;
> }
>
> pac = bap_pac_new(bap->rdb, NULL, type, &p->codec, NULL, &data,
> - &metadata);
> + metadata);
> + free(metadata);
> if (!pac)
> continue;
>
> @@ -4591,6 +4598,15 @@ struct bt_bap_qos *bt_bap_stream_get_qos(struct bt_bap_stream *stream)
> return &stream->qos;
> }
>
> +void bt_bap_stream_set_metadata(struct bt_bap_stream *stream,
> + struct iovec *meta) {
> + if (!stream)
> + return;
> +
> + stream_metadata(stream, meta, NULL); }
> +
> struct iovec *bt_bap_stream_get_metadata(struct bt_bap_stream
> *stream) {
> if (!stream)
> diff --git a/src/shared/bap.h b/src/shared/bap.h index
> 47a15636c..bcf830ceb 100644
> --- a/src/shared/bap.h
> +++ b/src/shared/bap.h
> @@ -248,6 +248,8 @@ uint8_t bt_bap_stream_get_dir(struct bt_bap_stream
> *stream); uint32_t bt_bap_stream_get_location(struct bt_bap_stream
> *stream); struct iovec *bt_bap_stream_get_config(struct bt_bap_stream
> *stream); struct bt_bap_qos *bt_bap_stream_get_qos(struct
> bt_bap_stream *stream);
> +void bt_bap_stream_set_metadata(struct bt_bap_stream *stream,
> + struct iovec *meta);

There is already bt_bap_steam_metadata so Im not sure why you want to add another one that just set it locally?

> struct iovec *bt_bap_stream_get_metadata(struct bt_bap_stream
> *stream);
>
> struct io *bt_bap_stream_get_io(struct bt_bap_stream *stream);
> --
> 2.25.1
>

--
Luiz Augusto von Dentz