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.ro...@gmail.com
signature.asc
Description: PGP signature
_______________________________________________ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
