On Fri, 2013-07-12 at 15:06 -0300, jprvita at gmail.com wrote:
> From: Jo?o Paulo Rechi Vita <jprvita at openbossa.org>
>
> ---
> src/Makefile.am | 3 +-
> src/modules/bluetooth/bluez5-util.c | 155 +++++++++++++++++++++++++++++++++++-
> 2 files changed, 155 insertions(+), 3 deletions(-)
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 52cd63f..0458a57 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -2052,7 +2052,8 @@ module_bluez4_device_la_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS) $(SBC_CFLAGS)
> # Bluetooth BlueZ 5 sink / source
> libbluez5_util_la_SOURCES = \
> modules/bluetooth/bluez5-util.c \
> - modules/bluetooth/bluez5-util.h
> + modules/bluetooth/bluez5-util.h \
> + modules/bluetooth/a2dp-codecs.h
> libbluez5_util_la_LDFLAGS = -avoid-version
> libbluez5_util_la_LIBADD = $(MODULE_LIBADD) $(DBUS_LIBS)
> libbluez5_util_la_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS)
> diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c
> index a68f8ca..d8cf1df 100644
> --- a/src/modules/bluetooth/bluez5-util.c
> +++ b/src/modules/bluetooth/bluez5-util.c
> @@ -33,6 +33,8 @@
> #include <pulsecore/refcnt.h>
> #include <pulsecore/shared.h>
>
> +#include "a2dp-codecs.h"
> +
> #include "bluez5-util.h"
>
> #define BLUEZ_SERVICE "org.bluez"
> @@ -349,6 +351,51 @@ fail:
> return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
> }
>
> +static uint8_t a2dp_default_bitpool(uint8_t freq, uint8_t mode) {
I think this function should have a comment explaining on what basis the
return values are chosen. As it stands, they're just random numbers to
me.
> +
> + 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;
> +
> + default:
> + 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;
> +
> + default:
> + pa_log_warn("Invalid channel mode %u", mode);
> + return 51;
> + }
> +
> + default:
> + pa_log_warn("Invalid sampling freq %u", freq);
> + return 53;
> + }
> +}
> +
> static DBusMessage *endpoint_set_configuration(DBusConnection *conn, DBusMessage *m, void *userdata) {
> pa_bluetooth_discovery *y = userdata;
> pa_bluetooth_device *d;
> @@ -471,11 +518,115 @@ 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;
> DBusMessage *r;
> + DBusError err;
>
> - pa_assert_se(r = dbus_message_new_error(m, BLUEZ_MEDIA_ENDPOINT_INTERFACE ".Error.NotImplemented",
> - "Method not implemented"));
> + 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 }
> + };
> +
> + dbus_error_init(&err);
> +
> + if (!dbus_message_get_args(m, &err, DBUS_TYPE_ARRAY, DBUS_TYPE_BYTE, &cap, &size, DBUS_TYPE_INVALID)) {
> + pa_log_error("Endpoint SelectConfiguration(): %s", err.message);
> + dbus_error_free(&err);
> + goto fail;
> + }
> +
> + pa_assert(size == sizeof(config));
size is input from another process, and we don't trust input from other
processes. This needs proper error handling.
> +
> + memset(&config, 0, sizeof(config));
pa_zero() can be used.
> +
> + /* 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;
> + }
> +
> + 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 default_sample_spec.channels is 1, but cap doesn't contain
SBC_CHANNEL_MODE_MONO, then config.channel_mode is left uninitialized
(well, it's initialized to zero in the beginning). I believe this is not
what you intended.
Also, if default_sample_spec.channels is 2, but cap only contains
SBC_CHANNEL_MODE_MONO, then the negotiation fails. I think that's not
right, you shouldn't treat default_sample_spec as a hard restriction.
>
> + 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;
> + }
> +
> + 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;
else branch is missing.
> +
> + 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);
Should the negotiation fail if the resulting config.min_bitpool is
greater than config.max_bitpool?
--
Tanu
