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

samples: Bluetooth: Few fixups for USB data for broadcast audio sink #69086

Merged
merged 1 commit into from
Mar 1, 2024

Conversation

Thalley
Copy link
Collaborator

@Thalley Thalley commented Feb 16, 2024

This commit does several but minor changes to the USB handling of
the broadcast audio sink, such as improving logging, removing a few
unncessary pieces of code and some minor performance improvements.

It removes some uses of the ring buffers, which effectively clears
up around 30KB of RAM, while also reducing how much memory is being
copied, improving performance.

Some of this removes the existing code for partial support for
stereo, but that code did not work in the first place. Proper stereo
support will be added in a later commit.

jhedberg
jhedberg previously approved these changes Feb 21, 2024
@Thalley Thalley added the DNM This PR should not be merged (Do Not Merge) label Feb 21, 2024
@Thalley
Copy link
Collaborator Author

Thalley commented Feb 21, 2024

Adding DNM until review/approval from @fredrikdanebjer :)

@Thalley
Copy link
Collaborator Author

Thalley commented Feb 21, 2024

Fixed a minor copy-size issue that caused only half of the decoded data to reach USB, so now it works (again) :)

@Thalley Thalley requested a review from jhedberg February 21, 2024 09:04
@Thalley Thalley force-pushed the broadcast_sink_usb_fixup branch from 38a3ad2 to d5efa1d Compare February 21, 2024 10:48
This commit does several but minor changes to the USB handling of
the broadcast audio sink, such as improving logging, removing a few
unncessary pieces of code and some minor performance improvements.

It removes some uses of the ring buffers, which effectively clears
up around 30KB of RAM, while also reducing how much memory is being
copied, improving performance.

Some of this removes the existing code for partial support for
stereo, but that code did not work in the first place. Proper stereo
support will be added in a later commit.

Signed-off-by: Emil Gydesen <[email protected]>
@Thalley Thalley force-pushed the broadcast_sink_usb_fixup branch from d5efa1d to 9764d00 Compare February 21, 2024 10:50
@Thalley Thalley requested a review from tmon-nordic February 27, 2024 07:26
desowin

This comment was marked as duplicate.

Copy link
Contributor

@tmon-nordic tmon-nordic left a comment

Choose a reason for hiding this comment

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

I don't know Bluetooth and therefore I cannot really review it properly. The only thing, that is not modified by this PR, is the fact that there will inevitably be buffer underruns or overflows because the sink and source are not synchronized (the old USB stack claims synchronous synchronization, which means that 1 SOF = reference 1 ms, but this is false because Bluetooth is running on independent clock).

@fredrikdanebjer
Copy link
Contributor

Some of this removes the existing code for partial support for
stereo, but that code did not work in the first place. Proper stereo
support will be added in a later commit.

It seems to me that what you mean with this is that you removed the (non-working) support for CONFIG_TARGET_BROADCAST_CHANNEL ? Do you plan to add this? Do we need to add a note in the Kconfig about this?

Beside this, LGTM

@Thalley
Copy link
Collaborator Author

Thalley commented Feb 29, 2024

It seems to me that what you mean with this is that you removed the (non-working) support for CONFIG_TARGET_BROADCAST_CHANNEL ? Do you plan to add this? Do we need to add a note in the Kconfig about this?

Beside this, LGTM

@fredrikdanebjer CONFIG_TARGET_BROADCAST_CHANNEL still works as that is either LEFT or RIGHT or MONO, so it is still capable of getting the right BIS for that and mixing that one channel into a stereo USB stream.

There are 2 followup PRs:

  1. samples: Bluetooth: Broadcast sink: Add proper parsing decoding of LC3 data #69340 Adding additional LC3 validation
  2. samples: Bluetooth: Add stereo support for broadcast audio sink #69341 Implementing proper stereo support

@Thalley Thalley removed the DNM This PR should not be merged (Do Not Merge) label Feb 29, 2024
@fredrikdanebjer
Copy link
Contributor

@fredrikdanebjer CONFIG_TARGET_BROADCAST_CHANNEL still works as that is either LEFT or RIGHT or MONO, so it is still capable of getting the right BIS for that and mixing that one channel into a stereo USB stream.

But you are saying that it can take the right channel, assuming that the stream received is a one channel stream. This removes support where you have two channels on a single BIS, and then only want to listen to one channel? (but admittedly it wasn't working with 2-channel BIS to only use the Right channel).
It looks to me as if you always only take the first LC3_MAX_NUM_SAMPLES_MONO bytes, regardless of channel allocation.

But that this is handled in some part in:
#69341

@Thalley
Copy link
Collaborator Author

Thalley commented Mar 1, 2024

@fredrikdanebjer

This removes support where you have two channels on a single BIS, and then only want to listen to one channel?

That part was never working anyways, as it never took the frame blocks into account (fixed by #69340).

As mentioned, this PR does actually not remove any actual functionality, as the functionality it does remove was never working correctly in the first place. So this is a cleanup PR, then followed by #69340 which then adds proper support for decoding a single channel with LC3 in a single BIS, and then #69341 which adds stereo support (either from 1 or 2 BIS)

@aescolar aescolar merged commit 0051731 into zephyrproject-rtos:main Mar 1, 2024
14 of 15 checks passed
@Thalley Thalley deleted the broadcast_sink_usb_fixup branch March 1, 2024 13:30
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.

8 participants