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

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

 



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.

> 
> > >      case PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT:
> > >          cp = pa_card_profile_new(name, _("Headset Head Unit (HSP/HFP)"), sizeof(pa_bluetooth_profile_t));
> > >          cp->priority = 30;
> > > @@ -1840,8 +1871,10 @@ off:
> > >  }
> > >  
> > >  static int uuid_to_profile(const char *uuid, pa_bluetooth_profile_t *_r) {
> > > +    /* FIXME: PA_BLUETOOTH_UUID_A2DP_SINK maps to both SBC and APTX */
> > >      if (pa_streq(uuid, PA_BLUETOOTH_UUID_A2DP_SINK))
> > >          *_r = PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK;
> > > +    /* FIXME: PA_BLUETOOTH_UUID_A2DP_SOURCE maps to both SBC and APTX */
> > >      else if (pa_streq(uuid, PA_BLUETOOTH_UUID_A2DP_SOURCE))
> > >          *_r = PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE;
> > >      else if (pa_bluetooth_uuid_is_hsp_hs(uuid) || pa_streq(uuid, PA_BLUETOOTH_UUID_HFP_HF))
> > > @@ -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.
> 
> Ok.
> 
> > > +    if (sample_spec->channels != 2 || !(capabilities->channel_mode & APTX_CHANNEL_MODE_STEREO)) {
> > > +        pa_log_error("No supported channel modes");
> > > +        return 0;
> > > +    }
> > 
> > sample_spec->channels is the global default channel count, and it's
> > used only as a hint for choosing the preferable channel mode. Since we
> > support only one channel mode, we can ignore sample_spec->channels
> > altogether.
> 
> Ok.
> 
> > > +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.
> 
> Ok.
> 
> Input is sequence of 4 raw 24bit signed stereo samples. And at one time
> we need to process multiple of 8 due to synchronization as aptX uses 3
> bits in every eight sequence for checksum. aptX compression ratio is
> fixed 6:1. Therefore 8*4*6 bytes are on input and 8*4 bytes on output.

Thanks! Makes sense.

-- 
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk



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

  Powered by Linux