On Tue, 2013-08-13 at 01:54 -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 | 156 +++++++++++++++++++++++++++++++++++-
> 2 files changed, 156 insertions(+), 3 deletions(-)
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index ff8bb42..3759b3c 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -2055,7 +2055,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 d0428a9..1194503 100644
> --- a/src/modules/bluetooth/bluez5-util.c
> +++ b/src/modules/bluetooth/bluez5-util.c
> @@ -361,6 +361,51 @@ fail:
> return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
> }
>
> +static uint8_t a2dp_default_bitpool(uint8_t freq, uint8_t mode) {
You didn't respond to my previous comment: "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;
> + }
> +}
> +
> const char *pa_bluetooth_profile_to_string(pa_bluetooth_profile_t profile) {
> switch(profile) {
> case PROFILE_A2DP_SINK:
> @@ -536,11 +581,118 @@ 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)) {
Passing &cap may cause alignment issues when accessing cap later. A byte
array pointer should be passed instead, and the memory should be copied
to an a2dp_sbc_t variable.
> + pa_log_error("Endpoint SelectConfiguration(): %s", err.message);
> + dbus_error_free(&err);
> + 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;
> + }
> +
> + 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;
> + }
> + }
You didn't address my earlier complaint: "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."
I also said that if default_sample_spec is 2 and cap only contains
SBC_CHANNEL_MODE_MONO, then the negotiation fails. It seems that that
complaint was bogus, though, so you're right to ignore that bit.
> +
> + 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;
> +
> + 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);
You didn't respond to my question: "Should the negotiation fail if the
resulting config.min_bitpool is greater than config.max_bitpool?"
--
Tanu
