Hi Marijn,
On Sat, Oct 24, 2020 at 3:10 PM Marijn Suijten <marijns95@xxxxxxxxx> wrote:
>
> set_configuration creates a transport before calling SetConfiguration on
> the MediaEndpoint1 DBus interface. If this DBus call fails the
> transport sticks around while it should instead be cleaned up.
>
> When the peer retries or reconnects (in case of BlueZ: See
> avdtp_parse_rej for SET_CONFIGURATION returning TRUE on failure,
> resulting in connection_lost with EIO) set_configuration finds this old
> transport and returns FALSE. The peer will never succeed this call
> unless it randomly decides to call clear_configuration or BlueZ is
> restarted.
> ---
>
> Hi all,
>
> This issue was found while playing around with broken configurations in
> PulseAudio. All attempts to set a configuration after the first failure
> are rejected. Note that BlueZ disconnects if SET_CONFIGURATION fails,
> is that expected? I did not analyze any of this code yet but it seems
> rather harsh for something that's not fatal and perhaps better
> propagated to the calling application?
>
> As far as I know Pali's A2DP codec additions to PulseAudio expect this
> this behaviour; if SetConfiguration fails it'll simply attempt the next
> profile until finding one that succeeds.
Weird, so does PA will attempt to select a configuration on its own
instead of using Device.Connect which does
MediaEndpoint.SelectConfiguration on the matching endpoints? Or is
this part of the codec switch logic? I thought the latter would
attempt to switch the codec and in case it fails it would then roll
back, but maybe things have changed.
> I moved the functions up above endpoint_reply instead of
> forward-declaring them as they were pretty close already and aren't that
> many line changes, let me know if that's okay.
>
> profiles/audio/media.c | 77 +++++++++++++++++++++++++-----------------
> 1 file changed, 46 insertions(+), 31 deletions(-)
>
> diff --git a/profiles/audio/media.c b/profiles/audio/media.c
> index 74064d398..0632fbe8a 100644
> --- a/profiles/audio/media.c
> +++ b/profiles/audio/media.c
> @@ -271,6 +271,37 @@ static void clear_endpoint(struct media_endpoint *endpoint)
> clear_configuration(endpoint, endpoint->transports->data);
> }
>
> +static int transport_device_cmp(gconstpointer data, gconstpointer user_data)
> +{
> + struct media_transport *transport = (struct media_transport *) data;
> + const struct btd_device *device = user_data;
> + const struct btd_device *dev = media_transport_get_dev(transport);
> +
> + if (device == dev)
> + return 0;
> +
> + return -1;
> +}
> +
> +static struct media_transport *find_device_transport(
> + struct media_endpoint *endpoint,
> + struct btd_device *device)
> +{
> + GSList *match;
> +
> + match = g_slist_find_custom(endpoint->transports, device,
> + transport_device_cmp);
> + if (match == NULL)
> + return NULL;
> +
> + return match->data;
> +}
> +
> +struct a2dp_config_data {
> + struct a2dp_setup *setup;
> + a2dp_endpoint_config_t cb;
> +};
> +
> static void endpoint_reply(DBusPendingCall *call, void *user_data)
> {
> struct endpoint_request *request = user_data;
> @@ -298,6 +329,21 @@ static void endpoint_reply(DBusPendingCall *call, void *user_data)
> return;
> }
I'd probably just have a reference to the transport in the request
that way we don't need to look it up if SetConfiguration fails.
> + if (dbus_message_is_method_call(request->msg,
> + MEDIA_ENDPOINT_INTERFACE,
> + "SetConfiguration")) {
> + struct a2dp_config_data *data = request->user_data;
> + struct btd_device *device =
> + a2dp_setup_get_device(data->setup);
> + struct media_transport *transport =
> + find_device_transport(endpoint, device);
> +
> + if (transport == NULL)
> + error("Expected to destroy transport");
> + else
> + media_transport_destroy(transport);
> + }
> +
> dbus_error_free(&err);
> goto done;
> }
> @@ -396,37 +442,6 @@ static gboolean select_configuration(struct media_endpoint *endpoint,
> return media_endpoint_async_call(msg, endpoint, cb, user_data, destroy);
> }
>
> -static int transport_device_cmp(gconstpointer data, gconstpointer user_data)
> -{
> - struct media_transport *transport = (struct media_transport *) data;
> - const struct btd_device *device = user_data;
> - const struct btd_device *dev = media_transport_get_dev(transport);
> -
> - if (device == dev)
> - return 0;
> -
> - return -1;
> -}
> -
> -static struct media_transport *find_device_transport(
> - struct media_endpoint *endpoint,
> - struct btd_device *device)
> -{
> - GSList *match;
> -
> - match = g_slist_find_custom(endpoint->transports, device,
> - transport_device_cmp);
> - if (match == NULL)
> - return NULL;
> -
> - return match->data;
> -}
> -
> -struct a2dp_config_data {
> - struct a2dp_setup *setup;
> - a2dp_endpoint_config_t cb;
> -};
> -
> int8_t media_player_get_device_volume(struct btd_device *device)
> {
> struct avrcp_player *target_player;
> --
> 2.29.1
>
> Marijn Suijten
--
Luiz Augusto von Dentz
