src/README.md: map the source folder, its threads and its locks - #3875
src/README.md: map the source folder, its threads and its locks#3875mcfnord wants to merge 9 commits into
Conversation
ann0see
left a comment
ann0see
left a comment
There was a problem hiding this comment.
I'd aim this at human contributors for now.
AGPL. But I think it's weird as is. I'd think that a HTML comment for the full license would be enough and a visible short link to the license file. But @pljones should judge. I really dislike a wall of text first just stating the license. Nobody reads it. |
| - [protocol.cpp](protocol.cpp) — `CProtocol`: protocol message framing, acknowledgement, and | ||
| retransmission of unacknowledged messages from `SendMessQueue`. The wire format itself is | ||
| described in [../docs/JAMULUS_PROTOCOL.md](../docs/JAMULUS_PROTOCOL.md). | ||
| - [channel.cpp](channel.cpp) — `CChannel`: one connection, holding the receive jitter buffer |
There was a problem hiding this comment.
I think you should probably not describe too much of the file content. Rather the brief this file holds class X which does Y. As in anyone can just read the file to get more info. So here: Implements the channel + jitter buffer used for Server and Client.
| | recorder thread | server with recording | `CJamController` | `CJamRecorder`, fed by queued `AudioFrame` signals from the frame cycle | | ||
| | `QThreadPool` global pool | client GUI | the connect dialog | one task per listed server for the ping/info fan-out (`QtConcurrent::run`) | | ||
|
|
||
| Three consequences that are easy to miss: |
I believe if we write for a human audience, we will dramatically improve LLM outcomes "for free". |
Applies @ann0see's review on jamulussoftware#3875: - Intro cut to two sentences; the paragraph about what the file does and does not assert is gone. - File list back to one line each: the SendMessQueue detail, the SockBuf and CProtocol members and the vecChannels name are all readable in the file itself. Kept "the client has one; the server an array of MAX_NUM_CHANNELS", which is in server.h, not channel.cpp. - The three-bullet block after the thread table is one paragraph. The CSoundBase QThread note moves to src/sound/README.md (jamulussoftware#3873), where a reader meets the class; the send/receive clocking bullet is dropped, as the table above already carries it. The parenthetical about how the thread identities were checked is dropped too: it describes the method, not the code, and the util.cpp TODO makes the point on its own. 122 lines to 105. No claim changed.
CSoundBase derives from QThread, so a reader can reasonably expect a sound thread. There is none: no override of run() and no call to start() exists in the sound layer -- the only two run() overrides in src/ are CHighPrecisionTimer (util.h) and CSocketThread (socket.h). Audio callbacks always arrive on driver-owned threads. Moved here from the src/README.md draft (jamulussoftware#3875), where it sat under the thread table; this is the file that introduces CSoundBase.
| locks — `MutexAudioProcessCallback`, `MutexDevProperties`, and the per-backend ones — are | ||
| covered in [sound/README.md](sound/README.md). | ||
|
|
||
| ## Not yet documented |
There was a problem hiding this comment.
I guess we could drop this section.
| * mcfnord | ||
| * The Jamulus Development Team | ||
|
|
||
| As of Jamulus 3.12.1dev (commit eb172d47): All new source code contributions must be licensed |
There was a problem hiding this comment.
Doesn't need these lines. This only applied to existing files. New files should only have the AGPL header.
| - [util.h](util.h) / [util.cpp](util.cpp) — `CHighPrecisionTimer`, the server's frame clock, and | ||
| assorted helpers. | ||
|
|
||
| Client only: [client.cpp](client.cpp) (`CClient`), the sound layer in [sound/](sound/), the GUI |
There was a problem hiding this comment.
Section, laid out as bullets like the shared code. Same for the following ones.
| | `CChannel::Mutex` | per-channel state: the enable flag, gain and pan tables, name | setters in protocol slots on the main thread; getters in the server's frame cycle | | ||
| | `CChannel::MutexConvBuf` | the send-side conversion buffer | `PrepAndSendPacket()` on the sending thread; re-init from the main thread | | ||
|
|
||
| Smaller ones: `CProtocol::Mutex` (the queue of sent but not yet acknowledged messages), |
There was a problem hiding this comment.
Again, bulleted list. Or table.
|
|
||
| - `CProtocol::Mutex` — queue of sent but not yet acknowledged messages | ||
| - `CServer::MutexChanOrder` — channel allocation in `FindChannel` and `FreeChannel` | ||
| - `CServer::MutexWelcomeMessage` |
There was a problem hiding this comment.
Does this mean it's unused?
There was a problem hiding this comment.
🤖 AI: Not unused — it is taken in OnNewConnection and SetWelcomeMessage. The odd part is the coverage: the other three readers of strWelcomeMessage — OnCLReqServerFeatures, OnCLReqWelcomeMessage and GetWelcomeMessage() — do not take it.
Nothing is contended either way: all five accessors are reached on the main thread. The two CL slots hang off ConnLessProtocol, whose signals fire on the main thread behind the queued OnProtocolCLMessageReceived, and GetWelcomeMessage() is called from the server GUI, the JSON-RPC handler and settings. The bullet now says that rather than standing bare.
| ## Not yet documented | ||
|
|
||
| - the jitter buffer's automatic size algorithm (`CNetBufWithStats`) | ||
| - the connection lifecycle: how a channel goes from first packet to connected to timed out |
There was a problem hiding this comment.
Maybe a separate Channel.md as it's pretty much "here's what Jamulus really is".
… condition Two rows of the Threads table were wrong, both found by binding each quantified sentence to a command that would make it false. The CSocketThread row said "Protocol frames are not parsed here". CProtocol::ParseMessageFrame is called on exactly that thread (socket.cpp:643, its only call site) and does the tag, length and CRC validation plus extraction of the body, ID and counter. What crosses to the main thread is the message body, via the queued ProtocolMessageReceived signal, where ParseMessageBody runs it. The protocol API splits frame from body by name and the row asserted the inverse; the Qt main thread row inherited the same error and now says "message body". The CThreadPool row said "server with --multithreading". CServer's constructor also requires more than one core: on idealThreadCount() == 1 it logs "found only one core, disabling multithreading", clears the flag and creates no pool. A single-core server is a normal deployment, so the row described threads that do not exist there. Added to the frame-cycle paragraph as well. Also collapses the blank lines left in the licence block by an earlier suggestion apply. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pljones on jamulussoftware#3875: "Doesn't need these lines. This only applied to existing files. New files should only have the AGPL header." -- the "As of Jamulus 3.12.1dev" transition note is dropped. A file created after 3.12.1dev has no pre-3.12.1dev history for it to describe. tools/update-copyright-notices.sh carries that sentence only in its own header comment and does not scan .md files for it, so nothing depends on it being here. "Section, laid out as bullets like the shared code. Same for the following ones." -- Client only and Server only are bulleted like the shared list, one line per file with what it holds. "Does this mean it's unused?" on CServer::MutexWelcomeMessage -- no, it is taken in OnNewConnection and SetWelcomeMessage. But it is not taken by the other three readers of strWelcomeMessage (OnCLReqServerFeatures, OnCLReqWelcomeMessage, GetWelcomeMessage), and all five accessors are reached on the main thread: the two CL slots hang off ConnLessProtocol, whose signals are emitted on the main thread behind the queued OnProtocolCLMessageReceived, and GetWelcomeMessage is called from serverdlg, serverrpc and settings. The bullet now says so rather than standing bare. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
🤖 AI: Pushed. Two of these are corrections to the Threads table, found by binding each quantified sentence to a command that would make it false rather than one that confirms it. The The The rest is review follow-up: the licence transition note is dropped from this new file, Client only and Server only are bulleted like the shared list, and the blank lines an earlier suggestion-apply left in the header block are collapsed. |
4 participants
MY LLM WROTE:
Short description of changes
src/has no README. This adds one: a map for a reader new to the code — which class lives where, which threads exist at runtime, and which lock protects what. Same shape as the section recently proposed forsrc/sound/README.mdin #3873: it describes how the code behaves today, asserts no intent, and ends with an explicit list of what is still missing.What it covers:
CSocketThread, the audio driver threads,CHighPrecisionTimer's thread, theCThreadPoolworkers, the recorder thread, and the connect dialog's use of the global pool. Followed by the three facts easiest to miss: the server's complete frame cycle (CServer::OnTimer) executes on the main thread — theTimeCriticalPrioritytimer thread only paces a queuedemit timeout()(confirmed with a debugger on Linux, and the TODO inutil.cppsays the same);CSoundBaseinheritsQThreadbut that thread is never started; the client sends audio from inside the driver callback while receiving onCSocketThread.MutexSocketBuf,CSocket::Mutex,CServer::Mutexincluding exactly which part ofOnTimerholds it,CChannel::Mutex,MutexConvBuf), each with what it protects and the threads that take it, plus one line each for the smaller ones.The thread table is measured, not read: each identity was confirmed by breaking on the function in a release build under gdb on Linux and recording which thread hit (
CServer::OnTimerandCProtocol::ParseMessageBodyon the main thread,CServer::PutAudioDataandCChannel::PutAudioDataonCSocketThread,CClient::ProcessAudioDataInternon the JACK callback thread,CHighPrecisionTimer::runon its own thread).Two design choices, offered for discussion since later doc files could follow the pattern:
[channel.cpp](channel.cpp),[../docs/JAMULUS_PROTOCOL.md](../docs/JAMULUS_PROTOCOL.md). Rendered on GitHub, the map becomes browseable: every named file is one click away. Read raw in a terminal or an editor, each link degrades to the path plus punctuation, so the text still works for a reader without a browser.grepfinds them from either the rendered or the raw form.CHANGELOG: SKIP
Context: Fixes an issue?
No issue. Follows #3873, which starts the sound-layer half of the same documentation; this file links to it rather than repeating it.
Does this change need documentation? What needs to be documented and how?
This is the documentation. Developer-facing, so it belongs next to the code rather than on the website.
Status of this Pull Request
Working implementation. Every statement is checkable against the tree at the commit it was written on, and the thread identities were verified at runtime rather than inferred from the source.
What is missing until this pull request can be merged?
Review. One open question for reviewers: whether the license header on a brand-new documentation file should carry the full historical GPL paragraph or only the AGPL block used here.
Checklist
No checks run on this one:
autobuild.ymlcarriespaths-ignore: '**README.md'andcoding-style-check.ymlonly triggers on**.cpp/**.h, so the fourth box stays unticked rather than claiming a green run that never happened.