src/sound/README.md: start documenting the sound design - #3873
Conversation
Replaces the "Fixme: The sound design is not yet documented" placeholder with the parts that are load bearing for anyone touching a backend: how Init()'s return value negotiates the buffer size, how many times it is called and by whom, how a driver-initiated buffer size change re-enters the client, and how each backend keeps its audio callback off a device that is being re-initialised. The callback table is the part worth having written down. ASIO is the only backend that neither ignores its callback while stopped nor takes MutexAudioProcessCallback, so CSoundBase::Stop()'s wait for a callback in flight does nothing there and asio/CSound::Stop() waits on ASIOMutex instead. This is a start, not the whole design, so the blanket Fixme is replaced by a list of the areas still missing rather than dropped: device enumeration and SetDev() failure handling, channel selection and mixing, MIDI, latency reporting and the sound card conversion buffer.
|
I assume that's mostly human written or not? |
No. The LLM wrote that. What makes you think a human did? The lack of "MY LLM WROTE:" prefix? Or something else? |
|
No. This is fine. It sounds a bit more natural than the other LLM responses though. |
Interesting feedback. Very frequently I ask my LLM to speak in comments "with extreme concision" partly because I am sensitive to the "wall of text" criticism. So I'm often tilting it toward saying the bare minimum, as a way to avoid overwhelming the humans. Those humans who want to know will parse the dense prose. Perhaps extreme concision isn't helpful. For this in-repo content, people kind of are hoping for a "wall of text" that guides them through a basic understanding. And I bet this kind of content will super-charge new AIs that join us. My observation here is that I kind of wish the content routinely included hyperlinks, but I'm not sure that would work out. Maybe with relative paths? |
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.
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.
… code, not Jamulus core The text now explicitly states that ASIOMutex is defined and owned by the ASIO backend (in asio/sound.h), addressing the review feedback to make clear whether the mutex is owned by ASIO code or Jamulus code. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
… callback guarding Addresses the remaining review comments. Buffer-size negotiation now states what can be done and by whom: only ASIO (kAsioBufferSizeChange) and JACK have a native change-notification callback. The two CoreAudio backends watch device-identity and route events only, so a size change on its own is invisible to them; Oboe detects the mismatch but logs it rather than renegotiating. Callback guarding is split into two tables: what each backend's own Stop() calls before touching bRun, and where each callback reads the running flag relative to the mutex. IsRunning(), bRun and !bRun are three spellings of one check, but only Oboe's sits before any lock -- JACK's and CoreAudio (macOS)'s run after it, so they skip the processing without saving the mutex contention. CoreAudio (iOS) and ASIO have no flag check at all and depend entirely on their driver-level stop call. Also corrects the subdirectory count in the intro: src/sound/midi-win/ holds CMidi rather than a CSound backend, and the default Windows build compiles it alongside asio/. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The file said what ASIO does differently but not why, so the difference read as an unexplained inconsistency. It is chronological: ASIOMutex was added with the ASIO backend itself in 5eb8694 (2008-07-12), when ASIO was the only backend and CSoundBase did not yet exist (3fb2d9c, 2009-02-22); its drain-on-stop wait followed in 73f408e (2011-12-27). The shared MutexAudioProcessCallback arrived in ecff80f (2020-08-26) to fix a crash on quick JACK reconfiguration, touching linux/sound.cpp and soundbase.{h,cpp} only, and was never extended to ASIO. The two are also not interchangeable today: ASIOMutex is held across the whole of CSound::Init(), which no other backend does, and ASIO's Stop() uses tryLock ( 5000 ) where CSoundBase::Stop() blocks unconditionally. Since ASIO's stop can return with a callback still in flight, the Init() lock is what keeps ASIOCreateBuffers() off a live bufferSwitch(). The intro said the file describes how the code behaves today, while the QThread paragraph already explained how that inheritance arose; it now says origins are given where they are needed to read the behaviour correctly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…() table pljones on jamulussoftware#3873: keep a consistent order between the backend-based tables. The Stop() table orders ASIO, CoreAudio (macOS), CoreAudio (iOS), Oboe, JACK; the callback/mutex table below it didn't match. Reordered rows only, no content changed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…echanisms Three review points from pljones on jamulussoftware#3873, plus a defect in our own pushed text found while checking them. The correctness fix: "Every backend but ASIO also overrides Stop()" was false, and the ASIO table row said "no override". All five backends override Stop() (asio/sound.cpp:524, coreaudio-mac 725, jack 197, oboe 193, coreaudio-ios/sound.mm), and ASIO's is one of the four that calls a driver-level stop first: ASIOStop(), then CSoundBase::Stop(), then a tryLock on ASIOMutex. JACK's is the only override that goes straight to the base class. "I make that three ways, then. It should be written plainly." -- the three mechanisms are now named up front (driver-level stop call, bRun flag check, shared mutex) before the tables that detail them. No two backends combine the three the same way. "Probably best recorded in an issue rather than this file." -- the QThread paragraph loses its history and its QObject recommendation, keeping only the fact that no thread is ever created. Also corrects the iOS row of the callback table: the driver-invoked callback is recordingCallback(), not processBufferList(), and the mutex is taken inside the latter only -- the output copy back into ioData runs unlocked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MY LLM WROTE:
Short description of changes
src/sound/README.mdends with "Fixme: The sound design is not yet documented." This makes a start on that section.It describes how the code behaves today, not why it was designed that way — intent is not something a reader of the source can verify, so none is asserted. It is also not the whole design, so the blanket Fixme is replaced by a list of the areas still missing rather than dropped.
What the new section covers:
Init()returns the size the device actually accepted, which is howCClient::Init()discovers which sizes are supported, and why it callsInit()four times per invocation. Also that the threebFraSiFact*Supportedflags drive the buffer delay radio buttons, and that the settings dialog polls them once a second rather than being signalled.kAsioBufferSizeChangeand JACK's buffer size callback both arrive throughEmitReinitRequestSignal ( RS_ONLY_RESTART_AND_INIT ).Init()is only ever entered with the device stopped, and a table of which backend ignores its audio callback while stopped and which takesMutexAudioProcessCallback. ASIO is the exception to both, which is whyCSoundBase::Stop()'s wait for a callback in flight does nothing on Windows andasio/CSound::Stop()waits onASIOMutexinstead.SetDev()failure handling, channel selection and mixing, MIDI,GetInOutLatencyMs(), and the sound card conversion buffer.CHANGELOG: SKIP
Context: Fixes an issue?
No issue. The material comes out of #3869, where the buffer size display question (issuecomment-5227331894) turned into the lifecycle and callback rules written down here.
Does this change need documentation? What needs to be documented and how?
This is the documentation. Nothing is needed on the website: it is developer-facing detail about one source folder, which is what
src/sound/README.mdalready exists to hold.Status of this Pull Request
Working implementation. Every statement in it is checkable against the tree at the commit it was written on, and the table was read off all five backends rather than assumed.
What is missing until this pull request can be merged?
Review, and a decision on scope: whether a partial section plus an explicit "not yet documented" list is the right shape for that Fixme, or whether it should stay a placeholder until the whole design is covered.
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.