Comment accuracy: nine measured corrections (draft / experiment) by mcfnord · Pull Request #3866 · jamulussoftware/jamulus

mcfnord · 2026-08-08T03:47:42Z

MY LLM WROTE:

This draft PR is an experiment in format. Sixty-two "this must always hold"-style comments in the core files (buffer, channel, protocol, socket, server, client) are being tested one by one against the running code; nine came back misleading or outdated. Each commit here rewrites one comment to say what a fully accurate comment would say, and the commit message carries the measurement that justifies it - numbers, method, and what survived.

The diff is comments only. Every code line is byte-identical to main (two lines lost a trailing comment; the statements are unchanged). No behaviour is touched by this branch.

The nine split into two kinds:

Six where the comment was wrong about correct code - the code is fine, the comment is now precise.
Three where the comment was accurate about defective code. These are not softened: the comment is rewritten to describe the real, current behaviour, defect included, and marked FIXME. Fixing the code is separate work for another PR; the point here is that the comment should not keep asserting a guarantee the code does not provide.

What this deliberately is not:

It is not a request to merge as-is. It is meant to stay in draft as a reviewable ledger. Any subset can be cherry-picked, reworded, or rejected commit-by-commit; each stands alone.
It is not the whole audit. Most tested comments came back accurate, which is a result in itself; a further set of rows is still unrun and any that fail will be added as further commits.

The six comment-was-wrong corrections:

file	old claim	measured
buffer.cpp	seq-number byte "much smaller" than audio keeps the block-count divide exact	the real bound is factor < blocksize, enforced by the props validator in protocol.cpp; the divide fails exactly at factor == blocksize (AddressSanitizer locates the overflow)
buffer.cpp	wraparound undetectable past 256 counts, ">100 ms"	horizon is 128 counts (signed fold), which is 171-341 ms at real frame rates
buffer.cpp	window moves are how valid packets get thrown away	a second loss channel (slot overwrite) exists, is the only one at buffer length 1, and the window move is load-bearing: without it dropouts go 0.012% to 6.704%
client.cpp	non-zero iActiveChannels => "server must have been restarted"	a >30 s traffic gap produces the same state from a never-restarted server (receive-timeout path)
server.cpp	flag "must be a member since QtConcurrent::run can only take 5 params"	the path uses the variadic CThreadPool::enqueue; the Qt5 limit is real (measured: 5 compiles, 6 fails) but no longer applies
server.cpp	"each thread can only set it to true and never to false"	OnTimer clears it to false every tick; ordering (clear before enqueue, read after the futures join), not one-way writes, is what makes it safe

The three comment-accurate-but-code-defective (FIXME, code untouched):

file	old comment	real behaviour now recorded
channel.cpp	77-byte per-packet overhead (PPPoE + ATM DSL model)	measured 46 B on IPv4/Ethernet and 66 on IPv6; the returned upstream rate overstates real cost by ~15-50%, worst at the lowest bit-rate settings
server.cpp	"no memory must be allocated" in the realtime audio path	the send path still allocates once per packet (const-cast deep copy in CSocket::SendPacket): 73,308 allocations under OnTimer in 96 s with 3 clients
client.cpp	FindClientChannel here "should always return channel 0"	returns INVALID_INDEX (-1) for a server-sent channel id >= 150, unchecked; a UBSan build confirms a -1 dereference reached from an untrusted server

Reproduce commands and full harness output exist for every row and can be posted as a gist if wanted.

The comment attributes the exactness of the block-count divide ( iInSize / iBlockSize ) to the sequence-number byte being "much smaller" than the coded audio. That ratio holds (worst reachable case 1/12), but it is not the condition. Measured with a harness linking the unmodified buffer.cpp: the divide is exact across the whole protocol-reachable domain (59,973 blocksize x factor pairs, blocksize 9..19999, factor {1,2,4}, zero miscounts) and first fails exactly at factor == blocksize, where AddressSanitizer reports a heap-buffer-overflow READ in the window-move invalidate path and one block of bytes from behind the packet reaches the jitter buffer. What keeps that corner unreachable is the properties validator in protocol.cpp (base network packet size >= 10, hence blocksize >= 9; factor restricted to {1, 2, 4}), two files away. The rewritten comment names the bound and the cross-file coupling that enforces it. Audit row 1; comments only, no code change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Two numbers in this comment are off, both in the safe direction. The detection horizon is 128 counts, not 256: iSeqNumDiff is folded into the signed range [-128, 127] a few lines above, so a packet more than 128 counts late is mistaken for an early one. And ">100 ms" understates the horizon: the sequence advance rate was measured on 8 real client/server configurations with a CLOCK_MONOTONIC probe at 375.0 counts/s for the 128-sample default and 750.0 counts/s for 64-sample OPUS64 frames (48000/frame to four significant figures; 64 samples is the frame floor, so 750/s is the fastest possible advance). 128 counts is therefore 171 ms at the fastest frame rate and 341 ms at the default. The conclusion -- such a packet indicates a bad network situation and is useless anyway -- survives with the corrected numbers. Audit row 2; comments only, no code change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

The tradeoff the comment admits is real and is now measured: across a 3.2M-call Get() sweep, zero late packets were discarded (the "never throw away" half holds) and 115,448 valid blocks were invalidated by window moves, every one of them never played. But the window move is not the only way a valid block dies: 157,758 blocks were overwritten in their slot before playout in the same corpus. At a buffer length of 1 the overwrite channel is the only one (a window move cannot invalidate anything there); from length 3 upward the window-move channel dominates. An ablation arm that refuses late packets shows the admitted downside is the price of the buffer working at all: without the window move, occupancy pins at 1.366 blocks at every size and dropouts rise from 0.012% to 6.704% (sigma = 1 frame of jitter, buffer length >= 4) -- the late packet is the only thing that rewinds the playout clock, so the window move is the sole mechanism by which the buffer acquires depth. The added text documents the second loss channel. Audit row 4; comments only, no code change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Measured against one server process that never restarted (same pid verified alive after the run): a connected client that goes silent for 40 s -- longer than CON_TIME_OUT_SEC_MAX = 30 s -- is dropped by the receive-timeout path in CChannel::GetData(), and its next packet is treated as a new connection, producing a second CLIENT_ID with iActiveChannels != 0. The log of the single server process reads connected, then idling, then connected again. So "the server must have been restarted on the fly" names a cause that is not the only reachable one: a traffic gap longer than the receive timeout is enough. The remedy ( ClearClientChannels ) is correct in both cases; only the stated cause was wrong. Audit row 26; comments only, no code change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

This path no longer uses QtConcurrent::run at all: the decode workers are dispatched through CThreadPool::enqueue ( threadpool.h ), which is variadic. QtConcurrent survives only in connectdlg.cpp. The cited Qt5 limit is real and is exactly 5 -- measured against Qt 5.15.13: a free function compiles with 5 arguments and fails with 6; a member call compiles with 4 arguments after the object and fails with 5 -- but the call site passes 4 arguments after the callable, so the limit would not bind even on the old path. Premise and consequence are both obsolete; the replacement note records that nothing in the threading forces the flag to be a member. Audit row 44; comments only, no code change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

"each thread can only set it to true and never to false" is contradicted by the clear further up in the same function: OnTimer sets bChannelIsNowDisconnected = false once per tick. In the default configuration multithreading is off ( bUseMultithreading defaults to false in main.cpp, and the server also disables it itself when only one core is found ), the decode then runs inline, and the same thread writes both values. The access is safe, but by ordering, not by one-way writes: the clear is sequenced before any decode work is enqueued on the thread pool, and the flag is read back only after every future has been waited on -- the join supplies the ordering. The store itself has been std::atomic since e6eed12. The rewritten comment states that mechanism, which is also the constraint a future change must keep: moving the clear into the concurrent region would break it. Audit row 62; comments only, no code change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…not 77) The 28 + 26 + 23 = 77-byte overhead is a PPPoE-over-ATM DSL model. A packet capture on a real internet path (~90k packets) measures 20-byte IP headers and a 14-byte Ethernet header with no PPPoE, ATM, or VLAN present: 46 bytes over IPv4 and 66 over IPv6, the latter a header the constant does not account for at all. So GetUploadRateKbps overstates true IPv4/Ethernet cost by roughly 15% to 50% across the settings, worst at the lowest bit-rate where the bandwidth-constrained user it is meant to help is most affected. The code is unchanged here; the comment now records the real overhead and marks the constant FIXME, because what the figure should be is a design decision (which layer to bill) for the maintainers. Audit row 8; comment describes the current DEFECT, code unchanged (fix is separate work). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

… copy) "no memory must be allocated" in the realtime path is the intent, not the current behaviour. With a malloc/free interposer keyed by calling stack, a running server with 3 clients allocated 73,308 times under OnTimer over 96 s; 91% are CSocket::SendPacket, where taking the argument as a const CVector and casting the const away deep-copies every outgoing datagram -- one malloc, one memmove, one free per audio packet (packet count and malloc count match exactly). CNetBufWithStats::Init also allocates on the path. Attribution is by stack, not thread name: OnTimer runs on the Qt event-loop thread through a queued connection, not the TimeCritical timer thread, and matching the thread name undercounts. Code unchanged; the comment now states that the guarantee is not currently met and marks the copies FIXME. Audit row 20; comment describes the current DEFECT, code unchanged (fix is separate work). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…return 0 "should always return channel 0" is false for a server-controlled input. FindClientChannel returns INVALID_INDEX (-1) for any server channel id >= MAX_NUM_CHANNELS (150); EvaluateClientIDMes validates only the 1-byte length, never the range, so a server can send 150..255. Confirmed on the wire with a fake server and a real headless client: a UBSan build reports "index -1 out of bounds for type 'CClientChannel [150]'" in SetRemoteChanGain (reached via this line when the mute-me flag is set) and again in the gain/pan timer, and the out-of-bounds read is forwarded back onto the network as a CHANNEL_GAIN message. AddressSanitizer stays silent because index -1 lands on a preceding member inside the same allocation, with no redzone -- so a clean ASan run here is not exoneration. Code unchanged; the comment now states the hazard and marks the missing range check FIXME. Audit row 27; comment describes the current DEFECT, code unchanged (fix is separate work). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

pljones · 2026-08-08T09:30:46Z

+        // below CELT_MINIMUM_NUM_BYTES (10) and a block size factor outside
+        // { FRAME_SIZE_FACTOR_PREFERRED, _DEFAULT, _SAFE }. With iNumBytesSeqNum == 1 and
+        // iBlockSize == iBaseNetworkPacketSize - 1 (channel.cpp), the worst reachable case is
+        // a factor of 4 against a block size of 9.


The above is far too verbose for anyone to read it clearly. Two lines looks like a good length. Ten lines isn't.

It needs to be short and to the point:

Intent:
Purpose of this section, including what the result is used for.

Inputs:

iInSize: where this comes from and what it means

iBlockSize: ...

...

Method:
Explanation of why this method works. But only if it's not transparently obvious.

pljones · 2026-08-08T09:32:18Z

+            // late, not 256. At the fastest possible frame rate that is still 171 ms
+            // (64-sample frames, 750 counts/s) and at the default frame size 341 ms
+            // (128-sample frames, 375 counts/s), so such a packet is long useless either
+            // way and we have a bad network situation anyway. Therefore we


This doesn't add anything at all.

pljones · 2026-08-08T09:33:46Z

            // too late so we throw away valid packets when we move the "buffer window" to the delayed
-            // packet and then back to the correct place when the next normal packet is received. But
+            // packet and then back to the correct place when the next normal packet is received.
+            // Note that this is not the only way a valid block is lost: a block can also be


This addition is not in context and doesn't belong here.

pljones · 2026-08-08T09:35:06Z

+    // running. A restart is only one way that happens: the server also drops a channel whose
+    // receive timeout expires ( CON_TIME_OUT_SEC_MAX, channel.h ) and treats the next packet
+    // from the same peer as a new connection, so a traffic gap of longer than that is enough.
+    // Either way the channel list we hold may be stale, so clear it and get it afresh.


Too much waffle. The original got the point across succinctly.

pljones · 2026-08-08T09:36:00Z

    int  iNumBlocks           = 0;     // init number of blocks for multithreading
    int  iMTBlockSize         = 0;     // init block size for multithreading
-    bChannelIsNowDisconnected = false; // note that the flag must be a member function since QtConcurrent::run can only take 5 params
+    // The flag is a member variable, not a local. Nothing in the threading forces that: the


Too much waffle again. Say what is and why. Not what is not and why not.

pljones · 2026-08-08T09:37:39Z

-                // std::atomic write (not a read-modify-write operation) and also each
-                // thread can only set it to true and never to false
+                // Note that no mutex is needed for this shared resource: the store is a
+                // std::atomic write, not a read-modify-write operation. It is NOT true that


Again, saying what is not, is not helpful, just token-spend.

pljones · 2026-08-08T09:38:43Z

    // 8 (UDP) + 20 (IP without optional fields) = 28 bytes
    // 2 (PPP) + 6 (PPPoE) + 18 (MAC)            = 26 bytes
    // 5 (RFC1483B) + 8 (AAL) + 10 (ATM)         = 23 bytes
+    // A packet capture on a real internet path measures 46 bytes over IPv4/Ethernet


Experimental data should not inform code comments.

pljones · 2026-08-08T09:39:32Z

-    // the worst case here:
+    // To avoid audio glitches, the realtime timer audio routine ( OnTimer ->
+    // ProcessData ) should allocate no memory. The worst-case vectors below are
+    // pre-sized here for that reason. Note that the goal is not currently met: the


Again, do not say what is not, only what is.

If there is something that needs fixing, raise an issue or PR.

pljones · 2026-08-08T09:40:08Z


    // allocate and map client-side channel 0
-    int iChanID = FindClientChannel ( iServerChanID, true ); // should always return channel 0
+    // In normal operation this returns channel 0. It is NOT guaranteed: FindClientChannel


More negatives.

mcfnord · 2026-08-08T22:12:41Z

🤖 AI

Superseded by #3877: the same corrections, redone per the review feedback here as the smallest wording change per site, with the measurements in the commit messages instead of the comments.

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

jrd and others added 9 commits August 8, 2026 03:28

pljones reviewed Aug 8, 2026

View reviewed changes

dingodoppelt added the AI AI generated or potentially AI generated label Aug 8, 2026

mcfnord mentioned this pull request Aug 8, 2026

Comment accuracy: minimal wording fixes in src/ #3877

Draft

mcfnord closed this Aug 8, 2026

Navigation Menu

Uh oh!

Comment accuracy: nine measured corrections (draft / experiment) - #3866

Comment accuracy: nine measured corrections (draft / experiment)#3866
mcfnord wants to merge 9 commits into
jamulussoftware:mainfrom
mcfnord:comment-accuracy

mcfnord commented Aug 8, 2026

Uh oh!

pljones Aug 8, 2026

Uh oh!

pljones Aug 8, 2026

Uh oh!

pljones Aug 8, 2026

Uh oh!

pljones Aug 8, 2026

Uh oh!

pljones Aug 8, 2026

Uh oh!

pljones Aug 8, 2026

Uh oh!

pljones Aug 8, 2026

Uh oh!

pljones Aug 8, 2026

Uh oh!

pljones Aug 8, 2026

Uh oh!

mcfnord commented Aug 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

mcfnord commented Aug 8, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcfnord commented Aug 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants