[pulseaudio-discuss] [PATCH v2 1/2] Modular API for Bluetooth A2DP codec

[Arun Raghavan]

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