[pulseaudio-discuss] [PATCH v13 06/10] bluetooth: Add A2DP FastStream codec support
Georg Chini
georg at chini.tk
Sat Dec 7 15:44:20 UTC 2019
On 07.12.19 15:47, Pali Rohár wrote:
> On Saturday 07 December 2019 15:37:15 Georg Chini wrote:
>> On 06.10.19 19:58, Pali Rohár wrote:
>>> This patch provides support for FastStream codec in bluetooth A2DP profile.
>>> FastStream codec is bi-directional, which means that it supports both music
>>> playback and microphone voice at the same time.
>>>
>>> FastStream codec is just SBC codec with fixed parameters. For playback are
>>> used following parameters: 48.0kHz or 44.1kHz, Blocks 16, Sub-bands 8,
>>> Joint Stereo, Loudness, Bitpool = 29 (data rate = 212kbps, packet size =
>>> (71+1)*3 <= DM5 = 220, with 3 SBC frames). SBC frame size is 71 bytes, but
>>> FastStream is zero-padded to the even size (72). For microphone are used
>>> following SBC parameters: 16kHz, Mono, Blocks 16, Sub-bands 8, Loudness,
>>> Bitpool = 32 (data rate = 72kbps, packet size = 72*3 <= DM5 = 220, with
>>> 3 SBC frames).
>>>
>>> So FastStream codec is equivalent to SBC in Low Quality settings. But the
>>> main benefit of FastStream codec is support for microphone voice channel
>>> for audio calls. Compared to bluetooth HSP profile (with CVSD codec), it
>>> provides better audio quality for both playback and recording.
>>> ---
>>> src/Makefile.am | 2 +
>>> src/modules/bluetooth/a2dp-codec-faststream.c | 554 ++++++++++++++++++++++++++
>>> src/modules/bluetooth/a2dp-codec-util.c | 4 +
>>> src/modules/bluetooth/meson.build | 1 +
>>> 4 files changed, 561 insertions(+)
>>> create mode 100644 src/modules/bluetooth/a2dp-codec-faststream.c
>>>
>> ...
>>> +static bool is_configuration_valid_common(const a2dp_faststream_t *config, uint8_t config_size) {
>>> + uint8_t sink_frequency;
>>> +
>>> + if (config_size != sizeof(*config)) {
>>> + pa_log_error("Invalid size of config buffer");
>>> + return false;
>>> + }
>>> +
>>> + if (A2DP_GET_VENDOR_ID(config->info) != FASTSTREAM_VENDOR_ID || A2DP_GET_CODEC_ID(config->info) != FASTSTREAM_CODEC_ID) {
>>> + pa_log_error("Invalid vendor codec information in configuration");
>>> + return false;
>>> + }
>>> +
>>> + if (!(config->direction & FASTSTREAM_DIRECTION_SINK)) {
>>> + pa_log_error("Invalid direction in configuration");
>>> + return false;
>>> + }
>>> +
>>> + sink_frequency = config->sink_frequency;
>>> +
>>> + /* Some headsets are buggy and set both 48 kHz and 44.1 kHz in
>>> + * the config. In such situation trying to send audio at 44.1 kHz
>>> + * results in choppy audio, so we have to assume that the headset
>>> + * actually wants 48 kHz audio. */
>>> + if (sink_frequency == (FASTSTREAM_SINK_SAMPLING_FREQ_44100 | FASTSTREAM_SINK_SAMPLING_FREQ_48000))
>>> + sink_frequency = FASTSTREAM_SINK_SAMPLING_FREQ_48000;
>>> +
>>> + if (sink_frequency != FASTSTREAM_SINK_SAMPLING_FREQ_44100 && sink_frequency != FASTSTREAM_SINK_SAMPLING_FREQ_48000) {
>>> + pa_log_error("Invalid sink sampling frequency in configuration");
>>> + return false;
>>> + }
>>> +
>>> + return true;
>>> +}
>> Why not simply
>>
>> if (config->sink_frequency & (FASTSTREAM_SINK_SAMPLING_FREQ_44100 | FASTSTREAM_SINK_SAMPLING_FREQ_48000))
>> return true;
>>
>> return false;
> This change suggested Tanu in email Message-ID: <4622e48b2c79eaab2486cdc43a5fdf430859345e.camel at iki.fi>
Can't find the e-mail with that reference.
>
>>> +
>>> +static bool is_configuration_valid(const uint8_t *config_buffer, uint8_t config_size) {
>>> + const a2dp_faststream_t *config = (const a2dp_faststream_t *) config_buffer;
>>> +
>>> + if (!is_configuration_valid_common(config, config_size))
>>> + return false;
>>> +
>>> + if (config->direction & FASTSTREAM_DIRECTION_SOURCE) {
>>> + pa_log_error("Invalid direction in configuration");
>>> + return false;
>>> + }
>>> +
>>> + return true;
>>> +}
>>> +
>>> +static bool is_configuration_valid_mic(const uint8_t *config_buffer, uint8_t config_size) {
>>> + const a2dp_faststream_t *config = (const a2dp_faststream_t *) config_buffer;
>>> +
>>> + if (!is_configuration_valid_common(config, config_size))
>>> + return false;
>>> +
>>> + if (!(config->direction & FASTSTREAM_DIRECTION_SOURCE)) {
>>> + pa_log_error("Invalid direction in configuration");
>>> + return false;
>>> + }
>>> +
>>> + if (config->source_frequency != FASTSTREAM_SOURCE_SAMPLING_FREQ_16000) {
>>> + pa_log_error("Invalid source sampling frequency in configuration");
>>> + return false;
>>> + }
>>> +
>>> + return true;
>>> +}
>>> +
>>> +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_faststream_t *config = (a2dp_faststream_t *) config_buffer;
>>> + const a2dp_faststream_t *capabilities = (const a2dp_faststream_t *) capabilities_buffer;
>>> + int i;
>>> +
>>> + static const struct {
>>> + uint32_t rate;
>>> + uint8_t cap;
>>> + } freq_table[] = {
>>> + { 44100U, FASTSTREAM_SINK_SAMPLING_FREQ_44100 },
>>> + { 48000U, FASTSTREAM_SINK_SAMPLING_FREQ_48000 }
>>> + };
>>> +
>>> + if (capabilities_size != sizeof(*capabilities)) {
>>> + pa_log_error("Invalid size of capabilities buffer");
>>> + return 0;
>>> + }
>>> +
>>> + pa_zero(*config);
>>> +
>>> + if (A2DP_GET_VENDOR_ID(capabilities->info) != FASTSTREAM_VENDOR_ID || A2DP_GET_CODEC_ID(capabilities->info) != FASTSTREAM_CODEC_ID) {
>>> + pa_log_error("No supported vendor codec information");
>>> + return 0;
>>> + }
>>> +
>>> + config->info = A2DP_SET_VENDOR_ID_CODEC_ID(FASTSTREAM_VENDOR_ID, FASTSTREAM_CODEC_ID);
>>> +
>>> + /* 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->sink_frequency & freq_table[i].cap)) {
>>> + config->sink_frequency = freq_table[i].cap;
>>> + break;
>>> + }
>>> +
>>> + if ((unsigned) i == PA_ELEMENTSOF(freq_table)) {
>> This "if" does not seem necessary, we know that the table has some elements.
> It is necessary. It is fallback when first for loop (above) did not
> choose suitable sample rate. "i" is set to elements count when previous
> for loop did not break.
Yes, sorry. Misread the code.
>
>>> + for (--i; i >= 0; i--) {
>>> + if (capabilities->sink_frequency & freq_table[i].cap) {
>>> + config->sink_frequency = freq_table[i].cap;
>>> + break;
>>> + }
>>> + }
>>> +
>>> + if (i < 0) {
>>> + pa_log_error("Not suitable sample rate");
>>> + return 0;
>> As said in the previous patch, I think an assertion would be OK here.
> This error can happen. E.g. when remote side advertise frequencies
> different then 44.1kHz and 48kHz. We do not want to crash pulseaudio
> (with assertion) when some bluetooth headset either send garbage or send
> list of frequencies which pulseaudio does not support.
Can that really happen? Do you not check that the capabilities
are valid before filling the configuration?
>
>>> + }
>>> + }
>>> +
>>> + pa_assert((unsigned) i < PA_ELEMENTSOF(freq_table));
>>> +
>>> + if (!(capabilities->direction & FASTSTREAM_DIRECTION_SINK)) {
>>> + pa_log_error("No sink support");
>>> + return 0;
>>> + }
>>> +
>>> + config->direction = FASTSTREAM_DIRECTION_SINK;
>>> +
>>> + return sizeof(*config);
>>> +}
>>> +
>> ...
>>> +
>>> +static void *init(bool for_encoding, bool for_backchannel, const uint8_t *config_buffer, uint8_t config_size, pa_sample_spec *sample_spec) {
>>> + struct faststream_info *faststream_info;
>>> + const a2dp_faststream_t *config = (const a2dp_faststream_t *) config_buffer;
>>> + int ret;
>>> +
>>> + pa_assert(config_size == sizeof(*config));
>>> +
>>> + faststream_info = pa_xnew0(struct faststream_info, 1);
>>> + faststream_info->is_microphone = for_backchannel;
>>> +
>>> + ret = sbc_init(&faststream_info->sbc, 0);
>>> + if (ret != 0) {
>>> + pa_xfree(faststream_info);
>>> + pa_log_error("SBC initialization failed: %d", ret);
>>> + return NULL;
>>> + }
>>> +
>>> + sample_spec->format = PA_SAMPLE_S16LE;
>>> +
>>> + if (faststream_info->is_microphone) {
>>> + if (config->source_frequency == FASTSTREAM_SOURCE_SAMPLING_FREQ_16000) {
>>> + faststream_info->frequency = SBC_FREQ_16000;
>>> + sample_spec->rate = 16000U;
>>> + } else {
>>> + pa_assert_not_reached();
>>> + }
>>> +
>>> + sample_spec->channels = 1;
>>> + } else {
>>> + uint8_t sink_frequency = config->sink_frequency;
>>> +
>>> + /* Some headsets are buggy and set both 48 kHz and 44.1 kHz in
>>> + * the config. In such situation trying to send audio at 44.1 kHz
>>> + * results in choppy audio, so we have to assume that the headset
>>> + * actually wants 48 kHz audio. */
>>> + if (sink_frequency == (FASTSTREAM_SINK_SAMPLING_FREQ_44100 | FASTSTREAM_SINK_SAMPLING_FREQ_48000))
>>> + sink_frequency = FASTSTREAM_SINK_SAMPLING_FREQ_48000;
>>> +
>>> + if (sink_frequency == FASTSTREAM_SINK_SAMPLING_FREQ_48000) {
>> Why not simply
>>
>> if (config->sink_frequency & FASTSTREAM_SINK_SAMPLING_FREQ_48000) {
>>
>> This would take care of the case where both bits are set, because 48k is
>> preferred.
> See Message-ID: <4622e48b2c79eaab2486cdc43a5fdf430859345e.camel at iki.fi>
As said, can't find it.
>
>>> + faststream_info->frequency = SBC_FREQ_48000;
>>> + sample_spec->rate = 48000U;
>>> + } else if (config->sink_frequency == FASTSTREAM_SINK_SAMPLING_FREQ_44100) {
>> and
>>
>> } else if (config->sink_frequency & FASTSTREAM_SINK_SAMPLING_FREQ_44100) {
>>
>>> + faststream_info->frequency = SBC_FREQ_44100;
>>> + sample_spec->rate = 44100U;
>>> + } else {
>>> + pa_assert_not_reached();
>>> + }
>>> +
>>> + sample_spec->channels = 2;
>>> + }
>>> +
>>> + set_params(faststream_info);
>>> +
>>> + pa_log_info("SBC parameters: allocation=%s, subbands=%u, blocks=%u, mode=%s bitpool=%u codesize=%u frame_length=%u",
>>> + faststream_info->sbc.allocation ? "SNR" : "Loudness", faststream_info->sbc.subbands ? 8 : 4,
>>> + (faststream_info->sbc.blocks+1)*4, faststream_info->sbc.mode == SBC_MODE_MONO ? "Mono" :
>>> + faststream_info->sbc.mode == SBC_MODE_DUAL_CHANNEL ? "DualChannel" :
>>> + faststream_info->sbc.mode == SBC_MODE_STEREO ? "Stereo" : "JointStereo",
>>> + faststream_info->sbc.bitpool, (unsigned)faststream_info->codesize, (unsigned)faststream_info->frame_length);
>>> +
>>> + return faststream_info;
>>> +}
>>> +
More information about the pulseaudio-discuss
mailing list