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

[Pali Rohár]

On Sunday 02 February 2020 18:46:24 Georg Chini wrote:
> 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.

Yes, I can change it.

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

Ok, I will drop it for now.

-- 
Pali Rohár
pali.rohar at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20200205/b9f5da3d/attachment.sig>