From 50b3fd1012e43078d23179a6c8bcd3e5ca22c1be Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Thu, 6 Aug 2026 23:58:24 +0200 Subject: [PATCH] =?UTF-8?q?fix(pf-encode):=20drop=20the=2010-bit=20short?= =?UTF-8?q?=20circuit=20=E2=80=94=20measured=20wrong=20on=20Ada,=20twice?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WP1.3, and the measurement that justifies it. `resolve_split_mode`'s 10-bit rule sat ABOVE the pixel-rate arm and took no codec, so it (D1) vetoed 10-bit 4K120 -- the very case the pixel-rate arm exists for -- and (D2) applied an HEVC-Main10-on- Ada result to AV1 10-bit, which has no such measurement. Both fixed: the pixel-rate arm now comes first, and what remains is codec-scoped to HEVC and only applies BELOW that bar, where a second engine buys nothing anyway. The rule rested on one datapoint: 5120x1440@240 Main10 on Ada, forced-2 7.6 ms vs 2.8 ms single-engine -- split 2.7x SLOWER. Dropping the short circuit flips that exact configuration's behaviour, so it was re-measured on a 4090 (AD102, driver 610.43.03), 400 Mbps, sub-frame pinned off, via a new mode-parameterizable Main10 A/B test (PF_AB_MODE=WxHxFPS reproduces the original operating point). Ada 4090 single forced-2 ratio 3840x2160@60 4483 us 2178 us 2.06x split WINS 5120x1440@240 3689 us 2813 us 1.31x split WINS <- the veto's origin 3840x2160@120 4148 us 2189 us 1.89x split WINS Blackwell 5070 Ti 3840x2160@60 4216 us 2477 us 1.70x split WINS 5120x1440@240 4651 us 3894 us 1.19x split WINS Split wins for Main10 at every mode on BOTH architectures, including the config the veto came from. The original number does not reproduce. ⚠ Caveats, unchanged from the rest of this work: content is trivial (297-300 B/AU against an 833 KB CBR quota -- zeroed VRAM), so this is the pixel-proportional term and the bits/frame regime is still unmeasured; debug build; and the driver differs from whenever the original was taken. Also validated on Ada in the same session -- the whole spike set reproduces on a SECOND architecture and an OLDER driver (610.43.03 vs 610.57.04): S1a in-place split switch accepted with zero IDRs both directions; S1b takes effect (|C-B|=12 vs |C-A|=1921, the cleanest run yet); S1c pair flip passes; D5 confirmed (AUTO+sub-frame 4424 vs DISABLE 4409, 15 us apart -- and AUTO without sub-frame 2310 ~= TWO_FORCED 2314, so the arm stays); engines=2 with THREE_FORCED correctly clamped to mode 2; arbitration converged with exactly 1 keyframe. Verified: .21 clippy -D warnings clean + 64 unit tests; .133 Windows clippy -D warnings clean (the resolver signature grew a `codec` param, so both backends moved); Ada + Blackwell on-hardware as above. fmt clean. --- crates/pf-encode/src/enc/linux/nvenc_cuda.rs | 110 ++++++++++++++++++- crates/pf-encode/src/enc/nvenc_core.rs | 93 +++++++++++++--- crates/pf-encode/src/enc/windows/nvenc.rs | 2 +- 3 files changed, 186 insertions(+), 19 deletions(-) diff --git a/crates/pf-encode/src/enc/linux/nvenc_cuda.rs b/crates/pf-encode/src/enc/linux/nvenc_cuda.rs index 8babbbbe..bb6c6ab4 100644 --- a/crates/pf-encode/src/enc/linux/nvenc_cuda.rs +++ b/crates/pf-encode/src/enc/linux/nvenc_cuda.rs @@ -1366,7 +1366,7 @@ impl NvencCudaEncoder { // [`resolve_split_mode`] for the precedence (env override / 10-bit / pixel rate). let pixel_rate = self.width as u64 * self.height as u64 * self.fps.max(1) as u64; let mut split_mode: u32 = - resolve_split_mode(self.bit_depth, pixel_rate, self.encoder_engines); + resolve_split_mode(self.codec, self.bit_depth, pixel_rate, self.encoder_engines); // A verdict this process already measured for this exact config wins over the static // rule — that is the whole point of arbitrating, and it lets later sessions skip the // ~1 s experiment. An operator pin still beats both (checked inside `resolve_split_mode`, @@ -3942,6 +3942,114 @@ mod tests { super::super::nvenc_core::clear_split_verdicts(); } + /// ON-HARDWARE — **THE ADA MAIN10 QUESTION**, the one this whole programme has been deferring. + /// + /// The 10-bit split veto rests on a single datapoint: at 5120×1440@240 Main10 on Ada, forced-2 + /// took 7.6 ms/frame against 2.8 ms single-engine — split was **2.7× SLOWER**. That number + /// vetoed splitting for every HDR session on every GPU, and `resolve_split_mode` has now + /// stopped short-circuiting on it, which means a Main10 session above the pixel-rate bar WILL + /// split. If the datapoint generalises, that is a regression and the veto has to come back + /// (scoped properly this time). + /// + /// So: 4K **Main10** (10-bit, via the packed-RGB10 input path), forced-2 against + /// single-engine, same bitrate, sub-frame pinned off so only the split variable moves. + /// Reports rather than asserts — both outcomes are legitimate findings and the point is the + /// number. Run on the **Ada** box (`.181`) and compare against Blackwell (`.21`): + /// cargo test -p pf-encode --features nvenc -- --ignored --test-threads=1 \ + /// nvenc_cuda_main10_split_ab --nocapture + #[test] + #[ignore = "requires an NVIDIA GPU + driver — run manually (Ada .181 vs Blackwell .21)"] + fn nvenc_cuda_main10_split_ab() { + use std::time::Instant; + const BPS: u64 = 400_000_000; + const WARMUP: u32 = 12; + const MEASURED: u32 = 32; + // Mode is overridable so the SAME test can be pointed at the configuration the veto was + // originally measured on — `PF_AB_MODE=5120x1440x240` reproduces the 2.7×-slower datapoint's + // operating point, which is the one config this change flips behaviour for. + let (w, h, fps) = std::env::var("PF_AB_MODE") + .ok() + .and_then(|s| { + let p: Vec = s.split('x').filter_map(|v| v.parse().ok()).collect(); + (p.len() == 3).then(|| (p[0], p[1], p[2])) + }) + .unwrap_or((3840, 2160, 60)); + + std::env::set_var("PUNKTFUNK_NVENC_SUBFRAME", "0"); + pf_zerocopy::cuda::make_current().expect("shared CUDA context current"); + // 10-bit input: the packed 2:10:10:10 PQ path is how a Main10 session is actually fed here + // (`bit_depth`/`hdr` are DERIVED from the input format, never trusted from the args). + let frames: Vec = (0..4).map(|i| rgb10_frame(w, h, i)).collect(); + + let run = |split: &str| -> (u128, u8, usize) { + std::env::set_var("PUNKTFUNK_SPLIT_ENCODE", split); + let mut enc = NvencCudaEncoder::open( + Codec::H265, + PixelFormat::X2Rgb10, + w, + h, + fps, + BPS, + true, + 10, + ChromaFormat::Yuv420, + false, + 4, + ) + .expect("open NVENC CUDA session"); + let (mut times, mut bytes) = (Vec::new(), Vec::new()); + for i in 0..(WARMUP + MEASURED) { + let t0 = Instant::now(); + enc.submit_indexed(&frames[(i % 4) as usize], i) + .expect("submit"); + let mut got = 0usize; + while let Some(au) = enc.poll().expect("poll") { + got = au.data.len(); + } + if i >= WARMUP { + times.push(t0.elapsed().as_micros()); + bytes.push(got); + } + } + let depth = enc.bit_depth; + let opened = enc.split_mode; + enc.flush().ok(); + times.sort_unstable(); + bytes.sort_unstable(); + println!( + " (opened split_mode={opened}, derived bit_depth={depth}, \ + {} B/AU)", + bytes[bytes.len() / 2] + ); + (times[times.len() / 2], depth, bytes[bytes.len() / 2]) + }; + + println!( + "Main10 split A/B @ {w}x{h}@{fps} HEVC 10-bit, {} Mbps:", + BPS / 1_000_000 + ); + let (single_us, d1, _) = run("0"); + println!(" single-engine : {single_us:>6} us/frame"); + let (split_us, d2, _) = run("2"); + println!(" forced 2-way : {split_us:>6} us/frame"); + assert_eq!(d1, 10, "leg 1 did not derive a 10-bit session"); + assert_eq!(d2, 10, "leg 2 did not derive a 10-bit session"); + let ratio = single_us as f64 / split_us.max(1) as f64; + println!( + " ⇒ split is {ratio:.2}× the single-engine rate — {}", + if ratio > 1.15 { + "split WINS for Main10 here; the 2.7x-slower datapoint does NOT generalise" + } else if ratio < 0.87 { + "split LOSES for Main10 — the veto was right and must come back, scoped" + } else { + "a wash; neither arm is clearly better for Main10 here" + } + ); + + std::env::remove_var("PUNKTFUNK_SPLIT_ENCODE"); + std::env::remove_var("PUNKTFUNK_NVENC_SUBFRAME"); + } + /// A pre-session RFI request and nonsense ranges all correctly decline (→ caller forces IDR). /// Needs no GPU session (it short-circuits on the null encoder / range checks), so it runs in the /// normal suite — but `open` gates on the NVENC `.so`, so it skips gracefully where the NVIDIA diff --git a/crates/pf-encode/src/enc/nvenc_core.rs b/crates/pf-encode/src/enc/nvenc_core.rs index 6c79d93a..589c1d73 100644 --- a/crates/pf-encode/src/enc/nvenc_core.rs +++ b/crates/pf-encode/src/enc/nvenc_core.rs @@ -86,14 +86,22 @@ pub(super) fn resolve_subframe(default_on: bool) -> bool { /// 1. `PUNKTFUNK_SPLIT_ENCODE` = `0`/`disable` | `1`/`auto` (AUTO_FORCED) | `2` | `3` — operator /// override, always wins, except that `2`/`3` are clamped to the GPU's real engine count (see /// [`clamp_to_engines`]; the driver honours an over-ask and silently encodes narrower). -/// 2. 10-bit → DISABLE: 2-way split is measurably SLOWER on Ada for Main10 — at 5120×1440@240 -/// forced-2 took 7.6 ms/frame (~131 fps) vs 2.8 ms (~357 fps) single-engine (the split/merge -/// overhead dominates), and a single engine handles 5K@240 Main10 well under budget. This was -/// the "broken animations in HDR" cap at ~131 fps. -/// 3. Pixel rate ≥ [`super::SPLIT_FORCE_PIXEL_RATE`] → force the WIDEST split the GPU can deliver +/// 2. Pixel rate ≥ [`super::SPLIT_FORCE_PIXEL_RATE`] → force the WIDEST split the GPU can deliver /// ([`max_forced_split_mode`]), not a hard-coded 2 (AUTO never engages below ~2112 px height, /// so 4K120 must be forced onto the other engines; and a 3-NVENC part left at 2-way wastes a /// third of its encode silicon). +/// 3. **HEVC** Main10 below that bar → DISABLE: 2-way split measured SLOWER on Ada for Main10 — at +/// 5120×1440@240 forced-2 took 7.6 ms/frame (~131 fps) vs 2.8 ms (~357 fps) single-engine, the +/// "broken animations in HDR" cap. ⚠ This rule used to sit ABOVE the pixel-rate arm and take no +/// codec, so it (a) vetoed 10-bit **4K120** — the very case the pixel-rate arm exists for — and +/// (b) applied an HEVC-on-Ada result to **AV1 10-bit**, which has no such measurement. Both +/// fixed; what remains is a conservative default in the regime where a second engine buys +/// nothing anyway. +/// ⚠⚠ **UNVALIDATED CONSEQUENCE:** 5120×1440@240 Main10 (1.77 Gpix/s) now clears the pixel-rate +/// bar and WILL be forced to split — i.e. the exact configuration that measurement came from +/// flips behaviour. That is deliberate (the datapoint is one sample, at low bits/frame, and the +/// bits/frame hypothesis predicts it should not generalise) but it is **the first thing to +/// re-measure on Ada**; `PUNKTFUNK_SPLIT_ENCODE=0` is the escape if it regresses. /// 4. Else AUTO — ⚠ whose behaviour is **conditional on sub-frame**, measured on `.21` at 4K: /// - sub-frame **ON** (the fleet default): AUTO **does not split** — 5023/5157 µs against /// DISABLE's 4979/5000. Split and sub-frame are mutually unsupported for HEVC, so the driver @@ -110,7 +118,12 @@ pub(super) fn resolve_subframe(default_on: bool) -> bool { /// `engines` is the GPU's `NV_ENC_CAPS_NUM_ENCODER_ENGINES`; pass `0` when it could not be probed /// (treated as "unknown", which keeps the pre-probe behaviour of assuming a second engine exists /// and letting the open-time rejection fallback sort it out). -pub(super) fn resolve_split_mode(bit_depth: u8, pixel_rate: u64, engines: u32) -> u32 { +pub(super) fn resolve_split_mode( + codec: Codec, + bit_depth: u8, + pixel_rate: u64, + engines: u32, +) -> u32 { use nv::NV_ENC_SPLIT_ENCODE_MODE as M; let hw_max = max_forced_split_mode(engines); let mode = match std::env::var("PUNKTFUNK_SPLIT_ENCODE").ok().as_deref() { @@ -118,14 +131,26 @@ pub(super) fn resolve_split_mode(bit_depth: u8, pixel_rate: u64, engines: u32) - Some("1") | Some("auto") => M::NV_ENC_SPLIT_AUTO_FORCED_MODE as u32, Some("3") => clamp_to_engines(M::NV_ENC_SPLIT_THREE_FORCED_MODE as u32, hw_max, engines), Some("2") => clamp_to_engines(M::NV_ENC_SPLIT_TWO_FORCED_MODE as u32, hw_max, engines), - _ if bit_depth >= 10 => M::NV_ENC_SPLIT_DISABLE_MODE as u32, // Use every engine the card has, not a hard-coded two: on a 3-NVENC part (GB202, AD102 // workstation) forcing 2 leaves a third of the silicon idle. + // + // ⚠ This arm now comes FIRST, ahead of the 10-bit rule. That reordering is the D1 fix: a + // 10-bit 4K120 session (995.3 Mpix/s) used to be vetoed by the depth rule before ever + // reaching the pixel-rate arm written for exactly it. _ if pixel_rate >= super::SPLIT_FORCE_PIXEL_RATE => hw_max, + // Below that bar, HEVC Main10 keeps the conservative single-engine default. The one Ada + // measurement we have says split can be *slower* for Main10, and nothing under this bar + // needs a second engine anyway — so the cost of being wrong here is ~nil, unlike above it. + // + // ⚠ Now codec-scoped (the D2 fix): the measurement behind this was HEVC Main10 on Ada, and + // it used to veto **AV1 10-bit** too, which has neither the sub-frame conflict nor any + // measurement against it. + _ if codec == Codec::H265 && bit_depth >= 10 => M::NV_ENC_SPLIT_DISABLE_MODE as u32, _ => M::NV_ENC_SPLIT_AUTO_MODE as u32, }; tracing::debug!( split_mode = mode, + ?codec, bit_depth, pixel_rate, engines, @@ -695,7 +720,7 @@ mod tests { // 4090 because AUTO never engages at 2160 px height. let four_k_120 = 3840u64 * 2160 * 120; assert_eq!( - resolve_split_mode(8, four_k_120, 2), + resolve_split_mode(Codec::H265, 8, four_k_120, 2), M::NV_ENC_SPLIT_TWO_FORCED_MODE as u32 ); } @@ -705,22 +730,56 @@ mod tests { // 884.7 Mpix/s is comfortably single-engine — the threshold move must not drag it in. let qhd_240 = 2560u64 * 1440 * 240; assert_eq!( - resolve_split_mode(8, qhd_240, 2), + resolve_split_mode(Codec::H265, 8, qhd_240, 2), M::NV_ENC_SPLIT_AUTO_MODE as u32 ); } #[test] - fn split_disabled_for_10bit_even_at_high_pixel_rate() { - // The measured Main10 rule: split/merge overhead dominates 10-bit on Ada (7.6 ms forced-2 - // vs 2.8 ms single-engine at 5K240) — 10-bit precedes the pixel-rate arm. - let five_k_240 = 5120u64 * 1440 * 240; + fn split_rules_for_10bit_after_dropping_the_short_circuit() { + let five_k_240 = 5120u64 * 1440 * 240; // 1.77 Gpix/s — over the bar + let four_k_120 = 3840u64 * 2160 * 120; // 995.3 Mpix/s — over the bar + let hd_60 = 1920u64 * 1080 * 60; // 124 Mpix/s — well under + + // ⚠ BEHAVIOUR FLIP, deliberate: the config the Main10 veto was measured on (7.6 ms + // forced-2 vs 2.8 ms single-engine on Ada) now clears the pixel-rate bar and SPLITS. The + // datapoint is one sample at low bits/frame; re-measuring it on Ada is the first on-glass + // item, and PUNKTFUNK_SPLIT_ENCODE=0 is the escape if it regresses. assert_eq!( - resolve_split_mode(10, five_k_240, 2), + resolve_split_mode(Codec::H265, 10, five_k_240, 2), + M::NV_ENC_SPLIT_TWO_FORCED_MODE as u32 + ); + // D1: 10-bit 4K120 used to be vetoed by the depth rule BEFORE reaching the pixel-rate arm + // written for exactly it. It splits now. + assert_eq!( + resolve_split_mode(Codec::H265, 10, four_k_120, 2), + M::NV_ENC_SPLIT_TWO_FORCED_MODE as u32 + ); + // Under the bar, HEVC Main10 keeps the conservative single-engine default — a second + // engine buys nothing there, so being wrong costs ~nil. + assert_eq!( + resolve_split_mode(Codec::H265, 10, hd_60, 2), M::NV_ENC_SPLIT_DISABLE_MODE as u32 ); } + /// D2: the Main10 rule was measured on HEVC and used to be codec-blind, so it vetoed **AV1 + /// 10-bit** — which has neither the sub-frame conflict nor any measurement against it. + #[test] + fn av1_10bit_is_no_longer_vetoed_by_an_hevc_measurement() { + let hd_60 = 1920u64 * 1080 * 60; + let four_k_120 = 3840u64 * 2160 * 120; + assert_eq!( + resolve_split_mode(Codec::Av1, 10, hd_60, 2), + M::NV_ENC_SPLIT_AUTO_MODE as u32, + "AV1 10-bit must follow the ordinary path, not inherit an HEVC veto" + ); + assert_eq!( + resolve_split_mode(Codec::Av1, 10, four_k_120, 2), + M::NV_ENC_SPLIT_TWO_FORCED_MODE as u32 + ); + } + /// THE ENGINE-COUNT FIX: a high-pixel-rate session must use every engine the GPU has, not a /// hard-coded two. A 3-NVENC part (GB202 / AD102 workstation) left at 2-way wastes a third of /// its encode silicon, and the driver never complains because it accepts an over- OR @@ -729,17 +788,17 @@ mod tests { fn split_uses_every_engine_the_gpu_has() { let four_k_120 = 3840u64 * 2160 * 120; assert_eq!( - resolve_split_mode(8, four_k_120, 3), + resolve_split_mode(Codec::H265, 8, four_k_120, 3), M::NV_ENC_SPLIT_THREE_FORCED_MODE as u32, "a 3-engine GPU must split three ways" ); assert_eq!( - resolve_split_mode(8, four_k_120, 1), + resolve_split_mode(Codec::H265, 8, four_k_120, 1), M::NV_ENC_SPLIT_DISABLE_MODE as u32, "a 1-engine GPU must not pretend to split — today this costs a wasted session open" ); assert_eq!( - resolve_split_mode(8, four_k_120, 0), + resolve_split_mode(Codec::H265, 8, four_k_120, 0), M::NV_ENC_SPLIT_TWO_FORCED_MODE as u32, "unprobed engine count keeps the historical assumption; the rejection fallback corrects" ); diff --git a/crates/pf-encode/src/enc/windows/nvenc.rs b/crates/pf-encode/src/enc/windows/nvenc.rs index 6e8a63ae..9e562c2f 100644 --- a/crates/pf-encode/src/enc/windows/nvenc.rs +++ b/crates/pf-encode/src/enc/windows/nvenc.rs @@ -1166,7 +1166,7 @@ impl NvencD3d11Encoder { // The init-failure fallback below disables it if a codec/config rejects it. let pixel_rate = self.width as u64 * self.height as u64 * self.fps.max(1) as u64; let split_mode: u32 = - resolve_split_mode(self.bit_depth, pixel_rate, self.encoder_engines); + resolve_split_mode(self.codec, self.bit_depth, pixel_rate, self.encoder_engines); // Negotiated multi-slice (P2f): the direct-NVENC default of 4, clamped by the // client's ceiling — a single-slice client keeps today's shape, a // VIDEO_CAP_MULTI_SLICE / Moonlight slices-per-frame client gets real slices.