search

zephyrproject-rtos / liblc3codec

LC3 codec implementation
62 stars 36 forks source link

Fix Encoder Crash for 8_1 (8kHz, 7.5ms, 26 octets) #5

Closed mringwal closed 2 years ago

mringwal commented 2 years ago

Trying to encode with 8_1 I got a memory exception. When comparing the code to the LC3 Specification 1.0, it looks like the missing term caused the crash. After matching the code to the spec, the encoder does not crash anymore.

aescolar commented 2 years ago

Thanks @mringwal Please note that all contributions need to be signed-off as per the DCO (https://docs.zephyrproject.org/latest/contribute/index.html#developer-certification-of-origin-dco) to be considered.

mringwal commented 2 years ago

Thanks @mringwal Please note that all contributions need to be signed-off as per the DCO (https://docs.zephyrproject.org/latest/contribute/index.html#developer-certification-of-origin-dco) to be considered.

You're welcome. Is the additional "signed-off" in the force-pushed commit message sufficient?

Casper-Bonde-Bose commented 2 years ago

@mringwal the LC3 codec was replaced with the latest version from Android (a different c-based implementation). Would it be possible for you to check if this fix is also needed for the new codec implementation? (Note it moved to the branch called Zephyr)

mringwal commented 2 years ago

@Casper-Bonde-Bose Well, I only found the issue as it crashed on me and the EHIMA implementation was kind enough to directly point to the pseudo code in the LC3 Spec. In the new implementation, there's no reason to believe the same error (typo) exists as it's different. Anyway, the same logic is here: https://github.com/zephyrproject-rtos/liblc3codec/blob/zephyr/src/sns.c#L260 and it looks correct to me. I didn't update yet.

A bit of topic: why was the codec replaced? What's better besides from not requiring a C++ compiler (which is a good thing)? Is it faster?

Casper-Bonde-Bose commented 2 years ago

A bit of topic: why was the codec replaced? What's better besides from not requiring a C++ compiler (which is a good thing)? Is it faster?

@mringwal The original codec was reuse from Android (contributed by EHIMA), but was based on the 0.9 LC3 specification - hence not the adopted 1.0 version. Android replaced the EHIMA version with this C-version, and to get a 1.0 version and to keep them aligned we brought it into Zephyr as well. With regards to performance I did not compare - never managed to get the cpp-version running on nRF5430, but this new codec uses 50% of the CPUAPP core (running at default settings - not sure which speed that is) - this is for the default 16kHz 32kbps setting.

mringwal commented 2 years ago

@Casper-Bonde-Bose Thanks for the information. I've missed the bit that the first version already came from Android. In this case, let me close this PR as it won't be used anywhere (we have the fix in our copy, but won't need it when updating to the v1.0 version).

Assuming you were referring to a mono signal at 16 kHz, it looks like running LC3 for 48_6_2 on the nRF5340 will require some decent profiling to get at least 3x optimization... glad we'll only need to run the codec on our desktop machines for now :)

Casper-Bonde-Bose commented 2 years ago

@mringwal Yes mono, I also only run on a PC for now using the native_posix build and an attached HCI BT controller - it runs 48kHz stereo encoding to two CIS'es just fine. Before I enabled HW floating point support it used 500% of the CPU - hence it took 50ms to encode 10ms of audio :-) But yes optimization is needed - unless the nRF5340 CPU runs at low speed by default. The intention of adding an LC3 codec was to enable use of it for test on a PC - I actually did not think it would run on the nRF5340 or similar platforms at all.