[v2,1/2] Modular API for Bluetooth A2DP codec

Submitted by Pali Rohár on July 28, 2018, 3:34 p.m.

Details

Message ID 20180728153453.32694-2-pali.rohar@gmail.com
State New
Headers show
Series "WIP: Bluetooth A2DP aptX codec support" ( rev: 2 ) in PulseAudio

Not browsing as part of any series.

Commit Message

Pali Rohár July 28, 2018, 3:34 p.m.
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

Patch hide | download patch | download mbox

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@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, PA_BLUETOOTH_UUID_A2DP_SOURCE);
         case PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT:
             return !!pa_hashmap_get(device->uuids, PA_BLUETOOTH_UUID_HSP_HS)
@@ -888,10 +889,21 @@  finish:
 static void register_endpoint(pa_bluetooth_discovery *y, const char *path, const char *endpoint, const char *uuid) {
     DBusMessage *m;
     DBusMessageIter i, d;
-    uint8_t codec = 0;
+    uint8_t capabilities[1024];
+    size_t capabilities_size;
+    uint8_t codec_id;
+    const pa_a2dp_codec_t *codec;
+
+    codec = pa_a2dp_endpoint_to_a2dp_codec(endpoint);
+    if (!codec)
+        return;
 
     pa_log_debug("Registering %s on adapter %s", endpoint, path);
 
+    codec_id = codec->codec_id;
+    capabilities_size = codec->fill_capabilities(capabilities, sizeof(capabilities));
+    pa_assert(capabilities_size != 0);
+
     pa_assert_se(m = dbus_message_new_method_call(BLUEZ_SERVICE, path, BLUEZ_MEDIA_INTERFACE, "RegisterEndpoint"));
 
     dbus_message_iter_init_append(m, &i);
@@ -899,23 +911,8 @@  static void register_endpoint(pa_bluetooth_discovery *y, const char *path, const
     dbus_message_iter_open_container(&i, DBUS_TYPE_ARRAY, DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING DBUS_TYPE_STRING_AS_STRING
                                          DBUS_TYPE_VARIANT_AS_STRING DBUS_DICT_ENTRY_END_CHAR_AS_STRING, &d);
     pa_dbus_append_basic_variant_dict_entry(&d, "UUID", DBUS_TYPE_STRING, &uuid);
-    pa_dbus_append_basic_variant_dict_entry(&d, "Codec", DBUS_TYPE_BYTE, &codec);
-
-    if (pa_streq(uuid, PA_BLUETOOTH_UUID_A2DP_SOURCE) || pa_streq(uuid, PA_BLUETOOTH_UUID_A2DP_SINK)) {
-        a2dp_sbc_t capabilities;
-
-        capabilities.channel_mode = SBC_CHANNEL_MODE_MONO | SBC_CHANNEL_MODE_DUAL_CHANNEL | SBC_CHANNEL_MODE_STEREO |
-                                    SBC_CHANNEL_MODE_JOINT_STEREO;
-        capabilities.frequency = SBC_SAMPLING_FREQ_16000 | SBC_SAMPLING_FREQ_32000 | SBC_SAMPLING_FREQ_44100 |
-                                 SBC_SAMPLING_FREQ_48000;
-        capabilities.allocation_method = SBC_ALLOCATION_SNR | SBC_ALLOCATION_LOUDNESS;
-        capabilities.subbands = SBC_SUBBANDS_4 | SBC_SUBBANDS_8;
-        capabilities.block_length = SBC_BLOCK_LENGTH_4 | SBC_BLOCK_LENGTH_8 | SBC_BLOCK_LENGTH_12 | SBC_BLOCK_LENGTH_16;
-        capabilities.min_bitpool = MIN_BITPOOL;
-        capabilities.max_bitpool = MAX_BITPOOL;
-
-        pa_dbus_append_basic_array_variant_dict_entry(&d, "Capabilities", DBUS_TYPE_BYTE, &capabilities, sizeof(capabilities));
-    }
+    pa_dbus_append_basic_variant_dict_entry(&d, "Codec", DBUS_TYPE_BYTE, &codec_id);
+    pa_dbus_append_basic_array_variant_dict_entry(&d, "Capabilities", DBUS_TYPE_BYTE, &capabilities, capabilities_size);
 
     dbus_message_iter_close_container(&i, &d);
 
@@ -964,8 +961,8 @@  static void parse_interfaces_and_properties(pa_bluetooth_discovery *y, DBusMessa
             if (!a->valid)
                 return;
 
-            register_endpoint(y, path, A2DP_SOURCE_ENDPOINT, PA_BLUETOOTH_UUID_A2DP_SOURCE);
-            register_endpoint(y, path, A2DP_SINK_ENDPOINT, PA_BLUETOOTH_UUID_A2DP_SINK);
+            register_endpoint(y, path, A2DP_SOURCE_SBC_ENDPOINT, PA_BLUETOOTH_UUID_A2DP_SOURCE);
+            register_endpoint(y, path, A2DP_SINK_SBC_ENDPOINT, PA_BLUETOOTH_UUID_A2DP_SINK);
 
         } else if (pa_streq(interface, BLUEZ_DEVICE_INTERFACE)) {
 
@@ -1257,53 +1254,11 @@  fail:
     return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
 }
 
-static uint8_t a2dp_default_bitpool(uint8_t freq, uint8_t mode) {
-    /* These bitpool values were chosen based on the A2DP spec recommendation */
-    switch (freq) {
-        case SBC_SAMPLING_FREQ_16000:
-        case SBC_SAMPLING_FREQ_32000:
-            return 53;
-
-        case SBC_SAMPLING_FREQ_44100:
-
-            switch (mode) {
-                case SBC_CHANNEL_MODE_MONO:
-                case SBC_CHANNEL_MODE_DUAL_CHANNEL:
-                    return 31;
-
-                case SBC_CHANNEL_MODE_STEREO:
-                case SBC_CHANNEL_MODE_JOINT_STEREO:
-                    return 53;
-            }
-
-            pa_log_warn("Invalid channel mode %u", mode);
-            return 53;
-
-        case SBC_SAMPLING_FREQ_48000:
-
-            switch (mode) {
-                case SBC_CHANNEL_MODE_MONO:
-                case SBC_CHANNEL_MODE_DUAL_CHANNEL:
-                    return 29;
-
-                case SBC_CHANNEL_MODE_STEREO:
-                case SBC_CHANNEL_MODE_JOINT_STEREO:
-                    return 51;
-            }
-
-            pa_log_warn("Invalid channel mode %u", mode);
-            return 51;
-    }
-
-    pa_log_warn("Invalid sampling freq %u", freq);
-    return 53;
-}
-
 const char *pa_bluetooth_profile_to_string(pa_bluetooth_profile_t profile) {
     switch(profile) {
-        case PA_BLUETOOTH_PROFILE_A2DP_SINK:
+        case PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK:
             return "a2dp_sink";
-        case PA_BLUETOOTH_PROFILE_A2DP_SOURCE:
+        case PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE:
             return "a2dp_source";
         case PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT:
             return "headset_head_unit";
@@ -1316,6 +1271,38 @@  const char *pa_bluetooth_profile_to_string(pa_bluetooth_profile_t profile) {
     return NULL;
 }
 
+const char *pa_bluetooth_profile_to_a2dp_endpoint(pa_bluetooth_profile_t profile) {
+    switch (profile) {
+        case PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK:
+            return A2DP_SOURCE_SBC_ENDPOINT;
+        case PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE:
+            return A2DP_SINK_SBC_ENDPOINT;
+        default:
+            return NULL;
+    }
+
+    return NULL;
+}
+
+const pa_a2dp_codec_t *pa_bluetooth_profile_to_a2dp_codec(pa_bluetooth_profile_t profile) {
+    switch (profile) {
+        case PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK:
+        case PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE:
+            return &pa_a2dp_codec_sbc;
+        default:
+            return NULL;
+    }
+
+    return NULL;
+}
+
+const pa_a2dp_codec_t *pa_a2dp_endpoint_to_a2dp_codec(const char *endpoint) {
+    if (pa_streq(endpoint, A2DP_SOURCE_SBC_ENDPOINT) || pa_streq(endpoint, A2DP_SINK_SBC_ENDPOINT))
+        return &pa_a2dp_codec_sbc;
+    else
+        return NULL;
+}
+
 static DBusMessage *endpoint_set_configuration(DBusConnection *conn, DBusMessage *m, void *userdata) {
     pa_bluetooth_discovery *y = userdata;
     pa_bluetooth_device *d;
@@ -1345,6 +1332,8 @@  static DBusMessage *endpoint_set_configuration(DBusConnection *conn, DBusMessage
     if (dbus_message_iter_get_arg_type(&props) != DBUS_TYPE_DICT_ENTRY)
         goto fail;
 
+    endpoint_path = dbus_message_get_path(m);
+
     /* Read transport properties */
     while (dbus_message_iter_get_arg_type(&props) == DBUS_TYPE_DICT_ENTRY) {
         const char *key;
@@ -1367,13 +1356,12 @@  static DBusMessage *endpoint_set_configuration(DBusConnection *conn, DBusMessage
 
             dbus_message_iter_get_basic(&value, &uuid);
 
-            endpoint_path = dbus_message_get_path(m);
-            if (pa_streq(endpoint_path, A2DP_SOURCE_ENDPOINT)) {
+            if (pa_streq(endpoint_path, A2DP_SOURCE_SBC_ENDPOINT)) {
                 if (pa_streq(uuid, PA_BLUETOOTH_UUID_A2DP_SOURCE))
-                    p = PA_BLUETOOTH_PROFILE_A2DP_SINK;
-            } else if (pa_streq(endpoint_path, A2DP_SINK_ENDPOINT)) {
+                    p = PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK;
+            } else if (pa_streq(endpoint_path, A2DP_SINK_SBC_ENDPOINT)) {
                 if (pa_streq(uuid, PA_BLUETOOTH_UUID_A2DP_SINK))
-                    p = PA_BLUETOOTH_PROFILE_A2DP_SOURCE;
+                    p = PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE;
             }
 
             if (p == PA_BLUETOOTH_PROFILE_OFF) {
@@ -1389,7 +1377,7 @@  static DBusMessage *endpoint_set_configuration(DBusConnection *conn, DBusMessage
             dbus_message_iter_get_basic(&value, &dev_path);
         } else if (pa_streq(key, "Configuration")) {
             DBusMessageIter array;
-            a2dp_sbc_t *c;
+            const pa_a2dp_codec_t *codec;
 
             if (var != DBUS_TYPE_ARRAY) {
                 pa_log_error("Property %s of wrong type %c", key, (char)var);
@@ -1404,40 +1392,12 @@  static DBusMessage *endpoint_set_configuration(DBusConnection *conn, DBusMessage
             }
 
             dbus_message_iter_get_fixed_array(&array, &config, &size);
-            if (size != sizeof(a2dp_sbc_t)) {
-                pa_log_error("Configuration array of invalid size");
-                goto fail;
-            }
-
-            c = (a2dp_sbc_t *) config;
-
-            if (c->frequency != SBC_SAMPLING_FREQ_16000 && c->frequency != SBC_SAMPLING_FREQ_32000 &&
-                c->frequency != SBC_SAMPLING_FREQ_44100 && c->frequency != SBC_SAMPLING_FREQ_48000) {
-                pa_log_error("Invalid sampling frequency in configuration");
-                goto fail;
-            }
-
-            if (c->channel_mode != SBC_CHANNEL_MODE_MONO && c->channel_mode != SBC_CHANNEL_MODE_DUAL_CHANNEL &&
-                c->channel_mode != SBC_CHANNEL_MODE_STEREO && c->channel_mode != SBC_CHANNEL_MODE_JOINT_STEREO) {
-                pa_log_error("Invalid channel mode in configuration");
-                goto fail;
-            }
-
-            if (c->allocation_method != SBC_ALLOCATION_SNR && c->allocation_method != SBC_ALLOCATION_LOUDNESS) {
-                pa_log_error("Invalid allocation method in configuration");
-                goto fail;
-            }
 
-            if (c->subbands != SBC_SUBBANDS_4 && c->subbands != SBC_SUBBANDS_8) {
-                pa_log_error("Invalid SBC subbands in configuration");
-                goto fail;
-            }
+            codec = pa_a2dp_endpoint_to_a2dp_codec(endpoint_path);
+            pa_assert(codec);
 
-            if (c->block_length != SBC_BLOCK_LENGTH_4 && c->block_length != SBC_BLOCK_LENGTH_8 &&
-                c->block_length != SBC_BLOCK_LENGTH_12 && c->block_length != SBC_BLOCK_LENGTH_16) {
-                pa_log_error("Invalid block length in configuration");
+            if (!codec->validate_configuration(config, size))
                 goto fail;
-            }
         }
 
         dbus_message_iter_next(&props);
@@ -1484,21 +1444,17 @@  fail2:
 
 static DBusMessage *endpoint_select_configuration(DBusConnection *conn, DBusMessage *m, void *userdata) {
     pa_bluetooth_discovery *y = userdata;
-    a2dp_sbc_t *cap, config;
-    uint8_t *pconf = (uint8_t *) &config;
-    int i, size;
+    const char *endpoint_path;
+    uint8_t *cap;
+    int size;
+    const pa_a2dp_codec_t *codec;
+    uint8_t config[1024];
+    uint8_t *config_ptr = config;
+    size_t config_size;
     DBusMessage *r;
     DBusError err;
 
-    static const struct {
-        uint32_t rate;
-        uint8_t cap;
-    } freq_table[] = {
-        { 16000U, SBC_SAMPLING_FREQ_16000 },
-        { 32000U, SBC_SAMPLING_FREQ_32000 },
-        { 44100U, SBC_SAMPLING_FREQ_44100 },
-        { 48000U, SBC_SAMPLING_FREQ_48000 }
-    };
+    endpoint_path = dbus_message_get_path(m);
 
     dbus_error_init(&err);
 
@@ -1508,101 +1464,14 @@  static DBusMessage *endpoint_select_configuration(DBusConnection *conn, DBusMess
         goto fail;
     }
 
-    if (size != sizeof(config)) {
-        pa_log_error("Capabilities array has invalid size");
-        goto fail;
-    }
-
-    pa_zero(config);
-
-    /* Find the lowest freq that is at least as high as the requested sampling rate */
-    for (i = 0; (unsigned) i < PA_ELEMENTSOF(freq_table); i++)
-        if (freq_table[i].rate >= y->core->default_sample_spec.rate && (cap->frequency & freq_table[i].cap)) {
-            config.frequency = freq_table[i].cap;
-            break;
-        }
-
-    if ((unsigned) i == PA_ELEMENTSOF(freq_table)) {
-        for (--i; i >= 0; i--) {
-            if (cap->frequency & freq_table[i].cap) {
-                config.frequency = freq_table[i].cap;
-                break;
-            }
-        }
-
-        if (i < 0) {
-            pa_log_error("Not suitable sample rate");
-            goto fail;
-        }
-    }
-
-    pa_assert((unsigned) i < PA_ELEMENTSOF(freq_table));
-
-    if (y->core->default_sample_spec.channels <= 1) {
-        if (cap->channel_mode & SBC_CHANNEL_MODE_MONO)
-            config.channel_mode = SBC_CHANNEL_MODE_MONO;
-        else if (cap->channel_mode & SBC_CHANNEL_MODE_JOINT_STEREO)
-            config.channel_mode = SBC_CHANNEL_MODE_JOINT_STEREO;
-        else if (cap->channel_mode & SBC_CHANNEL_MODE_STEREO)
-            config.channel_mode = SBC_CHANNEL_MODE_STEREO;
-        else if (cap->channel_mode & SBC_CHANNEL_MODE_DUAL_CHANNEL)
-            config.channel_mode = SBC_CHANNEL_MODE_DUAL_CHANNEL;
-        else {
-            pa_log_error("No supported channel modes");
-            goto fail;
-        }
-    }
-
-    if (y->core->default_sample_spec.channels >= 2) {
-        if (cap->channel_mode & SBC_CHANNEL_MODE_JOINT_STEREO)
-            config.channel_mode = SBC_CHANNEL_MODE_JOINT_STEREO;
-        else if (cap->channel_mode & SBC_CHANNEL_MODE_STEREO)
-            config.channel_mode = SBC_CHANNEL_MODE_STEREO;
-        else if (cap->channel_mode & SBC_CHANNEL_MODE_DUAL_CHANNEL)
-            config.channel_mode = SBC_CHANNEL_MODE_DUAL_CHANNEL;
-        else if (cap->channel_mode & SBC_CHANNEL_MODE_MONO)
-            config.channel_mode = SBC_CHANNEL_MODE_MONO;
-        else {
-            pa_log_error("No supported channel modes");
-            goto fail;
-        }
-    }
-
-    if (cap->block_length & SBC_BLOCK_LENGTH_16)
-        config.block_length = SBC_BLOCK_LENGTH_16;
-    else if (cap->block_length & SBC_BLOCK_LENGTH_12)
-        config.block_length = SBC_BLOCK_LENGTH_12;
-    else if (cap->block_length & SBC_BLOCK_LENGTH_8)
-        config.block_length = SBC_BLOCK_LENGTH_8;
-    else if (cap->block_length & SBC_BLOCK_LENGTH_4)
-        config.block_length = SBC_BLOCK_LENGTH_4;
-    else {
-        pa_log_error("No supported block lengths");
-        goto fail;
-    }
-
-    if (cap->subbands & SBC_SUBBANDS_8)
-        config.subbands = SBC_SUBBANDS_8;
-    else if (cap->subbands & SBC_SUBBANDS_4)
-        config.subbands = SBC_SUBBANDS_4;
-    else {
-        pa_log_error("No supported subbands");
-        goto fail;
-    }
+    codec = pa_a2dp_endpoint_to_a2dp_codec(endpoint_path);
+    pa_assert(codec);
 
-    if (cap->allocation_method & SBC_ALLOCATION_LOUDNESS)
-        config.allocation_method = SBC_ALLOCATION_LOUDNESS;
-    else if (cap->allocation_method & SBC_ALLOCATION_SNR)
-        config.allocation_method = SBC_ALLOCATION_SNR;
-
-    config.min_bitpool = (uint8_t) PA_MAX(MIN_BITPOOL, cap->min_bitpool);
-    config.max_bitpool = (uint8_t) PA_MIN(a2dp_default_bitpool(config.frequency, config.channel_mode), cap->max_bitpool);
-
-    if (config.min_bitpool > config.max_bitpool)
-        goto fail;
+    config_size = codec->select_configuration(&y->core->default_sample_spec, cap, size, config, sizeof(config));
+    pa_assert(config_size != 0);
 
     pa_assert_se(r = dbus_message_new_method_return(m));
-    pa_assert_se(dbus_message_append_args(r, DBUS_TYPE_ARRAY, DBUS_TYPE_BYTE, &pconf, size, DBUS_TYPE_INVALID));
+    pa_assert_se(dbus_message_append_args(r, DBUS_TYPE_ARRAY, DBUS_TYPE_BYTE, &config_ptr, config_size, DBUS_TYPE_INVALID));
 
     return r;
 
@@ -1677,7 +1546,7 @@  static DBusHandlerResult endpoint_handler(DBusConnection *c, DBusMessage *m, voi
 
     pa_log_debug("dbus: path=%s, interface=%s, member=%s", path, interface, member);
 
-    if (!pa_streq(path, A2DP_SOURCE_ENDPOINT) && !pa_streq(path, A2DP_SINK_ENDPOINT))
+    if (!pa_streq(path, A2DP_SOURCE_SBC_ENDPOINT) && !pa_streq(path, A2DP_SINK_SBC_ENDPOINT))
         return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
 
     if (dbus_message_is_method_call(m, "org.freedesktop.DBus.Introspectable", "Introspect")) {
@@ -1709,38 +1578,22 @@  static void endpoint_init(pa_bluetooth_discovery *y, pa_bluetooth_profile_t prof
     static const DBusObjectPathVTable vtable_endpoint = {
         .message_function = endpoint_handler,
     };
+    const char *endpoint = pa_bluetooth_profile_to_a2dp_endpoint(profile);
 
     pa_assert(y);
+    pa_assert(endpoint);
 
-    switch(profile) {
-        case PA_BLUETOOTH_PROFILE_A2DP_SINK:
-            pa_assert_se(dbus_connection_register_object_path(pa_dbus_connection_get(y->connection), A2DP_SOURCE_ENDPOINT,
-                                                              &vtable_endpoint, y));
-            break;
-        case PA_BLUETOOTH_PROFILE_A2DP_SOURCE:
-            pa_assert_se(dbus_connection_register_object_path(pa_dbus_connection_get(y->connection), A2DP_SINK_ENDPOINT,
-                                                              &vtable_endpoint, y));
-            break;
-        default:
-            pa_assert_not_reached();
-            break;
-    }
+    pa_assert_se(dbus_connection_register_object_path(pa_dbus_connection_get(y->connection), endpoint,
+                                                      &vtable_endpoint, y));
 }
 
 static void endpoint_done(pa_bluetooth_discovery *y, pa_bluetooth_profile_t profile) {
+    const char *endpoint = pa_bluetooth_profile_to_a2dp_endpoint(profile);
+
     pa_assert(y);
+    pa_assert(endpoint);
 
-    switch(profile) {
-        case PA_BLUETOOTH_PROFILE_A2DP_SINK:
-            dbus_connection_unregister_object_path(pa_dbus_connection_get(y->connection), A2DP_SOURCE_ENDPOINT);
-            break;
-        case PA_BLUETOOTH_PROFILE_A2DP_SOURCE:
-            dbus_connection_unregister_object_path(pa_dbus_connection_get(y->connection), A2DP_SINK_ENDPOINT);
-            break;
-        default:
-            pa_assert_not_reached();
-            break;
-    }
+    dbus_connection_unregister_object_path(pa_dbus_connection_get(y->connection), endpoint);
 }
 
 pa_bluetooth_discovery* pa_bluetooth_discovery_get(pa_core *c, int headset_backend) {
@@ -1799,8 +1652,8 @@  pa_bluetooth_discovery* pa_bluetooth_discovery_get(pa_core *c, int headset_backe
     }
     y->matches_added = true;
 
-    endpoint_init(y, PA_BLUETOOTH_PROFILE_A2DP_SINK);
-    endpoint_init(y, PA_BLUETOOTH_PROFILE_A2DP_SOURCE);
+    endpoint_init(y, PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK);
+    endpoint_init(y, PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE);
 
     get_managed_objects(y);
 
@@ -1868,8 +1721,8 @@  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_SINK);
-        endpoint_done(y, PA_BLUETOOTH_PROFILE_A2DP_SOURCE);
+        endpoint_done(y, PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK);
+        endpoint_done(y, PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE);
 
         pa_dbus_connection_unref(y->connection);
     }
diff --git a/src/modules/bluetooth/bluez5-util.h b/src/modules/bluetooth/bluez5-util.h
index ad30708f0..365b9ef6f 100644
--- a/src/modules/bluetooth/bluez5-util.h
+++ b/src/modules/bluetooth/bluez5-util.h
@@ -5,6 +5,7 @@ 
   This file is part of PulseAudio.
 
   Copyright 2008-2013 João Paulo Rechi Vita
+  Copyrigth 2018      Pali Rohár <pali.rohar@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
@@ -22,6 +23,8 @@ 
 
 #include <pulsecore/core.h>
 
+#include "pa-a2dp-codec.h"
+
 #define PA_BLUETOOTH_UUID_A2DP_SOURCE "0000110a-0000-1000-8000-00805f9b34fb"
 #define PA_BLUETOOTH_UUID_A2DP_SINK   "0000110b-0000-1000-8000-00805f9b34fb"
 
@@ -51,8 +54,8 @@  typedef enum pa_bluetooth_hook {
 } pa_bluetooth_hook_t;
 
 typedef enum profile {
-    PA_BLUETOOTH_PROFILE_A2DP_SINK,
-    PA_BLUETOOTH_PROFILE_A2DP_SOURCE,
+    PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK,
+    PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE,
     PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT,
     PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY,
     PA_BLUETOOTH_PROFILE_OFF
@@ -163,6 +166,9 @@  pa_bluetooth_device* pa_bluetooth_discovery_get_device_by_address(pa_bluetooth_d
 pa_hook* pa_bluetooth_discovery_hook(pa_bluetooth_discovery *y, pa_bluetooth_hook_t hook);
 
 const char *pa_bluetooth_profile_to_string(pa_bluetooth_profile_t profile);
+const char *pa_bluetooth_profile_to_a2dp_endpoint(pa_bluetooth_profile_t profile);
+const pa_a2dp_codec_t *pa_bluetooth_profile_to_a2dp_codec(pa_bluetooth_profile_t profile);
+const pa_a2dp_codec_t *pa_a2dp_endpoint_to_a2dp_codec(const char *endpoint);
 
 static inline bool pa_bluetooth_uuid_is_hsp_hs(const char *uuid) {
     return pa_streq(uuid, PA_BLUETOOTH_UUID_HSP_HS) || pa_streq(uuid, PA_BLUETOOTH_UUID_HSP_HS_ALT);
diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
index 9dbdca316..e626e80e9 100644
--- a/src/modules/bluetooth/module-bluez5-device.c
+++ b/src/modules/bluetooth/module-bluez5-device.c
@@ -3,6 +3,7 @@ 
 
   Copyright 2008-2013 João Paulo Rechi Vita
   Copyright 2011-2013 BMW Car IT GmbH.
+  Copyright 2018      Pali Rohár <pali.rohar@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
@@ -25,7 +26,6 @@ 
 #include <errno.h>
 
 #include <arpa/inet.h>
-#include <sbc/sbc.h>
 
 #include <pulse/rtclock.h>
 #include <pulse/timeval.h>
@@ -47,8 +47,8 @@ 
 #include <pulsecore/time-smoother.h>
 
 #include "a2dp-codecs.h"
+#include "pa-a2dp-codec.h"
 #include "bluez5-util.h"
-#include "rtp.h"
 
 PA_MODULE_AUTHOR("João Paulo Rechi Vita");
 PA_MODULE_DESCRIPTION("BlueZ 5 Bluetooth audio sink and source");
@@ -62,8 +62,6 @@  PA_MODULE_USAGE("path=<device object path>"
 #define FIXED_LATENCY_RECORD_A2DP   (25 * PA_USEC_PER_MSEC)
 #define FIXED_LATENCY_RECORD_SCO    (25 * PA_USEC_PER_MSEC)
 
-#define BITPOOL_DEC_LIMIT 32
-#define BITPOOL_DEC_STEP 5
 #define HSP_MAX_GAIN 15
 
 static const char* const valid_modargs[] = {
@@ -94,18 +92,6 @@  typedef struct bluetooth_msg {
 PA_DEFINE_PRIVATE_CLASS(bluetooth_msg, pa_msgobject);
 #define BLUETOOTH_MSG(o) (bluetooth_msg_cast(o))
 
-typedef struct sbc_info {
-    sbc_t sbc;                           /* Codec data */
-    bool sbc_initialized;                /* Keep track if the encoder is initialized */
-    size_t codesize, frame_length;       /* SBC Codesize, frame_length. We simply cache those values here */
-    uint16_t seq_num;                    /* Cumulative packet sequence */
-    uint8_t min_bitpool;
-    uint8_t max_bitpool;
-
-    void* buffer;                        /* Codec transfer buffer */
-    size_t buffer_size;                  /* Size of the buffer */
-} sbc_info_t;
-
 struct userdata {
     pa_module *module;
     pa_core *core;
@@ -146,7 +132,11 @@  struct userdata {
     pa_smoother *read_smoother;
     pa_memchunk write_memchunk;
     pa_sample_spec sample_spec;
-    struct sbc_info sbc_info;
+    void *encoder_info;
+    void *decoder_info;
+
+    void* buffer;                        /* Codec transfer buffer */
+    size_t buffer_size;                  /* Size of the buffer */
 };
 
 typedef enum pa_bluetooth_form_factor {
@@ -418,105 +408,22 @@  static void a2dp_prepare_buffer(struct userdata *u) {
 
     pa_assert(u);
 
-    if (u->sbc_info.buffer_size >= min_buffer_size)
+    if (u->buffer_size >= min_buffer_size)
         return;
 
-    u->sbc_info.buffer_size = 2 * min_buffer_size;
-    pa_xfree(u->sbc_info.buffer);
-    u->sbc_info.buffer = pa_xmalloc(u->sbc_info.buffer_size);
+    u->buffer_size = 2 * min_buffer_size;
+    pa_xfree(u->buffer);
+    u->buffer = pa_xmalloc(u->buffer_size);
 }
 
 /* Run from IO thread */
-static int a2dp_process_render(struct userdata *u) {
-    struct sbc_info *sbc_info;
-    struct rtp_header *header;
-    struct rtp_payload *payload;
-    size_t nbytes;
-    void *d;
-    const void *p;
-    size_t to_write, to_encode;
-    unsigned frame_count;
+static int a2dp_write_buffer(struct userdata *u, size_t nbytes) {
     int ret = 0;
 
-    pa_assert(u);
-    pa_assert(u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK);
-    pa_assert(u->sink);
-
-    /* First, render some data */
-    if (!u->write_memchunk.memblock)
-        pa_sink_render_full(u->sink, u->write_block_size, &u->write_memchunk);
-
-    pa_assert(u->write_memchunk.length == u->write_block_size);
-
-    a2dp_prepare_buffer(u);
-
-    sbc_info = &u->sbc_info;
-    header = sbc_info->buffer;
-    payload = (struct rtp_payload*) ((uint8_t*) sbc_info->buffer + sizeof(*header));
-
-    frame_count = 0;
-
-    /* Try to create a packet of the full MTU */
-
-    p = (const uint8_t *) pa_memblock_acquire_chunk(&u->write_memchunk);
-    to_encode = u->write_memchunk.length;
-
-    d = (uint8_t*) sbc_info->buffer + sizeof(*header) + sizeof(*payload);
-    to_write = sbc_info->buffer_size - sizeof(*header) - sizeof(*payload);
-
-    while (PA_LIKELY(to_encode > 0 && to_write > 0)) {
-        ssize_t written;
-        ssize_t encoded;
-
-        encoded = sbc_encode(&sbc_info->sbc,
-                             p, to_encode,
-                             d, to_write,
-                             &written);
-
-        if (PA_UNLIKELY(encoded <= 0)) {
-            pa_log_error("SBC encoding error (%li)", (long) encoded);
-            pa_memblock_release(u->write_memchunk.memblock);
-            return -1;
-        }
-
-        pa_assert_fp((size_t) encoded <= to_encode);
-        pa_assert_fp((size_t) encoded == sbc_info->codesize);
-
-        pa_assert_fp((size_t) written <= to_write);
-        pa_assert_fp((size_t) written == sbc_info->frame_length);
-
-        p = (const uint8_t*) p + encoded;
-        to_encode -= encoded;
-
-        d = (uint8_t*) d + written;
-        to_write -= written;
-
-        frame_count++;
-    }
-
-    pa_memblock_release(u->write_memchunk.memblock);
-
-    pa_assert(to_encode == 0);
-
-    PA_ONCE_BEGIN {
-        pa_log_debug("Using SBC encoder implementation: %s", pa_strnull(sbc_get_implementation_info(&sbc_info->sbc)));
-    } PA_ONCE_END;
-
-    /* write it to the fifo */
-    memset(sbc_info->buffer, 0, sizeof(*header) + sizeof(*payload));
-    header->v = 2;
-    header->pt = 1;
-    header->sequence_number = htons(sbc_info->seq_num++);
-    header->timestamp = htonl(u->write_index / pa_frame_size(&u->sample_spec));
-    header->ssrc = htonl(1);
-    payload->frame_count = frame_count;
-
-    nbytes = (uint8_t*) d - (uint8_t*) sbc_info->buffer;
-
     for (;;) {
         ssize_t l;
 
-        l = pa_write(u->stream_fd, sbc_info->buffer, nbytes, &u->stream_write_type);
+        l = pa_write(u->stream_fd, u->buffer, nbytes, &u->stream_write_type);
 
         pa_assert(l != 0);
 
@@ -560,37 +467,65 @@  static int a2dp_process_render(struct userdata *u) {
 }
 
 /* Run from IO thread */
+static int a2dp_process_render(struct userdata *u) {
+    const pa_a2dp_codec_t *codec;
+    const uint8_t *ptr;
+    size_t length;
+
+    pa_assert(u);
+    pa_assert(u->sink);
+
+    codec = pa_bluetooth_profile_to_a2dp_codec(u->profile);
+    pa_assert(codec);
+
+    /* First, render some data */
+    if (!u->write_memchunk.memblock)
+        pa_sink_render_full(u->sink, u->write_block_size, &u->write_memchunk);
+
+    pa_assert(u->write_memchunk.length == u->write_block_size);
+
+    a2dp_prepare_buffer(u);
+
+    /* Try to create a packet of the full MTU */
+    ptr = (const uint8_t *) pa_memblock_acquire_chunk(&u->write_memchunk);
+
+    length = codec->encode(&u->encoder_info, u->write_index / pa_frame_size(&u->sample_spec), ptr, u->write_memchunk.length, u->buffer, u->buffer_size);
+
+    pa_memblock_release(u->write_memchunk.memblock);
+
+    if (length == 0)
+        return -1;
+
+    return a2dp_write_buffer(u, length);
+}
+
+/* Run from IO thread */
 static int a2dp_process_push(struct userdata *u) {
+    const pa_a2dp_codec_t *codec;
     int ret = 0;
     pa_memchunk memchunk;
 
     pa_assert(u);
-    pa_assert(u->profile == PA_BLUETOOTH_PROFILE_A2DP_SOURCE);
     pa_assert(u->source);
     pa_assert(u->read_smoother);
 
+    codec = pa_bluetooth_profile_to_a2dp_codec(u->profile);
+    pa_assert(codec);
+
     memchunk.memblock = pa_memblock_new(u->core->mempool, u->read_block_size);
     memchunk.index = memchunk.length = 0;
 
     for (;;) {
         bool found_tstamp = false;
         pa_usec_t tstamp;
-        struct sbc_info *sbc_info;
-        struct rtp_header *header;
-        struct rtp_payload *payload;
-        const void *p;
-        void *d;
+        uint8_t *ptr;
         ssize_t l;
-        size_t to_write, to_decode;
+        size_t processed;
         size_t total_written = 0;
 
         a2dp_prepare_buffer(u);
 
-        sbc_info = &u->sbc_info;
-        header = sbc_info->buffer;
-        payload = (struct rtp_payload*) ((uint8_t*) sbc_info->buffer + sizeof(*header));
-
-        l = pa_read(u->stream_fd, sbc_info->buffer, sbc_info->buffer_size, &u->stream_write_type);
+        l = pa_read(u->stream_fd, u->buffer, u->buffer_size, &u->stream_write_type);
 
         if (l <= 0) {
 
@@ -607,7 +542,7 @@  static int a2dp_process_push(struct userdata *u) {
             break;
         }
 
-        pa_assert((size_t) l <= sbc_info->buffer_size);
+        pa_assert((size_t) l <= u->buffer_size);
 
         /* TODO: get timestamp from rtp */
         if (!found_tstamp) {
@@ -615,50 +550,18 @@  static int a2dp_process_push(struct userdata *u) {
             tstamp = pa_rtclock_now();
         }
 
-        p = (uint8_t*) sbc_info->buffer + sizeof(*header) + sizeof(*payload);
-        to_decode = l - sizeof(*header) - sizeof(*payload);
-
-        d = pa_memblock_acquire(memchunk.memblock);
-        to_write = memchunk.length = pa_memblock_get_length(memchunk.memblock);
-
-        while (PA_LIKELY(to_decode > 0)) {
-            size_t written;
-            ssize_t decoded;
-
-            decoded = sbc_decode(&sbc_info->sbc,
-                                 p, to_decode,
-                                 d, to_write,
-                                 &written);
-
-            if (PA_UNLIKELY(decoded <= 0)) {
-                pa_log_error("SBC decoding error (%li)", (long) decoded);
-                pa_memblock_release(memchunk.memblock);
-                pa_memblock_unref(memchunk.memblock);
-                return 0;
-            }
-
-            total_written += written;
-
-            /* Reset frame length, it can be changed due to bitpool change */
-            sbc_info->frame_length = sbc_get_frame_length(&sbc_info->sbc);
-
-            pa_assert_fp((size_t) decoded <= to_decode);
-            pa_assert_fp((size_t) decoded == sbc_info->frame_length);
+        ptr = pa_memblock_acquire(memchunk.memblock);
+        memchunk.length = pa_memblock_get_length(memchunk.memblock);
 
-            pa_assert_fp((size_t) written == sbc_info->codesize);
-
-            p = (const uint8_t*) p + decoded;
-            to_decode -= decoded;
-
-            d = (uint8_t*) d + written;
-            to_write -= written;
-        }
+        total_written = codec->decode(&u->decoder_info, u->buffer, l, ptr, memchunk.length, &processed);
+        if (total_written == 0)
+            return 0;
 
         u->read_index += (uint64_t) total_written;
         pa_smoother_put(u->read_smoother, tstamp, pa_bytes_to_usec(u->read_index, &u->sample_spec));
         pa_smoother_resume(u->read_smoother, tstamp, true);
 
-        memchunk.length -= to_write;
+        memchunk.length -= l - processed;
 
         pa_memblock_release(memchunk.memblock);
 
@@ -705,72 +608,6 @@  static void update_buffer_size(struct userdata *u) {
     }
 }
 
-/* Run from I/O thread */
-static void a2dp_set_bitpool(struct userdata *u, uint8_t bitpool) {
-    struct sbc_info *sbc_info;
-
-    pa_assert(u);
-
-    sbc_info = &u->sbc_info;
-
-    if (sbc_info->sbc.bitpool == bitpool)
-        return;
-
-    if (bitpool > sbc_info->max_bitpool)
-        bitpool = sbc_info->max_bitpool;
-    else if (bitpool < sbc_info->min_bitpool)
-        bitpool = sbc_info->min_bitpool;
-
-    sbc_info->sbc.bitpool = bitpool;
-
-    sbc_info->codesize = sbc_get_codesize(&sbc_info->sbc);
-    sbc_info->frame_length = sbc_get_frame_length(&sbc_info->sbc);
-
-    pa_log_debug("Bitpool has changed to %u", sbc_info->sbc.bitpool);
-
-    u->read_block_size =
-        (u->read_link_mtu - sizeof(struct rtp_header) - sizeof(struct rtp_payload))
-        / sbc_info->frame_length * sbc_info->codesize;
-
-    u->write_block_size =
-        (u->write_link_mtu - sizeof(struct rtp_header) - sizeof(struct rtp_payload))
-        / sbc_info->frame_length * sbc_info->codesize;
-
-    pa_sink_set_max_request_within_thread(u->sink, u->write_block_size);
-    pa_sink_set_fixed_latency_within_thread(u->sink,
-            FIXED_LATENCY_PLAYBACK_A2DP + pa_bytes_to_usec(u->write_block_size, &u->sample_spec));
-
-    /* If there is still data in the memchunk, we have to discard it
-     * because the write_block_size may have changed. */
-    if (u->write_memchunk.memblock) {
-        pa_memblock_unref(u->write_memchunk.memblock);
-        pa_memchunk_reset(&u->write_memchunk);
-    }
-
-    update_buffer_size(u);
-}
-
-/* Run from I/O thread */
-static void a2dp_reduce_bitpool(struct userdata *u) {
-    struct sbc_info *sbc_info;
-    uint8_t bitpool;
-
-    pa_assert(u);
-
-    sbc_info = &u->sbc_info;
-
-    /* Check if bitpool is already at its limit */
-    if (sbc_info->sbc.bitpool <= BITPOOL_DEC_LIMIT)
-        return;
-
-    bitpool = sbc_info->sbc.bitpool - BITPOOL_DEC_STEP;
-
-    if (bitpool < BITPOOL_DEC_LIMIT)
-        bitpool = BITPOOL_DEC_LIMIT;
-
-    a2dp_set_bitpool(u, bitpool);
-}
-
 static void teardown_stream(struct userdata *u) {
     if (u->rtpoll_item) {
         pa_rtpoll_item_free(u->rtpoll_item);
@@ -860,32 +697,37 @@  static void transport_config_mtu(struct userdata *u) {
             u->write_block_size = pa_frame_align(u->write_block_size, &u->sink->sample_spec);
         }
     } else {
-        u->read_block_size =
-            (u->read_link_mtu - sizeof(struct rtp_header) - sizeof(struct rtp_payload))
-            / u->sbc_info.frame_length * u->sbc_info.codesize;
-
-        u->write_block_size =
-            (u->write_link_mtu - sizeof(struct rtp_header) - sizeof(struct rtp_payload))
-            / u->sbc_info.frame_length * u->sbc_info.codesize;
+        const pa_a2dp_codec_t *codec = pa_bluetooth_profile_to_a2dp_codec(u->profile);
+        pa_assert(codec);
+        pa_assert(u->sink || u->source);
+        codec->fill_blocksize(u->sink ? &u->encoder_info : &u->decoder_info, u->read_link_mtu, u->write_link_mtu, &u->read_block_size, &u->write_block_size);
     }
 
     if (u->sink) {
         pa_sink_set_max_request_within_thread(u->sink, u->write_block_size);
         pa_sink_set_fixed_latency_within_thread(u->sink,
-                                                (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK ?
+                                                (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK ?
                                                  FIXED_LATENCY_PLAYBACK_A2DP : FIXED_LATENCY_PLAYBACK_SCO) +
                                                 pa_bytes_to_usec(u->write_block_size, &u->sample_spec));
+
+        /* If there is still data in the memchunk, we have to discard it
+         * because the write_block_size may have changed. */
+        if (u->write_memchunk.memblock) {
+            pa_memblock_unref(u->write_memchunk.memblock);
+            pa_memchunk_reset(&u->write_memchunk);
+        }
     }
 
     if (u->source)
         pa_source_set_fixed_latency_within_thread(u->source,
-                                                  (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SOURCE ?
+                                                  (pa_bluetooth_profile_to_a2dp_codec(u->profile) ?
                                                    FIXED_LATENCY_RECORD_A2DP : FIXED_LATENCY_RECORD_SCO) +
                                                   pa_bytes_to_usec(u->read_block_size, &u->sample_spec));
 }
 
 /* Run from I/O thread */
 static void setup_stream(struct userdata *u) {
+    const pa_a2dp_codec_t *codec;
     struct pollfd *pollfd;
     int one;
 
@@ -895,6 +737,12 @@  static void setup_stream(struct userdata *u) {
 
     pa_log_info("Transport %s resuming", u->transport->path);
 
+    codec = pa_bluetooth_profile_to_a2dp_codec(u->profile);
+    if (codec) {
+        pa_assert(u->sink || u->source);
+        codec->setup(u->sink ? &u->encoder_info : &u->decoder_info);
+    }
+
     transport_config_mtu(u);
 
     pa_make_fd_nonblock(u->stream_fd);
@@ -904,12 +752,10 @@  static void setup_stream(struct userdata *u) {
     if (setsockopt(u->stream_fd, SOL_SOCKET, SO_TIMESTAMP, &one, sizeof(one)) < 0)
         pa_log_warn("Failed to enable SO_TIMESTAMP: %s", pa_cstrerror(errno));
 
-    pa_log_debug("Stream properly set up, we're ready to roll!");
-
-    if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK) {
-        a2dp_set_bitpool(u, u->sbc_info.max_bitpool);
+    if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK)
         update_buffer_size(u);
-    }
+
+    pa_log_debug("Stream properly set up, we're ready to roll!");
 
     u->rtpoll_item = pa_rtpoll_item_new(u->rtpoll, PA_RTPOLL_NEVER, 1);
     pollfd = pa_rtpoll_item_get_pollfd(u->rtpoll_item, NULL);
@@ -1078,7 +924,7 @@  static int add_source(struct userdata *u) {
 
     if (!u->transport_acquired)
         switch (u->profile) {
-            case PA_BLUETOOTH_PROFILE_A2DP_SOURCE:
+            case PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE:
             case PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY:
                 data.suspend_cause = PA_SUSPEND_USER;
                 break;
@@ -1090,8 +936,9 @@  static int add_source(struct userdata *u) {
                 else
                     pa_assert_not_reached();
                 break;
-            case PA_BLUETOOTH_PROFILE_A2DP_SINK:
+            case PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK:
             case PA_BLUETOOTH_PROFILE_OFF:
+            default:
                 pa_assert_not_reached();
                 break;
         }
@@ -1263,10 +1110,11 @@  static int add_sink(struct userdata *u) {
                 else
                     pa_assert_not_reached();
                 break;
-            case PA_BLUETOOTH_PROFILE_A2DP_SINK:
+            case PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK:
                 /* Profile switch should have failed */
-            case PA_BLUETOOTH_PROFILE_A2DP_SOURCE:
+            case PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE:
             case PA_BLUETOOTH_PROFILE_OFF:
+            default:
                 pa_assert_not_reached();
                 break;
         }
@@ -1296,111 +1144,11 @@  static void transport_config(struct userdata *u) {
         u->sample_spec.channels = 1;
         u->sample_spec.rate = 8000;
     } else {
-        sbc_info_t *sbc_info = &u->sbc_info;
-        a2dp_sbc_t *config;
-
+        const pa_a2dp_codec_t *codec = pa_bluetooth_profile_to_a2dp_codec(u->profile);
+        bool is_sink = (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK);
+        pa_assert(codec);
         pa_assert(u->transport);
-
-        u->sample_spec.format = PA_SAMPLE_S16LE;
-        config = (a2dp_sbc_t *) u->transport->config;
-
-        if (sbc_info->sbc_initialized)
-            sbc_reinit(&sbc_info->sbc, 0);
-        else
-            sbc_init(&sbc_info->sbc, 0);
-        sbc_info->sbc_initialized = true;
-
-        switch (config->frequency) {
-            case SBC_SAMPLING_FREQ_16000:
-                sbc_info->sbc.frequency = SBC_FREQ_16000;
-                u->sample_spec.rate = 16000U;
-                break;
-            case SBC_SAMPLING_FREQ_32000:
-                sbc_info->sbc.frequency = SBC_FREQ_32000;
-                u->sample_spec.rate = 32000U;
-                break;
-            case SBC_SAMPLING_FREQ_44100:
-                sbc_info->sbc.frequency = SBC_FREQ_44100;
-                u->sample_spec.rate = 44100U;
-                break;
-            case SBC_SAMPLING_FREQ_48000:
-                sbc_info->sbc.frequency = SBC_FREQ_48000;
-                u->sample_spec.rate = 48000U;
-                break;
-            default:
-                pa_assert_not_reached();
-        }
-
-        switch (config->channel_mode) {
-            case SBC_CHANNEL_MODE_MONO:
-                sbc_info->sbc.mode = SBC_MODE_MONO;
-                u->sample_spec.channels = 1;
-                break;
-            case SBC_CHANNEL_MODE_DUAL_CHANNEL:
-                sbc_info->sbc.mode = SBC_MODE_DUAL_CHANNEL;
-                u->sample_spec.channels = 2;
-                break;
-            case SBC_CHANNEL_MODE_STEREO:
-                sbc_info->sbc.mode = SBC_MODE_STEREO;
-                u->sample_spec.channels = 2;
-                break;
-            case SBC_CHANNEL_MODE_JOINT_STEREO:
-                sbc_info->sbc.mode = SBC_MODE_JOINT_STEREO;
-                u->sample_spec.channels = 2;
-                break;
-            default:
-                pa_assert_not_reached();
-        }
-
-        switch (config->allocation_method) {
-            case SBC_ALLOCATION_SNR:
-                sbc_info->sbc.allocation = SBC_AM_SNR;
-                break;
-            case SBC_ALLOCATION_LOUDNESS:
-                sbc_info->sbc.allocation = SBC_AM_LOUDNESS;
-                break;
-            default:
-                pa_assert_not_reached();
-        }
-
-        switch (config->subbands) {
-            case SBC_SUBBANDS_4:
-                sbc_info->sbc.subbands = SBC_SB_4;
-                break;
-            case SBC_SUBBANDS_8:
-                sbc_info->sbc.subbands = SBC_SB_8;
-                break;
-            default:
-                pa_assert_not_reached();
-        }
-
-        switch (config->block_length) {
-            case SBC_BLOCK_LENGTH_4:
-                sbc_info->sbc.blocks = SBC_BLK_4;
-                break;
-            case SBC_BLOCK_LENGTH_8:
-                sbc_info->sbc.blocks = SBC_BLK_8;
-                break;
-            case SBC_BLOCK_LENGTH_12:
-                sbc_info->sbc.blocks = SBC_BLK_12;
-                break;
-            case SBC_BLOCK_LENGTH_16:
-                sbc_info->sbc.blocks = SBC_BLK_16;
-                break;
-            default:
-                pa_assert_not_reached();
-        }
-
-        sbc_info->min_bitpool = config->min_bitpool;
-        sbc_info->max_bitpool = config->max_bitpool;
-
-        /* Set minimum bitpool for source to get the maximum possible block_size */
-        sbc_info->sbc.bitpool = u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK ? sbc_info->max_bitpool : sbc_info->min_bitpool;
-        sbc_info->codesize = sbc_get_codesize(&sbc_info->sbc);
-        sbc_info->frame_length = sbc_get_frame_length(&sbc_info->sbc);
-
-        pa_log_info("SBC parameters: allocation=%u, subbands=%u, blocks=%u, bitpool=%u",
-                    sbc_info->sbc.allocation, sbc_info->sbc.subbands ? 8 : 4, sbc_info->sbc.blocks, sbc_info->sbc.bitpool);
+        codec->init(is_sink ? &u->encoder_info : &u->decoder_info, &u->sample_spec, is_sink, u->transport->config, u->transport->config_size);
     }
 }
 
@@ -1421,7 +1169,7 @@  static int setup_transport(struct userdata *u) {
 
     u->transport = t;
 
-    if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SOURCE || u->profile == PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY)
+    if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE || u->profile == PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY)
         transport_acquire(u, true); /* In case of error, the sink/sources will be created suspended */
     else {
         int transport_error;
@@ -1439,8 +1187,8 @@  static int setup_transport(struct userdata *u) {
 /* Run from main thread */
 static pa_direction_t get_profile_direction(pa_bluetooth_profile_t p) {
     static const pa_direction_t profile_direction[] = {
-        [PA_BLUETOOTH_PROFILE_A2DP_SINK] = PA_DIRECTION_OUTPUT,
-        [PA_BLUETOOTH_PROFILE_A2DP_SOURCE] = PA_DIRECTION_INPUT,
+        [PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK] = PA_DIRECTION_OUTPUT,
+        [PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE] = PA_DIRECTION_INPUT,
         [PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT] = PA_DIRECTION_INPUT | PA_DIRECTION_OUTPUT,
         [PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY] = PA_DIRECTION_INPUT | PA_DIRECTION_OUTPUT,
         [PA_BLUETOOTH_PROFILE_OFF] = 0
@@ -1477,11 +1225,11 @@  static int write_block(struct userdata *u) {
     if (u->write_index <= 0)
         u->started_at = pa_rtclock_now();
 
-    if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK) {
-        if ((n_written = a2dp_process_render(u)) < 0)
+    if (u->profile == PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT || u->profile == PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY) {
+        if ((n_written = sco_process_render(u)) < 0)
             return -1;
     } else {
-        if ((n_written = sco_process_render(u)) < 0)
+        if ((n_written = a2dp_process_render(u)) < 0)
             return -1;
     }
 
@@ -1551,7 +1299,7 @@  static void thread_func(void *userdata) {
                 if (pollfd->revents & POLLIN) {
                     int n_read;
 
-                    if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SOURCE)
+                    if (pa_bluetooth_profile_to_a2dp_codec(u->profile))
                         n_read = a2dp_process_push(u);
                     else
                         n_read = sco_process_push(u);
@@ -1651,8 +1399,13 @@  static void thread_func(void *userdata) {
                                 skip_bytes -= bytes_to_render;
                             }
 
-                            if (u->write_index > 0 && u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK)
-                                a2dp_reduce_bitpool(u);
+                            if (u->write_index > 0 && u->sink) {
+                                const pa_a2dp_codec_t *codec = pa_bluetooth_profile_to_a2dp_codec(u->profile);
+                                if (codec && codec->fix_latency(&u->encoder_info)) {
+                                    transport_config_mtu(u);
+                                    update_buffer_size(u);
+                                }
+                            }
                         }
 
                         blocks_to_write = 1;
@@ -1767,7 +1520,7 @@  static int start_thread(struct userdata *u) {
         /* If we are in the headset role or the device is an a2dp source,
          * the source should not become default unless there is no other
          * sound device available. */
-        if (u->profile == PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY || u->profile == PA_BLUETOOTH_PROFILE_A2DP_SOURCE)
+        if (u->profile == PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY || u->profile == PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE)
             u->source->priority = 1500;
 
         pa_source_put(u->source);
@@ -1980,8 +1733,8 @@  static pa_card_profile *create_card_profile(struct userdata *u, pa_bluetooth_pro
     name = pa_bluetooth_profile_to_string(profile);
 
     switch (profile) {
-    case PA_BLUETOOTH_PROFILE_A2DP_SINK:
-        cp = pa_card_profile_new(name, _("High Fidelity Playback (A2DP Sink)"), sizeof(pa_bluetooth_profile_t));
+    case PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK:
+        cp = pa_card_profile_new(name, _("High Fidelity SBC Playback (A2DP Sink)"), sizeof(pa_bluetooth_profile_t));
         cp->priority = 40;
         cp->n_sinks = 1;
         cp->n_sources = 0;
@@ -1992,8 +1745,8 @@  static pa_card_profile *create_card_profile(struct userdata *u, pa_bluetooth_pro
         p = PA_CARD_PROFILE_DATA(cp);
         break;
 
-    case PA_BLUETOOTH_PROFILE_A2DP_SOURCE:
-        cp = pa_card_profile_new(name, _("High Fidelity Capture (A2DP Source)"), sizeof(pa_bluetooth_profile_t));
+    case PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE:
+        cp = pa_card_profile_new(name, _("High Fidelity SBC Capture (A2DP Source)"), sizeof(pa_bluetooth_profile_t));
         cp->priority = 20;
         cp->n_sinks = 0;
         cp->n_sources = 1;
@@ -2088,9 +1841,9 @@  off:
 
 static int uuid_to_profile(const char *uuid, pa_bluetooth_profile_t *_r) {
     if (pa_streq(uuid, PA_BLUETOOTH_UUID_A2DP_SINK))
-        *_r = PA_BLUETOOTH_PROFILE_A2DP_SINK;
+        *_r = PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK;
     else if (pa_streq(uuid, PA_BLUETOOTH_UUID_A2DP_SOURCE))
-        *_r = PA_BLUETOOTH_PROFILE_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))
         *_r = PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT;
     else if (pa_streq(uuid, PA_BLUETOOTH_UUID_HSP_AG) || pa_streq(uuid, PA_BLUETOOTH_UUID_HFP_AG))
@@ -2472,6 +2225,7 @@  fail:
 
 void pa__done(pa_module *m) {
     struct userdata *u;
+    const pa_a2dp_codec_t *codec;
 
     pa_assert(m);
 
@@ -2492,11 +2246,14 @@  void pa__done(pa_module *m) {
     if (u->transport_microphone_gain_changed_slot)
         pa_hook_slot_free(u->transport_microphone_gain_changed_slot);
 
-    if (u->sbc_info.buffer)
-        pa_xfree(u->sbc_info.buffer);
+    if (u->buffer)
+        pa_xfree(u->buffer);
 
-    if (u->sbc_info.sbc_initialized)
-        sbc_finish(&u->sbc_info.sbc);
+    codec = pa_bluetooth_profile_to_a2dp_codec(u->profile);
+    if (codec) {
+        codec->finish(&u->encoder_info);
+        codec->finish(&u->decoder_info);
+    }
 
     if (u->msg)
         pa_xfree(u->msg);
diff --git a/src/modules/bluetooth/pa-a2dp-codec-sbc.c b/src/modules/bluetooth/pa-a2dp-codec-sbc.c
new file mode 100644
index 000000000..2a61114a6
--- /dev/null
+++ b/src/modules/bluetooth/pa-a2dp-codec-sbc.c
@@ -0,0 +1,579 @@ 
+/***
+  This file is part of PulseAudio.
+
+  Copyright 2018 Pali Rohár <pali.rohar@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
+  published by the Free Software Foundation; either version 2.1 of the
+  License, or (at your option) any later version.
+
+  PulseAudio is distributed in the hope that it will be useful, but
+  WITHOUT ANY WARRANTY; without even the implied warranty of
+  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+  General Public License for more details.
+
+  You should have received a copy of the GNU Lesser General Public
+  License along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
+***/
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <pulsecore/core-util.h>
+#include <pulsecore/log.h>
+#include <pulsecore/macro.h>
+#include <pulsecore/once.h>
+#include <pulse/sample.h>
+#include <pulse/xmalloc.h>
+
+#include <arpa/inet.h>
+
+#include <sbc/sbc.h>
+
+#include "a2dp-codecs.h"
+#include "pa-a2dp-codec.h"
+#include "rtp.h"
+
+#define SBC_BITPOOL_DEC_LIMIT 32
+#define SBC_BITPOOL_DEC_STEP 5
+
+typedef struct sbc_info {
+    sbc_t sbc;                           /* Codec data */
+    bool sbc_initialized;                /* Keep track if the encoder is initialized */
+    size_t codesize, frame_length;       /* SBC Codesize, frame_length. We simply cache those values here */
+    uint16_t seq_num;                    /* Cumulative packet sequence */
+    uint8_t min_bitpool;
+    uint8_t max_bitpool;
+} sbc_info_t;
+
+static size_t pa_sbc_fill_capabilities(uint8_t *capabilities_buffer, size_t capabilities_size) {
+    a2dp_sbc_t *capabilities = (a2dp_sbc_t *) capabilities_buffer;
+
+    if (capabilities_size < sizeof(*capabilities)) {
+        pa_log_error("Invalid size of capabilities buffer");
+        return 0;
+    }
+
+    pa_zero(*capabilities);
+
+    capabilities->channel_mode = SBC_CHANNEL_MODE_MONO | SBC_CHANNEL_MODE_DUAL_CHANNEL | SBC_CHANNEL_MODE_STEREO |
+                                 SBC_CHANNEL_MODE_JOINT_STEREO;
+    capabilities->frequency = SBC_SAMPLING_FREQ_16000 | SBC_SAMPLING_FREQ_32000 | SBC_SAMPLING_FREQ_44100 |
+                              SBC_SAMPLING_FREQ_48000;
+    capabilities->allocation_method = SBC_ALLOCATION_SNR | SBC_ALLOCATION_LOUDNESS;
+    capabilities->subbands = SBC_SUBBANDS_4 | SBC_SUBBANDS_8;
+    capabilities->block_length = SBC_BLOCK_LENGTH_4 | SBC_BLOCK_LENGTH_8 | SBC_BLOCK_LENGTH_12 | SBC_BLOCK_LENGTH_16;
+    capabilities->min_bitpool = SBC_MIN_BITPOOL;
+    capabilities->max_bitpool = SBC_MAX_BITPOOL;
+
+    return sizeof(*capabilities);
+}
+
+static bool pa_sbc_validate_configuration(const uint8_t *config_buffer, size_t config_size) {
+    a2dp_sbc_t *config = (a2dp_sbc_t *) config_buffer;
+
+    if (config_size != sizeof(*config)) {
+        pa_log_error("Invalid size of config buffer");
+        return false;
+    }
+
+    if (config->frequency != SBC_SAMPLING_FREQ_16000 && config->frequency != SBC_SAMPLING_FREQ_32000 &&
+        config->frequency != SBC_SAMPLING_FREQ_44100 && config->frequency != SBC_SAMPLING_FREQ_48000) {
+        pa_log_error("Invalid sampling frequency in configuration");
+        return false;
+    }
+
+    if (config->channel_mode != SBC_CHANNEL_MODE_MONO && config->channel_mode != SBC_CHANNEL_MODE_DUAL_CHANNEL &&
+        config->channel_mode != SBC_CHANNEL_MODE_STEREO && config->channel_mode != SBC_CHANNEL_MODE_JOINT_STEREO) {
+        pa_log_error("Invalid channel mode in configuration");
+        return false;
+    }
+
+    if (config->allocation_method != SBC_ALLOCATION_SNR && config->allocation_method != SBC_ALLOCATION_LOUDNESS) {
+        pa_log_error("Invalid allocation method in configuration");
+        return false;
+    }
+
+    if (config->subbands != SBC_SUBBANDS_4 && config->subbands != SBC_SUBBANDS_8) {
+        pa_log_error("Invalid SBC subbands in configuration");
+        return false;
+    }
+
+    if (config->block_length != SBC_BLOCK_LENGTH_4 && config->block_length != SBC_BLOCK_LENGTH_8 &&
+        config->block_length != SBC_BLOCK_LENGTH_12 && config->block_length != SBC_BLOCK_LENGTH_16) {
+        pa_log_error("Invalid block length in configuration");
+        return false;
+    }
+
+    return true;
+}
+
+static uint8_t pa_sbc_default_bitpool(uint8_t freq, uint8_t mode) {
+    /* These bitpool values were chosen based on the A2DP spec recommendation */
+    switch (freq) {
+        case SBC_SAMPLING_FREQ_16000:
+        case SBC_SAMPLING_FREQ_32000:
+            return 53;
+
+        case SBC_SAMPLING_FREQ_44100:
+
+            switch (mode) {
+                case SBC_CHANNEL_MODE_MONO:
+                case SBC_CHANNEL_MODE_DUAL_CHANNEL:
+                    return 31;
+
+                case SBC_CHANNEL_MODE_STEREO:
+                case SBC_CHANNEL_MODE_JOINT_STEREO:
+                    return 53;
+            }
+
+            pa_log_warn("Invalid channel mode %u", mode);
+            return 53;
+
+        case SBC_SAMPLING_FREQ_48000:
+
+            switch (mode) {
+                case SBC_CHANNEL_MODE_MONO:
+                case SBC_CHANNEL_MODE_DUAL_CHANNEL:
+                    return 29;
+
+                case SBC_CHANNEL_MODE_STEREO:
+                case SBC_CHANNEL_MODE_JOINT_STEREO:
+                    return 51;
+            }
+
+            pa_log_warn("Invalid channel mode %u", mode);
+            return 51;
+    }
+
+    pa_log_warn("Invalid sampling freq %u", freq);
+    return 53;
+}
+
+static size_t pa_sbc_select_configuration(const pa_sample_spec *sample_spec, const uint8_t *capabilities_buffer, size_t capabilities_size, uint8_t *config_buffer, size_t config_size) {
+    a2dp_sbc_t *config = (a2dp_sbc_t *) config_buffer;
+    a2dp_sbc_t *capabilities = (a2dp_sbc_t *) capabilities_buffer;
+    int i;
+
+    static const struct {
+        uint32_t rate;
+        uint8_t cap;
+    } freq_table[] = {
+        { 16000U, SBC_SAMPLING_FREQ_16000 },
+        { 32000U, SBC_SAMPLING_FREQ_32000 },
+        { 44100U, SBC_SAMPLING_FREQ_44100 },
+        { 48000U, SBC_SAMPLING_FREQ_48000 }
+    };
+
+    if (capabilities_size != sizeof(*capabilities)) {
+        pa_log_error("Invalid size of capabilities buffer");
+        return 0;
+    }
+
+    if (config_size < sizeof(*config)) {
+        pa_log_error("Invalid size of config buffer");
+        return 0;
+    }
+
+    pa_zero(*config);
+
+    /* Find the lowest freq that is at least as high as the requested sampling rate */
+    for (i = 0; (unsigned) i < PA_ELEMENTSOF(freq_table); i++)
+        if (freq_table[i].rate >= sample_spec->rate && (capabilities->frequency & freq_table[i].cap)) {
+            config->frequency = freq_table[i].cap;
+            break;
+        }
+
+    if ((unsigned) i == PA_ELEMENTSOF(freq_table)) {
+        for (--i; i >= 0; i--) {
+            if (capabilities->frequency & freq_table[i].cap) {
+                config->frequency = freq_table[i].cap;
+                break;
+            }
+        }
+
+        if (i < 0) {
+            pa_log_error("Not suitable sample rate");
+            return 0;
+        }
+    }
+
+    pa_assert((unsigned) i < PA_ELEMENTSOF(freq_table));
+
+    if (sample_spec->channels <= 1) {
+        if (capabilities->channel_mode & SBC_CHANNEL_MODE_MONO)
+            config->channel_mode = SBC_CHANNEL_MODE_MONO;
+        else if (capabilities->channel_mode & SBC_CHANNEL_MODE_JOINT_STEREO)
+            config->channel_mode = SBC_CHANNEL_MODE_JOINT_STEREO;
+        else if (capabilities->channel_mode & SBC_CHANNEL_MODE_STEREO)
+            config->channel_mode = SBC_CHANNEL_MODE_STEREO;
+        else if (capabilities->channel_mode & SBC_CHANNEL_MODE_DUAL_CHANNEL)
+            config->channel_mode = SBC_CHANNEL_MODE_DUAL_CHANNEL;
+        else {
+            pa_log_error("No supported channel modes");
+            return 0;
+        }
+    }
+
+    if (sample_spec->channels >= 2) {
+        if (capabilities->channel_mode & SBC_CHANNEL_MODE_JOINT_STEREO)
+            config->channel_mode = SBC_CHANNEL_MODE_JOINT_STEREO;
+        else if (capabilities->channel_mode & SBC_CHANNEL_MODE_STEREO)
+            config->channel_mode = SBC_CHANNEL_MODE_STEREO;
+        else if (capabilities->channel_mode & SBC_CHANNEL_MODE_DUAL_CHANNEL)
+            config->channel_mode = SBC_CHANNEL_MODE_DUAL_CHANNEL;
+        else if (capabilities->channel_mode & SBC_CHANNEL_MODE_MONO)
+            config->channel_mode = SBC_CHANNEL_MODE_MONO;
+        else {
+            pa_log_error("No supported channel modes");
+            return 0;
+        }
+    }
+
+    if (capabilities->block_length & SBC_BLOCK_LENGTH_16)
+        config->block_length = SBC_BLOCK_LENGTH_16;
+    else if (capabilities->block_length & SBC_BLOCK_LENGTH_12)
+        config->block_length = SBC_BLOCK_LENGTH_12;
+    else if (capabilities->block_length & SBC_BLOCK_LENGTH_8)
+        config->block_length = SBC_BLOCK_LENGTH_8;
+    else if (capabilities->block_length & SBC_BLOCK_LENGTH_4)
+        config->block_length = SBC_BLOCK_LENGTH_4;
+    else {
+        pa_log_error("No supported block lengths");
+        return 0;
+    }
+
+    if (capabilities->subbands & SBC_SUBBANDS_8)
+        config->subbands = SBC_SUBBANDS_8;
+    else if (capabilities->subbands & SBC_SUBBANDS_4)
+        config->subbands = SBC_SUBBANDS_4;
+    else {
+        pa_log_error("No supported subbands");
+        return 0;
+    }
+
+    if (capabilities->allocation_method & SBC_ALLOCATION_LOUDNESS)
+        config->allocation_method = SBC_ALLOCATION_LOUDNESS;
+    else if (capabilities->allocation_method & SBC_ALLOCATION_SNR)
+        config->allocation_method = SBC_ALLOCATION_SNR;
+
+    config->min_bitpool = (uint8_t) PA_MAX(SBC_MIN_BITPOOL, capabilities->min_bitpool);
+    config->max_bitpool = (uint8_t) PA_MIN(pa_sbc_default_bitpool(config->frequency, config->channel_mode), capabilities->max_bitpool);
+
+    if (config->min_bitpool > config->max_bitpool)
+        return 0;
+
+    return sizeof(*config);
+}
+
+static void pa_sbc_init(void **info_ptr, pa_sample_spec *sample_spec, bool is_a2dp_sink, const uint8_t *config_buffer, size_t config_size) {
+    sbc_info_t *sbc_info;
+    a2dp_sbc_t *config = (a2dp_sbc_t *) config_buffer;
+
+    pa_assert(config_size == sizeof(*config));
+
+    if (!*info_ptr)
+        *info_ptr = pa_xmalloc0(sizeof(*sbc_info));
+
+    sbc_info = (sbc_info_t *) *info_ptr;
+
+    sample_spec->format = PA_SAMPLE_S16LE;
+
+    if (sbc_info->sbc_initialized)
+        sbc_reinit(&sbc_info->sbc, 0);
+    else
+        sbc_init(&sbc_info->sbc, 0);
+    sbc_info->sbc_initialized = true;
+
+    switch (config->frequency) {
+        case SBC_SAMPLING_FREQ_16000:
+            sbc_info->sbc.frequency = SBC_FREQ_16000;
+            sample_spec->rate = 16000U;
+            break;
+        case SBC_SAMPLING_FREQ_32000:
+            sbc_info->sbc.frequency = SBC_FREQ_32000;
+            sample_spec->rate = 32000U;
+            break;
+        case SBC_SAMPLING_FREQ_44100:
+            sbc_info->sbc.frequency = SBC_FREQ_44100;
+            sample_spec->rate = 44100U;
+            break;
+        case SBC_SAMPLING_FREQ_48000:
+            sbc_info->sbc.frequency = SBC_FREQ_48000;
+            sample_spec->rate = 48000U;
+            break;
+        default:
+            pa_assert_not_reached();
+    }
+
+    switch (config->channel_mode) {
+        case SBC_CHANNEL_MODE_MONO:
+            sbc_info->sbc.mode = SBC_MODE_MONO;
+            sample_spec->channels = 1;
+            break;
+        case SBC_CHANNEL_MODE_DUAL_CHANNEL:
+            sbc_info->sbc.mode = SBC_MODE_DUAL_CHANNEL;
+            sample_spec->channels = 2;
+            break;
+        case SBC_CHANNEL_MODE_STEREO:
+            sbc_info->sbc.mode = SBC_MODE_STEREO;
+            sample_spec->channels = 2;
+            break;
+        case SBC_CHANNEL_MODE_JOINT_STEREO:
+            sbc_info->sbc.mode = SBC_MODE_JOINT_STEREO;
+            sample_spec->channels = 2;
+            break;
+        default:
+            pa_assert_not_reached();
+    }
+
+    switch (config->allocation_method) {
+        case SBC_ALLOCATION_SNR:
+            sbc_info->sbc.allocation = SBC_AM_SNR;
+            break;
+        case SBC_ALLOCATION_LOUDNESS:
+            sbc_info->sbc.allocation = SBC_AM_LOUDNESS;
+            break;
+        default:
+            pa_assert_not_reached();
+    }
+
+    switch (config->subbands) {
+        case SBC_SUBBANDS_4:
+            sbc_info->sbc.subbands = SBC_SB_4;
+            break;
+        case SBC_SUBBANDS_8:
+            sbc_info->sbc.subbands = SBC_SB_8;
+            break;
+        default:
+            pa_assert_not_reached();
+    }
+
+    switch (config->block_length) {
+        case SBC_BLOCK_LENGTH_4:
+            sbc_info->sbc.blocks = SBC_BLK_4;
+            break;
+        case SBC_BLOCK_LENGTH_8:
+            sbc_info->sbc.blocks = SBC_BLK_8;
+            break;
+        case SBC_BLOCK_LENGTH_12:
+            sbc_info->sbc.blocks = SBC_BLK_12;
+            break;
+        case SBC_BLOCK_LENGTH_16:
+            sbc_info->sbc.blocks = SBC_BLK_16;
+            break;
+        default:
+            pa_assert_not_reached();
+    }
+
+    sbc_info->min_bitpool = config->min_bitpool;
+    sbc_info->max_bitpool = config->max_bitpool;
+
+    /* Set minimum bitpool for source to get the maximum possible block_size */
+    sbc_info->sbc.bitpool = is_a2dp_sink ? sbc_info->max_bitpool : sbc_info->min_bitpool;
+    sbc_info->codesize = sbc_get_codesize(&sbc_info->sbc);
+    sbc_info->frame_length = sbc_get_frame_length(&sbc_info->sbc);
+
+    pa_log_info("SBC parameters: allocation=%u, subbands=%u, blocks=%u, bitpool=%u",
+                sbc_info->sbc.allocation, sbc_info->sbc.subbands ? 8 : 4, sbc_info->sbc.blocks, sbc_info->sbc.bitpool);
+}
+
+static void pa_sbc_finish(void **info_ptr) {
+    sbc_info_t *sbc_info = (sbc_info_t *) *info_ptr;
+
+    if (!sbc_info)
+        return;
+
+    if (sbc_info->sbc_initialized)
+        sbc_finish(&sbc_info->sbc);
+
+    pa_xfree(sbc_info);
+    *info_ptr = NULL;
+}
+
+static void pa_sbc_set_bitpool(sbc_info_t *sbc_info, uint8_t bitpool) {
+    if (bitpool > sbc_info->max_bitpool)
+        bitpool = sbc_info->max_bitpool;
+    else if (bitpool < sbc_info->min_bitpool)
+        bitpool = sbc_info->min_bitpool;
+
+    sbc_info->sbc.bitpool = bitpool;
+
+    sbc_info->codesize = sbc_get_codesize(&sbc_info->sbc);
+    sbc_info->frame_length = sbc_get_frame_length(&sbc_info->sbc);
+
+    pa_log_debug("Bitpool has changed to %u", sbc_info->sbc.bitpool);
+}
+
+static void pa_sbc_setup(void **info_ptr) {
+    sbc_info_t *sbc_info = (sbc_info_t *) *info_ptr;
+
+    pa_sbc_set_bitpool(sbc_info, sbc_info->max_bitpool);
+}
+
+static void pa_sbc_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) {
+    sbc_info_t *sbc_info = (sbc_info_t *) *info_ptr;
+
+    *read_block_size =
+        (read_link_mtu - sizeof(struct rtp_header) - sizeof(struct rtp_payload))
+        / sbc_info->frame_length * sbc_info->codesize;
+
+    *write_block_size =
+        (write_link_mtu - sizeof(struct rtp_header) - sizeof(struct rtp_payload))
+        / sbc_info->frame_length * sbc_info->codesize;
+}
+
+static bool pa_sbc_fix_latency(void **info_ptr) {
+    sbc_info_t *sbc_info = (sbc_info_t *) *info_ptr;
+    uint8_t bitpool;
+
+    /* Check if bitpool is already at its limit */
+    if (sbc_info->sbc.bitpool <= SBC_BITPOOL_DEC_LIMIT)
+        return false;
+
+    bitpool = sbc_info->sbc.bitpool - SBC_BITPOOL_DEC_STEP;
+
+    if (bitpool < SBC_BITPOOL_DEC_LIMIT)
+        bitpool = SBC_BITPOOL_DEC_LIMIT;
+
+    if (sbc_info->sbc.bitpool == bitpool)
+        return false;
+
+    pa_sbc_set_bitpool(sbc_info, bitpool);
+    return true;
+}
+
+static size_t pa_sbc_encode(void **info_ptr, uint32_t timestamp, const uint8_t *input_buffer, size_t input_size, uint8_t *output_buffer, size_t output_size) {
+    sbc_info_t *sbc_info = (sbc_info_t *) *info_ptr;
+    struct rtp_header *header;
+    struct rtp_payload *payload;
+    uint8_t *d;
+    const uint8_t *p;
+    size_t to_write, to_encode;
+    unsigned frame_count;
+
+    header = (struct rtp_header*) output_buffer;
+    payload = (struct rtp_payload*) (output_buffer + sizeof(*header));
+
+    frame_count = 0;
+
+    p = input_buffer;
+    to_encode = input_size;
+
+    d = output_buffer + sizeof(*header) + sizeof(*payload);
+    to_write = output_size - sizeof(*header) - sizeof(*payload);
+
+    while (PA_LIKELY(to_encode > 0 && to_write > 0)) {
+        ssize_t written;
+        ssize_t encoded;
+
+        encoded = sbc_encode(&sbc_info->sbc,
+                             p, to_encode,
+                             d, to_write,
+                             &written);
+
+        if (PA_UNLIKELY(encoded <= 0)) {
+            pa_log_error("SBC encoding error (%li)", (long) encoded);
+            return 0;
+        }
+
+        pa_assert_fp((size_t) encoded <= to_encode);
+        pa_assert_fp((size_t) encoded == sbc_info->codesize);
+
+        pa_assert_fp((size_t) written <= to_write);
+        pa_assert_fp((size_t) written == sbc_info->frame_length);
+
+        p += encoded;
+        to_encode -= encoded;
+
+        d += written;
+        to_write -= written;
+
+        frame_count++;
+    }
+
+    pa_assert(to_encode == 0);
+
+    PA_ONCE_BEGIN {
+        pa_log_debug("Using SBC encoder implementation: %s", pa_strnull(sbc_get_implementation_info(&sbc_info->sbc)));
+    } PA_ONCE_END;
+
+    /* write it to the fifo */
+    memset(output_buffer, 0, sizeof(*header) + sizeof(*payload));
+    header->v = 2;
+    header->pt = 1;
+    header->sequence_number = htons(sbc_info->seq_num++);
+    header->timestamp = htonl(timestamp);
+    header->ssrc = htonl(1);
+    payload->frame_count = frame_count;
+
+    return d - output_buffer;
+}
+
+static size_t pa_sbc_decode(void **info_ptr, const uint8_t *input_buffer, size_t input_size, uint8_t *output_buffer, size_t output_size, size_t *processed) {
+    sbc_info_t *sbc_info = (sbc_info_t *) *info_ptr;
+
+    struct rtp_header *header;
+    struct rtp_payload *payload;
+    const uint8_t *p;
+    uint8_t *d;
+    size_t to_write, to_decode;
+
+    header = (struct rtp_header *) input_buffer;
+    payload = (struct rtp_payload*) (input_buffer + sizeof(*header));
+
+    p = input_buffer + sizeof(*header) + sizeof(*payload);
+    to_decode = input_size - sizeof(*header) - sizeof(*payload);
+
+    d = output_buffer;
+    to_write = output_size;
+
+    while (PA_LIKELY(to_decode > 0)) {
+        size_t written;
+        ssize_t decoded;
+
+        decoded = sbc_decode(&sbc_info->sbc,
+                             p, to_decode,
+                             d, to_write,
+                             &written);
+
+        if (PA_UNLIKELY(decoded <= 0)) {
+            pa_log_error("SBC decoding error (%li)", (long) decoded);
+            *processed = p - input_buffer;
+            return 0;
+        }
+
+        /* Reset frame length, it can be changed due to bitpool change */
+        sbc_info->frame_length = sbc_get_frame_length(&sbc_info->sbc);
+
+        pa_assert_fp((size_t) decoded <= to_decode);
+        pa_assert_fp((size_t) decoded == sbc_info->frame_length);
+
+        pa_assert_fp((size_t) written == sbc_info->codesize);
+
+        p += decoded;
+        to_decode -= decoded;
+
+        d += written;
+        to_write -= written;
+    }
+
+    *processed = p - input_buffer;
+    return d - output_buffer;
+}
+
+const pa_a2dp_codec_t pa_a2dp_codec_sbc = {
+    .codec_id = A2DP_CODEC_SBC,
+    .fill_capabilities = pa_sbc_fill_capabilities,
+    .validate_configuration = pa_sbc_validate_configuration,
+    .select_configuration = pa_sbc_select_configuration,
+    .init = pa_sbc_init,
+    .finish = pa_sbc_finish,
+    .setup = pa_sbc_setup,
+    .fill_blocksize = pa_sbc_fill_blocksize,
+    .fix_latency = pa_sbc_fix_latency,
+    .encode = pa_sbc_encode,
+    .decode = pa_sbc_decode,
+};
diff --git a/src/modules/bluetooth/pa-a2dp-codec.h b/src/modules/bluetooth/pa-a2dp-codec.h
new file mode 100644
index 000000000..68b1619c2
--- /dev/null
+++ b/src/modules/bluetooth/pa-a2dp-codec.h
@@ -0,0 +1,40 @@ 
+#ifndef foopaa2dpcodechfoo
+#define foopaa2dpcodechfoo
+
+/***
+  This file is part of PulseAudio.
+
+  Copyright 2018 Pali Rohár <pali.rohar@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
+  published by the Free Software Foundation; either version 2.1 of the
+  License, or (at your option) any later version.
+
+  PulseAudio is distributed in the hope that it will be useful, but
+  WITHOUT ANY WARRANTY; without even the implied warranty of
+  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+  General Public License for more details.
+
+  You should have received a copy of the GNU Lesser General Public
+  License along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
+***/
+
+typedef struct pa_a2dp_codec {
+    const char *codec_name;
+    uint8_t codec_id;
+    size_t (*fill_capabilities)(uint8_t *, size_t);
+    bool (*validate_configuration)(const uint8_t *, size_t);
+    size_t (*select_configuration)(const pa_sample_spec *, const uint8_t *, size_t, uint8_t *, size_t);
+    void (*init)(void **, pa_sample_spec *, bool, const uint8_t *, size_t);
+    void (*finish)(void **);
+    void (*setup)(void **);
+    void (*fill_blocksize)(void **, size_t, size_t, size_t *, size_t *);
+    bool (*fix_latency)(void **);
+    size_t (*encode)(void **, uint32_t, const uint8_t *, size_t, uint8_t *, size_t);
+    size_t (*decode)(void **, const uint8_t *, size_t, uint8_t *, size_t, size_t *);
+} pa_a2dp_codec_t;
+
+extern const pa_a2dp_codec_t pa_a2dp_codec_sbc;
+
+#endif

Comments

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@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
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@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
Hi Pali!

Thanks a lot for working on this! Arun has been strongly advocating
using GStreamer, and I don't know if his position is that "AptX must be
implemented with GStreamer or not implemented at all". I hope not. To
me the library choice is not important. Using libopenaptx directly
doesn't prevent us from switching to GStreamer later if someone writes
the code.

Another point that was raised that the codec choice shouldn't be bound
to the card profile. I tend to agree with that. There are situations
where module-bluetooth-policy or the user only wants to switch from HSP
to A2DP and not care about the codec. You asked how the codec should be
negotiated or switched by the user if it's not bound to the profile.
Regarding negotiation, we can add a hook that module-bluetooth-policy
can use to make the codec decision (if module-bluetooth-policy isn't
loaded, SBC seems like a sensible default).

Is there a need for the user to be able to choose the codec? Shouldn't
we always automatically pick the highest-quality one? I'm not against
adding the functionality, it would be useful for testing if nothing
else. It just doesn't seem very important.

We could have a "preferred-a2dp-codec" option in bluetooth.conf (that
file doesn't exist, but can be added). A per-card option would be
possible too, as would be having both a global option that could be
overridden with a per-card option.

For runtime configuration, we can add a command to set the codec
preference. This should be done with the message API that Georg Chini
has been working on (not yet finished). There's then need for saving
the choice too. We can either introduce a new database for bluetooth
stuff, or we can add some API to module-card-restore so that other
modules (module-bluetooth-discover in this case) can save arbitrary
parameters for cards.

I recall there was some lack of relevant APIs in bluetoothd for
choosing the codec from PulseAudio. Can you remind me, what are the
limitations, and how did you plan to deal with them?

Comments on the code below. This review was done over several days,
sorry for any inconsistencies or strange repetition (I noticed I
explain some things multiple times).

On Sat, 2018-07-28 at 17:34 +0200, 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 ++

The "pa-" prefix doesn't look very good to me. Maybe you didn't want to
add a2dp-codec.h, because it looks so similar to the existing a2dp-
codecs.h header? I think we can get rid of a2dp-codecs.h: the SBC stuff
can be moved to a2dp-codec-sbc.c, and the MPEG stuff can be dropped
since it's not used anywhere.

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

Cosmetic nitpicking: these can be merged with the earlier variable
assignemnts. Maybe you're aiming for some kind of "keep the SBC stuff
separate" modularization, but that's not really in line of how the rest
of the file is written.

> +
>  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@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, PA_BLUETOOTH_UUID_A2DP_SOURCE);
>          case PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT:
>              return !!pa_hashmap_get(device->uuids, PA_BLUETOOTH_UUID_HSP_HS)
> @@ -888,10 +889,21 @@ finish:
>  static void register_endpoint(pa_bluetooth_discovery *y, const char *path, const char *endpoint, const char *uuid) {
>      DBusMessage *m;
>      DBusMessageIter i, d;
> -    uint8_t codec = 0;
> +    uint8_t capabilities[1024];
> +    size_t capabilities_size;
> +    uint8_t codec_id;
> +    const pa_a2dp_codec_t *codec;
> +
> +    codec = pa_a2dp_endpoint_to_a2dp_codec(endpoint);

I think it would be better to change the function parameters so that
instead of an endpoint path the function would take a codec id. There
would be a function called get_a2dp_codec() that would take the codec
id as a parameter and return the codec struct. The endpoint path could
be added to the codec struct.

Shuffling things around like this wouldn't have huge benefits, I just
find looking up the codec based on the id neater than based on the
endpoint path.

I hope you don't have a problem with that suggestion, but if you do and
you want to keep using pa_a2dp_endpoint_to_a2dp_codec(), then the
function should be made static (it's not used outside bluez5-utils.c)
and the pa_ prefix should be dropped, since functions that aren't
exported through headers should not use the pa_ prefix according to our
coding style.

> +    if (!codec)
> +        return;

As far as I can tell, this should never happen, so an assertion would
be better (and it could be in the lookup function so that every caller
doesn't need to add a check).

>  
>      pa_log_debug("Registering %s on adapter %s", endpoint, path);
>  
> +    codec_id = codec->codec_id;
> +    capabilities_size = codec->fill_capabilities(capabilities, sizeof(capabilities));
> +    pa_assert(capabilities_size != 0);
> +
>      pa_assert_se(m = dbus_message_new_method_call(BLUEZ_SERVICE, path, BLUEZ_MEDIA_INTERFACE, "RegisterEndpoint"));
>  
>      dbus_message_iter_init_append(m, &i);
> @@ -899,23 +911,8 @@ static void register_endpoint(pa_bluetooth_discovery *y, const char *path, const
>      dbus_message_iter_open_container(&i, DBUS_TYPE_ARRAY, DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING DBUS_TYPE_STRING_AS_STRING
>                                           DBUS_TYPE_VARIANT_AS_STRING DBUS_DICT_ENTRY_END_CHAR_AS_STRING, &d);
>      pa_dbus_append_basic_variant_dict_entry(&d, "UUID", DBUS_TYPE_STRING, &uuid);
> -    pa_dbus_append_basic_variant_dict_entry(&d, "Codec", DBUS_TYPE_BYTE, &codec);
> -
> -    if (pa_streq(uuid, PA_BLUETOOTH_UUID_A2DP_SOURCE) || pa_streq(uuid, PA_BLUETOOTH_UUID_A2DP_SINK)) {
> -        a2dp_sbc_t capabilities;
> -
> -        capabilities.channel_mode = SBC_CHANNEL_MODE_MONO | SBC_CHANNEL_MODE_DUAL_CHANNEL | SBC_CHANNEL_MODE_STEREO |
> -                                    SBC_CHANNEL_MODE_JOINT_STEREO;
> -        capabilities.frequency = SBC_SAMPLING_FREQ_16000 | SBC_SAMPLING_FREQ_32000 | SBC_SAMPLING_FREQ_44100 |
> -                                 SBC_SAMPLING_FREQ_48000;
> -        capabilities.allocation_method = SBC_ALLOCATION_SNR | SBC_ALLOCATION_LOUDNESS;
> -        capabilities.subbands = SBC_SUBBANDS_4 | SBC_SUBBANDS_8;
> -        capabilities.block_length = SBC_BLOCK_LENGTH_4 | SBC_BLOCK_LENGTH_8 | SBC_BLOCK_LENGTH_12 | SBC_BLOCK_LENGTH_16;
> -        capabilities.min_bitpool = MIN_BITPOOL;
> -        capabilities.max_bitpool = MAX_BITPOOL;
> -
> -        pa_dbus_append_basic_array_variant_dict_entry(&d, "Capabilities", DBUS_TYPE_BYTE, &capabilities, sizeof(capabilities));
> -    }
> +    pa_dbus_append_basic_variant_dict_entry(&d, "Codec", DBUS_TYPE_BYTE, &codec_id);
> +    pa_dbus_append_basic_array_variant_dict_entry(&d, "Capabilities", DBUS_TYPE_BYTE, &capabilities, capabilities_size);
>  
>      dbus_message_iter_close_container(&i, &d);
>  
> @@ -964,8 +961,8 @@ static void parse_interfaces_and_properties(pa_bluetooth_discovery *y, DBusMessa
>              if (!a->valid)
>                  return;
>  
> -            register_endpoint(y, path, A2DP_SOURCE_ENDPOINT, PA_BLUETOOTH_UUID_A2DP_SOURCE);
> -            register_endpoint(y, path, A2DP_SINK_ENDPOINT, PA_BLUETOOTH_UUID_A2DP_SINK);
> +            register_endpoint(y, path, A2DP_SOURCE_SBC_ENDPOINT, PA_BLUETOOTH_UUID_A2DP_SOURCE);
> +            register_endpoint(y, path, A2DP_SINK_SBC_ENDPOINT, PA_BLUETOOTH_UUID_A2DP_SINK);
>  
>          } else if (pa_streq(interface, BLUEZ_DEVICE_INTERFACE)) {
>  
> @@ -1257,53 +1254,11 @@ fail:
>      return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
>  }
>  
> -static uint8_t a2dp_default_bitpool(uint8_t freq, uint8_t mode) {
> -    /* These bitpool values were chosen based on the A2DP spec recommendation */
> -    switch (freq) {
> -        case SBC_SAMPLING_FREQ_16000:
> -        case SBC_SAMPLING_FREQ_32000:
> -            return 53;
> -
> -        case SBC_SAMPLING_FREQ_44100:
> -
> -            switch (mode) {
> -                case SBC_CHANNEL_MODE_MONO:
> -                case SBC_CHANNEL_MODE_DUAL_CHANNEL:
> -                    return 31;
> -
> -                case SBC_CHANNEL_MODE_STEREO:
> -                case SBC_CHANNEL_MODE_JOINT_STEREO:
> -                    return 53;
> -            }
> -
> -            pa_log_warn("Invalid channel mode %u", mode);
> -            return 53;
> -
> -        case SBC_SAMPLING_FREQ_48000:
> -
> -            switch (mode) {
> -                case SBC_CHANNEL_MODE_MONO:
> -                case SBC_CHANNEL_MODE_DUAL_CHANNEL:
> -                    return 29;
> -
> -                case SBC_CHANNEL_MODE_STEREO:
> -                case SBC_CHANNEL_MODE_JOINT_STEREO:
> -                    return 51;
> -            }
> -
> -            pa_log_warn("Invalid channel mode %u", mode);
> -            return 51;
> -    }
> -
> -    pa_log_warn("Invalid sampling freq %u", freq);
> -    return 53;
> -}
> -
>  const char *pa_bluetooth_profile_to_string(pa_bluetooth_profile_t profile) {
>      switch(profile) {
> -        case PA_BLUETOOTH_PROFILE_A2DP_SINK:
> +        case PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK:
>              return "a2dp_sink";
> -        case PA_BLUETOOTH_PROFILE_A2DP_SOURCE:
> +        case PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE:
>              return "a2dp_source";
>          case PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT:
>              return "headset_head_unit";
> @@ -1316,6 +1271,38 @@ const char *pa_bluetooth_profile_to_string(pa_bluetooth_profile_t profile) {
>      return NULL;
>  }
>  
> +const char *pa_bluetooth_profile_to_a2dp_endpoint(pa_bluetooth_profile_t profile) {

This function isn't used outside bluez5-util.c, so it can be made
static and removed from bluez5-util.h. Then the pa_bluetooth_ prefix
should be dropped.

> +    switch (profile) {
> +        case PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK:
> +            return A2DP_SOURCE_SBC_ENDPOINT;
> +        case PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE:
> +            return A2DP_SINK_SBC_ENDPOINT;
> +        default:
> +            return NULL;

I think it would be good to use pa_assert_not_reached() here. I assume
this won't be used in a context where a non-a2dp profile would be
passed to the function.

> +    }
> +
> +    return NULL;

This is redundant.

> +}
> +
> +const pa_a2dp_codec_t *pa_bluetooth_profile_to_a2dp_codec(pa_bluetooth_profile_t profile) {
> +    switch (profile) {
> +        case PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK:
> +        case PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE:
> +            return &pa_a2dp_codec_sbc;
> +        default:
> +            return NULL;
> +    }
> +
> +    return NULL;
> +}

This function seems to be used also for checking whether a profile is
an a2dp profile. I think it would be clearer to have a separate
pa_bluetooth_profile_is_a2dp() function. Then this function could also
have an assertion rather than returning NULL for non-a2dp profiles.

If we don't tie the codec and the profile together, however, then
there's no need for an is_a2dp() function.

> +
> +const pa_a2dp_codec_t *pa_a2dp_endpoint_to_a2dp_codec(const char *endpoint) {
> +    if (pa_streq(endpoint, A2DP_SOURCE_SBC_ENDPOINT) || pa_streq(endpoint, A2DP_SINK_SBC_ENDPOINT))
> +        return &pa_a2dp_codec_sbc;
> +    else
> +        return NULL;

pa_assert_not_reached() could be used here.

> +}
> +
>  static DBusMessage *endpoint_set_configuration(DBusConnection *conn, DBusMessage *m, void *userdata) {
>      pa_bluetooth_discovery *y = userdata;
>      pa_bluetooth_device *d;
> @@ -1345,6 +1332,8 @@ static DBusMessage *endpoint_set_configuration(DBusConnection *conn, DBusMessage
>      if (dbus_message_iter_get_arg_type(&props) != DBUS_TYPE_DICT_ENTRY)
>          goto fail;
>  
> +    endpoint_path = dbus_message_get_path(m);
> +
>      /* Read transport properties */
>      while (dbus_message_iter_get_arg_type(&props) == DBUS_TYPE_DICT_ENTRY) {
>          const char *key;
> @@ -1367,13 +1356,12 @@ static DBusMessage *endpoint_set_configuration(DBusConnection *conn, DBusMessage
>  
>              dbus_message_iter_get_basic(&value, &uuid);
>  
> -            endpoint_path = dbus_message_get_path(m);
> -            if (pa_streq(endpoint_path, A2DP_SOURCE_ENDPOINT)) {
> +            if (pa_streq(endpoint_path, A2DP_SOURCE_SBC_ENDPOINT)) {
>                  if (pa_streq(uuid, PA_BLUETOOTH_UUID_A2DP_SOURCE))
> -                    p = PA_BLUETOOTH_PROFILE_A2DP_SINK;
> -            } else if (pa_streq(endpoint_path, A2DP_SINK_ENDPOINT)) {
> +                    p = PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK;
> +            } else if (pa_streq(endpoint_path, A2DP_SINK_SBC_ENDPOINT)) {
>                  if (pa_streq(uuid, PA_BLUETOOTH_UUID_A2DP_SINK))
> -                    p = PA_BLUETOOTH_PROFILE_A2DP_SOURCE;
> +                    p = PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE;
>              }
>  
>              if (p == PA_BLUETOOTH_PROFILE_OFF) {
> @@ -1389,7 +1377,7 @@ static DBusMessage *endpoint_set_configuration(DBusConnection *conn, DBusMessage
>              dbus_message_iter_get_basic(&value, &dev_path);
>          } else if (pa_streq(key, "Configuration")) {
>              DBusMessageIter array;
> -            a2dp_sbc_t *c;
> +            const pa_a2dp_codec_t *codec;
>  
>              if (var != DBUS_TYPE_ARRAY) {
>                  pa_log_error("Property %s of wrong type %c", key, (char)var);
> @@ -1404,40 +1392,12 @@ static DBusMessage *endpoint_set_configuration(DBusConnection *conn, DBusMessage
>              }
>  
>              dbus_message_iter_get_fixed_array(&array, &config, &size);
> -            if (size != sizeof(a2dp_sbc_t)) {
> -                pa_log_error("Configuration array of invalid size");
> -                goto fail;
> -            }
> -
> -            c = (a2dp_sbc_t *) config;
> -
> -            if (c->frequency != SBC_SAMPLING_FREQ_16000 && c->frequency != SBC_SAMPLING_FREQ_32000 &&
> -                c->frequency != SBC_SAMPLING_FREQ_44100 && c->frequency != SBC_SAMPLING_FREQ_48000) {
> -                pa_log_error("Invalid sampling frequency in configuration");
> -                goto fail;
> -            }
> -
> -            if (c->channel_mode != SBC_CHANNEL_MODE_MONO && c->channel_mode != SBC_CHANNEL_MODE_DUAL_CHANNEL &&
> -                c->channel_mode != SBC_CHANNEL_MODE_STEREO && c->channel_mode != SBC_CHANNEL_MODE_JOINT_STEREO) {
> -                pa_log_error("Invalid channel mode in configuration");
> -                goto fail;
> -            }
> -
> -            if (c->allocation_method != SBC_ALLOCATION_SNR && c->allocation_method != SBC_ALLOCATION_LOUDNESS) {
> -                pa_log_error("Invalid allocation method in configuration");
> -                goto fail;
> -            }
>  
> -            if (c->subbands != SBC_SUBBANDS_4 && c->subbands != SBC_SUBBANDS_8) {
> -                pa_log_error("Invalid SBC subbands in configuration");
> -                goto fail;
> -            }
> +            codec = pa_a2dp_endpoint_to_a2dp_codec(endpoint_path);
> +            pa_assert(codec);
>  
> -            if (c->block_length != SBC_BLOCK_LENGTH_4 && c->block_length != SBC_BLOCK_LENGTH_8 &&
> -                c->block_length != SBC_BLOCK_LENGTH_12 && c->block_length != SBC_BLOCK_LENGTH_16) {
> -                pa_log_error("Invalid block length in configuration");
> +            if (!codec->validate_configuration(config, size))
>                  goto fail;
> -            }
>          }
>  
>          dbus_message_iter_next(&props);
> @@ -1484,21 +1444,17 @@ fail2:
>  
>  static DBusMessage *endpoint_select_configuration(DBusConnection *conn, DBusMessage *m, void *userdata) {
>      pa_bluetooth_discovery *y = userdata;
> -    a2dp_sbc_t *cap, config;
> -    uint8_t *pconf = (uint8_t *) &config;
> -    int i, size;
> +    const char *endpoint_path;
> +    uint8_t *cap;
> +    int size;
> +    const pa_a2dp_codec_t *codec;
> +    uint8_t config[1024];
> +    uint8_t *config_ptr = config;
> +    size_t config_size;
>      DBusMessage *r;
>      DBusError err;
>  
> -    static const struct {
> -        uint32_t rate;
> -        uint8_t cap;
> -    } freq_table[] = {
> -        { 16000U, SBC_SAMPLING_FREQ_16000 },
> -        { 32000U, SBC_SAMPLING_FREQ_32000 },
> -        { 44100U, SBC_SAMPLING_FREQ_44100 },
> -        { 48000U, SBC_SAMPLING_FREQ_48000 }
> -    };
> +    endpoint_path = dbus_message_get_path(m);
>  
>      dbus_error_init(&err);
>  
> @@ -1508,101 +1464,14 @@ static DBusMessage *endpoint_select_configuration(DBusConnection *conn, DBusMess
>          goto fail;
>      }
>  
> -    if (size != sizeof(config)) {
> -        pa_log_error("Capabilities array has invalid size");
> -        goto fail;
> -    }
> -
> -    pa_zero(config);
> -
> -    /* Find the lowest freq that is at least as high as the requested sampling rate */
> -    for (i = 0; (unsigned) i < PA_ELEMENTSOF(freq_table); i++)
> -        if (freq_table[i].rate >= y->core->default_sample_spec.rate && (cap->frequency & freq_table[i].cap)) {
> -            config.frequency = freq_table[i].cap;
> -            break;
> -        }
> -
> -    if ((unsigned) i == PA_ELEMENTSOF(freq_table)) {
> -        for (--i; i >= 0; i--) {
> -            if (cap->frequency & freq_table[i].cap) {
> -                config.frequency = freq_table[i].cap;
> -                break;
> -            }
> -        }
> -
> -        if (i < 0) {
> -            pa_log_error("Not suitable sample rate");
> -            goto fail;
> -        }
> -    }
> -
> -    pa_assert((unsigned) i < PA_ELEMENTSOF(freq_table));
> -
> -    if (y->core->default_sample_spec.channels <= 1) {
> -        if (cap->channel_mode & SBC_CHANNEL_MODE_MONO)
> -            config.channel_mode = SBC_CHANNEL_MODE_MONO;
> -        else if (cap->channel_mode & SBC_CHANNEL_MODE_JOINT_STEREO)
> -            config.channel_mode = SBC_CHANNEL_MODE_JOINT_STEREO;
> -        else if (cap->channel_mode & SBC_CHANNEL_MODE_STEREO)
> -            config.channel_mode = SBC_CHANNEL_MODE_STEREO;
> -        else if (cap->channel_mode & SBC_CHANNEL_MODE_DUAL_CHANNEL)
> -            config.channel_mode = SBC_CHANNEL_MODE_DUAL_CHANNEL;
> -        else {
> -            pa_log_error("No supported channel modes");
> -            goto fail;
> -        }
> -    }
> -
> -    if (y->core->default_sample_spec.channels >= 2) {
> -        if (cap->channel_mode & SBC_CHANNEL_MODE_JOINT_STEREO)
> -            config.channel_mode = SBC_CHANNEL_MODE_JOINT_STEREO;
> -        else if (cap->channel_mode & SBC_CHANNEL_MODE_STEREO)
> -            config.channel_mode = SBC_CHANNEL_MODE_STEREO;
> -        else if (cap->channel_mode & SBC_CHANNEL_MODE_DUAL_CHANNEL)
> -            config.channel_mode = SBC_CHANNEL_MODE_DUAL_CHANNEL;
> -        else if (cap->channel_mode & SBC_CHANNEL_MODE_MONO)
> -            config.channel_mode = SBC_CHANNEL_MODE_MONO;
> -        else {
> -            pa_log_error("No supported channel modes");
> -            goto fail;
> -        }
> -    }
> -
> -    if (cap->block_length & SBC_BLOCK_LENGTH_16)
> -        config.block_length = SBC_BLOCK_LENGTH_16;
> -    else if (cap->block_length & SBC_BLOCK_LENGTH_12)
> -        config.block_length = SBC_BLOCK_LENGTH_12;
> -    else if (cap->block_length & SBC_BLOCK_LENGTH_8)
> -        config.block_length = SBC_BLOCK_LENGTH_8;
> -    else if (cap->block_length & SBC_BLOCK_LENGTH_4)
> -        config.block_length = SBC_BLOCK_LENGTH_4;
> -    else {
> -        pa_log_error("No supported block lengths");
> -        goto fail;
> -    }
> -
> -    if (cap->subbands & SBC_SUBBANDS_8)
> -        config.subbands = SBC_SUBBANDS_8;
> -    else if (cap->subbands & SBC_SUBBANDS_4)
> -        config.subbands = SBC_SUBBANDS_4;
> -    else {
> -        pa_log_error("No supported subbands");
> -        goto fail;
> -    }
> +    codec = pa_a2dp_endpoint_to_a2dp_codec(endpoint_path);
> +    pa_assert(codec);
>  
> -    if (cap->allocation_method & SBC_ALLOCATION_LOUDNESS)
> -        config.allocation_method = SBC_ALLOCATION_LOUDNESS;
> -    else if (cap->allocation_method & SBC_ALLOCATION_SNR)
> -        config.allocation_method = SBC_ALLOCATION_SNR;
> -
> -    config.min_bitpool = (uint8_t) PA_MAX(MIN_BITPOOL, cap->min_bitpool);
> -    config.max_bitpool = (uint8_t) PA_MIN(a2dp_default_bitpool(config.frequency, config.channel_mode), cap->max_bitpool);
> -
> -    if (config.min_bitpool > config.max_bitpool)
> -        goto fail;
> +    config_size = codec->select_configuration(&y->core->default_sample_spec, cap, size, config, sizeof(config));
> +    pa_assert(config_size != 0);

We don't trust input from bluetoothd to be always valid, so
select_configuration() can fail. Proper error handling is needed here.

>  
>      pa_assert_se(r = dbus_message_new_method_return(m));
> -    pa_assert_se(dbus_message_append_args(r, DBUS_TYPE_ARRAY, DBUS_TYPE_BYTE, &pconf, size, DBUS_TYPE_INVALID));
> +    pa_assert_se(dbus_message_append_args(r, DBUS_TYPE_ARRAY, DBUS_TYPE_BYTE, &config_ptr, config_size, DBUS_TYPE_INVALID));
>  
>      return r;
>  
> @@ -1677,7 +1546,7 @@ static DBusHandlerResult endpoint_handler(DBusConnection *c, DBusMessage *m, voi
>  
>      pa_log_debug("dbus: path=%s, interface=%s, member=%s", path, interface, member);
>  
> -    if (!pa_streq(path, A2DP_SOURCE_ENDPOINT) && !pa_streq(path, A2DP_SINK_ENDPOINT))
> +    if (!pa_streq(path, A2DP_SOURCE_SBC_ENDPOINT) && !pa_streq(path, A2DP_SINK_SBC_ENDPOINT))
>          return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
>  
>      if (dbus_message_is_method_call(m, "org.freedesktop.DBus.Introspectable", "Introspect")) {
> @@ -1709,38 +1578,22 @@ static void endpoint_init(pa_bluetooth_discovery *y, pa_bluetooth_profile_t prof
>      static const DBusObjectPathVTable vtable_endpoint = {
>          .message_function = endpoint_handler,
>      };
> +    const char *endpoint = pa_bluetooth_profile_to_a2dp_endpoint(profile);

The function could just take the endpoint path as an argument.
pa_bluetooth_profile_t is not needed by this function.

>  
>      pa_assert(y);
> +    pa_assert(endpoint);
>  
> -    switch(profile) {
> -        case PA_BLUETOOTH_PROFILE_A2DP_SINK:
> -            pa_assert_se(dbus_connection_register_object_path(pa_dbus_connection_get(y->connection), A2DP_SOURCE_ENDPOINT,
> -                                                              &vtable_endpoint, y));
> -            break;
> -        case PA_BLUETOOTH_PROFILE_A2DP_SOURCE:
> -            pa_assert_se(dbus_connection_register_object_path(pa_dbus_connection_get(y->connection), A2DP_SINK_ENDPOINT,
> -                                                              &vtable_endpoint, y));
> -            break;
> -        default:
> -            pa_assert_not_reached();
> -            break;
> -    }
> +    pa_assert_se(dbus_connection_register_object_path(pa_dbus_connection_get(y->connection), endpoint,
> +                                                      &vtable_endpoint, y));
>  }
>  
>  static void endpoint_done(pa_bluetooth_discovery *y, pa_bluetooth_profile_t profile) {
> +    const char *endpoint = pa_bluetooth_profile_to_a2dp_endpoint(profile);

Same comment as above.

> +
>      pa_assert(y);
> +    pa_assert(endpoint);
>  
> -    switch(profile) {
> -        case PA_BLUETOOTH_PROFILE_A2DP_SINK:
> -            dbus_connection_unregister_object_path(pa_dbus_connection_get(y->connection), A2DP_SOURCE_ENDPOINT);
> -            break;
> -        case PA_BLUETOOTH_PROFILE_A2DP_SOURCE:
> -            dbus_connection_unregister_object_path(pa_dbus_connection_get(y->connection), A2DP_SINK_ENDPOINT);
> -            break;
> -        default:
> -            pa_assert_not_reached();
> -            break;
> -    }
> +    dbus_connection_unregister_object_path(pa_dbus_connection_get(y->connection), endpoint);
>  }
>  
>  pa_bluetooth_discovery* pa_bluetooth_discovery_get(pa_core *c, int headset_backend) {
> @@ -1799,8 +1652,8 @@ pa_bluetooth_discovery* pa_bluetooth_discovery_get(pa_core *c, int headset_backe
>      }
>      y->matches_added = true;
>  
> -    endpoint_init(y, PA_BLUETOOTH_PROFILE_A2DP_SINK);
> -    endpoint_init(y, PA_BLUETOOTH_PROFILE_A2DP_SOURCE);
> +    endpoint_init(y, PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK);
> +    endpoint_init(y, PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE);
>  
>      get_managed_objects(y);
>  
> @@ -1868,8 +1721,8 @@ 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_SINK);
> -        endpoint_done(y, PA_BLUETOOTH_PROFILE_A2DP_SOURCE);
> +        endpoint_done(y, PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK);
> +        endpoint_done(y, PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE);
>  
>          pa_dbus_connection_unref(y->connection);
>      }
> diff --git a/src/modules/bluetooth/bluez5-util.h b/src/modules/bluetooth/bluez5-util.h
> index ad30708f0..365b9ef6f 100644
> --- a/src/modules/bluetooth/bluez5-util.h
> +++ b/src/modules/bluetooth/bluez5-util.h
> @@ -5,6 +5,7 @@
>    This file is part of PulseAudio.
>  
>    Copyright 2008-2013 João Paulo Rechi Vita
> +  Copyrigth 2018      Pali Rohár <pali.rohar@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
> @@ -22,6 +23,8 @@
>  
>  #include <pulsecore/core.h>
>  
> +#include "pa-a2dp-codec.h"
> +
>  #define PA_BLUETOOTH_UUID_A2DP_SOURCE "0000110a-0000-1000-8000-00805f9b34fb"
>  #define PA_BLUETOOTH_UUID_A2DP_SINK   "0000110b-0000-1000-8000-00805f9b34fb"
>  
> @@ -51,8 +54,8 @@ typedef enum pa_bluetooth_hook {
>  } pa_bluetooth_hook_t;
>  
>  typedef enum profile {
> -    PA_BLUETOOTH_PROFILE_A2DP_SINK,
> -    PA_BLUETOOTH_PROFILE_A2DP_SOURCE,
> +    PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK,
> +    PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE,
>      PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT,
>      PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY,
>      PA_BLUETOOTH_PROFILE_OFF
> @@ -163,6 +166,9 @@ pa_bluetooth_device* pa_bluetooth_discovery_get_device_by_address(pa_bluetooth_d
>  pa_hook* pa_bluetooth_discovery_hook(pa_bluetooth_discovery *y, pa_bluetooth_hook_t hook);
>  
>  const char *pa_bluetooth_profile_to_string(pa_bluetooth_profile_t profile);
> +const char *pa_bluetooth_profile_to_a2dp_endpoint(pa_bluetooth_profile_t profile);
> +const pa_a2dp_codec_t *pa_bluetooth_profile_to_a2dp_codec(pa_bluetooth_profile_t profile);
> +const pa_a2dp_codec_t *pa_a2dp_endpoint_to_a2dp_codec(const char *endpoint);
>  
>  static inline bool pa_bluetooth_uuid_is_hsp_hs(const char *uuid) {
>      return pa_streq(uuid, PA_BLUETOOTH_UUID_HSP_HS) || pa_streq(uuid, PA_BLUETOOTH_UUID_HSP_HS_ALT);
> diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
> index 9dbdca316..e626e80e9 100644
> --- a/src/modules/bluetooth/module-bluez5-device.c
> +++ b/src/modules/bluetooth/module-bluez5-device.c
> @@ -3,6 +3,7 @@
>  
>    Copyright 2008-2013 João Paulo Rechi Vita
>    Copyright 2011-2013 BMW Car IT GmbH.
> +  Copyright 2018      Pali Rohár <pali.rohar@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
> @@ -25,7 +26,6 @@
>  #include <errno.h>
>  
>  #include <arpa/inet.h>
> -#include <sbc/sbc.h>
>  
>  #include <pulse/rtclock.h>
>  #include <pulse/timeval.h>
> @@ -47,8 +47,8 @@
>  #include <pulsecore/time-smoother.h>
>  
>  #include "a2dp-codecs.h"
> +#include "pa-a2dp-codec.h"
>  #include "bluez5-util.h"
> -#include "rtp.h"
>  
>  PA_MODULE_AUTHOR("João Paulo Rechi Vita");
>  PA_MODULE_DESCRIPTION("BlueZ 5 Bluetooth audio sink and source");
> @@ -62,8 +62,6 @@ PA_MODULE_USAGE("path=<device object path>"
>  #define FIXED_LATENCY_RECORD_A2DP   (25 * PA_USEC_PER_MSEC)
>  #define FIXED_LATENCY_RECORD_SCO    (25 * PA_USEC_PER_MSEC)
>  
> -#define BITPOOL_DEC_LIMIT 32
> -#define BITPOOL_DEC_STEP 5
>  #define HSP_MAX_GAIN 15
>  
>  static const char* const valid_modargs[] = {
> @@ -94,18 +92,6 @@ typedef struct bluetooth_msg {
>  PA_DEFINE_PRIVATE_CLASS(bluetooth_msg, pa_msgobject);
>  #define BLUETOOTH_MSG(o) (bluetooth_msg_cast(o))
>  
> -typedef struct sbc_info {
> -    sbc_t sbc;                           /* Codec data */
> -    bool sbc_initialized;                /* Keep track if the encoder is initialized */
> -    size_t codesize, frame_length;       /* SBC Codesize, frame_length. We simply cache those values here */
> -    uint16_t seq_num;                    /* Cumulative packet sequence */
> -    uint8_t min_bitpool;
> -    uint8_t max_bitpool;
> -
> -    void* buffer;                        /* Codec transfer buffer */
> -    size_t buffer_size;                  /* Size of the buffer */
> -} sbc_info_t;
> -
>  struct userdata {
>      pa_module *module;
>      pa_core *core;
> @@ -146,7 +132,11 @@ struct userdata {
>      pa_smoother *read_smoother;
>      pa_memchunk write_memchunk;
>      pa_sample_spec sample_spec;
> -    struct sbc_info sbc_info;
> +    void *encoder_info;
> +    void *decoder_info;

I think it would be better to put these inside the pa_a2dp_codec
struct, preferably behind a single void pointer named "userdata" (as
that would follow the usual PA code conventions). The various callbacks
in pa_a2dp_codec that now take a void** parameter as their first
argument would take a pa_a2dp_codec pointer instead.

> +
> +    void* buffer;                        /* Codec transfer buffer */
> +    size_t buffer_size;                  /* Size of the buffer */
>  };
>  
>  typedef enum pa_bluetooth_form_factor {
> @@ -418,105 +408,22 @@ static void a2dp_prepare_buffer(struct userdata *u) {
>  
>      pa_assert(u);
>  
> -    if (u->sbc_info.buffer_size >= min_buffer_size)
> +    if (u->buffer_size >= min_buffer_size)
>          return;
>  
> -    u->sbc_info.buffer_size = 2 * min_buffer_size;
> -    pa_xfree(u->sbc_info.buffer);
> -    u->sbc_info.buffer = pa_xmalloc(u->sbc_info.buffer_size);
> +    u->buffer_size = 2 * min_buffer_size;
> +    pa_xfree(u->buffer);
> +    u->buffer = pa_xmalloc(u->buffer_size);
>  }
>  
>  /* Run from IO thread */
> -static int a2dp_process_render(struct userdata *u) {
> -    struct sbc_info *sbc_info;
> -    struct rtp_header *header;
> -    struct rtp_payload *payload;
> -    size_t nbytes;
> -    void *d;
> -    const void *p;
> -    size_t to_write, to_encode;
> -    unsigned frame_count;
> +static int a2dp_write_buffer(struct userdata *u, size_t nbytes) {
>      int ret = 0;
>  
> -    pa_assert(u);
> -    pa_assert(u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK);
> -    pa_assert(u->sink);
> -
> -    /* First, render some data */
> -    if (!u->write_memchunk.memblock)
> -        pa_sink_render_full(u->sink, u->write_block_size, &u->write_memchunk);
> -
> -    pa_assert(u->write_memchunk.length == u->write_block_size);
> -
> -    a2dp_prepare_buffer(u);
> -
> -    sbc_info = &u->sbc_info;
> -    header = sbc_info->buffer;
> -    payload = (struct rtp_payload*) ((uint8_t*) sbc_info->buffer + sizeof(*header));
> -
> -    frame_count = 0;
> -
> -    /* Try to create a packet of the full MTU */
> -
> -    p = (const uint8_t *) pa_memblock_acquire_chunk(&u->write_memchunk);
> -    to_encode = u->write_memchunk.length;
> -
> -    d = (uint8_t*) sbc_info->buffer + sizeof(*header) + sizeof(*payload);
> -    to_write = sbc_info->buffer_size - sizeof(*header) - sizeof(*payload);
> -
> -    while (PA_LIKELY(to_encode > 0 && to_write > 0)) {
> -        ssize_t written;
> -        ssize_t encoded;
> -
> -        encoded = sbc_encode(&sbc_info->sbc,
> -                             p, to_encode,
> -                             d, to_write,
> -                             &written);
> -
> -        if (PA_UNLIKELY(encoded <= 0)) {
> -            pa_log_error("SBC encoding error (%li)", (long) encoded);
> -            pa_memblock_release(u->write_memchunk.memblock);
> -            return -1;
> -        }
> -
> -        pa_assert_fp((size_t) encoded <= to_encode);
> -        pa_assert_fp((size_t) encoded == sbc_info->codesize);
> -
> -        pa_assert_fp((size_t) written <= to_write);
> -        pa_assert_fp((size_t) written == sbc_info->frame_length);
> -
> -        p = (const uint8_t*) p + encoded;
> -        to_encode -= encoded;
> -
> -        d = (uint8_t*) d + written;
> -        to_write -= written;
> -
> -        frame_count++;
> -    }
> -
> -    pa_memblock_release(u->write_memchunk.memblock);
> -
> -    pa_assert(to_encode == 0);
> -
> -    PA_ONCE_BEGIN {
> -        pa_log_debug("Using SBC encoder implementation: %s", pa_strnull(sbc_get_implementation_info(&sbc_info->sbc)));
> -    } PA_ONCE_END;
> -
> -    /* write it to the fifo */
> -    memset(sbc_info->buffer, 0, sizeof(*header) + sizeof(*payload));
> -    header->v = 2;
> -    header->pt = 1;
> -    header->sequence_number = htons(sbc_info->seq_num++);
> -    header->timestamp = htonl(u->write_index / pa_frame_size(&u->sample_spec));
> -    header->ssrc = htonl(1);
> -    payload->frame_count = frame_count;
> -
> -    nbytes = (uint8_t*) d - (uint8_t*) sbc_info->buffer;
> -
>      for (;;) {
>          ssize_t l;
>  
> -        l = pa_write(u->stream_fd, sbc_info->buffer, nbytes, &u->stream_write_type);
> +        l = pa_write(u->stream_fd, u->buffer, nbytes, &u->stream_write_type);
>  
>          pa_assert(l != 0);
>  
> @@ -560,37 +467,65 @@ static int a2dp_process_render(struct userdata *u) {
>  }
>  
>  /* Run from IO thread */
> +static int a2dp_process_render(struct userdata *u) {
> +    const pa_a2dp_codec_t *codec;
> +    const uint8_t *ptr;
> +    size_t length;
> +
> +    pa_assert(u);
> +    pa_assert(u->sink);
> +
> +    codec = pa_bluetooth_profile_to_a2dp_codec(u->profile);
> +    pa_assert(codec);
> +
> +    /* First, render some data */
> +    if (!u->write_memchunk.memblock)
> +        pa_sink_render_full(u->sink, u->write_block_size, &u->write_memchunk);
> +
> +    pa_assert(u->write_memchunk.length == u->write_block_size);
> +
> +    a2dp_prepare_buffer(u);
> +
> +    /* Try to create a packet of the full MTU */
> +    ptr = (const uint8_t *) pa_memblock_acquire_chunk(&u->write_memchunk);
> +
> +    length = codec->encode(&u->encoder_info, u->write_index / pa_frame_size(&u->sample_spec), ptr, u->write_memchunk.length, u->buffer, u->buffer_size);
> +
> +    pa_memblock_release(u->write_memchunk.memblock);
> +
> +    if (length == 0)
> +        return -1;
> +
> +    return a2dp_write_buffer(u, length);
> +}
> +
> +/* Run from IO thread */
>  static int a2dp_process_push(struct userdata *u) {
> +    const pa_a2dp_codec_t *codec;
>      int ret = 0;
>      pa_memchunk memchunk;
>  
>      pa_assert(u);
> -    pa_assert(u->profile == PA_BLUETOOTH_PROFILE_A2DP_SOURCE);
>      pa_assert(u->source);
>      pa_assert(u->read_smoother);
>  
> +    codec = pa_bluetooth_profile_to_a2dp_codec(u->profile);
> +    pa_assert(codec);
> +
>      memchunk.memblock = pa_memblock_new(u->core->mempool, u->read_block_size);
>      memchunk.index = memchunk.length = 0;
>  
>      for (;;) {
>          bool found_tstamp = false;
>          pa_usec_t tstamp;
> -        struct sbc_info *sbc_info;
> -        struct rtp_header *header;
> -        struct rtp_payload *payload;
> -        const void *p;
> -        void *d;
> +        uint8_t *ptr;
>          ssize_t l;
> -        size_t to_write, to_decode;
> +        size_t processed;
>          size_t total_written = 0;
>  
>          a2dp_prepare_buffer(u);
>  
> -        sbc_info = &u->sbc_info;
> -        header = sbc_info->buffer;
> -        payload = (struct rtp_payload*) ((uint8_t*) sbc_info->buffer + sizeof(*header));
> -
> -        l = pa_read(u->stream_fd, sbc_info->buffer, sbc_info->buffer_size, &u->stream_write_type);
> +        l = pa_read(u->stream_fd, u->buffer, u->buffer_size, &u->stream_write_type);
>  
>          if (l <= 0) {
>  
> @@ -607,7 +542,7 @@ static int a2dp_process_push(struct userdata *u) {
>              break;
>          }
>  
> -        pa_assert((size_t) l <= sbc_info->buffer_size);
> +        pa_assert((size_t) l <= u->buffer_size);
>  
>          /* TODO: get timestamp from rtp */
>          if (!found_tstamp) {
> @@ -615,50 +550,18 @@ static int a2dp_process_push(struct userdata *u) {
>              tstamp = pa_rtclock_now();
>          }
>  
> -        p = (uint8_t*) sbc_info->buffer + sizeof(*header) + sizeof(*payload);
> -        to_decode = l - sizeof(*header) - sizeof(*payload);
> -
> -        d = pa_memblock_acquire(memchunk.memblock);
> -        to_write = memchunk.length = pa_memblock_get_length(memchunk.memblock);
> -
> -        while (PA_LIKELY(to_decode > 0)) {
> -            size_t written;
> -            ssize_t decoded;
> -
> -            decoded = sbc_decode(&sbc_info->sbc,
> -                                 p, to_decode,
> -                                 d, to_write,
> -                                 &written);
> -
> -            if (PA_UNLIKELY(decoded <= 0)) {
> -                pa_log_error("SBC decoding error (%li)", (long) decoded);
> -                pa_memblock_release(memchunk.memblock);
> -                pa_memblock_unref(memchunk.memblock);
> -                return 0;
> -            }
> -
> -            total_written += written;
> -
> -            /* Reset frame length, it can be changed due to bitpool change */
> -            sbc_info->frame_length = sbc_get_frame_length(&sbc_info->sbc);
> -
> -            pa_assert_fp((size_t) decoded <= to_decode);
> -            pa_assert_fp((size_t) decoded == sbc_info->frame_length);
> +        ptr = pa_memblock_acquire(memchunk.memblock);
> +        memchunk.length = pa_memblock_get_length(memchunk.memblock);
>  
> -            pa_assert_fp((size_t) written == sbc_info->codesize);
> -
> -            p = (const uint8_t*) p + decoded;
> -            to_decode -= decoded;
> -
> -            d = (uint8_t*) d + written;
> -            to_write -= written;
> -        }
> +        total_written = codec->decode(&u->decoder_info, u->buffer, l, ptr, memchunk.length, &processed);
> +        if (total_written == 0)
> +            return 0;
>  
>          u->read_index += (uint64_t) total_written;
>          pa_smoother_put(u->read_smoother, tstamp, pa_bytes_to_usec(u->read_index, &u->sample_spec));
>          pa_smoother_resume(u->read_smoother, tstamp, true);
>  
> -        memchunk.length -= to_write;
> +        memchunk.length -= l - processed;
>  
>          pa_memblock_release(memchunk.memblock);
>  
> @@ -705,72 +608,6 @@ static void update_buffer_size(struct userdata *u) {
>      }
>  }
>  
> -/* Run from I/O thread */
> -static void a2dp_set_bitpool(struct userdata *u, uint8_t bitpool) {
> -    struct sbc_info *sbc_info;
> -
> -    pa_assert(u);
> -
> -    sbc_info = &u->sbc_info;
> -
> -    if (sbc_info->sbc.bitpool == bitpool)
> -        return;
> -
> -    if (bitpool > sbc_info->max_bitpool)
> -        bitpool = sbc_info->max_bitpool;
> -    else if (bitpool < sbc_info->min_bitpool)
> -        bitpool = sbc_info->min_bitpool;
> -
> -    sbc_info->sbc.bitpool = bitpool;
> -
> -    sbc_info->codesize = sbc_get_codesize(&sbc_info->sbc);
> -    sbc_info->frame_length = sbc_get_frame_length(&sbc_info->sbc);
> -
> -    pa_log_debug("Bitpool has changed to %u", sbc_info->sbc.bitpool);
> -
> -    u->read_block_size =
> -        (u->read_link_mtu - sizeof(struct rtp_header) - sizeof(struct rtp_payload))
> -        / sbc_info->frame_length * sbc_info->codesize;
> -
> -    u->write_block_size =
> -        (u->write_link_mtu - sizeof(struct rtp_header) - sizeof(struct rtp_payload))
> -        / sbc_info->frame_length * sbc_info->codesize;
> -
> -    pa_sink_set_max_request_within_thread(u->sink, u->write_block_size);
> -    pa_sink_set_fixed_latency_within_thread(u->sink,
> -            FIXED_LATENCY_PLAYBACK_A2DP + pa_bytes_to_usec(u->write_block_size, &u->sample_spec));
> -
> -    /* If there is still data in the memchunk, we have to discard it
> -     * because the write_block_size may have changed. */
> -    if (u->write_memchunk.memblock) {
> -        pa_memblock_unref(u->write_memchunk.memblock);
> -        pa_memchunk_reset(&u->write_memchunk);
> -    }
> -
> -    update_buffer_size(u);
> -}
> -
> -/* Run from I/O thread */
> -static void a2dp_reduce_bitpool(struct userdata *u) {
> -    struct sbc_info *sbc_info;
> -    uint8_t bitpool;
> -
> -    pa_assert(u);
> -
> -    sbc_info = &u->sbc_info;
> -
> -    /* Check if bitpool is already at its limit */
> -    if (sbc_info->sbc.bitpool <= BITPOOL_DEC_LIMIT)
> -        return;
> -
> -    bitpool = sbc_info->sbc.bitpool - BITPOOL_DEC_STEP;
> -
> -    if (bitpool < BITPOOL_DEC_LIMIT)
> -        bitpool = BITPOOL_DEC_LIMIT;
> -
> -    a2dp_set_bitpool(u, bitpool);
> -}
> -
>  static void teardown_stream(struct userdata *u) {
>      if (u->rtpoll_item) {
>          pa_rtpoll_item_free(u->rtpoll_item);
> @@ -860,32 +697,37 @@ static void transport_config_mtu(struct userdata *u) {
>              u->write_block_size = pa_frame_align(u->write_block_size, &u->sink->sample_spec);
>          }
>      } else {
> -        u->read_block_size =
> -            (u->read_link_mtu - sizeof(struct rtp_header) - sizeof(struct rtp_payload))
> -            / u->sbc_info.frame_length * u->sbc_info.codesize;
> -
> -        u->write_block_size =
> -            (u->write_link_mtu - sizeof(struct rtp_header) - sizeof(struct rtp_payload))
> -            / u->sbc_info.frame_length * u->sbc_info.codesize;
> +        const pa_a2dp_codec_t *codec = pa_bluetooth_profile_to_a2dp_codec(u->profile);
> +        pa_assert(codec);
> +        pa_assert(u->sink || u->source);
> +        codec->fill_blocksize(u->sink ? &u->encoder_info : &u->decoder_info, u->read_link_mtu, u->write_link_mtu, &u->read_block_size, &u->write_block_size);
>      }
>  
>      if (u->sink) {
>          pa_sink_set_max_request_within_thread(u->sink, u->write_block_size);
>          pa_sink_set_fixed_latency_within_thread(u->sink,
> -                                                (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK ?
> +                                                (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK ?
>                                                   FIXED_LATENCY_PLAYBACK_A2DP : FIXED_LATENCY_PLAYBACK_SCO) +
>                                                  pa_bytes_to_usec(u->write_block_size, &u->sample_spec));
> +
> +        /* If there is still data in the memchunk, we have to discard it
> +         * because the write_block_size may have changed. */
> +        if (u->write_memchunk.memblock) {
> +            pa_memblock_unref(u->write_memchunk.memblock);
> +            pa_memchunk_reset(&u->write_memchunk);
> +        }
>      }
>  
>      if (u->source)
>          pa_source_set_fixed_latency_within_thread(u->source,
> -                                                  (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SOURCE ?
> +                                                  (pa_bluetooth_profile_to_a2dp_codec(u->profile) ?
>                                                     FIXED_LATENCY_RECORD_A2DP : FIXED_LATENCY_RECORD_SCO) +
>                                                    pa_bytes_to_usec(u->read_block_size, &u->sample_spec));
>  }
>  
>  /* Run from I/O thread */
>  static void setup_stream(struct userdata *u) {
> +    const pa_a2dp_codec_t *codec;
>      struct pollfd *pollfd;
>      int one;
>  
> @@ -895,6 +737,12 @@ static void setup_stream(struct userdata *u) {
>  
>      pa_log_info("Transport %s resuming", u->transport->path);
>  
> +    codec = pa_bluetooth_profile_to_a2dp_codec(u->profile);
> +    if (codec) {
> +        pa_assert(u->sink || u->source);
> +        codec->setup(u->sink ? &u->encoder_info : &u->decoder_info);
> +    }
> +
>      transport_config_mtu(u);
>  
>      pa_make_fd_nonblock(u->stream_fd);
> @@ -904,12 +752,10 @@ static void setup_stream(struct userdata *u) {
>      if (setsockopt(u->stream_fd, SOL_SOCKET, SO_TIMESTAMP, &one, sizeof(one)) < 0)
>          pa_log_warn("Failed to enable SO_TIMESTAMP: %s", pa_cstrerror(errno));
>  
> -    pa_log_debug("Stream properly set up, we're ready to roll!");
> -
> -    if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK) {
> -        a2dp_set_bitpool(u, u->sbc_info.max_bitpool);
> +    if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK)
>          update_buffer_size(u);
> -    }
> +
> +    pa_log_debug("Stream properly set up, we're ready to roll!");
>  
>      u->rtpoll_item = pa_rtpoll_item_new(u->rtpoll, PA_RTPOLL_NEVER, 1);
>      pollfd = pa_rtpoll_item_get_pollfd(u->rtpoll_item, NULL);
> @@ -1078,7 +924,7 @@ static int add_source(struct userdata *u) {
>  
>      if (!u->transport_acquired)
>          switch (u->profile) {
> -            case PA_BLUETOOTH_PROFILE_A2DP_SOURCE:
> +            case PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE:
>              case PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY:
>                  data.suspend_cause = PA_SUSPEND_USER;
>                  break;
> @@ -1090,8 +936,9 @@ static int add_source(struct userdata *u) {
>                  else
>                      pa_assert_not_reached();
>                  break;
> -            case PA_BLUETOOTH_PROFILE_A2DP_SINK:
> +            case PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK:
>              case PA_BLUETOOTH_PROFILE_OFF:
> +            default:

If a switch statement has cases for all members of an enumeration, as
is the case here, then it's better to leave the default case out,
because the default case removes compiler warnings about unhandled
cases. If we add a new profile, it's good to get a warning if it's not
handled here.

>                  pa_assert_not_reached();
>                  break;
>          }
> @@ -1263,10 +1110,11 @@ static int add_sink(struct userdata *u) {
>                  else
>                      pa_assert_not_reached();
>                  break;
> -            case PA_BLUETOOTH_PROFILE_A2DP_SINK:
> +            case PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK:
>                  /* Profile switch should have failed */
> -            case PA_BLUETOOTH_PROFILE_A2DP_SOURCE:
> +            case PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE:
>              case PA_BLUETOOTH_PROFILE_OFF:
> +            default:

Same as above.

>                  pa_assert_not_reached();
>                  break;
>          }
> @@ -1296,111 +1144,11 @@ static void transport_config(struct userdata *u) {
>          u->sample_spec.channels = 1;
>          u->sample_spec.rate = 8000;
>      } else {
> -        sbc_info_t *sbc_info = &u->sbc_info;
> -        a2dp_sbc_t *config;
> -
> +        const pa_a2dp_codec_t *codec = pa_bluetooth_profile_to_a2dp_codec(u->profile);
> +        bool is_sink = (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK);
> +        pa_assert(codec);
>          pa_assert(u->transport);
> -
> -        u->sample_spec.format = PA_SAMPLE_S16LE;
> -        config = (a2dp_sbc_t *) u->transport->config;
> -
> -        if (sbc_info->sbc_initialized)
> -            sbc_reinit(&sbc_info->sbc, 0);
> -        else
> -            sbc_init(&sbc_info->sbc, 0);
> -        sbc_info->sbc_initialized = true;
> -
> -        switch (config->frequency) {
> -            case SBC_SAMPLING_FREQ_16000:
> -                sbc_info->sbc.frequency = SBC_FREQ_16000;
> -                u->sample_spec.rate = 16000U;
> -                break;
> -            case SBC_SAMPLING_FREQ_32000:
> -                sbc_info->sbc.frequency = SBC_FREQ_32000;
> -                u->sample_spec.rate = 32000U;
> -                break;
> -            case SBC_SAMPLING_FREQ_44100:
> -                sbc_info->sbc.frequency = SBC_FREQ_44100;
> -                u->sample_spec.rate = 44100U;
> -                break;
> -            case SBC_SAMPLING_FREQ_48000:
> -                sbc_info->sbc.frequency = SBC_FREQ_48000;
> -                u->sample_spec.rate = 48000U;
> -                break;
> -            default:
> -                pa_assert_not_reached();
> -        }
> -
> -        switch (config->channel_mode) {
> -            case SBC_CHANNEL_MODE_MONO:
> -                sbc_info->sbc.mode = SBC_MODE_MONO;
> -                u->sample_spec.channels = 1;
> -                break;
> -            case SBC_CHANNEL_MODE_DUAL_CHANNEL:
> -                sbc_info->sbc.mode = SBC_MODE_DUAL_CHANNEL;
> -                u->sample_spec.channels = 2;
> -                break;
> -            case SBC_CHANNEL_MODE_STEREO:
> -                sbc_info->sbc.mode = SBC_MODE_STEREO;
> -                u->sample_spec.channels = 2;
> -                break;
> -            case SBC_CHANNEL_MODE_JOINT_STEREO:
> -                sbc_info->sbc.mode = SBC_MODE_JOINT_STEREO;
> -                u->sample_spec.channels = 2;
> -                break;
> -            default:
> -                pa_assert_not_reached();
> -        }
> -
> -        switch (config->allocation_method) {
> -            case SBC_ALLOCATION_SNR:
> -                sbc_info->sbc.allocation = SBC_AM_SNR;
> -                break;
> -            case SBC_ALLOCATION_LOUDNESS:
> -                sbc_info->sbc.allocation = SBC_AM_LOUDNESS;
> -                break;
> -            default:
> -                pa_assert_not_reached();
> -        }
> -
> -        switch (config->subbands) {
> -            case SBC_SUBBANDS_4:
> -                sbc_info->sbc.subbands = SBC_SB_4;
> -                break;
> -            case SBC_SUBBANDS_8:
> -                sbc_info->sbc.subbands = SBC_SB_8;
> -                break;
> -            default:
> -                pa_assert_not_reached();
> -        }
> -
> -        switch (config->block_length) {
> -            case SBC_BLOCK_LENGTH_4:
> -                sbc_info->sbc.blocks = SBC_BLK_4;
> -                break;
> -            case SBC_BLOCK_LENGTH_8:
> -                sbc_info->sbc.blocks = SBC_BLK_8;
> -                break;
> -            case SBC_BLOCK_LENGTH_12:
> -                sbc_info->sbc.blocks = SBC_BLK_12;
> -                break;
> -            case SBC_BLOCK_LENGTH_16:
> -                sbc_info->sbc.blocks = SBC_BLK_16;
> -                break;
> -            default:
> -                pa_assert_not_reached();
> -        }
> -
> -        sbc_info->min_bitpool = config->min_bitpool;
> -        sbc_info->max_bitpool = config->max_bitpool;
> -
> -        /* Set minimum bitpool for source to get the maximum possible block_size */
> -        sbc_info->sbc.bitpool = u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK ? sbc_info->max_bitpool : sbc_info->min_bitpool;
> -        sbc_info->codesize = sbc_get_codesize(&sbc_info->sbc);
> -        sbc_info->frame_length = sbc_get_frame_length(&sbc_info->sbc);
> -
> -        pa_log_info("SBC parameters: allocation=%u, subbands=%u, blocks=%u, bitpool=%u",
> -                    sbc_info->sbc.allocation, sbc_info->sbc.subbands ? 8 : 4, sbc_info->sbc.blocks, sbc_info->sbc.bitpool);
> +        codec->init(is_sink ? &u->encoder_info : &u->decoder_info, &u->sample_spec, is_sink, u->transport->config, u->transport->config_size);
>      }
>  }
>  
> @@ -1421,7 +1169,7 @@ static int setup_transport(struct userdata *u) {
>  
>      u->transport = t;
>  
> -    if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SOURCE || u->profile == PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY)
> +    if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE || u->profile == PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY)
>          transport_acquire(u, true); /* In case of error, the sink/sources will be created suspended */
>      else {
>          int transport_error;
> @@ -1439,8 +1187,8 @@ static int setup_transport(struct userdata *u) {
>  /* Run from main thread */
>  static pa_direction_t get_profile_direction(pa_bluetooth_profile_t p) {
>      static const pa_direction_t profile_direction[] = {
> -        [PA_BLUETOOTH_PROFILE_A2DP_SINK] = PA_DIRECTION_OUTPUT,
> -        [PA_BLUETOOTH_PROFILE_A2DP_SOURCE] = PA_DIRECTION_INPUT,
> +        [PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK] = PA_DIRECTION_OUTPUT,
> +        [PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE] = PA_DIRECTION_INPUT,
>          [PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT] = PA_DIRECTION_INPUT | PA_DIRECTION_OUTPUT,
>          [PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY] = PA_DIRECTION_INPUT | PA_DIRECTION_OUTPUT,
>          [PA_BLUETOOTH_PROFILE_OFF] = 0
> @@ -1477,11 +1225,11 @@ static int write_block(struct userdata *u) {
>      if (u->write_index <= 0)
>          u->started_at = pa_rtclock_now();
>  
> -    if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK) {
> -        if ((n_written = a2dp_process_render(u)) < 0)
> +    if (u->profile == PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT || u->profile == PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY) {
> +        if ((n_written = sco_process_render(u)) < 0)
>              return -1;
>      } else {
> -        if ((n_written = sco_process_render(u)) < 0)
> +        if ((n_written = a2dp_process_render(u)) < 0)
>              return -1;
>      }
>  
> @@ -1551,7 +1299,7 @@ static void thread_func(void *userdata) {
>                  if (pollfd->revents & POLLIN) {
>                      int n_read;
>  
> -                    if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SOURCE)
> +                    if (pa_bluetooth_profile_to_a2dp_codec(u->profile))
>                          n_read = a2dp_process_push(u);
>                      else
>                          n_read = sco_process_push(u);
> @@ -1651,8 +1399,13 @@ static void thread_func(void *userdata) {
>                                  skip_bytes -= bytes_to_render;
>                              }
>  
> -                            if (u->write_index > 0 && u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK)
> -                                a2dp_reduce_bitpool(u);
> +                            if (u->write_index > 0 && u->sink) {
> +                                const pa_a2dp_codec_t *codec = pa_bluetooth_profile_to_a2dp_codec(u->profile);
> +                                if (codec && codec->fix_latency(&u->encoder_info)) {
> +                                    transport_config_mtu(u);
> +                                    update_buffer_size(u);
> +                                }
> +                            }
>                          }
>  
>                          blocks_to_write = 1;
> @@ -1767,7 +1520,7 @@ static int start_thread(struct userdata *u) {
>          /* If we are in the headset role or the device is an a2dp source,
>           * the source should not become default unless there is no other
>           * sound device available. */
> -        if (u->profile == PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY || u->profile == PA_BLUETOOTH_PROFILE_A2DP_SOURCE)
> +        if (u->profile == PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY || u->profile == PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE)
>              u->source->priority = 1500;
>  
>          pa_source_put(u->source);
> @@ -1980,8 +1733,8 @@ static pa_card_profile *create_card_profile(struct userdata *u, pa_bluetooth_pro
>      name = pa_bluetooth_profile_to_string(profile);
>  
>      switch (profile) {
> -    case PA_BLUETOOTH_PROFILE_A2DP_SINK:
> -        cp = pa_card_profile_new(name, _("High Fidelity Playback (A2DP Sink)"), sizeof(pa_bluetooth_profile_t));
> +    case PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK:
> +        cp = pa_card_profile_new(name, _("High Fidelity SBC Playback (A2DP Sink)"), sizeof(pa_bluetooth_profile_t));
>          cp->priority = 40;
>          cp->n_sinks = 1;
>          cp->n_sources = 0;
> @@ -1992,8 +1745,8 @@ static pa_card_profile *create_card_profile(struct userdata *u, pa_bluetooth_pro
>          p = PA_CARD_PROFILE_DATA(cp);
>          break;
>  
> -    case PA_BLUETOOTH_PROFILE_A2DP_SOURCE:
> -        cp = pa_card_profile_new(name, _("High Fidelity Capture (A2DP Source)"), sizeof(pa_bluetooth_profile_t));
> +    case PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE:
> +        cp = pa_card_profile_new(name, _("High Fidelity SBC Capture (A2DP Source)"), sizeof(pa_bluetooth_profile_t));
>          cp->priority = 20;
>          cp->n_sinks = 0;
>          cp->n_sources = 1;
> @@ -2088,9 +1841,9 @@ off:
>  
>  static int uuid_to_profile(const char *uuid, pa_bluetooth_profile_t *_r) {
>      if (pa_streq(uuid, PA_BLUETOOTH_UUID_A2DP_SINK))
> -        *_r = PA_BLUETOOTH_PROFILE_A2DP_SINK;
> +        *_r = PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK;
>      else if (pa_streq(uuid, PA_BLUETOOTH_UUID_A2DP_SOURCE))
> -        *_r = PA_BLUETOOTH_PROFILE_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))
>          *_r = PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT;
>      else if (pa_streq(uuid, PA_BLUETOOTH_UUID_HSP_AG) || pa_streq(uuid, PA_BLUETOOTH_UUID_HFP_AG))
> @@ -2472,6 +2225,7 @@ fail:
>  
>  void pa__done(pa_module *m) {
>      struct userdata *u;
> +    const pa_a2dp_codec_t *codec;
>  
>      pa_assert(m);
>  
> @@ -2492,11 +2246,14 @@ void pa__done(pa_module *m) {
>      if (u->transport_microphone_gain_changed_slot)
>          pa_hook_slot_free(u->transport_microphone_gain_changed_slot);
>  
> -    if (u->sbc_info.buffer)
> -        pa_xfree(u->sbc_info.buffer);
> +    if (u->buffer)
> +        pa_xfree(u->buffer);
>  
> -    if (u->sbc_info.sbc_initialized)
> -        sbc_finish(&u->sbc_info.sbc);
> +    codec = pa_bluetooth_profile_to_a2dp_codec(u->profile);
> +    if (codec) {
> +        codec->finish(&u->encoder_info);
> +        codec->finish(&u->decoder_info);
> +    }
>  
>      if (u->msg)
>          pa_xfree(u->msg);
> diff --git a/src/modules/bluetooth/pa-a2dp-codec-sbc.c b/src/modules/bluetooth/pa-a2dp-codec-sbc.c
> new file mode 100644
> index 000000000..2a61114a6
> --- /dev/null
> +++ b/src/modules/bluetooth/pa-a2dp-codec-sbc.c
> @@ -0,0 +1,579 @@
> +/***
> +  This file is part of PulseAudio.
> +
> +  Copyright 2018 Pali Rohár <pali.rohar@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
> +  published by the Free Software Foundation; either version 2.1 of the
> +  License, or (at your option) any later version.
> +
> +  PulseAudio is distributed in the hope that it will be useful, but
> +  WITHOUT ANY WARRANTY; without even the implied warranty of
> +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +  General Public License for more details.
> +
> +  You should have received a copy of the GNU Lesser General Public
> +  License along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
> +***/
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <pulsecore/core-util.h>
> +#include <pulsecore/log.h>
> +#include <pulsecore/macro.h>
> +#include <pulsecore/once.h>
> +#include <pulse/sample.h>
> +#include <pulse/xmalloc.h>
> +
> +#include <arpa/inet.h>
> +
> +#include <sbc/sbc.h>
> +
> +#include "a2dp-codecs.h"
> +#include "pa-a2dp-codec.h"
> +#include "rtp.h"
> +
> +#define SBC_BITPOOL_DEC_LIMIT 32
> +#define SBC_BITPOOL_DEC_STEP 5
> +
> +typedef struct sbc_info {
> +    sbc_t sbc;                           /* Codec data */
> +    bool sbc_initialized;                /* Keep track if the encoder is initialized */
> +    size_t codesize, frame_length;       /* SBC Codesize, frame_length. We simply cache those values here */
> +    uint16_t seq_num;                    /* Cumulative packet sequence */
> +    uint8_t min_bitpool;
> +    uint8_t max_bitpool;
> +} sbc_info_t;

According to our coding style, structs that aren't exported in any
headers should not be typedef'd. And if structs are typedef'd, they
shouldn't have a "_t" suffix. (Yes, the typedef existed in the old code
too, but I thought I'd mention this anyway.)

> +
> +static size_t pa_sbc_fill_capabilities(uint8_t *capabilities_buffer, size_t capabilities_size) {

Static functions shouldn't have the "pa_" prefix. The "sbc_" prefix
feels a bit redundant too. This comment applies to all functions in
this file.

> +    a2dp_sbc_t *capabilities = (a2dp_sbc_t *) capabilities_buffer;
> +
> +    if (capabilities_size < sizeof(*capabilities)) {
> +        pa_log_error("Invalid size of capabilities buffer");
> +        return 0;
> +    }
> +
> +    pa_zero(*capabilities);
> +
> +    capabilities->channel_mode = SBC_CHANNEL_MODE_MONO | SBC_CHANNEL_MODE_DUAL_CHANNEL | SBC_CHANNEL_MODE_STEREO |
> +                                 SBC_CHANNEL_MODE_JOINT_STEREO;
> +    capabilities->frequency = SBC_SAMPLING_FREQ_16000 | SBC_SAMPLING_FREQ_32000 | SBC_SAMPLING_FREQ_44100 |
> +                              SBC_SAMPLING_FREQ_48000;
> +    capabilities->allocation_method = SBC_ALLOCATION_SNR | SBC_ALLOCATION_LOUDNESS;
> +    capabilities->subbands = SBC_SUBBANDS_4 | SBC_SUBBANDS_8;
> +    capabilities->block_length = SBC_BLOCK_LENGTH_4 | SBC_BLOCK_LENGTH_8 | SBC_BLOCK_LENGTH_12 | SBC_BLOCK_LENGTH_16;
> +    capabilities->min_bitpool = SBC_MIN_BITPOOL;
> +    capabilities->max_bitpool = SBC_MAX_BITPOOL;
> +
> +    return sizeof(*capabilities);
> +}
> +
> +static bool pa_sbc_validate_configuration(const uint8_t *config_buffer, size_t config_size) {
> +    a2dp_sbc_t *config = (a2dp_sbc_t *) config_buffer;
> +
> +    if (config_size != sizeof(*config)) {
> +        pa_log_error("Invalid size of config buffer");
> +        return false;
> +    }
> +
> +    if (config->frequency != SBC_SAMPLING_FREQ_16000 && config->frequency != SBC_SAMPLING_FREQ_32000 &&
> +        config->frequency != SBC_SAMPLING_FREQ_44100 && config->frequency != SBC_SAMPLING_FREQ_48000) {
> +        pa_log_error("Invalid sampling frequency in configuration");
> +        return false;
> +    }
> +
> +    if (config->channel_mode != SBC_CHANNEL_MODE_MONO && config->channel_mode != SBC_CHANNEL_MODE_DUAL_CHANNEL &&
> +        config->channel_mode != SBC_CHANNEL_MODE_STEREO && config->channel_mode != SBC_CHANNEL_MODE_JOINT_STEREO) {
> +        pa_log_error("Invalid channel mode in configuration");
> +        return false;
> +    }
> +
> +    if (config->allocation_method != SBC_ALLOCATION_SNR && config->allocation_method != SBC_ALLOCATION_LOUDNESS) {
> +        pa_log_error("Invalid allocation method in configuration");
> +        return false;
> +    }
> +
> +    if (config->subbands != SBC_SUBBANDS_4 && config->subbands != SBC_SUBBANDS_8) {
> +        pa_log_error("Invalid SBC subbands in configuration");
> +        return false;
> +    }
> +
> +    if (config->block_length != SBC_BLOCK_LENGTH_4 && config->block_length != SBC_BLOCK_LENGTH_8 &&
> +        config->block_length != SBC_BLOCK_LENGTH_12 && config->block_length != SBC_BLOCK_LENGTH_16) {
> +        pa_log_error("Invalid block length in configuration");
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
> +static uint8_t pa_sbc_default_bitpool(uint8_t freq, uint8_t mode) {
> +    /* These bitpool values were chosen based on the A2DP spec recommendation */
> +    switch (freq) {
> +        case SBC_SAMPLING_FREQ_16000:
> +        case SBC_SAMPLING_FREQ_32000:
> +            return 53;
> +
> +        case SBC_SAMPLING_FREQ_44100:
> +
> +            switch (mode) {
> +                case SBC_CHANNEL_MODE_MONO:
> +                case SBC_CHANNEL_MODE_DUAL_CHANNEL:
> +                    return 31;
> +
> +                case SBC_CHANNEL_MODE_STEREO:
> +                case SBC_CHANNEL_MODE_JOINT_STEREO:
> +                    return 53;
> +            }
> +
> +            pa_log_warn("Invalid channel mode %u", mode);
> +            return 53;
> +
> +        case SBC_SAMPLING_FREQ_48000:
> +
> +            switch (mode) {
> +                case SBC_CHANNEL_MODE_MONO:
> +                case SBC_CHANNEL_MODE_DUAL_CHANNEL:
> +                    return 29;
> +
> +                case SBC_CHANNEL_MODE_STEREO:
> +                case SBC_CHANNEL_MODE_JOINT_STEREO:
> +                    return 51;
> +            }
> +
> +            pa_log_warn("Invalid channel mode %u", mode);
> +            return 51;
> +    }
> +
> +    pa_log_warn("Invalid sampling freq %u", freq);
> +    return 53;
> +}
> +
> +static size_t pa_sbc_select_configuration(const pa_sample_spec *sample_spec, const uint8_t *capabilities_buffer, size_t capabilities_size, uint8_t *config_buffer, size_t config_size) {
> +    a2dp_sbc_t *config = (a2dp_sbc_t *) config_buffer;
> +    a2dp_sbc_t *capabilities = (a2dp_sbc_t *) capabilities_buffer;

This looks likely to cause alignment errors, since the data in the
uint8_t arrays doesn't have any kind of alignment guarantees. However,
a2dp_sbc_t happens to only contain uint8_t values, so the values can't
be badly aligned. It would be good to have a comment that reassures the
reader that there won't be any alignment errors. Suggestion: "We cast a
byte array to struct here, which can often cause alignment errors, but
in this case it's safe, because a2dp_sbc_t contains only single-byte
values."

There are several more functions that cast a byte array to a2dp_sbc_t,
the same applies to them.

> +    int i;
> +
> +    static const struct {
> +        uint32_t rate;
> +        uint8_t cap;
> +    } freq_table[] = {
> +        { 16000U, SBC_SAMPLING_FREQ_16000 },
> +        { 32000U, SBC_SAMPLING_FREQ_32000 },
> +        { 44100U, SBC_SAMPLING_FREQ_44100 },
> +        { 48000U, SBC_SAMPLING_FREQ_48000 }
> +    };
> +
> +    if (capabilities_size != sizeof(*capabilities)) {
> +        pa_log_error("Invalid size of capabilities buffer");
> +        return 0;
> +    }
> +
> +    if (config_size < sizeof(*config)) {
> +        pa_log_error("Invalid size of config buffer");
> +        return 0;
> +    }
> +
> +    pa_zero(*config);
> +
> +    /* Find the lowest freq that is at least as high as the requested sampling rate */
> +    for (i = 0; (unsigned) i < PA_ELEMENTSOF(freq_table); i++)
> +        if (freq_table[i].rate >= sample_spec->rate && (capabilities->frequency & freq_table[i].cap)) {
> +            config->frequency = freq_table[i].cap;
> +            break;
> +        }
> +
> +    if ((unsigned) i == PA_ELEMENTSOF(freq_table)) {
> +        for (--i; i >= 0; i--) {
> +            if (capabilities->frequency & freq_table[i].cap) {
> +                config->frequency = freq_table[i].cap;
> +                break;
> +            }
> +        }
> +
> +        if (i < 0) {
> +            pa_log_error("Not suitable sample rate");
> +            return 0;
> +        }
> +    }
> +
> +    pa_assert((unsigned) i < PA_ELEMENTSOF(freq_table));
> +
> +    if (sample_spec->channels <= 1) {
> +        if (capabilities->channel_mode & SBC_CHANNEL_MODE_MONO)
> +            config->channel_mode = SBC_CHANNEL_MODE_MONO;
> +        else if (capabilities->channel_mode & SBC_CHANNEL_MODE_JOINT_STEREO)
> +            config->channel_mode = SBC_CHANNEL_MODE_JOINT_STEREO;
> +        else if (capabilities->channel_mode & SBC_CHANNEL_MODE_STEREO)
> +            config->channel_mode = SBC_CHANNEL_MODE_STEREO;
> +        else if (capabilities->channel_mode & SBC_CHANNEL_MODE_DUAL_CHANNEL)
> +            config->channel_mode = SBC_CHANNEL_MODE_DUAL_CHANNEL;
> +        else {
> +            pa_log_error("No supported channel modes");
> +            return 0;
> +        }
> +    }
> +
> +    if (sample_spec->channels >= 2) {
> +        if (capabilities->channel_mode & SBC_CHANNEL_MODE_JOINT_STEREO)
> +            config->channel_mode = SBC_CHANNEL_MODE_JOINT_STEREO;
> +        else if (capabilities->channel_mode & SBC_CHANNEL_MODE_STEREO)
> +            config->channel_mode = SBC_CHANNEL_MODE_STEREO;
> +        else if (capabilities->channel_mode & SBC_CHANNEL_MODE_DUAL_CHANNEL)
> +            config->channel_mode = SBC_CHANNEL_MODE_DUAL_CHANNEL;
> +        else if (capabilities->channel_mode & SBC_CHANNEL_MODE_MONO)
> +            config->channel_mode = SBC_CHANNEL_MODE_MONO;
> +        else {
> +            pa_log_error("No supported channel modes");
> +            return 0;
> +        }
> +    }
> +
> +    if (capabilities->block_length & SBC_BLOCK_LENGTH_16)
> +        config->block_length = SBC_BLOCK_LENGTH_16;
> +    else if (capabilities->block_length & SBC_BLOCK_LENGTH_12)
> +        config->block_length = SBC_BLOCK_LENGTH_12;
> +    else if (capabilities->block_length & SBC_BLOCK_LENGTH_8)
> +        config->block_length = SBC_BLOCK_LENGTH_8;
> +    else if (capabilities->block_length & SBC_BLOCK_LENGTH_4)
> +        config->block_length = SBC_BLOCK_LENGTH_4;
> +    else {
> +        pa_log_error("No supported block lengths");
> +        return 0;
> +    }
> +
> +    if (capabilities->subbands & SBC_SUBBANDS_8)
> +        config->subbands = SBC_SUBBANDS_8;
> +    else if (capabilities->subbands & SBC_SUBBANDS_4)
> +        config->subbands = SBC_SUBBANDS_4;
> +    else {
> +        pa_log_error("No supported subbands");
> +        return 0;
> +    }
> +
> +    if (capabilities->allocation_method & SBC_ALLOCATION_LOUDNESS)
> +        config->allocation_method = SBC_ALLOCATION_LOUDNESS;
> +    else if (capabilities->allocation_method & SBC_ALLOCATION_SNR)
> +        config->allocation_method = SBC_ALLOCATION_SNR;

Error handling for unsupported allocation method is missing. Not your
fault, and this doesn't look serious (because the final configuration
is validated anyway), but you can fix this if you want.

> +
> +    config->min_bitpool = (uint8_t) PA_MAX(SBC_MIN_BITPOOL, capabilities->min_bitpool);
> +    config->max_bitpool = (uint8_t) PA_MIN(pa_sbc_default_bitpool(config->frequency, config->channel_mode), capabilities->max_bitpool);
> +
> +    if (config->min_bitpool > config->max_bitpool)
> +        return 0;
> +
> +    return sizeof(*config);
> +}
> +
> +static void pa_sbc_init(void **info_ptr, pa_sample_spec *sample_spec, bool is_a2dp_sink, const uint8_t *config_buffer, size_t config_size) {
> +    sbc_info_t *sbc_info;
> +    a2dp_sbc_t *config = (a2dp_sbc_t *) config_buffer;
> +
> +    pa_assert(config_size == sizeof(*config));
> +
> +    if (!*info_ptr)
> +        *info_ptr = pa_xmalloc0(sizeof(*sbc_info));
> +
> +    sbc_info = (sbc_info_t *) *info_ptr;
> +
> +    sample_spec->format = PA_SAMPLE_S16LE;
> +
> +    if (sbc_info->sbc_initialized)
> +        sbc_reinit(&sbc_info->sbc, 0);
> +    else
> +        sbc_init(&sbc_info->sbc, 0);
> +    sbc_info->sbc_initialized = true;
> +
> +    switch (config->frequency) {
> +        case SBC_SAMPLING_FREQ_16000:
> +            sbc_info->sbc.frequency = SBC_FREQ_16000;
> +            sample_spec->rate = 16000U;
> +            break;
> +        case SBC_SAMPLING_FREQ_32000:
> +            sbc_info->sbc.frequency = SBC_FREQ_32000;
> +            sample_spec->rate = 32000U;
> +            break;
> +        case SBC_SAMPLING_FREQ_44100:
> +            sbc_info->sbc.frequency = SBC_FREQ_44100;
> +            sample_spec->rate = 44100U;
> +            break;
> +        case SBC_SAMPLING_FREQ_48000:
> +            sbc_info->sbc.frequency = SBC_FREQ_48000;
> +            sample_spec->rate = 48000U;
> +            break;
> +        default:
> +            pa_assert_not_reached();
> +    }
> +
> +    switch (config->channel_mode) {
> +        case SBC_CHANNEL_MODE_MONO:
> +            sbc_info->sbc.mode = SBC_MODE_MONO;
> +            sample_spec->channels = 1;
> +            break;
> +        case SBC_CHANNEL_MODE_DUAL_CHANNEL:
> +            sbc_info->sbc.mode = SBC_MODE_DUAL_CHANNEL;
> +            sample_spec->channels = 2;
> +            break;
> +        case SBC_CHANNEL_MODE_STEREO:
> +            sbc_info->sbc.mode = SBC_MODE_STEREO;
> +            sample_spec->channels = 2;
> +            break;
> +        case SBC_CHANNEL_MODE_JOINT_STEREO:
> +            sbc_info->sbc.mode = SBC_MODE_JOINT_STEREO;
> +            sample_spec->channels = 2;
> +            break;
> +        default:
> +            pa_assert_not_reached();
> +    }
> +
> +    switch (config->allocation_method) {
> +        case SBC_ALLOCATION_SNR:
> +            sbc_info->sbc.allocation = SBC_AM_SNR;
> +            break;
> +        case SBC_ALLOCATION_LOUDNESS:
> +            sbc_info->sbc.allocation = SBC_AM_LOUDNESS;
> +            break;
> +        default:
> +            pa_assert_not_reached();
> +    }
> +
> +    switch (config->subbands) {
> +        case SBC_SUBBANDS_4:
> +            sbc_info->sbc.subbands = SBC_SB_4;
> +            break;
> +        case SBC_SUBBANDS_8:
> +            sbc_info->sbc.subbands = SBC_SB_8;
> +            break;
> +        default:
> +            pa_assert_not_reached();
> +    }
> +
> +    switch (config->block_length) {
> +        case SBC_BLOCK_LENGTH_4:
> +            sbc_info->sbc.blocks = SBC_BLK_4;
> +            break;
> +        case SBC_BLOCK_LENGTH_8:
> +            sbc_info->sbc.blocks = SBC_BLK_8;
> +            break;
> +        case SBC_BLOCK_LENGTH_12:
> +            sbc_info->sbc.blocks = SBC_BLK_12;
> +            break;
> +        case SBC_BLOCK_LENGTH_16:
> +            sbc_info->sbc.blocks = SBC_BLK_16;
> +            break;
> +        default:
> +            pa_assert_not_reached();
> +    }
> +
> +    sbc_info->min_bitpool = config->min_bitpool;
> +    sbc_info->max_bitpool = config->max_bitpool;
> +
> +    /* Set minimum bitpool for source to get the maximum possible block_size */
> +    sbc_info->sbc.bitpool = is_a2dp_sink ? sbc_info->max_bitpool : sbc_info->min_bitpool;

Do you understand the logic here? If you do, could you make the comment
clearer? Why does minimum bitpool for source result in maximum block
size, and why does it matter? I thought that small bitpools result in
small blocks. Also my understanding is that if we're the receiving side
of an a2dp stream, then the bitpool, codesize and frame length can
change without warning on any packet, so it doesn't really matter what
values we set here for sources. Is my understanding wrong?

This led me to wonder about another thing. sbc.h has this
documentation:

/* Decodes ONE input block into ONE output block */
ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len,
			void *output, size_t output_len, size_t *written);

/* Encodes ONE input block into ONE output block */
ssize_t sbc_encode(sbc_t *sbc, const void *input, size_t input_len,
			void *output, size_t output_len, ssize_t *written);

/* Returns the output block size in bytes */
size_t sbc_get_frame_length(sbc_t *sbc);

/* Returns the input block size in bytes */
size_t sbc_get_codesize(sbc_t *sbc);

According to those comments "input block" means compressed audio when
decoding and uncompressed audio when encoding, and vice versa for
"output block". That's fine, no problem. But then the comments say that
"frame length" is the same thing as the output block size and
"codesize" is the same thing as the input block size. That seems weird
- I would have expected that "frame length" means decompressed block
size and "codesize" means compressed size (or perhaps vice versa).
Which is wrong, my expectations or the documentation? In any case, it
would be good to document in more detail the meaning of frame length
and codesize in the sbc_info struct.

... Update after reading the encoding and decoding code: it seems that
"frame length" always means the compressed block size and "codesize"
means the uncompressed block size, so the sbc.h documentation is wrong
(assuming that we don't have bugs related to this in PulseAudio, but
I'm pretty sure that's not the case, because otherwise there would be
crashing all the time). Maybe it would be good to rename the variables
to "compressed_block_size" and "uncompressed_block_size"? That would
make their meaning more obvious at least to me.

> +    sbc_info->codesize = sbc_get_codesize(&sbc_info->sbc);
> +    sbc_info->frame_length = sbc_get_frame_length(&sbc_info->sbc);
> +
> +    pa_log_info("SBC parameters: allocation=%u, subbands=%u, blocks=%u, bitpool=%u",
> +                sbc_info->sbc.allocation, sbc_info->sbc.subbands ? 8 : 4, sbc_info->sbc.blocks, sbc_info->sbc.bitpool);
> +}
> +
> +static void pa_sbc_finish(void **info_ptr) {
> +    sbc_info_t *sbc_info = (sbc_info_t *) *info_ptr;
> +
> +    if (!sbc_info)
> +        return;
> +
> +    if (sbc_info->sbc_initialized)
> +        sbc_finish(&sbc_info->sbc);
> +
> +    pa_xfree(sbc_info);
> +    *info_ptr = NULL;
> +}
> +
> +static void pa_sbc_set_bitpool(sbc_info_t *sbc_info, uint8_t bitpool) {
> +    if (bitpool > sbc_info->max_bitpool)
> +        bitpool = sbc_info->max_bitpool;
> +    else if (bitpool < sbc_info->min_bitpool)
> +        bitpool = sbc_info->min_bitpool;
> +
> +    sbc_info->sbc.bitpool = bitpool;
> +
> +    sbc_info->codesize = sbc_get_codesize(&sbc_info->sbc);
> +    sbc_info->frame_length = sbc_get_frame_length(&sbc_info->sbc);
> +
> +    pa_log_debug("Bitpool has changed to %u", sbc_info->sbc.bitpool);
> +}
> +
> +static void pa_sbc_setup(void **info_ptr) {
> +    sbc_info_t *sbc_info = (sbc_info_t *) *info_ptr;
> +
> +    pa_sbc_set_bitpool(sbc_info, sbc_info->max_bitpool);
> +}
> +
> +static void pa_sbc_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) {
> +    sbc_info_t *sbc_info = (sbc_info_t *) *info_ptr;
> +
> +    *read_block_size =
> +        (read_link_mtu - sizeof(struct rtp_header) - sizeof(struct rtp_payload))
> +        / sbc_info->frame_length * sbc_info->codesize;
> +
> +    *write_block_size =
> +        (write_link_mtu - sizeof(struct rtp_header) - sizeof(struct rtp_payload))
> +        / sbc_info->frame_length * sbc_info->codesize;
> +}
> +
> +static bool pa_sbc_fix_latency(void **info_ptr) {
> +    sbc_info_t *sbc_info = (sbc_info_t *) *info_ptr;
> +    uint8_t bitpool;
> +
> +    /* Check if bitpool is already at its limit */
> +    if (sbc_info->sbc.bitpool <= SBC_BITPOOL_DEC_LIMIT)
> +        return false;
> +
> +    bitpool = sbc_info->sbc.bitpool - SBC_BITPOOL_DEC_STEP;
> +
> +    if (bitpool < SBC_BITPOOL_DEC_LIMIT)
> +        bitpool = SBC_BITPOOL_DEC_LIMIT;
> +
> +    if (sbc_info->sbc.bitpool == bitpool)
> +        return false;
> +
> +    pa_sbc_set_bitpool(sbc_info, bitpool);
> +    return true;
> +}
> +
> +static size_t pa_sbc_encode(void **info_ptr, uint32_t timestamp, const uint8_t *input_buffer, size_t input_size, uint8_t *output_buffer, size_t output_size) {
> +    sbc_info_t *sbc_info = (sbc_info_t *) *info_ptr;
> +    struct rtp_header *header;
> +    struct rtp_payload *payload;
> +    uint8_t *d;
> +    const uint8_t *p;
> +    size_t to_write, to_encode;
> +    unsigned frame_count;
> +
> +    header = (struct rtp_header*) output_buffer;
> +    payload = (struct rtp_payload*) (output_buffer + sizeof(*header));
> +
> +    frame_count = 0;
> +
> +    p = input_buffer;
> +    to_encode = input_size;
> +
> +    d = output_buffer + sizeof(*header) + sizeof(*payload);
> +    to_write = output_size - sizeof(*header) - sizeof(*payload);
> +
> +    while (PA_LIKELY(to_encode > 0 && to_write > 0)) {
> +        ssize_t written;
> +        ssize_t encoded;
> +
> +        encoded = sbc_encode(&sbc_info->sbc,
> +                             p, to_encode,
> +                             d, to_write,
> +                             &written);
> +
> +        if (PA_UNLIKELY(encoded <= 0)) {
> +            pa_log_error("SBC encoding error (%li)", (long) encoded);
> +            return 0;
> +        }
> +
> +        pa_assert_fp((size_t) encoded <= to_encode);
> +        pa_assert_fp((size_t) encoded == sbc_info->codesize);
> +
> +        pa_assert_fp((size_t) written <= to_write);
> +        pa_assert_fp((size_t) written == sbc_info->frame_length);
> +
> +        p += encoded;
> +        to_encode -= encoded;
> +
> +        d += written;
> +        to_write -= written;
> +
> +        frame_count++;
> +    }
> +
> +    pa_assert(to_encode == 0);
> +
> +    PA_ONCE_BEGIN {
> +        pa_log_debug("Using SBC encoder implementation: %s", pa_strnull(sbc_get_implementation_info(&sbc_info->sbc)));
> +    } PA_ONCE_END;
> +
> +    /* write it to the fifo */
> +    memset(output_buffer, 0, sizeof(*header) + sizeof(*payload));
> +    header->v = 2;
> +    header->pt = 1;
> +    header->sequence_number = htons(sbc_info->seq_num++);
> +    header->timestamp = htonl(timestamp);
> +    header->ssrc = htonl(1);
> +    payload->frame_count = frame_count;
> +
> +    return d - output_buffer;
> +}
> +
> +static size_t pa_sbc_decode(void **info_ptr, const uint8_t *input_buffer, size_t input_size, uint8_t *output_buffer, size_t output_size, size_t *processed) {
> +    sbc_info_t *sbc_info = (sbc_info_t *) *info_ptr;
> +
> +    struct rtp_header *header;
> +    struct rtp_payload *payload;
> +    const uint8_t *p;
> +    uint8_t *d;
> +    size_t to_write, to_decode;
> +
> +    header = (struct rtp_header *) input_buffer;
> +    payload = (struct rtp_payload*) (input_buffer + sizeof(*header));
> +
> +    p = input_buffer + sizeof(*header) + sizeof(*payload);
> +    to_decode = input_size - sizeof(*header) - sizeof(*payload);
> +
> +    d = output_buffer;
> +    to_write = output_size;
> +
> +    while (PA_LIKELY(to_decode > 0)) {
> +        size_t written;
> +        ssize_t decoded;
> +
> +        decoded = sbc_decode(&sbc_info->sbc,
> +                             p, to_decode,
> +                             d, to_write,
> +                             &written);
> +
> +        if (PA_UNLIKELY(decoded <= 0)) {
> +            pa_log_error("SBC decoding error (%li)", (long) decoded);
> +            *processed = p - input_buffer;
> +            return 0;
> +        }
> +
> +        /* Reset frame length, it can be changed due to bitpool change */
> +        sbc_info->frame_length = sbc_get_frame_length(&sbc_info->sbc);
> +
> +        pa_assert_fp((size_t) decoded <= to_decode);
> +        pa_assert_fp((size_t) decoded == sbc_info->frame_length);
> +
> +        pa_assert_fp((size_t) written == sbc_info->codesize);
> +
> +        p += decoded;
> +        to_decode -= decoded;
> +
> +        d += written;
> +        to_write -= written;
> +    }
> +
> +    *processed = p - input_buffer;
> +    return d - output_buffer;
> +}
> +
> +const pa_a2dp_codec_t pa_a2dp_codec_sbc = {
> +    .codec_id = A2DP_CODEC_SBC,
> +    .fill_capabilities = pa_sbc_fill_capabilities,
> +    .validate_configuration = pa_sbc_validate_configuration,
> +    .select_configuration = pa_sbc_select_configuration,
> +    .init = pa_sbc_init,
> +    .finish = pa_sbc_finish,
> +    .setup = pa_sbc_setup,
> +    .fill_blocksize = pa_sbc_fill_blocksize,
> +    .fix_latency = pa_sbc_fix_latency,
> +    .encode = pa_sbc_encode,
> +    .decode = pa_sbc_decode,
> +};
> diff --git a/src/modules/bluetooth/pa-a2dp-codec.h b/src/modules/bluetooth/pa-a2dp-codec.h
> new file mode 100644
> index 000000000..68b1619c2
> --- /dev/null
> +++ b/src/modules/bluetooth/pa-a2dp-codec.h
> @@ -0,0 +1,40 @@
> +#ifndef foopaa2dpcodechfoo
> +#define foopaa2dpcodechfoo
> +
> +/***
> +  This file is part of PulseAudio.
> +
> +  Copyright 2018 Pali Rohár <pali.rohar@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
> +  published by the Free Software Foundation; either version 2.1 of the
> +  License, or (at your option) any later version.
> +
> +  PulseAudio is distributed in the hope that it will be useful, but
> +  WITHOUT ANY WARRANTY; without even the implied warranty of
> +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +  General Public License for more details.
> +
> +  You should have received a copy of the GNU Lesser General Public
> +  License along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
> +***/
> +
> +typedef struct pa_a2dp_codec {
> +    const char *codec_name;

This is currently unused, so it should be removed unless you come up
with some use for it.

> +    uint8_t codec_id;
> +    size_t (*fill_capabilities)(uint8_t *, size_t);

It would be good to have some documentation for the callbacks. Naming
the function parameters also makes it easier to understand the code.
Imagine being someone who's not that familiar with the code but has
decided to add support for a new codec and needs to write
implementations for the callbacks.

> +    bool (*validate_configuration)(const uint8_t *, size_t);

The bool return value is used for reporting failures, but according to
our coding style failures should be reported using a negative int.

> +    size_t (*select_configuration)(const pa_sample_spec *, const uint8_t *, size_t, uint8_t *, size_t);
> +    void (*init)(void **, pa_sample_spec *, bool, const uint8_t *, size_t);
> +    void (*finish)(void **);
> +    void (*setup)(void **);
> +    void (*fill_blocksize)(void **, size_t, size_t, size_t *, size_t *);
> +    bool (*fix_latency)(void **);

I feel this callback name is not very good. The callback reduces the
bitpool (in case of SBC), it doesn't fix the latency. Fixing the
latency is left to the caller.

I suggest changing the name to "reduce_bitrate" (sufficiently generic
to not be specific to SBC). I also suggest some restructuring, because
the logic behind calling transport_config_mtu() and
update_buffer_size() after fix_latency() wasn't obvious. Restructuring
ideas:

1) Allow the reduce_bitrate() callback to directly update the block
size. The callback should return true only if the block size was
changed.

This will require access to the MTU sizes from reduce_bitrate(), so
fill_blocksize() should save the MTU sizes in the codec private data
(or the MTU could be passed as arguments to reduce_bitrate(), but I
think it's cleaner to save the previously set MTU in the codec private
data). It might make sense to rename fill_blocksize() to set_mtu().

2) Add a handle_block_size_change() function and move there the sink
latency update code from transport_config_mtu() and the code from
update_buffer_size(). Call handle_block_size_change() from
transport_configu_mtu() and after the reduce_bitrate() call. Remove the
transport_mtu_config() and update_buffer_size() calls from
thread_function(). Remove the update_buffer_size() call from
setup_stream(), it's not needed because transport_mtu_config() will
update the kernel buffer size via handle_block_size_change().
update_buffer_size() can be removed altogheter.

As a result the code in thread_func() will look like this, which I find
much more self-explanatory:

    bool block_size_changed = codec->reduce_bitrate(&u->encoder_info, &u->write_block_size);

    if (block_size_changed)
        handle_block_size_change(u);

> +    size_t (*encode)(void **, uint32_t, const uint8_t *, size_t, uint8_t *, size_t);
> +    size_t (*decode)(void **, const uint8_t *, size_t, uint8_t *, size_t, size_t *);
> +} pa_a2dp_codec_t;

According to our coding style, struct names shouldn't contain the _t
suffix. The _t suffix is only used with basic types like ints.

> +
> +extern const pa_a2dp_codec_t pa_a2dp_codec_sbc;
> +
> +#endif
On Tue, 2018-09-04 at 11:44 +0300, Tanu Kaskinen wrote:
> On Sat, 2018-07-28 at 17:34 +0200, Pali Rohár wrote:
> > +static size_t pa_sbc_select_configuration(const pa_sample_spec *sample_spec, const uint8_t *capabilities_buffer, size_t capabilities_size, uint8_t *config_buffer, size_t config_size) {
> > +    a2dp_sbc_t *config = (a2dp_sbc_t *) config_buffer;
> > +    a2dp_sbc_t *capabilities = (a2dp_sbc_t *) capabilities_buffer;
> 
> This looks likely to cause alignment errors, since the data in the
> uint8_t arrays doesn't have any kind of alignment guarantees. However,
> a2dp_sbc_t happens to only contain uint8_t values, so the values can't
> be badly aligned. It would be good to have a comment that reassures the
> reader that there won't be any alignment errors. Suggestion: "We cast a
> byte array to struct here, which can often cause alignment errors, but
> in this case it's safe, because a2dp_sbc_t contains only single-byte
> values."

a2dp_aptx_t has variables that are bigger than just one byte, so I
thought that we're going to have alignment issues with it. However, I
found out now that packed structs don't have alignment restrictions,
because the compiler will always be extra careful when dealing with
them. Therefore a better comment would be: "We cast a byte array to
struct here, which can often cause alignment errors, but in this case
it's safe, because a2dp_sbc_t uses the packed attribute, which makes
the compiler extra careful."

Copying that comment everywhere where we do the casting may not be such
a great idea after all, though. So much repetition. Maybe just add to
the struct definition a comment that explains what the packed attribute
does.
(I sent my review both to you and to the list, but you replied only
privately - probably not on purpose. I'll send this mail only to the
list.)

On Thu, 2018-09-13 at 10:54 +0200, Pali Rohár wrote:
> On Tuesday 04 September 2018 11:44:10 Tanu Kaskinen wrote:
> > Using libopenaptx directly
> > doesn't prevent us from switching to GStreamer later if someone writes
> > the code.
> 
> Yes, that is truth.
> 
> > Another point that was raised that the codec choice shouldn't be bound
> > to the card profile. I tend to agree with that. There are situations
> > where module-bluetooth-policy or the user only wants to switch from HSP
> > to A2DP and not care about the codec. You asked how the codec should be
> > negotiated or switched by the user if it's not bound to the profile.
> > Regarding negotiation, we can add a hook that module-bluetooth-policy
> > can use to make the codec decision (if module-bluetooth-policy isn't
> > loaded, SBC seems like a sensible default).
> > 
> > Is there a need for the user to be able to choose the codec? Shouldn't
> > we always automatically pick the highest-quality one?
> 
> What is "highest-quality" codec? How you would compare e.g. our SBC
> implementation which can dynamically change bitpool and therefore
> quality?
> 
> Also how you can compare... has SBC higher quality as MPEG1? Or has aptX
> HD higher quality as MPEG/AAC?

Fair point.

> Until now everybody said that aptX "is better" then SBC. But today it
> does not look like it is truth.
> 
> Also if you have MP3 files on disk, then the best quality which you can
> achieve is to passthru it without any reencoding. In other cases AAC can
> be better.

Passthrough is a separate thing. If someone implements passthrough
support for bluetooth and a client requests it, we should always switch
to the requested codec regardless of other user preferences.

> So answer to this question depends on lot of things and either user
> itself or user's application would better it.
> 
> > I'm not against
> > adding the functionality, it would be useful for testing if nothing
> > else. It just doesn't seem very important.
> > 
> > We could have a "preferred-a2dp-codec" option in bluetooth.conf (that
> > file doesn't exist, but can be added). A per-card option would be
> > possible too, as would be having both a global option that could be
> > overridden with a per-card option.
> 
> Preferred codec is not helpful. The only mandatory codec is SBC, all
> other are optional. And basically every manufactor supports different
> set of codecs. So at least some kind of list or partially ordered set of
> codec is needed.

Sure, a priority list can better capture the user preferences, but
surely a "preferred codec" option is better than nothing. A priority
list of codecs isn't an obvious ultimate solution either, since some
codecs have adjustable parameters that can affect the codec preference
ranking. For example, someone's preference might be:

SBC (high bitrate)
aptX
SBC (medium bitrate)

It's not clear to me how fine-grained control is optimal. A solution
that allows specifying a priority list with an arbitrary number of
codec parameters would obviously be sufficient, but it's unlikely we
want that complex system.

Real use cases are needed. I can't provide any, since I don't use
bluetooth (and if I did, I probably wouldn't bother with the codec
settings at all - SBC and aptX seem both good enough for me). Now that
you found out that aptX isn't that great after all, do you have
personal use for aptX either?

> > For runtime configuration, we can add a command to set the codec
> > preference. This should be done with the message API that Georg Chini
> > has been working on (not yet finished). There's then need for saving
> > the choice too. We can either introduce a new database for bluetooth
> > stuff, or we can add some API to module-card-restore so that other
> > modules (module-bluetooth-discover in this case) can save arbitrary
> > parameters for cards.
> > 
> > I recall there was some lack of relevant APIs in bluetoothd for
> > choosing the codec from PulseAudio. Can you remind me, what are the
> > limitations, and how did you plan to deal with them?
> 
> Currently when bluetooth headset initialize connection, it is up to
> headset which codec would choose. I did some testing and my headset
> choose codec (sbc vs aptx) just randomly.
> 
> When bluez initialize connection, then it prefer codecs in order in
> which pulseaudio did dbus endpoint registration. Order is global and
> hardcoded in source code.
> 
> Once connection is established there is no way to change codec. bluez
> does not provide API for it.
> 
> So everything which you wrote above would apply only when pulseaudio
> starts a2dp connection, not headset.
> 
> And I think it is very bad if we cannot achieve stable codec selection.

So what do you propose? If your point was that my suggestions result in
unstable codec selection, I don't see how your original proposal of
having a separate card profile for each codec (or potentially each set
of codec parameters) makes the situation better.

> > Comments on the code below. This review was done over several days,
> > sorry for any inconsistencies or strange repetition (I noticed I
> > explain some things multiple times).
> 
> Currently I do not have much time to work on this. So just few comments
> below.
> 
> > > @@ -888,10 +889,21 @@ finish:
> > >  static void register_endpoint(pa_bluetooth_discovery *y, const char *path, const char *endpoint, const char *uuid) {
> > >      DBusMessage *m;
> > >      DBusMessageIter i, d;
> > > -    uint8_t codec = 0;
> > > +    uint8_t capabilities[1024];
> > > +    size_t capabilities_size;
> > > +    uint8_t codec_id;
> > > +    const pa_a2dp_codec_t *codec;
> > > +
> > > +    codec = pa_a2dp_endpoint_to_a2dp_codec(endpoint);
> > 
> > I think it would be better to change the function parameters so that
> > instead of an endpoint path the function would take a codec id.
> 
> What do you mean by codec id? Do you mean A2DP codec id? If yes, then it
> would not work, as every non-sbc and non-mpeg codec has codec is 0xFF.
> Codec itself is then determined by passed capabilities.

I thought we could use the id from the A2DP spec, but apparently not. I
propose having our own separate codec enumeration then that provides
the necessary ids.

> > There
> > would be a function called get_a2dp_codec() that would take the codec
> > id as a parameter and return the codec struct. The endpoint path could
> > be added to the codec struct.
> > 
> > Shuffling things around like this wouldn't have huge benefits, I just
> > find looking up the codec based on the id neater than based on the
> > endpoint path.
> > 
> > I hope you don't have a problem with that suggestion, but if you do and
> > you want to keep using pa_a2dp_endpoint_to_a2dp_codec(), then the
> > function should be made static (it's not used outside bluez5-utils.c)
> > and the pa_ prefix should be dropped, since functions that aren't
> > exported through headers should not use the pa_ prefix according to our
> > coding style.
> > > +    sbc_info->min_bitpool = config->min_bitpool;
> > > +    sbc_info->max_bitpool = config->max_bitpool;
> > > +
> > > +    /* Set minimum bitpool for source to get the maximum possible block_size */
> > > +    sbc_info->sbc.bitpool = is_a2dp_sink ? sbc_info->max_bitpool : sbc_info->min_bitpool;
> > 
> > Do you understand the logic here?
> 
> I have not looked deeply at this code. So I'm not fully sure. I just
> moved existing code into new file.

Ok, so this remains a mystery to all of us.
On Tue, 4 Sep 2018, at 2:14 PM, Tanu Kaskinen wrote:
> Hi Pali!
> 
> Thanks a lot for working on this! Arun has been strongly advocating
> using GStreamer, and I don't know if his position is that "AptX must be
> implemented with GStreamer or not implemented at all". I hope not. To
> me the library choice is not important. Using libopenaptx directly
> doesn't prevent us from switching to GStreamer later if someone writes
> the code.

A couple of notes. Firstly, I really do appreciate all the work that Pali has put into this (it is clearly a lot).

Secondly, while I am dead against adding a codec as a dep, once we get things in shape where this is the only issue, I volunteer to help with the GStreamer bits so as to not block this.

Cheers,
Arun
On Monday 17 September 2018 15:02:18 Tanu Kaskinen wrote:
> (I sent my review both to you and to the list, but you replied only
> privately - probably not on purpose. I'll send this mail only to the
> list.)

Sorry for that. I was not my intension.

> On Thu, 2018-09-13 at 10:54 +0200, Pali Rohár wrote:
> > On Tuesday 04 September 2018 11:44:10 Tanu Kaskinen wrote:
> > > Using libopenaptx directly
> > > doesn't prevent us from switching to GStreamer later if someone writes
> > > the code.
> > 
> > Yes, that is truth.
> > 
> > > Another point that was raised that the codec choice shouldn't be bound
> > > to the card profile. I tend to agree with that. There are situations
> > > where module-bluetooth-policy or the user only wants to switch from HSP
> > > to A2DP and not care about the codec. You asked how the codec should be
> > > negotiated or switched by the user if it's not bound to the profile.
> > > Regarding negotiation, we can add a hook that module-bluetooth-policy
> > > can use to make the codec decision (if module-bluetooth-policy isn't
> > > loaded, SBC seems like a sensible default).
> > > 
> > > Is there a need for the user to be able to choose the codec? Shouldn't
> > > we always automatically pick the highest-quality one?
> > 
> > What is "highest-quality" codec? How you would compare e.g. our SBC
> > implementation which can dynamically change bitpool and therefore
> > quality?
> > 
> > Also how you can compare... has SBC higher quality as MPEG1? Or has aptX
> > HD higher quality as MPEG/AAC?
> 
> Fair point.
> 
> > Until now everybody said that aptX "is better" then SBC. But today it
> > does not look like it is truth.
> > 
> > Also if you have MP3 files on disk, then the best quality which you can
> > achieve is to passthru it without any reencoding. In other cases AAC can
> > be better.
> 
> Passthrough is a separate thing. If someone implements passthrough
> support for bluetooth and a client requests it, we should always switch
> to the requested codec regardless of other user preferences.
> 
> > So answer to this question depends on lot of things and either user
> > itself or user's application would better it.
> > 
> > > I'm not against
> > > adding the functionality, it would be useful for testing if nothing
> > > else. It just doesn't seem very important.
> > > 
> > > We could have a "preferred-a2dp-codec" option in bluetooth.conf (that
> > > file doesn't exist, but can be added). A per-card option would be
> > > possible too, as would be having both a global option that could be
> > > overridden with a per-card option.
> > 
> > Preferred codec is not helpful. The only mandatory codec is SBC, all
> > other are optional. And basically every manufactor supports different
> > set of codecs. So at least some kind of list or partially ordered set of
> > codec is needed.
> 
> Sure, a priority list can better capture the user preferences, but
> surely a "preferred codec" option is better than nothing. A priority
> list of codecs isn't an obvious ultimate solution either, since some
> codecs have adjustable parameters that can affect the codec preference
> ranking. For example, someone's preference might be:
> 
> SBC (high bitrate)
> aptX
> SBC (medium bitrate)
> 
> It's not clear to me how fine-grained control is optimal. A solution
> that allows specifying a priority list with an arbitrary number of
> codec parameters would obviously be sufficient, but it's unlikely we
> want that complex system.

The best thing is allowing user to choose codec itself (in some GUI
under section additional settings, or similar) and ideally remember it
for every device separately.

> Real use cases are needed. I can't provide any, since I don't use
> bluetooth (and if I did, I probably wouldn't bother with the codec
> settings at all - SBC and aptX seem both good enough for me). Now that
> you found out that aptX isn't that great after all, do you have
> personal use for aptX either?

This is interesting question. For more then month I'm using aptX codec
and I have not hear difference or something which would disturb me.

So I probably really do not need aptX at all.

> > > For runtime configuration, we can add a command to set the codec
> > > preference. This should be done with the message API that Georg Chini
> > > has been working on (not yet finished). There's then need for saving
> > > the choice too. We can either introduce a new database for bluetooth
> > > stuff, or we can add some API to module-card-restore so that other
> > > modules (module-bluetooth-discover in this case) can save arbitrary
> > > parameters for cards.
> > > 
> > > I recall there was some lack of relevant APIs in bluetoothd for
> > > choosing the codec from PulseAudio. Can you remind me, what are the
> > > limitations, and how did you plan to deal with them?
> > 
> > Currently when bluetooth headset initialize connection, it is up to
> > headset which codec would choose. I did some testing and my headset
> > choose codec (sbc vs aptx) just randomly.
> > 
> > When bluez initialize connection, then it prefer codecs in order in
> > which pulseaudio did dbus endpoint registration. Order is global and
> > hardcoded in source code.
> > 
> > Once connection is established there is no way to change codec. bluez
> > does not provide API for it.
> > 
> > So everything which you wrote above would apply only when pulseaudio
> > starts a2dp connection, not headset.
> > 
> > And I think it is very bad if we cannot achieve stable codec selection.
> 
> So what do you propose? If your point was that my suggestions result in
> unstable codec selection, I don't see how your original proposal of
> having a separate card profile for each codec (or potentially each set
> of codec parameters) makes the situation better.

Wait until bluez adds new API for choosing codec and use this new API in
pulseaudio for codec selection. This is really something which we need
for proper and deterministic (multi)codec support.

> > > Comments on the code below. This review was done over several days,
> > > sorry for any inconsistencies or strange repetition (I noticed I
> > > explain some things multiple times).
> > 
> > Currently I do not have much time to work on this. So just few comments
> > below.
> > 
> > > > @@ -888,10 +889,21 @@ finish:
> > > >  static void register_endpoint(pa_bluetooth_discovery *y, const char *path, const char *endpoint, const char *uuid) {
> > > >      DBusMessage *m;
> > > >      DBusMessageIter i, d;
> > > > -    uint8_t codec = 0;
> > > > +    uint8_t capabilities[1024];
> > > > +    size_t capabilities_size;
> > > > +    uint8_t codec_id;
> > > > +    const pa_a2dp_codec_t *codec;
> > > > +
> > > > +    codec = pa_a2dp_endpoint_to_a2dp_codec(endpoint);
> > > 
> > > I think it would be better to change the function parameters so that
> > > instead of an endpoint path the function would take a codec id.
> > 
> > What do you mean by codec id? Do you mean A2DP codec id? If yes, then it
> > would not work, as every non-sbc and non-mpeg codec has codec is 0xFF.
> > Codec itself is then determined by passed capabilities.
> 
> I thought we could use the id from the A2DP spec, but apparently not. I
> propose having our own separate codec enumeration then that provides
> the necessary ids.
> 
> > > There
> > > would be a function called get_a2dp_codec() that would take the codec
> > > id as a parameter and return the codec struct. The endpoint path could
> > > be added to the codec struct.
> > > 
> > > Shuffling things around like this wouldn't have huge benefits, I just
> > > find looking up the codec based on the id neater than based on the
> > > endpoint path.
> > > 
> > > I hope you don't have a problem with that suggestion, but if you do and
> > > you want to keep using pa_a2dp_endpoint_to_a2dp_codec(), then the
> > > function should be made static (it's not used outside bluez5-utils.c)
> > > and the pa_ prefix should be dropped, since functions that aren't
> > > exported through headers should not use the pa_ prefix according to our
> > > coding style.
> > > > +    sbc_info->min_bitpool = config->min_bitpool;
> > > > +    sbc_info->max_bitpool = config->max_bitpool;
> > > > +
> > > > +    /* Set minimum bitpool for source to get the maximum possible block_size */
> > > > +    sbc_info->sbc.bitpool = is_a2dp_sink ? sbc_info->max_bitpool : sbc_info->min_bitpool;
> > > 
> > > Do you understand the logic here?
> > 
> > I have not looked deeply at this code. So I'm not fully sure. I just
> > moved existing code into new file.
> 
> Ok, so this remains a mystery to all of us.
>
Hi Tanu! I'm working on a new version of this patch series.

Below are comments, I'm fixing problems which you pointed. Thank you for
review.

Also, on bluez mailing list are patches which add support for profile
switching, so I'm implementing it for this patch series.

Once I have implemented it, I will send a new version to pulseaudio
mailing list.

Most important change is removal of all codec specific enums, ifdefs,
etc... from bluez5-util.c and module-bluez5-device.c files. So for
adding new codec it would not be needed to touch these files!

On Tuesday 04 September 2018 11:44:10 Tanu Kaskinen wrote:
> On Sat, 2018-07-28 at 17:34 +0200, 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 ++
> 
> The "pa-" prefix doesn't look very good to me. Maybe you didn't want to
> add a2dp-codec.h, because it looks so similar to the existing a2dp-
> codecs.h header? I think we can get rid of a2dp-codecs.h: the SBC stuff
> can be moved to a2dp-codec-sbc.c, and the MPEG stuff can be dropped
> since it's not used anywhere.

I'm going to change/fix it.

a2dp-codec-api.h     --> structure definitions for implementing codecs
a2dp-codec-<CODEC>.c --> particular codec implementation
a2dp-codecs.h        --> upstream bluez header file for A2DP definitions
a2dp-codec-util.h    --> header file for utility functions for working
                         with codecs (e.g. listing all codecs, etc.)
a2dp-codec-util.c    --> implementation of a2dp-codec-util.h

> > @@ -888,10 +889,21 @@ finish:
> >  static void register_endpoint(pa_bluetooth_discovery *y, const char *path, const char *endpoint, const char *uuid) {
> >      DBusMessage *m;
> >      DBusMessageIter i, d;
> > -    uint8_t codec = 0;
> > +    uint8_t capabilities[1024];
> > +    size_t capabilities_size;
> > +    uint8_t codec_id;
> > +    const pa_a2dp_codec_t *codec;
> > +
> > +    codec = pa_a2dp_endpoint_to_a2dp_codec(endpoint);
> 
> I think it would be better to change the function parameters so that
> instead of an endpoint path the function would take a codec id.

That is not possible. endpoint is bound to pair (codec + direction).
I'm changing function parameters, so pulseaudio codec structure is
passed too (together with endpoint).

This simplify lot of other things and removal of
pa_a2dp_endpoint_to_a2dp_codec function call from there.

> > +    if (!codec)
> > +        return;
> 
> As far as I can tell, this should never happen, so an assertion would
> be better (and it could be in the lookup function so that every caller
> doesn't need to add a check).

After above change, yes, so removing it.

> > @@ -1316,6 +1271,38 @@ const char *pa_bluetooth_profile_to_string(pa_bluetooth_profile_t profile) {
> >      return NULL;
> >  }
> >  
> > +const char *pa_bluetooth_profile_to_a2dp_endpoint(pa_bluetooth_profile_t profile) {
> 
> This function isn't used outside bluez5-util.c, so it can be made
> static and removed from bluez5-util.h. Then the pa_bluetooth_ prefix
> should be dropped.

Yes, doing it.

> > +    switch (profile) {
> > +        case PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK:
> > +            return A2DP_SOURCE_SBC_ENDPOINT;
> > +        case PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE:
> > +            return A2DP_SINK_SBC_ENDPOINT;
> > +        default:
> > +            return NULL;
> 
> I think it would be good to use pa_assert_not_reached() here. I assume
> this won't be used in a context where a non-a2dp profile would be
> passed to the function.

The whole bluez5-util.c file does not have any codec specific enums, so
this above profile switch was removed.

> > +    }
> > +
> > +    return NULL;
> 
> This is redundant.
> 
> > +}
> > +
> > +const pa_a2dp_codec_t *pa_bluetooth_profile_to_a2dp_codec(pa_bluetooth_profile_t profile) {
> > +    switch (profile) {
> > +        case PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK:
> > +        case PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE:
> > +            return &pa_a2dp_codec_sbc;
> > +        default:
> > +            return NULL;
> > +    }
> > +
> > +    return NULL;
> > +}
> 
> This function seems to be used also for checking whether a profile is
> an a2dp profile. I think it would be clearer to have a separate
> pa_bluetooth_profile_is_a2dp() function. Then this function could also
> have an assertion rather than returning NULL for non-a2dp profiles.

Yes, I added new functions pa_bluetooth_profile_is_a2dp(),
pa_bluetooth_profile_is_a2dp_sink() and
pa_bluetooth_profile_is_a2dp_source().

They helped in other parts of code.

> If we don't tie the codec and the profile together, however, then
> there's no need for an is_a2dp() function.
> 
> > +
> > +const pa_a2dp_codec_t *pa_a2dp_endpoint_to_a2dp_codec(const char *endpoint) {
> > +    if (pa_streq(endpoint, A2DP_SOURCE_SBC_ENDPOINT) || pa_streq(endpoint, A2DP_SINK_SBC_ENDPOINT))
> > +        return &pa_a2dp_codec_sbc;
> > +    else
> > +        return NULL;
> 
> pa_assert_not_reached() could be used here.

In endpoint_handler() I'm getting untrusted bluez dbus data, and above
function is calling on endpoint from bluez. So it should not throw
assert error, but in endpoint_handler() returns
DBUS_HANDLER_RESULT_NOT_YET_HANDLED.

So pa_assert_not_reached() cannot be used there.

> > +    config_size = codec->select_configuration(&y->core->default_sample_spec, cap, size, config, sizeof(config));
> > +    pa_assert(config_size != 0);
> 
> We don't trust input from bluetoothd to be always valid, so
> select_configuration() can fail. Proper error handling is needed here.

codec->select_configuration is not from bluetoothd, but from pulseaudio
codec. And we should trust to our pulseaudio code.

> > @@ -1709,38 +1578,22 @@ static void endpoint_init(pa_bluetooth_discovery *y, pa_bluetooth_profile_t prof
> >      static const DBusObjectPathVTable vtable_endpoint = {
> >          .message_function = endpoint_handler,
> >      };
> > +    const char *endpoint = pa_bluetooth_profile_to_a2dp_endpoint(profile);
> 
> The function could just take the endpoint path as an argument.
> pa_bluetooth_profile_t is not needed by this function.

Yes, I changed function argument from profile to endpoint.

> >  static void endpoint_done(pa_bluetooth_discovery *y, pa_bluetooth_profile_t profile) {
> > +    const char *endpoint = pa_bluetooth_profile_to_a2dp_endpoint(profile);
> 
> Same comment as above.

Fixed too.

> >  struct userdata {
> >      pa_module *module;
> >      pa_core *core;
> > @@ -146,7 +132,11 @@ struct userdata {
> >      pa_smoother *read_smoother;
> >      pa_memchunk write_memchunk;
> >      pa_sample_spec sample_spec;
> > -    struct sbc_info sbc_info;
> > +    void *encoder_info;
> > +    void *decoder_info;
> 
> I think it would be better to put these inside the pa_a2dp_codec
> struct, preferably behind a single void pointer named "userdata" (as
> that would follow the usual PA code conventions). The various callbacks
> in pa_a2dp_codec that now take a void** parameter as their first
> argument would take a pa_a2dp_codec pointer instead.

These two pointers are user data passed to function callbacks in
pa_a2dp_codec strctures. So they need to be in module/stream user data.

Anyway, I was thinking about putting encoder_info and decoder_info into
one structure, but that decreased complexity in codec implementation. So
in current state it is easier to implement and a new codec.

So I would rather leave it as is.

> > +
> > +    void* buffer;                        /* Codec transfer buffer */
> > +    size_t buffer_size;                  /* Size of the buffer */
> >  };

> > @@ -1078,7 +924,7 @@ static int add_source(struct userdata *u) {
> >  
> >      if (!u->transport_acquired)
> >          switch (u->profile) {
> > -            case PA_BLUETOOTH_PROFILE_A2DP_SOURCE:
> > +            case PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE:
> >              case PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY:
> >                  data.suspend_cause = PA_SUSPEND_USER;
> >                  break;
> > @@ -1090,8 +936,9 @@ static int add_source(struct userdata *u) {
> >                  else
> >                      pa_assert_not_reached();
> >                  break;
> > -            case PA_BLUETOOTH_PROFILE_A2DP_SINK:
> > +            case PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK:
> >              case PA_BLUETOOTH_PROFILE_OFF:
> > +            default:
> 
> If a switch statement has cases for all members of an enumeration, as
> is the case here, then it's better to leave the default case out,
> because the default case removes compiler warnings about unhandled
> cases. If we add a new profile, it's good to get a warning if it's not
> handled here.

All codec specific enums were removed from module-bluez5-device.c file.
So above comment is not relevant for new patch version.

> > +typedef struct sbc_info {
> > +    sbc_t sbc;                           /* Codec data */
> > +    bool sbc_initialized;                /* Keep track if the encoder is initialized */
> > +    size_t codesize, frame_length;       /* SBC Codesize, frame_length. We simply cache those values here */
> > +    uint16_t seq_num;                    /* Cumulative packet sequence */
> > +    uint8_t min_bitpool;
> > +    uint8_t max_bitpool;
> > +} sbc_info_t;
> 
> According to our coding style, structs that aren't exported in any
> headers should not be typedef'd. And if structs are typedef'd, they
> shouldn't have a "_t" suffix. (Yes, the typedef existed in the old code
> too, but I thought I'd mention this anyway.)

Fixing it.

> > +
> > +static size_t pa_sbc_fill_capabilities(uint8_t *capabilities_buffer, size_t capabilities_size) {
> 
> Static functions shouldn't have the "pa_" prefix. The "sbc_" prefix
> feels a bit redundant too. This comment applies to all functions in
> this file.

Fixing it.

> > +    if (capabilities->allocation_method & SBC_ALLOCATION_LOUDNESS)
> > +        config->allocation_method = SBC_ALLOCATION_LOUDNESS;
> > +    else if (capabilities->allocation_method & SBC_ALLOCATION_SNR)
> > +        config->allocation_method = SBC_ALLOCATION_SNR;
> 
> Error handling for unsupported allocation method is missing. Not your
> fault, and this doesn't look serious (because the final configuration
> is validated anyway), but you can fix this if you want.

Fixing it.

> > diff --git a/src/modules/bluetooth/pa-a2dp-codec.h b/src/modules/bluetooth/pa-a2dp-codec.h
> > new file mode 100644
> > index 000000000..68b1619c2
> > --- /dev/null
> > +++ b/src/modules/bluetooth/pa-a2dp-codec.h
> > @@ -0,0 +1,40 @@
> > +#ifndef foopaa2dpcodechfoo
> > +#define foopaa2dpcodechfoo
> > +
> > +/***
> > +  This file is part of PulseAudio.
> > +
> > +  Copyright 2018 Pali Rohár <pali.rohar@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
> > +  published by the Free Software Foundation; either version 2.1 of the
> > +  License, or (at your option) any later version.
> > +
> > +  PulseAudio is distributed in the hope that it will be useful, but
> > +  WITHOUT ANY WARRANTY; without even the implied warranty of
> > +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > +  General Public License for more details.
> > +
> > +  You should have received a copy of the GNU Lesser General Public
> > +  License along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
> > +***/
> > +
> > +typedef struct pa_a2dp_codec {
> > +    const char *codec_name;
> 
> This is currently unused, so it should be removed unless you come up
> with some use for it.

In new patch version it is used.

> > +    uint8_t codec_id;
> > +    size_t (*fill_capabilities)(uint8_t *, size_t);
> 
> It would be good to have some documentation for the callbacks. Naming
> the function parameters also makes it easier to understand the code.
> Imagine being someone who's not that familiar with the code but has
> decided to add support for a new codec and needs to write
> implementations for the callbacks.

I know. I'm adding documentation comments for all callback functions and
all members. So adding new codec would be easier.

Also in patch version, I'm changing function names so they would better
describe what they are doing, plus I'm changing API...

> > +    bool (*validate_configuration)(const uint8_t *, size_t);
> 
> The bool return value is used for reporting failures, but according to
> our coding style failures should be reported using a negative int.

It is really needed to use integer return value for true/false?

> > +    size_t (*select_configuration)(const pa_sample_spec *, const uint8_t *, size_t, uint8_t *, size_t);
> > +    void (*init)(void **, pa_sample_spec *, bool, const uint8_t *, size_t);
> > +    void (*finish)(void **);
> > +    void (*setup)(void **);
> > +    void (*fill_blocksize)(void **, size_t, size_t, size_t *, size_t *);
> > +    bool (*fix_latency)(void **);
> 
> I feel this callback name is not very good. The callback reduces the
> bitpool (in case of SBC), it doesn't fix the latency. Fixing the
> latency is left to the caller.
> 
> I suggest changing the name to "reduce_bitrate" (sufficiently generic
> to not be specific to SBC).

That is better name! I will use reduce_encoder_bitrate.

> I also suggest some restructuring, because
> the logic behind calling transport_config_mtu() and
> update_buffer_size() after fix_latency() wasn't obvious. Restructuring
> ideas:
> 
> 1) Allow the reduce_bitrate() callback to directly update the block
> size. The callback should return true only if the block size was
> changed.
> 
> This will require access to the MTU sizes from reduce_bitrate(), so
> fill_blocksize() should save the MTU sizes in the codec private data
> (or the MTU could be passed as arguments to reduce_bitrate(), but I
> think it's cleaner to save the previously set MTU in the codec private
> data). It might make sense to rename fill_blocksize() to set_mtu().
> 
> 2) Add a handle_block_size_change() function and move there the sink
> latency update code from transport_config_mtu() and the code from
> update_buffer_size(). Call handle_block_size_change() from
> transport_configu_mtu() and after the reduce_bitrate() call. Remove the
> transport_mtu_config() and update_buffer_size() calls from
> thread_function(). Remove the update_buffer_size() call from
> setup_stream(), it's not needed because transport_mtu_config() will
> update the kernel buffer size via handle_block_size_change().
> update_buffer_size() can be removed altogheter.
> 
> As a result the code in thread_func() will look like this, which I find
> much more self-explanatory:
> 
>     bool block_size_changed = codec->reduce_bitrate(&u->encoder_info, &u->write_block_size);
> 
>     if (block_size_changed)
>         handle_block_size_change(u);

That is nice code cleanup. I'm doing it.

> > +    size_t (*encode)(void **, uint32_t, const uint8_t *, size_t, uint8_t *, size_t);
> > +    size_t (*decode)(void **, const uint8_t *, size_t, uint8_t *, size_t, size_t *);
> > +} pa_a2dp_codec_t;
> 
> According to our coding style, struct names shouldn't contain the _t
> suffix. The _t suffix is only used with basic types like ints.

Fixed.