Re: [pulseaudio-discuss] new module module-plugin-sink

[Alexander E. Patrakov]

пт, 3 мая 2019 г. в 23:04, Georg Chini :
>
> On 03.05.19 16:32, Alexander E. Patrakov wrote:
> > пт, 3 мая 2019 г. в 11:57, Georg Chini :
> >> The channel layout of input/output
> >> and the playback device is known to module-plugin-sink, so if
> >> a filter needs it, it can be passed as parameter. No need to
> >> have it in the interface.
> > (I have also received your next email, ACK on the thought that the
> > channel maps don't change on the fly).
> >
> > Having it (also with other properties like sink or port name) as a
> > parameter is indeed a neat idea that solves a lot of problems.
> Why would the filter code need the sink name? I understand
> that it can be useful to know what kind of output you have,
> but the sink name will not mean anything to the filter. The
> plugins are compiled outside PA, similar to LADSPA plugins.

Because filter sinks can, in theory, be moved, e.g. due to device
hotplug or port status change, to a different master sink, to which
they somehow know they are not applicable.

> > However, we should still think about the boolean bypass parameter, how
> > it is supposed to work. Is my understanding below correct?
> >
> > 1. The virtual surround filter gets created by PulseAudio for 6 input
> > and 2 output channels and gets the input channel layout (5.1), output
> > channel layout (stereo), and playback sink and port as parameters.
> > 2. Some audio plays through it.
> > 3. The user unplugs headphones, so that the output now goes through
> > stereo speakers
> > 4. Before sending the next chunk of audio, PulseAudio updates the
> > filter parameters related to the sink port (a), and/or calls the
> > set_port callback function (b).
> > 5a. The filter notices the parameter change, processes the audio
> > anyway, and sets the self-disable parameter to true.
> > 6a. PulseAudio reads the audio and the self-disable parameter, throws
> > away the processed audio and downmixes 5.1 to stereo by itself.
> > 5b. set_port says "no" or updates the self-disable parameter,
> > PulseAudio notices and downmixes 5.1 to stereo by itself.
>
> When the filter is in the chain, audio is processed by the filter.
> Therefore, down mixing would have to be implemented in the
> filter code. The next chunk after the parameter change would
> need to be down mixed.
> It is impossible to pass the original signal on to PA without
> destroying and re-creating the sink input. (See also below).

And that's exactly the problem with the filter sink model when the
filter is applicable only in some cases that depend e.g. on what's
connected.

I have just tried to do this:

0. Make sure that both analog outputs and HDMI outputs are available
as two sinks
1. Add a module-virtual-sink on top of analog stereo output, play some
file in vlc through it.
2. Switch the analog sound card profile to 5.1 profile. AFAIK this
could happen automatically on headphone unplugging on systems that
have a separate headphone jack (mine doesn't, it only has line
outputs, spdif, line input and microphone input on the back panel).

Result: the virtual sink is still on top of the onboard analog sound
card, and vlc is still playing through it.

If this happens with a module-virtual-surround-sink, then it would
convert the 5.1 sound from vlc to stereo according to an algorithm
that is only valid for headphones (or, according to any other
algorithm that it falls back to), and then PulseAudio would upmix this
back to 5.1. Which is bad. The original 5.1 sound should have been
passed through. Or, if it was not 5.1, the remixing should have
happened from the original, not from the sound corrupted to stereo by
the virtual-surround effect.

I don't mind at all if you use the existing virtual surround sink code
as an example to validate your API, but due to the above scenario, I
think that it was a mistake to implement the virtual surround effect
as a virtual sink. It should have been a special case in the remixer,
just like the current LFE remixing code is now.

I do understand that there are cases where the "external filter plugin
sink" model is applicable. Just saying that virtual surround, if done
properly, is not one of them.

> > It would also be interesting to see what happens if unplugging the
> > headphones causes the number of channels (of the channel layout in
> > general) to change. Note that I don't have such setup, not even sure
> > how it is handled currently.
> >
> The sink input that the filter uses will not change, regardless
> what kind of sink the input is connected to. From the filter
> perspective the channel counts and layouts are static. What does
> PA usually do if the number of channels for a stream does not
> match the number of sink channels, for example if you play
> stereo content to a 5.1 sink?

It remixes the content according to the settings in daemon.conf such
as enable-remixing, remixing-use-all-sink-channels and
enable-lfe-remixing.

-- 
Alexander E. Patrakov
___

[pulseaudio-discuss] [PATCH v10 10/11] bluetooth: Implement A2DP codec switching and backchannel support

[Pali Rohár]

Some A2DP codecs (like FastStream or aptX Low Latency) are bi-directional
and can be used for both music playback and audio call. This patch
implements usage of backchannel if A2DP codec provided by pulseaudio API
supports it.

A2DP codec switching needs new version of bluez as older version does not
provide needed DBus API.

Pulseaudio use for each A2DP codec separate pulseaudio profile, therefore
codec switching is implemented via changing pulseaudio profile and
currently used A2DP codec is visible in pulseaudio profile.

Getting list of supported codecs by remote device is supported only by new
version of bluez daemon.

If old version is detected then profile is created for every codec
supported by pulseaudio (therefore also codecs unavailable by remote
endpoint). Old version of bluez daemon does not support profile switching,
so unavailable profile codecs are harmless.
---
 src/modules/bluetooth/bluez5-util.c| 476 +++--
 src/modules/bluetooth/bluez5-util.h|  39 +-
 src/modules/bluetooth/module-bluez5-device.c   | 411 ++---
 src/modules/bluetooth/module-bluez5-discover.c |   3 +-
 4 files changed, 765 insertions(+), 164 deletions(-)

diff --git a/src/modules/bluetooth/bluez5-util.c 
b/src/modules/bluetooth/bluez5-util.c
index 3c5a000c0..4e4ab0cb8 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -171,11 +171,13 @@ static const char 
*transport_state_to_string(pa_bluetooth_transport_state_t stat
 }
 
 static bool device_supports_profile(pa_bluetooth_device *device, 
pa_bluetooth_profile_t profile) {
+const pa_a2dp_codec_capabilities *a2dp_codec_capabilities;
+const pa_a2dp_codec *a2dp_codec;
+bool is_a2dp_sink;
+pa_hashmap *endpoints;
+void *state;
+
 switch (profile) {
-case PA_BLUETOOTH_PROFILE_A2DP_SINK:
-return !!pa_hashmap_get(device->uuids, 
PA_BLUETOOTH_UUID_A2DP_SINK);
-case PA_BLUETOOTH_PROFILE_A2DP_SOURCE:
-return !!pa_hashmap_get(device->uuids, 
PA_BLUETOOTH_UUID_A2DP_SOURCE);
 case PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT:
 return !!pa_hashmap_get(device->uuids, PA_BLUETOOTH_UUID_HSP_HS)
 || !!pa_hashmap_get(device->uuids, 
PA_BLUETOOTH_UUID_HSP_HS_ALT)
@@ -184,10 +186,33 @@ static bool device_supports_profile(pa_bluetooth_device 
*device, pa_bluetooth_pr
 return !!pa_hashmap_get(device->uuids, PA_BLUETOOTH_UUID_HSP_AG)
 || !!pa_hashmap_get(device->uuids, PA_BLUETOOTH_UUID_HFP_AG);
 case PA_BLUETOOTH_PROFILE_OFF:
-pa_assert_not_reached();
+return true;
+default:
+break;
+}
+
+a2dp_codec = pa_bluetooth_profile_to_a2dp_codec(profile);
+is_a2dp_sink = pa_bluetooth_profile_is_a2dp_sink(profile);
+
+if (is_a2dp_sink && !pa_hashmap_get(device->uuids, 
PA_BLUETOOTH_UUID_A2DP_SINK))
+return false;
+else if (!is_a2dp_sink && !pa_hashmap_get(device->uuids, 
PA_BLUETOOTH_UUID_A2DP_SOURCE))
+return false;
+
+if (is_a2dp_sink)
+endpoints = pa_hashmap_get(device->a2dp_sink_endpoints, 
_codec->id);
+else
+endpoints = pa_hashmap_get(device->a2dp_source_endpoints, 
_codec->id);
+
+if (!endpoints)
+return false;
+
+PA_HASHMAP_FOREACH(a2dp_codec_capabilities, endpoints, state) {
+if 
(a2dp_codec->can_accept_capabilities(a2dp_codec_capabilities->buffer, 
a2dp_codec_capabilities->size, is_a2dp_sink))
+return true;
 }
 
-pa_assert_not_reached();
+return false;
 }
 
 static bool device_is_profile_connected(pa_bluetooth_device *device, 
pa_bluetooth_profile_t profile) {
@@ -199,9 +224,11 @@ static bool 
device_is_profile_connected(pa_bluetooth_device *device, pa_bluetoot
 
 static unsigned device_count_disconnected_profiles(pa_bluetooth_device 
*device) {
 pa_bluetooth_profile_t profile;
+unsigned bluetooth_profile_count;
 unsigned count = 0;
 
-for (profile = 0; profile < PA_BLUETOOTH_PROFILE_COUNT; profile++) {
+bluetooth_profile_count = pa_bluetooth_profile_count();
+for (profile = 0; profile < bluetooth_profile_count; profile++) {
 if (!device_supports_profile(device, profile))
 continue;
 
@@ -224,6 +251,7 @@ static void wait_for_profiles_cb(pa_mainloop_api *api, 
pa_time_event* event, con
 pa_bluetooth_device *device = userdata;
 pa_strbuf *buf;
 pa_bluetooth_profile_t profile;
+unsigned bluetooth_profile_count;
 bool first = true;
 char *profiles_str;
 
@@ -231,7 +259,8 @@ static void wait_for_profiles_cb(pa_mainloop_api *api, 
pa_time_event* event, con
 
 buf = pa_strbuf_new();
 
-for (profile = 0; profile < PA_BLUETOOTH_PROFILE_COUNT; profile++) {
+bluetooth_profile_count = pa_bluetooth_profile_count();
+for (profile = 0; profile < bluetooth_profile_count; profile++) {
 if (device_is_profile_connected(device, profile))
 

[pulseaudio-discuss] [PATCH v10 03/11] bluetooth: Update TODO: about tstamp in a2dp_process_push()

[Pali Rohár]

tstamp is local time when packet was received -- not remote time when packet 
was sent.
---
 src/modules/bluetooth/module-bluez5-device.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/src/modules/bluetooth/module-bluez5-device.c 
b/src/modules/bluetooth/module-bluez5-device.c
index c0b293d94..08de1154b 100644
--- a/src/modules/bluetooth/module-bluez5-device.c
+++ b/src/modules/bluetooth/module-bluez5-device.c
@@ -536,7 +536,6 @@ static int a2dp_process_push(struct userdata *u) {
 memchunk.index = memchunk.length = 0;
 
 for (;;) {
-bool found_tstamp = false;
 pa_usec_t tstamp;
 uint8_t *ptr;
 ssize_t l;
@@ -563,11 +562,8 @@ static int a2dp_process_push(struct userdata *u) {
 
 pa_assert((size_t) l <= u->decoder_buffer_size);
 
-/* TODO: get timestamp from rtp */
-if (!found_tstamp) {
-/* pa_log_warn("Couldn't find SO_TIMESTAMP data in auxiliary 
recvmsg() data!"); */
-tstamp = pa_rtclock_now();
-}
+/* TODO: retrieve SO_TIMESTAMP, local tstamp for received bluz packet 
from u->stream_fd socket */
+tstamp = pa_rtclock_now();
 
 ptr = pa_memblock_acquire(memchunk.memblock);
 memchunk.length = pa_memblock_get_length(memchunk.memblock);
-- 
2.11.0

___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] [PATCH v10 11/11] bluetooth: policy: Treat bi-directional A2DP profiles as suitable for VOIP

[Pali Rohár]

Previously module-bluetooth-policy was switching from A2DP to HSP profile
when VOIP application started recording of source. Now it switch to profile
with the highest priority which has both sink and source. In most cases it
is HSP profile, but it can be also bi-directional A2DP profile (e.g.
FastStream codec) as it has better audio quality.
---
 src/modules/bluetooth/module-bluetooth-policy.c | 123 
 1 file changed, 62 insertions(+), 61 deletions(-)

diff --git a/src/modules/bluetooth/module-bluetooth-policy.c 
b/src/modules/bluetooth/module-bluetooth-policy.c
index 04313aa84..9652a91fe 100644
--- a/src/modules/bluetooth/module-bluetooth-policy.c
+++ b/src/modules/bluetooth/module-bluetooth-policy.c
@@ -59,7 +59,12 @@ struct userdata {
 pa_hook_slot *card_init_profile_slot;
 pa_hook_slot *card_unlink_slot;
 pa_hook_slot *profile_available_changed_slot;
-pa_hashmap *will_need_revert_card_map;
+pa_hashmap *profile_switch_map;
+};
+
+struct profile_switch {
+const char *from_profile;
+const char *to_profile;
 };
 
 /* When a source is created, loopback it to default sink */
@@ -142,43 +147,57 @@ static pa_hook_result_t sink_put_hook_callback(pa_core 
*c, pa_sink *sink, void *
 return PA_HOOK_OK;
 }
 
-static void card_set_profile(struct userdata *u, pa_card *card, bool 
revert_to_a2dp)
-{
+static void card_set_profile(struct userdata *u, pa_card *card, const char 
*revert_to_profile_name) {
+pa_card_profile *iter_profile;
 pa_card_profile *profile;
+struct profile_switch *ps;
+char *old_profile_name;
 void *state;
 
-/* Find available profile and activate it */
-PA_HASHMAP_FOREACH(profile, card->profiles, state) {
-if (profile->available == PA_AVAILABLE_NO)
-continue;
-
-/* Check for correct profile based on revert_to_a2dp */
-if (revert_to_a2dp) {
-if (!pa_startswith(profile->name, "a2dp_sink"))
+if (revert_to_profile_name) {
+profile = pa_hashmap_get(card->profiles, revert_to_profile_name);
+} else {
+/* Find highest priority profile with both sink and source */
+profile = NULL;
+PA_HASHMAP_FOREACH(iter_profile, card->profiles, state) {
+if (iter_profile->available == PA_AVAILABLE_NO)
 continue;
-} else {
-if (!pa_streq(profile->name, "headset_head_unit"))
+if (iter_profile->n_sources == 0 || iter_profile->n_sinks == 0)
 continue;
+if (!profile || profile->priority < iter_profile->priority)
+profile = iter_profile;
 }
+}
 
-pa_log_debug("Setting card '%s' to profile '%s'", card->name, 
profile->name);
+if (!profile) {
+pa_log_warn("Could not find any suitable profile for card '%s'", 
card->name);
+return;
+}
 
-if (pa_card_set_profile(card, profile, false) != 0) {
-pa_log_warn("Could not set profile '%s'", profile->name);
-continue;
-}
+old_profile_name = card->active_profile->name;
+
+pa_log_debug("Setting card '%s' from profile '%s' to profile '%s'", 
card->name, old_profile_name, profile->name);
 
-/* When we are not in revert_to_a2dp phase flag this card for 
will_need_revert */
-if (!revert_to_a2dp)
-pa_hashmap_put(u->will_need_revert_card_map, card, 
PA_INT_TO_PTR(1));
+if (pa_card_set_profile(card, profile, false) != 0) {
+pa_log_warn("Could not set profile '%s'", profile->name);
+return;
+}
 
-break;
+/* When not reverting, store data for future reverting */
+if (!revert_to_profile_name) {
+ps = pa_xnew0(struct profile_switch, 1);
+ps->from_profile = old_profile_name;
+ps->to_profile = profile->name;
+pa_hashmap_put(u->profile_switch_map, card, ps);
 }
 }
 
 /* Switch profile for one card */
-static void switch_profile(pa_card *card, bool revert_to_a2dp, void *userdata) 
{
+static void switch_profile(pa_card *card, bool revert, void *userdata) {
 struct userdata *u = userdata;
+struct profile_switch *ps;
+const char *from_profile;
+const char *to_profile;
 const char *s;
 
 /* Only consider bluetooth cards */
@@ -186,29 +205,25 @@ static void switch_profile(pa_card *card, bool 
revert_to_a2dp, void *userdata) {
 if (!s || !pa_streq(s, "bluetooth"))
 return;
 
-if (revert_to_a2dp) {
-/* In revert_to_a2dp phase only consider cards with will_need_revert 
flag and remove it */
-if (!pa_hashmap_remove(u->will_need_revert_card_map, card))
+if (revert) {
+/* In revert phase only consider cards which switched profile */
+if (!(ps = pa_hashmap_remove(u->profile_switch_map, card)))
 return;
 
-/* Skip card if does not have active hsp profile */
-if (!pa_streq(card->active_profile->name, "headset_head_unit"))
-return;
+

[pulseaudio-discuss] [PATCH v10 09/11] bluetooth: policy: Reflect a2dp profile names

[Pali Rohár]

In next patches, codec name is appended end the end of a2dp profile names.
---
 src/modules/bluetooth/module-bluetooth-policy.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/modules/bluetooth/module-bluetooth-policy.c 
b/src/modules/bluetooth/module-bluetooth-policy.c
index 0a6d59d28..04313aa84 100644
--- a/src/modules/bluetooth/module-bluetooth-policy.c
+++ b/src/modules/bluetooth/module-bluetooth-policy.c
@@ -85,7 +85,7 @@ static pa_hook_result_t source_put_hook_callback(pa_core *c, 
pa_source *source,
 if (!s)
 return PA_HOOK_OK;
 
-if (u->enable_a2dp_source && pa_streq(s, "a2dp_source"))
+if (u->enable_a2dp_source && pa_startswith(s, "a2dp_source"))
 role = "music";
 else if (u->enable_ag && pa_streq(s, "headset_audio_gateway"))
 role = "phone";
@@ -154,7 +154,7 @@ static void card_set_profile(struct userdata *u, pa_card 
*card, bool revert_to_a
 
 /* Check for correct profile based on revert_to_a2dp */
 if (revert_to_a2dp) {
-if (!pa_streq(profile->name, "a2dp_sink"))
+if (!pa_startswith(profile->name, "a2dp_sink"))
 continue;
 } else {
 if (!pa_streq(profile->name, "headset_head_unit"))
@@ -196,11 +196,11 @@ static void switch_profile(pa_card *card, bool 
revert_to_a2dp, void *userdata) {
 return;
 
 /* Skip card if already has active a2dp profile */
-if (pa_streq(card->active_profile->name, "a2dp_sink"))
+if (pa_startswith(card->active_profile->name, "a2dp_sink"))
 return;
 } else {
 /* Skip card if does not have active a2dp profile */
-if (!pa_streq(card->active_profile->name, "a2dp_sink"))
+if (!pa_startswith(card->active_profile->name, "a2dp_sink"))
 return;
 
 /* Skip card if already has active hsp profile */
@@ -307,7 +307,7 @@ static pa_hook_result_t 
card_init_profile_hook_callback(pa_core *c, pa_card *car
 
 /* Ignore card if has already set other initial profile than a2dp */
 if (card->active_profile &&
-!pa_streq(card->active_profile->name, "a2dp_sink"))
+!pa_startswith(card->active_profile->name, "a2dp_sink"))
 return PA_HOOK_OK;
 
 /* Set initial profile to hsp */
@@ -359,7 +359,7 @@ static pa_hook_result_t 
profile_available_hook_callback(pa_core *c, pa_card_prof
 return PA_HOOK_OK;
 
 /* Do not automatically switch profiles for headsets, just in case */
-if (pa_streq(profile->name, "a2dp_sink") || pa_streq(profile->name, 
"headset_head_unit"))
+if (pa_startswith(profile->name, "a2dp_sink") || pa_streq(profile->name, 
"headset_head_unit"))
 return PA_HOOK_OK;
 
 is_active_profile = card->active_profile == profile;
-- 
2.11.0

___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] [PATCH v10 08/11] bluetooth: Add more variants of SBC codec

[Pali Rohár]

Specify configuration for Low, Middle, High and Ultra High Quality of SBC
codec. SBC codec in Ultra High Quality has higher quality than aptX.

Automatic Quality mode matches configuration of SBC codec which was used
before this change. Which means that it accept configuration between Low
and High quality.

Current SBC code was extended to allow definitions of arbitrary
configuration variants of SBC codec parameters.
---
 src/modules/bluetooth/a2dp-codec-sbc.c  | 731 ++--
 src/modules/bluetooth/a2dp-codec-util.c |  18 +-
 2 files changed, 614 insertions(+), 135 deletions(-)

diff --git a/src/modules/bluetooth/a2dp-codec-sbc.c 
b/src/modules/bluetooth/a2dp-codec-sbc.c
index 142c76ea4..abd4f3d61 100644
--- a/src/modules/bluetooth/a2dp-codec-sbc.c
+++ b/src/modules/bluetooth/a2dp-codec-sbc.c
@@ -36,8 +36,71 @@
 #include "a2dp-codec-api.h"
 #include "rtp.h"
 
-#define SBC_BITPOOL_DEC_LIMIT 32
-#define SBC_BITPOOL_DEC_STEP 5
+/* Below are capabilities tables for different qualities. Order of 
capabilities in tables are from the most preferred to the least preferred. */
+
+#define FIXED_SBC_CAPS(mode, freq, bitpool) { .channel_mode = (mode), 
.frequency = (freq), .min_bitpool = (bitpool), .max_bitpool = (bitpool), 
.allocation_method = SBC_ALLOCATION_LOUDNESS, .subbands = SBC_SUBBANDS_8, 
.block_length = SBC_BLOCK_LENGTH_16 }
+
+/* SBC Low Quality, Joint Stereo is same as FastStream's SBC codec 
configuration, Mono was calculated to match Joint Stereo */
+static const a2dp_sbc_t sbc_lq_caps_table[] = {
+FIXED_SBC_CAPS(SBC_CHANNEL_MODE_JOINT_STEREO, SBC_SAMPLING_FREQ_44100, 
29), /* 195.7 kbps */
+FIXED_SBC_CAPS(SBC_CHANNEL_MODE_JOINT_STEREO, SBC_SAMPLING_FREQ_48000, 
29), /* 213   kbps */
+FIXED_SBC_CAPS(SBC_CHANNEL_MODE_MONO, SBC_SAMPLING_FREQ_44100, 
15), /* 104.7 kbps */
+FIXED_SBC_CAPS(SBC_CHANNEL_MODE_MONO, SBC_SAMPLING_FREQ_48000, 
15), /* 114   kbps */
+};
+
+/* SBC Middle Quality, based on A2DP spec: Recommended sets of SBC parameters 
*/
+static const a2dp_sbc_t sbc_mq_caps_table[] = {
+FIXED_SBC_CAPS(SBC_CHANNEL_MODE_JOINT_STEREO, SBC_SAMPLING_FREQ_44100, 
SBC_BITPOOL_MQ_JOINT_STEREO_44100), /* bitpool = 35, 228.8 kbps */
+FIXED_SBC_CAPS(SBC_CHANNEL_MODE_JOINT_STEREO, SBC_SAMPLING_FREQ_48000, 
SBC_BITPOOL_MQ_JOINT_STEREO_48000), /* bitpool = 33, 237   kbps */
+FIXED_SBC_CAPS(SBC_CHANNEL_MODE_MONO, SBC_SAMPLING_FREQ_44100, 
SBC_BITPOOL_MQ_MONO_44100), /* bitpool = 19, 126.8 kbps */
+FIXED_SBC_CAPS(SBC_CHANNEL_MODE_MONO, SBC_SAMPLING_FREQ_48000, 
SBC_BITPOOL_MQ_MONO_48000), /* bitpool = 18, 132   kbps */
+};
+
+/* SBC High Quality, based on A2DP spec: Recommended sets of SBC parameters */
+static const a2dp_sbc_t sbc_hq_caps_table[] = {
+FIXED_SBC_CAPS(SBC_CHANNEL_MODE_JOINT_STEREO, SBC_SAMPLING_FREQ_44100, 
SBC_BITPOOL_HQ_JOINT_STEREO_44100), /* bitpool = 53, 328   kbps */
+FIXED_SBC_CAPS(SBC_CHANNEL_MODE_JOINT_STEREO, SBC_SAMPLING_FREQ_48000, 
SBC_BITPOOL_HQ_JOINT_STEREO_48000), /* bitpool = 51, 345   kbps */
+FIXED_SBC_CAPS(SBC_CHANNEL_MODE_MONO, SBC_SAMPLING_FREQ_44100, 
SBC_BITPOOL_HQ_MONO_44100), /* bitpool = 31, 192.9 kbps */
+FIXED_SBC_CAPS(SBC_CHANNEL_MODE_MONO, SBC_SAMPLING_FREQ_48000, 
SBC_BITPOOL_HQ_MONO_48000), /* bitpool = 29, 210   kbps */
+};
+
+/* SBC Ultra High Quality, calculated to minimize wasted bytes and to be below 
max possible 512 kbps */
+static const a2dp_sbc_t sbc_uhq1_caps_table[] = {
+FIXED_SBC_CAPS(SBC_CHANNEL_MODE_JOINT_STEREO, SBC_SAMPLING_FREQ_44100, 
76), /* 454.8 kbps */
+FIXED_SBC_CAPS(SBC_CHANNEL_MODE_JOINT_STEREO, SBC_SAMPLING_FREQ_48000, 
76), /* 495   kbps */
+FIXED_SBC_CAPS(SBC_CHANNEL_MODE_STEREO,   SBC_SAMPLING_FREQ_44100, 
76), /* 452   kbps */
+FIXED_SBC_CAPS(SBC_CHANNEL_MODE_STEREO,   SBC_SAMPLING_FREQ_48000, 
76), /* 492   kbps */
+};
+static const a2dp_sbc_t sbc_uhq2_caps_table[] = {
+FIXED_SBC_CAPS(SBC_CHANNEL_MODE_DUAL_CHANNEL, SBC_SAMPLING_FREQ_44100, 
38), /* 452   kbps */
+FIXED_SBC_CAPS(SBC_CHANNEL_MODE_DUAL_CHANNEL, SBC_SAMPLING_FREQ_48000, 
38), /* 492   kbps */
+FIXED_SBC_CAPS(SBC_CHANNEL_MODE_MONO, SBC_SAMPLING_FREQ_44100, 
37), /* 226   kbps */
+FIXED_SBC_CAPS(SBC_CHANNEL_MODE_MONO, SBC_SAMPLING_FREQ_48000, 
37), /* 246   kbps */
+};
+/* In most cases bluetooth headsets would support only sbc dual channel mode
+ * for 2 channels as they have limited maximal bitpool value to 53.
+ * We need to define it in two tables to disallow invalid combination of
+ * joint stereo with bitpool 38 which is not UHQ. */
+
+#undef FIXED_SBC_CAPS
+
+/* SBC Auto Quality, only one row which allow any possible configuration up to 
common High Quality */
+/* We need to ensure that bitrate is below max possible 512 kbps, therefore 
limit configuration to High Quality */
+static const a2dp_sbc_t sbc_auto_caps_table[] = { {
+.channel_mode = 

[pulseaudio-discuss] [PATCH v10 00/11] Bluetooth A2DP codecs

[Pali Rohár]

Changes in v10:
* SBC: Mark const table with "const"
* SBC: Fix choosing frequency
* SBC: Fix checking for channel mode capability
* SBC: Fix usage of RTP structures
* aptX-HD: Fix calculation of buffer size
* aptX-HD: Wrap data to RTP packets
* Fix choosing initial profile, use directly bluetooth transport state
* Handle codec change in progress state in device_connection_changed_cb
* Parse remote timestamp from A2DP RTP packets
* Update TODO: about tstamp in a2dp_process_push()

Pali Rohár (11):
  bluetooth: Fix A2DP codec API to provide information about data buffer
  bluetooth: Fix usage of RTP structures in SBC codec
  bluetooth: Update TODO: about tstamp in a2dp_process_push()
  bluetooth: Parse remote timestamp from A2DP RTP packets when available
  bluetooth: Set initial A2DP profile which bluez already activated
  bluetooth: Add A2DP aptX and aptX HD codecs support
  bluetooth: Add A2DP FastStream codec support
  bluetooth: Add more variants of SBC codec
  bluetooth: policy: Reflect a2dp profile names
  bluetooth: Implement A2DP codec switching and backchannel support
  bluetooth: policy: Treat bi-directional A2DP profiles as suitable for
VOIP

 configure.ac|  36 ++
 src/Makefile.am |   8 +
 src/modules/bluetooth/a2dp-codec-api.h  |  19 +-
 src/modules/bluetooth/a2dp-codec-aptx.c | 447 ++
 src/modules/bluetooth/a2dp-codec-faststream.c   | 453 ++
 src/modules/bluetooth/a2dp-codec-sbc.c  | 771 +++-
 src/modules/bluetooth/a2dp-codec-util.c |  26 +-
 src/modules/bluetooth/bluez5-util.c | 476 ++-
 src/modules/bluetooth/bluez5-util.h |  39 +-
 src/modules/bluetooth/module-bluetooth-policy.c | 127 ++--
 src/modules/bluetooth/module-bluez5-device.c| 523 +++-
 src/modules/bluetooth/module-bluez5-discover.c  |   3 +-
 src/modules/bluetooth/rtp.h |  58 +-
 13 files changed, 2561 insertions(+), 425 deletions(-)
 create mode 100644 src/modules/bluetooth/a2dp-codec-aptx.c
 create mode 100644 src/modules/bluetooth/a2dp-codec-faststream.c

-- 
2.11.0

___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] [PATCH v10 05/11] bluetooth: Set initial A2DP profile which bluez already activated

[Pali Rohár]

Bluez and remote device decide which A2DP codec would use. Use this
selected A2DP codec as initial profile in pulseaudio.

In most cases it is either last used codec or codec with higher priority by
defined by remote device.

To detect which A2DP profile was activated by bluez, look at bluez
transport state.
---
 src/modules/bluetooth/module-bluez5-device.c | 66 +++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/src/modules/bluetooth/module-bluez5-device.c 
b/src/modules/bluetooth/module-bluez5-device.c
index 0c4a69448..3c17181ae 100644
--- a/src/modules/bluetooth/module-bluez5-device.c
+++ b/src/modules/bluetooth/module-bluez5-device.c
@@ -1928,6 +1928,70 @@ static int uuid_to_profile(const char *uuid, 
pa_bluetooth_profile_t *_r) {
 return 0;
 }
 
+static void choose_initial_profile(struct userdata *u) {
+struct pa_bluetooth_transport *transport;
+pa_card_profile *iter_profile;
+pa_card_profile *profile;
+void *state;
+
+pa_log_debug("Looking for A2DP profile which has active bluez transport 
for card %s", u->card->name);
+
+profile = NULL;
+
+/* First try to find the best A2DP profile with transport in playing state 
*/
+PA_HASHMAP_FOREACH(iter_profile, u->card->profiles, state) {
+transport = u->device->transports[iter_profile->priority];
+
+/* Ignore profiles without active bluez transport in playing state */
+if (!transport || transport->state != 
PA_BLUETOOTH_TRANSPORT_STATE_PLAYING)
+continue;
+
+/* Ignore non-A2DP profiles */
+if (!pa_startswith(iter_profile->name, "a2dp_"))
+continue;
+
+pa_log_debug("%s has active bluez transport in playing state", 
iter_profile->name);
+
+if (!profile || iter_profile->priority > profile->priority)
+profile = iter_profile;
+}
+
+/* Second if there is no profile A2DP profile with transport in playing 
state, look at active transports */
+if (!profile) {
+PA_HASHMAP_FOREACH(iter_profile, u->card->profiles, state) {
+transport = u->device->transports[iter_profile->priority];
+
+/* Ignore profiles without active bluez transport */
+if (!transport || transport->state == 
PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED)
+continue;
+
+/* Ignore non-A2DP profiles */
+if (!pa_startswith(iter_profile->name, "a2dp_"))
+continue;
+
+pa_log_debug("%s has active bluez transport", iter_profile->name);
+
+if (!profile || iter_profile->priority > profile->priority)
+profile = iter_profile;
+}
+}
+
+/* When there is no active A2DP bluez transport, fallback to core 
pulseaudio function for choosing initial profile */
+if (!profile) {
+pa_log_debug("No A2DP profile with bluez active transport was found 
for card %s", u->card->name);
+pa_card_choose_initial_profile(u->card);
+return;
+}
+
+/* Do same job as pa_card_choose_initial_profile() */
+pa_log_info("Setting initial A2DP profile '%s' for card %s", 
profile->name, u->card->name);
+u->card->active_profile = profile;
+u->card->save_profile = false;
+
+/* Let policy modules override the default. */
+
pa_hook_fire(>card->core->hooks[PA_CORE_HOOK_CARD_CHOOSE_INITIAL_PROFILE], 
u->card);
+}
+
 /* Run from main thread */
 static int add_card(struct userdata *u) {
 const pa_bluetooth_device *d;
@@ -1998,7 +2062,7 @@ static int add_card(struct userdata *u) {
 
 u->card->userdata = u;
 u->card->set_profile = set_profile_cb;
-pa_card_choose_initial_profile(u->card);
+choose_initial_profile(u);
 pa_card_put(u->card);
 
 p = PA_CARD_PROFILE_DATA(u->card->active_profile);
-- 
2.11.0

___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] [PATCH v10 07/11] bluetooth: Add A2DP FastStream codec support

[Pali Rohár]

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 <= DM5 = 220, with 3 SBC frames). 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 = 72*3 <= DM5 =
220, with 3 SBC frames).

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 | 453 ++
 src/modules/bluetooth/a2dp-codec-util.c   |   2 +
 3 files changed, 457 insertions(+)
 create mode 100644 src/modules/bluetooth/a2dp-codec-faststream.c

diff --git a/src/Makefile.am b/src/Makefile.am
index a303578bb..a08dd3090 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -2154,6 +2154,8 @@ libbluez5_util_la_SOURCES += 
modules/bluetooth/a2dp-codec-sbc.c
 libbluez5_util_la_LIBADD += $(SBC_LIBS)
 libbluez5_util_la_CFLAGS += $(SBC_CFLAGS)
 
+libbluez5_util_la_SOURCES += modules/bluetooth/a2dp-codec-faststream.c
+
 if HAVE_OPENAPTX
 libbluez5_util_la_SOURCES += modules/bluetooth/a2dp-codec-aptx.c
 libbluez5_util_la_CPPFLAGS += $(OPENAPTX_CPPFLAGS)
diff --git a/src/modules/bluetooth/a2dp-codec-faststream.c 
b/src/modules/bluetooth/a2dp-codec-faststream.c
new file mode 100644
index 0..bb669bca0
--- /dev/null
+++ b/src/modules/bluetooth/a2dp-codec-faststream.c
@@ -0,0 +1,453 @@
+/***
+  This file is part of PulseAudio.
+
+  Copyright 2018-2019 Pali Rohár 
+
+  PulseAudio is free software; you can redistribute it and/or modify
+  it under the terms of the GNU Lesser General Public License as
+  published by the Free Software Foundation; either version 2.1 of the
+  License, or (at your option) any later version.
+
+  PulseAudio is distributed in the hope that it will be useful, but
+  WITHOUT ANY WARRANTY; without even the implied warranty of
+  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+  General Public License for more details.
+
+  You should have received a copy of the GNU Lesser General Public
+  License along with PulseAudio; if not, see .
+***/
+
+#ifdef HAVE_CONFIG_H
+#include 
+#endif
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#include "a2dp-codecs.h"
+#include "a2dp-codec-api.h"
+
+struct faststream_info {
+sbc_t sbc;   /* Codec data */
+size_t codesize, frame_length;   /* SBC Codesize, frame_length. We 
simply cache those values here */
+bool is_backchannel;
+uint8_t frequency;
+};
+
+static bool can_accept_capabilities(const uint8_t *capabilities_buffer, 
uint8_t capabilities_size, bool for_encoding) {
+const a2dp_faststream_t *capabilities = (const a2dp_faststream_t *) 
capabilities_buffer;
+
+if (A2DP_GET_VENDOR_ID(capabilities->info) != FASTSTREAM_VENDOR_ID || 
A2DP_GET_CODEC_ID(capabilities->info) != FASTSTREAM_CODEC_ID)
+return false;
+
+if (!(capabilities->direction & FASTSTREAM_DIRECTION_SINK) || 
!(capabilities->direction & FASTSTREAM_DIRECTION_SOURCE))
+return false;
+
+if (!(capabilities->sink_frequency & (FASTSTREAM_SINK_SAMPLING_FREQ_44100 
| FASTSTREAM_SINK_SAMPLING_FREQ_48000)))
+return false;
+
+if (!(capabilities->source_frequency & 
FASTSTREAM_SOURCE_SAMPLING_FREQ_16000))
+return false;
+
+return true;
+}
+
+static const char *choose_remote_endpoint(const pa_hashmap 
*capabilities_hashmap, const pa_sample_spec *default_sample_spec, bool 
for_encoding) {
+const pa_a2dp_codec_capabilities *a2dp_capabilities;
+const char *key;
+void *state;
+
+/* There is no preference, just choose random valid entry */
+PA_HASHMAP_FOREACH_KV(key, a2dp_capabilities, capabilities_hashmap, state) 
{
+if (can_accept_capabilities(a2dp_capabilities->buffer, 
a2dp_capabilities->size, for_encoding))
+return key;
+}
+
+return NULL;
+}
+
+static uint8_t fill_capabilities(uint8_t 
capabilities_buffer[MAX_A2DP_CAPS_SIZE]) {
+a2dp_faststream_t *capabilities = (a2dp_faststream_t *) 
capabilities_buffer;
+
+pa_zero(*capabilities);
+
+capabilities->info = A2DP_SET_VENDOR_ID_CODEC_ID(FASTSTREAM_VENDOR_ID, 
FASTSTREAM_CODEC_ID);
+capabilities->direction = FASTSTREAM_DIRECTION_SINK 

[pulseaudio-discuss] [PATCH v10 06/11] bluetooth: Add A2DP aptX and aptX HD codecs support

[Pali Rohár]

This patch provides support for aptX and aptX HD codecs in bluetooth A2DP
profile. It uses open source LGPLv2.1+ licensed libopenaptx library which
can be found at https://github.com/pali/libopenaptx.

aptX for s24 stereo samples provides fixed 6:1 compression ratio and
bitrate 352.8 kbit/s, aptX HD provides fixed 4:1 compression ratio and
bitrate 529.2 kbit/s.

According to soundexpert research, aptX codec used in bluetooth A2DP is no
better than SBC High Quality settings. And you cannot hear difference
between aptX and SBC High Quality, aptX is just a copper-less overpriced
audio cable.

aptX HD is high-bitrate version of aptX. It has clearly noticeable increase
in sound quality (not dramatic though taking into account the increase in
bitrate).

http://soundexpert.org/news/-/blogs/audio-quality-of-bluetooth-aptx
---
 configure.ac|  36 +++
 src/Makefile.am |   6 +
 src/modules/bluetooth/a2dp-codec-aptx.c | 447 
 src/modules/bluetooth/a2dp-codec-util.c |   8 +
 4 files changed, 497 insertions(+)
 create mode 100644 src/modules/bluetooth/a2dp-codec-aptx.c

diff --git a/configure.ac b/configure.ac
index b44ed1595..de63d7aa7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1106,6 +1106,40 @@ AC_SUBST(HAVE_BLUEZ_5_NATIVE_HEADSET)
 AM_CONDITIONAL([HAVE_BLUEZ_5_NATIVE_HEADSET], [test 
"x$HAVE_BLUEZ_5_NATIVE_HEADSET" = x1])
 AS_IF([test "x$HAVE_BLUEZ_5_NATIVE_HEADSET" = "x1"], 
AC_DEFINE([HAVE_BLUEZ_5_NATIVE_HEADSET], 1, [Bluez 5 native headset backend 
enabled]))
 
+ Bluetooth A2DP aptX codec (optional) ###
+
+AC_ARG_ENABLE([aptx],
+AS_HELP_STRING([--disable-aptx],[Disable optional bluetooth A2DP aptX and 
aptX HD codecs support (via libopenaptx)]))
+AC_ARG_VAR([OPENAPTX_CPPFLAGS], [C preprocessor flags for openaptx])
+AC_ARG_VAR([OPENAPTX_LDFLAGS], [linker flags for openaptx])
+
+CPPFLAGS_SAVE="$CPPFLAGS"
+LDFLAGS_SAVE="$LDFLAGS"
+LIBS_SAVE="$LIBS"
+
+CPPFLAGS="$CPPFLAGS $OPENAPTX_CPPFLAGS"
+LDFLAGS="$LDFLAGS $OPENAPTX_LDFLAGS"
+
+AS_IF([test "x$HAVE_BLUEZ_5" = "x1" && test "x$enable_aptx" != "xno"],
+[AC_CHECK_HEADER([openaptx.h],
+[AC_SEARCH_LIBS([aptx_init], [openaptx],
+[HAVE_OPENAPTX=1; AS_IF([test "x$ac_cv_search_aptx_init" != "xnone 
required"], [OPENAPTX_LDFLAGS="$OPENAPTX_LDFLAGS $ac_cv_search_aptx_init"])],
+[HAVE_OPENAPTX=0])],
+[HAVE_OPENAPTX=0])])
+
+CPPFLAGS="$CPPFLAGS_SAVE"
+LDFLAGS="$LDFLAGS_SAVE"
+LIBS="$LIBS_SAVE"
+
+AS_IF([test "x$HAVE_BLUEZ_5" = "x1" && test "x$enable_aptx" = "xyes" && test 
"x$HAVE_OPENAPTX" = "x0"],
+[AC_MSG_ERROR([*** libopenaptx from https://github.com/pali/libopenaptx 
not found])])
+
+AC_SUBST(OPENAPTX_CPPFLAGS)
+AC_SUBST(OPENAPTX_LDFLAGS)
+AC_SUBST(HAVE_OPENAPTX)
+AM_CONDITIONAL([HAVE_OPENAPTX], [test "x$HAVE_OPENAPTX" = "x1"])
+AS_IF([test "x$HAVE_OPENAPTX" = "x1"], AC_DEFINE([HAVE_OPENAPTX], 1, [Have 
openaptx codec library]))
+
  UDEV support (optional) 
 
 AC_ARG_ENABLE([udev],
@@ -1591,6 +1625,7 @@ AS_IF([test "x$HAVE_SYSTEMD_JOURNAL" = "x1"], 
ENABLE_SYSTEMD_JOURNAL=yes, ENABLE
 AS_IF([test "x$HAVE_BLUEZ_5" = "x1"], ENABLE_BLUEZ_5=yes, ENABLE_BLUEZ_5=no)
 AS_IF([test "x$HAVE_BLUEZ_5_OFONO_HEADSET" = "x1"], 
ENABLE_BLUEZ_5_OFONO_HEADSET=yes, ENABLE_BLUEZ_5_OFONO_HEADSET=no)
 AS_IF([test "x$HAVE_BLUEZ_5_NATIVE_HEADSET" = "x1"], 
ENABLE_BLUEZ_5_NATIVE_HEADSET=yes, ENABLE_BLUEZ_5_NATIVE_HEADSET=no)
+AS_IF([test "x$HAVE_OPENAPTX" = "x1"], ENABLE_APTX=yes, ENABLE_APTX=no)
 AS_IF([test "x$HAVE_HAL_COMPAT" = "x1"], ENABLE_HAL_COMPAT=yes, 
ENABLE_HAL_COMPAT=no)
 AS_IF([test "x$HAVE_TCPWRAP" = "x1"], ENABLE_TCPWRAP=yes, ENABLE_TCPWRAP=no)
 AS_IF([test "x$HAVE_LIBSAMPLERATE" = "x1"], ENABLE_LIBSAMPLERATE="yes 
(DEPRECATED)", ENABLE_LIBSAMPLERATE=no)
@@ -1649,6 +1684,7 @@ echo "
   Enable BlueZ 5:  ${ENABLE_BLUEZ_5}
 Enable ofono headsets: ${ENABLE_BLUEZ_5_OFONO_HEADSET}
 Enable native headsets:${ENABLE_BLUEZ_5_NATIVE_HEADSET}
+Enable aptX+aptXHD codecs: ${ENABLE_APTX}
 Enable udev:   ${ENABLE_UDEV}
   Enable HAL->udev compat: ${ENABLE_HAL_COMPAT}
 Enable systemd
diff --git a/src/Makefile.am b/src/Makefile.am
index 5ef870e32..a303578bb 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -2154,6 +2154,12 @@ libbluez5_util_la_SOURCES += 
modules/bluetooth/a2dp-codec-sbc.c
 libbluez5_util_la_LIBADD += $(SBC_LIBS)
 libbluez5_util_la_CFLAGS += $(SBC_CFLAGS)
 
+if HAVE_OPENAPTX
+libbluez5_util_la_SOURCES += modules/bluetooth/a2dp-codec-aptx.c
+libbluez5_util_la_CPPFLAGS += $(OPENAPTX_CPPFLAGS)
+libbluez5_util_la_LDFLAGS += $(OPENAPTX_LDFLAGS)
+endif
+
 module_bluez5_discover_la_SOURCES = modules/bluetooth/module-bluez5-discover.c
 module_bluez5_discover_la_LDFLAGS = $(MODULE_LDFLAGS)
 module_bluez5_discover_la_LIBADD = $(MODULE_LIBADD) $(DBUS_LIBS) 
libbluez5-util.la
diff --git a/src/modules/bluetooth/a2dp-codec-aptx.c 

[pulseaudio-discuss] [PATCH v10 02/11] bluetooth: Fix usage of RTP structures in SBC codec

[Pali Rohár]

Rename struct rtp_payload to rtp_sbc_payload as it is specific for SBC
codec. Add proper checks for endianity in rtp.h header and use uint8_t type
where appropriated. And because rtp_sbc_payload structure is not parsed by
decoder there is no support for fragmented SBC frames. Add warning for it.
---
 src/modules/bluetooth/a2dp-codec-sbc.c | 16 ++
 src/modules/bluetooth/rtp.h| 58 +++---
 2 files changed, 42 insertions(+), 32 deletions(-)

diff --git a/src/modules/bluetooth/a2dp-codec-sbc.c 
b/src/modules/bluetooth/a2dp-codec-sbc.c
index f339b570d..6ab0b46cd 100644
--- a/src/modules/bluetooth/a2dp-codec-sbc.c
+++ b/src/modules/bluetooth/a2dp-codec-sbc.c
@@ -480,7 +480,7 @@ static void reset(void *codec_info) {
 
 static void get_buffer_size(void *codec_info, size_t link_mtu, size_t 
*decoded_buffer_size, size_t *encoded_buffer_size) {
 struct sbc_info *sbc_info = (struct sbc_info *) codec_info;
-size_t rtp_size = sizeof(struct rtp_header) + sizeof(struct rtp_payload);
+size_t rtp_size = sizeof(struct rtp_header) + sizeof(struct 
rtp_sbc_payload);
 size_t num_of_frames = (link_mtu - rtp_size) / sbc_info->frame_length;
 
 *decoded_buffer_size = num_of_frames * sbc_info->codesize;
@@ -510,14 +510,14 @@ static int reduce_encoder_bitrate(void *codec_info) {
 static size_t encode_buffer(void *codec_info, uint32_t timestamp, const 
uint8_t *input_buffer, size_t input_size, uint8_t *output_buffer, size_t 
output_size, size_t *processed) {
 struct sbc_info *sbc_info = (struct sbc_info *) codec_info;
 struct rtp_header *header;
-struct rtp_payload *payload;
+struct rtp_sbc_payload *payload;
 uint8_t *d;
 const uint8_t *p;
 size_t to_write, to_encode;
 unsigned frame_count;
 
 header = (struct rtp_header*) output_buffer;
-payload = (struct rtp_payload*) (output_buffer + sizeof(*header));
+payload = (struct rtp_sbc_payload*) (output_buffer + sizeof(*header));
 
 frame_count = 0;
 
@@ -562,7 +562,7 @@ static size_t encode_buffer(void *codec_info, uint32_t 
timestamp, const uint8_t
 } PA_ONCE_END;
 
 /* write it to the fifo */
-memset(output_buffer, 0, sizeof(*header) + sizeof(*payload));
+pa_memzero(output_buffer, sizeof(*header) + sizeof(*payload));
 header->v = 2;
 
 /* A2DP spec: "A payload type in the RTP dynamic range shall be chosen".
@@ -583,13 +583,17 @@ static size_t decode_buffer(void *codec_info, const 
uint8_t *input_buffer, size_
 struct sbc_info *sbc_info = (struct sbc_info *) codec_info;
 
 struct rtp_header *header;
-struct rtp_payload *payload;
+struct rtp_sbc_payload *payload;
 const uint8_t *p;
 uint8_t *d;
 size_t to_write, to_decode;
 
 header = (struct rtp_header *) input_buffer;
-payload = (struct rtp_payload*) (input_buffer + sizeof(*header));
+payload = (struct rtp_sbc_payload*) (input_buffer + sizeof(*header));
+
+/* TODO: Add support for decoding fragmented SBC frames */
+if (payload->is_fragmented)
+pa_log_warn("SBC frame is fragmented, decoding may fail");
 
 p = input_buffer + sizeof(*header) + sizeof(*payload);
 to_decode = input_size - sizeof(*header) - sizeof(*payload);
diff --git a/src/modules/bluetooth/rtp.h b/src/modules/bluetooth/rtp.h
index 20694c1e1..813d9e390 100644
--- a/src/modules/bluetooth/rtp.h
+++ b/src/modules/bluetooth/rtp.h
@@ -3,6 +3,7 @@
  *  BlueZ - Bluetooth protocol stack for Linux
  *
  *  Copyright (C) 2004-2010  Marcel Holtmann 
+ *  Copyright (C) 2019   Pali Rohár 
  *
  *
  *  This library is free software; you can redistribute it and/or
@@ -19,16 +20,20 @@
  *  License along with this library; if not, see 
.
  */
 
-#if __BYTE_ORDER == __LITTLE_ENDIAN
+#include 
+#include 
+
+#if defined(__BYTE_ORDER) && defined(__LITTLE_ENDIAN) && \
+   __BYTE_ORDER == __LITTLE_ENDIAN
 
 struct rtp_header {
-   unsigned cc:4;
-   unsigned x:1;
-   unsigned p:1;
-   unsigned v:2;
+   uint8_t cc:4;
+   uint8_t x:1;
+   uint8_t p:1;
+   uint8_t v:2;
 
-   unsigned pt:7;
-   unsigned m:1;
+   uint8_t pt:7;
+   uint8_t m:1;
 
uint16_t sequence_number;
uint32_t timestamp;
@@ -36,24 +41,25 @@ struct rtp_header {
uint32_t csrc[0];
 } __attribute__ ((packed));
 
-struct rtp_payload {
-   unsigned frame_count:4;
-   unsigned rfa0:1;
-   unsigned is_last_fragment:1;
-   unsigned is_first_fragment:1;
-   unsigned is_fragmented:1;
+struct rtp_sbc_payload {
+   uint8_t frame_count:4;
+   uint8_t rfa0:1;
+   uint8_t is_last_fragment:1;
+   uint8_t is_first_fragment:1;
+   uint8_t is_fragmented:1;
 } __attribute__ ((packed));
 
-#elif __BYTE_ORDER == __BIG_ENDIAN
+#elif defined(__BYTE_ORDER) && defined(__BIG_ENDIAN) && \
+   __BYTE_ORDER == __BIG_ENDIAN
 
 struct rtp_header {
-   unsigned v:2;
-   unsigned p:1;
-   unsigned x:1;

[pulseaudio-discuss] [PATCH v10 04/11] bluetooth: Parse remote timestamp from A2DP RTP packets when available

[Pali Rohár]

Some A2DP codecs (e.g. SBC or aptX-HD) use RTP packets. For sources use
timestamps from RTP packets to calculate read index and therefore remote
timestamp for synchronization.
---
 src/modules/bluetooth/a2dp-codec-api.h   |  4 ++--
 src/modules/bluetooth/a2dp-codec-sbc.c   |  3 ++-
 src/modules/bluetooth/module-bluez5-device.c | 15 ++-
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/src/modules/bluetooth/a2dp-codec-api.h 
b/src/modules/bluetooth/a2dp-codec-api.h
index 517dc76f1..2bdc768df 100644
--- a/src/modules/bluetooth/a2dp-codec-api.h
+++ b/src/modules/bluetooth/a2dp-codec-api.h
@@ -87,8 +87,8 @@ typedef struct pa_a2dp_codec {
 size_t (*encode_buffer)(void *codec_info, uint32_t timestamp, const 
uint8_t *input_buffer, size_t input_size, uint8_t *output_buffer, size_t 
output_size, size_t *processed);
 /* Decode input_buffer of input_size to output_buffer of output_size,
  * returns size of filled ouput_buffer and set processed to size of
- * processed input_buffer */
-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);
+ * processed input_buffer and set timestamp */
+size_t (*decode_buffer)(void *codec_info, uint32_t *timestamp, const 
uint8_t *input_buffer, size_t input_size, uint8_t *output_buffer, size_t 
output_size, size_t *processed);
 } pa_a2dp_codec;
 
 #endif
diff --git a/src/modules/bluetooth/a2dp-codec-sbc.c 
b/src/modules/bluetooth/a2dp-codec-sbc.c
index 6ab0b46cd..142c76ea4 100644
--- a/src/modules/bluetooth/a2dp-codec-sbc.c
+++ b/src/modules/bluetooth/a2dp-codec-sbc.c
@@ -579,7 +579,7 @@ static size_t encode_buffer(void *codec_info, uint32_t 
timestamp, const uint8_t
 return d - output_buffer;
 }
 
-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) {
+static size_t decode_buffer(void *codec_info, uint32_t *timestamp, const 
uint8_t *input_buffer, size_t input_size, uint8_t *output_buffer, size_t 
output_size, size_t *processed) {
 struct sbc_info *sbc_info = (struct sbc_info *) codec_info;
 
 struct rtp_header *header;
@@ -631,6 +631,7 @@ static size_t decode_buffer(void *codec_info, const uint8_t 
*input_buffer, size_
 to_write -= written;
 }
 
+*timestamp = ntohl(header->timestamp);
 *processed = p - input_buffer;
 return d - output_buffer;
 }
diff --git a/src/modules/bluetooth/module-bluez5-device.c 
b/src/modules/bluetooth/module-bluez5-device.c
index 08de1154b..0c4a69448 100644
--- a/src/modules/bluetooth/module-bluez5-device.c
+++ b/src/modules/bluetooth/module-bluez5-device.c
@@ -536,6 +536,7 @@ static int a2dp_process_push(struct userdata *u) {
 memchunk.index = memchunk.length = 0;
 
 for (;;) {
+uint32_t timestamp;
 pa_usec_t tstamp;
 uint8_t *ptr;
 ssize_t l;
@@ -568,7 +569,8 @@ static int a2dp_process_push(struct userdata *u) {
 ptr = pa_memblock_acquire(memchunk.memblock);
 memchunk.length = pa_memblock_get_length(memchunk.memblock);
 
-memchunk.length = u->a2dp_codec->decode_buffer(u->decoder_info, 
u->decoder_buffer, l, ptr, memchunk.length, );
+timestamp = 0; /* Decoder does not have to fill RTP timestamp */
+memchunk.length = u->a2dp_codec->decode_buffer(u->decoder_info, 
, u->decoder_buffer, l, ptr, memchunk.length, );
 
 if (processed != (size_t) l) {
 pa_log_error("Decoding error");
@@ -583,6 +585,17 @@ static int a2dp_process_push(struct userdata *u) {
 break;
 }
 
+/* Some codecs may provide RTP timestamp, so use it to update 
read_index for calculation of remote tstamp */
+if (timestamp) {
+/* RTP timestamp is only 32bit and may overflow, try to detect it 
*/
+size_t frame_size = pa_frame_size(>decoder_sample_spec);
+uint64_t new_read_index1 = (uint64_t) timestamp * frame_size;
+uint64_t new_read_index2 = (uint64_t) (0x1ULL + timestamp) 
* frame_size;
+uint64_t delta_read_index1 = (new_read_index1 > u->read_index) ? 
(new_read_index1 - u->read_index) : (u->read_index - new_read_index1);
+uint64_t delta_read_index2 = (new_read_index2 > u->read_index) ? 
(new_read_index2 - u->read_index) : (u->read_index - new_read_index2);
+u->read_index = (delta_read_index1 < delta_read_index2) ? 
delta_read_index1 : delta_read_index2;
+}
+
 u->read_index += (uint64_t) memchunk.length;
 pa_smoother_put(u->read_smoother, tstamp, 
pa_bytes_to_usec(u->read_index, >decoder_sample_spec));
 pa_smoother_resume(u->read_smoother, tstamp, true);
-- 
2.11.0

___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org

[pulseaudio-discuss] [PATCH v10 01/11] bluetooth: Fix A2DP codec API to provide information about data buffer

[Pali Rohár]

Each codec has different compression ratio and own method how to calculate
buffer size of encoded or decoded samples. So change A2DP codec API to
provide this information for module-bluez5-device module and fix
a2dp_prepare_encoder_buffer() and a2dp_prepare_decoder_buffer() functions.

API functions get_read_buffer_size() and get_write_buffer_size() now set
both decoded and encoded buffer sizes. Function reduce_encoder_bitrate()
was changed to not return new buffer size (it was not obvious if buffer
size was for encoded or decoded samples), but caller rather should call
get_write_buffer_size() to get new sizes.
---
 src/modules/bluetooth/a2dp-codec-api.h   | 17 ++--
 src/modules/bluetooth/a2dp-codec-sbc.c   | 25 ++---
 src/modules/bluetooth/module-bluez5-device.c | 41 ++--
 3 files changed, 50 insertions(+), 33 deletions(-)

diff --git a/src/modules/bluetooth/a2dp-codec-api.h 
b/src/modules/bluetooth/a2dp-codec-api.h
index 55bb9ff70..517dc76f1 100644
--- a/src/modules/bluetooth/a2dp-codec-api.h
+++ b/src/modules/bluetooth/a2dp-codec-api.h
@@ -72,15 +72,14 @@ typedef struct pa_a2dp_codec {
 /* Reset internal state of codec info data in codec_info */
 void (*reset)(void *codec_info);
 
-/* Get read block size for codec */
-size_t (*get_read_block_size)(void *codec_info, size_t read_link_mtu);
-/* Get write block size for codec */
-size_t (*get_write_block_size)(void *codec_info, size_t write_link_mtu);
-
-/* Reduce encoder bitrate for codec, returns new write block size or zero
- * if not changed, called when socket is not accepting encoded data fast
- * enough */
-size_t (*reduce_encoder_bitrate)(void *codec_info, size_t write_link_mtu);
+/* Get buffer sizes for read operations */
+void (*get_read_buffer_size)(void *codec_info, size_t read_link_mtu, 
size_t *output_buffer_size, size_t *encoded_buffer_size);
+/* Get buffer sizes for write operations */
+void (*get_write_buffer_size)(void *codec_info, size_t write_link_mtu, 
size_t *input_buffer_size, size_t *encoded_buffer_size);
+
+/* Reduce encoder bitrate for codec, returns non-zero on failure,
+ * called when socket is not accepting encoded data fast enough */
+int (*reduce_encoder_bitrate)(void *codec_info);
 
 /* Encode input_buffer of input_size to output_buffer of output_size,
  * returns size of filled ouput_buffer and set processed to size of
diff --git a/src/modules/bluetooth/a2dp-codec-sbc.c 
b/src/modules/bluetooth/a2dp-codec-sbc.c
index cdc20d7f0..f339b570d 100644
--- a/src/modules/bluetooth/a2dp-codec-sbc.c
+++ b/src/modules/bluetooth/a2dp-codec-sbc.c
@@ -423,7 +423,10 @@ static void *init(bool for_encoding, bool for_backchannel, 
const uint8_t *config
 sbc_info->min_bitpool = config->min_bitpool;
 sbc_info->max_bitpool = config->max_bitpool;
 
-/* Set minimum bitpool for source to get the maximum possible block_size */
+/* Set minimum bitpool for source to get the maximum possible buffer size
+ * in get_buffer_size() function. Buffer size is inversely proportional to
+ * frame length which depends on bitpool value. Bitpool is controlled by
+ * other side from range [min_bitpool, max_bitpool]. */
 sbc_info->initial_bitpool = for_encoding ? sbc_info->max_bitpool : 
sbc_info->min_bitpool;
 
 set_params(sbc_info);
@@ -475,20 +478,22 @@ static void reset(void *codec_info) {
 sbc_info->seq_num = 0;
 }
 
-static size_t get_block_size(void *codec_info, size_t link_mtu) {
+static void get_buffer_size(void *codec_info, size_t link_mtu, size_t 
*decoded_buffer_size, size_t *encoded_buffer_size) {
 struct sbc_info *sbc_info = (struct sbc_info *) codec_info;
+size_t rtp_size = sizeof(struct rtp_header) + sizeof(struct rtp_payload);
+size_t num_of_frames = (link_mtu - rtp_size) / sbc_info->frame_length;
 
-return (link_mtu - sizeof(struct rtp_header) - sizeof(struct rtp_payload))
-   / sbc_info->frame_length * sbc_info->codesize;
+*decoded_buffer_size = num_of_frames * sbc_info->codesize;
+*encoded_buffer_size = num_of_frames * sbc_info->frame_length + rtp_size;
 }
 
-static size_t reduce_encoder_bitrate(void *codec_info, size_t write_link_mtu) {
+static int reduce_encoder_bitrate(void *codec_info) {
 struct sbc_info *sbc_info = (struct sbc_info *) codec_info;
 uint8_t bitpool;
 
 /* Check if bitpool is already at its limit */
 if (sbc_info->sbc.bitpool <= SBC_BITPOOL_DEC_LIMIT)
-return 0;
+return -1;
 
 bitpool = sbc_info->sbc.bitpool - SBC_BITPOOL_DEC_STEP;
 
@@ -496,10 +501,10 @@ static size_t reduce_encoder_bitrate(void *codec_info, 
size_t write_link_mtu) {
 bitpool = SBC_BITPOOL_DEC_LIMIT;
 
 if (sbc_info->sbc.bitpool == bitpool)
-return 0;
+return -1;
 
 set_bitpool(sbc_info, bitpool);
-return get_block_size(codec_info, write_link_mtu);
+return 0;
 }
 
 static size_t 

Re: [pulseaudio-discuss] new module module-plugin-sink

[Georg Chini]

On 03.05.19 16:32, Alexander E. Patrakov wrote:

пт, 3 мая 2019 г. в 11:57, Georg Chini :

The channel layout of input/output
and the playback device is known to module-plugin-sink, so if
a filter needs it, it can be passed as parameter. No need to
have it in the interface.

(I have also received your next email, ACK on the thought that the
channel maps don't change on the fly).

Having it (also with other properties like sink or port name) as a
parameter is indeed a neat idea that solves a lot of problems.

Why would the filter code need the sink name? I understand
that it can be useful to know what kind of output you have,
but the sink name will not mean anything to the filter. The
plugins are compiled outside PA, similar to LADSPA plugins.


However, we should still think about the boolean bypass parameter, how
it is supposed to work. Is my understanding below correct?

1. The virtual surround filter gets created by PulseAudio for 6 input
and 2 output channels and gets the input channel layout (5.1), output
channel layout (stereo), and playback sink and port as parameters.
2. Some audio plays through it.
3. The user unplugs headphones, so that the output now goes through
stereo speakers
4. Before sending the next chunk of audio, PulseAudio updates the
filter parameters related to the sink port (a), and/or calls the
set_port callback function (b).
5a. The filter notices the parameter change, processes the audio
anyway, and sets the self-disable parameter to true.
6a. PulseAudio reads the audio and the self-disable parameter, throws
away the processed audio and downmixes 5.1 to stereo by itself.
5b. set_port says "no" or updates the self-disable parameter,
PulseAudio notices and downmixes 5.1 to stereo by itself.


When the filter is in the chain, audio is processed by the filter.
Therefore, down mixing would have to be implemented in the
filter code. The next chunk after the parameter change would
need to be down mixed.
It is impossible to pass the original signal on to PA without
destroying and re-creating the sink input. (See also below).



It would also be interesting to see what happens if unplugging the
headphones causes the number of channels (of the channel layout in
general) to change. Note that I don't have such setup, not even sure
how it is handled currently.


The sink input that the filter uses will not change, regardless
what kind of sink the input is connected to. From the filter
perspective the channel counts and layouts are static. What does
PA usually do if the number of channels for a stream does not
match the number of sink channels, for example if you play
stereo content to a 5.1 sink?

___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] new module module-plugin-sink

[Georg Chini]

On 03.05.19 08:57, Georg Chini wrote:

On 03.05.19 06:56, Alexander E. Patrakov wrote:

пт, 3 мая 2019 г. в 09:31, Georg Chini :

On 03.05.19 02:22, Alexander E. Patrakov wrote:

чт, 2 мая 2019 г. в 23:45, Georg Chini :

Hi,

I created a new module-plugin-sink and a small amplifier demo plugin
based on the attached header file. I did not (yet) drop max_latency,
disable_rewind and rewind_filter() because I am still waiting for 
more

feedback on the specification. I made it however more clear (using
Alexander's wording) that this should only be used if "real" 
rewinding

is possible.




If you think channel layout and playback device should be
in the interface, I can integrate it, but using parameters looks
easier to me because this way we need no additional notification
functions.


The channel maps do not change on the fly, so we can pass them in at
filter creation time as arguments to the create_filter() function.
Then we can have a set_port() function, which - if provided - will be called
by module-plugin-sink when the port of the master sink changes. I will 
integrate

that in the next version.
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] new module module-plugin-sink

[Alexander E. Patrakov]

пт, 3 мая 2019 г. в 11:57, Georg Chini :
> The channel layout of input/output
> and the playback device is known to module-plugin-sink, so if
> a filter needs it, it can be passed as parameter. No need to
> have it in the interface.

(I have also received your next email, ACK on the thought that the
channel maps don't change on the fly).

Having it (also with other properties like sink or port name) as a
parameter is indeed a neat idea that solves a lot of problems.
However, we should still think about the boolean bypass parameter, how
it is supposed to work. Is my understanding below correct?

1. The virtual surround filter gets created by PulseAudio for 6 input
and 2 output channels and gets the input channel layout (5.1), output
channel layout (stereo), and playback sink and port as parameters.
2. Some audio plays through it.
3. The user unplugs headphones, so that the output now goes through
stereo speakers
4. Before sending the next chunk of audio, PulseAudio updates the
filter parameters related to the sink port (a), and/or calls the
set_port callback function (b).
5a. The filter notices the parameter change, processes the audio
anyway, and sets the self-disable parameter to true.
6a. PulseAudio reads the audio and the self-disable parameter, throws
away the processed audio and downmixes 5.1 to stereo by itself.
5b. set_port says "no" or updates the self-disable parameter,
PulseAudio notices and downmixes 5.1 to stereo by itself.

It would also be interesting to see what happens if unplugging the
headphones causes the number of channels (of the channel layout in
general) to change. Note that I don't have such setup, not even sure
how it is handled currently.

-- 
Alexander E. Patrakov
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH v9 0/8] Bluetooth A2DP codecs

[Pali Rohár]

On Tuesday 30 April 2019 12:42:28 Luiz Augusto von Dentz wrote:
> 1. At least Android seems to prefer aptX-HD but that doesn't seems to
> produce any audio, in fact it crashed a few times so either have to
> find out why aptX-HD is not working or don't include it.

It is possible that android sends aptX HD data encapsulated in RTP packets:
https://android.googlesource.com/platform/system/bt/+/refs/heads/master/stack/a2dp/a2dp_vendor_aptx_hd.cc#249

So some logic with truncating RTP header would be needed.

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: PGP signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH v9 0/8] Bluetooth A2DP codecs

[Pali Rohár]

On Wednesday 01 May 2019 13:25:04 Luiz Augusto von Dentz wrote:
> Hi Pali,
> 
> On Wed, May 1, 2019 at 12:09 PM Pali Rohár  wrote:
> >
> > On Tuesday 30 April 2019 12:42:28 Luiz Augusto von Dentz wrote:
> > > 1. At least Android seems to prefer aptX-HD but that doesn't seems to
> > > produce any audio, in fact it crashed a few times so either have to
> > > find out why aptX-HD is not working or don't include it.
> > > 2. Codec switching with Android always seems to timeout.
> > > 3. There seem to be some regression with loopback module, there seems
> > > it always produces some glitch when resuming (during the playback it
> > > seems alright):
> > >
> > > I: [alsa-sink-ALC293 Analog] module-loopback.c: Dropping 17414 usec of
> > > audio from queue
> > > I: [alsa-sink-ALC293 Analog] module-loopback.c: Dropping 20317 usec of
> > > audio from queue
> > > I: [alsa-sink-ALC293 Analog] module-loopback.c: Adding 30657 usec of
> > > silence to queue
> > > I: [alsa-sink-ALC293 Analog] module-loopback.c: Dropping 105170 usec
> > > of audio from queue
> >
> > Can you send btmon/wireshark dump? I can inspect aptx HD packets if
> > decoder can correctly decode them.
> 
> You can probably reproduce this faster with a phone, I suspect it also
> doesn't work with iOS either.

Sure, but all tested android phones lacks aptX HD support. They have
only aptX and aptX is working fine. Codec switching is sometimes working
fine and sometimes android does not send any sound data, only shows that
it is connected. It is strange, but nothing I can debug if other side do
not send anything, also no error. I just found one problem in
enumeration of SBC capabilities. I will fix it in next SBC patch series.

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: PGP signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] new module module-plugin-sink

[Georg Chini]

On 03.05.19 06:56, Alexander E. Patrakov wrote:

пт, 3 мая 2019 г. в 09:31, Georg Chini :

On 03.05.19 02:22, Alexander E. Patrakov wrote:

чт, 2 мая 2019 г. в 23:45, Georg Chini :

Hi,

I created a new module-plugin-sink and a small amplifier demo plugin
based on the attached header file. I did not (yet) drop max_latency,
disable_rewind and rewind_filter() because I am still waiting for more
feedback on the specification. I made it however more clear (using
Alexander's wording) that this should only be used if "real" rewinding
is possible.

Thanks for that.

However, the interface still thinks in terms of "number of channels",
which is, in the general case, wrong. E.g., a "proper"
module-virtual-surround-sink (not what we currently have) would sound
differently if given the same samples in "5.1" and "5.1 (Side)"
channel layouts.

I do not understand what you mean. How would you do it without
number of channels? You have to know how many input and
output channels are there, otherwise you can't process audio.

I am saying that, if we are going to support filters with non-trivial
interaction between channels, we would need to have a pa_channel_map
somewhere. In your header, this structure is not mentioned at all.

"5.1" and "5.1 (Side)" both have 6 channels. The hypothetical proper
virtual surround plugin would need to know if its input is "5.1" or
"5.1 (Side)", but, based on your interface, it can't.


I still don't see your point. The channel layout of input/output
and the playback device is known to module-plugin-sink, so if
a filter needs it, it can be passed as parameter. No need to
have it in the interface.




I wouldn't mind the number of channels, if we limit the interface to
"true" filters that have the same number of input and output channels
and don't care about the channel map. I.e., explicitly declare the
virtual surround sink as out-of-scope. But then, what useful things
are left in scope except the equalizer and maybe a dynamic range
compressor?

Why would there be any reason to limit it to filters with
equal number of channels?

Because that follows from the premise that the filter does not mix
channels. And without channel layout information (i.e. given only the
total number of inputs, but not even their order), it is impossible to
mix channels meaningfully.

See above.



Also, please make sure that, in the callbacks, the filter handle
always comes as the first parameter.

No problem, but usually in pulseaudio the userdata pointer
comes last in callbacks.

Ok, ok, the real idea was "be consistent" :)


Well, things are still at a very early stage and surely I will do
some cleanup once the general approach is accepted.




Regarding the virtual surround plugin, I actually have one more
business argument why, if implemented properly (and not how it is done
currently) it should be out of scope. The reason is that this plugin
with publicly available HRIRs only applies to headphones. From a
usability perspective, it should be switched off (or to a different
filter, specifically crafted for each set of speakers and the
listening room) when the audio is playing to something else than
headphones. The proposed interface does not have (and likely should
not have, because we want to keep it simple and stable) any place to
express such policy.

Why would that be a problem? You can have a boolean bypass
parameter that switches off the filter when needed. Or even a
choice that switches between different HRIR files.

Who would switch it (the virtual surround effect, I am not talking
about other effects) off? The idea is that it is not the user (because
with this particular effect it should really be automatic), and is not
the filter itself (because it cannot know, based on your interface,
that it is playing to speakers). So it has to be some third entity,
with enough knowledge both about the filter and the PulseAudio notion
of ports (to distinguish between speakers, headphones, and something
unknown). And, given that entity, it does not really make sense to
abstract the virtual surround implementation behind some common plugin
interface, it has already leaked.

As an example of prior art, please also consider the LFE channel
remixer - it is not implemented as a virtual sink precisely for this
reason.

As said above, there are parameters that can be passed to
the filter if needed and module-plugin-sink could propagate
changes in the playback device to the filter that way.
We could reserve special parameter names for this information,
because these controls would not be exposed to the user.

If you think channel layout and playback device should be
in the interface, I can integrate it, but using parameters looks
easier to me because this way we need no additional notification
functions.
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss