On Sun, Aug 18, 2013 at 6:37 AM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> 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."
>
Sorry, the original message about this patch has been lost in my
inbox. These values are the SBC bitpool values that we chosen for each
sampling frequency and channel mode. The choices for sampling
frequencies of 44100 and 48000 are based on the recommended values in
the A2DP spec. I'm not sure where the the other choices came from
(IIRC this code has been copied from the original ALSA A2DP support),
maybe Luiz can comment about it.
>> +
>> + 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.
>
In my understanding this is the same case of your previous comment in
enpoint_set_configuration(): a2dp_sbc_t is 1-byte aligned (packed and
has only uint8_t fields), so there shouldn't be any problem. Or am I
missing something?
>> + 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."
>
Yes, I think it is better to return an error to the caller here.
> 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.
>
Sorry, I didn't understand the second part of your comment. If
default_sample_spec.channels == 2 and cap->channel_mode has only
SBC_CHANNEL_MODE_MONO, then config.channel_mode will receive
SBC_CHANNEL_MODE_MONO.
>> +
>> + 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?"
>
Yes, that makes sense.
--
Jo?o Paulo Rechi Vita
http://about.me/jprvita
