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

Mon Apr 22 08:01:01 UTC 2019

On Sat, 2019-04-20 at 11:24 +0200, Pali Rohár wrote:
> 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.

Ok, so this is a firmware bug. That should be documented as such, and I
think it would be best to do the firmware bug fixup as a separate step,
like this:

static bool is_configuration_valid(...) {
    uint8_t sink_frequency;

    ...

    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;
    }

    ...
}

By the way, your original error message is "Invalid sampling sink
frequency in configuration" and the same for source. The word ordering
needs changing: it's "sink sampling frequency", not "sampling sink
frequency".

> > > +        
> > > +        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.

We only need to deal with known firmware bugs, we shouldn't deviate
from the specification (or the assumed specification - if I've
understood correctly, the FastStream spec isn't available) due to
speculation that other bugs might exist in some implementations. Even
if similar bugs exist also with the backchannel frequencies, we can't
assume without testing that the device wants 16 kHz audio rather than
whatever other frequency it ended up putting in the config.

-- 
Tanu

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