Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LE Audio: BAP shell LC3 decode #68920

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

Thalley
Copy link
Collaborator

@Thalley Thalley commented Feb 13, 2024

Fixes #53527

@Thalley Thalley force-pushed the bap_shell_lc3_decode branch 6 times, most recently from 39b9785 to fa5a15a Compare February 14, 2024 16:06
@Thalley Thalley changed the title LE AUdio: BAP shell LC3 decode LE Audio: BAP shell LC3 decode Feb 26, 2024
@Thalley Thalley force-pushed the bap_shell_lc3_decode branch 6 times, most recently from 738579a to 5eda773 Compare March 1, 2024 11:36
@Thalley Thalley force-pushed the bap_shell_lc3_decode branch 4 times, most recently from ac3006d to 2413adc Compare March 11, 2024 09:12
@Thalley Thalley marked this pull request as ready for review March 11, 2024 10:55
@Thalley Thalley force-pushed the bap_shell_lc3_decode branch from 1920196 to 61d868a Compare March 13, 2024 21:48
kruithofa
kruithofa previously approved these changes Mar 14, 2024
@Thalley Thalley force-pushed the bap_shell_lc3_decode branch 3 times, most recently from b1f4980 to 5e32f4c Compare April 1, 2024 12:47
@Thalley Thalley requested a review from kruithofa April 1, 2024 12:47
shell_print(ctx_shell,
"Initializing the LC3 decoder with %u us duration and %u Hz frequency",
sh_stream->lc3_frame_duration_us, sh_stream->lc3_freq_hz);
/* Create the encoder instance. This shall complete before stream_started() is called. */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/* Create the encoder instance. This shall complete before stream_started() is called. */
/* Create the decoder instance. This shall complete before stream_started() is called. */

sh_stream->lc3_freq_hz, 0, /* No resampling */
&sh_stream->lc3_decoder_mem);
if (sh_stream->lc3_decoder == NULL) {
shell_error(ctx_shell, "Failed to setup LC3 encoder - wrong parameters?\n");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
shell_error(ctx_shell, "Failed to setup LC3 encoder - wrong parameters?\n");
shell_error(ctx_shell, "Failed to setup LC3 decoder - wrong parameters?\n");

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops - Too much copy-paste :)

@Thalley Thalley force-pushed the bap_shell_lc3_decode branch from 5e32f4c to 47dab1c Compare April 4, 2024 09:28
@Thalley Thalley requested a review from kruithofa April 4, 2024 09:29
kruithofa
kruithofa previously approved these changes Apr 4, 2024

if ((sh_stream->decoded_cnt % recv_stats_interval) == 0) {
shell_print(ctx_shell, "[%zu]: Performing PLC", sh_stream->decoded_cnt);
}
Copy link
Collaborator

@pin-zephyr pin-zephyr Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when data == NULL, should we continue to execute lc3_decode() ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL will perform PLC

sh_stream->lc3_frame_duration_us, sh_stream->lc3_freq_hz);
/* Create the decoder instance. This shall complete before stream_started() is called. */
sh_stream->lc3_decoder = lc3_setup_decoder(sh_stream->lc3_frame_duration_us,
sh_stream->lc3_freq_hz, 0, /* No resampling */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if the bap shell intended for non-USB device?

if not only for non-USB device, the sameple rate should not always 0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR does not touch USB, but only does decoding. My followup PRs add proper resampling for USB


#if defined(CONFIG_BT_BAP_BROADCAST_SINK)
struct shell_stream *sh_stream = shell_stream_from_bap_stream(stream);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about sh_stream == NULL ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shell_stream_from_bap_stream cannot return NULL, as it always returns an address.


if (default_broadcast_sink.stream_cnt == 0) {
/* All streams in the broadcast sink has been terminated */
default_broadcast_sink.syncable = true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to set default_broadcast_sink.syncable = true ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that doesn't make any sense, especially since we are setting it to false just below.

Must have been a bad merge conflict resolution or something :o

break;
}

frame_cnt += ret;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may have a code error, the frame_cnt should not add itself, because the ret = frame_cnt + chan_cnt.

for (uint8_t i = 0U; i < frame_blocks_per_sdu; i++) {
const int ret = decode_frame_block(buf, sh_stream, frame_cnt);

if (ret < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"ret" never have a chance could less than 0.

@Thalley Thalley force-pushed the bap_shell_lc3_decode branch from e8de570 to 842e220 Compare April 11, 2024 14:13
@Thalley Thalley requested a review from ChangNice April 11, 2024 14:13
frame_cnt++;
}

return frame_cnt;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function should return how many channnel decode success in one frame block, rather than the total frame_cnt.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, good catch

Add support for decoding incoming LC3 data. At this point
we only instantiate a single LC3 decoder, so only one frequency
and duration is supported.

To accomodate for the increased RAM usage, the number of unicast
streams support have been decreased.

Further more, the LC3 handling in the shell has overall been
improved, also for encoding.

Signed-off-by: Emil Gydesen <[email protected]>
@Thalley Thalley force-pushed the bap_shell_lc3_decode branch from 842e220 to 2d51bce Compare April 12, 2024 07:34
@Thalley Thalley requested a review from ChangNice April 12, 2024 07:34
@aescolar aescolar merged commit b4eac67 into zephyrproject-rtos:main Apr 16, 2024
24 checks passed
@Thalley Thalley deleted the bap_shell_lc3_decode branch April 16, 2024 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

LE Audio: Decode incoming LC3 encoded data in the audio shell
6 participants