[pulseaudio-discuss] [PATCH v4 09/41] bluetooth: Implement org.bluez.MediaEndpoint1.SelectConfiguration()

[João Paulo Rechi Vita]

On Tue, Sep 24, 2013 at 6:19 AM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> On Mon, 2013-09-23 at 08:39 -0500, João Paulo Rechi Vita wrote:
>> On Sun, Sep 22, 2013 at 12:19 AM, Tanu Kaskinen
>> <tanu.kaskinen at linux.intel.com> wrote:
>> > On Sat, 2013-09-21 at 13:30 -0500, João Paulo Rechi Vita wrote:
>> >> On Sat, Sep 21, 2013 at 5:16 AM, Tanu Kaskinen
>> >> <tanu.kaskinen at linux.intel.com> wrote:
>> >> > On Wed, 2013-09-18 at 16:17 -0500, 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)) {
>> >> >> +        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;
>> >> >
>> >> > You forgot to fix this code. The problem still exists that if
>> >> > default_sample_spec.channels is 1 and the cap->channel_mode doesn't
>> >> > contain SBC_CHANNEL_MODE_MONO, then config.channel_mode is left
>> >> > uninitialized.
>> >> >
>> >>
>> >> No, I didn't, but I may have misunderstood you. Re-reading it new
>> >> trying to give another interpretation, it seems you suggest that we
>> >> don't differentiate between (y->core->default_sample_spec.channels <=
>> >> 1) and (y->core->default_sample_spec.channels >= 2), but I don't think
>> >> that's what you meant. Can you please explain how we should
>> >> differentiate those two cases?
>> >
>> > There are four modes we support: mono, and three stereo modes. We should
>> > always allow any any of those modes. Depending on
>> > default_sample_spec.channels, we should either prefer the mono mode or
>> > the stereo modes, but if the device doesn't support our preferred mode,
>> > we should fall back to a mode that the device supports. In your code
>> > that fallback is implemented when default_sample_spec.channels >= 2, but
>> > not if default_sample_spec.channels == 1.
>> >
>>
>> Ok, and what preference order we should use between the stereo modes
>> for default_sample_spec.channels == 1? (I'm assuming mono is the 1st
>> preference in this case).
>
> Yes, mono is the first preferred mode. I don't have any idea about the
> ordering of the different stereo modes, except that the ordering should
> be the same for both default_sample_spec.channels == 1 and
> default_sample_spec.channels == 2, unless you know some reason to use
> different ordering.
>

Ok, I'll update that accordingly.

-- 
João Paulo Rechi Vita
http://about.me/jprvita