Comment accuracy: minimal wording fixes in src/ - #3877
Conversation
The input is iNumBlocks * ( iBlockSize + iNumBytesSeqNum ) bytes, so the divide by iBlockSize is exact only while iNumBlocks * iNumBytesSeqNum stays below iBlockSize. The sequence number being "much smaller" than the coded audio is not the condition: at iNumBlocks == iBlockSize the count comes out one too high whatever the ratio is.
The fold five lines above maps the difference into -128...127, so detection fails past half the wrap distance, not the full 256. 128 counts is 170 ms at the fastest frame rate (64 samples at 48 kHz, 750 counts/s) and 341 ms at 128-sample frames, so the "more than 100 ms" figure was low as well.
CChannel drops a channel whose receive timeout expires (CON_TIME_OUT_SEC_MAX, 30 s in channel.h) and treats the next packet from the same peer as a new connection, so a traffic gap alone produces this state from a server that never restarted. The action taken is right either way.
"should always return channel 0" contradicts the declaration in client.h, which documents the return as "a client channel ID or INVALID_INDEX": FindClientChannel returns INVALID_INDEX for iServerChannelID >= MAX_NUM_CHANNELS.
The decode workers are dispatched through CThreadPool::enqueue (threadpool.h), which is variadic; QtConcurrent does not appear in server.cpp at all. What does require a member is that DecodeReceiveData writes the flag and OnTimer reads it after the futures join.
…ateAutoSetting SYSTEM_FRAME_SIZE_SAMPLES is 64 and UpdateAutoSetting() assumes nothing about it. The frame-size assumption lives in the IIR_WEIGTH_* constants in buffer.h:80-91, which exist in a 128 set (*_DOUBLE_FRAME_SIZE) and a 64 set, selected by frame size at buffer.cpp:486 and :496. The 64 set is derived from the 128 set by exp(64/128*log(x)) (buffer.h:86); that conversion checks out for all four constants.
No init-order dependency exists. pSignalHandler ( CSignalHandler::getSingletonP() ) and pSettings ( nullptr ) are adjacent initialisers at client.cpp:60-61, neither reads the other, and nothing constructed between them reads either; pSignalHandler is first used at client.cpp:199 and pSettings only via SetSettings() at :219. Swapping the two declarations and rebuilding produced byte-identical startup output. CServer supplies the control: it declares pSignalHandler last (server.h:325) with no ordering comment.
A server GUI started with a fresh ini, no directory options and a clean quit writes <directorytype>-1</directorytype>. -1 is AT_NONE, defined at util.h:612 as "means not registered, invalid value"; settings.cpp:1162 initialises directoryType = AT_NONE and says so. The same run with the ini pre-set to a genre directory logged "Server Registration Status update: Registered"; the -1 arm logged nothing. The justification for not validating here survives without the false premise.
The width of all five visible columns is set a few lines below, including the last one (LVC_VERSION), and setStretchLastSection ( false ) is the Qt call that stops the last section absorbing remaining space. Measured on a CConnectDlg resized to 900x600: the five columns total 680 px in an 876 px viewport, so nothing absorbs the rest. The only columns without a set width are the two hidden sorting columns.
A size_t arm of this array produces identical output on every probe: INVALID_INDEX is -1 and both sides of iFaderNumber[i] == INVALID_INDEX convert to SIZE_MAX, so the sentinel round-trips. What the signed type does buy is measured: the size_t arm draws -Wsign-compare at the comparison below and the shipped arm compiles that file without warnings.
…ction flag With and without the explicit Qt::QueuedConnection the behaviour is identical: emits land on QtConcurrent worker threads and every slot invocation runs on the main thread after the emit returns. CConnectDlg::EmitCLServerListPingMes is only ever reached through QtConcurrent::run (connectdlg.cpp:863/866), so Qt::AutoConnection queues the call regardless. The requirement the comment states is real; the flag is not what satisfies it.
QThread::TimeCriticalPriority is a request the platform can refuse. Measured from inside the thread with pthread_getschedparam plus getpriority, on an ordinary unprivileged process (RLIMIT_RTPRIO soft limit 0, the desktop-Linux default): the shipped thread runs SCHED_OTHER, rt_priority 0, nice 0 - identical to NormalPriority. The IdlePriority control shows Qt does apply the argument on this platform, downward, to SCHED_IDLE. On macOS 12.7.6 the same harness reads sched_priority 47 against a main thread at 31, so the request is granted there. QThread::start returns void, so a refusal is silent. Also fixes effect -> affect in the same block.
socket ( AF_INET, SOCK_DGRAM, 0 ) returns -1 under file descriptor exhaustion (measured: RLIMIT_NOFILE=16, 13 sockets, then EMFILE), so the check is live and "should never happen" was wrong. Inside an empty network namespace - loopback only, no interfaces - the same call still succeeds, so "IPv4 not available" is not a state any tested configuration reaches. bDisableIPv4 is a hardcoded const false at socket.cpp:152, so the branch is unconditional.
…d uses iCurPingTime is written in exactly one place, OnCLPingReceived (client.cpp:440), reachable only through CClient::CreateCLPingMes(), whose only caller in the tree is CClientDlg::OnTimerPing (clientdlg.cpp:1146). Measured on the wire with a recording UDP proxy: a headless client emits zero CLM_PING frames at RTT 0 and at RTT 122 ms and keeps the 50 ms default period, while a GUI client built from identical sources spaces gain messages at 49, 122 and 244 ms - twice the ping. The headless consequence is issue jamulussoftware#3874.
The three terms - 28 (UDP+IP), 26 (PPP+PPPoE+MAC), 23 (RFC1483B+AAL+ATM) - are the PPPoE-over-ATM DSL access path of the cited paper, not a property of "the UDP packet which is transported via IP". A packet capture on a real internet path (~90k packets) measures 46 bytes over IPv4/Ethernet and 66 over IPv6, with no PPPoE, ATM or VLAN present. What the constant should be is a design question about which layer to bill; the comment now names the model it quotes and the code is unchanged.
| // else INVALID_INDEX to specify it is not in use | ||
| // so must use "int" for the array type. | ||
| // so the array type is "int", which also keeps the comparisons below | ||
| // free of signedness warnings. |
There was a problem hiding this comment.
Seems a pretty strange way of putting it.
else INVALID_INDEX to specify it is not in use.
As INVALID_INDEX is of type int, an int type must also be
used here to avoid signedness warnings elsewhere.
| // further than this we cannot detect it. But it does not matter since such a packet is | ||
| // more than 100 ms delayed so we have a bad network situation anyway. Therefore we | ||
| // further than half of this we cannot detect it. But it does not matter since such a packet is | ||
| // more than 170 ms delayed so we have a bad network situation anyway. Therefore we |
There was a problem hiding this comment.
Actually, I find the whole comment (not this changed bit) very hard to digest with lots of repeated phrases.
we always move our "buffer window"
since we adjust the window
we move the "buffer window"
| // band networks; Carot, Kraemer, Schuller; 2006") | ||
| // we assume the PPPoE-over-ATM DSL access path described in ("Network Music | ||
| // Performance (NMP) in narrow band networks; Carot, Kraemer, Schuller; 2006"), | ||
| // whose additional header size is |
There was a problem hiding this comment.
This comment now omits the first line (UDP) from the description. It's probably also worth stating something like "PPPoE-over-ATM DSL chosen as a general domestic use case" (or whatever reason the authors gave in their paper).
| // When the first gain or pan change message is requested after an idle period (i.e. the timer is not | ||
| // running), it will be sent immediately, and a timer started. The timer period is dependent on | ||
| // the current ping time to the remote server. | ||
| // the current ping time to the remote server, which only a GUI client measures (see #3874). |
There was a problem hiding this comment.
Not worth mentioning. Any "bad design" should be captured in an issue and addressed that way, rather than in code comments.
There was a problem hiding this comment.
Overall AI hasn't done great on this "update the comments" exercise, but I think this change is valuable. Since the day robots took my writing career, I miss this eternal tension between developers and documentarians. The perfect should not be the enemy of the good. Futureware doesn't help anyone today. Should this matter become fixed, one could revel and delight in the removal of this mention of the fixed flaw. I've linked arms with developers and spun with delight as we threw out obsolete guidance. This worked well so long as I was the benevolent dictator of the documentation. Once the world moved to repos and toward engineers applying their engineering acumen to editorial judgements, as you do here, quality has suffered. Perfected futureware is the psychological bias of the lead engineer. Why mention a thing we should fix? To avoid suffering, that's why. My career involved using a magnifying lense on things developers want to disappear, and sometimes even hide. So this is nothing new. But please consider my differing point of view, oh benevolent dictator.
| // note that this connection must be a queued connection, otherwise the server list ping | ||
| // times are not accurate and the client list may not be retrieved for all servers listed | ||
| // note that this delivery must be queued, otherwise the server list ping times are not | ||
| // accurate and the client list may not be retrieved for all servers listed |
There was a problem hiding this comment.
Hm. That's not what Qt::QueuedConnection means. The explanation should indicate something about the effect within the Qt signal/slot framework of the parameter and then why it's needed within the Jamulus architecture here.
| // System block size, this is the block size on which the audio coder works. | ||
| // All other block sizes must be a multiple of this size. | ||
| // Note that the UpdateAutoSetting() function assumes a value of 128. | ||
| // Note that the IIR_WEIGTH_* filter constants in buffer.h assume values of 64 and 128. |
There was a problem hiding this comment.
Possibly worth splitting out "completely wrong" from "unclear or unhelpful". This looks like the former.
| 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 | ||
| bChannelIsNowDisconnected = false; // note that the flag is a member since DecodeReceiveData sets it and the check below reads it |
There was a problem hiding this comment.
Hm. Seems a pretty useless comment now. "This is a flag. It's read and written." The earlier comment explained why it existed at all.
| // does not effect the stability of the audio stream (e.g. if the GUI is on | ||
| // does not affect the stability of the audio stream (e.g. if the GUI is on | ||
| // high load because of a table update, the incoming network packets must still | ||
| // be put in the jitter buffer with highest priority). |
There was a problem hiding this comment.
(non-feedback)
It's really the GUI that needs its own thread. The main Jamulus Client should all run at real time scheduling / raised priority: audio, client and network. That part of the code needs to be kept "lean and mean" in terms of performance...
(I'll continue rumbling about this...)
3 participants
🤖 AI
Fifteen comments in
src/state something the code does not do, and three misspell a word. Each fix is the smallest wording change that makes the sentence true; the diff is comment-only (+25/−26, no code line changes), and each commit message carries the measurement or code reference behind its correction.buffer.cpp(Put) - the block-count divide is exact only whileiNumBlocks * iNumBytesSeqNumstays belowiBlockSize; the per-block size ratio is not the condition.buffer.cpp(sequence wrap) - the fold above limits detection to 128 counts, half the wrap distance, and 128 counts is at least 170 ms.client.cpp(OnClientIDReceived) - a 30 s traffic gap (CON_TIME_OUT_SEC_MAX) also produces non-zeroiActiveChannels, from a server that never restarted.client.cpp(same function) -FindClientChannelreturnsINVALID_INDEXfor an out-of-range ID, asclient.hdocuments; channel 0 is the in-range result.client.cpp(gain/pan rate limiter) - only a GUI client measures the ping time the timer period depends on (#3874).server.cpp(OnTimer) - the flag'sQtConcurrent::runfive-parameter rationale is stale: dispatch isCThreadPool::enqueue, which is variadic.main.cpp- a GUI server with a fresh ini writes<directorytype>-1</directorytype>, which isAT_NONE, so the "always default to registering somewhere" premise goes; the justification for not validating here stands without it.connectdlg.cpp- the width of every visible column is set, including the last, andsetStretchLastSection ( false )six lines below is what stops the last column taking the remaining space.global.h- the 128 assumption belongs to theIIR_WEIGTH_*filter constants inbuffer.h, which come in a 64 set and a 128 set, not toUpdateAutoSetting().client.h- swapping thepSignalHandler/pSettingsdeclarations and rebuilding gives byte-identical startup output, so the init-order comment goes.socket.h-TimeCriticalPriorityis requested, not guaranteed: on unprivileged Linux the thread runsSCHED_OTHER, rt_priority 0 (theIdlePrioritycontrol does take effect, downward), while macOS grants sched_priority 47 against a main thread at 31.socket.cpp- a failed IPv4socket()is reachable under descriptor exhaustion, so "should never happen" goes; "IPv4 not available" did not occur even inside an empty network namespace.clientdlg.cpp- the delivery is queued with or without the explicitQt::QueuedConnection, because the emit always comes from aQtConcurrentworker thread; the requirement belongs to the delivery, not this flag.audiomixerboard.cpp- asize_tarm of the in-use array behaves identically (theINVALID_INDEXsentinel round-trips); whatintbuys is a comparison free of signedness warnings.channel.cpp(GetUploadRateKbps) - the 77-byte overhead is the PPPoE-over-ATM DSL access path of the cited paper; the comment now names the model it quotes.clitches→glitches(server.cpp),sequred→secured(protocol.h),effect→affect(socket.h).This replaces #3866 and applies the review feedback there: comments stay short, say what is rather than what is not, and carry no experimental data - the measurements live in the commit messages.
clang-format14 reports every touched file clean. Sites where the measurement points at the code rather than the comment are excluded and will be raised separately.🤖 This message was written by AI and reviewed by @mcfnord.