Add support for LHDC V3 A2DP source and sink #672
Conversation
anonymix007
force-pushed
the
lhdc
branch
3 times, most recently
from
971397e to
b8598eb
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #672 +/- ##
==========================================
+ Coverage 70.46% 70.49% +0.02%
==========================================
Files 96 96
Lines 15997 15999 +2
Branches 2509 2510 +1
==========================================
+ Hits 11272 11278 +6
+ Misses 4608 4604 -4
Partials 117 117 ☔ View full report in Codecov by Sentry. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
anonymix007
force-pushed
the
lhdc
branch
4 times, most recently
from
854ed5a to
568280b
Compare
|
I still have no idea why this happens even with BA_TRANSPORT_PCM_FORMAT_S32_4LE. Apparently, some variables in |
|
I'd like to test it locally. From where I can get the ldhcBT-dec/enc libraries? If this PR were to go to master I need to have a possibility to test the code for potential regressions. |
|
UPDATED Nov 13, 2024 Source code for libraries will be available later. LHDC_INCLUDE_DIR=/path/to/headers
export LHDC_DEC_CFLAGS="-I$LHDC_INCLUDE_DIR/liblhdcdec/include -I$LHDC_INCLUDE_DIR/liblhdcdec/inc -L$LHDC_INCLUDE_DIR"
export LHDC_DEC_LIBS="-llhdcBT_dec"
export LHDC_ENC_CFLAGS="-I$LHDC_INCLUDE_DIR/liblhdc/include -I$LHDC_INCLUDE_DIR/liblhdc/inc -L$LHDC_INCLUDE_DIR"
export LHDC_ENC_LIBS="-llhdcBT_enc"
autoreconf --install
mkdir build && cd build
../configure --enable-aac --enable-lhdc --enable-debug
make
sudo make install |
anonymix007
changed the title
|
In the last few days I've done long overdue refactoring around A2DP codecs. Now, the logic is kept mostly in one file, which should simplify maintenance and addition of new codecs. Due to extensive changes, I've rebased your work on top of current master. The code seems to compile, but I've got no time yet to test whether it works :D Also, I've left some commented code (I'm not sure whether it will be required or not). In the A2DP configuration for LHDC you've added |
|
Most constants were directly copied from AOSP patches. This must be a remnant of that. Seems to be unused, so might be removed probably. The commented out code seems to be for channel mode, so could be removed as well. Another issue would be to implement LLAC/V3/V4 selection logic, which is basically the following:
At least sink seems to still work. |
|
I've tried to run it once again, but now it doesn't seem to work (even AAC doesn't work). or from root: Probably related to #681, though I haven't recompiled PipeWire, just renamed /usr/lib/spa-0.2/bluez5. My phone also says "Problem connecting. Turn device off & back on", but I definitely can see audio packets in WireShark. UPD sudo bluealsa-cli open /org/bluealsa/hci0/dev_XX_XX_XX_XX_XX_XX/a2dpsnk/source | play -e signed-integer -b 16 -c 2 -r 44100 -t raw -But it segfaults with LHDC: |
anonymix007
force-pushed
the
lhdc
branch
2 times, most recently
from
ce802b3 to
12725d6
Compare
It seems that the problem is somewhere within encoder code. The reason of abort is heap corruption due to |
|
I was testing the decoder, but encoder having the same issue is really strange. None of them should call Do you have any suggestions for coexisting with pipewire? |
I've added a test code which encodes PCM and then decodes it, so due to encoder failure I was not able to check decoder, not yet :D
What do you mean "coexisting"? If you have BlueALSA and PipeWire running on the same host at the same time, then you should disable (somehow) Bluetooth support in PipeWire (I'm not sure whether renaming BlueZ plugin is sufficient, it might be though). Then, remove paired device and pair it again, so BlueZ will not reuse cached SDP information. EDIT: make check TESTS=
CK_FORK=no valgrind ./test/test-io lhdc-v3 |
Though, I guess, for debugging this issue I might just run that test. |
I guess that you have to use PCM playback which is not grabbed by PipeWire or use some playback PCM exposed by PipeWire itself (if there is any). However, I'm not PipeWire user (not yet), so I've got zero knowledge about PipeWire configuration. Anyway, the error code "Host is down" is kinda strange... |
|
Apparently, it doesn't like the following line: And this is exactly what original library does. So this, per say, is not a library error. Should be fixed now. |
OK, now the test passes but only with the old version of the library. With the new one (UPDATED Mar 1, 2024) encoder produces only 78 bytes per packet, where the old one produces 270 bytes. It looks like the output is truncated because these 78 bytes matches with the first 78 bytes in the packet from the old encoder. |
|
@anonymix007 Do you have any plans of opensourcing the library? I'm OK with the library being closed-sorce and even commercial, but the case is that I'd like to merge this PR sooner or later because syncing it is a burden I'd like to avoid. However, I need some way of verifying whether the codec code compiles and the unit test passes. I'm mostly developing on arm64, so it would be more convenient for me to have arm64 libraries instead of x64 (sometimes I'm doing some checks on x64, so such version is also required). If you don't want to opensource the lib and you don't want to release arm64 builds to the public, could you at least send them to me (on my gmail address)? If there will be no way for me to verify the code during further development of bluez-alsa I'm not going to merge it, because it would be impossible for me to maintain it... I hope that you understand that :) |
|
Yes, I'm going to publish all the sources once V5 encoding library starts working. Maybe I should publish V2/V3/V4/LLAC already even in the current state, but I haven't decided yet. I kinda want to make them at ABI-compatible with latest official libraries (4.1.0). Encoder is more or less at parity with 4.0.6p1, while decoder is in a worse state being only a bit better than 4.0.2, but the latest official LHDC libraries aren't available anywhere. Sure, uploaded aarch64 builds along with the same x86-64 versions to #672 (comment). Please let me know if they work, these were compiled on aarch64 (X Elite) without any special flags. I wasn't able to make bluez-alsa work at all last time I checked, so I shifted efforts to pipewire and ExtA2DP implementations and both worked fine, so you may use them for tests. |
I was able to compile LHDC support and run tests, but it seems that something is not right with the encoder/decoder itself. When running IO test, the encoder produced payloads with lengths: 266, 262, 250, 230, 226, 222, 222, 214 So, it's strange that payload length is not constant. After decoding it I have almost all silence... I can see one warning reported by the library:
And yes, I've used MTU greater than that in the IO test, so maybe that's the case... Making MTU smaller yields different warning:
I will have to verify with old x64 libraries to see whether the problem is on bluez-alsa side or maybe with the arm64 version, or in general with your new libraries... EDIT: |
|
@anonymix007 I've been trying to figure out what is wrong with the encoder/decoder, so I've created a simple recoder which feeds audio through lhdc_encode and lhdc_decode. Unfortunately, the code does not work as expected (logic is copied from the src/a2dp-lhdc.c). If you have some time, could you have a look at it? Maybe you will find out what is wrong with the usage of LHDC API. You can compile the code from my https://github.com/arkq/bluez-alsa/tree/lhdc branch as follows:
I've check with valgrind and it reports lots of |
|
Also, I have another question. Is the decoder thread safe? I see that there is |
|
I'll take a look at these issues.
I'm quite sure it is not. However, there's not much I can do about it without changing ABI. This is what the original library does. I guess I can provide separate thread-safe API version if it's really needed. |
|
I wrote a few simple utils for testing my library, you may refer to them for the library usage. |
|
Your test program is wrong in many ways, in fact, it doesn't even compile (though I tried compiling it alongside liblhdc, not in bluez-alsa): lhdcrecode.c: In function ‘main’:
lhdcrecode.c:186:50: error: passing argument 2 of ‘lhdcBT_encode_stereo’ from incompatible pointer type [-Wincompatible-pointer-types]
186 | if (lhdcBT_encode_stereo(handle, pcm_ch1, pcm_ch2, encoded_data.data, &encoded, &frames) < 0) {
| ^~~~~~~
| |
| int16_t * {aka short int *}
In file included from lhdcrecode.c:10:
liblhdc/inc/lhdcBT.h:359:58: note: expected ‘int *’ but argument is of type ‘int16_t *’ {aka ‘short int *’}
359 | int lhdcBT_encode_stereo(HANDLE_LHDC_BT hLhdcParam, int *left_pcm, int *right_pcm, unsigned char *out_put, uint32_t *written, uint32_t *out_frames);
| ~~~~~^~~~~~~~
lhdcrecode.c:186:59: error: passing argument 3 of ‘lhdcBT_encode_stereo’ from incompatible pointer type [-Wincompatible-pointer-types]
186 | if (lhdcBT_encode_stereo(handle, pcm_ch1, pcm_ch2, encoded_data.data, &encoded, &frames) < 0) {
| ^~~~~~~
| |
| int16_t * {aka short int *}
liblhdc/inc/lhdcBT.h:359:73: note: expected ‘int *’ but argument is of type ‘int16_t *’ {aka ‘short int *’}
359 | ncode_stereo(HANDLE_LHDC_BT hLhdcParam, int *left_pcm, int *right_pcm, unsigned char *out_put, uint32_t *written, uint32_t *out_frames);
| ~~~~~^~~~~~~~~
lhdcrecode.c:209:35: error: passing argument 1 of ‘lhdcBT_dec_decode’ from incompatible pointer type [-Wincompatible-pointer-types]
209 | lhdcBT_dec_decode(&encoded_data, encoded_data_len, decoded_pcm, &decoded_len, bit_depth);
| ^~~~~~~~~~~~~
| |
| struct <anonymous> *
In file included from lhdcrecode.c:11:
liblhdcdec/inc/lhdcBT_dec.h:24:38: note: expected ‘const uint8_t *’ {aka ‘const unsigned char *’} but argument is of type ‘struct <anonymous> *’
24 | int lhdcBT_dec_decode(const uint8_t *frameData, uint32_t frameBytes, uint8_t* pcmData, uint32_t* pcmBytes, uint32_t bits_depth);
| ~~~~~~~~~~~~~~~^~~~~~~~~
lhdcrecode.c:209:68: error: passing argument 3 of ‘lhdcBT_dec_decode’ from incompatible pointer type [-Wincompatible-pointer-types]
209 | lhdcBT_dec_decode(&encoded_data, encoded_data_len, decoded_pcm, &decoded_len, bit_depth);
| ^~~~~~~~~~~
| |
| int16_t * {aka short int *}
liblhdcdec/inc/lhdcBT_dec.h:24:79: note: expected ‘uint8_t *’ {aka ‘unsigned char *’} but argument is of type ‘int16_t *’ {aka ‘short int *’}
24 | ode(const uint8_t *frameData, uint32_t frameBytes, uint8_t* pcmData, uint32_t* pcmBytes, uint32_t bits_depth);
| ~~~~~~~~~^~~~~~~With these warnings and a few other logic errors fixed, it appears to be working.
It is not. LHDC V2/V3/V4 are quite similar to LossyFLAC/LossyWavpack, all are by default VBR. Bitrate only defines the upper limit, actual bitrate might be lower. For example, it'll always encode "constant frames" into 12 bytes or something about it. I think you may set
I guess I shouldn't have made this a warning... This is normal, library just reports the actual MTU used internally.
And so did I. Fortunately, I now have a good arm64 machine so it's quite easy for me to test it.
I'm quite sure it won't be in most cases, though it should sound the same. LHDC is a lossy codec. |
It might be, however, it should compile without I have more questions, though (in order to better understand the API). What is the purpose of the |
The are some implication of that, though. BlueALSA runs a dedicated thread for encoder/decoder for every connected transport. So, in case when two A2DP source devices will try to use LHDC, bad things will happen... The encoder looks thread-safe, but it would be nice to know it for sure. As for decoder, I will have to add some restrictions, or maybe you can mark decoder global state as thread_local (C11), so at least for cases where there is a single decoder instance per-thread (like in bluealsa), there will be no side effects (with your copy of the library). I can also add some warning message which will warn user about the thread-safety concerns regarding the LHDC decoder library. |
Well, it probably isn't either. Worked fine in AOSP which also creates a separate thread for encoding though.
There is more than variable for the global state and I don't really want to fix theoretical issue while there are more practical ones. I might do something about this after the public release though, but for now let's assume that only 1 instance of LHDC encoder/decoder could be running in the same process and maybe add some runtime checks for that. |
Thanks for the explanation. As for support for 32, I think that I will produce s32_le for both encoder and decoder anyway. It will allow easier internal testing (my entire testing infrastructure relies on decoder and encoder be able to consume/produce the same PCM sample format). So, for now I will add right-shift for produces 24-bit output.
As far as I know Android is not able to stream audio to more than one remote device at a time. So, for Android that's not a problem. |
|
I was able to test the codec on my side. It seems that everything works as expected (within tested scope). The a2dp-lhdc.c code was OK after all, but the issue was with my test harness. However, after understanding how the codec API works I was able to fix that. Many thank for contribution! Also as a side note. I've seen that the lhdcBT.h file contains commented out |
|
@anonymix007 I have another question :D Is the decoder capable of decoding data encoded by devices with support for LHDC-v2 A2DP capabilities type? I have a Huawei phone which seems to support that. So, I've made an adaptation layer for LHDC-v2: https://github.com/arkq/bluez-alsa/tree/lhdc-v2 However, it seems that the codec payload is bit different than in case of v3. There is some header though, because second byte is a counter. Here is an example of captured payload when playing some sound followed by a silence (without RTP header), maybe you will spot some syncword or something in these bytes :D : EDIT: EDIT2: |
It should be capable, V3 is V2+lpc+different frame size and V4 is basically V3+LLAC. I definitely tested that once though, but something might've broke since then.
There is more than one header: first is 2 bytes (
|
OK, so something is not right, because only silence is decoded (and some random other frames). Do you have some real data with encoded v3 (from some real retail device)? I'd like to see a single frame with RTP header to double check that lhdc media header is correct in bluez-alsa code base. |
|
Funnily enough, I don't have real retail device with LHDC. My phone doesn't support it out of the box (though is seems that LHDC is somewhat added in a newer version), so I'm using ExtA2DP. You may refer to the official AOSP integration to verify that payload header is correct. Looks good to me though. Will capture a few frames a bit later. |
But looking here it seems that the LHDC 2-bytes header is in fact a byte header (number of frames + latency) followed by a byte sequence counter, which matches the header that I've got from my phone (which is a real retail device with LHDC v2 support). So this structure seems to be incorrect after all. |
I do have a phone and a headset.
Offtopic here, but as a pipewire user, I can't wait for introducing LHDC support in pipewire! Thanks all your effort! 🙏 |
|
@xabolcs If you have a device which supports that I'd be glad if you could dump Bluetooth traffic from your phone when streaming short (1-2 seconds) audio from your phone to headset which supports LHDC. You can use this https://www.wireshark.org/docs/man-pages/androiddump.html or any manufacturer specicif way to obtain HCI log from android. If you have a device with bluealsa, you can use this patch (applied on top of https://github.com/arkq/bluez-alsa/tree/lhdc-v2 branch) to capture incoming packets: diff --git a/src/a2dp-lhdc.c b/src/a2dp-lhdc.c
index da394c4..5c67989 100644
--- a/src/a2dp-lhdc.c
+++ b/src/a2dp-lhdc.c
@@ -422,6 +422,8 @@ void *a2dp_lhdc_dec_thread(struct ba_transport_pcm *t_pcm) {
if ((rtp_lhdc_media_header = rtp_a2dp_get_payload(rtp_header)) == NULL)
continue;
+ hexdump("PAYLOAD", rtp_header, len);
+
int missing_rtp_frames = 0;
rtp_state_sync_stream(&rtp, rtp_header, &missing_rtp_frames, NULL);
diff --git a/src/bluez.c b/src/bluez.c
index d4a0d69..d17332a 100644
--- a/src/bluez.c
+++ b/src/bluez.c
@@ -1367,6 +1367,7 @@ static void bluez_signal_interfaces_added(GDBusConnection *conn, const char *sen
debug("Adding new Stream End-Point: %s: %s: %s",
batostr_(&addr), sep_cfg.type == A2DP_SOURCE ? "SRC" : "SNK",
a2dp_codecs_codec_id_to_string(sep_cfg.codec_id));
+ hexdump("SEP capabilities", &sep_cfg.capabilities, sep_cfg.caps_size);
GArray *sep_cfgs = bluez_adapter_get_device_sep_configs(&bluez_adapters[dev_id], &addr);
g_array_append_val(sep_cfgs, sep_cfg);Compile bluez-alsa with debug enabled: |
|
hci_dump_intel.zip |
|
@O2C14 thanks for the dump! So, the header for v3 is the same as for v2, where the first byte is a "header" and the second one is a sequence. |
|
@O2C14 I've just updated the https://github.com/arkq/bluez-alsa/tree/lhdc-v2 branch, if you have a system with bluez-alsa on it could you verify that LHDC v3 works as expected? Use setup from #672 (comment) to compile bluez-alsa with LHDC. Then run it as |
Closes #588
Source does not work, only some buzzing noise is produced instead of music. I've tried saving pcm.data as raw pcm and it seems to be incorrect already.It seems to be somewhat working now, but not yet perfect.Library expects interleaved 24 or 16 bit data depending on selected bit depth, so probably related to the use of BA_TRANSPORT_PCM_FORMAT_S24_4LE.