jamulussoftware / jamulussoftware/jamulus
Raw audio should be declared, not inferred from packet size
- Lingua principale
- C
- Stelle
- 1.1k
- Fork
- 248
- Merge medio
- 2g 3h
- PR unite (30g)
- 9
Descrizione
**🤖 AI:** Raw audio is detected by measuring the packet, not by asking the client. [`CServer::DecodeReceiveData`](https://github.com/jamulussoftware/jamulus/blob/b3c93588bb447d2c47987aba35284131a3e3c456/src/server.cpp#L976-L977) decides a stream is uncompressed PCM when `iCeltNumCodedBytes == sizeof(int16_t) * iClientFrameSizeSamples * channels`, and [the inverted send-side test](https://github.com/jamulussoftware/jamulus/blob/b3c93588bb447d2c47987aba35284131a3e3c456/src/server.cpp#L1266). That reserves four coded-byte counts — 256, 512, 128, and 256 again — which are also legal declarations for a *compressed* stream, since [`EvaluateNetwTranspPropsMes`](https://github.com/jamulussoftware/jamulus/blob/b3c93588bb447d2c47987aba35284131a3e3c456/src/protocol.cpp#L1505-L1509) accepts any size in [`CELT_MINIMUM_NUM_BYTES` 10, `MAX_SIZE_BYTES_NETW_BUF` 20000].
A client that declares `CT_OPUS` mono and sends real Opus at 256 coded bytes has its bitstream copied into the mix verbatim as int16 PCM. Now it's measured, with a purpose-built third-party client sending a 440 Hz sine, against a stock server started `-s -n -R`:
| declared coded bytes | server's own recording |
|---:|---|
| 255 (control) | 440.0 Hz, 100.0% of the energy within ±20 Hz, 0.0% above 5 kHz |
| 256 (collides) | no tone, peak bin 22500 Hz, 81.5% of the energy above 5 kHz |
Full-scale broadband noise. A separate run with a plain Opus listener joined as a second participant shows the mix carries it too, 77.6% of the energy above 5 kHz against 0.0% for the control, so this is not an artefact of how the recorder writes files. Both runs log the same `connected (1)` and nothing else — there is no error path, because a size test cannot fail.
All four reserved sizes behave the same way, and each one is a size the stock client itself emits — the declarations below were captured off the wire from the stock client, not derived:
| configuration | stock client declares | reserved | collide arm | N−1 control |
|---|---|---:|---|---|
| `CT_OPUS` mono | netwsize=257 fact=1 chans=1 codec=2 | 256 | 22500 Hz peak, 81.5% above 5 kHz | 440.0 Hz, 100.0% |
| `CT_OPUS` stereo | netwsize=513 fact=1 chans=2 codec=2 | 512 | 21750 Hz peak, 78.7% above 5 kHz | 440.0 Hz, 100.0% |
| `CT_OPUS64` mono | netwsize=129 fact=1 chans=1 codec=3 | 128 | 19500 Hz peak, 78.8% above 5 kHz | 440.0 Hz, 100.0% |
| `CT_OPUS64` stereo | netwsize=257 fact=1 chans=2 codec=3 | 256 | 6750 Hz peak, 76.9% above 5 kHz | 440.0 Hz, 100.0% |
The last row is the one that resists a narrow fix: `CT_OPUS` mono and `CT_OPUS64` stereo reserve the same 256 and put the same 257 bytes on the wire, so special-casing one configuration is wrong for the other while looking correct.
This is not a new concern: [a sentinel-byte proposal](https://github.com/jamulussoftware/jamulus/pull/3653#discussion_r3132422723) raised recognising the audio frame this way during review.
The reservation is also unwritten. [The protocol documentation block](https://github.com/jamulussoftware/jamulus/blob/b3c93588bb447d2c47987aba35284131a3e3c456/src/protocol.cpp#L260-L262) describes `PROTMESSID_RAWAUDIO_SUPPORTED` as carrying no data and says nothing about four coded-byte counts being spoken for, so an implementer reading the protocol section has no way to learn which sizes are unsafe. The constraint lives only in the two arithmetic expressions above.
### The field to carry the declaration is already on the wire
`CNetworkTransportProps` has a 4-byte `iAudioCodingArg` that is [serialised on send](https://github.com/jamulussoftware/jamulus/blob/b3c93588bb447d2c47987aba35284131a3e3c456/src/protocol.cpp#L1476), [parsed on receipt](https://github.com/jamulussoftware/jamulus/blob/b3c93588bb447d2c47987aba35284131a3e3c456/src/protocol.cpp#L1549) and [stored unread](https://github.com/jamulussoftware/jamulus/blob/b3c93588bb447d2c47987aba35284131a3e3c456/src/util.h#L1098) — and whose value nothing in the product reads. [`GetNetworkTransportPropsFromCurrentSettings`](https://github.com/jamulussoftware/jamulus/blob/b3c93588bb447d2c47987aba35284131a3e3c456/src/channel.cpp#L519-L526) passes a literal `0` for it. So the client→server declaration channel exists already, in the same message that declares the size, costing no new protocol message and no new bytes. It also complements [`PROTMESSID_RAWAUDIO_SUPPORTED`](https://github.com/jamulussoftware/jamulus/blob/b3c93588bb447d2c47987aba35284131a3e3c456/src/protocol.h#L87), which today runs server→client only, so the server announces a capability and the client never answers.
A patch doing that is **31 lines across 5 files**: set the bit where the client already knows the answer, read it in [`OnNetTranspPropsReceived`](https://github.com/jamulussoftware/jamulus/blob/b3c93588bb447d2c47987aba35284131a3e3c456/src/channel.cpp#L427), and replace both size tests with the stored flag. Built and re-run against the same rigs, every colliding size now behaves like its non-colliding control:
| reserved size | stock build | patched build |
|---|---|---|
| `CT_OPUS` mono 256 | 22500 Hz peak, 81.5% above 5 kHz | 440.0 Hz, 100.0% concentration |
| `CT_OPUS` stereo 512 | 21750 Hz peak, 78.7% above 5 kHz | 440.0 Hz, 100.0% |
| `CT_OPUS64` mono 128 | 19500 Hz peak, 78.8% above 5 kHz | 440.0 Hz, 100.0% |
| `CT_OPUS64` stereo 256 | 6750 Hz peak, 76.9% above 5 kHz | 440.0 Hz, 100.0% |
All sixteen arms on one machine: in the patched build every colliding size and every N−1 control lands on 440.0 Hz with 100.0% of the energy within ±20 Hz and **0.0%** above 5 kHz, so a reserved size is no longer distinguishable from any other size.
The compatibility cost is the part worth arguing about, so it is measured too. Because both directions switch on the same flag, the server→client stream shows which branch the server took even when the client sends silence: PCM silence is ~all zero bytes, a coded frame never is.
| server | client | server→client payload | outcome |
|---|---|---|---|
| patched | patched | 99.6% zero bytes | raw audio works |
| stock | stock | 99.6% zero bytes | unchanged |
| stock | patched | 99.6% zero bytes | a patched client still gets raw from a beta server |
| patched | **stock** | 68.2% zero bytes, coded | **a beta-era client loses raw** |
That last row is the whole decision. Making the declaration authoritative with no size fallback breaks raw audio for clients already built from the beta line. Today that population is beta testers. After 4.0.0 it is released users, and the fallback — with both collisions intact — becomes permanent.
### Why the timing matters
Raw audio is in no 3.12.x release — `git show r3_12_4:src/protocol.h | grep -c RAWAUDIO_SUPPORTED` returns 0 — and ships only in [r4_0_0beta1](https://github.com/jamulussoftware/jamulus/releases/tag/r4_0_0beta1) and [r4_0_0beta2](https://github.com/jamulussoftware/jamulus/releases/tag/r4_0_0beta2). While that is true, the wire contract can still be changed against beta users. After 4.0.0, a correct server has to keep the size test as a fallback for released clients, so the ambiguity — and both reserved-size collisions — become permanent, and the fix grows a compatibility branch instead of replacing code.
Two smaller things found in the same audit, both from the merged diff:
- [`--noraw`](https://github.com/jamulussoftware/jamulus/blob/b3c93588bb447d2c47987aba35284131a3e3c456/src/server.cpp#L405-L409) suppresses the advertisement only. The decode and encode paths are unconditional, so a server started with it still speaks raw to any client that sends it. Without a declaration there is also no way for a server to log, warn about, or refuse an incoming raw stream.
- [`CClient::OnRawAudioSupported`](https://github.com/jamulussoftware/jamulus/blob/b3c93588bb447d2c47987aba35284131a3e3c456/src/client.cpp#L1033-L1050) runs `Sound.Stop() / Init() / Sound.Start()` with no check on `eAudioQuality`. Every branch reading `bRawAudioIsSupported` sits under `case AQ_RAW`, so a Low/Normal/High client re-initialises its audio device for a flag that cannot change any of its settings. Counted with gdb breakpoints on a stock build, 3/3 reps: connect-time `CSound::Init()` goes from 4 to 8, plus a device stop/start, versus a `--noraw` control. That interacts with [#3892](https://github.com/jamulussoftware/jamulus/issues/3892), where each `CSound::Init()` costs hundreds of milliseconds on a real ASIO device.
Rig, harness and the full method are available if useful. The behaviour of raw audio under packet loss is a separate matter, covered in [#3895](https://github.com/jamulussoftware/jamulus/issues/3895).
---
🤖 *This message was written by AI and reviewed by @mcfnord.*
Guida per i contributori
Apri la guida per i contributori
Direzione di ricerca
Inizia con CNetworkTransportProps e OnNetTranspPropsReceived in src/channel.cpp, poi segui i controlli delle dimensioni in src/server.cpp e la serializzazione in src/protocol.cpp. Usa l’harness audio dell’issue per confrontare tutte e quattro le dimensioni riservate con controlli N−1 e verifica la compatibilità tra client stock e patchati; completato significa che l’audio raw non viene più classificato erroneamente senza compromettere i client beta.
Scritto dal modello di indicizzazione a partire dal testo della issue.
Valutazione
- Stack tecnologico
- c
- Ambito
- audio-video-rtc
- Tipo di issue
- Bug
- Difficoltà
- 4/5
- Tempo stimato
- 3-5 giorni
- Stato di attività
- Tranquilla
- Chiarezza
- Specificata chiaramente
- Idoneità per principianti
- 45/100