Bluetooth: BAP: Broadcast Source: Update stream codec config data #72908

niym-ot · 2024-05-17T04:16:01Z

When creating a BAP broadcast source with bt_bap_broadcast_source_create, only the subgroup information is stored in the streams and the remaining BIS specific information is not stored in the stream->codec_cfg, which it should.
Fix is to store bis specific information also in stream codec config.

Fixes #71125

Thalley

This won't work :(

The code looks good, but it has the issue that the merged result is stored in the subgroup codec configuration struct.

This means that if you have something like
subgroup channel allocation = LEFT | RIGHT
and BIS 1 channel allocation = LEFT and BIS 2 channel allocation = RIGHT, then the codec_cfg struct first contains LEFT | RIGHT, but after the first call to update_codec_cfg_data it is now just LEFT (because it's just being overridden) and after the second call the value is just RIGHT.

The sideeffect of this is that now the subgroup channel allocation is RIGHT (and not LEFT | RIGHT as requested by the application), but what's worse is that both BIS 1 and BIS 2 are simply pointing to the subgroups codec_cfg, which means that they are also both just RIGHT.

If you look at the similar changes I did for the broadcast sink in #69342, then you'll notice that I had to allocate a codec_cfg struct per stream in the broadcast sink. I think we need to do the same here: i.e. we need to be able to store codec_cfg per stream, which currently isn't possible as we only get 1 codec_cfg per subgroup.

The upside of storing it in the broadcast source struct, besides being able to fix the problem, is that we can make the parameters immutable, i.e. the application does not need to store the params for the lifetime of the broadcast source, and can't accidentally modify them without using the proper APIs.

For the next iteration of this PR, I also suggest to add testing of this behavior, i.e. verify that the values set by each stream in the parameters are the same as if you get it from the stream->codec_cfg pointer.

subsys/bluetooth/audio/bap_broadcast_source.c

subsys/bluetooth/audio/bap_endpoint.h

tests/bsim/bluetooth/audio/src/bap_broadcast_source_test.c

Nithin has contributed the following PRs zephyrproject-rtos#72908 zephyrproject-rtos#72333 zephyrproject-rtos#71798 zephyrproject-rtos#70373 and is attending the weekly LE Audio Zephyr meetings. Signed-off-by: Lars Knudsen <larsgk@gmail.com>

Nithin has contributed the following PRs #72908 #72333 #71798 #70373 and is attending the weekly LE Audio Zephyr meetings. Signed-off-by: Lars Knudsen <larsgk@gmail.com>

Nithin has contributed the following PRs zephyrproject-rtos/zephyr#72908 zephyrproject-rtos/zephyr#72333 zephyrproject-rtos/zephyr#71798 zephyrproject-rtos/zephyr#70373 and is attending the weekly LE Audio Zephyr meetings. (cherry picked from commit 3fd01c5) Original-Signed-off-by: Lars Knudsen <larsgk@gmail.com> GitOrigin-RevId: 3fd01c5 Change-Id: I1539d7f9acb7d333ae6c6200dffaec5deceb0c3b Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/5591121 Commit-Queue: Fabio Baltieri <fabiobaltieri@google.com> Reviewed-by: Fabio Baltieri <fabiobaltieri@google.com> Tested-by: ChromeOS Prod (Robot) <chromeos-ci-prod@chromeos-bot.iam.gserviceaccount.com>

Nithin has contributed the following PRs zephyrproject-rtos#72908 zephyrproject-rtos#72333 zephyrproject-rtos#71798 zephyrproject-rtos#70373 and is attending the weekly LE Audio Zephyr meetings. Signed-off-by: Lars Knudsen <larsgk@gmail.com>

subsys/bluetooth/audio/bap_broadcast_source.c

subsys/bluetooth/audio/bap_endpoint.h

tests/bsim/bluetooth/audio/src/bap_broadcast_source_test.c

Thalley · 2024-06-07T10:52:13Z

tests/bsim/bluetooth/audio/src/bap_broadcast_source_test.c

+	const struct bt_audio_codec_cfg *codec_cfg = stream->codec_cfg;
+	const struct bt_audio_codec_cfg *exp_codec_cfg = &preset_16_1_1.codec_cfg;


I'm not following this.

The whole point of this change is that the stream->codec_cfg can be different from the preset_16_1_1.codec_cfg which is only applied to the subgroup.

I strongly suggest that you attempt to write the test first and have it fail, and then apply your fix, and then verify that the test passes.

I heavily assume that without your patch, this test still passes.

This is again something that would have been much easier to verify as a unit test ;)

As discussed, bis specific data is contained in bis_codec_data and contains only channel info (BT_AUDIO_LOCATION_FRONT_CENTER). So channel info is verified to be bis specific data.

static uint8_t bis_codec_data[] = {
BT_AUDIO_CODEC_DATA(BT_AUDIO_CODEC_CFG_CHAN_ALLOC,
BT_BYTES_LIST_LE32(BT_AUDIO_LOCATION_FRONT_CENTER)),
};

Rest of the codec data is the subgroup codec data which is preset_16_1_1.codec_cfg

I think it's the naming that throws me off here. You are setting

exp_codec_cfg = &preset_16_1_1.codec_cfg;

and the preset_16_1_1 is what we use for the subgroups.

We specifically do not expect the values of the subgroup, but rather the merged result of the subgroup and the BIS, and thus calling the subgroup's codec config for exp_codec_cfg does not sound right

Thalley · 2024-06-07T10:53:25Z

tests/bsim/bluetooth/audio/src/bap_broadcast_source_test.c

-	uint8_t bis_codec_data[] = {
-		BT_AUDIO_CODEC_DATA(BT_AUDIO_CODEC_CFG_FREQ,
-				    BT_BYTES_LIST_LE16(BT_AUDIO_CODEC_CFG_FREQ_16KHZ)),
-	};


It is imperative that you set BIS specific data.

These should definitely not be removed.

BIS specific data is moved as global variable

When creating a BAP broadcast source with bt_bap_broadcast_source_create only the subgroup information is stored in the streams and the remaining BIS specific information is not stored in the stream->codec_cfg, which it should. Fix is to store bis specific information also in stream codec config. Updated broadcast source BSIM test to verify above usecase. Signed-off-by: Nithin Ramesh Myliattil <niym@demant.com>

Thalley

The implementation looks good and it's nice that it gets tested.

There are a few outstanding comments, but nothing that absolutely needs to be fixed

Thalley · 2024-06-13T08:50:31Z

tests/bluetooth/audio/bap_broadcast_source/src/main.c

@@ -228,6 +229,19 @@ ZTEST_F(bap_broadcast_source_test_suite, test_broadcast_source_create_start_send
 		for (size_t j = 0U; j < create_param->params[i].params_count; j++) {
 			struct bt_bap_stream *bap_stream = create_param->params[i].params[j].stream;

+			/* verify bap stream started cb stream parameter */
+			zassert_equal(mock_bap_stream_started_cb_fake.arg0_history[i], bap_stream);


I've been looking for a way to verify values in the fake callbacks - Thanks for finding a way to do this :D

Thalley · 2024-06-13T08:53:38Z

tests/bluetooth/audio/bap_broadcast_source/src/main.c

+			zassert_equal(bt_audio_codec_cfg_get_freq(codec_cfg),
+				      BT_AUDIO_CODEC_CFG_FREQ_16KHZ);
+			zassert_equal(bt_audio_codec_cfg_get_frame_dur(codec_cfg),
+				      BT_AUDIO_CODEC_CFG_DURATION_10);
+			/* verify bis specific codec data */
+			bt_audio_codec_cfg_get_chan_allocation(codec_cfg, &chan_allocation);
+			zassert_equal(chan_allocation,
+				      BT_AUDIO_LOCATION_FRONT_LEFT | BT_AUDIO_LOCATION_FRONT_RIGHT);


For this to properly test it, we should verify that the BIS specific data is not the same as the subgroup data.

The test is working as I can see that the location values are different and it's being checked properly here, but if we can a zassert_not_equal between the subgroup chan_alloc and the BIS chan_alloc, then we can ensure that it stays that way in case someone modifies the test

Thalley · 2024-06-13T08:53:43Z

tests/bluetooth/audio/bap_broadcast_source/src/main.c

+
+

Suggested change

Thalley · 2024-06-13T08:55:15Z

tests/bluetooth/audio/bap_broadcast_source/src/main.c

+		0x06,                                           /* bis cc length */
+		0x05, 0x03, 0x03, 0x00, 0x00, 0x00              /* bis cc length */


Thanks for fixing this bug :)

Thalley · 2024-06-13T09:11:07Z

tests/bsim/bluetooth/audio/src/bap_broadcast_source_test.c

+	const struct bt_audio_codec_cfg *codec_cfg = stream->codec_cfg;
+	const struct bt_audio_codec_cfg *exp_codec_cfg = &preset_16_1_1.codec_cfg;


I think it's the naming that throws me off here. You are setting

exp_codec_cfg = &preset_16_1_1.codec_cfg;

and the preset_16_1_1 is what we use for the subgroups.

We specifically do not expect the values of the subgroup, but rather the merged result of the subgroup and the BIS, and thus calling the subgroup's codec config for exp_codec_cfg does not sound right

zephyrbot added area: Bluetooth Audio area: Bluetooth labels May 17, 2024

zephyrbot requested review from asbjornsabo, Casper-Bonde-Bose, fredrikdanebjer, jhedberg, kruithofa, larsgk, MariuszSkamra, pin-zephyr, sjanc and Thalley May 17, 2024 04:16

zephyrbot assigned Thalley May 17, 2024

Thalley requested changes May 17, 2024

View reviewed changes

niym-ot force-pushed the 71125_store_bis_specific_info_to_stream_codec_config branch from fedbe23 to 7efc14e Compare May 22, 2024 05:53

niym-ot requested a review from Thalley May 22, 2024 05:56

niym-ot force-pushed the 71125_store_bis_specific_info_to_stream_codec_config branch from 7efc14e to 9c36397 Compare May 22, 2024 07:01

Thalley requested changes May 24, 2024

View reviewed changes

larsgk mentioned this pull request May 31, 2024

MAINTAINERS: Add niym-ot to Bluetooth Audio #73550

Merged

niym-ot force-pushed the 71125_store_bis_specific_info_to_stream_codec_config branch 2 times, most recently from 82af33a to 048a70b Compare June 6, 2024 13:34

niym-ot requested a review from Thalley June 7, 2024 04:53

Thalley requested changes Jun 7, 2024

View reviewed changes

niym-ot force-pushed the 71125_store_bis_specific_info_to_stream_codec_config branch from 048a70b to 2399859 Compare June 12, 2024 18:44

niym-ot requested a review from Thalley June 12, 2024 18:57

Thalley approved these changes Jun 13, 2024

View reviewed changes

larsgk approved these changes Jun 13, 2024

View reviewed changes

nashif merged commit df45858 into zephyrproject-rtos:main Jun 13, 2024
25 checks passed

aescolar mentioned this pull request Jun 13, 2024

2 BT audio tests fail to build in main #74261

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluetooth: BAP: Broadcast Source: Update stream codec config data #72908

Bluetooth: BAP: Broadcast Source: Update stream codec config data #72908

niym-ot commented May 17, 2024

Thalley left a comment

Thalley Jun 7, 2024

niym-ot Jun 12, 2024

Thalley Jun 13, 2024

Thalley Jun 7, 2024

niym-ot Jun 12, 2024

Thalley left a comment

Thalley Jun 13, 2024

Thalley Jun 13, 2024

Thalley Jun 13, 2024

Thalley Jun 13, 2024

Thalley Jun 13, 2024

		const struct bt_audio_codec_cfg *codec_cfg = stream->codec_cfg;
		const struct bt_audio_codec_cfg *exp_codec_cfg = &preset_16_1_1.codec_cfg;

		0x06, /* bis cc length */
		0x05, 0x03, 0x03, 0x00, 0x00, 0x00 /* bis cc length */

Bluetooth: BAP: Broadcast Source: Update stream codec config data #72908

Bluetooth: BAP: Broadcast Source: Update stream codec config data #72908

Conversation

niym-ot commented May 17, 2024

Thalley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thalley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment