Re: [PATCH v13 10/10] bluetooth: policy: Treat bi-directional A2DP profiles as suitable for VOIP

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

 



I'm not talking about old Bluez versions : only about the current stable 5.51, and not talking about codec switchin either.

Just talking about improvement of MAX negociated values for SBC. Please take a look at my latest patch.

07.10.2019, 16:22, "Pali Rohár" <pali.rohar@xxxxxxxxx>:
> Old bluez versions have bugs and different behavior. So not break
> anything else and still have working audio playback, it is better to not
> touch it and use what is currently supported / provided.
>
> New features like additional codec support, codec switching, etc...
> needs new bluez API and new functionality which were introduced in 5.51
> together with fixed more A2DP related bugs. Instead of workarounding
> bluez bugs, just to use working one stable SBC codec like before OR
> update bluez to new version and have new features.
>
> On Monday 07 October 2019 16:18:21 Hyperion wrote:
>>  I disagree with "Also there would not be any feature/functional changes in pulseaudio when older bluez version is in use".
>>  Tests prove that there's no reason for this, if only one mode/profile is used.
>>
>>  JP
>>
>>  07.10.2019, 15:36, "Pali Rohár" <pali.rohar@xxxxxxxxx>:
>>  > I will try to look what is the problem without --experimental. This
>>  > should work correctly prior merging this PR.
>>  >
>>  > Need to --experimental is just temporary until support in bluez is not
>>  > fully stable. I guess it would be non-experimental in next bluez
>>  > version.
>>  >
>>  > Also there would not be any feature/functional changes in pulseaudio
>>  > when older bluez version is in use. So also no change in automatic mode
>>  > for older bluez versions.
>>  >
>>  > On Monday 07 October 2019 15:30:49 Hyperion wrote:
>>  >>  Without the --experimental flag : negociation stops (see hcidump below) and device falls back to "Off" status.
>>  >>
>>  >>  btw : if you implement my (simple) negociation algorithm for the "Automatic Quality" mode ; AND make it to work without --experimental Bluez flag : it woiuld be close to perfect.
>>  >>
>>  >>  See my latest patch https://gitlab.freedesktop.org/Hyperion/pulseaudio/tree/SBC-XQ
>>  >>
>>  >>  jp
>>  >>
>>  >>  07.10.2019, 15:25, "Pali Rohár" <pali.rohar@xxxxxxxxx>:
>>  >>  > And what happened without --experimental?
>>  >>  >
>>  >>  > Aim is to support all bluez versions also with and without
>>  >>  > --experimental flag. Just for older versions (or without experimental)
>>  >>  > it should behave like before this patch series -- only SBC codec in
>>  >>  > automatic mode and no codec switching.
>>  >>  >
>>  >>  > On Monday 07 October 2019 15:20:35 Hyperion wrote:
>>  >>  >>  Thanks, I forgot the "--experimental" param.
>>  >>  >>
>>  >>  >>  Works as expected, like the previous v12 serie of patches
>>  >>  >>
>>  >>  >>  JP
>>  >>  >>
>>  >>  >>  07.10.2019, 15:15, "Pali Rohár" <pali.rohar@xxxxxxxxx>:
>>  >>  >>  > Can you check if you have Bluez 5.51 and bluetoothd daemon is running with --experimental param?
>>  >>  >>  >
>>  >>  >>  > And is not it possible to change profile from Off to Automatic Quality?
>>  >>  >>  >
>>  >>  >>  > On Monday 07 October 2019 15:08:59 Hyperion wrote:
>>  >>  >>  >>  The 10 patches compile OK on PA master without warnings.
>>  >>  >>  >>
>>  >>  >>  >>  But doesn't work (device is Off with "Automatic Quality unavailable" status).
>>  >>  >>  >>
>>  >>  >>  >>  Tested on 2 devices.
>>  >>  >>  >>
>>  >>  >>  >>  hcidump avdtp
>>  >>  >>  >>  HCI sniffer - Bluetooth packet analyzer ver 5.51
>>  >>  >>  >>  device: hci0 snap_len: 1500 filter: 0x400
>>  >>  >>  >>  < AVDTP(s): Discover cmd: transaction 9 nsp 0x00
>>  >>  >>  >>  > AVDTP(s): Discover rsp: transaction 9 nsp 0x00
>>  >>  >>  >>      ACP SEID 1 - Audio Sink
>>  >>  >>  >>      ACP SEID 2 - Audio Sink
>>  >>  >>  >>      ACP SEID 3 - Audio Sink
>>  >>  >>  >>  < AVDTP(s): Set config cmd: transaction 10 nsp 0x00
>>  >>  >>  >>      ACP SEID 1 - INT SEID 2
>>  >>  >>  >>      Media Transport
>>  >>  >>  >>      Media Codec - SBC
>>  >>  >>  >>        44.1kHz
>>  >>  >>  >>        DualChannel
>>  >>  >>  >>        16 Blocks
>>  >>  >>  >>        8 Subbands
>>  >>  >>  >>        Loudness
>>  >>  >>  >>        Bitpool Range 38-38
>>  >>  >>  >>  > AVDTP(s): Set config rsp: transaction 10 nsp 0x00
>>  >>  >>  >>  < AVDTP(s): Open cmd: transaction 11 nsp 0x00
>>  >>  >>  >>      ACP SEID 1
>>  >>  >>  >>  > AVDTP(s): Open rsp: transaction 11 nsp 0x00
>>  >>  >>  >>
>>  >>  >>  >>  06.10.2019, 19:59, "Pali Rohár" <pali.rohar@xxxxxxxxx>:
>>  >>  >>  >>  > 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;
>>  >>  >>  >>  > + from_profile = ps->from_profile;
>>  >>  >>  >>  > + to_profile = ps->to_profile;
>>  >>  >>  >>  > + pa_xfree(ps);
>>  >>  >>  >>  >
>>  >>  >>  >>  > - /* Skip card if already has active a2dp profile */
>>  >>  >>  >>  > - if (pa_startswith(card->active_profile->name, "a2dp_sink"))
>>  >>  >>  >>  > + /* Skip card if does not have active profile to which was switched */
>>  >>  >>  >>  > + if (!pa_streq(card->active_profile->name, to_profile))
>>  >>  >>  >>  >              return;
>>  >>  >>  >>  >      } else {
>>  >>  >>  >>  > - /* Skip card if does not have active a2dp profile */
>>  >>  >>  >>  > - if (!pa_startswith(card->active_profile->name, "a2dp_sink"))
>>  >>  >>  >>  > - return;
>>  >>  >>  >>  > -
>>  >>  >>  >>  > - /* Skip card if already has active hsp profile */
>>  >>  >>  >>  > - if (pa_streq(card->active_profile->name, "headset_head_unit"))
>>  >>  >>  >>  > + /* Skip card if already has both sink and source */
>>  >>  >>  >>  > + if (card->active_profile->n_sources > 0 && card->active_profile->n_sinks > 0)
>>  >>  >>  >>  >              return;
>>  >>  >>  >>  >      }
>>  >>  >>  >>  >
>>  >>  >>  >>  > - card_set_profile(u, card, revert_to_a2dp);
>>  >>  >>  >>  > + card_set_profile(u, card, revert ? from_profile : NULL);
>>  >>  >>  >>  >  }
>>  >>  >>  >>  >
>>  >>  >>  >>  >  /* Return true if we should ignore this source output */
>>  >>  >>  >>  > @@ -254,15 +269,15 @@ static unsigned source_output_count(pa_core *c, void *userdata) {
>>  >>  >>  >>  >  }
>>  >>  >>  >>  >
>>  >>  >>  >>  >  /* Switch profile for all cards */
>>  >>  >>  >>  > -static void switch_profile_all(pa_idxset *cards, bool revert_to_a2dp, void *userdata) {
>>  >>  >>  >>  > +static void switch_profile_all(pa_idxset *cards, bool revert, void *userdata) {
>>  >>  >>  >>  >      pa_card *card;
>>  >>  >>  >>  >      uint32_t idx;
>>  >>  >>  >>  >
>>  >>  >>  >>  >      PA_IDXSET_FOREACH(card, cards, idx)
>>  >>  >>  >>  > - switch_profile(card, revert_to_a2dp, userdata);
>>  >>  >>  >>  > + switch_profile(card, revert, userdata);
>>  >>  >>  >>  >  }
>>  >>  >>  >>  >
>>  >>  >>  >>  > -/* When a source output is created, switch profile a2dp to profile hsp */
>>  >>  >>  >>  > +/* When a source output is created, switch profile to some which has both sink and source */
>>  >>  >>  >>  >  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);
>>  >>  >>  >>  > @@ -274,7 +289,7 @@ static pa_hook_result_t source_output_put_hook_callback(pa_core *c, pa_source_ou
>>  >>  >>  >>  >      return PA_HOOK_OK;
>>  >>  >>  >>  >  }
>>  >>  >>  >>  >
>>  >>  >>  >>  > -/* When all source outputs are unlinked, switch profile hsp back back to profile a2dp */
>>  >>  >>  >>  > +/* When all source outputs are unlinked, switch to previous profile */
>>  >>  >>  >>  >  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);
>>  >>  >>  >>  > @@ -291,30 +306,16 @@ static pa_hook_result_t source_output_unlink_hook_callback(pa_core *c, pa_source
>>  >>  >>  >>  >  }
>>  >>  >>  >>  >
>>  >>  >>  >>  >  static pa_hook_result_t card_init_profile_hook_callback(pa_core *c, pa_card *card, void *userdata) {
>>  >>  >>  >>  > - struct userdata *u = userdata;
>>  >>  >>  >>  > - const char *s;
>>  >>  >>  >>  > -
>>  >>  >>  >>  >      pa_assert(c);
>>  >>  >>  >>  >      pa_assert(card);
>>  >>  >>  >>  >
>>  >>  >>  >>  > + /* If there are not some source outputs do nothing */
>>  >>  >>  >>  >      if (source_output_count(c, userdata) == 0)
>>  >>  >>  >>  >          return PA_HOOK_OK;
>>  >>  >>  >>  >
>>  >>  >>  >>  > - /* Only consider bluetooth cards */
>>  >>  >>  >>  > - s = pa_proplist_gets(card->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 (card->active_profile &&
>>  >>  >>  >>  > - !pa_startswith(card->active_profile->name, "a2dp_sink"))
>>  >>  >>  >>  > - return PA_HOOK_OK;
>>  >>  >>  >>  > -
>>  >>  >>  >>  > - /* Set initial profile to hsp */
>>  >>  >>  >>  > - card_set_profile(u, card, false);
>>  >>  >>  >>  > + /* Set initial profile to some with source */
>>  >>  >>  >>  > + switch_profile(card, false, userdata);
>>  >>  >>  >>  >
>>  >>  >>  >>  > - /* Flag this card for will_need_revert */
>>  >>  >>  >>  > - pa_hashmap_put(u->will_need_revert_card_map, card, PA_INT_TO_PTR(1));
>>  >>  >>  >>  >      return PA_HOOK_OK;
>>  >>  >>  >>  >  }
>>  >>  >>  >>  >
>>  >>  >>  >>  > @@ -447,7 +448,7 @@ int pa__init(pa_module *m) {
>>  >>  >>  >>  >          goto fail;
>>  >>  >>  >>  >      }
>>  >>  >>  >>  >
>>  >>  >>  >>  > - u->will_need_revert_card_map = pa_hashmap_new(pa_idxset_trivial_hash_func, pa_idxset_trivial_compare_func);
>>  >>  >>  >>  > + u->profile_switch_map = pa_hashmap_new(pa_idxset_trivial_hash_func, pa_idxset_trivial_compare_func);
>>  >>  >>  >>  >
>>  >>  >>  >>  >      u->source_put_slot = pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_SOURCE_PUT], PA_HOOK_NORMAL,
>>  >>  >>  >>  >                                           (pa_hook_cb_t) source_put_hook_callback, u);
>>  >>  >>  >>  > @@ -512,7 +513,7 @@ void pa__done(pa_module *m) {
>>  >>  >>  >>  >      if (u->profile_available_changed_slot)
>>  >>  >>  >>  >          pa_hook_slot_free(u->profile_available_changed_slot);
>>  >>  >>  >>  >
>>  >>  >>  >>  > - pa_hashmap_free(u->will_need_revert_card_map);
>>  >>  >>  >>  > + pa_hashmap_free(u->profile_switch_map);
>>  >>  >>  >>  >
>>  >>  >>  >>  >      pa_xfree(u);
>>  >>  >>  >>  >  }
>>  >>  >>  >>  > --
>>  >>  >>  >>  > 2.11.0
>>  >>  >>  >>  >
>>  >>  >>  >>  > _______________________________________________
>>  >>  >>  >>  > pulseaudio-discuss mailing list
>>  >>  >>  >>  > pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx
>>  >>  >>  >>  > https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
>>  >>  >>  >>  _______________________________________________
>>  >>  >>  >>  pulseaudio-discuss mailing list
>>  >>  >>  >>  pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx
>>  >>  >>  >>  https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
>>  >>  >>  >
>>  >>  >>  > --
>>  >>  >>  > Pali Rohár
>>  >>  >>  > pali.rohar@xxxxxxxxx
>>  >>  >
>>  >>  > --
>>  >>  > Pali Rohár
>>  >>  > pali.rohar@xxxxxxxxx
>>  >
>>  > --
>>  > Pali Rohár
>>  > pali.rohar@xxxxxxxxx
>
> --
> Pali Rohár
> pali.rohar@xxxxxxxxx
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss




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

  Powered by Linux