On Wednesday 05 September 2018 13:57:08 Tanu Kaskinen wrote: > On Sat, 2018-07-28 at 17:34 +0200, Pali Rohár wrote: > > This patch provides support for aptX codec in bluetooth A2DP profile. In > > pulseaudio it is implemented as a new profile a2dp_aptx_sink. For aptX > > encoding it uses open source LGPLv2.1+ licensed libopenaptx library which > > can be found at https://github.com/pali/libopenaptx. > > > > Limitations: > > > > Codec selection (either SBC or aptX) is done by bluez itself and it does > > not provide API for switching codec. Therefore pulseaudio is not able to > > change codec and it is up to bluez if it decide to use aptX or not. > > > > Only standard aptX codec is supported for now. Support for other variants > > like aptX HD, aptX Low Latency, FastStream may come up later. > > --- > > configure.ac | 19 ++ > > src/Makefile.am | 5 + > > src/modules/bluetooth/a2dp-codecs.h | 118 ++++++++++- > > src/modules/bluetooth/bluez5-util.c | 48 ++++- > > src/modules/bluetooth/bluez5-util.h | 2 + > > src/modules/bluetooth/module-bluez5-device.c | 65 +++++- > > src/modules/bluetooth/pa-a2dp-codec-aptx.c | 297 +++++++++++++++++++++++++++ > > src/modules/bluetooth/pa-a2dp-codec.h | 1 + > > 8 files changed, 548 insertions(+), 7 deletions(-) > > create mode 100644 src/modules/bluetooth/pa-a2dp-codec-aptx.c > > > > diff --git a/configure.ac b/configure.ac > > index d2bfab23b..c2d13fa53 100644 > > --- a/configure.ac > > +++ b/configure.ac > > @@ -1094,6 +1094,23 @@ AC_SUBST(HAVE_BLUEZ_5_NATIVE_HEADSET) > > AM_CONDITIONAL([HAVE_BLUEZ_5_NATIVE_HEADSET], [test "x$HAVE_BLUEZ_5_NATIVE_HEADSET" = x1]) > > AS_IF([test "x$HAVE_BLUEZ_5_NATIVE_HEADSET" = "x1"], AC_DEFINE([HAVE_BLUEZ_5_NATIVE_HEADSET], 1, [Bluez 5 native headset backend enabled])) > > > > +#### Bluetooth A2DP aptX codec (optional) ### > > + > > +AC_ARG_ENABLE([aptx], > > + AS_HELP_STRING([--disable-aptx],[Disable optional bluetooth A2DP aptX codec support (via libopenaptx)])) > > + > > +AS_IF([test "x$HAVE_BLUEZ_5" = "x1" && test "x$enable_aptx" != "xno"], > > + [AC_CHECK_HEADER([openaptx.h], > > + [AC_CHECK_LIB([openaptx], [aptx_init], [HAVE_OPENAPTX=1], [HAVE_OPENAPTX=0])], > > + [HAVE_OPENAPTX=0])]) > > Have you considered providing a .pc file? I will think about it (again). > Now we have to hardcode the > openaptx specific CFLAGS and LIBADD for libbluez5-util. As a first step, I will remove hardcoded CFLAGS and LIBADD from libbluez5-util. In autoconf, everything is possible to discover, so really no need to hardcode and let autoconf find them. > If you ever need to add new flags, This is something which is not going to happen. > all openaptx users need to update their build > systems. Also, if the library is installed to a non-standard location, > the .pc file can set the -L and -I flags to point to the right place. > > diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c > > index 9c4e3367b..c139f7fc3 100644 > > --- a/src/modules/bluetooth/bluez5-util.c > > +++ b/src/modules/bluetooth/bluez5-util.c > > @@ -50,7 +50,9 @@ > > #define BLUEZ_ERROR_NOT_SUPPORTED "org.bluez.Error.NotSupported" > > > > #define A2DP_SOURCE_SBC_ENDPOINT "/MediaEndpoint/A2DPSourceSBC" > > +#define A2DP_SOURCE_APTX_ENDPOINT "/MediaEndpoint/A2DPSourceAPTX" > > #define A2DP_SINK_SBC_ENDPOINT "/MediaEndpoint/A2DPSinkSBC" > > +#define A2DP_SINK_APTX_ENDPOINT "/MediaEndpoint/A2DPSinkAPTX" > > > > #define ENDPOINT_INTROSPECT_XML \ > > DBUS_INTROSPECT_1_0_XML_DOCTYPE_DECL_NODE \ > > @@ -173,8 +175,22 @@ static bool device_supports_profile(pa_bluetooth_device *device, pa_bluetooth_pr > > switch (profile) { > > case PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK: > > return !!pa_hashmap_get(device->uuids, PA_BLUETOOTH_UUID_A2DP_SINK); > > + case PA_BLUETOOTH_PROFILE_A2DP_APTX_SINK: > > +#ifdef HAVE_OPENAPTX > > + /* TODO: Implement once bluez provides API to check if codec is supported */ > > + return !!pa_hashmap_get(device->uuids, PA_BLUETOOTH_UUID_A2DP_SINK); > > Is someone working on that API? Yes, Luiz posted patches to bluez mailing list, and I will use this API in new patch version. Also I will add fallback code, so users of "old" bluez would be still able to use pulseaudio, just with same limitation as now (no codec switching). > > +#else > > + return false; > > +#endif > > case PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE: > > return !!pa_hashmap_get(device->uuids, PA_BLUETOOTH_UUID_A2DP_SOURCE); > > + case PA_BLUETOOTH_PROFILE_A2DP_APTX_SOURCE: > > +#ifdef HAVE_OPENAPTX > > + /* TODO: Implement once bluez provides API to check if codec is supported */ > > + return !!pa_hashmap_get(device->uuids, PA_BLUETOOTH_UUID_A2DP_SOURCE); > > +#else > > + return false; > > +#endif And all these #ifdefs are removed in new patch version. There would be no codec specific code. > > @@ -961,7 +977,9 @@ static void parse_interfaces_and_properties(pa_bluetooth_discovery *y, DBusMessa > > if (!a->valid) > > return; > > > > + register_endpoint(y, path, A2DP_SOURCE_APTX_ENDPOINT, PA_BLUETOOTH_UUID_A2DP_SOURCE); > > register_endpoint(y, path, A2DP_SOURCE_SBC_ENDPOINT, PA_BLUETOOTH_UUID_A2DP_SOURCE); > > + register_endpoint(y, path, A2DP_SINK_APTX_ENDPOINT, PA_BLUETOOTH_UUID_A2DP_SINK); > > register_endpoint(y, path, A2DP_SINK_SBC_ENDPOINT, PA_BLUETOOTH_UUID_A2DP_SINK); > > We shouldn't register aptX endpoints if aptX support is not enabled. Yes, fixing this via new/update pulseaudio codec API. > Does the registration order matter? I have some vague recollection from > the earlier discussion that BlueZ chooses the codec based on the > endpoint registration order - is that true? If the order is important, > we should document that here. Yes, you are right. New/updated pulseaudio codec API contains priority list, so this code is updated to respect codec API priority. > > @@ -1652,7 +1694,9 @@ pa_bluetooth_discovery* pa_bluetooth_discovery_get(pa_core *c, int headset_backe > > } > > y->matches_added = true; > > > > + endpoint_init(y, PA_BLUETOOTH_PROFILE_A2DP_APTX_SINK); > > endpoint_init(y, PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK); > > + endpoint_init(y, PA_BLUETOOTH_PROFILE_A2DP_APTX_SOURCE); > > endpoint_init(y, PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE); > > We shouldn't set up endpoints for aptX if the aptX support is not > enabled. Fixing it. > > > > get_managed_objects(y); > > @@ -1721,7 +1765,9 @@ void pa_bluetooth_discovery_unref(pa_bluetooth_discovery *y) { > > if (y->filter_added) > > dbus_connection_remove_filter(pa_dbus_connection_get(y->connection), filter_cb, y); > > > > + endpoint_done(y, PA_BLUETOOTH_PROFILE_A2DP_APTX_SINK); > > endpoint_done(y, PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK); > > + endpoint_done(y, PA_BLUETOOTH_PROFILE_A2DP_APTX_SOURCE); > > endpoint_done(y, PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE); > > If endpoint_init() is made conditional, these need to be made > conditional too (according to the dbus docs, unregistering an object > path that hasn't been registered first is considered an application > bug). Yes, fixing it. > > diff --git a/src/modules/bluetooth/bluez5-util.h b/src/modules/bluetooth/bluez5-util.h > > index 365b9ef6f..29d862fe1 100644 > > --- a/src/modules/bluetooth/bluez5-util.h > > +++ b/src/modules/bluetooth/bluez5-util.h > > @@ -55,7 +55,9 @@ typedef enum pa_bluetooth_hook { > > > > typedef enum profile { > > PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK, > > + PA_BLUETOOTH_PROFILE_A2DP_APTX_SINK, > > PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE, > > + PA_BLUETOOTH_PROFILE_A2DP_APTX_SOURCE, > > PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT, > > PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY, > > PA_BLUETOOTH_PROFILE_OFF This profile enum would be dynamically constructed, so bluez5-util would not contain codec specific data. > > diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c > > index e626e80e9..e8a07d067 100644 > > --- a/src/modules/bluetooth/module-bluez5-device.c > > +++ b/src/modules/bluetooth/module-bluez5-device.c > > @@ -1757,6 +1776,18 @@ static pa_card_profile *create_card_profile(struct userdata *u, pa_bluetooth_pro > > p = PA_CARD_PROFILE_DATA(cp); > > break; > > > > + case PA_BLUETOOTH_PROFILE_A2DP_APTX_SOURCE: > > + cp = pa_card_profile_new(name, _("High Fidelity aptX Capture (A2DP Source)"), sizeof(pa_bluetooth_profile_t)); > > + cp->priority = 25; > > + cp->n_sinks = 0; > > + cp->n_sources = 1; > > + cp->max_sink_channels = 0; > > + cp->max_source_channels = 2; > > + pa_hashmap_put(input_port->profiles, cp->name, cp); > > + > > + p = PA_CARD_PROFILE_DATA(cp); > > + break; > > + > > My compiler started to be unsure whether p is always initialized: > > CC modules/bluetooth/module_bluez5_device_la-module-bluez5-device.lo > modules/bluetooth/module-bluez5-device.c: In function ‘create_card_profile’: > modules/bluetooth/module-bluez5-device.c:1821:8: warning: ‘p’ may be used uninitialized in this function [-Wmaybe-uninitialized] > *p = profile; > ~~~^~~~~~~~~ > > This is a false positive, but we should do something to make the > compiler shut up. Please look at compile warnings after I send new patch series. I'm testing compilation on Debian 9 (with default gcc compiler) and currently I do not have warnings. So if if you find something in patch version, let me know. > > @@ -1907,6 +1940,28 @@ static int add_card(struct userdata *u) { > > pa_hashmap_put(data.profiles, cp->name, cp); > > } > > > > + PA_HASHMAP_FOREACH(uuid, d->uuids, state) { > > + pa_bluetooth_profile_t profile; > > + > > + /* FIXME: PA_BLUETOOTH_UUID_A2DP_SINK maps to both SBC and APTX */ > > + /* FIXME: PA_BLUETOOTH_UUID_A2DP_SOURCE maps to both SBC and APTX */ > > + if (uuid_to_profile(uuid, &profile) < 0) > > + continue; > > + > > + /* Handle APTX */ > > + if (profile == PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK) > > + profile = PA_BLUETOOTH_PROFILE_A2DP_APTX_SINK; > > + else if (profile == PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE) > > + profile = PA_BLUETOOTH_PROFILE_A2DP_APTX_SOURCE; > > + else > > + continue; > > + > > + if (!pa_hashmap_get(data.profiles, pa_bluetooth_profile_to_string(profile))) { > > + cp = create_card_profile(u, profile, data.ports); > > + pa_hashmap_put(data.profiles, cp->name, cp); > > + } > > + } > > + > > We shouldn't create the card profile if aptX support is disabled. Yes, this will be handled in new codec API in new patch version. > > pa_assert(!pa_hashmap_isempty(data.profiles)); > > > > cp = pa_card_profile_new("off", _("Off"), sizeof(pa_bluetooth_profile_t)); > > diff --git a/src/modules/bluetooth/pa-a2dp-codec-aptx.c b/src/modules/bluetooth/pa-a2dp-codec-aptx.c > > new file mode 100644 > > index 000000000..8ce1fc67c > > --- /dev/null > > +++ b/src/modules/bluetooth/pa-a2dp-codec-aptx.c > > +static void pa_aptx_fill_blocksize(void **info_ptr, size_t read_link_mtu, size_t write_link_mtu, size_t *read_block_size, size_t *write_block_size) { > > + *write_block_size = (write_link_mtu/(8*4)) * 8*4*6; > > + *read_block_size = (read_link_mtu/(8*4)) * 8*4*6; > > Please add comments that explain the math here. Adding comment into code. -- Pali Rohár pali.rohar@xxxxxxxxx
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ pulseaudio-discuss mailing list pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
