AudioSpectrumNode Fixes & Improvements#245
Conversation
…hreading, and crash fixes Inspector / behaviour: - Replace the original `Gain` input with `Sensitivity` (drives the noise floor / how responsive the analyser is to quiet sounds) plus an overdrive `Gain` that multiplies normalised bar values for visual punch without touching the underlying signal. Adds a `BandsVisualizer` settings popover so authors can see what the analyser is producing inline. - Wire the `Device Name` dropdown to actually route the capture session to the selected `AVCaptureDevice`, instead of always using the system default input. - Rename the output port display name from "Number" to "Spectrum" so it reads correctly in connection menus. Threading + state: - Wrap the device list in `Mutex<DeviceList>`. The capture-session setup pulls `devices` from the embedder's consumer queue while `AVCaptureDevice` connect/disconnect notifications mutate it from main; the lock makes the cross-queue access safe. - Move `samples` access behind a `Mutex` and drop the `DispatchQueue.main.async` hop in the capture callback. The capture thread now only stages raw samples + the latest sample rate, and `execute()` is the sole site that drains them and mutates filter-bank state. Works regardless of which queue the embedder runs the graph on. Fixes: - Engine wasn't starting for newly-added nodes. Addressed by deferring filter-bank creation until the first real audio buffer arrives. - NaN poisoning of the biquad filter bank: `AVAudioEngine.inputNode. inputFormat` returns `sampleRate=0` on macOS before the engine is running, which poisoned every biquad coefficient with NaN and never recovered. Build/rebuild the filter bank from the actual stream rate observed in the tap callback, gated on `bufferSampleRate.isFinite && > 0`. Self-heal on the envelope side too: if `env[k]` becomes non-finite from a transient, reset it to 0 instead of letting NaN propagate forever.
vade
left a comment
vade
left a comment
There was a problem hiding this comment.
Thanks for tackling this! Theres a lot in here. This is a first cursory glance so take this review with a grain of salt!
My biggest question is - if we have queues, why do we need locks? Queues can act as locks by enforcing serial access to data from a specific context and removing data races, and I'm not convinced high level the locks are needed / required if we have the queues.
Maybe at audio data rate they are faster than GCD dispatch? LMK your thoughts.
Secondly, even with diff threads, can't we do lock free by using producer / consumer model or maybe a more functional design of allocate and pass the data through through the filter bank ?
Thanks!
|
On queues and locks – AVCaptureAudioDataOutput works with a queue, so typical RT concerns of no locks etc. don’t apply. So why then have a lock? In combination with the suppression of the default Copyable conformance, it’s a way to ensure thread safety at compile time. And it’s faster. We could go lock free with a circular buffer, but that’s more code complexity in a different direction, without the compile time safety. I don’t think either the extra Swift complexity or possible performance gain is worth breaking the norms of Fabric elsewhere. Given a queue is already in there, let’s just With a similar rationale but for a much simpler case, scaling back from (Though if you want to push the Fabric idiom to be more defensive with type enforced locking etc., digging into this might be interesting as a starting point. And standard disclaimer: I’m no expert here) |
|
is that an llm response? :D |
|
Via Discord, now that the fix for #61 is in Feature/Satin 2.0 support, if you could target that branch for this PR, along with the minor changes to execute / enable / disable / start / stop exection function signatures, that would be awesome! |
Match the `fabric.<NodeName>.<purpose>` pattern used by CameraProviderNode and ScreenCaptureProviderNode.
Replace Mutex<AudioBuffer> (and the ~Copyable struct) with plain
properties owned by captureQueue. The producer (AVCaptureSession
delegate callback) already runs on captureQueue, so it writes
pendingSamples and lastSeenSampleRate directly. The consumer
(execute, setupCaptureSession teardown) accesses them via
`captureQueue.sync { … }` — the "owning queue + dispatch_sync"
pattern used elsewhere in Fabric, instead of layering a second
synchronization primitive on top.
Drop the didRequestAudioSetup latch and requestAudioSetupIfNeeded() that bootstrapped capture from execute() to work around GraphRenderer not calling startExecution on nodes added to a running graph; issue Fabric-Project#61 now fixed.
Both auth-status branches now call setupCaptureSession() directly, matching the .authorized case. The node's documented design is queue- agnostic (main in Fabric Editor, a private serial queue in Spark Stage), so pinning setup to main was wrong for non-main embedders. AVCaptureSession.beginConfiguration/commitConfiguration is queue- tolerant.
Expand the doc comment on `devicesLock` to lead with the actual architectural cause of the cross-queue case, not its proximate symptom: Fabric's `@Observable` engine types are main-thread-affine, which is why the AVCaptureDevice notification observer is registered with `queue: .main`, which is what creates the cross-queue read into the consumer queue. SwiftUI is a downstream beneficiary, not the binding constraint. Notes that a UI-agnostic factoring of Parameter (separating engine state from observable UI binding state) would dissolve the constraint, and that sibling provider nodes (CameraProviderNode, ScreenCaptureProviderNode) tolerate the same race rather than guarding it. This node uses Mutex to make the hand-off explicit instead.
|
These commits should resolve all comments except for Mutex on the DeviceList, for which the rationale is in the code comment (and points at a Fabric design issue that’s out of scope here). I’ve changed the base branch for this PR to the Satin 2.0 branch, and now there are conflicts. I’ll get to those... in a bit. |
vade
left a comment
vade
left a comment
There was a problem hiding this comment.
One threading issue, one probably annoying comment by me on LLM comments - thanks for bearing with me.
| /// Available audio devices, populated from `AVCaptureDevice` | ||
| /// connect/disconnect notifications. | ||
| /// | ||
| /// The observer is registered with `queue: .main` because the |
There was a problem hiding this comment.
I think this LLM slop documentation is wrong and misleading as far as I can tell?
The observer could absolutely use its own queue, and dispatch to the node to set on main. These sorts of comments littered in the code base will start to skew other downstream LLM agents reads of the code and compound to bad decisions and a sort of 'look ma no hands' scenario and the accompanying context drift + reasoning collapse.
Can we cut this down into a succinct, no story telling, just the fact's on the threading model without editorialization?
| case .authorized: | ||
| self.setupCaptureSession() | ||
| case .notDetermined: | ||
| AVCaptureDevice.requestAccess(for: .audio) { [weak self] granted in |
There was a problem hiding this comment.
The completion handler is called on an arbitrary dispatch queue. It is the client's responsibility to ensure that any UIKit-related updates are called on the main queue or main thread as a result.
this should be punted to main
2 participants
Inspector / behaviour:
Gaininput withSensitivity(drives the noise floor / how responsive the analyser is to quiet sounds) plus an overdriveGainthat multiplies normalised bar values for visual punch without touching the underlying signal. Adds aBandsVisualizersettings popover so authors can see what the analyser is producing inline.Device Namedropdown to actually route the capture session to the selectedAVCaptureDevice, instead of always using the system default input.Threading + state:
Mutex<DeviceList>. The capture-session setup pullsdevicesfrom the embedder's consumer queue whileAVCaptureDeviceconnect/disconnect notifications mutate it from main; the lock makes the cross-queue access safe.samplesaccess behind aMutexand drop theDispatchQueue.main.asynchop in the capture callback. The capture thread now only stages raw samples + the latest sample rate, andexecute()is the sole site that drains them and mutates filter-bank state. Works regardless of which queue the embedder runs the graph on.Fixes:
AVAudioEngine.inputNode. inputFormatreturnssampleRate=0on macOS before the engine is running, which poisoned every biquad coefficient with NaN and never recovered. Build/rebuild the filter bank from the actual stream rate observed in the tap callback, gated onbufferSampleRate.isFinite && > 0. Self-heal on the envelope side too: ifenv[k]becomes non-finite from a transient, reset it to 0 instead of letting NaN propagate forever.