[pulseaudio-discuss] [PATCH v13 06/10] bluetooth: Add A2DP FastStream codec support
Pali Rohár
pali.rohar at gmail.com
Sat Dec 7 15:56:34 UTC 2019
On Saturday 07 December 2019 16:44:20 Georg Chini wrote:
> 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.
https://www.spinics.net/lists/pulse-audio/msg30932.html
> >
> > > > +
> > > > +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?
Yes
> Do you not check that the capabilities
> are valid before filling the configuration?
Advertised remote capabilities are checked that are valid. But remote
side send another set of capabilities when is establishing connection.
And this is validated in this function.
>
> >
> > > > + }
> > > > + }
> > > > +
> > > > + 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;
> > > > +}
> > > > +
>
>
--
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/20191207/61342913/attachment.sig>
More information about the pulseaudio-discuss
mailing list