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
| 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.
| 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.
|
|
||
| `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. |
There was a problem hiding this comment.
Make it clear whether ASIOMutex is owned by the ASIO code or by the Jamulus code.
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.