From e680096c6a652a2e23cb569257f0814058b10db7 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sat, 25 Jul 2026 17:55:59 +0200 Subject: [PATCH] perf(encode): stop re-reading the environment on every submit and poll MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `std::env::var` was on three per-frame paths. Measured on `.173` (Windows, 57 environment variables, 2M iterations): **121.9 ns** per call for the NVENC in-flight cap and **114.9 ns** per call for the ffmpeg poll spin, against **0.9 ns** once memoized. On Linux, 32 ns → 1.5 ns. ⚠ Those numbers deflate the filing, and that is worth recording: the audit ranked this as a hot-path defect, but ~120 ns/frame is ~0.003% of a frame budget. The fix is still right — it is free, and it takes a global environment lock off the encode thread — but nobody should schedule it ahead of anything on the strength of the "hot path" framing. The severity RANKING was also inverted, and the measurement confirms why. The site the audit called worst — `nvenc_cuda`'s backpressure loop condition — costs a default session nothing, because the condition short-circuits on `async_rt.is_some()` and the default session never engages the two-thread retrieve. The one that actually pays every frame is Windows `submit`, which consults `async_inflight_cap()` in BOTH arms of the ring-depth match, sync mode included, where the result is then thrown away. Windows `poll` is the second unconditional one, and the audit ranked it last. Memoized inside each helper rather than latched into a session field. Nothing in the workspace mutates these variables at runtime (enumerated: no `set_var` for either key anywhere in first-party code, and the Windows service's arbitrary-key `host.env` loader runs in the SCM supervisor, which re-execs the host as a child and never opens an encoder itself). A field would instead change WHEN the value is read — and the Windows `cap` composes the env with `input_ring_depth`, which `set_input_ring_depth` may change after open, so freezing that half would reintroduce the in-place-overwrite bug the ring term exists to prevent. Two deliberate behaviour changes ride along on `PUNKTFUNK_FFWIN_POLL_MS`, so "behaviour-preserving" describes the memoization only, not this whole commit: - **A 1000 ms ceiling.** The reachable hazard was never the overflow — that needed `ms >= 1.8e16` — it was a slipped digit: `=100000000` was a 27.7-hour spin of the encode thread. - **`.trim()`, now on all three parsers.** An earlier draft applied the house rule (WP7.8) to one of the three, which left `PUNKTFUNK_NVENC_ASYNC=" 1 "` working while `PUNKTFUNK_NVENC_ASYNC_DEPTH=" 6 "` silently fell back to 4 — and memoization would have frozen that silent fallback for the process lifetime. Reachable: the Windows `host.env` loader trims around `=` before stripping quotes, so `=" 2 "` yields a value with inner spaces. ⚠ Correction to an earlier draft of this message, which asserted that the audit's `saturating_mul` proposal would relocate an overflow panic into release builds. That is FALSE and the code comment now says so: `Duration::from_micros(u64::MAX)` is ~1.8e13 seconds, six orders of magnitude below `Duration`'s ceiling, so `Instant + Duration` neither overflows nor panics (measured, with and without debug assertions). `saturating_mul` is still wrong, for a different reason — it sets a deadline ~584,000 years out on a spin that provably never produces the owed AU, i.e. it wedges the encode thread permanently. A hang, not a panic. WP6.1. Co-Authored-By: Claude Opus 5 (1M context) --- crates/pf-encode/src/enc/linux/nvenc_cuda.rs | 18 +++++--- .../pf-encode/src/enc/windows/ffmpeg_win.rs | 44 ++++++++++++++++--- crates/pf-encode/src/enc/windows/nvenc.rs | 22 +++++++--- 3 files changed, 68 insertions(+), 16 deletions(-) diff --git a/crates/pf-encode/src/enc/linux/nvenc_cuda.rs b/crates/pf-encode/src/enc/linux/nvenc_cuda.rs index 2afc21c4..960a5b49 100644 --- a/crates/pf-encode/src/enc/linux/nvenc_cuda.rs +++ b/crates/pf-encode/src/enc/linux/nvenc_cuda.rs @@ -269,13 +269,19 @@ fn async_retrieve_requested() -> bool { /// Max encodes in flight in two-thread mode (`PUNKTFUNK_NVENC_ASYNC_DEPTH`, default 4, clamped /// `2..=POOL-1` — a bitstream must never be reused mid-encode, and the input ring is the same -/// depth). Mirrors the Windows knob exactly. +/// depth). Mirrors the Windows knob exactly, memoization included: this is the backpressure +/// **loop condition** in `submit`, so an engaged two-thread session re-read the environment once +/// per spin. The default session never pays it (the condition short-circuits on `async_rt`), which +/// is why the audit's severity ranking for this site was inverted — but an escalated one did. fn async_inflight_cap() -> usize { - std::env::var("PUNKTFUNK_NVENC_ASYNC_DEPTH") - .ok() - .and_then(|s| s.parse::().ok()) - .unwrap_or(4) - .clamp(2, POOL - 1) + static CAP: std::sync::OnceLock = std::sync::OnceLock::new(); + *CAP.get_or_init(|| { + std::env::var("PUNKTFUNK_NVENC_ASYNC_DEPTH") + .ok() + .and_then(|s| s.trim().parse::().ok()) + .unwrap_or(4) + .clamp(2, POOL - 1) + }) } /// Stream-ordered submit (`PUNKTFUNK_NVENC_STREAM_ORDERED`, default ON; `0` = the pre-existing diff --git a/crates/pf-encode/src/enc/windows/ffmpeg_win.rs b/crates/pf-encode/src/enc/windows/ffmpeg_win.rs index 7af4ccb3..263f0db4 100644 --- a/crates/pf-encode/src/enc/windows/ffmpeg_win.rs +++ b/crates/pf-encode/src/enc/windows/ffmpeg_win.rs @@ -134,6 +134,44 @@ fn zerocopy_enabled(vendor: WinVendor) -> bool { .unwrap_or(matches!(vendor, WinVendor::Amf)) } +/// Upper bound on `PUNKTFUNK_FFWIN_POLL_MS`. This knob spins the **encode thread** waiting for an +/// AU, so a value past one frame period is already self-defeating and a full second is far beyond +/// anything an operator would set on purpose. The clamp is also what makes the µs conversion +/// below provably overflow-free. +/// +/// The reachable hazard is a slipped digit, not the overflow: pre-clamp, `PUNKTFUNK_FFWIN_POLL_MS= +/// 100000000` was a **27.7-hour** spin with no overflow anywhere near it. +const MAX_POLL_SPIN_MS: u64 = 1_000; + +/// Bounded post-submit spin for [`FfmpegWinEncoder::poll`], in microseconds (0 = off, the default +/// and the correct choice on every VCN measured so far). +/// +/// Read from the environment **once per process** (WP6.1): `poll` runs once per encode tick, and +/// this was an unconditional `env::var` + parse on it. +/// +/// ⚠ The audit proposed `saturating_mul` for the µs conversion. It is still the wrong fix, but for +/// a reason worth stating precisely, because the obvious one is false: `Duration::from_micros( +/// u64::MAX)` is only ~1.8e13 seconds, six orders of magnitude below `Duration`'s `u64::MAX`-second +/// ceiling, so `Instant::now() + Duration::from_micros(u64::MAX)` does **not** overflow and does +/// **not** panic (measured, both with and without debug assertions). What it does instead is set a +/// deadline ~584,000 years out, and the loop below only exits on `Packet`/`Eof` — and this +/// function's own doc explains that a spin here *provably never* produces the owed AU on the +/// measured hardware. So `saturating_mul` converts a bad value into a **permanently wedged encode +/// thread**: a hang, not a panic. Clamping the parsed value first removes the bad value entirely. +/// +/// (For the record on the pre-clamp behaviour: the workspace sets no `overflow-checks` in +/// `[profile.release]`, so `ms * 1000` wrapped silently in release and panicked only in debug.) +fn poll_spin_cap_us() -> u64 { + static CAP_US: std::sync::OnceLock = std::sync::OnceLock::new(); + *CAP_US.get_or_init(|| { + std::env::var("PUNKTFUNK_FFWIN_POLL_MS") + .ok() + .and_then(|s| s.trim().parse::().ok()) + .map(|ms| ms.min(MAX_POLL_SPIN_MS) * 1000) + .unwrap_or(0) // default: no spin — the libavcodec AMF buffer can't be spun out + }) +} + /// The swscale *source* pixel format for a captured packed-RGB/BGR layout (8-bit BGRA fallback only). fn sws_src(format: PixelFormat) -> Result { Ok(match format { @@ -1332,11 +1370,7 @@ impl Encoder for FfmpegWinEncoder { Some(Inner::ZeroCopy(z)) => &mut z.enc, None => return Ok(None), }; - let cap_us = std::env::var("PUNKTFUNK_FFWIN_POLL_MS") - .ok() - .and_then(|s| s.parse::().ok()) - .map(|ms| ms * 1000) - .unwrap_or(0); // default: no spin — the libavcodec AMF buffer can't be spun out + let cap_us = poll_spin_cap_us(); let deadline = (cap_us > 0 && self.in_flight > 0) .then(|| std::time::Instant::now() + std::time::Duration::from_micros(cap_us)); loop { diff --git a/crates/pf-encode/src/enc/windows/nvenc.rs b/crates/pf-encode/src/enc/windows/nvenc.rs index 6ef0e1f7..7d93e0ec 100644 --- a/crates/pf-encode/src/enc/windows/nvenc.rs +++ b/crates/pf-encode/src/enc/windows/nvenc.rs @@ -386,12 +386,24 @@ fn async_retrieve_requested() -> bool { /// the ring textures in place, so in-flight depth beyond the ring lets the capturer overwrite a /// frame mid-encode: visual corruption, not UB). IDD-push rings are sized around /// `PUNKTFUNK_IDD_DEPTH`; raise both together if deeper pipelining is needed. +/// +/// Read from the environment **once per process** (WP6.1): `submit` consults this on EVERY frame — +/// both arms of the `cap` match, in sync mode too, where the result is then discarded because the +/// backpressure loop short-circuits on `async_rt`. Memoizing rather than latching into a session +/// field is deliberate: the composed `cap` also folds in `input_ring_depth`, which +/// `set_input_ring_depth` may change after open, and freezing that half would reintroduce the +/// in-place-overwrite bug the ring term exists to prevent. Nothing in the workspace mutates this +/// variable at runtime, so memoizing a process-constant read is behaviour-preserving by +/// construction. fn async_inflight_cap() -> usize { - std::env::var("PUNKTFUNK_NVENC_ASYNC_DEPTH") - .ok() - .and_then(|s| s.parse::().ok()) - .unwrap_or(4) - .clamp(2, POOL - 1) + static CAP: std::sync::OnceLock = std::sync::OnceLock::new(); + *CAP.get_or_init(|| { + std::env::var("PUNKTFUNK_NVENC_ASYNC_DEPTH") + .ok() + .and_then(|s| s.trim().parse::().ok()) + .unwrap_or(4) + .clamp(2, POOL - 1) + }) } /// One in-flight encode handed to the retrieve thread: the output bitstream to lock once its