Hi Pali,
To go away from the debate about "what's deprecated and what's not?" , and if yes or no : devices that don't implement a MANDATORY A2DP feature like DUAL CHANNEL, while FAKING that they DO IMPLEMENT IT (which is the only case where my algorithm fails) : have to be the main target, or not ...
... I have a proposal :
IMHO : what could be a good compromise would be to keep the Automatic mode that you already implemented it as DEFAULT, calling it "Legacy Automatic" or so.
AND add an alternative Automatic mode called "XQ Automatic" (following negotiation strategy similar to my patch). This secondary Automatic mode could replace "forced bitpool" modes.
If you really want to keep forced bitpool modes, it doesn't hurt, btw.
All the best
JP
08.10.2019, 18:52, "Hyperion" <h1p8r10n@xxxxxxxxxx>:
I disagree.As an engineer you should know that an implementation should not be based on broken devices , but rarher on good standard specifications.Rare broken devices, and rare very old devices that don't follow A2DP specifications shouldn't be the main design target for Pulseaudio.Jp18:29, 8 octobre 2019, "Pali Rohár" <pali.rohar@xxxxxxxxx>:
Automatic SBC profile is not going to be changed. It is there to support
all devices without any tweaks. ValdikSS already did more tests and
there are devices which do not work with higher SBC bitpool. So
increasing max value of bitpool in Automatic SBC profile would lead to
broken support for these devices and therefore regressions.
As Automatic SBC profile is the only one available for systems where
codec switching is not supported, it would mean complete regression as
these devices completely stops working on those systems.
Upgrading either pulseaudio or bluez must not lead to problem that some
bluetooth devices stop working (if they worked before upgrade).
So no, there would not be any changes in Automatic SBC profile. This one
should stay untouched, to make it always working with all existing
devices without any regression.
On Monday 07 October 2019 16:27:19 Hyperion wrote: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 = "" revert_to_profile_name);
>> >> >> >> > + } else {
>> >> >> >> > + /* Find highest priority profile with both sink and source */
>> >> >> >> > + profile = "" /> >> >> >> >> > + 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 = "" /> >> >> >> >> > }
>> >> >> >> > + }
>> >> >> >> >
>> >> >> >> > - 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--
Pali Rohár
pali.rohar@xxxxxxxxx
--
Sent from Yandex.Mail for mobile
_______________________________________________ pulseaudio-discuss mailing list pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
