Skip to content

Raw audio should be declared, not inferred from packet size #3894

Description

@mcfnord

🤖 AI: Raw audio is detected by measuring the packet, not by asking the client. CServer::DecodeReceiveData decides a stream is uncompressed PCM when iCeltNumCodedBytes == sizeof(int16_t) * iClientFrameSizeSamples * channels, and the inverted send-side test. That reserves four coded-byte counts — 256, 512, 128, and 256 again — which are also legal declarations for a compressed stream, since EvaluateNetwTranspPropsMes 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 raised recognising the audio frame this way during review.

The reservation is also unwritten. The protocol documentation block 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, parsed on receipt and stored unread — and whose value nothing in the product reads. GetNetworkTransportPropsFromCurrentSettings 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, 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, 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 and 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 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 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, 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.


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

Metadata

Metadata

Assignees

No one assigned

    Labels

    AIAI generated or potentially AI generated

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions