On Monday 22 April 2019 11:01:01 Tanu Kaskinen wrote:
> 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".
Ok, I will adjust code according to your suggestion.
> > > > +
> > > > + 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.
Ok, no problem. I will remove that part for source frequency. Currently
only 16kHz is supported...
--
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
