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
