jamulussoftware / jamulussoftware/jamulus

Headless client: the gain/pan rate limiter never adapts to ping time, and branches on an uninitialised value

Aperta
#3,874 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub

Nessuno ha ancora preso questa issue.

AI
Lingua principale
C
Stelle
1.1k
Fork
248
Merge medio
2g 3h
PR unite (30g)
9

Descrizione

**🤖 AI:** The rate limiter added in #2535 has two halves: a [50 ms floor](https://github.com/jamulussoftware/jamulus/blob/c862872e7beacdc8c9fd232cb33def2d93ee01d1/src/client.h#L113), and double the current ping time, whichever is greater. In a headless build only the floor is ever used, and the value the branch tests has never been written.

[`CClient::StartTimerGainOrPan`](https://github.com/jamulussoftware/jamulus/blob/c862872e7beacdc8c9fd232cb33def2d93ee01d1/src/client.cpp#L575-L591) tests [`iCurPingTime`](https://github.com/jamulussoftware/jamulus/blob/c862872e7beacdc8c9fd232cb33def2d93ee01d1/src/client.h#L479). That member is assigned in [exactly one place](https://github.com/jamulussoftware/jamulus/blob/c862872e7beacdc8c9fd232cb33def2d93ee01d1/src/client.cpp#L449), `CClient::OnCLPingReceived`, which is reached only after [`CClient::CreateCLPingMes()`](https://github.com/jamulussoftware/jamulus/blob/c862872e7beacdc8c9fd232cb33def2d93ee01d1/src/client.h#L315). That function has [one caller](https://github.com/jamulussoftware/jamulus/blob/c862872e7beacdc8c9fd232cb33def2d93ee01d1/src/clientdlg.cpp#L1133) in the tree: `CClientDlg::OnTimerPing`. A client started with `-n` has no dialog, so no ping is sent and `iCurPingTime` is never assigned.

A recording UDP proxy sat between a real client and a real server with a symmetric delay injector; five bursts of 150 back-to-back fader changes per trial; [`CLM_PING_MS`](https://github.com/jamulussoftware/jamulus/blob/c862872e7beacdc8c9fd232cb33def2d93ee01d1/src/protocol.h#L91) (1001) frames counted and gain-message spacing read off the capture:

| client | round trip | CLM_PING_MS frames on wire | gain spacing |
|---|---:|---:|---:|
| headless | 0 ms | 0 | 49 ms |
| headless | 122 ms | 0 | 121 ms |
| GUI | 2 ms | 61 | 49 ms |
| GUI | 60 ms | 66 | 122 ms |
| GUI | 122 ms | 64 | 244 ms |

Spacing follows `max(timer period, round trip)` within 2 ms across all five trials, with the period being `max(50, 2 x ping)` for the GUI client and a constant 50 ms for the headless one. The headless 121 ms is the protocol's one-unacknowledged-message-at-a-time limit, not adaptation; only the 0 ms row separates the two.

### Uninitialised read

`iCurPingTime` has no initialiser and is absent from the [constructor's initialiser list](https://github.com/jamulussoftware/jamulus/blob/c862872e7beacdc8c9fd232cb33def2d93ee01d1/src/client.cpp#L52-L97), and main.cpp constructs `CClient` [on the stack](https://github.com/jamulussoftware/jamulus/blob/c862872e7beacdc8c9fd232cb33def2d93ee01d1/src/main.cpp#L985). valgrind memcheck with `--track-origins=yes` reports exactly one uninitialised-conditional context in a full headless session, and it is this branch:

Conditional jump or move depends on uninitialised value(s)
at CClient::StartTimerGainOrPan()
by CClient::SetRemoteChanGain(int, float, bool)
by CClient::OnControllerInFaderLevel(int, int)
...
Uninitialised value was created by a stack allocation
at main

### Effect

#2492 reported MIDI fader changes lagging the audio by about 10 s, which #2535 fixed. The same symptom is reproducible in a headless client at a round trip of 122 ms, because gain messages are generated faster than the acknowledgement gate clears them. 600 s of continuous fader motion at 50 ms intervals:

| | headless | GUI |
|---|---:|---:|
| fader changes driven | 11,938 | 11,940 |
| gain messages generated | 11,938 (nothing coalesced) | 2,402 |
| sent during the drive | 4,963 | 2,401 |
| still queued when the fader stopped | 6,975 | 1 |
| drain rate | 8.27/s (one per 121 ms) | — |
| delay on a chat message sent at that moment | **843 s** | 0.0 s |

The queue itself is cheap: RSS grew 228 KB for those 6,975 messages, which is less than the GUI client's ordinary growth over the same ten minutes, and the client's outbound audio cadence is unchanged between an empty queue and a 6,975-deep one (median/p95/p99 3/3/3 ms in both windows). The wire rate stays at one protocol message per round trip throughout. What grows is the delay on everything else sharing the send queue — chat, mute state, jitter buffer size, channel info all wait behind the gain messages.

The onset sits at the limiter's own floor. The same fader drive held for 60 s per round-trip step, with a chat message sent at drive end through the same queue as a wire-visible marker:

| round trip | queued at drive end | chat marker delay |
|---:|---:|---:|
| 0 ms | 0 | 0 ms |
| 22 ms | 0 | 0 ms |
| 32 ms | 0 | 0 ms |
| 40 ms | 0 | 0 ms |
| 52 ms | 18 | 935 ms |
| 62 ms | 211 | 12.9 s |
| 80 ms | 453 | 36.7 s |
| 122 ms | 698 | 84.5 s |
| 122 ms (GUI) | 0 | 99 ms |

Every measured round trip above the 50 ms floor builds a backlog; every one below it does not.

Headless clients take fader input from [`--ctrlmidich`](https://github.com/jamulussoftware/jamulus/blob/c862872e7beacdc8c9fd232cb33def2d93ee01d1/src/main.cpp#L593) and from [`jamulusclient/setFaderLevel`](https://github.com/jamulussoftware/jamulus/blob/c862872e7beacdc8c9fd232cb33def2d93ee01d1/src/clientrpc.cpp#L395), which is the same path #2492 came in on.

### Two directions

Initialising `iCurPingTime` removes the undefined behaviour and leaves the headless limiter at the fixed floor. Moving [the ping timer](https://github.com/jamulussoftware/jamulus/blob/c862872e7beacdc8c9fd232cb33def2d93ee01d1/src/clientdlg.cpp#L1240) out of `CClientDlg` into `CClient` makes both builds adapt, at the cost of a headless client sending a ping [every 500 ms](https://github.com/jamulussoftware/jamulus/blob/c862872e7beacdc8c9fd232cb33def2d93ee01d1/src/global.h#L245) it currently does not send. Which is preferred?

Rig: Jamulus 3.12.3dev, Raspberry Pi 4 aarch64, Qt 5.15.15, `jackd -d dummy`, valgrind 3.24.0. The headless binary is fa8766ad and the GUI binary 24f8f95; `git diff 24f8f95 fa8766ad -- src/client.cpp src/client.h src/clientdlg.cpp src/clientdlg.h src/global.h` is empty, so the two arms differ in build configuration only. All line references above are to current `main` (c862872e); between it and the measured tree, the only changed line containing any identifier cited here is an indentation-only move of `TimerPing.start`, so the code measured is the code on `main`.

---

🤖 *This message was written by AI and reviewed by @mcfnord.*

Guida per i contributori

Apri la guida per i contributori

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Direzione di ricerca

Inizia in src/client.cpp e src/client.h, in corrispondenza di CClient::StartTimerGainOrPan, iCurPingTime e del costruttore, quindi segui la gestione del ping attraverso src/clientdlg.cpp e CClientDlg::OnTimerPing. Confronta i due approcci indicati per gli headless clients e valida il comportamento scelto con una headless session, valgrind e la spaziatura dei fader-message in presenza di un ritardo di andata e ritorno diverso da zero.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Valutazione

Stack tecnologico
cpp
Ambito
networking, performance
Tipo di issue
Bug
Difficoltà
4/5
Tempo stimato
3-5 giorni
Stato di attività
Tranquilla
Chiarezza
Abbastanza chiara
Idoneità per principianti
48/100

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.