[PATCH BlueZ v4 1/3] client/player: Set number of channels based on locations

[PATCH BlueZ v4 1/3] client/player: Set number of channels based on locations

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This sets the number of channels based on the locations set rather than
always hardcoding it to 3 which in certain case is incorrect and can
lead for the same location to be configured multiple times.
---
 client/player.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/client/player.c b/client/player.c
index 5b0b918fb8d7..9334a053d34d 100644
--- a/client/player.c
+++ b/client/player.c
@@ -1140,10 +1140,9 @@ static DBusMessage *endpoint_set_configuration(DBusConnection *conn,
 		.meta = _meta, \
 	}
 
-#define LC3_DATA(_freq, _duration, _chan_count, _len_min, _len_max) \
+#define LC3_DATA(_freq, _duration, _len_min, _len_max) \
 	UTIL_IOV_INIT(0x03, LC3_FREQ, _freq, _freq >> 8, \
 			0x02, LC3_DURATION, _duration, \
-			0x02, LC3_CHAN_COUNT, _chan_count, \
 			0x05, LC3_FRAME_LEN, _len_min, _len_min >> 8, \
 			_len_max, _len_max >> 8)
 
@@ -1182,11 +1181,10 @@ static const struct capabilities {
 	 *
 	 * Frequencies: 8Khz 11Khz 16Khz 22Khz 24Khz 32Khz 44.1Khz 48Khz
 	 * Duration: 7.5 ms 10 ms
-	 * Channel count: 3
 	 * Frame length: 26-240
 	 */
 	CODEC_CAPABILITIES("pac_snk/lc3", PAC_SINK_UUID, LC3_ID,
-				LC3_DATA(LC3_FREQ_ANY, LC3_DURATION_ANY, 3u, 26,
+				LC3_DATA(LC3_FREQ_ANY, LC3_DURATION_ANY, 26,
 					240),
 				UTIL_IOV_INIT()),
 
@@ -1198,7 +1196,7 @@ static const struct capabilities {
 	 * Frame length: 26-240
 	 */
 	CODEC_CAPABILITIES("pac_src/lc3", PAC_SOURCE_UUID, LC3_ID,
-				LC3_DATA(LC3_FREQ_ANY, LC3_DURATION_ANY, 3u, 26,
+				LC3_DATA(LC3_FREQ_ANY, LC3_DURATION_ANY, 26,
 					240),
 				UTIL_IOV_INIT()),
 
@@ -1210,7 +1208,7 @@ static const struct capabilities {
 	 * Frame length: 26-240
 	 */
 	CODEC_CAPABILITIES("bcaa/lc3", BCAA_SERVICE_UUID, LC3_ID,
-				LC3_DATA(LC3_FREQ_ANY, LC3_DURATION_ANY, 3u, 26,
+				LC3_DATA(LC3_FREQ_ANY, LC3_DURATION_ANY, 26,
 					240),
 				UTIL_IOV_INIT()),
 
@@ -1222,7 +1220,7 @@ static const struct capabilities {
 	 * Frame length: 26-240
 	 */
 	CODEC_CAPABILITIES("baa/lc3", BAA_SERVICE_UUID, LC3_ID,
-				LC3_DATA(LC3_FREQ_ANY, LC3_DURATION_ANY, 3u, 26,
+				LC3_DATA(LC3_FREQ_ANY, LC3_DURATION_ANY, 26,
 					240),
 				UTIL_IOV_INIT()),
 };
@@ -3220,6 +3218,7 @@ static void endpoint_locations(const char *input, void *user_data)
 	struct endpoint *ep = user_data;
 	char *endptr = NULL;
 	int value;
+	uint8_t channels;
 
 	value = strtol(input, &endptr, 0);
 
@@ -3230,6 +3229,12 @@ static void endpoint_locations(const char *input, void *user_data)
 
 	ep->locations = value;
 
+	channels = __builtin_popcount(value);
+	/* Automatically set LC3_CHAN_COUNT if only 1 location is supported */
+	if (channels == 1)
+		util_ltv_push(ep->caps, sizeof(channels), LC3_CHAN_COUNT,
+				&channels);
+
 	bt_shell_prompt_input(ep->path, "Supported Context (value):",
 				endpoint_supported_context, ep);
 }
-- 
2.45.2

[PATCH BlueZ v4 2/3] client/player: Add support to enter alternative preset

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This adds support for alternative preset to be entered so when auto
accepting configuration a different preset can be selected following the
order given to endpoint.presets.
---
 client/player.c | 120 ++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 101 insertions(+), 19 deletions(-)

diff --git a/client/player.c b/client/player.c
index 9334a053d34d..3c3587f2ca3a 100644
--- a/client/player.c
+++ b/client/player.c
@@ -1230,7 +1230,10 @@ struct codec_preset {
 	const struct iovec data;
 	struct bt_bap_qos qos;
 	uint8_t target_latency;
+	uint32_t chan_alloc;
 	bool custom;
+	bool alt;
+	struct codec_preset *alt_preset;
 };
 
 #define SBC_PRESET(_name, _data) \
@@ -1969,12 +1972,31 @@ static int parse_chan_alloc(DBusMessageIter *iter, uint32_t *location,
 			if (*channels)
 				*channels = __builtin_popcount(*location);
 			return 0;
+		} else if (!strcasecmp(key, "Locations")) {
+			uint32_t tmp;
+
+			if (var != DBUS_TYPE_UINT32)
+				return -EINVAL;
+
+			dbus_message_iter_get_basic(&value, &tmp);
+			*location &= tmp;
+
+			if (*channels)
+				*channels = __builtin_popcount(*location);
 		}
 
 		dbus_message_iter_next(iter);
 	}
 
-	return -EINVAL;
+	return *location ? 0 : -EINVAL;
+}
+
+static void ltv_find(size_t i, uint8_t l, uint8_t t, uint8_t *v,
+					void *user_data)
+{
+	bool *found = user_data;
+
+	*found = true;
 }
 
 static DBusMessage *endpoint_select_properties_reply(struct endpoint *ep,
@@ -1985,7 +2007,7 @@ static DBusMessage *endpoint_select_properties_reply(struct endpoint *ep,
 	DBusMessageIter iter, props;
 	struct endpoint_config *cfg;
 	struct bt_bap_io_qos *qos;
-	uint32_t location = 0;
+	uint32_t location = ep->locations;
 	uint8_t channels = 1;
 
 	if (!preset)
@@ -2006,13 +2028,44 @@ static DBusMessage *endpoint_select_properties_reply(struct endpoint *ep,
 	dbus_message_iter_recurse(&iter, &props);
 
 	if (!parse_chan_alloc(&props, &location, &channels)) {
-		uint8_t chan_alloc_ltv[] = {
-			0x05, LC3_CONFIG_CHAN_ALLOC, location & 0xff,
-			location >> 8, location >> 16, location >> 24
-		};
+		uint32_t chan_alloc = 0;
+		uint8_t type = LC3_CONFIG_CHAN_ALLOC;
+		bool found = false;
 
-		util_iov_append(cfg->caps, &chan_alloc_ltv,
+		if (preset->chan_alloc & location)
+			chan_alloc = preset->chan_alloc & location;
+		else if (preset->alt_preset &&
+					preset->alt_preset->chan_alloc &
+					location) {
+			chan_alloc = preset->alt_preset->chan_alloc & location;
+			preset = preset->alt_preset;
+
+			/* Copy alternate capabilities */
+			util_iov_free(cfg->caps, 1);
+			cfg->caps = util_iov_dup(&preset->data, 1);
+			cfg->target_latency = preset->target_latency;
+		} else
+			chan_alloc = location;
+
+		/* Check if Channel Allocation is present in caps */
+		util_ltv_foreach(cfg->caps->iov_base, cfg->caps->iov_len,
+					&type, ltv_find, &found);
+
+		/* If Channel Allocation has not been set directly via
+		 * preset->data then attempt to set it if chan_alloc has been
+		 * set.
+		 */
+		if (!found && chan_alloc) {
+			uint8_t chan_alloc_ltv[] = {
+				0x05, LC3_CONFIG_CHAN_ALLOC, chan_alloc & 0xff,
+				chan_alloc >> 8, chan_alloc >> 16,
+				chan_alloc >> 24
+			};
+
+			put_le32(chan_alloc, &chan_alloc_ltv[2]);
+			util_iov_append(cfg->caps, &chan_alloc_ltv,
 						sizeof(chan_alloc_ltv));
+		}
 	}
 
 	/* Copy metadata */
@@ -2035,6 +2088,8 @@ static DBusMessage *endpoint_select_properties_reply(struct endpoint *ep,
 
 	dbus_message_iter_init_append(reply, &iter);
 
+	bt_shell_printf("selecting %s...\n", preset->name);
+
 	append_properties(&iter, cfg);
 
 	free(cfg);
@@ -2098,8 +2153,6 @@ static DBusMessage *endpoint_select_properties(DBusConnection *conn,
 	if (!reply)
 		return NULL;
 
-	bt_shell_printf("Auto Accepting using %s...\n", p->name);
-
 	return reply;
 }
 
@@ -3621,14 +3674,6 @@ add_meta:
 			endpoint_set_metadata_cfg, cfg);
 }
 
-static void ltv_find(size_t i, uint8_t l, uint8_t t, uint8_t *v,
-					void *user_data)
-{
-	bool *found = user_data;
-
-	*found = true;
-}
-
 static void config_endpoint_iso_group(const char *input, void *user_data)
 {
 	struct endpoint_config *cfg = user_data;
@@ -4106,13 +4151,38 @@ static void print_presets(struct preset *preset)
 
 	for (i = 0; i < preset->num_presets; i++) {
 		p = &preset->presets[i];
-		bt_shell_printf("%s%s\n", p == preset->default_preset ?
-						"*" : "", p->name);
+
+		if (p == preset->default_preset)
+			bt_shell_printf("*%s\n", p->name);
+		else if (preset->default_preset &&
+					p == preset->default_preset->alt_preset)
+			bt_shell_printf("**%s\n", p->name);
+		else
+			bt_shell_printf("%s\n", p->name);
 	}
 
 	queue_foreach(preset->custom, foreach_custom_preset_print, preset);
 }
 
+static void custom_chan_alloc(const char *input, void *user_data)
+{
+	struct codec_preset *p = user_data;
+	char *endptr = NULL;
+
+	p->chan_alloc = strtol(input, &endptr, 0);
+	if (!endptr || *endptr != '\0') {
+		bt_shell_printf("Invalid argument: %s\n", input);
+		return bt_shell_noninteractive_quit(EXIT_FAILURE);
+	}
+
+	if (p->alt_preset)
+		bt_shell_prompt_input(p->alt_preset->name,
+					"Enter Channel Allocation: ",
+					custom_chan_alloc, p->alt_preset);
+	else
+		return bt_shell_noninteractive_quit(EXIT_SUCCESS);
+}
+
 static void cmd_presets_endpoint(int argc, char *argv[])
 {
 	struct preset *preset;
@@ -4133,8 +4203,20 @@ static void cmd_presets_endpoint(int argc, char *argv[])
 		preset->default_preset = default_preset;
 
 		if (argc > 4) {
+			struct codec_preset *alt_preset;
 			struct iovec *iov = (void *)&default_preset->data;
 
+			/* Check if and alternative preset was given */
+			alt_preset = preset_find_name(preset, argv[4]);
+			if (alt_preset) {
+				default_preset->alt_preset = alt_preset;
+				bt_shell_prompt_input(default_preset->name,
+						"Enter Channel Allocation: ",
+						custom_chan_alloc,
+						default_preset);
+				return;
+			}
+
 			iov->iov_base = str2bytearray(argv[4], &iov->iov_len);
 			if (!iov->iov_base) {
 				bt_shell_printf("Invalid configuration %s\n",
-- 
2.45.2

[PATCH BlueZ v4 3/3] shared/bap: Fix bt_bap_select with multiple lpacs

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

When there are multiple local PAC records of the same codec with
different locations only the first was consider, also bt_bap_select
would stop doing location matching early if the location don't match
without considering there could be more remote channels.
---
 src/shared/bap.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/src/shared/bap.c b/src/shared/bap.c
index 0aa89c2781ba..499e740c9162 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -3249,25 +3249,32 @@ static void *ltv_merge(struct iovec *data, struct iovec *cont)
 	return util_iov_append(data, cont->iov_base, cont->iov_len);
 }
 
-static void bap_pac_foreach_channel(size_t i, uint8_t l, uint8_t t, uint8_t *v,
-					void *user_data)
+static void bap_pac_chan_add(struct bt_bap_pac *pac, uint8_t count,
+				uint32_t location)
 {
-	struct bt_bap_pac *pac = user_data;
 	struct bt_bap_chan *chan;
 
-	if (!v)
-		return;
-
 	if (!pac->channels)
 		pac->channels = queue_new();
 
 	chan = new0(struct bt_bap_chan, 1);
-	chan->count = *v;
-	chan->location = bt_bap_pac_get_locations(pac) ? : pac->qos.location;
+	chan->count = count;
+	chan->location = location;
 
 	queue_push_tail(pac->channels, chan);
 }
 
+static void bap_pac_foreach_channel(size_t i, uint8_t l, uint8_t t, uint8_t *v,
+					void *user_data)
+{
+	struct bt_bap_pac *pac = user_data;
+
+	if (!v)
+		return;
+
+	bap_pac_chan_add(pac, *v, bt_bap_pac_get_locations(pac));
+}
+
 static void bap_pac_update_channels(struct bt_bap_pac *pac, struct iovec *data)
 {
 	uint8_t type = 0x03;
@@ -3277,6 +3284,13 @@ static void bap_pac_update_channels(struct bt_bap_pac *pac, struct iovec *data)
 
 	util_ltv_foreach(data->iov_base, data->iov_len, &type,
 				bap_pac_foreach_channel, pac);
+
+	/* If record didn't set a channel count but set a location use that as
+	 * channel count.
+	 */
+	if (queue_isempty(pac->channels) && pac->qos.location)
+		bap_pac_chan_add(pac, pac->qos.location, pac->qos.location);
+
 }
 
 static void bap_pac_merge(struct bt_bap_pac *pac, struct iovec *data,
@@ -3607,6 +3621,9 @@ uint32_t bt_bap_pac_get_locations(struct bt_bap_pac *pac)
 	if (!pac)
 		return 0;
 
+	if (pac->qos.location)
+		return pac->qos.location;
+
 	pacs = pac->bdb->pacs;
 
 	switch (pac->type) {
@@ -5411,7 +5428,7 @@ int bt_bap_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
 
 			/* Try matching the channel location */
 			if (!(map.location & rc->location))
-				break;
+				continue;
 
 			lpac->ops->select(lpac, rpac, map.location &
 						rc->location, &rpac->qos,
-- 
2.45.2

RE: [BlueZ,v4,1/3] client/player: Set number of channels based on locations

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1559 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=873929

---Test result---

Test Summary:
CheckPatch                    PASS      0.87 seconds
GitLint                       PASS      0.55 seconds
BuildEll                      PASS      24.34 seconds
BluezMake                     PASS      1636.82 seconds
MakeCheck                     PASS      12.85 seconds
MakeDistcheck                 PASS      176.01 seconds
CheckValgrind                 PASS      249.69 seconds
CheckSmatch                   WARNING   351.84 seconds
bluezmakeextell               PASS      118.52 seconds
IncrementalBuild              PASS      4526.64 seconds
ScanBuild                     PASS      982.20 seconds

Details
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
src/shared/bap.c:288:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:288:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:288:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structures


---
Regards,
Linux Bluetooth

Re: [PATCH BlueZ v4 1/3] client/player: Set number of channels based on locations

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Thu, 25 Jul 2024 17:36:24 -0400 you wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> This sets the number of channels based on the locations set rather than
> always hardcoding it to 3 which in certain case is incorrect and can
> lead for the same location to be configured multiple times.
> ---
>  client/player.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)

Here is the summary with links:
  - [BlueZ,v4,1/3] client/player: Set number of channels based on locations
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=4c9d4ed059b5
  - [BlueZ,v4,2/3] client/player: Add support to enter alternative preset
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=c2312ebe3184
  - [BlueZ,v4,3/3] shared/bap: Fix bt_bap_select with multiple lpacs
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=fcf39175e35e

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html