[pulseaudio-discuss] [PATCH v8 08/12] bluetooth: Add A2DP FastStream codec support

[Tanu Kaskinen]

On Sat, 2019-04-06 at 11:16 +0200, Pali Rohár wrote:
> This patch provides support for FastStream codec in bluetooth A2DP profile.
> FastStream codec is bi-directional, which means that support 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+4 = 220 = DM5). SBC frame size is 71 bytes, but padded with one
> zero byte due to rounding to 72 bytes. For microphone are used following
> SBC parameters: 16kHz, Mono, Blocks 16, Sub-bands 8, Loudness, Bitpool = 32
> (data rate = 72kbps, packet size = 3*72 + 4 = 220 <= DM5).
> 
> So FastStream codec is slightly equivalent to SBC Low Quality settings
> (which uses bitpool value 30). 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 | 436 ++++++++++++++++++++++++++
>  src/modules/bluetooth/a2dp-codec-util.c       |   2 +
>  3 files changed, 440 insertions(+)
>  create mode 100644 src/modules/bluetooth/a2dp-codec-faststream.c

> +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 (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) || !(config->direction & FASTSTREAM_DIRECTION_SOURCE)) {
> +        pa_log_error("Invalid direction in configuration");
> +        return false;
> +    }
> +
> +    /* Remote endpoint indicates list of frequences, not just one frequency */

Typo: "frequences" -> "frequencies".

> +    if ((config->sink_frequency & ~(FASTSTREAM_SINK_SAMPLING_FREQ_44100|FASTSTREAM_SINK_SAMPLING_FREQ_48000)) ||
> +       (!(config->sink_frequency & (FASTSTREAM_SINK_SAMPLING_FREQ_44100|FASTSTREAM_SINK_SAMPLING_FREQ_48000)))) {

Do we really need to be this strict? The final sample rate is decided
by us, so isn't it enough if the config contains just one of the rates
that we support? If the sink supports some other rates, we don't need
to care about that.

> +        pa_log_error("Invalid sampling sink frequency in configuration");
> +        return false;
> +    }
> +
> +    /* Remote endpoint indicates list of frequences, not just one frequency */

Typo: "frequences" -> "frequencies".

Is this comment really true for sources? I can understand that the
device can list multiple sink frequencies if it doesn't care what we
send, but is the device really allowed to be imprecise about what kind
of audio it will send through the backchannel?

> +    if (config->source_frequency != FASTSTREAM_SOURCE_SAMPLING_FREQ_16000) {
> +        pa_log_error("Invalid sampling source frequency in configuration");
> +        return false;
> +    }
> +
> +    return true;
> +}


> +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_backchannel = 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_backchannel) {
> +        /* config buffer contains list of frequences, not just one frequency */

As with the earlier comment on this topic, it seems unlikely to me that
there can be multiple source frequencies listed. If there can, then how
do we communicate to the device that we expect 16kHz?

> +        if (config->source_frequency & FASTSTREAM_SOURCE_SAMPLING_FREQ_16000) {
> +            faststream_info->frequency = SBC_FREQ_16000;
> +            sample_spec->rate = 16000U;
> +        } else {
> +            pa_assert_not_reached();
> +        }


> +static size_t decode_buffer(void *codec_info, const uint8_t *input_buffer, size_t input_size, uint8_t *output_buffer, size_t output_size, size_t *processed) {
> +    struct faststream_info *faststream_info = (struct faststream_info *) codec_info;
> +
> +    const uint8_t *p;
> +    uint8_t *d;
> +    size_t to_write, to_decode;
> +
> +    p = input_buffer;
> +    to_decode = input_size;
> +
> +    d = output_buffer;
> +    to_write = output_size;
> +
> +    while (PA_LIKELY(to_decode > 0)) {
> +        size_t written;
> +        ssize_t decoded;
> +
> +        decoded = sbc_decode(&faststream_info->sbc,
> +                             p, to_decode,
> +                             d, to_write,
> +                             &written);
> +
> +        if (PA_UNLIKELY(decoded <= 0)) {
> +            pa_log_error("SBC decoding error (%li)", (long) decoded);
> +            *processed = p - input_buffer;
> +            return 0;
> +        }
> +
> +        pa_assert_fp((size_t) decoded <= to_decode);
> +
> +        if (faststream_info->is_backchannel)
> +            pa_assert_fp((size_t) decoded == faststream_info->frame_length);
> +        else
> +            pa_assert_fp((size_t) decoded == faststream_info->frame_length-1);

In set_params() frame_length is set to 71, so you shouldn't do the
subtraction here.

-- 
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk