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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux