Hi Luiz,
ma, 2024-04-01 kello 12:00 -0400, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
>
> On Mon, Apr 1, 2024 at 11:05 AM Pauli Virtanen <pav@xxxxxx> wrote:
> >
> > Hi,
> >
> > ti, 2024-03-26 kello 11:33 -0400, Luiz Augusto von Dentz kirjoitti:
> > > Hi Vlad,
> > >
> > > On Tue, Mar 26, 2024 at 11:21 AM Vlad Pruteanu <vlad.pruteanu@xxxxxxx> wrote:
> > > >
> > > > Hello Luiz,
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
> > > > > Sent: Monday, March 25, 2024 6:04 PM
> > > > > To: Vlad Pruteanu <vlad.pruteanu@xxxxxxx>
> > > > > Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Mihai-Octavian Urzica <mihai-
> > > > > octavian.urzica@xxxxxxx>; Silviu Florian Barbulescu
> > > > > <silviu.barbulescu@xxxxxxx>; Iulia Tanasescu <iulia.tanasescu@xxxxxxx>;
> > > > > Andrei Istodorescu <andrei.istodorescu@xxxxxxx>
> > > > > Subject: [EXT] Re: [PATCH BlueZ 1/1] client/player: Fix transport.send
> > > > > command's transfer of packets
> > > > >
> > > > > Caution: This is an external email. Please take care when clicking links or
> > > > > opening attachments. When in doubt, report the message using the 'Report
> > > > > this email' button
> > > > >
> > > > >
> > > > > Hi Vlad,
> > > > >
> > > > > On Mon, Mar 25, 2024 at 10:40 AM Vlad Pruteanu
> > > > > <vlad.pruteanu@xxxxxxx> wrote:
> > > > > >
> > > > > > The transport.send command sends a number num of packets at intervals
> > > > > > specified by the transport latency reported by the CIS Establsihed event.
> > > > > > Num is defined as qos.ucast.out.latency * 1000 / qos.ucast.out.interval.
> > > > > > Since this latency could be smaller than the SDU interval for some presets,
> > > > > > the resulting num would be 0, causing the file transfer to stop after the
> > > > > > first packet. Instead, one packet should be sent at SDU interval distance
> > > > > > apart.
> > > > >
> > > > > Please add some output samples showing the wrong interval ends up being
> > > > > used.
> > > > >
> > > > Below you can find what I consider to a relevant excerpt taken from btmon log:
> > > > ...
> > > > < HCI Command: LE Set Connected Isochronous Group Parameters (0x08|0x0062)
> > > > CIG ID: 0x00
> > > > Central to Peripheral SDU Interval: 10000 us (0x002710)
> > > > Peripheral to Central SDU Interval: 10000 us (0x002710)
> > > > SCA: 201 - 500 ppm (0x00)
> > > > Packing: Sequential (0x00)
> > > > Framing: Unframed (0x00)
> > > > Central to Peripheral Maximum Latency: 10 ms (0x000a)
> > > > Peripheral to Central Maximum Latency: 10 ms (0x000a)
> > > > Number of CIS: 1
> > > > CIS ID: 0x00
> > > > Central to Peripheral Maximum SDU Size: 40
> > > > Peripheral to Central Maximum SDU Size: 0
> > > > Central to Peripheral PHY: LE 2M (0x02)
> > > > Peripheral to Central PHY: LE 2M (0x02)
> > > > Central to Peripheral Retransmission attempts: 0x02
> > > > Peripheral to Central Retransmission attempts: 0x00
> > > > ...
> > > > > HCI Event: Command Complete (0x0e)
> > > > LE Set Connected Isochronous Group Parameters (0x08|0x0062) ncmd 1
> > > > Status: Success (0x00)
> > > > CIG ID: 0x00
> > > > Number of Handles: 1
> > > > Connection Handle #0: 144
> > > > ...
> > > > < HCI Command: LE Create Connected Isochronous Stream (0x08|0x0064)
> > > > Number of CIS: 1
> > > > CIS Handle: 144
> > > > ACL Handle: 128
> > > > ...
> > > > > HCI Event: LE Meta Event (0x3e)
> > > > LE Connected Isochronous Stream Established (0x19)
> > > > Status: Success (0x00)
> > > > Connection Handle: 144
> > > > CIG Synchronization Delay: 1992 us (0x0007c8)
> > > > CIS Synchronization Delay: 1992 us (0x0007c8)
> > > > Central to Peripheral Latency: 1992 us (0x0007c8)
> > > > Peripheral to Central Latency: 1992 us (0x0007c8)
> > > > Central to Peripheral PHY: LE 2M (0x02)
> > > > Peripheral to Central PHY: LE 2M (0x02)
> > > > Number of Subevents: 3
> > > > Central to Peripheral Burst Number: 1
> > > > Peripheral to Central Burst Number: 0
> > > > Central to Peripheral Flush Timeout: 1
> > > > Peripheral to Central Flush Timeout: 1
> > > > Central to Peripheral MTU: 40
> > > > Peripheral to Central MTU: 0
> > > > ISO Interval: 10.00 msec (0x0008)
> > > > ...
> > > > bluetoothd[349393]: < ISO Data TX: Handle 144 flags 0x02
> > > > > HCI Event: Number of Completed Packets (0x13)
> > > > Num handles: 1
> > > > Handle: 144 Address: A0:CD:F3:78:04:0A (Murata Manufacturing Co., Ltd.)
> > > > Count: 1
> > > > #470: len 44 (27 Kb/s)
> > > > Latency: 13 msec (13-13 msec ~13 msec)
> > > >
> > > > The Maximum Latency is correctly set to 10ms in the LE Set Connected Isochronous Group Parameters command, and the controller reports the final latency as being 1992 us. This is the value that (after conversion to ms) qos.ucast.out.latency takes in the current implementation. num is defined as num = (qos.ucast.out.latency * 1000 / qos.ucast.out.interval), with interval being the ISO interval, 10000us and therefore num will be 0 in this case. As you can see the first packet is always sent, but after that none can be seen. Instead, the Host should send 1 packet every time this function is called. The timer related change below ensures that this function is triggered at every SDU interval. So, in the end, the Host will send 1 packet every SDU interval.
> > > > > > ---
> > > > > > client/player.c | 34 +++++++++++++++++++++++++++-------
> > > > > > 1 file changed, 27 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/client/player.c b/client/player.c
> > > > > > index 1f56bfd27..f579d9904 100644
> > > > > > --- a/client/player.c
> > > > > > +++ b/client/player.c
> > > > > > @@ -34,6 +34,7 @@
> > > > > >
> > > > > > #include "lib/bluetooth.h"
> > > > > > #include "lib/uuid.h"
> > > > > > +#include "lib/iso.h"
> > > > > >
> > > > > > #include "profiles/audio/a2dp-codecs.h"
> > > > > > #include "src/shared/lc3.h"
> > > > > > @@ -5007,7 +5008,6 @@ static bool transport_timer_read(struct io *io,
> > > > > void *user_data)
> > > > > > struct bt_iso_qos qos;
> > > > > > socklen_t len;
> > > > > > int ret, fd;
> > > > > > - uint32_t num;
> > > > > > uint64_t exp;
> > > > > >
> > > > > > if (transport->fd < 0)
> > > > > > @@ -5031,10 +5031,7 @@ static bool transport_timer_read(struct io *io,
> > > > > void *user_data)
> > > > > > return false;
> > > > > > }
> > > > > >
> > > > > > - /* num of packets = latency (ms) / interval (us) */
> > > > > > - num = (qos.ucast.out.latency * 1000 / qos.ucast.out.interval);
> > > > > > -
> > > > > > - ret = transport_send_seq(transport, transport->fd, num);
> > > > > > + ret = transport_send_seq(transport, transport->fd, 1);
> > > > > > if (ret < 0) {
> > > > > > bt_shell_printf("Unable to send: %s (%d)\n",
> > > > > > strerror(-ret), ret);
> > > > > > @@ -5052,6 +5049,8 @@ static bool transport_timer_read(struct io *io,
> > > > > void *user_data)
> > > > > > static int transport_send(struct transport *transport, int fd,
> > > > > > struct bt_iso_qos *qos)
> > > > > > {
> > > > > > + struct sockaddr_iso addr;
> > > > > > + socklen_t optlen;
> > > > > > struct itimerspec ts;
> > > > > > int timer_fd;
> > > > > >
> > > > > > @@ -5068,9 +5067,30 @@ static int transport_send(struct transport
> > > > > *transport, int fd,
> > > > > > return -errno;
> > > > > >
> > > > > > memset(&ts, 0, sizeof(ts));
> > > > > > - ts.it_value.tv_nsec = qos->ucast.out.latency * 1000000;
> > > > > > - ts.it_interval.tv_nsec = qos->ucast.out.latency * 1000000;
> > > > > >
> > > > > > + /* Need to know if the transport on which data is sent is
> > > > > > + * broadcast or unicast so that the correct qos structure
> > > > > > + * can be accessed. At this point in code there no other
> > > > > > + * way of knowing this besides checking the peer address.
> > > > > > + * Broadcast will use BDADDR_ANY, while Unicast will use
> > > > > > + * the connected peer's actual address.
> > > > > > + */
> > > > > > + memset(&addr, 0, sizeof(addr));
> > > > > > + optlen = sizeof(addr);
> > > > > > +
> > > > > > + if (getpeername(transport->sk, &addr, &optlen) < 0)
> > > > > > + return -errno;
> > > > >
> > > > > The description seems to be suggesting there is a rounding error, but
> > > > > the code below only deals with the fact we didn't use proper fields
> > > > > for broadcast, so is it really fixing what is the patch description or
> > > > > something else?
> > > > >
> > > > The main change that I made here is the value of the interval for the timer.
> > > > Before, it used to be set to qos.ucast.out.latency and I propose to change
> > > > it to qos.ucast.out.interval, so that the Host will send 1SDU packet on
> > > > every ISO Interval. I included the ucast/bcast qos update since I was
> > > > already updating this line of code. That is, I didn't want to send the patch
> > > > with "qos->ucast.out.interval" without differentiating between bcast and
> > > > ucast since it would've been wrong.
> > >
> > > Hmm if I recall correctly this code used to be sending a packet per
> > > interval but we run into problems because the interval is rather short
> > > and OS itself would take time to transfer the packet from userspace to
> > > socket and then to driver, etc, so I suggest we round up the value if
> > > it ends up being 0.
> >
> > ISO_Interval is not necessarily equal to the SDU_Interval, for unframed
> > it may be an integer multiple of it, and for framed I guess it can be
> > incommensurate. See Core v5.4 Vol 6 Part G Sec 2.
> >
> > There might be similar confusion also in the
> > MediaTransport.QoS.Interval property, which IIRC for Client is
> > SDU_Interval and for Server is ISO_Interval.
> >
> > ***
> >
> > The code here probably should not use ISO_Interval for anything, since
> > this is Host -> ISOAL data flow.
> >
> > Instead, IIUC, it should determine SDU_Interval from the codec
> > parameters and for LC3 it is the frame duration. Then it should (on
> > average) send one SDU per SDU_Interval.
> >
> > Or, send N*SDU every N*SDU_Interval for some N>=1. But since it's using
> > timerfd intervals here for the timing, I'm not that sure it is really
> > necessary as the average data rate is then maintained even if some
> > wakeups are late and the controller flow control should smooth it out.
>
> Well afaik there is no flow control when using unframed, so the
> controller will not smooth anything it will just take whatever there
> is in its buffers and send each at its own interval, so if we don't
> want to risk being late and thus cause no data at each interval we
> better pre-load them, which is what we have been doing since the first
> set of packets is sent immediately, after establishing the connection,
> and then we send at each latency, we could perhaps just send on
> POLLOUT though so we use the Number of Complete Packets aka. TX
> Complete to control the flow that way we don't have to do any timing
> calculations to know when we need to send the next packet, the problem
> is that normally POLLOUT is generated when there are socket buffers
> not really controller buffers, so afaik we would be buffering a lot
> more than expected.
So the flow control behavior observed is that if a packet is late,
controller does not drop it but uses it for the next interval. Each
miss then increases the latency until you reach a value where it's
larger than your userspace jitter, and you no longer should run out.
I don't see sending N*SDU each N*SDU_Interval changing this: after
N*SDU_Interval you are in a situation where socket/controller queue is
again empty, and late wakeup causes a miss.
To preload, I think you'd need to send M > N packets on the first timer
round, and then you'd have M-N packets of latency buffer in the queues.
>
> > >
> > > > > > + if (!(bacmp(&addr.iso_bdaddr, BDADDR_ANY))) {
> > > > > > + /* Interval is measured in us, multiply by 1000 to get ns */
> > > > > > + ts.it_value.tv_nsec = qos->bcast.out.interval * 1000;
> > > > > > + ts.it_interval.tv_nsec = qos->bcast.out.interval * 1000;
> > > > > > + } else {
> > > > > > + /* Interval is measured in us, multiply by 1000 to get ns */
> > > > > > + ts.it_value.tv_nsec = qos->ucast.out.interval * 1000;
> > > > > > + ts.it_interval.tv_nsec = qos->ucast.out.interval * 1000;
> > > > > > +
> > > > > > + }
> > > > > > if (timerfd_settime(timer_fd, TFD_TIMER_ABSTIME, &ts, NULL) < 0)
> > > > > > return -errno;
> > > > > >
> > > > > > --
> > > > > > 2.39.2
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Luiz Augusto von Dentz
> > >
> > >
> > >
> >
>
>
