On Saturday 20 April 2019 11:37:56 Tanu Kaskinen wrote:
> 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".
Ok, I will fix all occurrences.
> > + 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?
Yes.
> 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.
Normally in A2DP capabilities buffer device announces combination (list)
of supported features and configurations. In A2DP config buffer device
usually says one exact configuration which should be used.
But my testing headphones in A2DP FastStream config buffer sometimes
says that sink frequency is both 44100|48000 -- which does not make
sense. When pulseaudio started sending SBC data with 44.1kHz then sound
was obviously bad, there was periodically quiet period (should be for
1.8 us). When pulseaudio started sending SBC data with 48kHz everything
was OK. So conclusion is that when headphones says that sampling
frequency is both 44100|48000 in config buffer they means it is 48kHz.
In function is_configuration_valid we need to ensure that pulseaudio and
remove device negotiate correctly all parameters, including sampling
frequency and both sides know how to interpret config buffer.
So strictness is needed to ensure that audio is correctly encoded to SBC
codec and then correctly decoded from SBC codec to RAW.
> > + 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?
Based on above observation about sinks I can deduce that it may be truth
also for sources... Currently we support only one frequency (because my
testing faststream headphones does not support more) but in future
somebody may extend support... and should be aware of this "bug".
I think that having more frequencies in config buffer is error and bug
in headphones firmware. But we need to deal with it.
> I can understand that the
> device can list multiple sink frequencies if it doesn't care what we
> send,
Device must always care about sampling frequency and both sides need to
know it. Otherwise audio would be broken by decoder.
> 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?
It could be similar "bug" as for sinks. Always only one is valid.
> > + 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.
Good catch, this is bug. I will fix it. This is reason why proper review
by somebody who is not author of that patch is needed :-)
I have not tested FastStream as receiver as I do not have any hardware
transmitter device with FastStream support -- they are rare.
--
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
