Hi Pali,
On Sun, Apr 28, 2019 at 9:53 PM Pali Rohár <pali.rohar@xxxxxxxxx> wrote:
>
> On Thursday 25 April 2019 11:00:07 Luiz Augusto von Dentz wrote:
> > Hi Pali,
> >
> > On Wed, Apr 24, 2019 at 7:57 PM Pali Rohár <pali.rohar@xxxxxxxxx> wrote:
> > >
> > > On Wednesday 24 April 2019 12:42:22 Luiz Augusto von Dentz wrote:
> > > > Hi Pali,
> > > >
> > > > On Wed, Apr 24, 2019 at 11:39 AM Luiz Augusto von Dentz
> > > > <luiz.dentz@xxxxxxxxx> wrote:
> > > > >
> > > > > From: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
> > > > >
> > > > > This stores the last used endpoint whenever it is considered open and
> > > > > then prefer to use it when attempting to reconnect.
> > > > > ---
> > > > > profiles/audio/a2dp.c | 104 ++++++++++++++++++++++++++++++++++++------
> > > > > 1 file changed, 91 insertions(+), 13 deletions(-)
> > > > >
> > > > > diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> > > > > index 8f141739c..78b02dc84 100644
> > > > > --- a/profiles/audio/a2dp.c
> > > > > +++ b/profiles/audio/a2dp.c
> > > > > @@ -147,6 +147,7 @@ struct a2dp_channel {
> > > > > unsigned int auth_id;
> > > > > struct avdtp *session;
> > > > > struct queue *seps;
> > > > > + struct a2dp_remote_sep *last_used;
> > > > > };
> > > > >
> > > > > static GSList *servers = NULL;
> > > > > @@ -860,6 +861,60 @@ static gboolean open_ind(struct avdtp *session, struct avdtp_local_sep *sep,
> > > > > return TRUE;
> > > > > }
> > > > >
> > > > > +static bool match_remote_sep(const void *data, const void *user_data)
> > > > > +{
> > > > > + const struct a2dp_remote_sep *sep = data;
> > > > > + const struct avdtp_remote_sep *rsep = user_data;
> > > > > +
> > > > > + return sep->sep == rsep;
> > > > > +}
> > > > > +
> > > > > +static void store_last_used(struct a2dp_channel *chan,
> > > > > + struct avdtp_remote_sep *rsep)
> > > > > +{
> > > > > + GKeyFile *key_file;
> > > > > + char filename[PATH_MAX];
> > > > > + char dst_addr[18];
> > > > > + char value[4];
> > > > > + char *data;
> > > > > + size_t len = 0;
> > > > > +
> > > > > + ba2str(device_get_address(chan->device), dst_addr);
> > > > > +
> > > > > + snprintf(filename, PATH_MAX, STORAGEDIR "/%s/cache/%s",
> > > > > + btd_adapter_get_storage_dir(device_get_adapter(chan->device)),
> > > > > + dst_addr);
> > > > > + key_file = g_key_file_new();
> > > > > + g_key_file_load_from_file(key_file, filename, 0, NULL);
> > > > > +
> > > > > + sprintf(value, "%02hhx", avdtp_get_seid(rsep));
> > > > > +
> > > > > + g_key_file_set_string(key_file, "Endpoints", "LastUsed", value);
> > > > > +
> > > > > + data = g_key_file_to_data(key_file, &len, NULL);
> > > > > + g_file_set_contents(filename, data, len, NULL);
> > > > > +
> > > > > + g_free(data);
> > > > > + g_key_file_free(key_file);
> > > > > +}
> > > > > +
> > > > > +static void update_last_used(struct a2dp_channel *chan,
> > > > > + struct avdtp_stream *stream)
> > > > > +{
> > > > > + struct avdtp_remote_sep *rsep;
> > > > > + struct a2dp_remote_sep *sep;
> > > > > +
> > > > > + rsep = avdtp_stream_get_remote_sep(stream);
> > > > > +
> > > > > + /* Update last used */
> > > > > + sep = queue_find(chan->seps, match_remote_sep, rsep);
> > > > > + if (chan->last_used == sep)
> > > > > + return;
> > > > > +
> > > > > + chan->last_used = sep;
> > > > > + store_last_used(chan, rsep);
> > > > > +}
> > > > > +
> > > > > static void open_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
> > > > > struct avdtp_stream *stream, struct avdtp_error *err,
> > > > > void *user_data)
> > > > > @@ -884,7 +939,8 @@ static void open_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
> > > > > setup->err = err;
> > > > > if (setup->start)
> > > > > finalize_resume(setup);
> > > > > - }
> > > > > + } else if (setup->chan)
> > > > > + update_last_used(setup->chan, stream);
> > > > >
> > > > > finalize_config(setup);
> > > > >
> > > > > @@ -1077,14 +1133,6 @@ static gboolean close_ind(struct avdtp *session, struct avdtp_local_sep *sep,
> > > > > return TRUE;
> > > > > }
> > > > >
> > > > > -static bool match_remote_sep(const void *data, const void *user_data)
> > > > > -{
> > > > > - const struct a2dp_remote_sep *sep = data;
> > > > > - const struct avdtp_remote_sep *rsep = user_data;
> > > > > -
> > > > > - return sep->sep == rsep;
> > > > > -}
> > > > > -
> > > > > static struct a2dp_remote_sep *find_remote_sep(struct a2dp_channel *chan,
> > > > > struct a2dp_sep *sep)
> > > > > {
> > > > > @@ -1791,19 +1839,28 @@ done:
> > > > > queue_push_tail(chan->seps, sep);
> > > > > }
> > > > >
> > > > > +static bool match_seid(const void *data, const void *user_data)
> > > > > +{
> > > > > + const struct a2dp_remote_sep *sep = data;
> > > > > + const uint8_t *seid = user_data;
> > > > > +
> > > > > + return avdtp_get_seid(sep->sep) == *seid;
> > > > > +}
> > > > > +
> > > > > static void load_remote_sep(struct a2dp_channel *chan, GKeyFile *key_file,
> > > > > char **seids)
> > > > > {
> > > > > struct avdtp_remote_sep *sep;
> > > > > + uint8_t seid;
> > > > > + char *value;
> > > > >
> > > > > if (!seids)
> > > > > return;
> > > > >
> > > > > for (; *seids; seids++) {
> > > > > - uint8_t seid;
> > > > > uint8_t type;
> > > > > uint8_t codec;
> > > > > - char *value, caps[256];
> > > > > + char caps[256];
> > > > > uint8_t data[128];
> > > > > int i, size;
> > > > > GSList *l = NULL;
> > > > > @@ -1847,6 +1904,15 @@ static void load_remote_sep(struct a2dp_channel *chan, GKeyFile *key_file,
> > > > >
> > > > > register_remote_sep(sep, chan);
> > > > > }
> > > > > +
> > > > > + value = g_key_file_get_string(key_file, "Endpoints", "LastUsed", NULL);
> > > > > + if (!value)
> > > > > + return;
> > > > > +
> > > > > + if (sscanf(value, "%02hhx", &seid) != 1)
> > > > > + return;
> > > > > +
> > > > > + chan->last_used = queue_find(chan->seps, match_seid, &seid);
> > > > > }
> > > > >
> > > > > static void load_remote_seps(struct a2dp_channel *chan)
> > > > > @@ -2355,8 +2421,12 @@ done:
> > > > > static struct a2dp_sep *a2dp_find_sep(struct avdtp *session, GSList *list,
> > > > > const char *sender)
> > > > > {
> > > > > + struct a2dp_sep *first = NULL;
> > > > > + struct a2dp_channel *chan = find_channel(session);
> > > > > +
> > > > > for (; list; list = list->next) {
> > > > > struct a2dp_sep *sep = list->data;
> > > > > + struct avdtp_remote_sep *rsep;
> > > > >
> > > > > /* Use sender's endpoint if available */
> > > > > if (sender) {
> > > > > @@ -2370,14 +2440,22 @@ static struct a2dp_sep *a2dp_find_sep(struct avdtp *session, GSList *list,
> > > > > continue;
> > > > > }
> > > > >
> > > > > - if (avdtp_find_remote_sep(session, sep->lsep) == NULL)
> > > > > + rsep = avdtp_find_remote_sep(session, sep->lsep);
> > > > > + if (!rsep)
> > > > > continue;
> > > > >
> > > > > + /* Locate last used if set */
> > > > > + if (chan->last_used) {
> > > > > + if (chan->last_used->sep == rsep)
> > > > > + return sep;
> > > > > + first = sep;
> > > > > + }
> > > > > +
> > > > > return sep;
> > > > >
> > > > > }
> > > > >
> > > > > - return NULL;
> > > > > + return first;
> > > > > }
> > > > >
> > > > > static struct a2dp_sep *a2dp_select_sep(struct avdtp *session, uint8_t type,
> > > > > --
> > > > > 2.20.1
> > > >
> > > > Can you give this a try, it should make the daemon remember what was
> > > > the last endpoint used (locally the remote selection we cannot really
> > > > control).
> > >
> > > Great, I will try it. Remote selection is done by remote device so this
> > > really cannot be controlled.
> > >
> > > I have there another suggestion for improvement. bluez support
> > > registering more endpoints for one codec. I'm going to use it for
> > > registering one SBC endpoint with fixed bitpool as UHQ profile and
> > > another with lower bitpool values. To distinguish in pulseaudio between
> > > low and high quality of SBC. Seems that bluez sometimes try to only
> > > select only one endpoint, even there are more and do not try another if
> > > first one selection fails (e.g. because remote capability of bitpool is
> > > too low and our local selection needs higher). Could be bluez modified
> > > to try another local endpoint if there are more registered until some
> > > accept capabilities?
> >
> > Well that was actually one of the reason Ive recommended not
> > registering more than one endpoint for the same codec, we should
> > perhaps fix that since those endpoints can belong to different
> > processes but if we do insist to have PA with multiple SBC endpoints
> > that means it would not be backward compatible since it may try to
> > configure with UHQ as it should be higher priority, besides having a
> > higher fixed range means we can no longer reduce the quality at
> > runtime, but perhaps there is a way to make it work if PA detects it
> > has multiple endpoints that could be used it could just switch to
> > normal quality if UHQ cannot be used, this is actually beneficial in
> > terms of latency as well since we don't have extra D-Bus round trips
> > asking different endpoints.
>
> Registering multiple endpoints is the only way how to enforce SBC UHQ.
> Most of headset supports only SBC bitpools 1-53, therefore for SBC UHQ
> is needed Dualchannel mode, not Joint Stereo.
Sure, but that is just one extra endpoint, not the 4 or so you have
currently in your set.
> So I see there only one backward-compatible implementation: Runtime
> check of bluez version (or capability or whatever). Also this is
> probably also useful/needed to check if bluez supports API for changing
> codec.
>
> So... can you implement that "fallback" mechanisms of trying to use all
> possible endpoints until one succeed; and provide ability for
> applications to detect this functionality is supported?
I will have a fix for that, but cycling thru that many endpoints might
not be the best experience, Id really just leave 2 endpoints for SBC,
one for dual channel and another for the rest.
> Application (like pulseaudio) would register multiple endpoints only in
> the case then bluez declares its functionality to be code backward
> compatible. And if case it is not supported then application would not
> support fixed profiles like SBC UHQ.
Well, going with 76 bitpool is in fact supported by early versions of
BlueZ, thus why I was suggesting to register just 2 SBC endpoints as
explained above since you can order register the joint-stereo before
the dual-band that should not be affected by the lack of retrying
logic.
> --
> Pali Rohár
> pali.rohar@xxxxxxxxx
--
Luiz Augusto von Dentz
