[pulseaudio-discuss] [PATCH v5 2/4] bluetooth: separate HSP and HFP

Sat Sep 30 23:10:41 UTC 2017

Good evening.

I found the problem with the distorted microphone audio.

According to the HFP 1.6 spec, if a device supports remote volume
control, only the speaker is mandatory.

It happens that my HF headset does not support setting the microphone
volume, only the speaker. So the volume was too loud and was clipping
(I'm using Fedora, where flat_volumes defaults to "no").

But anyway, the spec says that remote volume control isn't mandatory at
all, so the same problem must be present for the speaker on HF devices
that do not support it.

So I've added some flags on the hfp_config struct and made some checks.
By default, we assume software volume control. When the HF sends us its
features during negotiation, if it supports remote volume control then
we disable software VC for the speaker, but we keep assuming software VC
for the mic unless we receive a unsolicited +VGM at some point.

Then, when setting the volume, we check those flags and act accordingly.

A consequence is that struct hfp_config must be known by
module-bluez5-device so its declaration went to one of the header files
(I think we'll need it there anyway later when we start adding flags for
wide-band-speech, but more on that later).

I also added some other fixes, mostly related to gain control but some
minor ones related to better HFP 1.6 compliance.

I really can't tell which changes should go to James' patches, so I'm
attaching a full patch with all of my modifications (including the
previous one that fixes auto_switch) in hope it will be useful. James,
could you take a look and if all if fine, merge these changes with
yours? As soon as you have a new version ready of your patches, I'm
willing to test them.

Also, should you also need help implementing the suggestions/fixes given
by Georg, please say so and I'll try to take some time to help. For
instance, I agree the new naming conventions need a bit of rethinking,
because we are implementing a HFP AG but the constants we are using
sometimes refer to HFP_HF or HFP_AG inconsistently. So feel free to tell
me should you need anything.

I've used this modified version with success with my HFP only headset,
in applications like skype and linphone. Will test it further during the
next week and will report any fixes I find needed, but for now I think
I'm happy with it on a "works for me" base. However several other
devices can still have issues due to supporting or not the
aforementioned features, so to the anyone reading this, all feedback is
welcome and will certainly help to make native HFP AG support official
in pulseaudio (and consequently, easily supported in all recent Linux
distributions).

Now, regarding wide band speech that I mentioned earlier: I started
implementing the codec negotiation and SDP advertising, and I think I
got it done (in the end, it's not too complicated when you find the
right screw), but only when I was about to test it was that I realized
that my headset has only HFP version 1.5. So I can't really test the
basics, much less starting to mess with the transport using mSBC. For
what I've seen though, it shouldn't be much hard to implement the rest,
as the sbc library already supports mSBC so it seems to the part I'm now
missing should be a variation of part of the A2DP code already present
(but don't just take my word for it).

But since I can't test anything related to that (I lack a premium HFP
headset, I suppose) I've decided not to proceed with it, at least until
we get the all the basics right and these patches are ready be official.
So to avoid confusion, the related changes I've made to support WBS
aren't in the attached patch, but should anyone having a HFP device that
supports it want them to proceed with the rest of the implementation,
feel free to ask (preferably on a new thread, I think).

Best regards.

On 09/26/2017 11:54 AM, Rodrigo Araujo wrote:
> Hello again.
>
> On 09/25/2017 03:30 PM, Rodrigo Araujo wrote:
>> Hi.
>>
>> On 09/25/2017 02:37 PM, James Bottomley wrote:
>>> On Sat, 2017-09-23 at 23:33 +0100, Rodrigo Araujo wrote:
>>>> Hi.
>>>>
>>>> First, just to say that your patches are going great. Finally I can
>>>> use
>>>> the microphone of my HFP only headset (a version of a Bluedio T2+).
>>>>
>>>> So far, I've only encontered one problem: the auto_switch option of
>>>> module_bluetooth_policy stops working. Dug through the code and I
>>>> think
>>>> you missed a few spots were you have to hangle the new
>>>> headset_handsfree
>>>> profile in module_bluetooth_policy.c
>>>>
>>>> Applying the following after applying your v5 patches fixed the issue
>>>> for me, now when I start making a VOIP call the profile switches to
>>>> headset_handsfree and the mic works automatically, and when the call
>>>> finishes it reverts back to a2dp.
>>> That's great, thanks!  Sad to say I didn't think automatic profile
>>> switching worked in pulseaudio, so I hadn't thought to investigate it.
>>> Did you want me to merge this into patch 2 or add it as a new patch
>>> with you as the author?
>>>
>>> James
>> Seems to me it makes sense to merge into patch 2. Would simplify things
>> also, too many patches and lots of work for you guys already ;)
>>
>> If you want to mention my name as contributor in that patch, also go
>> ahead :)
>>
>> Best regards.
> Meanwhile I found another issue: setting the volume (gain) of the
> speaker and microphone doesn't work. It's easily reproducible when using
> pavucontrol.
>
> This happens because of a clash with the HS role, and because we are a
> HFP AG we are falling on the reversed logic and sending mic commands
> when we want to send speaker commands, and vice-versa.
>
> The following patch is simple and fixes that:
>
> --- a/src/modules/bluetooth/backend-native.c	2017-09-23 23:12:35.220031951 +0100
> +++ b/src/modules/bluetooth/backend-native.c	2017-09-25 22:18:29.094127139 +0100
> @@ -565,7 +565,8 @@
>      /* If we are in the AG role, we send a command to the head set to change
>       * the speaker gain. In the HS role, source and sink are swapped, so
>       * in this case we notify the AG that the microphone gain has changed */
> -    if (t->profile == PA_BLUETOOTH_PROFILE_HSP_HS) {
> +    if (t->profile == PA_BLUETOOTH_PROFILE_HSP_HS ||
> +	t->profile == PA_BLUETOOTH_PROFILE_HFP_HF) {
>          len = sprintf(buf, "\r\n+VGS=%d\r\n", gain);
>          pa_log_debug("RFCOMM >> +VGS=%d", gain);
>      } else {
> @@ -592,7 +593,8 @@
>      /* If we are in the AG role, we send a command to the head set to change
>       * the microphone gain. In the HS role, source and sink are swapped, so
>       * in this case we notify the AG that the speaker gain has changed */
> -    if (t->profile == PA_BLUETOOTH_PROFILE_HSP_HS) {
> +    if (t->profile == PA_BLUETOOTH_PROFILE_HSP_HS ||
> +	t->profile == PA_BLUETOOTH_PROFILE_HFP_HF) {
>          len = sprintf(buf, "\r\n+VGM=%d\r\n", gain);
>          pa_log_debug("RFCOMM >> +VGM=%d", gain);
>      } else {
>
>
> Can you also merge that in you patch? :)
>
> This fixes when we explicitly set the volume, but also should help when
> the system tries to set it because it needs to.
>
> However, audio quality still leaves to be desired when compared with
> what is achieved using the HSP role (the voice is still too "metallic"
> and with excessively poor quality). This will surely be fixed when
> implementing wide band audio, but still should be better as it is now
> since the audio encoding is mostly the same. I will try to look into
> into too and if I find that it has to do with any of the other patches,
> I will reply to the respective thread. I will also see if I can help
> with implementing wide band audio, but not I'm promising anything since
> this is not my area.
>
> Best regards and keep up the good work.
> Rodrigo Araujo

-------------- next part --------------
A non-text attachment was scrubbed...
Name: pulseaudio-v5-extra-bluetooth-fix-auto_switch-gain-softvolume-better_hfp_compliance.patch
Type: text/x-patch
Size: 10627 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20171001/7336b69d/attachment-0001.bin>