[pulseaudio-discuss] [PATCH v13 07/10] bluetooth: Add more variants of SBC codec

[Pali Rohár]

On Tuesday 21 January 2020 16:17:16 Georg Chini wrote:
> > +    /* choose remote capabilities which are compatible and its bitpool range is nearest to one from capabilities table */
> >       PA_HASHMAP_FOREACH_KV(key, a2dp_capabilities, capabilities_hashmap, state) {
> > -        if (can_accept_capabilities(a2dp_capabilities->buffer, a2dp_capabilities->size, for_encoding))
> > -            return key;
> > +        /* skip remote capabilities which are not compatible */
> > +        if (!can_accept_capabilities(a2dp_capabilities->buffer, a2dp_capabilities->size, false))
> > +            continue;
> > +
> > +        capabilities = (const a2dp_sbc_t *) a2dp_capabilities->buffer;
> > +
> > +        /* choose capabilities from our table which is compatible with sample spec and remote capabilities */
> > +        for (i = 0; i < capabilities_table_elements; i++) {
> > +            if (!are_capabilities_compatible(capabilities, &capabilities_table[i]))
> > +                continue;
> > +            /* For mono mode both capabilities must support mono */
> > +            if (is_mono && !((capabilities->channel_mode & SBC_CHANNEL_MODE_MONO) & (capabilities_table[i].channel_mode & SBC_CHANNEL_MODE_MONO)))
> > +                continue;
> > +            /* For non-mono mode both capabilities must support at least one common non-mode mode */
> > +            if (!is_mono && !((capabilities->channel_mode & (SBC_CHANNEL_MODE_JOINT_STEREO | SBC_CHANNEL_MODE_STEREO | SBC_CHANNEL_MODE_DUAL_CHANNEL)) & (capabilities_table[i].channel_mode & (SBC_CHANNEL_MODE_JOINT_STEREO | SBC_CHANNEL_MODE_STEREO | SBC_CHANNEL_MODE_DUAL_CHANNEL))))
> > +                continue;
> > +            /* And both capabilites must be compatible with chosen frequency */
> > +            if (!(capabilities->frequency & frequency) || !(capabilities_table[i].frequency & frequency))
> > +                continue;
> > +            break;
> > +        }
> > +
> > +        /* skip if nothing is compatible */
> > +        if (i == capabilities_table_elements)
> > +            continue;
> > +
> > +        /* calculate current bitpool range compatible with both remote capabilities and capabilities from our table */
> > +        if (capabilities->min_bitpool > capabilities_table[i].min_bitpool) {
> > +            if (capabilities->max_bitpool > capabilities_table[i].max_bitpool)
> > +                current_range = capabilities_table[i].max_bitpool - capabilities->min_bitpool;
> > +            else
> > +                current_range = capabilities->max_bitpool - capabilities->min_bitpool;
> > +        } else {
> > +            if (capabilities->max_bitpool > capabilities_table[i].max_bitpool)
> > +                current_range = capabilities_table[i].max_bitpool - capabilities_table[i].min_bitpool;
> > +            else
> > +                current_range = capabilities->max_bitpool - capabilities_table[i].min_bitpool;
> > +        }
> 
> Why not simply
> 
> current_range = PA_MIN(capabilities->max_bitpool,
> capabilities_table[i].max_bitpool) - PA_MAX(capabilities->min_bitpool,
> capabilities_table[i].min_bitpool)
> 
> > +
> > +        table_range = capabilities_table[i].max_bitpool - capabilities_table[i].min_bitpool;
> > +
> > +        /* use current remote capabilities if its bitpool range is closer to bitpool range in table */
> > +        if (!best_key || abs((int)current_range - (int)(table_range)) < abs((int)best_range - (int)(table_range))) {
> > +            best_range = current_range;
> > +            best_key = key;
> > +        }
> Does that best_key evaluation really make sense? current_range and
> table_range will both be 0 in all
> cases except for the sbc_auto_caps case and in that case there is only one
> possible element in the table.

I'm revisiting this part of code and I figured out, that I was right and
this "best_key" evaluation make sense for "sbc_auto_caps" configuration.

In this code there are two for-loops:

  PA_HASHMAP_FOREACH_KV(key, a2dp_capabilities, capabilities_hashmap, state) {

    for (i = 0; i < capabilities_table_elements; i++) {

    }

    if (...) {
      best_key = ...
    }

  }

outer loop is iterating capabilities_hashmap members which contains list
of "remote" device capabilities. Remote device may provide up to the N
arbitrary entries which are not under pulseaudio control.

inner loop is iterating capabilities_table members, which are pulseaudio
list, for auto profile it is one-member array sbc_auto_caps with range
of bitpools.

So the whole outer loop try to find the best remote capability for
pulseaudio capability sbc_auto_caps[0].

I hope it is clear now.

-- 
Pali Rohár
pali.rohar at gmail.com