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? Otherwise 44k1 could be chosen falsely here
if the headset
again sends both frequencies.
+ 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);
+}
+
...
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
