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.
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?
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.
--
Pali Rohár
pali.rohar@xxxxxxxxx
Attachment:
signature.asc
Description: PGP signature
