[RESEND] [PATCH v3 1/2] bluetooth: Add support for automatic switch between hsp and a2dp profiles

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

On Tue, 2016-01-19 at 09:29 +0100, Pali Rohár wrote:
> On Tuesday 19 January 2016 11:12:40 Arun Raghavan wrote:
> > On Wed, 2016-01-13 at 13:32 +0100, Pali Rohár wrote:
> > > With this patch module-bluetooth-policy automatically switch from
> > > a2dp profile
> > > to hsp profile if some VOIP application with media.role=phone
> > > wants
> > > to start
> > > recording audio.
> > > 
> > > By default a2dp profile is used for listening music, but for VOIP
> > > calls is
> > > needed profile with microphone support (hsp). So this patch will
> > > switch to
> > > hsp profile if some application want to use microphone (and
> > > specify
> > > it in
> > > media.role as "phone). After recording is stopped profile is
> > > switched
> > > back
> > > to a2dp. So this patch allows to use bluetooth microphone for
> > > VOIP
> > > applications
> > > with media.role=phone automatically without need of user
> > > interaction.
> > 
> > Thanks, I haven't tested this yet, but some quick comments.
> 
> Thanks for review, this is just resent v3 version (originally sent
> year
> and half ago) which was without response.

Sorry about that. It's been a challenge trying to keep on top of the
patch inflow.

> > > Signed-off-by: Pali Rohár <pali.rohar at gmail.com>
> > > ---
> > >  src/modules/bluetooth/module-bluetooth-policy.c |  200
> > > ++++++++++++++++++++++-
> > >  1 file changed, 198 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/modules/bluetooth/module-bluetooth-policy.c
> > > b/src/modules/bluetooth/module-bluetooth-policy.c
> > > index 860868f..200fc84 100644
> > > --- a/src/modules/bluetooth/module-bluetooth-policy.c
> > > +++ b/src/modules/bluetooth/module-bluetooth-policy.c
> > > @@ -35,16 +35,18 @@
> > >  
> > >  #include "module-bluetooth-policy-symdef.h"
> > >  
> > > -PA_MODULE_AUTHOR("Frédéric Dalleau");
> > > -PA_MODULE_DESCRIPTION("When a bluetooth sink or source is added,
> > > load module-loopback");
> > > +PA_MODULE_AUTHOR("Frédéric Dalleau, Pali Rohár");
> > > +PA_MODULE_DESCRIPTION("Automatically switch between bluetooth
> > > hsp
> > > and a2dp profiles and automatically load module-loopback when a
> > > bluetooth sink (ag) or source (ag, a2dp_source) is added");
> > 
> > I'd just rewrite this to be "Policy module to make using bluetooth
> > devices out-of-the-box easier".
> 
> Personally I'm fine with both descriptions so its up to other
> pulseaudio
> reviewers/maintainers.

I prefer the shorter name in the description, so let's go with that.

> > >  PA_MODULE_VERSION(PACKAGE_VERSION);
> > >  PA_MODULE_LOAD_ONCE(true);
> > >  PA_MODULE_USAGE(
> > > +        "switch=<Switch between hsp and a2dp profile?> "
> > 
> > Maybe call this "autoswitch".
> 
> "autoswitch" or "auto_switch"?

I'm not a huge fan of the underscores in our modarg names, but I'm fine
with either.

> > >          "a2dp_source=<Handle a2dp_source card profile (sink
> > > role)?>
> > > "
> > >          "ag=<Handle headset_audio_gateway card profile (headset
> > > role)?> "
> > >          "hfgw=<Handle hfgw card profile (headset role)?>
> > > DEPRECATED");
> > >  
> > >  static const char* const valid_modargs[] = {
> > > +    "switch",
> > >      "a2dp_source",
> > >      "ag",
> > >      "hfgw",
> > > @@ -56,6 +58,10 @@ struct userdata {
> > >      bool enable_ag;
> > >      pa_hook_slot *source_put_slot;
> > >      pa_hook_slot *sink_put_slot;
> > > +    pa_hook_slot *source_output_put_slot;
> > > +    pa_hook_slot *source_output_unlink_slot;
> > > +    pa_hook_slot *card_new_slot;
> > > +    pa_hook_slot *card_unlink_slot;
> > >      pa_hook_slot *profile_available_changed_slot;
> > >  };
> > >  
> > > @@ -139,6 +145,163 @@ static pa_hook_result_t
> > > sink_put_hook_callback(pa_core *c, pa_sink *sink, void *
> > >      return PA_HOOK_OK;
> > >  }
> > >  
> > > +/* Switch profile for one card */
> > > +static void switch_profile(pa_card *card, bool revert) {
> > 
> > From a readability perspective, I think it makes sense to call this
> > "a2dp" rather than "revert".
> 
> This whole patch is doing autoswitch to hsp mode. And revert param is
> reverting back from hsp back to a2dp.
> 
> So rather I propose name revert_to_a2dp.

Okay.

> > > +    const char *from_profile;
> > > +    const char *to_profile;
> > > +    pa_card_profile *profile;
> > > +    const char *s;
> > > +    void *state;
> > > +
> > > +    if (revert) {
> > > +        from_profile = "hsp";
> > > +        to_profile = "a2dp";
> > > +    } else {
> > > +        from_profile = "a2dp";
> > > +        to_profile = "hsp";
> > > +    }
> > > +
> > > +    /* Only consider bluetooth cards */
> > > +    s = pa_proplist_gets(card->proplist, PA_PROP_DEVICE_BUS);
> > > +    if (!s || !pa_streq(s, "bluetooth"))
> > > +        return;
> > > +
> > > +    if (revert) {
> > > +        /* In revert phase only consider cards with revert flag
> > > */
> > > +        s = pa_proplist_gets(card->proplist, "bluez-revert");
> > > +        if (!s || !pa_streq(s, "true"))
> > > +            return;
> > > +
> > > +        /* Remove revert flag */
> > > +        pa_proplist_unset(card->proplist, "bluez-revert");
> > > +    }
> > > +
> > > +    /* Skip card if does not have active from_profile */
> > > +    if (!pa_streq(card->active_profile->name, from_profile))
> > > +        return;
> > > +
> > > +    /* Skip card if already has active profile to_profile */
> > > +    if (pa_streq(card->active_profile->name, to_profile))
> > > +        return;
> > > +
> > > +    /* Find available to_profile and activate it */
> > > +    PA_HASHMAP_FOREACH(profile, card->profiles, state) {
> > > +        if (!pa_streq(profile->name, to_profile))
> > > +            continue;
> > > +
> > > +        if (profile->available == PA_AVAILABLE_NO)
> > > +            continue;
> > > +
> > > +        pa_log_debug("Setting card '%s' to profile '%s'", card-
> > > > name, to_profile);
> > > +
> > > +        if (pa_card_set_profile(card, profile, false) != 0) {
> > > +            pa_log_warn("Could not set profile '%s'",
> > > to_profile);
> > > +            continue;
> > > +        }
> > > +
> > > +        /* When we are not in revert phase flag this card for
> > > revert
> > > */
> > > +        if (!revert)
> > > +            pa_proplist_sets(card->proplist, "bluez-revert",
> > > "true");
> > > +
> > > +        break;
> > > +    }
> > > +}
> > > +
> > > +/* Return true if we should ignore this source output */
> > > +static bool ignore_output(pa_source_output *source_output) {
> > > +    const char *s;
> > > +
> > > +    /* New applications could set media.role for identifing
> > > streams
> > > */
> > > +    /* We are iterested only in media.role=phone */
> > 
> > "interested"
> 
> Ok.
> 
> > > +    s = pa_proplist_gets(source_output->proplist,
> > > PA_PROP_MEDIA_ROLE);
> > > +    if (s)
> > > +        return !pa_streq(s, "phone");
> > > +
> > > +    return true;
> > > +}
> > > +
> > > +static unsigned source_output_count(pa_core *c) {
> > > +    pa_source_output *source_output;
> > > +    uint32_t idx;
> > > +    unsigned count = 0;
> > > +
> > > +    PA_IDXSET_FOREACH(source_output, c->source_outputs, idx)
> > > +        if (!ignore_output(source_output))
> > > +            ++count;
> > > +
> > > +    return count;
> > > +}
> > > +
> > > +/* Switch profile for all cards */
> > > +static void switch_profile_all(pa_idxset *cards, bool revert) {
> > > +    pa_card *card;
> > > +    uint32_t idx;
> > > +
> > > +    PA_IDXSET_FOREACH(card, cards, idx)
> > > +        switch_profile(card, revert);
> > > +}
> > > +
> > > +/* When a source output is created, switch profile a2dp to
> > > profile
> > > hsp */
> > > +static pa_hook_result_t source_output_put_hook_callback(pa_core
> > > *c,
> > > pa_source_output *source_output, void *userdata) {
> > > +    pa_assert(c);
> > > +    pa_assert(source_output);
> > > +
> > > +    if (ignore_output(source_output))
> > > +        return PA_HOOK_OK;
> > > +
> > > +    switch_profile_all(c->cards, false);
> > > +    return PA_HOOK_OK;
> > > +}
> > > +
> > > +/* When all sources are unlinked, switch profile hsp back back
> > > to
> > > profile a2dp */
> > 
> > Should be "all source outputs".
> 
> Ok.
> 
> > > +static pa_hook_result_t
> > > source_output_unlink_hook_callback(pa_core
> > > *c, pa_source_output *source_output, void *userdata) {
> > > +    pa_assert(c);
> > > +    pa_assert(source_output);
> > > +
> > > +    if (ignore_output(source_output))
> > > +        return PA_HOOK_OK;
> > > +
> > > +    /* If there are still some source outputs do nothing (count
> > > is
> > > with *this* source_output, so +1) */
> > > +    if (source_output_count(c) > 1)
> > > +        return PA_HOOK_OK;
> > > +
> > > +    switch_profile_all(c->cards, true);
> > > +    return PA_HOOK_OK;
> > > +}
> > > +
> > > +static pa_hook_result_t card_new_hook_callback(pa_core *c,
> > > pa_card_new_data *new_data, void *userdata) {
> > > +    const char *s;
> > > +
> > > +    pa_assert(c);
> > > +    pa_assert(new_data);
> > > +
> > > +    if (source_output_count(c) == 0)
> > > +        return PA_HOOK_OK;
> > > +
> > > +    /* Only consider bluetooth cards */
> > > +    s = pa_proplist_gets(new_data->proplist,
> > > PA_PROP_DEVICE_BUS);
> > > +    if (!s || !pa_streq(s, "bluetooth"))
> > > +        return PA_HOOK_OK;
> > > +
> > > +    /* Ignore card if has already set other initial profile than
> > > a2dp */
> > > +    if (new_data->active_profile && !pa_streq(new_data-
> > > > active_profile, "a2dp"))
> > > +        return PA_HOOK_OK;
> > > +
> > > +    /* Set initial profile to hsp */
> > > +    pa_card_new_data_set_profile(new_data, "hsp");
> > > +
> > > +    /* Flag this card for revert */
> > > +    pa_proplist_sets(new_data->proplist, "bluez-revert",
> > > "true");
> > > +    return PA_HOOK_OK;
> > 
> > What is the reasoning behind having this property?
> 
> For each card I need to know if card needs to be "reverted" in
> future.

Is this for a case where a device is manually kept in the hsp profile
and you don't want to touch that?

> > In general, I prefer the module doing its book-keeping locally
> > rather
> > than (ab)using the proplist for state.
> 
> Ok, what to use then? How to store for each private boolean flag
> needed
> just by this module?

Usually you'd do something like have a hashmap in the module's
userdata, and then you could add your pa_card to that hashmap if it
needs a revert (and remove when you're done with the revert). The
userdata is available in all the hook callbacks.

> > > +}
> > > +
> > > +static pa_hook_result_t card_unlink_hook_callback(pa_core *c,
> > > pa_card *card, void *userdata) {
> > > +    pa_assert(c);
> > > +    pa_assert(card);
> > > +    switch_profile(card, true);
> > > +    return PA_HOOK_OK;
> > > +}
> > > +
> > >  static pa_card_profile *find_best_profile(pa_card *card) {
> > >      void *state;
> > >      pa_card_profile *profile;
> > > @@ -222,6 +385,7 @@ static void handle_all_profiles(pa_core
> > > *core) {
> > >  int pa__init(pa_module *m) {
> > >      pa_modargs *ma;
> > >      struct userdata *u;
> > > +    bool auto_switch;
> > >  
> > >      pa_assert(m);
> > >  
> > > @@ -232,6 +396,12 @@ int pa__init(pa_module *m) {
> > >  
> > >      m->userdata = u = pa_xnew0(struct userdata, 1);
> > >  
> > > +    auto_switch = true;
> > > +    if (pa_modargs_get_value_boolean(ma, "switch", &auto_switch)
> > > <
> > > 0) {
> > > +        pa_log("Failed to parse switch argument.");
> > > +        goto fail;
> > > +    }
> > > +
> > >      u->enable_a2dp_source = true;
> > >      if (pa_modargs_get_value_boolean(ma, "a2dp_source", &u-
> > > > enable_a2dp_source) < 0) {
> > >          pa_log("Failed to parse a2dp_source argument.");
> > > @@ -254,6 +424,20 @@ int pa__init(pa_module *m) {
> > >      u->sink_put_slot = pa_hook_connect(&m->core-
> > > > hooks[PA_CORE_HOOK_SINK_PUT], PA_HOOK_NORMAL,
> > >                                         (pa_hook_cb_t)
> > > sink_put_hook_callback, u);
> > >  
> > > +    if (auto_switch) {
> > > +        u->source_output_put_slot = pa_hook_connect(&m->core-
> > > > hooks[PA_CORE_HOOK_SOURCE_OUTPUT_PUT], PA_HOOK_NORMAL,
> > > +                                                    (pa_hook_cb_
> > > t)
> > > source_output_put_hook_callback, u);
> > > +
> > > +        u->source_output_unlink_slot = pa_hook_connect(&m->core-
> > > > hooks[PA_CORE_HOOK_SOURCE_OUTPUT_UNLINK], PA_HOOK_NORMAL,
> > > +                                                       (pa_hook_
> > > cb_t
> > > ) source_output_unlink_hook_callback, u);
> > > +
> > > +        u->card_new_slot = pa_hook_connect(&m->core-
> > > > hooks[PA_CORE_HOOK_CARD_NEW], PA_HOOK_NORMAL,
> > > +                                           (pa_hook_cb_t)
> > > card_new_hook_callback, u);
> > > +
> > > +        u->card_unlink_slot = pa_hook_connect(&m->core-
> > > > hooks[PA_CORE_HOOK_CARD_UNLINK], PA_HOOK_NORMAL,
> > > +                                           (pa_hook_cb_t)
> > > card_unlink_hook_callback, u);
> > > +    }
> > > +
> > >      u->profile_available_changed_slot = pa_hook_connect(&m-
> > > >core-
> > > > hooks[PA_CORE_HOOK_CARD_PROFILE_AVAILABLE_CHANGED],
> > >                                                          PA_HOOK_
> > > NORM
> > > AL, (pa_hook_cb_t) profile_available_hook_callback, u);
> > >  
> > > @@ -281,6 +465,18 @@ void pa__done(pa_module *m) {
> > >      if (u->sink_put_slot)
> > >          pa_hook_slot_free(u->sink_put_slot);
> > >  
> > > +    if (u->source_output_put_slot)
> > > +        pa_hook_slot_free(u->source_output_put_slot);
> > > +
> > > +    if (u->source_output_unlink_slot)
> > > +        pa_hook_slot_free(u->source_output_unlink_slot);
> > > +
> > > +    if (u->card_new_slot)
> > > +        pa_hook_slot_free(u->card_new_slot);
> > > +
> > > +    if (u->card_unlink_slot)
> > > +        pa_hook_slot_free(u->card_unlink_slot);
> > > +
> > >      if (u->profile_available_changed_slot)
> > >          pa_hook_slot_free(u->profile_available_changed_slot);
> > 
> > This one's just a nice-to-have -- we now have a
> > pa_module_hook_connect() that manages the slots for us.
> 
> Ok, but this is now not related to this patch. I will let this one
> for
> later.

Okay.

-- Arun
[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux