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

Enable aptX (HD) decoding support using standalone codec extracted from ffmpeg. #253

Closed
wants to merge 4 commits into from

Conversation

t123yh
Copy link

@t123yh t123yh commented Sep 30, 2019

This is powered by a simple library ffaptx that I extracted from ffmpeg source code. I tested both aptX and aptX HD from my OnePlus 6, and it seems to be working fine.

I believe encoding support with this library can be added with little effort (and thus eliminate the need for openaptx library), but as I don't have a aptX receiver I can't test it. Hope somebody can implement it.

@t123yh t123yh mentioned this pull request Oct 1, 2019
@joerg-krause
Copy link
Contributor

https://github.com/pali/libopenaptx also derives the aptx code from the FFmpeg project. Why another library?

Furthermore, the aptx decoder samples contain 24 bit signed integers stored as 32 bit signed integers. Your code right-shifts the 24 bit samples into 16 bit samples, am I right?

@dmcallejo
Copy link

dmcallejo commented Oct 8, 2019

Using Raspbian Stretch (9.11) and default apt repositories I found this issue compiling ffaptx:

CMake Error at CMakeLists.txt:1 (cmake_minimum_required):
CMake 3.14 or higher is required.  You are running version 3.7.2`

So I had to upgrade it manually. This took more than an hour on a Pi 3.

After this, I configured it with ../configure --enable-aptx-sink --enable-aac --enable-ldac --disable-hcitop --with-alsaplugindir=/usr/lib/arm-linux-gnueabihf/alsa-lib but unfortunately my Mi 9T Pro won't connect to the sink.
Tested around with different combination of configurations and whenever I put --enable-aptx-sink the phone won't connect.

I'll check if could be something about my setup.

Edit.: Apparently I'm missing libffaptx.so.0.0.1 in runtime.
Edit2.: Compiled ffaptx with -DCMAKE_INSTALL_PREFIX:PATH=/usr (default is /usr/local/) and now systemd is able to start bluealsa. Phone is connected with aptX HD and streaming. Volume control doesn't work very well (there are like three steps) and is a bit in the low side. I will continue testing.

@t123yh
Copy link
Author

t123yh commented Oct 9, 2019

@joerg-krause I didn't notice that library before, but whatever :)
As for the 16-bit problem, bluez-alsa operates at 16-bit samples everywhere, so I have to change the sample to 16-bit. If not so, I would have to modify code throughout the project to make it 24-bit capable, which is a huge amount of work and seems unnecessary.

@dmcallejo I'll change the minimum CMake version required.
The volume problem might associate with the ignored aptX HD RTP header, I guess? I'll have a look soon. Meanwhile you can test whether the volume for aptX non-HD is working, as it doesn't contain RTP header.

Edit: it's not caused by RTP header. Anyway, please test aptX behaviour.

@gpeter12
Copy link

gpeter12 commented Oct 13, 2019

After my previous observation (#241 (comment)) I found that a lot of parity check error happens at the time of the crackling noise and much much more error follows it. It stucks in the error.

To reproduce it just put the following code in the int ff_aptx_decode_samples(AptXContext *ctx, const uint8_t *input, int32_t samples[NB_CHANNELS][4]) function after ret = aptx_check_parity(ctx->channels, &ctx->sync_idx);

if(ret != 0) 
{
    error("PARITY ERROR!!!");
}

The first parity error happens after 3-5 minutes of audio playing.

Isn't there an automatic error correction feature in the data link layer of bluetooth? If the answer is yes then how can I turn it on?

@joerg-krause
Copy link
Contributor

@t123yh Downscaling 24-bit to 16-bit means you're losing quality.

@gpeter12 This probably means that you're not receiving all packets, so the aptx synchronization fails. You can add some synchronization handling, e.g. like pali did in his aptx decoder.

@dmcallejo
Copy link

Hi @t123yh , I've been testing extensively the build and I can confirm that there's no bug with the volume control. I've used it for long sessions (4 hours of continuous play) and no problems at all.

@gpeter12
Copy link

Hi @t123yh,
I recently bought a new 24bit DAC for the RPi Zero W. Could you make it work without the right-shift in the io_thread_a2dp_sink_aptx_aptxhd() function? When I change the audio format in the aplay.c I get nothing but white noise (I used the SND_PCM_FORMAT_S24_3LE constant because that is what my DAC supports).

Thank you for your help.

@t123yh
Copy link
Author

t123yh commented Dec 1, 2019

@gpeter12 The internal of bluez-alsa uses 16bit samples. We will have to change all shorts to ints in order to enjoy 24-bit quality.

There's one thing I'm not very sure of: why this could not get merged. Maybe I should rebase it to the latest master?

@arkq
Copy link
Owner

arkq commented Dec 1, 2019

There's one thing I'm not very sure of: why this could not get merged. Maybe I should rebase it to the latest master?

Firstly, sorry for not responding earlier :/. The reason why this PR has not been merged is mainly due to maintenance problem. This project for the last two years has been used commercially with the requirement of apt-X support. Unfortunately, ffmpeg can not be used in such way due to legal issues. In order to "allow" community to use apt-X encoding, there is a shim library openaptx, which implements commercial apt-X API (API is not a patent subject) - one needs to adopt open-source encoder to such API and you are ready to go (breaking the law, though). The problem with decoding is, that the API has not been publicly available and the only one I'm aware of is released under NDA.

So first of all switching to ffmpeg for encoding is not an option because it will make hard to maintain commercial forks. Secondly, I could mainstream decoding support but first of all I have to be sure that the API has been publicly released by Qualcomm so I'm not breaking NDA (more or less).

For apt-X HD there is also mentioned 24-bit issue. Some work has been already done by @joerg-krause, you might check his fork. However, I think that for dynamic bit-with support there is still a long way to go - mostly due to required architectural changes.

@t123yh
Copy link
Author

t123yh commented Dec 1, 2019

Does that mean it's best to close this PR and maintain a "illegal" fork myself?

@arkq
Copy link
Owner

arkq commented Dec 1, 2019

I think that this PR might still be there, so if someone is looking for apt-X (HD), your fork could be easily found. Also, it will be a right place to notify subscribers about any progress with this feature :)

@gpeter12
Copy link

gpeter12 commented Dec 2, 2019

... Some work has been already done by @joerg-krause, you might check his fork. ...

@arkq I searched for the commit but i didn't find it. :(

@pali
Copy link

pali commented Mar 14, 2020

Hello! My https://github.com/pali/libopenaptx has some fixes related to both encoding and decoding process, specially related to algorithm delay. Also in decoder I implemented simple rescue and re-synchronization. So funcionality in my libopenaptx should be better then in proprietary qualcom implementation. Also affected patents for this code already expired, so there should not be any legal problems with either using or redistributing libopenaptx code. If you found some problems in libopenaptx, please report issue to my repository.

@pali
Copy link

pali commented Apr 11, 2020

@gpeter12 wrote:

Isn't there an automatic error correction feature in the data link layer of bluetooth?

I asked this question year ago on linux-bluetooth ML, see thread:
https://lore.kernel.org/linux-bluetooth/20190518190618.m7rdkthvpz4agxd2@pali/T/#u

If the answer is yes then how can I turn it on?

Nope, it should be turned off! Reliable sockets (like TCP) are unsuitable for real-time audio/video transports. If it is turned on then you would hear of of glitches, big and variable latency and have audio unsynchronized.

In real-time streaming it is up to the codec to do error correction, not for transport layer. Transport layer should be low latency and should not try to retransmit packets on error.

@joerg-krause wrote:

@gpeter12 This probably means that you're not receiving all packets, so the aptx synchronization fails. You can add some synchronization handling, e.g. like pali did in his aptx decoder.

Now I released a new version of libopenaptx 0.2.0 which contains new functions in libopenaptx.so library for synchronization support, so caller just needs to call new function and do not need to do anything with parity check or syncing. Also code for synchronization handling in libopenaptx is improved. I did simulation, took aptx stream, removed some bytes from random places and play it. And there is no crackling noise anymore.

So libopenaptx 0.2.0 implementation should be more suitable for bluetooth streaming (both encoder and decoder deals with algorithm delay and decoder supports synchronization) compared to ffmpeg implementation or proprietary qualcomm android implementation.

If you think that there is something missing in libopenaptx or have suggestions for improvement, please let me know.

@janro1
Copy link

janro1 commented May 2, 2020

Has someone already tried libopenaptx 0.2.0 with bluez-alsa or is working on that?

@gpeter12
Copy link

gpeter12 commented May 2, 2020

@pali The crackling noise problem was because of my hardware choice. It turned out that the Raspberry Pi Zero W has a very low quality built in Bluetooth module. I bought a new USB Bluetooth stick and the problem was completely eliminated. Now the sound is perfect. I can’t hear any distortion or noise even after hours of usage.

Currently I’m using @t123yh ’s code on the RPi, but I will try libopenaptx 0.2.0 code too because @t123yh ’s code has about 1.5 second latency.

Sorry for the late reply.

@pali
Copy link

pali commented Aug 8, 2020

@janro1 for libopenaptx integration into bluez-alsa see PR: #356

@t123yh
Copy link
Author

t123yh commented Aug 8, 2020

It's nice to see all of these:) Closing this for #356.

@t123yh t123yh closed this Aug 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants