[pulseaudio-discuss] [PATCHv2 31/60] bluetooth: Implement org.bluez.MediaEndpoint1.SelectConfiguration()

[Tanu Kaskinen]

On Tue, 2013-09-03 at 21:18 -0300, João Paulo Rechi Vita wrote:
> 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:
> >>  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?

No, you're not missing anything. My mistake.

> >> +        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.

There's no need to return an error, we should allow any of the
two-channel modes even if default_sample_spec.channels is 1.

> > 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.

Yes, this is what I meant by the complaint being bogus, so your code is
fine on this part.

-- 
Tanu