Hi Pauli,
On Sun, May 4, 2025 at 12:02 PM Pauli Virtanen <pav@xxxxxx> wrote:
>
> The ucast client stream design has issues:
>
> * Assuming lpac & rpac pair uniquely identifies a stream. False for
> AC6(i) and other multi-stream configs.
>
> * No way for ASE in Config state be configured with different codec.
>
> * Assuming ASE enters idle state at end of stream life cycle. False for
> some devices like Sony headsets, which always cache codec config so
> RELEASING -> CONFIG always, never RELEASING -> IDLE, so ASE never go
> IDLE again.
>
> * Assuming Unicast Client upper layer always creates streams for all
> non-idle ASEs. False, as when switching between duplex & sink-only
> modes, only some streams shall be used regardless of whether Server is
> caching config or not.
I think it would make sense to fix these points one by one, or you
tried that and ended up running into some problems?
> In practice, these currently prevent reconfiguring ASEs on devices which
> cache codec config, and multi-stream configs work only accidentally (and
> fail if server does Config codec itself, like some devices do).
>
> Minimally fixed design for Unicast Client streams:
>
> Streams correspond 1-to-1 to ASEs in non-IDLE state.
>
> Track explicitly which streams upper level is using:
>
> - bt_bap_stream_new() always makes new stream with client_active=true
> - bt_bap_stream_discard() releases stream and marks client_active=false
This perhaps could be done differently, perhaps having a lock flag, so
if the stream is locked it means its configuration cannot be changed.
> Streams (ASEs) with no active client may be reused when upper level asks
> for a new stream.
>
> Streams with no active client may have their lpac changed. The
> need_reconfig state is a bit ugly, but not really avoidable.
Do we have a situation where client_active=false (lock=false) and
need_reconfig!=true? In other words could we just use the
client_active/lock flag instead of introducing yet another flag?
> Streams with no active client shall be ignored when constructing
> bidirectional CIS pairs.
>
> Streams shall clear transport and detach io on RELEASING. (cf BAP v1.0.2
> §5.6.6). Also unlink them since QoS is gone at that point.
I'd consider fixing this as a separate patch as well.
> Streams shall have transport only for >= QOS state. (Server streams
> work differently, which makes sense since upper level cannot acquire
> them before they are pending.)
Sounds good, that said we might want to check if there aren't missing
tests that would cover such conditions, there is a whole session of
the BAP testing spec dedicated for streaming configuration (AC-#), if
will probably need some changes to test-bap to create socketpair to
emulate CIS since those tests require CIS/BIS in order to pass.
> ---
> src/shared/bap.c | 170 +++++++++++++++++++++++++++++++++--------------
> src/shared/bap.h | 2 +
> 2 files changed, 123 insertions(+), 49 deletions(-)
>
> diff --git a/src/shared/bap.c b/src/shared/bap.c
> index 976e3c0b1..456450d40 100644
> --- a/src/shared/bap.c
> +++ b/src/shared/bap.c
> @@ -296,6 +296,8 @@ struct bt_bap_stream {
> struct queue *pending_states;
> bool no_cache_config;
> bool client;
> + bool client_active;
> + bool need_reconfig;
> void *user_data;
> };
>
> @@ -1488,6 +1490,13 @@ static void bap_stream_state_changed(struct bt_bap_stream *stream)
> if (stream->client)
> bt_bap_stream_stop(stream, stream_stop_complete, NULL);
> break;
> + case BT_ASCS_ASE_STATE_RELEASING:
> + if (stream->client) {
> + bap_stream_clear_cfm(stream);
> + bap_stream_io_detach(stream);
> + bt_bap_stream_io_unlink(stream, NULL);
> + }
> + break;
> }
> }
>
> @@ -1898,6 +1907,9 @@ static unsigned int bap_ucast_qos(struct bt_bap_stream *stream,
> if (!stream->client)
> return 0;
>
> + if (stream->need_reconfig)
> + return 0;
> +
> memset(&qos, 0, sizeof(qos));
>
> /* TODO: Figure out how to pass these values around */
> @@ -2300,7 +2312,6 @@ static unsigned int bap_ucast_release(struct bt_bap_stream *stream,
> /* If stream does not belong to a client session, clean it up now */
> if (!bap_stream_valid(stream)) {
> stream_set_state(stream, BT_BAP_STREAM_STATE_IDLE);
> - stream = NULL;
> return 0;
> }
>
> @@ -2583,6 +2594,9 @@ static int bap_ucast_io_link(struct bt_bap_stream *stream,
> stream->ep->dir == link->ep->dir)
> return -EINVAL;
>
> + if (stream->client && !(stream->client_active && link->client_active))
> + return -EINVAL;
> +
> if (!stream->links)
> stream->links = queue_new();
>
> @@ -2603,6 +2617,30 @@ static int bap_ucast_io_link(struct bt_bap_stream *stream,
> return 0;
> }
>
> +static void stream_unlink_ucast(void *data)
> +{
> + struct bt_bap_stream *link = data;
> +
> + DBG(link->bap, "stream %p unlink", link);
> +
> + queue_destroy(link->links, NULL);
> + link->links = NULL;
> +}
> +
> +static int bap_ucast_io_unlink(struct bt_bap_stream *stream,
> + struct bt_bap_stream *link)
> +{
> + if (!stream)
> + return -EINVAL;
> +
> + queue_destroy(stream->links, stream_unlink_ucast);
> + stream->links = NULL;
> +
> + DBG(stream->bap, "stream %p unlink", stream);
> + return 0;
> +
> +}
> +
> static void stream_link(void *data, void *user_data)
> {
> struct bt_bap_stream *stream = (void *)data;
> @@ -2708,7 +2746,7 @@ static const struct bt_bap_stream_ops stream_ops[] = {
> bap_ucast_release, bap_ucast_detach,
> bap_ucast_set_io, bap_ucast_get_io,
> bap_ucast_io_dir, bap_ucast_io_link,
> - NULL),
> + bap_ucast_io_unlink),
> STREAM_OPS(BT_BAP_SOURCE, bap_ucast_set_state,
> bap_ucast_get_state,
> bap_ucast_config, bap_ucast_qos, bap_ucast_enable,
> @@ -2718,7 +2756,7 @@ static const struct bt_bap_stream_ops stream_ops[] = {
> bap_ucast_release, bap_ucast_detach,
> bap_ucast_set_io, bap_ucast_get_io,
> bap_ucast_io_dir, bap_ucast_io_link,
> - NULL),
> + bap_ucast_io_unlink),
> STREAM_OPS(BT_BAP_BCAST_SINK, bap_bcast_set_state,
> bap_bcast_get_state,
> bap_bcast_config, bap_bcast_qos, bap_bcast_sink_enable,
> @@ -5015,6 +5053,8 @@ static void ep_status_config(struct bt_bap *bap, struct bt_bap_endpoint *ep,
> ep->stream->cc = new0(struct iovec, 1);
>
> util_iov_memcpy(ep->stream->cc, cfg->cc, cfg->cc_len);
> +
> + ep->stream->need_reconfig = false;
> }
>
> static void bap_stream_config_cfm_cb(struct bt_bap_stream *stream, int err)
> @@ -5922,43 +5962,6 @@ bool bt_bap_pac_bcast_is_local(struct bt_bap *bap, struct bt_bap_pac *pac)
> return false;
> }
>
> -static bool find_ep_unused(const void *data, const void *user_data)
> -{
> - const struct bt_bap_endpoint *ep = data;
> - const struct match_pac *match = user_data;
> -
> - if (ep->stream)
> - return false;
> -
> - if (match->rpac)
> - return ep->dir == match->rpac->type;
> - else
> - return true;
> -}
> -
> -static bool find_ep_pacs(const void *data, const void *user_data)
> -{
> - const struct bt_bap_endpoint *ep = data;
> - const struct match_pac *match = user_data;
> -
> - if (!ep->stream)
> - return false;
> -
> - if (ep->stream->lpac != match->lpac)
> - return false;
> -
> - if (ep->stream->rpac != match->rpac)
> - return false;
> -
> - switch (ep->state) {
> - case BT_BAP_STREAM_STATE_CONFIG:
> - case BT_BAP_STREAM_STATE_QOS:
> - return true;
> - }
> -
> - return false;
> -}
> -
> static bool find_ep_source(const void *data, const void *user_data)
> {
> const struct bt_bap_endpoint *ep = data;
> @@ -6136,6 +6139,48 @@ static struct bt_bap_stream *bap_bcast_stream_new(struct bt_bap *bap,
> return stream;
> }
>
> +static bool find_ep_ucast(const void *data, const void *user_data)
> +{
> + const struct bt_bap_endpoint *ep = data;
> + const struct match_pac *match = user_data;
> +
> + if (ep->stream) {
> + if (!ep->stream->client)
> + return false;
> + if (ep->stream->client_active)
> + return false;
> + if (!queue_isempty(ep->stream->pending_states))
> + return false;
> +
> + switch (ep->stream->state) {
> + case BT_BAP_STREAM_STATE_IDLE:
> + case BT_BAP_STREAM_STATE_CONFIG:
> + case BT_BAP_STREAM_STATE_QOS:
> + break;
> + default:
> + return false;
> + }
> + }
> +
> + if (ep->dir != match->rpac->type)
> + return false;
> +
> + switch (match->lpac->type) {
> + case BT_BAP_SOURCE:
> + if (ep->dir != BT_BAP_SINK)
> + return false;
> + break;
> + case BT_BAP_SINK:
> + if (ep->dir != BT_BAP_SOURCE)
> + return false;
> + break;
> + default:
> + return false;
> + }
> +
> + return true;
> +}
> +
> static struct bt_bap_stream *bap_ucast_stream_new(struct bt_bap *bap,
> struct bt_bap_pac *lpac,
> struct bt_bap_pac *rpac,
> @@ -6153,21 +6198,28 @@ static struct bt_bap_stream *bap_ucast_stream_new(struct bt_bap *bap,
> match.lpac = lpac;
> match.rpac = rpac;
>
> - /* Check for existing stream */
> - ep = queue_find(bap->remote_eps, find_ep_pacs, &match);
> + /* Get free ASE */
> + ep = queue_find(bap->remote_eps, find_ep_ucast, &match);
> if (!ep) {
> - /* Check for unused ASE */
> - ep = queue_find(bap->remote_eps, find_ep_unused, &match);
> - if (!ep) {
> - DBG(bap, "Unable to find unused ASE");
> - return NULL;
> - }
> + DBG(bap, "Unable to find usable ASE");
> + return NULL;
> }
>
> stream = ep->stream;
> - if (!stream)
> + if (stream) {
> + /* Replace lpac: the stream generally needs to be reconfigured
> + * after this, otherwise things like codec config not match.
> + */
> + bap_stream_clear_cfm(stream);
> + stream->lpac = lpac;
> + util_iov_free(stream->cc, 1);
> + stream->cc = util_iov_dup(data, 1);
> + stream->need_reconfig = true;
> + } else {
> stream = bap_stream_new(bap, ep, lpac, rpac, data, true);
> + }
>
> + stream->client_active = true;
> return stream;
> }
>
> @@ -6187,6 +6239,26 @@ struct bt_bap_stream *bt_bap_stream_new(struct bt_bap *bap,
> return bap_bcast_stream_new(bap, lpac, pqos, data);
> }
>
> +void bt_bap_stream_discard(struct bt_bap_stream *stream)
> +{
> + if (!stream || !stream->client)
> + return;
> +
> + stream->client_active = false;
> +
> + switch (stream->ep->state) {
> + case BT_BAP_STREAM_STATE_IDLE:
> + case BT_BAP_STREAM_STATE_RELEASING:
> + break;
> + case BT_BAP_STREAM_STATE_CONFIG:
> + if (stream->ep->old_state == BT_BAP_STREAM_STATE_RELEASING)
> + break;
> + /* Fallthrough */
> + default:
> + bt_bap_stream_release(stream, NULL, NULL);
> + }
> +}
> +
> struct bt_bap *bt_bap_stream_get_session(struct bt_bap_stream *stream)
> {
> if (!stream)
> diff --git a/src/shared/bap.h b/src/shared/bap.h
> index d10581428..5949eb4b1 100644
> --- a/src/shared/bap.h
> +++ b/src/shared/bap.h
> @@ -183,6 +183,8 @@ struct bt_bap_stream *bt_bap_stream_new(struct bt_bap *bap,
> struct bt_bap_qos *pqos,
> struct iovec *data);
>
> +void bt_bap_stream_discard(struct bt_bap_stream *stream);
> +
> struct bt_bap *bt_bap_stream_get_session(struct bt_bap_stream *stream);
> uint8_t bt_bap_stream_get_state(struct bt_bap_stream *stream);
>
> --
> 2.49.0
>
>
--
Luiz Augusto von Dentz
