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 bool is_configuration_valid_common(const a2dp_faststream_t *config, uint8_t config_size) {
> > > > > > + uint8_t sink_frequency;
> > > > > > +
> > > > > > + 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)) {
> > > > > > + pa_log_error("Invalid direction in configuration");
> > > > > > + return false;
> > > > > > + }
> > > > > > +
> > > > > > + 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;
> > > > > > + }
> > > > > > +
> > > > > > + return true;
> > > > > > +}
> > > > > Why not simply
> > > > >
> > > > > if (config->sink_frequency & (FASTSTREAM_SINK_SAMPLING_FREQ_44100 | FASTSTREAM_SINK_SAMPLING_FREQ_48000))
> > > > > return true;
> > > > >
> > > > > return false;
> > > > This change suggested Tanu in email Message-ID: <4622e48b2c79eaab2486cdc43a5fdf430859345e.camel@xxxxxx>
> > > Can't find the e-mail with that reference.
> > https://www.spinics.net/lists/pulse-audio/msg30932.html
>
> Well, if he says so lets keep it, though I think the comment would be fully
> sufficient.
>
>
> >
> > > > > > +
> > > > > > +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 (!is_configuration_valid_common(config, config_size))
> > > > > > + return false;
> > > > > > +
> > > > > > + if (config->direction & FASTSTREAM_DIRECTION_SOURCE) {
> > > > > > + pa_log_error("Invalid direction in configuration");
> > > > > > + return false;
> > > > > > + }
> > > > > > +
> > > > > > + return true;
> > > > > > +}
> > > > > > +
> > > > > > +static bool is_configuration_valid_mic(const uint8_t *config_buffer, uint8_t config_size) {
> > > > > > + const a2dp_faststream_t *config = (const a2dp_faststream_t *) config_buffer;
> > > > > > +
> > > > > > + if (!is_configuration_valid_common(config, config_size))
> > > > > > + return false;
> > > > > > +
> > > > > > + if (!(config->direction & FASTSTREAM_DIRECTION_SOURCE)) {
> > > > > > + pa_log_error("Invalid direction in configuration");
> > > > > > + return false;
> > > > > > + }
> > > > > > +
> > > > > > + if (config->source_frequency != FASTSTREAM_SOURCE_SAMPLING_FREQ_16000) {
> > > > > > + pa_log_error("Invalid source sampling frequency in configuration");
> > > > > > + return false;
> > > > > > + }
> > > > > > +
> > > > > > + return true;
> > > > > > +}
> > > > > > +
> > > > > > +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".
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.
> 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.
>
> > > > > > + config->sink_frequency = freq_table[i].cap;
> > > > > > + break;
> > > > > > + }
> > > > > > +
> > > > > > + if ((unsigned) i == PA_ELEMENTSOF(freq_table)) {
> > > > > This "if" does not seem necessary, we know that the table has some elements.
> > > > It is necessary. It is fallback when first for loop (above) did not
> > > > choose suitable sample rate. "i" is set to elements count when previous
> > > > for loop did not break.
> > > Yes, sorry. Misread the code.
> > >
> > >
> > > > > > + for (--i; i >= 0; i--) {
> > > > > > + if (capabilities->sink_frequency & freq_table[i].cap) {
> > > > > > + config->sink_frequency = freq_table[i].cap;
> > > > > > + break;
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > > + if (i < 0) {
> > > > > > + pa_log_error("Not suitable sample rate");
> > > > > > + return 0;
> > > > > As said in the previous patch, I think an assertion would be OK here.
> > > > This error can happen. E.g. when remote side advertise frequencies
> > > > different then 44.1kHz and 48kHz. We do not want to crash pulseaudio
> > > > (with assertion) when some bluetooth headset either send garbage or send
> > > > list of frequencies which pulseaudio does not support.
> > > Can that really happen?
> > Yes
> >
> > > Do you not check that the capabilities
> > > are valid before filling the configuration?
> > Advertised remote capabilities are checked that are valid. But remote
> > side send another set of capabilities when is establishing connection.
> > And this is validated in this function.
> >
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > > + pa_assert((unsigned) i < PA_ELEMENTSOF(freq_table));
> > > > > > +
> > > > > > + if (!(capabilities->direction & FASTSTREAM_DIRECTION_SINK)) {
> > > > > > + pa_log_error("No sink support");
> > > > > > + return 0;
> > > > > > + }
> > > > > > +
> > > > > > + config->direction = FASTSTREAM_DIRECTION_SINK;
> > > > > > +
> > > > > > + return sizeof(*config);
> > > > > > +}
> > > > > > +
> > > > > ...
>
--
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
