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.
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
