Re: [pulseaudio-discuss] [PATCH v2 2/2] Bluetooth A2DP aptX codec support

2018-09-05 Thread Tanu Kaskinen
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 

Re: [pulseaudio-discuss] [PATCH v2 1/2] Modular API for Bluetooth A2DP codec

2018-09-05 Thread Tanu Kaskinen
On Tue, 2018-09-04 at 11:44 +0300, Tanu Kaskinen wrote:
> On Sat, 2018-07-28 at 17:34 +0200, Pali Rohár wrote:
> > +static size_t pa_sbc_select_configuration(const pa_sample_spec 
> > *sample_spec, const uint8_t *capabilities_buffer, size_t capabilities_size, 
> > uint8_t *config_buffer, size_t config_size) {
> > +a2dp_sbc_t *config = (a2dp_sbc_t *) config_buffer;
> > +a2dp_sbc_t *capabilities = (a2dp_sbc_t *) capabilities_buffer;
> 
> This looks likely to cause alignment errors, since the data in the
> uint8_t arrays doesn't have any kind of alignment guarantees. However,
> a2dp_sbc_t happens to only contain uint8_t values, so the values can't
> be badly aligned. It would be good to have a comment that reassures the
> reader that there won't be any alignment errors. Suggestion: "We cast a
> byte array to struct here, which can often cause alignment errors, but
> in this case it's safe, because a2dp_sbc_t contains only single-byte
> values."

a2dp_aptx_t has variables that are bigger than just one byte, so I
thought that we're going to have alignment issues with it. However, I
found out now that packed structs don't have alignment restrictions,
because the compiler will always be extra careful when dealing with
them. Therefore a better comment would be: "We cast a byte array to
struct here, which can often cause alignment errors, but in this case
it's safe, because a2dp_sbc_t uses the packed attribute, which makes
the compiler extra careful."

Copying that comment everywhere where we do the casting may not be such
a great idea after all, though. So much repetition. Maybe just add to
the struct definition a comment that explains what the packed attribute
does.

-- 
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss