[pulseaudio-discuss] [PATCH v13 07/10] bluetooth: Add more variants of SBC codec
Pali Rohár
pali.rohar at gmail.com
Wed Jan 22 19:15:07 UTC 2020
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.
On Tuesday 21 January 2020 16:17:16 Georg Chini wrote:
> Finally managed to review this one. The general remarks concerning the
> return
> values of the API functions apply here as well. Sorry for the delay.
>
> On 06.10.19 19:58, Pali Rohár wrote:
> > Specify configuration for Low, Middle, High and eXtreme Quality of SBC
> > codec. SBC codec in eXtreme Quality has higher quality than aptX.
> >
> > Automatic Quality mode matches configuration of SBC codec which was used
> > before this change. Which means that it accept configuration between Low
> > and High quality.
> >
> > Current SBC code was extended to allow definitions of arbitrary
> > configuration variants of SBC codec parameters.
> >
> > SBC XQ testing is in following article:
> > http://soundexpert.org/articles/-/blogs/audio-quality-of-sbc-xq-bluetooth-audio-codec
> > ---
> > src/modules/bluetooth/a2dp-codec-sbc.c | 737 ++++++++++++++++++++++++++------
> > src/modules/bluetooth/a2dp-codec-util.c | 20 +-
> > 2 files changed, 618 insertions(+), 139 deletions(-)
> >
> > diff --git a/src/modules/bluetooth/a2dp-codec-sbc.c b/src/modules/bluetooth/a2dp-codec-sbc.c
> > index 733c1a9ab..8db3416b9 100644
> > --- a/src/modules/bluetooth/a2dp-codec-sbc.c
> > +++ b/src/modules/bluetooth/a2dp-codec-sbc.c
> > @@ -36,8 +36,71 @@
> > #include "a2dp-codec-api.h"
> > #include "rtp.h"
> > -#define SBC_BITPOOL_DEC_LIMIT 32
> > -#define SBC_BITPOOL_DEC_STEP 5
> > +/* Below are capabilities tables for different qualities. Order of capabilities in tables are from the most preferred to the least preferred. */
> > +
> > +#define FIXED_SBC_CAPS(mode, freq, bitpool) { .channel_mode = (mode), .frequency = (freq), .min_bitpool = (bitpool), .max_bitpool = (bitpool), .allocation_method = SBC_ALLOCATION_LOUDNESS, .subbands = SBC_SUBBANDS_8, .block_length = SBC_BLOCK_LENGTH_16 }
> > +
> > +/* SBC Low Quality, Joint Stereo is same as FastStream's SBC codec configuration, Mono was calculated to match Joint Stereo */
> > +static const a2dp_sbc_t sbc_lq_caps_table[] = {
> > + FIXED_SBC_CAPS(SBC_CHANNEL_MODE_JOINT_STEREO, SBC_SAMPLING_FREQ_44100, 29), /* 195.7 kbps */
> > + FIXED_SBC_CAPS(SBC_CHANNEL_MODE_JOINT_STEREO, SBC_SAMPLING_FREQ_48000, 29), /* 213 kbps */
> > + FIXED_SBC_CAPS(SBC_CHANNEL_MODE_MONO, SBC_SAMPLING_FREQ_44100, 15), /* 104.7 kbps */
> > + FIXED_SBC_CAPS(SBC_CHANNEL_MODE_MONO, SBC_SAMPLING_FREQ_48000, 15), /* 114 kbps */
> > +};
> > +
> > +/* SBC Middle Quality, based on A2DP spec: Recommended sets of SBC parameters */
> > +static const a2dp_sbc_t sbc_mq_caps_table[] = {
> > + FIXED_SBC_CAPS(SBC_CHANNEL_MODE_JOINT_STEREO, SBC_SAMPLING_FREQ_44100, SBC_BITPOOL_MQ_JOINT_STEREO_44100), /* bitpool = 35, 228.8 kbps */
> > + FIXED_SBC_CAPS(SBC_CHANNEL_MODE_JOINT_STEREO, SBC_SAMPLING_FREQ_48000, SBC_BITPOOL_MQ_JOINT_STEREO_48000), /* bitpool = 33, 237 kbps */
> > + FIXED_SBC_CAPS(SBC_CHANNEL_MODE_MONO, SBC_SAMPLING_FREQ_44100, SBC_BITPOOL_MQ_MONO_44100), /* bitpool = 19, 126.8 kbps */
> > + FIXED_SBC_CAPS(SBC_CHANNEL_MODE_MONO, SBC_SAMPLING_FREQ_48000, SBC_BITPOOL_MQ_MONO_48000), /* bitpool = 18, 132 kbps */
> > +};
> > +
> > +/* SBC High Quality, based on A2DP spec: Recommended sets of SBC parameters */
> > +static const a2dp_sbc_t sbc_hq_caps_table[] = {
> > + FIXED_SBC_CAPS(SBC_CHANNEL_MODE_JOINT_STEREO, SBC_SAMPLING_FREQ_44100, SBC_BITPOOL_HQ_JOINT_STEREO_44100), /* bitpool = 53, 328 kbps */
> > + FIXED_SBC_CAPS(SBC_CHANNEL_MODE_JOINT_STEREO, SBC_SAMPLING_FREQ_48000, SBC_BITPOOL_HQ_JOINT_STEREO_48000), /* bitpool = 51, 345 kbps */
> > + FIXED_SBC_CAPS(SBC_CHANNEL_MODE_MONO, SBC_SAMPLING_FREQ_44100, SBC_BITPOOL_HQ_MONO_44100), /* bitpool = 31, 192.9 kbps */
> > + FIXED_SBC_CAPS(SBC_CHANNEL_MODE_MONO, SBC_SAMPLING_FREQ_48000, SBC_BITPOOL_HQ_MONO_48000), /* bitpool = 29, 210 kbps */
> > +};
> > +
> > +/* SBC eXtreme Quality, calculated to minimize wasted bytes and to be below max possible 512 kbps */
> > +static const a2dp_sbc_t sbc_xq1_caps_table[] = {
> > + FIXED_SBC_CAPS(SBC_CHANNEL_MODE_JOINT_STEREO, SBC_SAMPLING_FREQ_44100, 76), /* 454.8 kbps */
> > + FIXED_SBC_CAPS(SBC_CHANNEL_MODE_JOINT_STEREO, SBC_SAMPLING_FREQ_48000, 76), /* 495 kbps */
> > + FIXED_SBC_CAPS(SBC_CHANNEL_MODE_STEREO, SBC_SAMPLING_FREQ_44100, 76), /* 452 kbps */
> > + FIXED_SBC_CAPS(SBC_CHANNEL_MODE_STEREO, SBC_SAMPLING_FREQ_48000, 76), /* 492 kbps */
> > +};
> > +static const a2dp_sbc_t sbc_xq2_caps_table[] = {
> > + FIXED_SBC_CAPS(SBC_CHANNEL_MODE_DUAL_CHANNEL, SBC_SAMPLING_FREQ_44100, 38), /* 452 kbps */
> > + FIXED_SBC_CAPS(SBC_CHANNEL_MODE_DUAL_CHANNEL, SBC_SAMPLING_FREQ_48000, 38), /* 492 kbps */
> > + FIXED_SBC_CAPS(SBC_CHANNEL_MODE_MONO, SBC_SAMPLING_FREQ_44100, 37), /* 226 kbps */
> > + FIXED_SBC_CAPS(SBC_CHANNEL_MODE_MONO, SBC_SAMPLING_FREQ_48000, 37), /* 246 kbps */
> > +};
> > +/* In most cases bluetooth headsets would support only sbc dual channel mode
> > + * for 2 channels as they have limited maximal bitpool value to 53.
> > + * We need to define it in two tables to disallow invalid combination of
> > + * joint stereo with bitpool 38 which is not XQ. */
>
> The comment should be above the definition.
Ok.
> > +
> > +#undef FIXED_SBC_CAPS
> > +
> > +/* SBC Auto Quality, only one row which allow any possible configuration up to common High Quality */
> > +/* We need to ensure that bitrate is below max possible 512 kbps, therefore limit configuration to High Quality */
> > +static const a2dp_sbc_t sbc_auto_caps_table[] = { {
> > + .channel_mode = SBC_CHANNEL_MODE_MONO | SBC_CHANNEL_MODE_DUAL_CHANNEL | SBC_CHANNEL_MODE_STEREO | SBC_CHANNEL_MODE_JOINT_STEREO,
> > + .frequency = SBC_SAMPLING_FREQ_16000 | SBC_SAMPLING_FREQ_32000 | SBC_SAMPLING_FREQ_44100 | SBC_SAMPLING_FREQ_48000,
> > + .allocation_method = SBC_ALLOCATION_SNR | SBC_ALLOCATION_LOUDNESS,
> > + .subbands = SBC_SUBBANDS_4 | SBC_SUBBANDS_8,
> > + .block_length = SBC_BLOCK_LENGTH_4 | SBC_BLOCK_LENGTH_8 | SBC_BLOCK_LENGTH_12 | SBC_BLOCK_LENGTH_16,
> > + .min_bitpool = SBC_MIN_BITPOOL,
> > + .max_bitpool = SBC_BITPOOL_HQ_JOINT_STEREO_44100,
> > +} };
> > +
> > +/* Bitpool limits and steps for reducing bitrate in Auto Quality mode */
> > +#define SBC_SEPARATE_BITPOOL_DEC_LIMIT 10
> > +#define SBC_COMBINED_BITPOOL_DEC_LIMIT 25
> > +#define SBC_SEPARATE_BITPOOL_DEC_STEP 2
> > +#define SBC_COMBINED_BITPOOL_DEC_STEP 4
> > struct sbc_info {
> > sbc_t sbc; /* Codec data */
> > @@ -53,62 +116,237 @@ struct sbc_info {
> > uint8_t max_bitpool;
> > };
> > -static bool can_accept_capabilities(const uint8_t *capabilities_buffer, uint8_t capabilities_size, bool for_encoding) {
> > - const a2dp_sbc_t *capabilities = (const a2dp_sbc_t *) capabilities_buffer;
> > +static bool are_capabilities_compatible(const a2dp_sbc_t *capabilities1, const a2dp_sbc_t *capabilities2) {
> > + if (!(capabilities1->channel_mode & capabilities2->channel_mode))
> > + return false;
> > - if (capabilities_size != sizeof(*capabilities))
> > + if (!(capabilities1->frequency & capabilities2->frequency))
> > return false;
> > - if (!(capabilities->frequency & (SBC_SAMPLING_FREQ_16000 | SBC_SAMPLING_FREQ_32000 | SBC_SAMPLING_FREQ_44100 | SBC_SAMPLING_FREQ_48000)))
> > + if (!(capabilities1->allocation_method & capabilities2->allocation_method))
> > return false;
> > - if (!(capabilities->channel_mode & (SBC_CHANNEL_MODE_MONO | SBC_CHANNEL_MODE_DUAL_CHANNEL | SBC_CHANNEL_MODE_STEREO | SBC_CHANNEL_MODE_JOINT_STEREO)))
> > + if (!(capabilities1->subbands & capabilities2->subbands))
> > return false;
> > - if (!(capabilities->allocation_method & (SBC_ALLOCATION_SNR | SBC_ALLOCATION_LOUDNESS)))
> > + if (!(capabilities1->block_length & capabilities2->block_length))
> > return false;
> > - if (!(capabilities->subbands & (SBC_SUBBANDS_4 | SBC_SUBBANDS_8)))
> > + if (capabilities1->min_bitpool > capabilities2->max_bitpool || capabilities2->min_bitpool > capabilities1->max_bitpool
> > return false;
> > - if (!(capabilities->block_length & (SBC_BLOCK_LENGTH_4 | SBC_BLOCK_LENGTH_8 | SBC_BLOCK_LENGTH_12 | SBC_BLOCK_LENGTH_16)))
> > + if (capabilities1->min_bitpool > capabilities1->max_bitpool || capabilities2->min_bitpool > capabilities2->max_bitpool)
> Why is this check needed?
To ensure validity of capabilities. Invalid capability is not compatible
with any other capability.
> > return false;
> > return true;
> > }
> > -static const char *choose_remote_endpoint(const pa_hashmap *capabilities_hashmap, const pa_sample_spec *default_sample_spec, bool for_encoding) {
> > +static bool can_accept_capabilities(const uint8_t *capabilities_buffer, uint8_t capabilities_size, bool for_encoding) {
> > + const a2dp_sbc_t *capabilities = (const a2dp_sbc_t *) capabilities_buffer;
> > +
> > + if (capabilities_size != sizeof(*capabilities))
> > + return false;
> > +
> > + return are_capabilities_compatible(capabilities, &sbc_auto_caps_table[0]);
> > +}
> > +
> > +static bool can_accept_capabilities_table(const uint8_t *capabilities_buffer, uint8_t capabilities_size, const a2dp_sbc_t capabilities_table[], unsigned capabilities_table_elements) {
> > + const a2dp_sbc_t *capabilities = (const a2dp_sbc_t *) capabilities_buffer;
> > + unsigned i;
> > +
> > + if (!can_accept_capabilities(capabilities_buffer, capabilities_size, false))
> > + return false;
> > +
> > + for (i = 0; i < capabilities_table_elements; i++) {
> > + if (!are_capabilities_compatible(capabilities, &capabilities_table[i]))
> > + continue;
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > +static bool can_accept_capabilities_lq(const uint8_t *capabilities_buffer, uint8_t capabilities_size, bool for_encoding) {
> > + return can_accept_capabilities_table(capabilities_buffer, capabilities_size, sbc_lq_caps_table, PA_ELEMENTSOF(sbc_lq_caps_table));
> > +}
> > +
> > +static bool can_accept_capabilities_mq(const uint8_t *capabilities_buffer, uint8_t capabilities_size, bool for_encoding) {
> > + return can_accept_capabilities_table(capabilities_buffer, capabilities_size, sbc_mq_caps_table, PA_ELEMENTSOF(sbc_mq_caps_table));
> > +}
> > +
> > +static bool can_accept_capabilities_hq(const uint8_t *capabilities_buffer, uint8_t capabilities_size, bool for_encoding) {
> > + return can_accept_capabilities_table(capabilities_buffer, capabilities_size, sbc_hq_caps_table, PA_ELEMENTSOF(sbc_hq_caps_table));
> > +}
> > +
> > +static bool can_accept_capabilities_xq1(const uint8_t *capabilities_buffer, uint8_t capabilities_size, bool for_encoding) {
> > + return can_accept_capabilities_table(capabilities_buffer, capabilities_size, sbc_xq1_caps_table, PA_ELEMENTSOF(sbc_xq1_caps_table));
> I don't understand how this can work (but maybe I am just blind again ...).
> can_accept_capabilities_table()
> calls can_accept_capabilities() which calls are_capabilities_compatible()
> with sbc_auto_caps_table[0] as
> argument. This contains a max_bitpool of 53, while xq1 uses 76.
You are right, this needs to be fixed.
Previously (prior to introduction of SBC-XQ) &sbc_auto_caps_table[0]
contained all possible SBC configurations. So to check if some SBC
capability is valid, one could call are_capabilities_compatible() with
&sbc_auto_caps_table[0] to check it.
Now when we have bitpool above limit in &sbc_auto_caps_table[0], this
check needs to be reworked.
> > +}
> > +
> > +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.
> > + 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 :-)
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.
> > }
> > - return NULL;
> > + return best_key;
> > }
> > -static uint8_t fill_capabilities(uint8_t capabilities_buffer[MAX_A2DP_CAPS_SIZE]) {
> > +static const char *choose_remote_endpoint_lq(const pa_hashmap *capabilities_hashmap, const pa_sample_spec *default_sample_spec, bool for_encoding) {
> > + return choose_remote_endpoint_table(capabilities_hashmap, default_sample_spec, sbc_lq_caps_table, PA_ELEMENTSOF(sbc_lq_caps_table));
> > +}
> > +
> > +static const char *choose_remote_endpoint_mq(const pa_hashmap *capabilities_hashmap, const pa_sample_spec *default_sample_spec, bool for_encoding) {
> > + return choose_remote_endpoint_table(capabilities_hashmap, default_sample_spec, sbc_mq_caps_table, PA_ELEMENTSOF(sbc_mq_caps_table));
> > +}
> > +
> > +static const char *choose_remote_endpoint_hq(const pa_hashmap *capabilities_hashmap, const pa_sample_spec *default_sample_spec, bool for_encoding) {
> > + return choose_remote_endpoint_table(capabilities_hashmap, default_sample_spec, sbc_hq_caps_table, PA_ELEMENTSOF(sbc_hq_caps_table));
> > +}
> > +
> > +static const char *choose_remote_endpoint_xq1(const pa_hashmap *capabilities_hashmap, const pa_sample_spec *default_sample_spec, bool for_encoding) {
> > + return choose_remote_endpoint_table(capabilities_hashmap, default_sample_spec, sbc_xq1_caps_table, PA_ELEMENTSOF(sbc_xq1_caps_table));
> > +}
> > +
> > +static const char *choose_remote_endpoint_xq2(const pa_hashmap *capabilities_hashmap, const pa_sample_spec *default_sample_spec, bool for_encoding) {
> > + return choose_remote_endpoint_table(capabilities_hashmap, default_sample_spec, sbc_xq2_caps_table, PA_ELEMENTSOF(sbc_xq2_caps_table));
> > +}
> > +
> > +static const char *choose_remote_endpoint(const pa_hashmap *capabilities_hashmap, const pa_sample_spec *default_sample_spec, bool for_encoding) {
> > + return choose_remote_endpoint_table(capabilities_hashmap, default_sample_spec, sbc_auto_caps_table, PA_ELEMENTSOF(sbc_auto_caps_table));
> > +}
> > +
> > +static uint8_t fill_capabilities_table(uint8_t capabilities_buffer[MAX_A2DP_CAPS_SIZE], const a2dp_sbc_t capabilities_table[], unsigned capabilities_table_elements) {
> > a2dp_sbc_t *capabilities = (a2dp_sbc_t *) capabilities_buffer;
> > + unsigned i;
> > pa_zero(*capabilities);
> > - capabilities->channel_mode = SBC_CHANNEL_MODE_MONO | SBC_CHANNEL_MODE_DUAL_CHANNEL | SBC_CHANNEL_MODE_STEREO |
> > - SBC_CHANNEL_MODE_JOINT_STEREO;
> > - capabilities->frequency = SBC_SAMPLING_FREQ_16000 | SBC_SAMPLING_FREQ_32000 | SBC_SAMPLING_FREQ_44100 |
> > - SBC_SAMPLING_FREQ_48000;
> > - capabilities->allocation_method = SBC_ALLOCATION_SNR | SBC_ALLOCATION_LOUDNESS;
> > - capabilities->subbands = SBC_SUBBANDS_4 | SBC_SUBBANDS_8;
> > - capabilities->block_length = SBC_BLOCK_LENGTH_4 | SBC_BLOCK_LENGTH_8 | SBC_BLOCK_LENGTH_12 | SBC_BLOCK_LENGTH_16;
> > - capabilities->min_bitpool = SBC_MIN_BITPOOL;
> > - capabilities->max_bitpool = SBC_BITPOOL_HQ_JOINT_STEREO_44100;
> > + capabilities->min_bitpool = 0xFF;
> > + capabilities->max_bitpool = 0x00;
> > +
> > + for (i = 0; i < capabilities_table_elements; i++) {
> > + capabilities->channel_mode |= capabilities_table[i].channel_mode;
> > + capabilities->frequency |= capabilities_table[i].frequency;
> > + capabilities->allocation_method |= capabilities_table[i].allocation_method;
> > + capabilities->subbands |= capabilities_table[i].subbands;
> > + capabilities->block_length |= capabilities_table[i].block_length;
> > + if (capabilities->min_bitpool > capabilities_table[i].min_bitpool)
> > + capabilities->min_bitpool = capabilities_table[i].min_bitpool;
> > + if (capabilities->max_bitpool < capabilities_table[i].max_bitpool)
> > + capabilities->max_bitpool = capabilities_table[i].max_bitpool;
> > + }
> > +
> > + pa_assert(capabilities->min_bitpool != 0xFF);
> > + pa_assert(capabilities->max_bitpool != 0x00);
> The assertions seem unneeded.
I added these assertions to ensure that min and max bitpools were
correctly filled. So if somebody expand/change capabilities_table, it
would be still asserted for correctness.
I'm using asserts in code to describe invariants.
> > return sizeof(*capabilities);
> > }
> > +static uint8_t fill_capabilities_lq(uint8_t capabilities_buffer[MAX_A2DP_CAPS_SIZE]) {
> > + return fill_capabilities_table(capabilities_buffer, sbc_lq_caps_table, PA_ELEMENTSOF(sbc_lq_caps_table));
> > +}
> > +
> > +static uint8_t fill_capabilities_mq(uint8_t capabilities_buffer[MAX_A2DP_CAPS_SIZE]) {
> > + return fill_capabilities_table(capabilities_buffer, sbc_mq_caps_table, PA_ELEMENTSOF(sbc_mq_caps_table));
> > +}
> > +
> > +static uint8_t fill_capabilities_hq(uint8_t capabilities_buffer[MAX_A2DP_CAPS_SIZE]) {
> > + return fill_capabilities_table(capabilities_buffer, sbc_hq_caps_table, PA_ELEMENTSOF(sbc_hq_caps_table));
> > +}
> > +
> > +static uint8_t fill_capabilities_xq1(uint8_t capabilities_buffer[MAX_A2DP_CAPS_SIZE]) {
> > + return fill_capabilities_table(capabilities_buffer, sbc_xq1_caps_table, PA_ELEMENTSOF(sbc_xq1_caps_table));
> > +}
> > +
> > +static uint8_t fill_capabilities_xq2(uint8_t capabilities_buffer[MAX_A2DP_CAPS_SIZE]) {
> > + return fill_capabilities_table(capabilities_buffer, sbc_xq2_caps_table, PA_ELEMENTSOF(sbc_xq2_caps_table));
> > +}
> > +
> > +static uint8_t fill_capabilities(uint8_t capabilities_buffer[MAX_A2DP_CAPS_SIZE]) {
> > + return fill_capabilities_table(capabilities_buffer, sbc_auto_caps_table, PA_ELEMENTSOF(sbc_auto_caps_table));
> > +}
> > +
> > static bool is_configuration_valid(const uint8_t *config_buffer, uint8_t config_size) {
> > const a2dp_sbc_t *config = (const a2dp_sbc_t *) config_buffer;
> > @@ -153,52 +391,76 @@ static bool is_configuration_valid(const uint8_t *config_buffer, uint8_t config_
> > return true;
> > }
> > -static uint8_t default_bitpool(uint8_t freq, uint8_t mode) {
> > - /* These bitpool values were chosen based on the A2DP spec recommendation */
> > - switch (freq) {
> > - case SBC_SAMPLING_FREQ_16000:
> > - case SBC_SAMPLING_FREQ_32000:
> > - switch (mode) {
> > - case SBC_CHANNEL_MODE_MONO:
> > - case SBC_CHANNEL_MODE_DUAL_CHANNEL:
> > - case SBC_CHANNEL_MODE_STEREO:
> > - case SBC_CHANNEL_MODE_JOINT_STEREO:
> > - return SBC_BITPOOL_HQ_JOINT_STEREO_44100;
> > - }
> > - break;
> > +static bool are_configs_compatible(const a2dp_sbc_t *config1, const a2dp_sbc_t *config2) {
> > + if (config1->frequency != config2->frequency)
> > + return false;
> > - case SBC_SAMPLING_FREQ_44100:
> > - switch (mode) {
> > - case SBC_CHANNEL_MODE_MONO:
> > - case SBC_CHANNEL_MODE_DUAL_CHANNEL:
> > - return SBC_BITPOOL_HQ_MONO_44100;
> > + if (config1->channel_mode != config2->channel_mode)
> > + return false;
> > - case SBC_CHANNEL_MODE_STEREO:
> > - case SBC_CHANNEL_MODE_JOINT_STEREO:
> > - return SBC_BITPOOL_HQ_JOINT_STEREO_44100;
> > - }
> > - break;
> > + if (config1->allocation_method != config2->allocation_method)
> > + return false;
> > - case SBC_SAMPLING_FREQ_48000:
> > - switch (mode) {
> > - case SBC_CHANNEL_MODE_MONO:
> > - case SBC_CHANNEL_MODE_DUAL_CHANNEL:
> > - return SBC_BITPOOL_HQ_MONO_48000;
> > + if (config1->subbands != config2->subbands)
> > + return false;
> > - case SBC_CHANNEL_MODE_STEREO:
> > - case SBC_CHANNEL_MODE_JOINT_STEREO:
> > - return SBC_BITPOOL_HQ_JOINT_STEREO_48000;
> > - }
> > - break;
> > + if (config1->block_length != config2->block_length)
> > + return false;
> > +
> > + /* second config must have constant bitpool */
> > + pa_assert(config2->min_bitpool == config2->max_bitpool);
> > +
> > + if (config1->min_bitpool > config2->min_bitpool || config1->max_bitpool < config2->min_bitpool)
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > +static bool is_configuration_valid_table(const uint8_t *config_buffer, uint8_t config_size, const a2dp_sbc_t capabilities_table[], unsigned capabilities_table_elements) {
> > + const a2dp_sbc_t *config;
> > + unsigned i;
> > +
> > + if (!is_configuration_valid(config_buffer, config_size))
> > + return false;
> > +
> > + config = (const a2dp_sbc_t *) config_buffer;
> > +
> > + for (i = 0; i < capabilities_table_elements; i++) {
> > + if (!are_configs_compatible(config, &capabilities_table[i]))
> > + continue;
> > + return true;
> > }
> > - pa_assert_not_reached();
> > + pa_log_error("Some configuration settings are invalid for current quality");
> > + return false;
> > }
> > -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]) {
> > +static bool is_configuration_valid_lq(const uint8_t *config_buffer, uint8_t config_size) {
> > + return is_configuration_valid_table(config_buffer, config_size, sbc_lq_caps_table, PA_ELEMENTSOF(sbc_lq_caps_table));
> > +}
> > +
> > +static bool is_configuration_valid_mq(const uint8_t *config_buffer, uint8_t config_size) {
> > + return is_configuration_valid_table(config_buffer, config_size, sbc_mq_caps_table, PA_ELEMENTSOF(sbc_mq_caps_table));
> > +}
> > +
> > +static bool is_configuration_valid_hq(const uint8_t *config_buffer, uint8_t config_size) {
> > + return is_configuration_valid_table(config_buffer, config_size, sbc_hq_caps_table, PA_ELEMENTSOF(sbc_hq_caps_table));
> > +}
> > +
> > +static bool is_configuration_valid_xq1(const uint8_t *config_buffer, uint8_t config_size) {
> > + return is_configuration_valid_table(config_buffer, config_size, sbc_xq1_caps_table, PA_ELEMENTSOF(sbc_xq1_caps_table));
> > +}
> > +
> > +static bool is_configuration_valid_xq2(const uint8_t *config_buffer, uint8_t config_size) {
> > + return is_configuration_valid_table(config_buffer, config_size, sbc_xq2_caps_table, PA_ELEMENTSOF(sbc_xq2_caps_table));
> > +}
> > +
> > +static uint8_t fill_preferred_configuration_table(const pa_sample_spec *default_sample_spec, const uint8_t *capabilities_buffer, uint8_t capabilities_size, uint8_t config_buffer[MAX_A2DP_CAPS_SIZE], const a2dp_sbc_t capabilities_table[], unsigned capabilities_table_elements) {
> > a2dp_sbc_t *config = (a2dp_sbc_t *) config_buffer;
> > const a2dp_sbc_t *capabilities = (const a2dp_sbc_t *) capabilities_buffer;
> > - int i;
> > + bool is_mono = (default_sample_spec->channels <= 1);
> > + unsigned i;
> > + int j;
> > static const struct {
> > uint32_t rate;
> > @@ -218,96 +480,191 @@ static uint8_t fill_preferred_configuration(const pa_sample_spec *default_sample
> > 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 >= default_sample_spec->rate && (capabilities->frequency & freq_table[i].cap)) {
> > - config->frequency = freq_table[i].cap;
> > - break;
> > + for (j = 0; (unsigned) j < PA_ELEMENTSOF(freq_table); j++) {
> > + if (freq_table[j].rate >= default_sample_spec->rate && (capabilities->frequency & freq_table[j].cap)) {
> > + for (i = 0; i < capabilities_table_elements; i++) {
> > + if (capabilities_table[i].frequency & freq_table[j].cap) {
> > + config->frequency = freq_table[j].cap;
> > + break;
> > + }
> > + }
> > + if (i != capabilities_table_elements)
> > + break;
> > }
> > + }
> > - if ((unsigned) i == PA_ELEMENTSOF(freq_table)) {
> > - for (--i; i >= 0; i--) {
> > - if (capabilities->frequency & freq_table[i].cap) {
> > - config->frequency = freq_table[i].cap;
> > - break;
> > + if ((unsigned) j == PA_ELEMENTSOF(freq_table)) {
> > + for (--j; j >= 0; j--) {
> > + if (capabilities->frequency & freq_table[j].cap) {
> > + for (i = 0; i < capabilities_table_elements; i++) {
> > + if (capabilities_table[i].frequency & freq_table[j].cap) {
> > + config->frequency = freq_table[j].cap;
> > + break;
> > + }
> > + }
> > + if (i != capabilities_table_elements)
> > + break;
> > }
> > }
> > - if (i < 0) {
> > - pa_log_error("Not suitable sample rate");
> > + if (j < 0) {
> > + pa_log_error("No suitable sample rate");
> > return 0;
> > }
> > }
> > - pa_assert((unsigned) i < PA_ELEMENTSOF(freq_table));
> > -
> > - if (default_sample_spec->channels <= 1) {
> > - if (capabilities->channel_mode & SBC_CHANNEL_MODE_MONO)
> > - config->channel_mode = SBC_CHANNEL_MODE_MONO;
> > - else if (capabilities->channel_mode & SBC_CHANNEL_MODE_JOINT_STEREO)
> > - config->channel_mode = SBC_CHANNEL_MODE_JOINT_STEREO;
> > - else if (capabilities->channel_mode & SBC_CHANNEL_MODE_STEREO)
> > - config->channel_mode = SBC_CHANNEL_MODE_STEREO;
> > - else if (capabilities->channel_mode & SBC_CHANNEL_MODE_DUAL_CHANNEL)
> > - config->channel_mode = SBC_CHANNEL_MODE_DUAL_CHANNEL;
> > + pa_assert((unsigned) j < PA_ELEMENTSOF(freq_table));
> > +
> > + for (i = 0; i < capabilities_table_elements; i++) {
> > + if ((capabilities->block_length & SBC_BLOCK_LENGTH_16) && (capabilities_table[i].block_length & SBC_BLOCK_LENGTH_16))
> > + config->block_length = SBC_BLOCK_LENGTH_16;
> > + else if ((capabilities->block_length & SBC_BLOCK_LENGTH_12) && (capabilities_table[i].block_length & SBC_BLOCK_LENGTH_12))
> > + config->block_length = SBC_BLOCK_LENGTH_12;
> > + else if ((capabilities->block_length & SBC_BLOCK_LENGTH_8) && (capabilities_table[i].block_length & SBC_BLOCK_LENGTH_8))
> > + config->block_length = SBC_BLOCK_LENGTH_8;
> > + else if ((capabilities->block_length & SBC_BLOCK_LENGTH_4) && (capabilities_table[i].block_length & SBC_BLOCK_LENGTH_4))
> > + config->block_length = SBC_BLOCK_LENGTH_4;
> > else {
> > - pa_log_error("No supported channel modes");
> > - return 0;
> > + pa_log_debug("No supported block lengths in table %u", i);
> > + continue;
> > }
> > - } else {
> > - if (capabilities->channel_mode & SBC_CHANNEL_MODE_JOINT_STEREO)
> > - config->channel_mode = SBC_CHANNEL_MODE_JOINT_STEREO;
> > - else if (capabilities->channel_mode & SBC_CHANNEL_MODE_STEREO)
> > - config->channel_mode = SBC_CHANNEL_MODE_STEREO;
> > - else if (capabilities->channel_mode & SBC_CHANNEL_MODE_DUAL_CHANNEL)
> > - config->channel_mode = SBC_CHANNEL_MODE_DUAL_CHANNEL;
> > - else if (capabilities->channel_mode & SBC_CHANNEL_MODE_MONO)
> > - config->channel_mode = SBC_CHANNEL_MODE_MONO;
> > +
> > + if ((capabilities->subbands & SBC_SUBBANDS_8) && (capabilities_table[i].subbands & SBC_SUBBANDS_8))
> > + config->subbands = SBC_SUBBANDS_8;
> > + else if ((capabilities->subbands & SBC_SUBBANDS_4) && (capabilities_table[i].subbands & SBC_SUBBANDS_4))
> > + config->subbands = SBC_SUBBANDS_4;
> > else {
> > - pa_log_error("No supported channel modes");
> > - return 0;
> > + pa_log_debug("No supported subbands in table %u", i);
> > + continue;
> > }
> > - }
> > - if (capabilities->block_length & SBC_BLOCK_LENGTH_16)
> > - config->block_length = SBC_BLOCK_LENGTH_16;
> > - else if (capabilities->block_length & SBC_BLOCK_LENGTH_12)
> > - config->block_length = SBC_BLOCK_LENGTH_12;
> > - else if (capabilities->block_length & SBC_BLOCK_LENGTH_8)
> > - config->block_length = SBC_BLOCK_LENGTH_8;
> > - else if (capabilities->block_length & SBC_BLOCK_LENGTH_4)
> > - config->block_length = SBC_BLOCK_LENGTH_4;
> > - else {
> > - pa_log_error("No supported block lengths");
> > - return 0;
> > - }
> > + if ((capabilities->allocation_method & SBC_ALLOCATION_LOUDNESS) && (capabilities_table[i].allocation_method & SBC_ALLOCATION_LOUDNESS))
> > + config->allocation_method = SBC_ALLOCATION_LOUDNESS;
> > + else if ((capabilities->allocation_method & SBC_ALLOCATION_SNR) && (capabilities_table[i].allocation_method & SBC_ALLOCATION_SNR))
> > + config->allocation_method = SBC_ALLOCATION_SNR;
> > + else {
> > + pa_log_debug("No supported allocation method in table %u", i);
> > + continue;
> > + }
> > - if (capabilities->subbands & SBC_SUBBANDS_8)
> > - config->subbands = SBC_SUBBANDS_8;
> > - else if (capabilities->subbands & SBC_SUBBANDS_4)
> > - config->subbands = SBC_SUBBANDS_4;
> > - else {
> > - pa_log_error("No supported subbands");
> > - return 0;
> > + if (is_mono) {
> > + if ((capabilities->channel_mode & SBC_CHANNEL_MODE_MONO) && (capabilities_table[i].channel_mode & SBC_CHANNEL_MODE_MONO))
> > + config->channel_mode = SBC_CHANNEL_MODE_MONO;
> > + else if ((capabilities->channel_mode & SBC_CHANNEL_MODE_JOINT_STEREO) && (capabilities_table[i].channel_mode & SBC_CHANNEL_MODE_JOINT_STEREO))
> > + config->channel_mode = SBC_CHANNEL_MODE_JOINT_STEREO;
> > + else if ((capabilities->channel_mode & SBC_CHANNEL_MODE_STEREO) && (capabilities_table[i].channel_mode & SBC_CHANNEL_MODE_STEREO))
> > + config->channel_mode = SBC_CHANNEL_MODE_STEREO;
> > + else if ((capabilities->channel_mode & SBC_CHANNEL_MODE_DUAL_CHANNEL) && (capabilities_table[i].channel_mode & SBC_CHANNEL_MODE_DUAL_CHANNEL))
> > + config->channel_mode = SBC_CHANNEL_MODE_DUAL_CHANNEL;
> > + else {
> > + pa_log_debug("No supported channel mode in table %u", i);
> > + continue;
> > + }
> > + } else {
> > + if ((capabilities->channel_mode & SBC_CHANNEL_MODE_JOINT_STEREO) && (capabilities_table[i].channel_mode & SBC_CHANNEL_MODE_JOINT_STEREO))
> > + config->channel_mode = SBC_CHANNEL_MODE_JOINT_STEREO;
> > + else if ((capabilities->channel_mode & SBC_CHANNEL_MODE_STEREO) && (capabilities_table[i].channel_mode & SBC_CHANNEL_MODE_STEREO))
> > + config->channel_mode = SBC_CHANNEL_MODE_STEREO;
> > + else if ((capabilities->channel_mode & SBC_CHANNEL_MODE_DUAL_CHANNEL) && (capabilities_table[i].channel_mode & SBC_CHANNEL_MODE_DUAL_CHANNEL))
> > + config->channel_mode = SBC_CHANNEL_MODE_DUAL_CHANNEL;
> > + else if ((capabilities->channel_mode & SBC_CHANNEL_MODE_MONO) && (capabilities_table[i].channel_mode & SBC_CHANNEL_MODE_MONO))
> > + config->channel_mode = SBC_CHANNEL_MODE_MONO;
> > + else {
> > + pa_log_debug("No supported channel mode in table %u", i);
> > + continue;
> > + }
> > + }
> > +
> > + config->min_bitpool = PA_MAX(capabilities->min_bitpool, capabilities_table[i].min_bitpool);
> > + config->max_bitpool = PA_MIN(capabilities->max_bitpool, capabilities_table[i].max_bitpool);
> > +
> > + if (config->min_bitpool > config->max_bitpool) {
> > + pa_log_debug("No supported bitpool in table %u [%u, %u], need [%u, %u]", i, capabilities_table[i].min_bitpool, capabilities_table[i].max_bitpool, capabilities->min_bitpool,
> Better "No supported ... in table entry %u" because the number is not a
> table number.
> This is also valid for the other similar log messages above.
No problem, I can change error/log/debug messages to anything if it
better fits into pulseaudio log output.
> > capabilities->max_bitpool);
> > + continue;
> > + }
> > +
> > + break;
> > }
> > - if (capabilities->allocation_method & SBC_ALLOCATION_LOUDNESS)
> > - config->allocation_method = SBC_ALLOCATION_LOUDNESS;
> > - else if (capabilities->allocation_method & SBC_ALLOCATION_SNR)
> > - config->allocation_method = SBC_ALLOCATION_SNR;
> > - else {
> > - pa_log_error("No supported allocation method");
> > + if (i == capabilities_table_elements) {
> > + pa_log_error("No supported configuration");
> > return 0;
> > }
> > - config->min_bitpool = (uint8_t) PA_MAX(SBC_MIN_BITPOOL, capabilities->min_bitpool);
> > - config->max_bitpool = (uint8_t) PA_MIN(default_bitpool(config->frequency, config->channel_mode), capabilities->max_bitpool);
> > + return sizeof(*config);
> > +}
> > - if (config->min_bitpool > config->max_bitpool) {
> > - pa_log_error("No supported bitpool");
> > - return 0;
> > +static uint8_t fill_preferred_configuration_lq(const pa_sample_spec *default_sample_spec, const uint8_t *capabilities_buffer, uint8_t capabilities_size, uint8_t config_buffer[MAX_A2DP_CAPS_SIZE]) {
> > + return fill_preferred_configuration_table(default_sample_spec, capabilities_buffer, capabilities_size, config_buffer, sbc_lq_caps_table, PA_ELEMENTSOF(sbc_lq_caps_table));
> > +}
> > +
> > +static uint8_t fill_preferred_configuration_mq(const pa_sample_spec *default_sample_spec, const uint8_t *capabilities_buffer, uint8_t capabilities_size, uint8_t config_buffer[MAX_A2DP_CAPS_SIZE]) {
> > + return fill_preferred_configuration_table(default_sample_spec, capabilities_buffer, capabilities_size, config_buffer, sbc_mq_caps_table, PA_ELEMENTSOF(sbc_mq_caps_table));
> > +}
> > +
> > +static uint8_t fill_preferred_configuration_hq(const pa_sample_spec *default_sample_spec, const uint8_t *capabilities_buffer, uint8_t capabilities_size, uint8_t config_buffer[MAX_A2DP_CAPS_SIZE]) {
> > + return fill_preferred_configuration_table(default_sample_spec, capabilities_buffer, capabilities_size, config_buffer, sbc_hq_caps_table, PA_ELEMENTSOF(sbc_hq_caps_table));
> > +}
> > +
> > +static uint8_t fill_preferred_configuration_xq1(const pa_sample_spec *default_sample_spec, const uint8_t *capabilities_buffer, uint8_t capabilities_size, uint8_t config_buffer[MAX_A2DP_CAPS_SIZE]) {
> > + return fill_preferred_configuration_table(default_sample_spec, capabilities_buffer, capabilities_size, config_buffer, sbc_xq1_caps_table, PA_ELEMENTSOF(sbc_xq1_caps_table));
> > +}
> > +
> > +static uint8_t fill_preferred_configuration_xq2(const pa_sample_spec *default_sample_spec, const uint8_t *capabilities_buffer, uint8_t capabilities_size, uint8_t config_buffer[MAX_A2DP_CAPS_SIZE]) {
> > + return fill_preferred_configuration_table(default_sample_spec, capabilities_buffer, capabilities_size, config_buffer, sbc_xq2_caps_table, PA_ELEMENTSOF(sbc_xq2_caps_table));
> > +}
> > +
> > +static uint8_t default_bitpool(uint8_t freq, uint8_t mode) {
> > + /* These bitpool values were chosen based on the A2DP spec recommendation */
> > + switch (freq) {
> > + case SBC_SAMPLING_FREQ_16000:
> > + case SBC_SAMPLING_FREQ_32000:
> > + switch (mode) {
> > + case SBC_CHANNEL_MODE_MONO:
> > + case SBC_CHANNEL_MODE_DUAL_CHANNEL:
> > + case SBC_CHANNEL_MODE_STEREO:
> > + case SBC_CHANNEL_MODE_JOINT_STEREO:
> > + return SBC_BITPOOL_HQ_JOINT_STEREO_44100;
> > + }
> > + break;
> > +
> > + case SBC_SAMPLING_FREQ_44100:
> > + switch (mode) {
> > + case SBC_CHANNEL_MODE_MONO:
> > + case SBC_CHANNEL_MODE_DUAL_CHANNEL:
> > + return SBC_BITPOOL_HQ_MONO_44100;
> > +
> > + case SBC_CHANNEL_MODE_STEREO:
> > + case SBC_CHANNEL_MODE_JOINT_STEREO:
> > + return SBC_BITPOOL_HQ_JOINT_STEREO_44100;
> > + }
> > + break;
> > +
> > + case SBC_SAMPLING_FREQ_48000:
> > + switch (mode) {
> > + case SBC_CHANNEL_MODE_MONO:
> > + case SBC_CHANNEL_MODE_DUAL_CHANNEL:
> > + return SBC_BITPOOL_HQ_MONO_48000;
> > +
> > + case SBC_CHANNEL_MODE_STEREO:
> > + case SBC_CHANNEL_MODE_JOINT_STEREO:
> > + return SBC_BITPOOL_HQ_JOINT_STEREO_48000;
> > + }
> > + break;
> > }
> > - return sizeof(*config);
> > + pa_assert_not_reached();
> > +}
> > +
> > +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...
> > + return 0;
> > + }
> > if (sbc_info->sbc.bitpool == bitpool)
> > return 0;
> > @@ -515,6 +876,10 @@ static size_t reduce_encoder_bitrate(void *codec_info, size_t write_link_mtu) {
> > return get_block_size(codec_info, write_link_mtu);
> > }
> > +static size_t reduce_encoder_bitrate_none(void *codec_info, size_t write_link_mtu) {
> > + return 0;
> > +}
> > +
> > static size_t encode_buffer(void *codec_info, uint32_t timestamp, const uint8_t *input_buffer, size_t input_size, uint8_t *output_buffer, size_t output_size, size_t *processed) {
> > struct sbc_info *sbc_info = (struct sbc_info *) codec_info;
> > struct rtp_header *header;
> > @@ -662,9 +1027,69 @@ static size_t decode_buffer(void *codec_info, uint32_t *timestamp, const uint8_t
> > return d - output_buffer;
> > }
> > +const pa_a2dp_codec pa_a2dp_codec_sbc_lq = {
> > + .name = "sbc_lq",
> > + .description = "SBC (Low Quality)",
> > + .id = { A2DP_CODEC_SBC, 0, 0 },
> > + .support_backchannel = false,
> > + .can_accept_capabilities = can_accept_capabilities_lq,
> > + .choose_remote_endpoint = choose_remote_endpoint_lq,
> > + .fill_capabilities = fill_capabilities_lq,
> > + .is_configuration_valid = is_configuration_valid_lq,
> > + .fill_preferred_configuration = fill_preferred_configuration_lq,
> > + .init = init,
> > + .deinit = deinit,
> > + .reset = reset,
> > + .get_read_block_size = get_block_size,
> > + .get_write_block_size = get_block_size,
> > + .reduce_encoder_bitrate = reduce_encoder_bitrate_none,
> > + .encode_buffer = encode_buffer,
> > + .decode_buffer = decode_buffer,
> > +};
> > +
> > +const pa_a2dp_codec pa_a2dp_codec_sbc_mq = {
> > + .name = "sbc_mq",
> > + .description = "SBC (Middle Quality)",
> > + .id = { A2DP_CODEC_SBC, 0, 0 },
> > + .support_backchannel = false,
> > + .can_accept_capabilities = can_accept_capabilities_mq,
> > + .choose_remote_endpoint = choose_remote_endpoint_mq,
> > + .fill_capabilities = fill_capabilities_mq,
> > + .is_configuration_valid = is_configuration_valid_mq,
> > + .fill_preferred_configuration = fill_preferred_configuration_mq,
> > + .init = init,
> > + .deinit = deinit,
> > + .reset = reset,
> > + .get_read_block_size = get_block_size,
> > + .get_write_block_size = get_block_size,
> > + .reduce_encoder_bitrate = reduce_encoder_bitrate_none,
> > + .encode_buffer = encode_buffer,
> > + .decode_buffer = decode_buffer,
> > +};
> > +
> > +const pa_a2dp_codec pa_a2dp_codec_sbc_hq = {
> > + .name = "sbc_hq",
> > + .description = "SBC (High Quality)",
> > + .id = { A2DP_CODEC_SBC, 0, 0 },
> > + .support_backchannel = false,
> > + .can_accept_capabilities = can_accept_capabilities_hq,
> > + .choose_remote_endpoint = choose_remote_endpoint_hq,
> > + .fill_capabilities = fill_capabilities_hq,
> > + .is_configuration_valid = is_configuration_valid_hq,
> > + .fill_preferred_configuration = fill_preferred_configuration_hq,
> > + .init = init,
> > + .deinit = deinit,
> > + .reset = reset,
> > + .get_read_block_size = get_block_size,
> > + .get_write_block_size = get_block_size,
> > + .reduce_encoder_bitrate = reduce_encoder_bitrate_none,
> > + .encode_buffer = encode_buffer,
> > + .decode_buffer = decode_buffer,
> > +};
> > +
> > const pa_a2dp_codec pa_a2dp_codec_sbc = {
> > .name = "sbc",
> > - .description = "SBC",
> > + .description = "SBC (Automatic Quality)",
> > .id = { A2DP_CODEC_SBC, 0, 0 },
> > .support_backchannel = false,
> > .can_accept_capabilities = can_accept_capabilities,
> > @@ -681,3 +1106,43 @@ const pa_a2dp_codec pa_a2dp_codec_sbc = {
> > .encode_buffer = encode_buffer,
> > .decode_buffer = decode_buffer,
> > };
> > +
> > +const pa_a2dp_codec pa_a2dp_codec_sbc_xq1 = {
> > + .name = "sbc_xq1",
> > + .description = "SBC (eXtreme Quality profile 1)",
> > + .id = { A2DP_CODEC_SBC, 0, 0 },
> > + .support_backchannel = false,
> > + .can_accept_capabilities = can_accept_capabilities_xq1,
> > + .choose_remote_endpoint = choose_remote_endpoint_xq1,
> > + .fill_capabilities = fill_capabilities_xq1,
> > + .is_configuration_valid = is_configuration_valid_xq1,
> > + .fill_preferred_configuration = fill_preferred_configuration_xq1,
> > + .init = init,
> > + .deinit = deinit,
> > + .reset = reset,
> > + .get_read_block_size = get_block_size,
> > + .get_write_block_size = get_block_size,
> > + .reduce_encoder_bitrate = reduce_encoder_bitrate_none,
> > + .encode_buffer = encode_buffer,
> > + .decode_buffer = decode_buffer,
> > +};
> > +
> > +const pa_a2dp_codec pa_a2dp_codec_sbc_xq2 = {
> > + .name = "sbc_xq2",
> > + .description = "SBC (eXtreme Quality profile 2)",
> > + .id = { A2DP_CODEC_SBC, 0, 0 },
> > + .support_backchannel = false,
> > + .can_accept_capabilities = can_accept_capabilities_xq2,
> > + .choose_remote_endpoint = choose_remote_endpoint_xq2,
> > + .fill_capabilities = fill_capabilities_xq2,
> > + .is_configuration_valid = is_configuration_valid_xq2,
> > + .fill_preferred_configuration = fill_preferred_configuration_xq2,
> > + .init = init,
> > + .deinit = deinit,
> > + .reset = reset,
> > + .get_read_block_size = get_block_size,
> > + .get_write_block_size = get_block_size,
> > + .reduce_encoder_bitrate = reduce_encoder_bitrate_none,
> > + .encode_buffer = encode_buffer,
> > + .decode_buffer = decode_buffer,
> > +};
> > 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.
> > +#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.
> > + &pa_a2dp_codec_sbc_mq,
> > + &pa_a2dp_codec_sbc, /* SBC in automatic mode, from SBC-LQ to SBC-HQ; not SBC-XQ */
> > #ifdef HAVE_OPENAPTX
> > &pa_a2dp_codec_aptx,
> > +#endif
> > + &pa_a2dp_codec_sbc_hq, /* SBC-HQ has similar quality as aptX */
> > +#ifdef HAVE_OPENAPTX
> > &pa_a2dp_codec_aptx_hd,
> > #endif
> > + &pa_a2dp_codec_sbc_xq1, /* SBC-XQ has similar quality as aptX-HD */
> > + &pa_a2dp_codec_sbc_xq2, /* SBC-XQ has similar quality as aptX-HD */
> > };
> > unsigned int pa_bluetooth_a2dp_codec_count(void) {
>
>
--
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/20200122/f88c3f3b/attachment-0001.sig>
More information about the pulseaudio-discuss
mailing list