anonymix007 / libldacdec

A library for decoding audio in Sony's LDAC format
11 stars 7 forks source link

L2CAP receive buffer overflow #1

Open anonymix007 opened 3 years ago

anonymix007 commented 3 years ago

Dropping L2CAP data: receive buffer overflow error in dmesg. After a little research I've came to conclusion that it's my Android device that does things wrong: image Screenshot from 2021-09-05 21-19-40

Some L2CAP packets have length bigger than MTU. This causes playback to stop after few seconds.

anonymix007 commented 3 years ago

This, however, can be fixed:

  1. Bad solution: comment out goto drop; in net/bluetooth/l2cap_core.c, function l2cap_data_channel, next line after BT_ERR("Dropping L2CAP data: receive buffer overflow");
  2. Better one: Increase L2CAP_DEFAULT_MTU to >=673 in include/net/bluetooth/l2cap.h
  3. The best: Report to vendor and wait for fix (already done, but fix will probably never happen)

I've already done 2 and 3, issue will be closed then patches will be accepted upstream or vendor will fix it.

UPD 2 doesn't actually fixes the problem, MTU is still set to 672 according to Wireshark, so source devices which respect MTU will not be able to transfer packets.

javaman77 commented 2 years ago

I've managed to run LDAC sink on Raspberry Pi 3b with Volumio 2 (Debian Jessie). First approach was 1-5 secs of sound finished by "Segmentation fault".

Then i've recompied Bluez (version 5.23 - the same as was installed from repo before) increasing L2CAP_DEFAULT_MTU to 673 and then to 1023, it didn't fix the crash.

Then i did some debugging and found the reason - libldacdec retrieves compressed data from blues-alsa split into 655-byte pieces, interleaving with small 4-byte sequences. And seems like these short chunks do not have sound data at all - maybe it's a LDAC service messages. I've fixed a2dp-ldac.c, method a2dp_ldac_dec_thread, main loop like this:

size_t frames = rtp_media_header->frame_count;

/* added from this line */
if (rtp_payload_len == 4){
    continue; // skip short chunk
}
/* to this line */

while (frames--){
    int used;
    int decoded;
...

Now libldacdec doesn't try to decode these 4-byte chunks, and now it works stable - 1 hour of listening without crash. Don't know if this is only Samsung's codec feature with this extra short packets.

BUT: still there are some problems.

  1. Playback is unstable. Sometimes it drops frames from stream, or replaces music content by short portion of sine wave (like short beep). Maybe these 4-byte packets still contain sound data which is important.
  2. Playback is very sensitive to any activities on the phone - there are hangs and drops when doing something on the phone.
  3. I've tried to measure bitrate, made a traffic dump on Raspberry by hcidump avdtp --raw --save-dump=/home/volumio/ldac.bin - output file is always approx 2.8 Mb per minute, which means 350-370 kbps, independing of bitrate changing in Dev options. This is most unpleasant issue with LDAC - there is no reason to develop this decoder when we can use stable AptX with the same bitrate. Dont know maybe it's a "feature" of Samsung A50 with Android 10, will try later on another A50 with Anoroid 11.
anonymix007 commented 2 years ago

Then i've recompied Bluez (version 5.23 - the same as was installed from repo before) increasing L2CAP_DEFAULT_MTU to 673 and then to 1023, it didn't fix the crash.

You don't need to recompile bluez, but rather kernel.

Then i did some debugging and found the reason - libldacdec retrieves compressed data from blues-alsa split into 655-byte pieces, interleaving with small 4-byte sequences. And seems like these short chunks do not have sound data at all - maybe it's a LDAC service messages. I've fixed a2dp-ldac.c, method a2dp_ldac_dec_thread, main loop like this:

size_t frames = rtp_media_header->frame_count;

/* added from this line */
if (rtp_payload_len == 4){
    continue; // skip short chunk
}
/* to this line */

while (frames--){
    int used;
    int decoded;
...

Don't know if this is only Samsung's codec feature with this extra short packets.

Yes, I tried it on A51 (or was it A50? I don't remember) and had the same result. My OnePlus 7 Pro works fine (except MTU issue, but custom kernel fixed that). Would be great if you'd send this patch to bluez-alsa. Maybe also report to Samsung.

BUT: still there are some problems.

  1. Playback is unstable. Sometimes it drops frames from stream, or replaces music content by short portion of sine wave (like short beep). Maybe these 4-byte packets still contain sound data which is important.

I haven't encountered such problems, but expect 990 kbps mode to be unstable. This is completely normal, even in official Sony products.

  1. Playback is very sensitive to any activities on the phone - there are hangs and drops when doing something on the phone.

That's really strange. On my phone the encoding is done on Hexagon DSP, so the CPU isn't used. Do you have similar problems with other phones?

  1. I've tried to measure bitrate, made a traffic dump on Raspberry by hcidump avdtp --raw --save-dump=/home/volumio/ldac.bin - output file is always approx 2.8 Mb per minute, which means 350-370 kbps, independing of bitrate changing in Dev options.

Well, that's Android 10 problem. The bitrate is affected by this setting on Android 11. You may want to look at this file and search of frame header. It should start with 0xAA, here's the code to decode the header:

    unsigned syncword             = (u.frame_header >> 24) & 255;
    unsigned sampling_rate_index  = (u.frame_header >> 21) & 7;
    unsigned channel_config_index = (u.frame_header >> 19) & 3;
    unsigned frame_length_index   = (u.frame_header >> 10) & 511;
    unsigned frame_status         = (u.frame_header >> 8)  & 3;
javaman77 commented 2 years ago

I've fixed drops and clicks and beeps by changing in the code above from

if (rtp_payload_len == 4){

to

if (rtp_payload_len <= 4){

Now playback is absolutely clean and stable. Some short hangs occur when using the phone, but very rare, maybe once in a minute.

About bitrate - now it suddently increased to about 740 kbps on the same A50 / Android 10! Don't know what happened, maybe because i moved from office to home with less noisy environment in BT range, will check tomorrow again. Thanks a lot for a library and assistance!

javaman77 commented 2 years ago

About bitrate - now it suddently increased to about 740 kbps on the same A50 / Android 10! Don't know what happened, maybe because i moved from office to home with less noisy environment in BT range, will check tomorrow again.

The reason why bitrate degrades from 660+ kbps to 330+ kbps is - active wireless access point on the Samsung A50, even when no devices connected to it. Seems like Android divides bandwidth between wifi and BT.

The bitrate is affected by this setting on Android 11

I found that it works on my A50 / Android 10 too, but it needs to switch bitrate AND something else after that - sampling or bit per sample. Only changing bitrate does not affect. Btw, bitrate 909/990 kbps is useless on my hardware - sound is totally messed up.

anonymix007 commented 2 years ago

The reason why bitrate degrades from 660+ kbps to 330+ kbps is - active wireless access point on the Samsung A50, even when no devices connected to it. Seems like Android divides bandwidth between wifi and BT.

This is not Android issue, it's more like hardware (and even physics) one. They share the frequency band, so even turning on the microwave (which is also 2.4GHz) will affect that. It may become better if you'll switch to 5GHz access point.

Btw, bitrate 909/990 kbps is useless on my hardware - sound is totally messed up.

There's no reason to use anything other than ABR, it will work fine in most cases. I had success with 990kbps, but usually it really depends on environment, so ABR is preferred.

javaman77 commented 2 years ago

Hi. Now LDAC works perfect, but i mentioned that it sounds quieter than other codecs (sbc, aac, aptx).

I have changed multiplifier 1<<15 to 1<<16 in libldacdec.c:

static void pcmFloatToInt( frame_t *this, int32_t *pcmOut )
{
    int i=0;
    for(int smpl=0; smpl<this->frameSamples; ++smpl )
    {
        for( int ch=0; ch<this->channelCount; ++ch, ++i )
        {
            pcmOut[i] = Round(this->channels[ch].pcm[smpl]*(1<<15)); // to 1 << 16
        }
    }
}

I didnt measure result output with oscilliscope, just used peak meter on my car stereo with test sine signal - now it shows the same level as other codecs, and i hear the same result.

But then some kind of clicks appeared on signal peaks - looks like result value exceeds pcm bit depth. After changed Round function to Floor - it plays just fine, no clicks: pcmOut[i] = floor(this->channels[ch].pcm[smpl]*(1<<16)); Don't know how precise is this new calculation, what you think?

anonymix007 commented 2 years ago

Don't know how precise is this new calculation, what you think?

Well, probably it is precise enough so it won't make any difference in real-world applications. But you may try to find some kind THD+N test and check how it performs. It would be great if you will open PR with all your modifications.

anonymix007 commented 1 year ago

Seems that the following patch to bluez fixes the issue:

diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index 10ef380d4..e24c8af73 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -2481,8 +2481,8 @@ static GIOChannel *l2cap_connect(struct avdtp *session, BtIOMode mode)
                                        BT_IO_OPT_PSM, AVDTP_PSM,
                                        BT_IO_OPT_MODE, mode,
                                        BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_MEDIUM,
-                                       /* Set Input MTU to 0 to auto-tune */
-                                       BT_IO_OPT_IMTU, 0,
+                                       /* Set Input MTU to 895 */
+                                       BT_IO_OPT_IMTU, 895,
                                        BT_IO_OPT_INVALID);
        else
                io = bt_io_connect(avdtp_connect_cb, session,

wireshark_mtu

sydragos commented 1 year ago

Seems that the following patch to bluez fixes the issue:

diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index 10ef380d4..e24c8af73 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -2481,8 +2481,8 @@ static GIOChannel *l2cap_connect(struct avdtp *session, BtIOMode mode)
                                        BT_IO_OPT_PSM, AVDTP_PSM,
                                        BT_IO_OPT_MODE, mode,
                                        BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_MEDIUM,
-                                       /* Set Input MTU to 0 to auto-tune */
-                                       BT_IO_OPT_IMTU, 0,
+                                       /* Set Input MTU to 895 */
+                                       BT_IO_OPT_IMTU, 895,
                                        BT_IO_OPT_INVALID);
        else
                io = bt_io_connect(avdtp_connect_cb, session,

wireshark_mtu

Can't seem to find when/where this patch went in. Still doesn't work on my Rpi zero W.

I just compiled everything today using latest, and switching phone to LDAC doesn't play anything. I'll redo it with --enable-debug, where should I look for logs/traces?

EDIT: aptX-HD works just fine. LDAC no sound, and no errors in the logs when pressing play/pause...

Apr 24 21:40:44 rpi bluealsa[2004]: bluealsa: [2004] D: ../../src/bluez.c:1401: Signal: org.freedesktop.DBus.Properties.PropertiesChanged(): org.bluez.MediaTransport1: State
Apr 24 21:40:44 rpi bluealsa[2004]: bluealsa: [2004] D: ../../src/ba-transport.c:723: New A2DP transport: 16
Apr 24 21:40:44 rpi bluealsa[2004]: bluealsa: [2004] D: ../../src/ba-transport.c:724: A2DP socket MTU: 16: R:672 W:1024
Apr 24 21:40:44 rpi bluealsa[2004]: bluealsa: [2004] D: ../../src/bluez.c:1401: Signal: org.freedesktop.DBus.Properties.PropertiesChanged(): org.bluez.MediaTransport1: State
Apr 24 21:40:44 rpi bluealsa[2004]: bluealsa: [2004] D: ../../src/ba-transport.c:1508: Starting transport: A2DP Sink (LDAC)
Apr 24 21:40:44 rpi bluealsa[2004]: bluealsa: [2004] D: ../../src/ba-transport.c:430: Created BT socket duplicate: [16]: 14
Apr 24 21:40:44 rpi bluealsa[2004]: bluealsa: [2004] D: ../../src/ba-transport.c:1924: Created new IO thread [ba-a2dp-ldac]: A2DP Sink (LDAC)
Apr 24 21:40:44 rpi bluealsa[2004]: bluealsa: [4772] D: ../../src/a2dp-ldac.c:332: IO loop: START: a2dp_ldac_dec_thread: A2DP Sink (LDAC)
Apr 24 21:40:55 rpi bluealsa[2004]: bluealsa: [2004] D: ../../src/bluez.c:1401: Signal: org.freedesktop.DBus.Properties.PropertiesChanged(): org.bluez.MediaTransport1: State
Apr 24 21:40:55 rpi bluealsa[2004]: bluealsa: [4772] D: ../../src/ba-transport.c:444: Closing BT socket duplicate [16]: 14
Apr 24 21:40:55 rpi bluealsa[2004]: bluealsa: [4772] D: ../../src/ba-transport.c:777: Closing A2DP transport: 16
Apr 24 21:40:55 rpi bluealsa[2004]: bluealsa: [4772] D: ../../src/ba-transport.c:1958: Exiting IO thread [ba-a2dp-ldac]: A2DP Sink (LDAC)
anonymix007 commented 1 year ago

@sydragos This patch is for bluez. However, the latest proposed patch is for kernel (applied on top of 6.2.12):

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index eebe25610..46a056db6 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1825,7 +1825,8 @@ static void l2cap_sock_init(struct sock *sk, struct sock *parent)
                       break;
               }

-               chan->imtu = L2CAP_DEFAULT_MTU;
+               /* Set MTU to 0 so L2CAP can auto select the MTU */
+               chan->imtu = 0;
               chan->omtu = 0;
               if (!disable_ertm && sk->sk_type == SOCK_STREAM) {
                       chan->mode = L2CAP_MODE_ERTM;

Logs should be appearing in dmesg.

sydragos commented 1 year ago

I've finally recompiled the kernel 6.1.25+, on WSL2 had to use /tmp file system because NTFS mounts were causing issues. Recompiled bluez-alsa with:

../configure --disable-hcitop --with-alsaplugindir=/usr/lib/arm-linux-gnueabihf/alsa-lib --enable-aac \
  --enable-aptx --enable-aptx-hd --with-libopenaptx --enable-ldac && \
make && sudo make install

And still, phone connects as LDAC but there's no audio. Error:

Apr 25 14:59:57 moode bluetoothd[1048]: src/service.c:btd_service_connect() audio-avrcp-target profile connect failed for 0C:C4:***: Input/output error

EDIT: what kernel version are you using?

anonymix007 commented 1 year ago

Maybe WSL is your problem? You error looks different compared to one discussed in this issue. I've tried different kernel versions (starting from 5.10 to 6.2), all of them could have LDAC properly working. Actually, I used different patches for that, but the patch I'm going to push to upstream is going to be chan->imtu = 0;.

sydragos commented 1 year ago

Thank you for the swift replies! Maybe 6.1.y latest has some untested issues.

I'll try the 6.1.21 hash 0afb5e98488aed7017b9bf321b575d0177feb7ed since it's supposed to be the rpi 6.1.21+ src.

Otherwise I might just have to compile the kernel directly on the rpi if it doesn't work.

anonymix007 commented 1 year ago

I built kernel for RPi 4 (quite some time ago) on Linux PC just fine. Your issue might be caused by your bluez-alsa configure options. Try to use ones from readme, they were tested on Pi 4.

sydragos commented 1 year ago

Rebuilt kernel 6.1.21 on ubuntu, followed your readme flags for bluez-alsa, and I still cannot get any audio out. It's probably not related to your or bluez-alsa repo, but to whatever patches moOde makes on raspiOS drivers and kernel.

anonymix007 commented 1 year ago

Same error? BTW, do other codecs work? I'll try to replicate the steps I used for RPi 4 and write (a bit better) instructions on the weekend. Until then, you might try using RPi OS Lite

sydragos commented 1 year ago

It's so weird, maybe I should create a new issue. With moOde 6.1.21 unmodded kernel, every other codec except LDAC works.

With modified rpi 6.1.21 kerrnel removing L2CAP_DEFAULT_MTU, no codec works. I'm about to go through moode's imgbuild script and see where how he changes any kernel stuff.