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.
> >
> > > > + 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.
Now I'm looking at this again and seems it could work.
> >
> > 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.
That is because all currently defined profiles (except auto) have fixed
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?
Limits are usable, but probably are not the best. Bitpool values below
the current limits generates unusable audio. But probably bitpool values
around limits could generate audio which for some cases is too degraded.
> >
> > > > 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.
Ok, this could be changed.
> >
> > > > +#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;
--
Pali Rohár
pali.rohar@xxxxxxxxx
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ pulseaudio-discuss mailing list pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
