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.
| radio buttons, and the settings dialog polls them once a second; no signal runs from the sound | ||
| device to that dialog. | ||
|
|
||
| A driver may also change the buffer size on its own. `kAsioBufferSizeChange` in the ASIO backend |
There was a problem hiding this comment.
I'd turn this around to state what can be done, how it can be done and then state which drivers currently do it (ideally, "and why" as well as "and why none of the others").
| and restart it afterwards, which is the `bWasRunning` pattern throughout `client.cpp`. | ||
|
|
||
| The audio callback runs on a thread owned by the driver. `CSoundBase` inherits `QThread`, but | ||
| nothing here overrides `run()` or calls `start()`, so no such thread exists. Backends keep the |
There was a problem hiding this comment.
This is worth investigating. If the architectural framework was put in place to put this on a thread, it's probably meant to be on a thread or the code should be cleaned up.
The architecture is meant to have one of the following:
- GUI:
- Qt main thread at normal priority
- CClient with CSound (to/from the audio hardware) and CChannel (to/from the network) at raised "real time" priority
- Headless:
- CClient with CSound (to/from the audio hardware) and CChannel (to/from the network) at raised "real time" priority
There was a problem hiding this comment.
🤖 AI: On the QThread question: run() is never overridden and start() is never called anywhere in this class's tracked history, back to the commit that introduced it, and no other QThread-specific method (setPriority, msleep, currentThread) appears anywhere in the sound layer. There is no thread to put anything on. Changing the base to QObject requires nothing from this file; whether anything downstream relies on the class actually being a QThread (a qobject_cast<QThread*>, moveToThread) was not checked, so that change stays out of this PR rather than being asserted risk-free.
On ASIO doing it differently: the history inverts the guess. ASIOMutex arrives on 2008-07-12 (5eb86941) with the ASIO backend itself, when ASIO was the only backend — seven months before CSoundBase exists at all (3fb2d9ca, 2009-02-22). Its drain-on-stop wait was added on 2011-12-27 (73f408e4, "make sure that sound working thread is actually killed on stopping the audio interface"). The shared MutexAudioProcessCallback appears nine years after that, on 2020-08-26 (ecff80fc), as a fix for a JACK crash on quick reconfiguration; that commit touches linux/sound.cpp and soundbase.{h,cpp} and nothing else. So ASIO did not skip an existing architecture — the shared mutex is a later, independent re-implementation of a guard ASIO already had, scoped to the JACK bug that prompted it and never extended outward.
On whether the same mutex is worth using: not as a substitution, because the two are not doing the same job today. ASIOMutex is also held across the whole of CSound::Init() — ASIODisposeBuffers(), ASIOCreateBuffers() and the vecsMultChanAudioSndCrd reallocation — and no other backend locks anything in Init(). The waits differ too: CSoundBase::Stop() blocks unconditionally on a QMutexLocker, while ASIO's uses tryLock ( 5000 ) and returns anyway on timeout. Since ASIO's stop can return with a callback still in flight, that Init() lock is what keeps ASIOCreateBuffers() off a live bufferSwitch(). Narrowing exactly that scope is what #3869 proposes, so the merge question sits better with it than here: unifying the two would have to preserve the coverage across Init() and settle which of the two wait policies applies.
Two unrelated items surfaced in the same pass, both out of scope for a docs PR: CSound::onAudioInput() in the Oboe backend memcpys using the driver-delivered frame count into a buffer sized for the requested count, and the mismatch check runs after the copy rather than before — a device delivering more frames than requested overflows the buffer before anything notices. And the callback-guarding column in the revised table takes AudioOutputUnitStop() and ASIOStop() to synchronously stop the driver calling back again; that is untested on hardware.
| The audio callback runs on a thread owned by the driver. `CSoundBase` inherits `QThread`, but | ||
| nothing here overrides `run()` or calls `start()`, so no such thread exists. Backends keep the | ||
| callback away from a device that is being re-initialised in two ways, and the ASIO backend is the | ||
| exception to both: |
There was a problem hiding this comment.
I make that three ways, then. It should be written plainly.
There was a problem hiding this comment.
🤖 AI: Now the three are named ahead of the tables that detail them: the driver-level stop call in a backend's own Stop(), the bRun flag the callback may test, and the mutex Stop() shares with the callback. No two of the five backends combine them the same way.
Checking that turned up a false sentence in the pushed text — "Every backend but ASIO also overrides Stop()", sitting above a table row reading "no override". ASIO's override does exist, and is one of the four that calls a driver-level stop first; JACK's is the only one going straight to the base class. Both corrected, along with the iOS row of the callback table: the driver-invoked callback is recordingCallback(), and the mutex is taken inside processBufferList() only, so the output copy back into ioData runs unlocked.
| callback away from a device that is being re-initialised in two ways, and the ASIO backend is the | ||
| exception to both: | ||
|
|
||
| | backend | audio callback | ignores the callback while stopped | takes `MutexAudioProcessCallback` | |
There was a problem hiding this comment.
It might be worth seeing why this variation exists, anyway.
… 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>
| `CSoundBase::Stop()` clears `bRun` and then takes `MutexAudioProcessCallback` to wait for a | ||
| callback that is already in flight. The ASIO backend never takes that mutex, so on Windows that | ||
| wait returns immediately and `CSound::Stop()` waits on `ASIOMutex` instead. | ||
| callback that is already in flight. The ASIO backend is the exception: it defines and owns its |
There was a problem hiding this comment.
OK, I guess this leaves open the question: is there a good reason for having a different way of doing it for ASIO? (My guess: no, it was just the second backend and done completely separately without forward thinking; only when the third backend came along did the idea of an architecture appear and it's not particularly apparent even now...) And the next question: is it worth using the same mutex?
… 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>
| `AVAudioSessionRouteChangeNotification` on iOS) and never register a buffer-size-specific | ||
| listener, so a size change on its own is invisible to them. Oboe does detect it — | ||
| `onAudioInput()` compares the delivered frame count against the requested size on every callback | ||
| — but the mismatch is only logged (`qDebug()`), never renegotiated. |
There was a problem hiding this comment.
Ah the joy of code reviews... I guess we'll need a few backend-specific issues to clean up those inconsistencies.
There was a problem hiding this comment.
🤖 AI: The candidates this pass surfaced, each self-contained:
- A buffer-size change on the device's own initiative goes unnoticed on three of the five. Oboe compares and only logs it; neither CoreAudio backend registers a size listener at all.
- The Oboe input copy uses the driver-delivered frame count into a buffer sized for the requested count, and the mismatch check runs after the copy rather than before it.
- On iOS the mutex covers the input half only, so the output copy back into
ioDatais unlocked. - The running-flag check sits after the lock in JACK and CoreAudio (macOS), before it in Oboe, and is absent in ASIO and CoreAudio (iOS) — so two backends skip the processing at full mutex cost, and two never check.
- The two mutexes:
ASIOMutexis also held across the whole ofCSound::Init(), which the shared one never is anywhere, so collapsing them is a behaviour change rather than a rename.
| neither appears anywhere in this file's tracked history, and no other `QThread`-specific method | ||
| is used in the sound layer either. The inheritance predates the current driver-callback | ||
| architecture and contributes nothing; changing it to `QObject` looks safe from this file alone, | ||
| but that wasn't verified further here. |
There was a problem hiding this comment.
Probably best recorded in an issue rather than this file (with "but see issue #xxx" if you like).
There was a problem hiding this comment.
🤖 AI: Trimmed to the fact — no such thread is ever created — with the history and the QObject suggestion out of the file, for an issue instead.
On the architecture point: the raised-priority half of it does exist, on the network side. CHighPrioSocket::Start() runs its CSocketThread at QThread::TimeCriticalPriority. Sound has no equivalent: CSoundBase : public QThread is never started, and src/sound/ holds no start() call, no run() override and no other QThread method at all — so the audio callback arrives on a thread the driver owns, at whatever priority that driver gives it, a value Jamulus neither sets nor reads.
…() 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>
4 participants
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.