[pulseaudio-discuss] [PATCH v4 0/3] use bluetooth HFP in pulseaudio when available

Georg Chini georg at chini.tk
Thu Sep 21 18:16:40 UTC 2017


On 21.09.2017 19:08, James Bottomley wrote:
> On Thu, 2017-09-21 at 19:15 +0300, Tanu Kaskinen wrote:
>> On Thu, 2017-09-21 at 17:47 +0200, Georg Chini wrote:
>>> On 21.09.2017 16:47, James Bottomley wrote:
>>>> On Thu, 2017-09-21 at 17:28 +0300, Tanu Kaskinen wrote:
>>>>> I think we should use the native backend for the HFP AG role by
>>>>> default. If the native HFP AG implementation causes a conflict
>>>>> with ofono in its default configuration (which seems likely to
>>>>> be the case), then "headset=auto" should not enable the native
>>>>> HFP AG implementation, which then means that we should use
>>>>> "headset=native" by default.
>>>>>
>>>>> Using "headset=native" by default means that we lose HFP HF
>>>>> support in the default configuration, but I don't think that's
>>>>> a big loss.
>>>> Setting the default to native works for me: it's basically what
>>>> all distros get today anyway because they don't install ofono by
>>>> default.   That probably means we don't need the elaborate
>>>> switching for HSP_AG either, but it would become harmless, so
>>>> could be removed later.
>>>>
>>>>> If we want to support the "HFP AG support through PA, HFP HF
>>>>> support through ofono" case, then some new configuration option
>>>>> is needed, and I think it would be clearest if that would be a
>>>>> separate patch after this series.
>>> This is not true, the patch supplies an option to enable/disable
>>> the HFP implementation. Simple usage of that switch would
>>> provide all the required functionality.
>> Yeah, sorry, I wasn't aware of that option, as I hadn't read the
>> patch. You two just seemed to be choosing between enabling or
>> disabling the native HFP AG implementation by default, and I just
>> wanted to say that I want it to be enabled by default, but without
>> breaking "headset=auto".
>>
>>> The default for the option would just be disabled in "auto" mode
>>> and enabled in "native" mode.
>> The option seems to be named "enable_profile_hfp_hf", which suggests
>> that disabling it will disable the ofono implementation of HFP AG as
>> well, and that's not what we want. Maybe rename the option to
>> "enable_native_hfp_hf"?
> I updated the current patch to rename the option and condition it on
> the backend setting (see below.  Patch would need integrating into the
> series, since the option rename needs to go back into patch 2 but it
> gives you the idea)
>
>>> There is not a big code change needed, only a check
>>> if the headset mode is "auto" or "native" and changing the
>>> default accordingly.
>>> The above is the essence of what I proposed to solve the
>>> issue with headset=auto and I really don't understand why
>>> this is causing such discussions. Leaving it as is definitely
>>> breaks "headset=auto".
>> Ok, sounds fine, with the added note that we should set
>> "headset=native" by default.
> OK, something like this
>
> James
>
> ---
>
> diff --git a/src/modules/bluetooth/backend-native.c b/src/modules/bluetooth/backend-native.c
> index 16b2aa6c..c3a79d75 100644
> --- a/src/modules/bluetooth/backend-native.c
> +++ b/src/modules/bluetooth/backend-native.c
> @@ -822,7 +822,7 @@ pa_bluetooth_backend *pa_bluetooth_native_backend_new(pa_core *c, pa_bluetooth_d
>       if (enable_hs_role)
>          profile_init(backend, PA_BLUETOOTH_PROFILE_HFP_AG);
>       profile_init(backend, PA_BLUETOOTH_PROFILE_HSP_HS);
> -    if (pa_bluetooth_discovery_get_enable_profile_hfp_hf(y))
> +    if (pa_bluetooth_discovery_get_enable_native_hfp_hf(y))
>           profile_init(backend, PA_BLUETOOTH_PROFILE_HFP_HF);
>   
>       return backend;
> diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c
> index 6c77113e..8be8a11d 100644
> --- a/src/modules/bluetooth/bluez5-util.c
> +++ b/src/modules/bluetooth/bluez5-util.c
> @@ -92,7 +92,7 @@ struct pa_bluetooth_discovery {
>       int headset_backend;
>       pa_bluetooth_backend *ofono_backend, *native_backend;
>       PA_LLIST_HEAD(pa_dbus_pending, pending);
> -    bool enable_profile_hfp_hf;
> +    bool enable_native_hfp_hf;
>   };
>   
>   static pa_dbus_pending* send_and_add_to_pending(pa_bluetooth_discovery *y, DBusMessage *m,
> @@ -173,11 +173,11 @@ 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) {
> -    bool show_hfp, show_hsp, enable_profile_hfp_hf;
> +    bool show_hfp, show_hsp, enable_native_hfp_hf;
>   
> -    enable_profile_hfp_hf = pa_bluetooth_discovery_get_enable_profile_hfp_hf(device->discovery);
> +    enable_native_hfp_hf = pa_bluetooth_discovery_get_enable_native_hfp_hf(device->discovery);
>   
> -    if (enable_profile_hfp_hf) {
> +    if (enable_native_hfp_hf) {
>           show_hfp = pa_hashmap_get(device->uuids, PA_BLUETOOTH_UUID_HFP_HF);
>           show_hsp = !show_hfp;
>       } else {
> @@ -553,12 +553,12 @@ pa_bluetooth_device* pa_bluetooth_discovery_get_device_by_path(pa_bluetooth_disc
>       return NULL;
>   }
>   
> -bool pa_bluetooth_discovery_get_enable_profile_hfp_hf(pa_bluetooth_discovery *y)
> +bool pa_bluetooth_discovery_get_enable_native_hfp_hf(pa_bluetooth_discovery *y)
>   {
>       pa_assert(y);
>       pa_assert(PA_REFCNT_VALUE(y) > 0);
>   
> -    return y->enable_profile_hfp_hf;
> +    return y->enable_native_hfp_hf;
>   }
>   
>   pa_bluetooth_device* pa_bluetooth_discovery_get_device_by_address(pa_bluetooth_discovery *y, const char *remote, const char *local) {
> @@ -1754,7 +1754,7 @@ static void endpoint_done(pa_bluetooth_discovery *y, pa_bluetooth_profile_t prof
>       }
>   }
>   
> -pa_bluetooth_discovery* pa_bluetooth_discovery_get(pa_core *c, int headset_backend, bool enable_profile_hfp_hf) {
> +pa_bluetooth_discovery* pa_bluetooth_discovery_get(pa_core *c, int headset_backend, bool enable_native_hfp_hf) {
>       pa_bluetooth_discovery *y;
>       DBusError err;
>       DBusConnection *conn;
> @@ -1764,7 +1764,7 @@ pa_bluetooth_discovery* pa_bluetooth_discovery_get(pa_core *c, int headset_backe
>       PA_REFCNT_INIT(y);
>       y->core = c;
>       y->headset_backend = headset_backend;
> -    y->enable_profile_hfp_hf = enable_profile_hfp_hf;
> +    y->enable_native_hfp_hf = enable_native_hfp_hf;
>       y->adapters = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func, NULL,
>                                         (pa_free_cb_t) adapter_free);
>       y->devices = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func, NULL,
> diff --git a/src/modules/bluetooth/bluez5-util.h b/src/modules/bluetooth/bluez5-util.h
> index 78c4f2fa..23f9a798 100644
> --- a/src/modules/bluetooth/bluez5-util.h
> +++ b/src/modules/bluetooth/bluez5-util.h
> @@ -166,5 +166,5 @@ pa_bluetooth_discovery* pa_bluetooth_discovery_get(pa_core *core, int headset_ba
>   pa_bluetooth_discovery* pa_bluetooth_discovery_ref(pa_bluetooth_discovery *y);
>   void pa_bluetooth_discovery_unref(pa_bluetooth_discovery *y);
>   void pa_bluetooth_discovery_set_ofono_running(pa_bluetooth_discovery *y, bool is_running);
> -bool pa_bluetooth_discovery_get_enable_profile_hfp_hf(pa_bluetooth_discovery *y);
> +bool pa_bluetooth_discovery_get_enable_native_hfp_hf(pa_bluetooth_discovery *y);
>   #endif
> diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
> index 3d362f4b..d37ce9ce 100644
> --- a/src/modules/bluetooth/module-bluez5-device.c
> +++ b/src/modules/bluetooth/module-bluez5-device.c
> @@ -2010,7 +2010,7 @@ static int add_card(struct userdata *u) {
>       pa_bluetooth_profile_t *p;
>       const char *uuid;
>       void *state;
> -    bool enable_profile_hfp_hf, has_both;
> +    bool enable_native_hfp_hf, has_both;
>   
>       pa_assert(u);
>       pa_assert(u->device);
> @@ -2041,13 +2041,13 @@ static int add_card(struct userdata *u) {
>   
>       create_card_ports(u, data.ports);
>   
> -    enable_profile_hfp_hf = pa_bluetooth_discovery_get_enable_profile_hfp_hf(u->discovery);
> +    enable_native_hfp_hf = pa_bluetooth_discovery_get_enable_native_hfp_hf(u->discovery);
>   
> -    has_both = enable_profile_hfp_hf && pa_hashmap_get(d->uuids, PA_BLUETOOTH_UUID_HFP_HF) && pa_hashmap_get(d->uuids, PA_BLUETOOTH_UUID_HSP_HS);
> +    has_both = enable_native_hfp_hf && pa_hashmap_get(d->uuids, PA_BLUETOOTH_UUID_HFP_HF) && pa_hashmap_get(d->uuids, PA_BLUETOOTH_UUID_HSP_HS);
>       PA_HASHMAP_FOREACH(uuid, d->uuids, state) {
>           pa_bluetooth_profile_t profile;
>   
> -        if (!enable_profile_hfp_hf && pa_streq(uuid, PA_BLUETOOTH_UUID_HFP_HF)) {
> +        if (!enable_native_hfp_hf && pa_streq(uuid, PA_BLUETOOTH_UUID_HFP_HF)) {
>               pa_log_info("device supports HFP but disabling profile as requested");
>               continue;
>           }
> diff --git a/src/modules/bluetooth/module-bluez5-discover.c b/src/modules/bluetooth/module-bluez5-discover.c
> index 52d848f0..b6e2803e 100644
> --- a/src/modules/bluetooth/module-bluez5-discover.c
> +++ b/src/modules/bluetooth/module-bluez5-discover.c
> @@ -93,7 +93,7 @@ static pa_hook_result_t device_connection_changed_cb(pa_bluetooth_discovery *y,
>   }
>   
>   #ifdef HAVE_BLUEZ_5_NATIVE_HEADSET
> -const char *default_headset_backend = "auto";
> +const char *default_headset_backend = "native";
>   #else
>   const char *default_headset_backend = "ofono";
>   #endif
> @@ -104,7 +104,7 @@ int pa__init(pa_module *m) {
>       const char *headset_str;
>       int headset_backend;
>       bool autodetect_mtu;
> -    bool enable_profile_hfp_hf = true;
> +    bool enable_native_hfp_hf;
>   
>       pa_assert(m);
>   
> @@ -125,12 +125,14 @@ int pa__init(pa_module *m) {
>           goto fail;
>       }
>   
> +    enable_native_hfp_hf = (headset_backend == HEADSET_BACKEND_NATIVE);
> +
>       autodetect_mtu = false;
>       if (pa_modargs_get_value_boolean(ma, "autodetect_mtu", &autodetect_mtu) < 0) {
>           pa_log("Invalid boolean value for autodetect_mtu parameter");
>       }
> -    if (pa_modargs_get_value_boolean(ma, "enable_profile_hfp_hf", &enable_profile_hfp_hf) < 0) {
> -        pa_log("enable_profile_hfp_hf must be true or false");
> +    if (pa_modargs_get_value_boolean(ma, "enable_native_hfp_hf", &enable_native_hfp_hf) < 0) {
> +        pa_log("enable_native_hfp_hf must be true or false");
>           goto fail;
>       }
>   
> @@ -140,7 +142,7 @@ int pa__init(pa_module *m) {
>       u->autodetect_mtu = autodetect_mtu;
>       u->loaded_device_paths = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
>   
> -    if (!(u->discovery = pa_bluetooth_discovery_get(u->core, headset_backend, enable_profile_hfp_hf)))
> +    if (!(u->discovery = pa_bluetooth_discovery_get(u->core, headset_backend, enable_native_hfp_hf)))
>           goto fail;
>   
>       u->device_connection_changed_slot =
> _______________________________________________
Yes, that looks OK to me. BTW, I did not yet look deeper into the code
but I noticed that you are calling profile_done() unconditionally. This will
break if the profile was never initialized.



More information about the pulseaudio-discuss mailing list