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

[Georg Chini]

On 22.01.20 20:15, Pali Rohár wrote:
> Hello! Thank you for review. Comments are below inlined. Remarks about
> return values of API is I guess out of scope of this patch as patch just
> implements API. API is already in pulseaudio git master, so it needs to
> be changed first there if really needed.

Yes, I think it needs to be changed. I agree that this is not part of
this patch and only wanted to remind that there is an open topic.
Therefore the general remark here and no comment in the code.

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

>
>>> +            frequency = freq_table[i].cap;
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>> +    if (!frequency)
>>> +        return NULL;
>>> -    /* There is no preference, just choose random valid entry */
>>> +    /* 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)
> Because it is incorrect :-)
Please give an example where the calculations give different results. To 
me they look identical.
>
> But I think that absolute value of that difference which you wrote
> should be correct.
> I wrote that calculation explicitly as implicit calculation via min/max
> can lot of times be written incorrectly (like your attempt with missing
> abs()).
>
>>> +
>>> +        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. So I would
drop this unnecessary evaluation. If it is needed at some point, we can 
still
add it back.


...

>
>>> +static uint8_t fill_preferred_configuration(const pa_sample_spec *default_sample_spec, const uint8_t *capabilities_buffer, uint8_t capabilities_size, uint8_t config_buffer[MAX_A2DP_CAPS_SIZE]) {
>>> +    a2dp_sbc_t *config = (a2dp_sbc_t *) config_buffer;
>>> +    uint8_t ret;
>>> +
>>> +    ret = fill_preferred_configuration_table(default_sample_spec, capabilities_buffer, capabilities_size, config_buffer, sbc_auto_caps_table, PA_ELEMENTSOF(sbc_auto_caps_table));
>>> +    config->max_bitpool = PA_MIN(default_bitpool(config->frequency, config->channel_mode), config->max_bitpool);
>>> +    config->max_bitpool = PA_MAX(config->max_bitpool, config->min_bitpool);
>>> +
>>> +    return ret;
>>>    }
>>>    static void set_params(struct sbc_info *sbc_info) {
>>> @@ -500,13 +857,17 @@ static size_t reduce_encoder_bitrate(void *codec_info, size_t write_link_mtu) {
>>>        uint8_t bitpool;
>>>        /* Check if bitpool is already at its limit */
>>> -    if (sbc_info->sbc.bitpool <= SBC_BITPOOL_DEC_LIMIT)
>>> -        return 0;
>>> -
>>> -    bitpool = sbc_info->sbc.bitpool - SBC_BITPOOL_DEC_STEP;
>>> -
>>> -    if (bitpool < SBC_BITPOOL_DEC_LIMIT)
>>> -        bitpool = SBC_BITPOOL_DEC_LIMIT;
>>> +    if (sbc_info->mode == SBC_CHANNEL_MODE_MONO || sbc_info->mode == SBC_CHANNEL_MODE_DUAL_CHANNEL) {
>>> +        /* For Mono and Dual Channel modes bitpool value is separete for each channel */
>>> +        bitpool = sbc_info->sbc.bitpool - SBC_SEPARATE_BITPOOL_DEC_STEP;
>>> +        if (bitpool <= SBC_SEPARATE_BITPOOL_DEC_LIMIT)
>> Should it not be "<" instead of "<="? Or are the limits themselves no
>> acceptable values?
>>> +            return 0;
>>> +    } else {
>>> +        /* For Stereo modes bitpool value is combined for both channels */
>>> +        bitpool = sbc_info->sbc.bitpool - SBC_COMBINED_BITPOOL_DEC_STEP;
>>> +        if (bitpool <= SBC_COMBINED_BITPOOL_DEC_LIMIT)
>> Same here.
> Both macros are some minimal values which during my testing generated
> unusable audio output. So maybe somebody propose different values (maybe
> higher?) and therefore +/-1 errors could be there...
So I understand right that the limits themselves are not usable?
>
>>> diff --git a/src/modules/bluetooth/a2dp-codec-util.c b/src/modules/bluetooth/a2dp-codec-util.c
>>> index 521251aea..7b123f7e1 100644
>>> --- a/src/modules/bluetooth/a2dp-codec-util.c
>>> +++ b/src/modules/bluetooth/a2dp-codec-util.c
>>> @@ -26,24 +26,38 @@
>>>    #include "a2dp-codec-util.h"
>>> +extern const pa_a2dp_codec pa_a2dp_codec_sbc_lq;
>>>    extern const pa_a2dp_codec pa_a2dp_codec_faststream;
>>>    extern const pa_a2dp_codec pa_a2dp_codec_faststream_mic;
>>> +extern const pa_a2dp_codec pa_a2dp_codec_sbc_mq;
>>>    extern const pa_a2dp_codec pa_a2dp_codec_sbc;
>>>    #ifdef HAVE_OPENAPTX
>>>    extern const pa_a2dp_codec pa_a2dp_codec_aptx;
>>> +#endif
>>> +extern const pa_a2dp_codec pa_a2dp_codec_sbc_hq;
>> Why have you put this in between? The order of declaration does not
>> matter and for me it looks better if everything related to one codec
>> stays together.
> Just because to have same order as in table pa_a2dp_codecs.
As said, I would prefer per-codec ordering here.
>
>>> +#ifdef HAVE_OPENAPTX
>>>    extern const pa_a2dp_codec pa_a2dp_codec_aptx_hd;
>>>    #endif
>>> +extern const pa_a2dp_codec pa_a2dp_codec_sbc_xq1;
>>> +extern const pa_a2dp_codec pa_a2dp_codec_sbc_xq2;
>>>    /* This is list of supported codecs. Their order is important.
>>>     * Codec with higher index has higher priority. */
>>>    const pa_a2dp_codec *pa_a2dp_codecs[] = {
>>> -    &pa_a2dp_codec_faststream,
>>> -    &pa_a2dp_codec_faststream_mic,
>>> -    &pa_a2dp_codec_sbc,
>>> +    &pa_a2dp_codec_sbc_lq,
>>> +    &pa_a2dp_codec_faststream,        /* Exactly same as SBC-LQ, but could provide lower latency */
>>> +    &pa_a2dp_codec_faststream_mic,    /* Exactly same as FastStream, but with voice backchannel */
>> The two lines above appear to belong to another patch.
> Apparently not. Comment talks about SBC-LQ which was added in this
> patch.
Sorry, was just blind, I overlooked the lines you removed ...