[pulseaudio-discuss] [PATCH 30/56] bluetooth: Implement org.bluez.MediaEndpoint1.SelectConfiguration()

Tanu Kaskinen tanu.kaskinen at linux.intel.com
Fri Jul 19 04:04:53 PDT 2013


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



More information about the pulseaudio-discuss mailing list