[pulseaudio-discuss] [PATCH v13 06/10] bluetooth: Add A2DP FastStream codec support

[Georg Chini]

On 07.12.19 18:25, Pali Rohár wrote:
> On Saturday 07 December 2019 18:15:38 Georg Chini wrote:
>> On 07.12.19 16:56, Pali Rohár wrote:
>>> 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 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)) {
>> If this is a "new" set of capabilities, do you not need to check that the
>> frequencies only
>> contain one of 44k1 or 48k?
> No. Capabilities is set of supported features. Config is one specific
> model of capabilities, one exact configuration. I tried to call all
> variables consistency, so it is know if buffer contains either
> "capabilities" or "config".
That's clear, but you said below (cut off in this mail) that the device 
sends a new
set of capabilities that must be checked in this function.

>
> Here, in fill_preferred_configuration function, we get remote
> capabilities (all supported features advertised by remote endpoint) and
> we need create one specific configuration which is compatible with
> advertised capabilities.

That's the point. If the remote device previously falsely announced 44k1 
and 48k
it might do the same thing here and you might end up trying to use 44k1 
which
as you say does not work if both frequencies are announced.

>
>> Otherwise 44k1 could be chosen falsely here if
>> the headset
>> again sends both frequencies.
> Remote side here send all supported frequencies. If there are more
> frequencies we choose just one which we want to use and then send it to
> remote side to inform which we have chosen. We must send exactly one
> frequency to remote side.
>
But wasn't it the case, that if both frequencies are sent, 44k1 does not 
work?