From 9e8473b46ad833dcb378b7d2f4dd73524c8cde3a Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Tue, 28 Jul 2026 22:45:43 +0200 Subject: [PATCH] refactor(capture/windows): eleven D3D11 helpers had no caller contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The DXGI converters (`HdrP010Converter`, `BgraToYuvPlanes`, `VideoConverter`), the cursor blend pass, and `create_ring_slots` declared their methods `unsafe fn` because their bodies are D3D11 FFI. That is a property of the body, not an obligation on the caller, and their own proofs said so: "`?`-checked D3D11 methods on the live `device` borrow". Every parameter is either a borrowed windows-rs COM wrapper (`&ID3D11Device`, `&ID3D11DeviceContext`, `&ID3D11Texture2D`, the views) or a plain `u32`/`bool`/`DXGI_FORMAT`; each body builds its own descriptors from those and every interface it creates owns its reference. There is nothing a caller can pass that makes them unsound. Not one of the eleven carried a `# Safety` section — the marker had no stated contract to inherit, which is the tell. Each body was already one `unsafe {}` with its proof, so this is purely subtractive: no block moved and no proof was written. The four call sites lost `unsafe {}` wrappers whose own comments gave the game away — "`X::new` is `unsafe` (it compiles D3D11 shaders…)", "`create_ring_slots` is an `unsafe fn` (it makes D3D11/DXGI COM calls)" — comments explaining that the marker described the callee's body rather than anything the caller had to guarantee. `compile_shader` KEEPS its `unsafe fn`: it takes `PCSTR`, a raw pointer the caller must guarantee points at a NUL-terminated literal. That is the line — a contract a caller can actually break. pf-zerocopy was surveyed the same way and deliberately left alone: its CUDA/Vulkan helpers look identical by signature but state real obligations ("the shared context is current", "single-threaded use of handles this bridge owns"), so signature screening finds candidates and only reading decides. pf-capture already denies both `unsafe_op_in_unsafe_fn` and `undocumented_unsafe_blocks`, so the crate stays fully enforced; the reduction is in obligations, not in warnings. It now has exactly one `unsafe fn` left. Verified on the Intel box .47: pf-capture clippy `-D warnings` rc=0 and its 18 Windows tests pass, with host/pf-encode/pf-vdisplay clippy still green; Linux .21 fmt + both CI clippy steps rc=0. --- crates/pf-capture/src/windows/dxgi.rs | 27 ++++++++----- crates/pf-capture/src/windows/idd_push.rs | 40 +++++++++---------- .../src/windows/idd_push/cursor_blend.rs | 10 ++--- .../pf-capture/src/windows/idd_push/open.rs | 2 +- 4 files changed, 38 insertions(+), 41 deletions(-) diff --git a/crates/pf-capture/src/windows/dxgi.rs b/crates/pf-capture/src/windows/dxgi.rs index e0eefbbf..1a0cd1b9 100644 --- a/crates/pf-capture/src/windows/dxgi.rs +++ b/crates/pf-capture/src/windows/dxgi.rs @@ -387,11 +387,20 @@ pub(crate) struct HdrP010Converter { cbuf: ID3D11Buffer, } +// The three converters' methods below are SAFE fns. They were `unsafe fn` because their bodies are +// D3D11 FFI, which is not the same thing as having a caller contract: every parameter is a borrowed +// windows-rs COM wrapper (`&ID3D11Device`, `&ID3D11DeviceContext`, `&ID3D11Texture2D`, the views) or +// a plain `u32`/`bool`, each body builds its own descriptors from those, and every created interface +// owns its reference. There is nothing a caller can pass that makes them unsound, and the proofs the +// markers carried said exactly that — "`?`-checked D3D11 methods on the live `device` borrow" — which +// is a description of the body, not an obligation. `unsafe` now marks the FFI inside them, where the +// blocks and their proofs already were. `compile_shader` KEEPS its marker: it takes `PCSTR`, a raw +// pointer the caller must guarantee is a NUL-terminated literal. impl HdrP010Converter { /// `w`/`h` are the SOURCE dimensions this converter's chroma pass will sample, baked into the /// immutable constant buffer. Rebuild the converter if they change (the IDD capturer's /// `recreate_ring` already drops it). - pub(crate) unsafe fn new(device: &ID3D11Device, w: u32, h: u32) -> Result { + pub(crate) fn new(device: &ID3D11Device, w: u32, h: u32) -> Result { // SAFETY: every call is a `?`-checked D3D11 method on the live `device` borrow, over // fully-initialized stack descriptors and live `Option` out-params; `compile_shader` receives // `s!()` literals (its contract). Each created COM interface owns its own reference, and no @@ -458,7 +467,7 @@ impl HdrP010Converter { /// Called ONCE PER OUT-RING SLOT by the owner of the P010 textures, not per frame — see /// [`Self::convert`]. Fails when the driver rejects a planar RTV, which is the one hard /// requirement of this whole path (a D3D11.3+ runtime plus driver support). - pub(crate) unsafe fn plane_rtv( + pub(crate) fn plane_rtv( device: &ID3D11Device, dst: &ID3D11Texture2D, format: DXGI_FORMAT, @@ -498,7 +507,7 @@ impl HdrP010Converter { /// ring slot's keyed-mutex hold, i.e. time the DRIVER spent blocked on that slot. Both are /// lifetime-of-mode facts: the views belong to the out-ring slot (built in `ensure_out_ring`), /// the buffer to this converter, which is already rebuilt on every mode change. - pub(crate) unsafe fn convert( + pub(crate) fn convert( &self, ctx: &ID3D11DeviceContext, src_srv: &ID3D11ShaderResourceView, @@ -693,7 +702,7 @@ pub(crate) struct BgraToYuvPlanes { } impl BgraToYuvPlanes { - pub(crate) unsafe fn new(device: &ID3D11Device, hdr: bool, chroma444: bool) -> Result { + pub(crate) fn new(device: &ID3D11Device, hdr: bool, chroma444: bool) -> Result { // SAFETY: as `HdrP010Converter::new` — `?`-checked D3D11 shader creation on the live // `device` borrow, with `s!()` literals into `compile_shader` and live out-params. unsafe { @@ -731,7 +740,7 @@ impl BgraToYuvPlanes { /// texture) + `cbcr_rtv` (half- or full-res CbCr texture per the constructed mode). Two opaque /// passes; `w`/`h` are the full luma dims (even for 4:2:0). #[allow(clippy::too_many_arguments)] - pub(crate) unsafe fn convert( + pub(crate) fn convert( &self, ctx: &ID3D11DeviceContext, src_srv: &ID3D11ShaderResourceView, @@ -828,7 +837,7 @@ impl VideoConverter { /// linear (the HDR ring, used by a PyroWave session that tone-maps the HDR desktop down to the /// 8-bit wavelet stream). The output is always studio-range BT.709 NV12 — the P010/BT.2020 HDR /// path is [`HdrP010Converter`]'s job, never this one. - pub(crate) unsafe fn new( + pub(crate) fn new( device: &ID3D11Device, context: &ID3D11DeviceContext, width: u32, @@ -897,11 +906,7 @@ impl VideoConverter { /// Convert `input` (BGRA, or scRGB FP16 for a converter built with `scrgb_input`) → `output` /// (NV12, BT.709 studio-range — see the type doc: never P010) on the video engine. Views are /// created per call (cheap relative to the Blt) so the input texture can vary frame to frame. - pub(crate) unsafe fn convert( - &self, - input: &ID3D11Texture2D, - output: &ID3D11Texture2D, - ) -> Result<()> { + pub(crate) fn convert(&self, input: &ID3D11Texture2D, output: &ID3D11Texture2D) -> Result<()> { // SAFETY: both view creations are `?`-checked calls on `self.vdev` with fully-initialized // stack descriptors and live out-params. `stream.pInputSurface` is a `ManuallyDrop` of the // input view just created: `VideoProcessorBlt` only BORROWS it (a COM in-param never transfers diff --git a/crates/pf-capture/src/windows/idd_push.rs b/crates/pf-capture/src/windows/idd_push.rs index 54ff1de6..db39c2ac 100644 --- a/crates/pf-capture/src/windows/idd_push.rs +++ b/crates/pf-capture/src/windows/idd_push.rs @@ -648,11 +648,7 @@ impl IddPushCapturer { // nothing downstream could detect the mismatch, because every field it would compare against // had already been moved. Nothing below this line fails. let fmt = Self::ring_format_for(new_display_hdr); - // SAFETY: `create_ring_slots` is an `unsafe fn` (it makes D3D11/DXGI COM calls); we pass a live - // borrow of `self.device` (the capturer's own device, on which the slots are created) plus plain - // `u32`/`DXGI_FORMAT` values, and `?` propagates any failure before the slots are used. Every - // returned slot's texture + keyed mutex belongs to that same `self.device`. - let new_slots = unsafe { Self::create_ring_slots(&self.device, new_w, new_h, fmt)? }; + let new_slots = Self::create_ring_slots(&self.device, new_w, new_h, fmt)?; self.display_hdr = new_display_hdr; self.width = new_w; self.height = new_h; @@ -969,11 +965,11 @@ impl IddPushCapturer { /// mid-session mode swap. fn ensure_pyro_conv(&mut self) -> Result<()> { if self.pyro_conv.is_none() { - // SAFETY: `BgraToYuvPlanes::new` compiles D3D11 shaders on `self.device`; `?` propagates - // failure before it is stored. - self.pyro_conv = Some(unsafe { - BgraToYuvPlanes::new(&self.device, self.display_hdr, self.want_444)? - }); + self.pyro_conv = Some(BgraToYuvPlanes::new( + &self.device, + self.display_hdr, + self.want_444, + )?); } Ok(()) } @@ -984,22 +980,22 @@ impl IddPushCapturer { fn ensure_converter(&mut self) -> Result<()> { if self.display_hdr { if self.hdr_p010_conv.is_none() { - // SAFETY: `HdrP010Converter::new` is `unsafe` (it compiles D3D11 shaders + creates - // resources); we pass a live borrow of `self.device`, the device the converter's resources - // belong to, and `?` propagates any failure before the converter is stored. - self.hdr_p010_conv = - Some(unsafe { HdrP010Converter::new(&self.device, self.width, self.height)? }); + self.hdr_p010_conv = Some(HdrP010Converter::new( + &self.device, + self.width, + self.height, + )?); } } else if self.want_444 { // Full-chroma passthrough — no conversion resources to build. } else if self.video_conv.is_none() { - // SAFETY: `VideoConverter::new` is `unsafe` (it sets up the D3D11 VIDEO processor); we pass live - // borrows of `self.device` + its immediate `self.context` (single-threaded, this thread) plus - // plain `u32` dimensions, and `?` propagates any failure before it is stored. The converter's - // resources belong to that same device/context. - self.video_conv = Some(unsafe { - VideoConverter::new(&self.device, &self.context, self.width, self.height, false)? - }); + self.video_conv = Some(VideoConverter::new( + &self.device, + &self.context, + self.width, + self.height, + false, + )?); } Ok(()) } diff --git a/crates/pf-capture/src/windows/idd_push/cursor_blend.rs b/crates/pf-capture/src/windows/idd_push/cursor_blend.rs index 77b35b01..cf389496 100644 --- a/crates/pf-capture/src/windows/idd_push/cursor_blend.rs +++ b/crates/pf-capture/src/windows/idd_push/cursor_blend.rs @@ -56,7 +56,7 @@ pub(super) struct CursorBlendPass { } impl CursorBlendPass { - pub(super) unsafe fn new(device: &ID3D11Device) -> Result { + pub(super) fn new(device: &ID3D11Device) -> Result { // SAFETY: `?`-checked D3D11 resource creation on the live `device` borrow, over // fully-initialized stack descriptors and live out-params; `compile_shader` receives `s!()` // literals (its contract). @@ -116,11 +116,7 @@ impl CursorBlendPass { } /// Upload `ov`'s bitmap if its serial is new; reuse the cached SRV otherwise. - unsafe fn ensure_shape( - &mut self, - device: &ID3D11Device, - ov: &pf_frame::CursorOverlay, - ) -> Result<()> { + fn ensure_shape(&mut self, device: &ID3D11Device, ov: &pf_frame::CursorOverlay) -> Result<()> { // SAFETY: `CreateTexture2D`/`CreateShaderResourceView` are `?`-checked calls on the live // `device` borrow. `init.pSysMem` points into `ov.rgba`, which the length check above proves // holds at least `ov.w * ov.h * 4` bytes for the declared `SysMemPitch = ov.w * 4`, and which @@ -170,7 +166,7 @@ impl CursorBlendPass { /// composition) — linearize and scale to the target's SDR white. The quad is placed purely /// via the viewport (the fullscreen-triangle VS fills whatever viewport is set), clipped by /// the target automatically. - pub(super) unsafe fn blend( + pub(super) fn blend( &mut self, device: &ID3D11Device, ctx: &ID3D11DeviceContext, diff --git a/crates/pf-capture/src/windows/idd_push/open.rs b/crates/pf-capture/src/windows/idd_push/open.rs index a8ed1470..6103bd44 100644 --- a/crates/pf-capture/src/windows/idd_push/open.rs +++ b/crates/pf-capture/src/windows/idd_push/open.rs @@ -85,7 +85,7 @@ impl IddPushCapturer { /// to the display's composition format — FP16 in HDR, BGRA in SDR). Each is shared through an /// UNNAMED NT handle (nothing to open by name — the sealed channel); the driver reaches it only via /// the duplicate the [`ChannelBroker`] sends after the ring is published. - pub(super) unsafe fn create_ring_slots( + pub(super) fn create_ring_slots( device: &ID3D11Device, w: u32, h: u32,