ALSA CTL controls for codec and softvol selection

[borine]

[arkq]

When reviewing this PR and playing a little bit with it I thought about some simplification regarding control element names. Especially in regards to battery element. I really do not like the "|" character in the name. The function of this character is to push the battery element to the very end of the element list (in amixer and alsamixer). After some thinking I come to the conclusion that maybe we should drop the "|" character - what the heck, battery will appear in the middle of the list, not a big deal especially when the battery element is disabled by default. But later I thought that maybe it will be possible to add "bluealsa" support to alsa-lib (all in hands of alsa-lib maintainers) :) So, I've prepared a small patch: https://github.com/Arkq/alsa-lib/commit/462006b5514987d607e050165503a80f8b82c63a (it's not send for review yet) for alsa-lib and I've made some changes to bluealsa control plugin: https://github.com/Arkq/bluez-alsa/compare/tests I've reversed the order of BT profile prefix and BT device name. With such change alsa-lib will be able to sort elements correctly with +4 lines of code. The downside of such change is backward compatibility of some potential scripts which some users might be using for automation...

@borine what do you think about such change?

In regards to this PR please don't worry. I will resolve merge conflicts on my own (in case if the tests branch will be merged to master).

[arkq]

Just to let you know, I've resolved conflicts by rebasing this PR on top of master.

[arkq]

There was something wrong with control names for 'sofvol'. In your original PR (before I've rebased in on current master - before https://github.com/Arkq/bluez-alsa/commit/f37840621a8b3c57ece9b66e923c68cbfaee5ed5 there was a bug which allowed to write one more byte into the name than the alsa-lib was able to handle) the output of amixer was like this:

Simple mixer control 'Jabra MOVE v2.3 - A2DP Codec',0
  Capabilities: enum
  Items: 'AAC' 'SBC'
  Item0: 'SBC'
Simple mixer control 'Jabra MOVE v2.3. - SCO Codec',0
  Capabilities: enum
  Items: 'CVSD'
  Item0: 'CVSD'
Simple mixer control 'Jabra MOVE v2.3.0 - A2DP',0
  Capabilities: pvolume pswitch
  Playback channels: Front Left - Front Right
  Limits: Playback 0 - 127
  Mono:
  Front Left: Playback 64 [50%] [-9.75dB] [on]
  Front Right: Playback 64 [50%] [-9.75dB] [on]
Simple mixer control 'Jabra MOVE v2.3.0 - A2DP Softvol P Playback',0
  Capabilities: enum
  Items: 'disabled' 'enabled'
  Item0: 'disabled'
Simple mixer control 'Jabra MOVE v2.3.0 - SCO',0
  Capabilities: pvolume pvolume-joined cvolume cvolume-joined pswitch pswitch-joined cswitch cswitch-joined
  Playback channels: Mono
  Capture channels: Mono
  Limits: Playback 0 - 15 Capture 0 - 15
  Mono: Playback 10 [67%] [-5.82dB] [on] Capture 15 [100%] [0.00dB] [on]
Simple mixer control 'Jabra MOVE v2.3.0 - SCO Softvol C Capture E',0
  Capabilities: enum
  Items: 'disabled' 'enabled'
  Item0: 'disabled'
Simple mixer control 'Jabra MOVE v2.3.0 - SCO Softvol P Playback ',0
  Capabilities: enum
  Items: 'disabled' 'enabled'
  Item0: 'disabled'
Simple mixer control 'Jabra MOVE v2.3.0 | Battery',0
  Capabilities: pvolume pvolume-joined
  Playback channels: Mono
  Limits: Playback 0 - 100
  Mono: Playback 88 [88%]

After fixing the space reservation for actual name, the output looks like this:

Simple mixer control 'Jabra MOVE - A2DP',0
  Capabilities: pvolume pswitch
  Playback channels: Front Left - Front Right
  Limits: Playback 0 - 127
  Mono:
  Front Left: Playback 64 [50%] [-9.75dB] [on]
  Front Right: Playback 64 [50%] [-9.75dB] [on]
Simple mixer control 'Jabra MOVE - A2DP Codec',0
  Capabilities: enum
  Items: 'AAC' 'SBC'
  Item0: 'SBC'
Simple mixer control 'Jabra MOVE - A2DP Softvol P',0
  Capabilities: penum
  Items: 'disabled' 'enabled'
  Item0: 'disabled'
Simple mixer control 'Jabra MOVE - SCO',0
  Capabilities: pvolume pvolume-joined cvolume cvolume-joined pswitch pswitch-joined cswitch cswitch-joined
  Playback channels: Mono
  Capture channels: Mono
  Limits: Playback 0 - 15 Capture 0 - 15
  Mono: Playback 10 [67%] [-5.82dB] [on] Capture 15 [100%] [0.00dB] [on]
Simple mixer control 'Jabra MOVE - SCO Codec',0
  Capabilities: enum
  Items: 'CVSD'
  Item0: 'CVSD'
Simple mixer control 'Jabra MOVE - SCO Softvol C',0
  Capabilities: cenum
  Items: 'disabled' 'enabled'
  Item0: 'disabled'
Simple mixer control 'Jabra MOVE - SCO Softvol P',0
  Capabilities: penum
  Items: 'disabled' 'enabled'
  Item0: 'disabled'
Simple mixer control 'Jabra MOVE | Battery',0
  Capabilities: pvolume pvolume-joined
  Playback channels: Mono
  Limits: Playback 0 - 100
  Mono: Playback 88 [88%]

In order to keep device name consistent among all control we've got only 10 characters for the name. Plain BT address needs 17 bytes (00:00:00:00:00:00). I know that it might be tough nut but I'd like to be able to have at least 17 bytes for the name, so the BT address will not be truncated... Or maybe name reservation should depend on enabled plug-in features. So, by default we will reserve space only for " - A2DP" suffix, but when the EXT is enable we will reserve more space. The downside of this is that the name of a volume control will vary based on selected features. So, to be honest I don't know how to handle that :D

[borine]

In answer to your earlier question, I think it is not essential for Battery to appear last. I do like the idea of ALSA simple controls attaching a priority to "A2DP" and "SCO" if you can persuade alsa-project to accept that. I also think that changing the naming format will be OK so long as there is time between the change and the next release so that users are able to update scripts.

The downside of this is that the name of a volume control will vary based on selected features. So, to be honest I don't know how to handle that

So long as the plugins manual page describes this behaviour correctly so that users know hat to expect, then that is the best that can be done. If a script opens the CTL in a consistent way then it will see consistent name formats, so that should be fine.

BTW I've been experimenting with user controls created by the softvol pcm plugin, to see what happens when they are added/removed as a comparison with adding/removing bluealsa pcms. (I've just discovered the alsactl clean command which makes removing user controls so easy, previously I always thought a reboot was necessary!). When a user control is deleted, the numids of other controls remain unchanged, leaving a "gap" in the sequence. If the control is re-created, then it gets a new numid (the next available in the sequence) and the old numid remains unused, so the numid sequence remains non-contiguous.

Unfortunately, the external control plugin always creates a contiguous sequence of numids so it is impossible for an external plugin to replicate this behaviour of the hw ctl plugin in this respect. So I am thinking a manual section highlighting the significant differences in behaviour between bluealsa controls and hw controls may be a worthwhile addition.

[arkq]

I do like the idea of ALSA simple controls attaching a priority to "A2DP" and "SCO" if you can persuade alsa-project to accept that.

I think that such thing would be most generic. However, adding "A2DP" and "SCO" to the list in alsa-lib might not be ideal. I think that better idea is to (somehow) tell alsa-lib to use different default sorting algorithm if control plug-in elements are already sorted (if low-level API returns already sorted elements). I've been trying to implement such thing, but that would require some changes in ALSA control structure which is shared between kernel and userspace. So, this would lead to ABI version bump, which might not be welcomed by alsa-lib maintainers. Or maybe I will think of some other approach. I will push my WIP to some branch on my alsa-lib fork.

Unfortunately, the external control plugin always creates a contiguous sequence of numids so it is impossible for an external plugin to replicate this behaviour of the hw ctl plugin in this respect. So I am thinking a manual section highlighting the significant differences in behaviour between bluealsa controls and hw controls may be a worthwhile addition.

Yes, that is other place which maybe could be fixed/enhanced in alsa-lib. Maybe alsa-lib shall allow external control plug-in to specify numid's by itself.

Also, I've got some more thoughts regarding this PR. At first I thought that maybe it would be easier to pull only codec switch for now. I was thinking that maybe there is some other way to workaround the issue with softvol name being the same for playback and capture (other then adding P/C suffixes). But it has struck me that codec has got the same issue (and not only that one...). The case is that a single device might have 4 A2DP stream... with 2 different codecs... :D A2DP codes is per Bluetooth transport, which can be sink and source. So, it is possible to connect two bluealsa-based devices together in both directions using -p a2dp-sink -p a2dp-source. But on top of that there is a possibility of bi-directional codecs :D So, let say that we will use FastStream codec in both direction. This will lead to:

bluealsa-cli list-pcms
/org/bluealsa/hci0/dev_1C_48_F9_9D_81_5C/a2dpsrc/sink
/org/bluealsa/hci0/dev_1C_48_F9_9D_81_5C/a2dpsrc/source
/org/bluealsa/hci0/dev_1C_48_F9_9D_81_5C/a2dpsnk/sink
/org/bluealsa/hci0/dev_1C_48_F9_9D_81_5C/a2dpsnk/source

And such scenario will break current #master of bluealsa-ctl :D

[borine]

And such scenario will break current #master of bluealsa-ctl

Hmm, could BlueALSA treat such case as if it were 2 different devices with same alias? So in case where only one role is connected we have no name suffix, but when both SRC and SNK are connected we suffix #1 and #2, perhaps SNK gets #1 and SRC gets #2? If two different physical devices with same alias both connect both roles, then we get suffices #1,#2, #3, #4 ?

I think such use case is likely to be very rare - the vast majority of remote devices are either headsets or phones/music players, not both at the same time. So BlueALSA support should not compromise the presentation of the most common cases in order to provide support for very rare, possibly theoretical cases.

[arkq]

I think such use case is likely to be very rare - the vast majority of remote devices are either headsets or phones/music players, not both at the same time. So BlueALSA support should not compromise the presentation of the most common cases in order to provide support for very rare, possibly theoretical cases.

Yes, I totally agree with you :) I've created a test case for such scenario in the test suite (it's here: https://github.com/Arkq/bluez-alsa/tree/fs-rare-use-case). But right now I will have to think of a way how to handle that case, so the test will pass. I thought about adding prefix/suffix for A2DP back-channel streams. This will keep current behavior as it is now, but will handle current name duplication correctly. The downside of that is different naming for the same BT connection (and that I do not like, but maybe it's a lesser evil). But also I'm struggling how such prefix/suffix should look like :D (naming things is the hardest part of programmers life)

Run ./test/test-alsa-ctl tests or with updated bluealsa-mock, you can test such scenario with real mixer app as follows:

In one terminal run bluealsa-mock:

./test/bluealsa-mock -t 3600000 -p a2dp-source -p a2dp-sink -c faststream

In the other terminal run:

# export DBUS_SYSTEM_BUS_ADDRESS printed by bluealsa-mock
export DBUS_SYSTEM_BUS_ADDRESS=unix:abstract=/tmp/dbus-xxxxxxxxxxxxxx
# test CTL plugin with some real app, e.g.:
amixer -D bluealsa 

EDIT: Of course you will have to enable faststream support during configuration.

[arkq]

@borine, I've updated https://github.com/Arkq/bluez-alsa/tree/fs-rare-use-case branch. I think that I've made my mind how to handle name duplications for this PR and for FastStream case :D. I think that the lesser evil is to add some meaningful suffixes to control element names in exchange for the length of the BT device name. And since such cases are rare, I've made such suffixes optional. So, I think that everyone will be happy. What do you think about this new BTT option?

[borine]

I am struggling with this at the moment. 2 issues, I think not directly related.

1. So far I have been unable to achieve simultaneous SRC and SNK faststream connections from the same device. I'm testing bluealsa<->bluealsa. I'll do a complete re-build and try again next week.

2. When I connect the remote device, the bluetooth daemon emits a stream of Glib-CRITICAL errors:

(process:103297): GLib-CRITICAL **: 10:41:16.742: g_variant_new_variant: assertion 'value != NULL' failed

(process:103297): GLib-CRITICAL **: 10:41:16.742: g_variant_get_type: assertion 'value != NULL' failed

(process:103297): GLib-CRITICAL **: 10:41:16.742: g_variant_type_is_subtype_of: assertion 'g_variant_type_check (type)' failed

(process:103297): GLib-CRITICAL **: 10:41:16.742: g_variant_builder_add_value: assertion '!GVSB(builder)->expected_type || g_variant_is_of_type (value, GVSB(builder)->expected_type)' failed

(process:103297): GLib-CRITICAL **: 10:41:16.743: g_variant_builder_end: assertion 'GVSB(builder)->offset >= GVSB(builder)->min_items' failed

(process:103297): GLib-CRITICAL **: 10:41:16.743: g_variant_get_type: assertion 'value != NULL' failed

(process:103297): GLib-CRITICAL **: 10:41:16.743: g_variant_type_is_subtype_of: assertion 'g_variant_type_check (type)' failed

(process:103297): GLib-CRITICAL **: 10:41:16.743: g_variant_builder_add_value: assertion '!GVSB(builder)->expected_type || g_variant_is_of_type (value, GVSB(builder)->expected_type)' failed

(process:103297): GLib-CRITICAL **: 10:41:16.743: g_variant_new_variant: assertion 'value != NULL' failed

(process:103297): GLib-CRITICAL **: 10:41:16.743: g_variant_get_type: assertion 'value != NULL' failed

(process:103297): GLib-CRITICAL **: 10:41:16.744: g_variant_type_is_subtype_of: assertion 'g_variant_type_check (type)' failed

(process:103297): GLib-CRITICAL **: 10:41:16.744: g_variant_builder_add_value: assertion '!GVSB(builder)->expected_type || g_variant_is_of_type (value, GVSB(builder)->expected_type)' failed

(process:103297): GLib-CRITICAL **: 10:41:16.744: g_variant_builder_end: assertion 'GVSB(builder)->offset >= GVSB(builder)->min_items' failed

(process:103297): GLib-CRITICAL **: 10:41:16.744: g_variant_get_type: assertion 'value != NULL' failed

(process:103297): GLib-CRITICAL **: 10:41:16.744: g_variant_type_is_subtype_of: assertion 'g_variant_type_check (type)' failed

(process:103297): GLib-CRITICAL **: 10:41:16.744: g_variant_builder_add_value: assertion '!GVSB(builder)->expected_type || g_variant_is_of_type (value, GVSB(builder)->expected_type)' failed

I have no idea what these are yet, possibly also an error in my build or installation.

[arkq]

So far I have been unable to achieve simultaneous SRC and SNK faststream

Hmm, I'll check that on my own as well. Maybe there is some regression in current master :/

When I connect the remote device, the bluetooth daemon emits a stream of Glib-CRITICAL errors:

This will allow you to debug this:

G_DEBUG=fatal_warnings gdb --args <COMMAND>

Also, I think that finally I've finished cherry-picking codec selection from this PR: https://github.com/Arkq/bluez-alsa/tree/control-codec I'm still doing some tests, but I think that it's ready to be pushed to master.

EDIT: Nope, it's not ready :D There is still some peculiar bug in there.

1. Connect headset with A2DP and HFP (with mSBC support)
2. Run alsamixer -V all -D bluealsa:EXT=yes
3. Change SCO codec to CVSD (and keep alsamixer running)
4. Disconnect HFP profile: gdbus call --system -d org.bluez -o /org/bluez/hci0/dev_1C_48_F9_9D_81_5C -m org.bluez.Device1.DisconnectProfile "0000111e-0000-1000-8000-00805f9b34fb"
5. SCO PCM is not removed from mixer... (it works fine when codec is not changed)

EDIT2: The bug is in the master branch, but it has showed up when using enum element :)

[arkq]

So far I have been unable to achieve simultaneous SRC and SNK faststream connections from the same device.

I've just tested that on Ubuntu 20.04 and indeed it seems that BlueZ is not able to handle that properly...

bluetoothd[1666809]: Could not register remote sep /org/bluez/hci1/dev_00_1A_7D_DA_71_15/sep7
bluetoothd[1666809]: Could not register remote sep /org/bluez/hci1/dev_00_1A_7D_DA_71_15/sep8
kernel: bluetoothd[1666809]: segfault at 3 ip 000055e745955032 sp 00007ffce9288f30 error 4 in bluetoothd[55e74594d000+9b000]

[borine]

The GIO critical message was introduced by commit f7da96296bf1180b313d1fa3776c5506c977831f

This patch appears to fix it (only very limited testing done though):

diff --git a/src/bluealsa-dbus.c b/src/bluealsa-dbus.c
index 0d245c6..ac349c7 100644
--- a/src/bluealsa-dbus.c
+++ b/src/bluealsa-dbus.c
@@ -899,10 +899,16 @@ void bluealsa_dbus_pcm_update(struct ba_transport_pcm *pcm, unsigned int mask) {
 		g_variant_builder_add(&props, "{sv}", "Channels", ba_variant_new_pcm_channels(pcm));
 	if (mask & BA_DBUS_PCM_UPDATE_SAMPLING)
 		g_variant_builder_add(&props, "{sv}", "Sampling", ba_variant_new_pcm_sampling(pcm));
-	if (mask & BA_DBUS_PCM_UPDATE_CODEC)
-		g_variant_builder_add(&props, "{sv}", "Codec", ba_variant_new_pcm_codec(pcm));
-	if (mask & BA_DBUS_PCM_UPDATE_CODEC_CONFIG)
-		g_variant_builder_add(&props, "{sv}", "CodecConfiguration", ba_variant_new_pcm_codec_config(pcm));
+	if (mask & BA_DBUS_PCM_UPDATE_CODEC) {
+		GVariant *value = ba_variant_new_pcm_codec(pcm);
+		if (value != NULL)
+			g_variant_builder_add(&props, "{sv}", "Codec", value);
+	}
+	if (mask & BA_DBUS_PCM_UPDATE_CODEC_CONFIG) {
+		GVariant *value = ba_variant_new_pcm_codec_config(pcm);
+		if (value != NULL)
+			g_variant_builder_add(&props, "{sv}", "CodecConfiguration", value);
+	}
 	if (mask & BA_DBUS_PCM_UPDATE_DELAY)
 		g_variant_builder_add(&props, "{sv}", "Delay", ba_variant_new_pcm_delay(pcm));
 	if (mask & BA_DBUS_PCM_UPDATE_SOFT_VOLUME)

[arkq]

The GIO critical message was introduced by commit https://github.com/Arkq/bluez-alsa/commit/f7da96296bf1180b313d1fa3776c5506c977831f

Yes, my bad... Recently I've been using bluealsa with systemd unit file and I've missed these warning messages.... :/ I've tested changes manually but only with A2DP and HSP....

EDIT: Fix is in master branch 5d9a6dc. I've fixed it little bit differently. The omission of "if"s in the update function was intentional. Sending update for codec which is not set (empty update) is rather pointless (there is no case in bluealsa code where the codec could be "deinitialized"). In fact the issue was that the RFCOMM state machine was sending codec update even though the codec was not selected during SLC establishment.

[arkq]

I've rebased this PR on top of current master.

I still don't like the idea of sacrificing BT device name for " Softvol C" label :/ But maybe there is no other way here.... Maybe, just maybe the label could be litttttle bit shorter :D I will think about that for few days. If nothing will pop in my mind I will merge this PR in the current form :)

PS. @borine Many, many thanks for maintaining wiki section. The articles are of excellent quality. In comparison with wiki, current README looks like a garbage :D I don't know whether it's not too much to ask you for, but could you try to revise README as well? Maybe even it could be shorter in favor of wiki pages? To be honest I don't even have a "vision" how the README could look like... :/

[borine]

maybe the label could be litttttle bit shorter

Some ideas:

1. Change the control type to switch instead of enum; then the same name can be used for both capture and playback. The problem is that alsamixer then renders it as a "mute" switch, which I thought very confusing and misleading.

2. Change the softvol control name to "SVp" and "SVc". I think that is OK, others may disagree?

3. Change the separator for all controls from " - " to " ". That also works for me.

could you try to revise README

Actually I think the README is good, but I'll have a think about it over the coming weeks and raise a PR if I get any new ideas.

[arkq]

Change the control type to switch instead of enum;

Yes, I've been experimenting with that, but rendered controls are rather misleading (like you've said). The "mute" checkbox is OK, but the problem is with capture, it is rendered as "capture on/off" control, which is not OK.

Change the softvol control name to "SVp" and "SVc". I think that is OK, others may disagree?

That's exactly what I've done by myself :D But I'm not sure whether the name "SVp" (or "SVP" or "SV P") will not be too cryptic.

Also, I've been experimenting with different values like: { "pass-through", "software" } and then the label might be cryptic, by the value will be more or less explanatory.

[borine]

It turns out we can use the element index for softvol controls instead of having different names for playback and capture. I've added a commit that demonstrates this. It uses the name "Mode" for the softvol control, with your suggested values.

This still leaves us two characters short to fit in a BT address. So perhaps the name "SV" might suffice, or else consider dropping the "-" separator?

[arkq]

Indeed it looks promising. Anyway, I'm not sure where the index is used in hardware mixers. Is it used for such cases?

Also, when looking at that I've got somehow mixed fillings... :D maybe the approach with P/C was be better. The problem here is that it's hard to tell whether the control is for playback or for capture (when using "all" (F5) view of alsamixer, it's OK for playback/capture view).

[borine]

I'm not sure where the index is used in hardware mixers. Is it used for such cases?

The only example I have is an HDA Intel PCH card. This has 5 HDMI PCM devices, numbered 0-4. Each of these has a mute switch, for which amixer scontrols shows:

Simple mixer control 'IEC958',0
Simple mixer control 'IEC958',1
Simple mixer control 'IEC958',2
Simple mixer control 'IEC958',3
Simple mixer control 'IEC958',4

and amixer controls shows:

numid=22,iface=MIXER,name='IEC958 Playback Switch'
numid=28,iface=MIXER,name='IEC958 Playback Switch',index=1
numid=34,iface=MIXER,name='IEC958 Playback Switch',index=2
numid=40,iface=MIXER,name='IEC958 Playback Switch',index=3
numid=46,iface=MIXER,name='IEC958 Playback Switch',index=4

So the index is used in much the same way as we have for softvol here. The same name is used for each control, and the index indicates which PCM it is bound to. Incidentally, alsamixer renames "IEC958" as "S/PDIF", even though this card does not have any S/PDIF ports, only HDMI.

The problem here is that it's hard to tell whether the control is for playback or for capture

I've changed the indexing so that playback is always index 0, and capture is always index 1. So alsamixer now shows "SCO Mode" for playback, and "SCO Mode 1" for capture.

[arkq]

Force pushed to resolved conflicts.