On Sunday 05 August 2018 11:07:23 Arun Raghavan wrote:
> On Sat, 28 Jul 2018, at 9:04 PM, Pali Rohár wrote:
> > Move current SBC codec implementation into separate source file and use
> > this new API for providing all needed functionality for Bluetooth A2DP.
> >
> > Both bluez5-util and module-bluez5-device are changed to use this new
> > modular codec API.
> > ---
> > src/Makefile.am | 9 +-
> > src/modules/bluetooth/a2dp-codecs.h | 5 +-
> > src/modules/bluetooth/bluez5-util.c | 331 +++++----------
> > src/modules/bluetooth/bluez5-util.h | 10 +-
> > src/modules/bluetooth/module-bluez5-device.c | 487 ++++++----------------
> > src/modules/bluetooth/pa-a2dp-codec-sbc.c | 579 +++++++++++++++++++++++++++
> > src/modules/bluetooth/pa-a2dp-codec.h | 40 ++
> > 7 files changed, 851 insertions(+), 610 deletions(-)
> > create mode 100644 src/modules/bluetooth/pa-a2dp-codec-sbc.c
> > create mode 100644 src/modules/bluetooth/pa-a2dp-codec.h
> >
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index f62e7d5c4..411b9e5e5 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -2117,6 +2117,7 @@ module_bluetooth_discover_la_CFLAGS = $(AM_CFLAGS)
> > -DPA_MODULE_NAME=module_bluet
> > libbluez5_util_la_SOURCES = \
> > modules/bluetooth/bluez5-util.c \
> > modules/bluetooth/bluez5-util.h \
> > + modules/bluetooth/pa-a2dp-codec.h \
> > modules/bluetooth/a2dp-codecs.h
> > if HAVE_BLUEZ_5_OFONO_HEADSET
> > libbluez5_util_la_SOURCES += \
> > @@ -2131,6 +2132,10 @@ libbluez5_util_la_LDFLAGS = -avoid-version
> > libbluez5_util_la_LIBADD = $(MODULE_LIBADD) $(DBUS_LIBS)
> > libbluez5_util_la_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS)
> >
> > +libbluez5_util_la_SOURCES += modules/bluetooth/pa-a2dp-codec-sbc.c
> > +libbluez5_util_la_LIBADD += $(SBC_LIBS)
> > +libbluez5_util_la_CFLAGS += $(SBC_CFLAGS)
> > +
> > 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
> > @@ -2138,8 +2143,8 @@ module_bluez5_discover_la_CFLAGS = $(AM_CFLAGS) $
> > (DBUS_CFLAGS) -DPA_MODULE_NAME=
> >
> > module_bluez5_device_la_SOURCES = modules/bluetooth/module-bluez5-
> > device.c
> > module_bluez5_device_la_LDFLAGS = $(MODULE_LDFLAGS)
> > -module_bluez5_device_la_LIBADD = $(MODULE_LIBADD) $(SBC_LIBS)
> > libbluez5-util.la
> > -module_bluez5_device_la_CFLAGS = $(AM_CFLAGS) $(SBC_CFLAGS) -
> > DPA_MODULE_NAME=module_bluez5_device
> > +module_bluez5_device_la_LIBADD = $(MODULE_LIBADD) libbluez5-util.la
> > +module_bluez5_device_la_CFLAGS = $(AM_CFLAGS) -
> > DPA_MODULE_NAME=module_bluez5_device
> >
> > # Apple Airtunes/RAOP
> > module_raop_sink_la_SOURCES = modules/raop/module-raop-sink.c
> > diff --git a/src/modules/bluetooth/a2dp-codecs.h b/src/modules/
> > bluetooth/a2dp-codecs.h
> > index 8afcfcb24..004975586 100644
> > --- a/src/modules/bluetooth/a2dp-codecs.h
> > +++ b/src/modules/bluetooth/a2dp-codecs.h
> > @@ -47,6 +47,9 @@
> > #define SBC_ALLOCATION_SNR (1 << 1)
> > #define SBC_ALLOCATION_LOUDNESS 1
> >
> > +#define SBC_MIN_BITPOOL 2
> > +#define SBC_MAX_BITPOOL 64
> > +
> > #define MPEG_CHANNEL_MODE_MONO (1 << 3)
> > #define MPEG_CHANNEL_MODE_DUAL_CHANNEL (1 << 2)
> > #define MPEG_CHANNEL_MODE_STEREO (1 << 1)
> > @@ -63,8 +66,6 @@
> > #define MPEG_SAMPLING_FREQ_44100 (1 << 1)
> > #define MPEG_SAMPLING_FREQ_48000 1
> >
> > -#define MAX_BITPOOL 64
> > -#define MIN_BITPOOL 2
> >
> > #if __BYTE_ORDER == __LITTLE_ENDIAN
> >
> > diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/
> > bluetooth/bluez5-util.c
> > index 2d8337317..9c4e3367b 100644
> > --- a/src/modules/bluetooth/bluez5-util.c
> > +++ b/src/modules/bluetooth/bluez5-util.c
> > @@ -2,6 +2,7 @@
> > This file is part of PulseAudio.
> >
> > Copyright 2008-2013 João Paulo Rechi Vita
> > + Copyrigth 2018 Pali Rohár <pali.rohar at gmail.com>
> >
> > PulseAudio is free software; you can redistribute it and/or modify
> > it under the terms of the GNU Lesser General Public License as
> > @@ -33,7 +34,7 @@
> > #include <pulsecore/refcnt.h>
> > #include <pulsecore/shared.h>
> >
> > -#include "a2dp-codecs.h"
> > +#include "pa-a2dp-codec.h"
> >
> > #include "bluez5-util.h"
> >
> > @@ -48,8 +49,8 @@
> >
> > #define BLUEZ_ERROR_NOT_SUPPORTED "org.bluez.Error.NotSupported"
> >
> > -#define A2DP_SOURCE_ENDPOINT "/MediaEndpoint/A2DPSource"
> > -#define A2DP_SINK_ENDPOINT "/MediaEndpoint/A2DPSink"
> > +#define A2DP_SOURCE_SBC_ENDPOINT "/MediaEndpoint/A2DPSourceSBC"
> > +#define A2DP_SINK_SBC_ENDPOINT "/MediaEndpoint/A2DPSinkSBC"
> >
> > #define ENDPOINT_INTROSPECT_XML
> > \
> > DBUS_INTROSPECT_1_0_XML_DOCTYPE_DECL_NODE
> > \
> > @@ -170,9 +171,9 @@ static const char
> > *transport_state_to_string(pa_bluetooth_transport_state_t stat
> >
> > static bool device_supports_profile(pa_bluetooth_device *device,
> > pa_bluetooth_profile_t profile) {
> > switch (profile) {
> > - case PA_BLUETOOTH_PROFILE_A2DP_SINK:
> > + case PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK:
> > return !!pa_hashmap_get(device->uuids,
> > PA_BLUETOOTH_UUID_A2DP_SINK);
> > - case PA_BLUETOOTH_PROFILE_A2DP_SOURCE:
> > + case PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE:
> > return !!pa_hashmap_get(device->uuids,
>
> I haven't done a thorough review yet, but this one caught my eye. Tying the codec and the profile together here does not seem to be a good choice. Seems cleaner to separate out the codec choice into a separate field.
For each codec you need bluez endpoint, bound to bluez code which is in
bluez5-util.c
If codec should not be part of profile? How should be codec selected,
negotiated or switched by user?
And how to handle situation for bi-rectional A2DP codecs? Because now
PA_BLUETOOTH_PROFILE_A2DP_SINK and PA_BLUETOOTH_PROFILE_A2DP_SOURCE
support only one direction.
> There's a bunch of other stuff, but probably makes sense to discuss this up-front.
>
> Also, we now have GitLab for patch submission, so feel free to use that for easier code reviews in the next iteration.
>
> -- Arun
--
Pali Rohár
pali.rohar at gmail.com
