Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
MIME-Version: 1.0
References: <20230922192335.1060601-1-luiz.dentz@gmail.com>
 <20230922192335.1060601-6-luiz.dentz@gmail.com> <9100eb19ee8fb16e78cfee86e953dfe2afd6efe9.camel@iki.fi>
In-Reply-To: <9100eb19ee8fb16e78cfee86e953dfe2afd6efe9.camel@iki.fi>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 25 Sep 2023 12:24:37 -0700
Message-ID: <CABBYNZJ51nCXhmohvjr+XMuJ5yb1fw-Wao50RyyGuBx+NADmSg@mail.gmail.com>
Subject: Re: [PATCH v2 6/6] doc/media: Convert media-api.rst into manpages
To:     Pauli Virtanen <pav@iki.fi>
Cc:     linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Pauli,

On Sat, Sep 23, 2023 at 2:14=E2=80=AFAM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi Luiz,
>
> pe, 2023-09-22 kello 12:23 -0700, Luiz Augusto von Dentz kirjoitti:
> > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >
> > This splits media-api.rst into org.bluez.Media<interface>.rst and
> > generate manpages for them.
>
> I made a pass looking at the implementations of the transport/endpoint
> and SetConfiguration/SelectProperties/SelectConfiguration and comparing
> to docs.
>
> Note that the SelectProperties API has a problem with BAP endpoint
> configuration. We maybe should redesign it while we still can?
>
> I think generally at least from Pipewire side there's no need to keep
> backward compatibility while this is experimental. We can target BlueZ
> master branch.
>
>
> SelectProperties API problem:
>
> As BAP client one first does Config Codec (ASCS =C2=A75.1) which takes
> Target_Latency, Target_PHY and Codec.
>
> After this, the ASE properties (ACSS Table 4.3) in Codec Configured
> state contain the server supported values for Max_Transport_Latency,
> Presentation_Delay_Min/Max etc.
>
> SelectProperties is called before Config Codec, so it cannot
> necessarily know the server supported values. In this case the sound
> server cannot fill in QoS correctly.
>
> The client-invoked SetConfiguration API also seems to have similar
> issue.
>
> Probably: SelectProperties should be called twice, once to get
> parameters for Config Codec, and then again to get parameters for
> Config QoS. Or, there should be a separate "SelectCodec" and
> "SelectQoS" calls. Calling "SelectProperties" twice could be simpler
> for everyone.
>
> In client-invoked "SetConfiguration" API, one probably should only pass
> in the parameters needed for Config Codec, and BlueZ should then make a
> SelectProperties call to get the QoS ones once the server-side values
> are known.
>
> I can take a look at this...

Sound like a good idea, it means the actual configuration is always
done via SelectProperties, that said we need to make sure this works
with A2DP as well or state that it only applies to BAP, we could
perhaps introduce a property to indicate that the endpoint implements
SelectProperties.

>
> MediaTransport:
>
> "Delay" field only exists for A2DP in code.

Yeah, the PresentationDelay was moved into QoS property.

> "Volume" field only exists for A2DP in code.

That is subject to change though.

> "Links" is ucast only in code.

Will mark it as unicast only.

> "QoS.TargetLatency" does not exist in code. Maybe it should be exposed
> for consistency, since it's expected as return from SelectProperties.

Yeah, it seems we never implemented it, but I consider it a gap not a
inconsistency with the code.

>
> SetConfiguration:
>
> SetConfiguration properties dict when called by BlueZ contains exactly
> the properties of the transport. The documentation probably should say
> that this is so.
>
> When called by client, the contents of the properties dict are expected
> to be different. QoS parameters are not packed in a "QoS" dict. Maybe
> they should be. The "PHY" key is also still a string here.

Is it? Or perhaps I had changed it later on when splitting the
documentation on a per-interface, at least on
org.bluez.MediaEndpoint.rst there is no entry were PHY is a string, or
you are talking about the actual code here?

> The documented input parameters expected from client are wrong, the
> documentation explains fields of struct bt_bap_pac_qos, but what the
> code parses are that of struct bt_bap_qos.
>
> "MaximumLatency": no such field in code, seems to be called "Latency"
> everywhere.

The way Im designing these properties are that MediaEndpoint actually
carries the preferences and MediaTransport the configuration as far as
BAP is concerned, perhaps I shall actually state it though.

>
> SelectProperties:
>
> In code, the return parameter expects QoS parameters (struct
> bt_bap_qos) to be packed in "QoS" dict.
>
> They are not packed in "QoS" dict in the input parameters. These are
> not exactly QoS, but instead struct bt_bap_pac_qos, so not clear if
> they should be packed.

Input shall be considered the remote preferences and the output the
local configuration.

> The input and return parameters are not documented. The input QoS
> contains fields of struct bt_bap_pac_qos. The return QoS has fields of
> struct bt_bap_qos.

I will attempt to clarify that.

>
> MediaEndpoint:
>
> The endpoints published by BlueZ only have the properties
> "UUID", "Codec", "Capabilities", "Device".
>
> The documentation lists various other things, but they are not
> implemented as server endpoint properties.
>
> As client endpoint properties:
>
> "RTN" is expected to be present in code, but is not documented.
>
> "MaximumLatency" "Location" "SupportedContext" "Context" are documented
> but not present in code.
>
>
> --
> Pauli Virtanen


--=20
Luiz Augusto von Dentz