Re: [pulseaudio-discuss] [PATCH v2 2/2] Bluetooth A2DP aptX codec support
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 */
> > +
Re: [pulseaudio-discuss] [PATCH v2 2/2] Bluetooth A2DP aptX codec support
On Tuesday 18 September 2018 13:55:08 Luiz Augusto von Dentz wrote: > Hi Pali, > > On Mon, Sep 17, 2018 at 3:27 PM, Tanu Kaskinen wrote: > > On Thu, 2018-09-13 at 11:12 +0200, Pali Rohár wrote: > >> On Wednesday 05 September 2018 13:57:08 Tanu Kaskinen wrote: > >> > > + 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? Now we have to hardcode the > >> > openaptx specific CFLAGS and LIBADD for libbluez5-util. If you ever > >> > need to add new flags, 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. > >> > >> Intension is that library is small and does not need any special cflags > >> or ldflags. So .pc file is not needed at all. And if library or include > >> file is in non-standard location then user really need to specify where > >> it is. But same argument can be used when .pc file is in non-standard > >> location. User again need to do some magic. > > Long term I think it is best to use autotools to properly generate the > .pc file, etc, otherwise it might be difficult for distros to pick > this up. I might be able to help you with that if you are willing to > accept patches. Because I know autotools, how to use it and how it works, I'm saying No. For small library I explicitly chose something which is easy and not big hell moloch. I really do not need anything special nor any custom or specific functionality. Also library has no dependences. -- Pali Rohár pali.ro...@gmail.com ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v2 2/2] Bluetooth A2DP aptX codec support
Hi Pali, On Mon, Sep 17, 2018 at 3:27 PM, Tanu Kaskinen wrote: > On Thu, 2018-09-13 at 11:12 +0200, Pali Rohár wrote: >> On Wednesday 05 September 2018 13:57:08 Tanu Kaskinen wrote: >> > > + 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? Now we have to hardcode the >> > openaptx specific CFLAGS and LIBADD for libbluez5-util. If you ever >> > need to add new flags, 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. >> >> Intension is that library is small and does not need any special cflags >> or ldflags. So .pc file is not needed at all. And if library or include >> file is in non-standard location then user really need to specify where >> it is. But same argument can be used when .pc file is in non-standard >> location. User again need to do some magic. Long term I think it is best to use autotools to properly generate the .pc file, etc, otherwise it might be difficult for distros to pick this up. I might be able to help you with that if you are willing to accept patches. -- Luiz Augusto von Dentz ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v2 2/2] Bluetooth A2DP aptX codec support
On Thu, 2018-09-13 at 11:12 +0200, Pali Rohár wrote:
> On Wednesday 05 September 2018 13:57:08 Tanu Kaskinen wrote:
> > > + 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? Now we have to hardcode the
> > openaptx specific CFLAGS and LIBADD for libbluez5-util. If you ever
> > need to add new flags, 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.
>
> Intension is that library is small and does not need any special cflags
> or ldflags. So .pc file is not needed at all. And if library or include
> file is in non-standard location then user really need to specify where
> it is. But same argument can be used when .pc file is in non-standard
> location. User again need to do some magic.
>
> > > +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?
>
> I do not know.
>
> > > +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.
>
> I thought that it would be better to not wrap above lines in another
> #ifdef, but rather function register_endpoint would do nothing when
> particular codec (e.g. aptX) was not enabled at compile time.
I thought that we'd end up telling bluetoothd that we support aptX when
we don't, but since register_endpoint() returns early, that's not an
issue. I still think #ifdefs are preferable here, since it makes it
obvious that the register_endpoint() calls don't do anything, but
that's pretty minor thing.
> > 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, order is important. Bluez based on registration order choose codec.
>
> > > @@ -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).
>
> ok.
>
> > > +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.
>
> What is the preferred way?
I suggest intializing p to NULL at the beginning of the function and
having pa_assert(p) before the *p = profile line.
Re: [pulseaudio-discuss] [PATCH v2 2/2] Bluetooth A2DP aptX codec support
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? Now we have to hardcode the
openaptx specific CFLAGS and LIBADD for libbluez5-util. If you ever
need to add new flags, 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.
> +
> +AS_IF([test "x$HAVE_BLUEZ_5" = "x1" && test "x$enable_aptx" = "xyes" && test
> "x$HAVE_OPENAPTX" = "x0"],
> +[AC_MSG_ERROR([*** libopenaptx from https://github.com/pali/libopenaptx
> not found])])
> +
> +AC_SUBST(HAVE_OPENAPTX)
> +AM_CONDITIONAL([HAVE_OPENAPTX], [test "x$HAVE_OPENAPTX" = "x1"])
> +AS_IF([test "x$HAVE_OPENAPTX" = "x1"], AC_DEFINE([HAVE_OPENAPTX], 1, [Have
> openaptx codec library]))
> +
> UDEV support (optional)
>
> AC_ARG_ENABLE([udev],
> @@ -1579,6 +1596,7 @@ AS_IF([test "x$HAVE_SYSTEMD_JOURNAL" = "x1"],
> ENABLE_SYSTEMD_JOURNAL=yes, ENABLE
> AS_IF([test "x$HAVE_BLUEZ_5" = "x1"], ENABLE_BLUEZ_5=yes, ENABLE_BLUEZ_5=no)
> AS_IF([test "x$HAVE_BLUEZ_5_OFONO_HEADSET" = "x1"],
> ENABLE_BLUEZ_5_OFONO_HEADSET=yes, ENABLE_BLUEZ_5_OFONO_HEADSET=no)
> AS_IF([test "x$HAVE_BLUEZ_5_NATIVE_HEADSET" = "x1"],
> ENABLE_BLUEZ_5_NATIVE_HEADSET=yes, ENABLE_BLUEZ_5_NATIVE_HEADSET=no)
> +AS_IF([test "x$HAVE_OPENAPTX" = "x1"], ENABLE_APTX=yes, ENABLE_APTX=no)
> AS_IF([test "x$HAVE_HAL_COMPAT" = "x1"], ENABLE_HAL_COMPAT=yes,
> ENABLE_HAL_COMPAT=no)
> AS_IF([test "x$HAVE_TCPWRAP" = "x1"], ENABLE_TCPWRAP=yes, ENABLE_TCPWRAP=no)
> AS_IF([test "x$HAVE_LIBSAMPLERATE" = "x1"], ENABLE_LIBSAMPLERATE="yes
> (DEPRECATED)", ENABLE_LIBSAMPLERATE=no)
> @@ -1637,6 +1655,7 @@ echo "
>Enable BlueZ 5: ${ENABLE_BLUEZ_5}
> Enable ofono headsets: ${ENABLE_BLUEZ_5_OFONO_HEADSET}
> Enable native headsets:${ENABLE_BLUEZ_5_NATIVE_HEADSET}
> +Enable aptX codec: ${ENABLE_APTX}
> Enable udev: ${ENABLE_UDEV}
>Enable HAL->udev compat: ${ENABLE_HAL_COMPAT}
> Enable systemd
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 411b9e5e5..bbd797589 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -2136,6 +2136,11 @@ libbluez5_util_la_SOURCES +=
> modules/bluetooth/pa-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/pa-a2dp-codec-aptx.c
> +libbluez5_util_la_LIBADD += -lopenaptx
> +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
> diff --git
