fix: mic selection, RT-safe denoising, and video pipeline robustness - #2
Open
perfectra1n wants to merge 11 commits into
Open
fix: mic selection, RT-safe denoising, and video pipeline robustness#2perfectra1n wants to merge 11 commits into
perfectra1n wants to merge 11 commits into
Conversation
Switching microphones returned Err("config changed"), so every device
change published Failed health and sat in the 5-second failure backoff
with a dead mic. Worse, the audio-disabled wait loop returned Ok both
for "stop flag set" and "audio re-enabled", and run() read every Ok as
stop — so toggling audio off and on ended the audio thread until a
daemon restart. The same conflation the video pipeline's Outcome enum
fixed; the audio pipeline now carries its own.
Also publish nominal health when denoise is off by choice: a passthrough
the user asked for is healthy, and leaving the previous state in place
could show "failed: config changed" over a working microphone.
On a restart the audio thread is still SCHED_RR from rtkit, and the DeepFilterNet load is a long continuous CPU burn — indistinguishable, under the RLIMIT_RTTIME budget rtkit made us set, from a wedged RT thread. The kernel's answer is SIGKILL to the whole process: no handler, no log line, exit 137 on every microphone switch in a debug build. The demotion has one trap of its own: rtkit promotes threads with SCHED_RESET_ON_FORK set, and a policy passed without that flag is a request to clear it, which needs CAP_SYS_NICE — so a plain SCHED_OTHER demotion fails with EPERM precisely on the thread it exists for. The flag is read back and preserved.
The config stores a PipeWire node.name, but everything the user sees — the GUI picker, list-microphones output — is the node.description. A description written into audio.device fails silently: target.object only matches names, so PipeWire falls back to some other source (the webcam's built-in mic, on the reference machine) while the status line goes on echoing the configured string as if it were live. Set now accepts either and stores the name. A description shared by two devices is deliberately left unresolved rather than guessed at: storing the wrong sibling would be the same silent-wrong-device failure this exists to prevent.
Two halves of one confusion. The picker sent the description it displays as audio.device — the silent-wrong-device trap the daemon commit describes — and never set microphone_index from daemon state, so the ComboBox sat on row 0 showing whichever mic sorts first while the daemon captured something else entirely. Selections now travel as a row index into a parallel microphone-ids model, and the highlight follows the configured id on every poll: -1 for unset or unplugged, never a wrong row.
…tions CI failed on rustfmt and on the ratchet, and every complaint had a structural answer rather than an entry in the ledger: - unsafe_blocks 13 -> 14: demote_current_thread added a second inline sched_setscheduler block. Promotion and demotion now share one set_policy helper, so the syscall has a single unsafe block and a single SAFETY argument again. - run_once cognitive 19 -> 23, lines 109: the disabled-wait park and the denoiser load move out into wait_while_disabled / load_denoiser. run_once lands at cognitive 13 and drops off the exceptions ledger. - wire_controls 143 -> 145 lines: the mic-picker row lookup moves out into mic_id_at, next to mic_index which answers the inverse question. cargo fmt reflowed two test asserts, and ratchet record lowered the floors the refactor earned (run_once and PwSource::run both left the ledger).
…line A truncated MJPG payload is a routine USB event — bandwidth contention on a hub is enough — and it ended in `?`: one bad frame published "JPEG decode failed: Corrupt JPEG data: premature end of data segment" as Failed health, sat out the five-second backoff, then tore down and reopened the camera, renegotiating the very USB link that was already struggling. On a machine where truncation recurs the pipeline flaps forever with a red banner while 29 of every 30 frames were fine. Observed live on a bandwidth-starved C920. A bad frame is now the same non-event a failed matte is: skip it, keep the last good picture on the virtual camera, count it as a drop, and only reopen the device after DECODE_ERROR_LIMIT consecutive failures — at that point the stream is broken, not unlucky. Corruption arrives two ways and both feed one counter: the driver may confess with V4L2_BUF_FLAG_ERROR (now surfaced on RawFrame and checked before paying for a decode — for raw formats it is the only warning there is, a truncated YUYV payload "decodes" into a torn picture), or the decoder finds out the hard way. The matte-inference error block moves out into infer_and_apply_matte unchanged, which pays for the new lines under run_once's ratchet ceiling and starts on the extraction its ledger note already names.
Three doc comments promised a worker thread; none existed. The denoiser ran inside the playback stream's RT_PROCESS callback, behind a Mutex, with a config snapshot (a lock AND a heap allocation) taken per hop — so every cycle where the forward pass overran the quantum budget became silence padding. Audibly choppy, only with denoise enabled, and invisible to any "is it silent" test: the gaps are sub-hop and scattered. Diagnosed from a machine where quantum 1024 meant two full DF3 passes per RT callback. The promised shape is now built. Two wait-free SPSC rings (rtrb) with a normal-priority worker between them: the capture callback pushes raw samples and unparks the worker, the worker chops hops and runs the denoiser, the playback callback pops whole cycles. Nothing on the RT path does more than a bounded memcpy and an atomic — the level meters move from Mutex<f32> (a priority-inversion pair with the daemon's metering poll) to atomic f32 bits. The playback side pops through a CycleReader with deliberate hysteresis: a normal-priority worker cannot beat the microseconds between the capture and playback callbacks of the same graph cycle, so delivery runs on a standing lead of processed audio, rebuilt whole-cycles-at-a-time after stream start and quantum increases, and trimmed after decreases so stale latency is not carried forever. The tests simulate the worst-case event order (push, pop, then worker) on purpose — the friendly order would pass against an implementation that chops in production. VirtualMic::run now takes a factory executed ON the worker thread, and the compiler closed the loop: DfTract holds Rc<Tensor> and is !Send, so the old code was moving a !Send value onto the RT thread through the FFI boundary with no check. Construction on the worker also puts the model load on a thread that is never realtime, making the recent RLIMIT_RTTIME SIGKILL structurally impossible rather than demoted around; health is still reported from where the load runs.
…status Three findings from one debugging session on a second machine, all enabled by us: - V4L2 streams whatever the LAST S_FMT set, and Camera::open's negotiation can be overwritten while we hold the fd idle across power-save stop/start — WirePlumber's v4l2 monitor probes with its own S_FMT. The stored mode then lies, and MJPG-negotiated frames arrive as raw YUYV: "Not a JPEG file: starts with 0x00 0x0a" in a 5-second crash loop. start() now re-asserts the format and reads back what was granted; a same-size decodable swap is adopted (the decoder dispatches per frame), anything else restarts the full negotiation. The policy is a pure function with hardware-free tests. - video.device pointing at a v4l2loopback node was stored verbatim and failed at open with "reports no pixel format we can use". set() now refuses positively-virtual devices with their identity named, the same restraint as resolve_microphone: unknown or unplugged paths still store as given. Open-time gets the same classification so the error names the loopback when config predates the guard. - gpu status seeded "not initialised" and was only ever written once Gpu::new ran — which sits AFTER camera open, so any earlier failure left a placeholder that reads as a GPU fault. Sent a user diagnosing a healthy RTX 5090. The seed now says GPU init was never reached, and the failure path publishes "not attempted" only when no run ever published an adapter — a camera unplug must not erase a working adapter name.
The module's modprobe line set no card_label, so every node — the one cleanroom produces into AND the spare — presented to raw-V4L2 browsers and PipeWire's v4l2 monitor as "Dummy video device (0x000N)". Observed consequences: apps did not recognise the virtual camera by name (the daemon's video.card_label only names its PipeWire node), and WirePlumber elected the producerless spare as the system default video source, handing anything that follows the default a dead device. card_label is now generated to match devices= (primary label from the new services.cleanroom.cardLabel option, defaulting to the name the daemon's sink selection actually matches; spares clearly named "Cleanroom Spare N"), and a WirePlumber rule drops the spares' priority.session so a real camera always outranks them.
VirtualMic::run cognitive 47 -> 39 (the RT callbacks shed their mutex plumbing) and video_pipeline::run_once nesting 6 -> 4 (GPU init extracted). record only ever lowers, so this commits the improvements as the new ceilings.
Found in review: the worker was spawned before a dozen fallible steps in VirtualMic::run, and only the success path stopped and joined it. The most plausible early failure — connect_rc when PipeWire is down — is exactly the case the daemon retries every five seconds, so an outage leaked one immortal thread per retry, each holding a loaded DeepFilterNet session and waking every 10 ms. Worse, each orphan's health report landed AFTER the supervisor's Failed report and replaced it with "nominal" — a mic reported working while no node existed. DenoiseWorker is now a join guard: Drop stops, unparks and joins, so every `?` is covered and the join's ordering closes the health race (the worker's report cannot land after an error it precedes). The regression test drops the guard and asserts the shared Arc's refcount returns to one — a parked-forever worker fails it. Also from review: refuse a comma in services.cleanroom.cardLabel at eval time — v4l2loopback's card_label is comma-delimited, so a comma would silently desync the label list from devices=N.
perfectra1n
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Started as the mic-selection fix and grew as two more machines surfaced real bugs in the same session. Every fix here was diagnosed from live failures, and each commit stands alone.
Microphone selection (original scope)
Audio: DeepFilterNet ran inside the realtime callback
Three doc comments promised a worker thread; none existed. Inference ran in the RT_PROCESS playback callback behind a mutex, with a per-hop config snapshot (a lock and a heap allocation). Any cycle where the forward pass overran the quantum became scattered sub-hop silence: audibly choppy, only with denoise on.
The promised architecture is now built: two lock-free SPSC rings (rtrb) with a normal-priority worker between them. RT callbacks do bounded copies, atomics and a futex wake, nothing else. Level meters moved from mutexes (a priority-inversion pair with the metering poll) to atomic f32 bits. The playback side pops whole cycles through a hysteresis reader that confines silence to settling after stream start or a quantum increase, and trims stale lead after a decrease; the tests simulate the worst-case event order on purpose.
The compiler closed the loop on the old design: DfTract holds Rc and is not Send, so the old code was moving a non-Send value onto the RT thread through the FFI boundary unchecked. The denoiser is now constructed on the worker via a factory, which also puts the model load on a never-realtime thread, making the RLIMIT_RTTIME SIGKILL (exit 137, no log line) structurally impossible instead of demoted around. Review caught that the worker leaked on early error paths and could overwrite a Failed health with nominal; it is now a join guard with a regression test.
Video: robustness against real-world failure modes
Packaging
The NixOS module set no card_label, so both loopback nodes appeared everywhere as "Dummy video device" and WirePlumber elected the producerless spare as the default video source. Labels are now generated to match the device count, with the spare deprioritized via a WirePlumber rule.
Verification
fmt, clippy -D warnings, full test suite and the complexity ratchet are green (the refactors lowered several recorded floors). Live-verified on the reference machine: description-based mic switches complete in about a second with correct pw-link bindings, and the new ring path delivers passthrough audio within 2 dB of input with a consumer attached.