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 at gmail.com>
>
> PulseAudio is free software; you can redistribute it and/or modify
> it under the terms of the GNU Lesser General Public License as
> @@ -33,7 +34,7 @@
> #include <pulsecore/refcnt.h>
> #include <pulsecore/shared.h>
>
> -#include "a2dp-codecs.h"
> +#include "pa-a2dp-codec.h"
>
> #include "bluez5-util.h"
>
> @@ -48,8 +49,8 @@
>
> #define BLUEZ_ERROR_NOT_SUPPORTED "org.bluez.Error.NotSupported"
>
> -#define A2DP_SOURCE_ENDPOINT "/MediaEndpoint/A2DPSource"
> -#define A2DP_SINK_ENDPOINT "/MediaEndpoint/A2DPSink"
> +#define A2DP_SOURCE_SBC_ENDPOINT "/MediaEndpoint/A2DPSourceSBC"
> +#define A2DP_SINK_SBC_ENDPOINT "/MediaEndpoint/A2DPSinkSBC"
>
> #define ENDPOINT_INTROSPECT_XML \
> DBUS_INTROSPECT_1_0_XML_DOCTYPE_DECL_NODE \
> @@ -170,9 +171,9 @@ static const char *transport_state_to_string(pa_bluetooth_transport_state_t stat
>
> static bool device_supports_profile(pa_bluetooth_device *device, pa_bluetooth_profile_t profile) {
> switch (profile) {
> - case PA_BLUETOOTH_PROFILE_A2DP_SINK:
> + case PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK:
> return !!pa_hashmap_get(device->uuids, PA_BLUETOOTH_UUID_A2DP_SINK);
> - case PA_BLUETOOTH_PROFILE_A2DP_SOURCE:
> + case PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE:
> return !!pa_hashmap_get(device->uuids, 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 at gmail.com>
>
> PulseAudio is free software; you can redistribute it and/or modify
> it under the terms of the GNU Lesser General Public License as
> @@ -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 at gmail.com>
>
> PulseAudio is free software; you can redistribute it and/or modify
> it under the terms of the GNU Lesser General Public License as
> @@ -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 at gmail.com>
> +
> + PulseAudio is free software; you can redistribute it and/or modify
> + it under the terms of the GNU Lesser General Public License as
> + 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 at gmail.com>
> +
> + PulseAudio is free software; you can redistribute it and/or modify
> + it under the terms of the GNU Lesser General Public License as
> + 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
--
Tanu
https://www.patreon.com/tanuk
https://liberapay.com/tanuk
