[pulseaudio-discuss] [PATCH v13 05/10] bluetooth: Add A2DP aptX and aptX HD codecs support
Pali Rohár
pali.rohar at gmail.com
Sat Nov 30 22:39:18 UTC 2019
On Saturday 30 November 2019 22:43:47 Georg Chini wrote:
> On 06.10.19 19:58, Pali Rohár wrote:
> > This patch provides support for aptX and aptX HD codecs in bluetooth A2DP
> > profile. It uses open source LGPLv2.1+ licensed libopenaptx library which
> > can be found at https://github.com/pali/libopenaptx.
> >
> > aptX for s24 stereo samples provides fixed 6:1 compression ratio and
> > bitrate 352.8 kbit/s, aptX HD provides fixed 4:1 compression ratio and
> > bitrate 529.2 kbit/s.
> >
> > According to soundexpert research, aptX codec used in bluetooth A2DP is no
> > better than SBC High Quality settings. And you cannot hear difference
> > between aptX and SBC High Quality, aptX is just a copper-less overpriced
> > audio cable.
> >
> > aptX HD is high-bitrate version of aptX. It has clearly noticeable increase
> > in sound quality (not dramatic though taking into account the increase in
> > bitrate).
> >
> > http://soundexpert.org/news/-/blogs/audio-quality-of-bluetooth-aptx
>
> One general remark: I would consider passing the a2dp_codec as argument
> to the API functions. This way you can avoid having to create one function
> per codec variant. Instead you can use the vendor ID/codec ID or just the
> codec name as a key to a "case" or "if" instruction to distinguish between
> different codec variants. I think especially in the SBC case the code would
> be better readable. In this patch you could avoid duplicating functions
> if you can read the vendor and codec ID from the a2dp_codec instead of
> hard coding them.
I do not think that passing codec structure into every function is a big
win. In your suggestion, instead of N functions (one for each codec) you
would have just one function with N if-then-else-else-else... branches,
one branch for each codec. Currently common parts for all codecs is
already in sub-function with suffix _common (so code is not duplicated).
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index b84c778cc..e317b7972 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -2164,6 +2164,12 @@ libbluez5_util_la_SOURCES += modules/bluetooth/a2dp-codec-sbc.c
> > libbluez5_util_la_LIBADD += $(SBC_LIBS)
> > libbluez5_util_la_CFLAGS += $(SBC_CFLAGS)
> > +if HAVE_OPENAPTX
> > +libbluez5_util_la_SOURCES += modules/bluetooth/a2dp-codec-aptx.c
> > +libbluez5_util_la_CPPFLAGS += $(OPENAPTX_CPPFLAGS)
> > +libbluez5_util_la_LDFLAGS += $(OPENAPTX_LDFLAGS)
> > +endif
> > +
> > module_bluez5_discover_la_SOURCES = modules/bluetooth/module-bluez5-discover.c
> > module_bluez5_discover_la_LDFLAGS = $(MODULE_LDFLAGS)
> > module_bluez5_discover_la_LIBADD = $(MODULE_LIBADD) $(DBUS_LIBS) libbluez5-util.la
>
> The part for the meson build system is missing.
Sorry, I do not understand meson build system deeply enough to write
needed rule for it (including searching & linking flags).
Can somebody help with this part?
>
> > diff --git a/src/modules/bluetooth/a2dp-codec-aptx.c b/src/modules/bluetooth/a2dp-codec-aptx.c
> > new file mode 100644
> > index 000000000..2bd9e7652
> > --- /dev/null
> > +++ b/src/modules/bluetooth/a2dp-codec-aptx.c
...
> > +
> > +static bool fill_preferred_configuration_common(const pa_sample_spec *default_sample_spec, const a2dp_aptx_t *capabilities, a2dp_aptx_t *config, uint32_t vendor_id, uint16_t codec_id) {
>
> The return type should be int not bool. Functions that return an error
> always return int
> in PA (negative value for error, >=0 for success).
Ok, I can change it.
>
> > + int i;
> > +
> > + static const struct {
> > + uint32_t rate;
> > + uint8_t cap;
> > + } freq_table[] = {
> > + { 16000U, APTX_SAMPLING_FREQ_16000 },
> > + { 32000U, APTX_SAMPLING_FREQ_32000 },
> > + { 44100U, APTX_SAMPLING_FREQ_44100 },
> > + { 48000U, APTX_SAMPLING_FREQ_48000 }
> > + };
> > +
> > + if (A2DP_GET_VENDOR_ID(capabilities->info) != vendor_id || A2DP_GET_CODEC_ID(capabilities->info) != codec_id) {
> > + pa_log_error("No supported vendor codec information");
> > + return false;
> > + }
> > +
> > + config->info = A2DP_SET_VENDOR_ID_CODEC_ID(vendor_id, codec_id);
> > +
> > + if (!(capabilities->channel_mode & APTX_CHANNEL_MODE_STEREO)) {
> > + pa_log_error("No supported channel modes");
> > + return false;
> > + }
> > +
> > + config->channel_mode = APTX_CHANNEL_MODE_STEREO;
> > +
> > + /* Find the lowest freq that is at least as high as the requested sampling rate */
> > + for (i = 0; (unsigned) i < PA_ELEMENTSOF(freq_table); i++) {
> > + if (freq_table[i].rate >= default_sample_spec->rate && (capabilities->frequency & freq_table[i].cap)) {
> > + config->frequency = freq_table[i].cap;
> > + break;
> > + }
> > + }
> > +
> > + if ((unsigned) i == PA_ELEMENTSOF(freq_table)) {
> > + for (--i; i >= 0; i--) {
> > + if (capabilities->frequency & freq_table[i].cap) {
> > + config->frequency = freq_table[i].cap;
> > + break;
> > + }
> > + }
> > +
> > + if (i < 0) {
>
> This should never happen, so I think you can use an assertion.
Hm... good question. Maybe it could happen when capabilities buffer does
not contain any valid frequency in specified freq_table?
>
> > + pa_log_error("Not suitable sample rate");
> > + return false;
> > + }
> > + }
> > +
> > + return true;
> > +}
> > +
> > +static uint8_t fill_preferred_configuration(const pa_sample_spec *default_sample_spec, const uint8_t *capabilities_buffer, uint8_t capabilities_size, uint8_t config_buffer[MAX_A2DP_CAPS_SIZE]) {
>
> Return type should be size_t and function should return (size_t) -1 on
> error.
No, it cannot. API expects that zero value is returned on error.
Also maximal size of A2DP cap buffer is 255 bytes, so it does not make
sense to use bigger types. It is A2DP specific.
>
> > + a2dp_aptx_t *config = (a2dp_aptx_t *) config_buffer;
> > + const a2dp_aptx_t *capabilities = (const a2dp_aptx_t *) capabilities_buffer;
> > +
> > + if (capabilities_size != sizeof(*capabilities)) {
> > + pa_log_error("Invalid size of capabilities buffer");
> > + return 0;
> > + }
> > +
> > + pa_zero(*config);
> > +
> > + if (!fill_preferred_configuration_common(default_sample_spec, capabilities, config, APTX_VENDOR_ID, APTX_CODEC_ID))
> > + return 0;
> > +
> > + return sizeof(*config);
> > +}
> > +
> > +static uint8_t fill_preferred_configuration_hd(const pa_sample_spec *default_sample_spec, const uint8_t *capabilities_buffer, uint8_t capabilities_size, uint8_t config_buffer[MAX_A2DP_CAPS_SIZE]) {
>
> Same here.
>
>
> > + a2dp_aptx_hd_t *config = (a2dp_aptx_hd_t *) config_buffer;
> > + const a2dp_aptx_hd_t *capabilities = (const a2dp_aptx_hd_t *) capabilities_buffer;
...
> > +
> > + PA_ONCE_BEGIN {
> > +#if OPENAPTX_MAJOR == 0 && OPENAPTX_MINOR == 0 && OPENAPTX_PATCH == 0
> > + /* libopenaptx version 0.0.0 does not export version global variables */
> > + const int aptx_major = OPENAPTX_MAJOR;
> > + const int aptx_minor = OPENAPTX_MINOR;
> > + const int aptx_patch = OPENAPTX_PATCH;
> > +#endif
>
> This does not make sense because it is evaluated at compile time. So if
> compiled on a system with version 0.0.0, it will never show anything else
> and
That is truth. But we know that this could happen. And I think that for
debugging purposes it make sense to print version.
> if compiled on a system with a higher version it will crash when run
> on a system with 0.0.0. So I would skip displaying the version.
Now question is if such thing is even supported. Compile program with
new version of library and then run it with older. I guess that more
libraries would crash in this case...
>
> > + pa_log_debug("Using aptX codec implementation: libopenaptx %d.%d.%d from https://github.com/pali/libopenaptx", aptx_major, aptx_minor, aptx_patch);
> > + } PA_ONCE_END;
> > +
> > + return aptx_c;
> > +}
...
> > +static size_t encode_buffer(void *codec_info, uint32_t timestamp, const uint8_t *input_buffer, size_t input_size, uint8_t *output_buffer, size_t output_size, size_t *processed) {
> > + struct aptx_context *aptx_c = (struct aptx_context *) codec_info;
> > + size_t written;
> > +
> > + *processed = aptx_encode(aptx_c, input_buffer, input_size, output_buffer, output_size, &written);
> > + if (PA_UNLIKELY(*processed == 0 || *processed != input_size))
> > + pa_log_error("aptX encoding error");
>
> Should that only produce a log message or does it indicate a severe error?
It is error. And because *processed is set to different value as
input_size, caller would handle this error (stop encoding and drop A2DP
connection).
>
> > +
> > + return written;
> > +}
...
> > +static size_t decode_buffer(void *codec_info, uint32_t *timestamp, const uint8_t *input_buffer, size_t input_size, uint8_t *output_buffer, size_t output_size, size_t *processed) {
> > + struct aptx_context *aptx_c = (struct aptx_context *) codec_info;
> > + size_t written;
> > +
> > + *processed = aptx_decode(aptx_c, input_buffer, input_size, output_buffer, output_size, &written);
> > +
> > + /* Due to aptX latency, aptx_decode starts filling output buffer after 90 input samples.
> > + * If input buffer contains less than 90 samples, aptx_decode returns zero (=no output)
> > + * but set *processed to non zero as input samples were processed. So do not check for
> > + * return value of aptx_decode, zero is valid. Decoding error is indicating by fact that
> > + * not all input samples were processed. */
> > + if (PA_UNLIKELY(*processed != input_size))
> > + pa_log_error("aptX decoding error");
>
> Should this only produce a log message or does it indicate a severe error?
Same here.
>
> > +
> > + return written;
> > +}
--
Pali Rohár
pali.rohar at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20191130/270d9068/attachment.sig>
More information about the pulseaudio-discuss
mailing list