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

[Georg Chini]

On 02.02.20 14:08, Pali Rohár wrote:
> On Thursday 23 January 2020 12:29:15 Georg Chini wrote:
>>> On Tuesday 21 January 2020 16:17:16 Georg Chini wrote:
>> ...
>>>>> +}
>>>>> +
>>>>> +static bool can_accept_capabilities_xq2(const uint8_t *capabilities_buffer, uint8_t capabilities_size, bool for_encoding) {
>>>>> +    return can_accept_capabilities_table(capabilities_buffer, capabilities_size, sbc_xq2_caps_table, PA_ELEMENTSOF(sbc_xq2_caps_table));
>>>>> +}
>>>>> +
>>>>> +static const char *choose_remote_endpoint_table(const pa_hashmap *capabilities_hashmap, const pa_sample_spec *default_sample_spec, const a2dp_sbc_t capabilities_table[], unsigned capabilities_table_elements) {
>>>>>         const pa_a2dp_codec_capabilities *a2dp_capabilities;
>>>>> +    const a2dp_sbc_t *capabilities;
>>>>>         const char *key;
>>>>>         void *state;
>>>>> +    unsigned i;
>>>>> +    uint8_t table_range;
>>>>> +    uint8_t current_range;
>>>>> +    const char *best_key = NULL;
>>>>> +    uint8_t best_range = 0;
>>>>> +    uint8_t frequency = 0;
>>>>> +    bool is_mono = (default_sample_spec->channels <= 1);
>>>>> +
>>>>> +    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 }
>>>>> +    };
>>>>> +
>>>>> +    for (i = 0; i < PA_ELEMENTSOF(freq_table); i++) {
>>>>> +        if (freq_table[i].rate == default_sample_spec->rate) {
>>>> Is an exact match necessary here? Or would >= be OK as well?
>>> For fixed SBC profiles we need to check for exact frequency. As there
>>> are specific bitpool values for specific frequency.
>> Sure, in the end we will use one of the specific frequencies.
>> But need the default_sample_spec already contain the exact frequency?
>> Or can we choose the one nearest to default_sample_spec->rate?
> Here we compare SBC freq capability with pulseaudio internal frequency.
> So non-exact nearest frequency could be used too.
  I think you do that for the other codecs as well.
>>>>> +
>>>>> +        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 written this code prior to defining final list of SBC profiles. So
>>> code was prepared to work with any list of SBC definitions.
>>>
>>> Maybe you are right that for currently defined SBC profiles it is not
>>> needed, but I wanted that current code would work with any SBC profile
>>> table definition.
>> I don't think that makes sense. In are_configs_compatible() you rely on the
>> fact that the table entries only contain a single bitpool value.
> That is because all currently defined profiles (except auto) have fixed
> bitpool value.
Do you intend to add any other profile which would require a variable 
bitpool?
If you don't have any use case, please drop it.