jamulussoftware / jamulussoftware/jamulus
Headless client: the gain/pan rate limiter never adapts to ping time, and branches on an uninitialised value
- Dominant language
- C
- Stars
- 1.1k
- Forks
- 248
- Avg merge
- 2d 3h
- Merged PRs (30d)
- 9
Description
**🤖 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.*
Contributor guide
Research direction
Start in src/client.cpp and src/client.h at CClient::StartTimerGainOrPan, iCurPingTime, and the constructor, then trace ping handling through src/clientdlg.cpp and CClientDlg::OnTimerPing. Compare the two stated approaches for headless clients, and validate the chosen behavior with a headless session, valgrind, and fader-message spacing under nonzero round-trip delay.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- networking, performance
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100