Re: [PATCH v3 3/3] a2dp: Fix not calling SelectConfiguration on other available endpoints

["Pali Rohár" <pali.rohar@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 10161 bytes --]

On Monday 29 April 2019 18:39:16 Pali Rohár wrote:
> On Monday 29 April 2019 19:35:24 Luiz Augusto von Dentz wrote:
> > Hi Pali,
> > 
> > On Mon, Apr 29, 2019 at 3:03 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > >
> > > Endpoint may not be able to select a valid configuration but there could
> > > be other endpoints available that could be used, so instead of just
> > > using the first match this collects all the matching endpoints and put
> > > them into a queue (ordered by priority) then proceed to next endpoint
> > > only failing if none of the available endpoits can select a valid
> > > configuration.
> > > ---
> > >  profiles/audio/a2dp.c | 77 ++++++++++++++++++++++++++++---------------
> > >  1 file changed, 50 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> > > index a23abdd97..4d378a91a 100644
> > > --- a/profiles/audio/a2dp.c
> > > +++ b/profiles/audio/a2dp.c
> > > @@ -105,6 +105,7 @@ struct a2dp_setup_cb {
> > >  struct a2dp_setup {
> > >         struct a2dp_channel *chan;
> > >         struct avdtp *session;
> > > +       struct queue *eps;
> > >         struct a2dp_sep *sep;
> > >         struct a2dp_remote_sep *rsep;
> > >         struct avdtp_stream *stream;
> > > @@ -2406,23 +2407,44 @@ void a2dp_remove_sep(struct a2dp_sep *sep)
> > >
> > >  static void select_cb(struct a2dp_setup *setup, void *ret, int size)
> > >  {
> > > -       if (size < 0) {
> > > -               DBG("Endpoint replied an invalid configuration");
> > > +       struct avdtp_service_capability *service;
> > > +       struct avdtp_media_codec_capability *codec;
> > > +       int err;
> > > +
> > > +       if (size) {
> > > +               caps_add_codec(&setup->caps, setup->sep->codec, ret, size);
> > >                 goto done;
> > >         }
> > >
> > > -       caps_add_codec(&setup->caps, setup->sep->codec, ret, size);
> > > +       setup->sep = queue_pop_head(setup->eps);
> > > +       if (!setup->sep) {
> > > +               error("Unable to select a valid configuration");
> > > +               queue_destroy(setup->eps, NULL);
> > > +               goto done;
> > > +       }
> > > +
> > > +       setup->rsep = find_remote_sep(setup->chan, setup->sep);
> > > +       service = avdtp_get_codec(setup->rsep->sep);
> > > +       codec = (struct avdtp_media_codec_capability *) service->data;
> > > +
> > > +       err = setup->sep->endpoint->select_configuration(setup->sep,
> > > +                                       codec->data,
> > > +                                       service->length - sizeof(*codec),
> > > +                                       setup_ref(setup),
> > > +                                       select_cb, setup->sep->user_data);
> > > +       if (err == 0)
> > > +               return;
> > >
> > >  done:
> > >         finalize_select(setup);
> > >         setup_unref(setup);
> > >  }
> > >
> > > -static struct a2dp_sep *a2dp_find_sep(struct avdtp *session, GSList *list,
> > > +static struct queue *a2dp_find_eps(struct avdtp *session, GSList *list,
> > >                                         const char *sender)
> > >  {
> > > -       struct a2dp_sep *first = NULL;
> > >         struct a2dp_channel *chan = find_channel(session);
> > > +       struct queue *seps = NULL;
> > >
> > >         for (; list; list = list->next) {
> > >                 struct a2dp_sep *sep = list->data;
> > > @@ -2444,26 +2466,25 @@ static struct a2dp_sep *a2dp_find_sep(struct avdtp *session, GSList *list,
> > >                 if (!rsep)
> > >                         continue;
> > >
> > > -               /* Locate last used if set */
> > > -               if (chan->last_used) {
> > > -                       if (chan->last_used->sep == rsep)
> > > -                               return sep;
> > > -                       first = sep;
> > > -                       continue;
> > > -               }
> > > +               if (!seps)
> > > +                       seps = queue_new();
> > >
> > > -               return sep;
> > > +               /* Prepend last used so it is preferred over others */
> > > +               if (chan->last_used && chan->last_used->sep == rsep)
> > > +                       queue_push_head(seps, sep);
> > > +               else
> > > +                       queue_push_tail(seps, sep);
> > >
> > >         }
> > >
> > > -       return first;
> > > +       return seps;
> > >  }
> > >
> > > -static struct a2dp_sep *a2dp_select_sep(struct avdtp *session, uint8_t type,
> > > +static struct queue *a2dp_select_eps(struct avdtp *session, uint8_t type,
> > >                                         const char *sender)
> > >  {
> > >         struct a2dp_server *server;
> > > -       struct a2dp_sep *sep;
> > > +       struct queue *seps;
> > >         GSList *l;
> > >
> > >         server = find_server(servers, avdtp_get_adapter(session));
> > > @@ -2473,11 +2494,11 @@ static struct a2dp_sep *a2dp_select_sep(struct avdtp *session, uint8_t type,
> > >         l = type == AVDTP_SEP_TYPE_SINK ? server->sources : server->sinks;
> > >
> > >         /* Check sender's seps first */
> > > -       sep = a2dp_find_sep(session, l, sender);
> > > -       if (sep != NULL)
> > > -               return sep;
> > > +       seps = a2dp_find_eps(session, l, sender);
> > > +       if (seps != NULL)
> > > +               return seps;
> > >
> > > -       return a2dp_find_sep(session, l, NULL);
> > > +       return a2dp_find_eps(session, l, NULL);
> > >  }
> > >
> > >  static void store_remote_sep(void *data, void *user_data)
> > > @@ -2580,13 +2601,13 @@ unsigned int a2dp_select_capabilities(struct avdtp *session,
> > >  {
> > >         struct a2dp_setup *setup;
> > >         struct a2dp_setup_cb *cb_data;
> > > -       struct a2dp_sep *sep;
> > > +       struct queue *eps;
> > >         struct avdtp_service_capability *service;
> > >         struct avdtp_media_codec_capability *codec;
> > >         int err;
> > >
> > > -       sep = a2dp_select_sep(session, type, sender);
> > > -       if (!sep) {
> > > +       eps = a2dp_select_eps(session, type, sender);
> > > +       if (!eps) {
> > >                 error("Unable to select SEP");
> > >                 return 0;
> > >         }
> > > @@ -2599,8 +2620,9 @@ unsigned int a2dp_select_capabilities(struct avdtp *session,
> > >         cb_data->select_cb = cb;
> > >         cb_data->user_data = user_data;
> > >
> > > -       setup->sep = sep;
> > > -       setup->rsep = find_remote_sep(setup->chan, sep);
> > > +       setup->eps = eps;
> > > +       setup->sep = queue_pop_head(eps);
> > > +       setup->rsep = find_remote_sep(setup->chan, setup->sep);
> > >
> > >         if (setup->rsep == NULL) {
> > >                 error("Could not find remote sep");
> > > @@ -2610,10 +2632,11 @@ unsigned int a2dp_select_capabilities(struct avdtp *session,
> > >         service = avdtp_get_codec(setup->rsep->sep);
> > >         codec = (struct avdtp_media_codec_capability *) service->data;
> > >
> > > -       err = sep->endpoint->select_configuration(sep, codec->data,
> > > +       err = setup->sep->endpoint->select_configuration(setup->sep,
> > > +                                       codec->data,
> > >                                         service->length - sizeof(*codec),
> > >                                         setup_ref(setup),
> > > -                                       select_cb, sep->user_data);
> > > +                                       select_cb, setup->sep->user_data);
> > >         if (err == 0)
> > >                 return cb_data->id;
> > >
> > > --
> > > 2.20.1
> > 
> > Le me know if you find any problem with this change, my headsets seems
> > to always succeed the first try so I cannot really reproduce the
> > problem.
> 
> Ok, I will try it in next days. I can register PA endpoints which always
> "fails" so I can test if this is working correctly as expected.

Nope, this your patch does not work. It cause segfaulting of bluetoothd
daemon itself. Here is stacktrace of current git master branch (where is
your patch too):

Program received signal SIGSEGV, Segmentation fault.
__memmove_avx_unaligned_erms () at ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:416
416     ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S: Adresár alebo súbor neexistuje.
(gdb) bt
#0  __memmove_avx_unaligned_erms () at ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:416
#1  0x0000557d2d2f3164 in caps_add_codec (l=0x557d2f1a9708, codec=<optimized out>, caps=0x0, size=18446744073709551615) at profiles/audio/a2dp.c:1519
#2  0x0000557d2d2f626a in select_cb (setup=0x557d2f1a96c0, ret=<optimized out>, size=<optimized out>) at profiles/audio/a2dp.c:2422
#3  0x0000557d2d2fe016 in endpoint_reply (call=<optimized out>, user_data=0x557d2f187810) at profiles/audio/media.c:316
#4  0x00007fd2dc6aa002 in ?? () from /lib/x86_64-linux-gnu/libdbus-1.so.3
#5  0x00007fd2dc6ada5f in dbus_connection_dispatch () from /lib/x86_64-linux-gnu/libdbus-1.so.3
#6  0x0000557d2d36f320 in message_dispatch (data=0x557d2f184560) at gdbus/mainloop.c:72
#7  0x00007fd2dc9326aa in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#8  0x00007fd2dc932a60 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#9  0x00007fd2dc932d82 in g_main_loop_run () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#10 0x0000557d2d384a45 in mainloop_run () at src/shared/mainloop-glib.c:79
#11 0x0000557d2d384e0f in mainloop_run_with_signal (func=<optimized out>, user_data=0x0) at src/shared/mainloop-notify.c:201
#12 0x0000557d2d2eb0a8 in main (argc=<optimized out>, argv=<optimized out>) at src/main.c:729

In syslog for bluetoothd is this last line:
bluetoothd[12605]: Endpoint replied with an error: org.bluez.Error.InvalidArguments

pulseaudio refused only one SelectConfiguration call.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]