8 Commits

Include renames

Author	SHA1	Message	Date
enricobuehler	e1ddd49e37	fix(client-core,ffvk): close the proof-lint hole in two of the three unguarded crates ... `clippy::undocumented_unsafe_blocks` is what makes the SAFETY convention a rule rather than a habit, and three crates had never adopted it — pf-client-core (91 unsafe items), pf-presenter (123) and punktfunk-core (167) — while every other subsystem crate denied it. That gap is why the decoders' `unsafe impl Send`s carried a one-line aside instead of an argument: nothing required one. pf-client-core and pf-ffvk now deny it, with a proof written for all 58 + 3 sites they had. ⚠️ 44 of those 58 were WINDOWS-ONLY — `clipboard.rs` 24 and `video_d3d11.rs` 20 — and invisible to the Linux measurement that sized this work at 14. Same trap as the E0133 sweep: a Linux-only survey of a cross-platform crate undercounts by whatever the `cfg` hides, here by 3x. Landing the deny on the strength of that number alone would have re-broken Windows CI, which is exactly the mistake this session already made once with the `warn`-that-was-really-`deny`. The proofs say what is actually load-bearing rather than restating the call. In `clipboard.rs` that is the ownership split Win32 requires and nothing in the code stated: `GetClipboardData` returns a handle BORROWED from the clipboard (never freed here), while `GlobalAlloc` + `SetClipboardData` TRANSFERS ownership to it (which is why nothing frees that one either) — two opposite rules, three lines apart. In `video_d3d11.rs` the recurring one is that libav's `get_format` list is NUL-terminated by `AV_PIX_FMT_NONE`, which is what keeps the walk in bounds. Remaining: punktfunk-core (~146, of which `abi.rs` is 141) and pf-presenter (~108). Both want the "state the contract once" treatment — abi.rs's sites are a handful of repeating shapes (`opt_cstr` on caller C strings, null-guarded out-param writes, forwarding calls), not 141 distinct arguments. Note the vendored `fec-rs` (18 sites) is a separate path-dependency crate, so it is out of scope rather than something to prove. Verified: Linux .21 fmt + both CI clippy steps rc=0; Windows .47 `-p pf-client-core` clippy `-D warnings` rc=0 (the only place the 44 are visible), plus the full Windows CI clippy set and pf-capture's 18 tests.	2026-07-29 08:48:42 +02:00
enricobuehler	d2b6f5b65f	docs(unsafe): audit all 49 unsafe impl — one proof was wrong, four were missing ... `unsafe impl Send`/`Sync` is the highest-risk unsafe category here and the one this program had never looked at: a wrong one is cross-thread UB that is invisible at every call site, with no `unsafe` block to catch a reviewer's eye. 49 of them (41 Send, 8 Sync). Two results. **`MappedView`'s `Sync` proof was factually wrong.** It read "only exposes accessors that are safe under concurrent use" — they are not. `read_u8`/`write_u8`/`read_u16` are plain unaligned accesses through `&self`, and `&MappedView` really is shared across threads: `ChannelState::data()` hands out `&'static MappedView`, and pf-xusb, pf-mouse and pf-gamepad all dispatch `WdfIoQueueDispatchParallel` with `NumberOfPresentedRequests = u32::MAX`. The struct's own doc had the right story — consistency is the channel protocol's job — but the `unsafe impl` stated a different, stronger claim, which is the one a reviewer checking that line would rely on. The impl is still sound, for a reason worth writing down: these bytes are mapped into ANOTHER PROCESS that writes them concurrently, so Rust-level exclusivity over them is unachievable no matter what this type does. Sync fields go through the atomic accessors; the plain ones cover only protocol-fenced bytes. The proof now says that, and states the rule it implies for accessors added later — plain path only for bytes the protocol already fences. **Four `Send` impls carried a one-line aside instead of a proof** — the pf-client-core decoders and `DrmFrameGuard`. All four are sound, and each now says why, including the two facts that make them work and were nowhere stated: libav permits a codec context to be used from a thread other than its creator provided use is serialised (`&mut self` is that serialisation), and D3D11's immediate context is thread-AGNOSTIC rather than thread-safe — it wants serialised use, not one fixed thread. Each also records that it is deliberately not `Sync`, which is the invariant a future `impl Sync` would silently break. Every one of the 49 now carries reasoning. Verified: Linux .21 fmt + both CI clippy steps rc=0; Windows .47 `-p pf-client-core` clippy `-D warnings` rc=0 (it compiles the `video_d3d11` proof the Linux run cannot see). The pf-umdf-util edit is comment-only — confirmed by diff, since that crate needs the WDK, which .47 does not have.	2026-07-29 00:37:14 +02:00
enricobuehler	ee29b3c3a9	refactor(client-core): three decoder lift methods that took nothing to get wrong ... `D3d11Decoder::lift`, `VaapiDecoder::map_dmabuf` and `VulkanDecoder::extract` each take `&mut self` and NOTHING else. They dereference `self.frame`, the `*mut AVFrame` the decoder allocates in its own constructor and owns until `Drop` — a pointer no caller supplies, can replace, or can invalidate. None carried a `# Safety` section, and each body was already one `unsafe {}` with its proof, so the marker was pure ceremony and its removal moves nothing. All three live in files the workspace `deny` already covers (`video_vulkan.rs` since the fence came off it), so they stay fully enforced. Worth recording why this is where the marker sweep STOPS for these two crates: every remaining candidate in pf-encode and pf-client-core sits inside a fenced file, and there removing a marker is not free. The fence excuses `unsafe_op_in_unsafe_fn`, which only applies to `unsafe fn` BODIES — a safe fn always needs explicit blocks — so unmarking an ash-dense helper forces exactly the whole-body `unsafe {}` this program rejected when it rejected `cargo fix`. The way to reach those is to reclaim the file first, not to unmark inside it. Verified on .21: fmt + `clippy --workspace --all-targets -- -D warnings` + the feature-gated `-p pf-encode` step, all rc=0.	2026-07-29 00:20:35 +02:00
enricobuehler	ed4bbc6b0b	refactor(client-core): the first fence comes off — video_vulkan.rs is at zero ... `video_vulkan.rs` was fenced with the other thirteen GPU/FFI backends, but it never belonged with them: its four sites are not ash calls, they are RAW POINTER DEREFERENCES inside the two FFmpeg `lock_queue`/`unlock_queue` callback trampolines — exactly the case where the lint pays, and where a proof carries an argument instead of restating a signature. Both bodies had none at all. They keep `unsafe extern "C"`: FFmpeg invokes them with a `*mut AVHWDeviceContext`, which is a real contract, now written down. What was missing is why the dereference chain is sound, and it is worth stating because it is not obvious — the trampoline casts between pf_ffvk's `AVHWDeviceContext` and ffmpeg-sys's (the same C struct declared twice), reads `user_opaque`, and dereferences it as a `QueueLock`. That pointer borrows `VkCtxStorage::_queue_lock`, an `Arc<QueueLock>` whose field doc already says it exists to outlive every call the context can make. The proof now connects those two facts, so a future edit to the storage's lifetime has something to contradict. With that, the file needs no exemption and the workspace `deny` covers it: 14 fenced files -> 13. This is what removing a fence is supposed to look like — the sites that are worth narrowing get narrowed, and the exemption disappears rather than being renewed. Verified on .21: fmt + `clippy --workspace --all-targets -- -D warnings` + the feature-gated `-p pf-encode` step, all rc=0 with no allow in the file.	2026-07-29 00:20:35 +02:00
enricobuehler	6de325a6b6	fix(ci): the unsafe lint said warn while CI enforced it as deny, and main went red ... `unsafe_op_in_unsafe_fn = "warn"` was adopted workspace-wide in 39513528 on the assumption that `warn` is a soft setting you can clear at leisure. It is not: ci.yml runs `cargo clippy … -D warnings`, which promotes it to a hard error, so main has failed on EVERY commit since — Linux `rust` and `rust-arm64` both dying on `pf-client-core` with 70 E0133 errors, and windows-host.yml alongside them. A lint level that understates its own severity is worse than a strict one, so this states what CI already does — `deny` — and writes the exemptions down instead. Fourteen GPU/FFI backend files take `#![allow(unsafe_op_in_unsafe_fn)]`, each with its reason and the workspace Cargo.toml carrying the argument in full. They are not "not done yet": measured across them, 64% of the sites are a single third-party FFI call (ash, pyrowave-sys, libav, the NVENC/AMF entry tables), and of the 44 `unsafe fn`s only 4 have a body containing no unsafe operation at all. Since pf-encode also denies `undocumented_unsafe_blocks`, narrowing them means a hand-written SAFETY comment per line that could only restate the signature — the exact noise that made `unsafe` stop meaning anything here before. Everything else stays at zero and enforced, and each allow is removable on its own terms. Two smaller things this had to clear, both invisible to the job that would have caught them: - `service.rs`: `undocumented_unsafe_blocks` wants the proof on EACH block, and a comment covering a group of consecutive `unsafe` statements only credits the first — so the two `OwnedHandle` wraps became their own statements. Windows-gated, so only the .47 gate sees it. 37a42078 fixed the same class of failure in pf-frame/dxgi.rs concurrently; that fix is taken as-is here. - `clients/linux/ui_settings.rs`: `show`'s deletion left its doc comment stranded on `show_scoped`, which silently merged two unrelated doc blocks onto one fn. Removed. Verified by running the real CI commands, not proxies: on .21 fmt + `clippy --workspace --all-targets -- -D warnings` + the feature-gated `-p pf-encode --features nvenc,vulkan-encode,pyrowave` + build + test, all rc=0; on the Intel box .47 all four windows-host.yml clippy invocations, all rc=0. The arm64 job lints pf-client-core and punktfunk-client-linux, both covered here.	2026-07-28 22:32:30 +02:00
enricobuehler	06a406adf2	refactor(client): the D3D11VA decoder owns its refs too, and the owner-only fields say so ... Completes the decoder trio. `D3d11vaDecoder::new` had the same cascading unwind as the VAAPI and Vulkan constructors — four branches each unref'ing the hwdevice by hand — and `d3d11va_decode_supported` opened a frames context it released on one exit path while relying on the null check for the other. Both are `AvBuffer` now. The one surviving `av_buffer_unref` in the file is deliberate: it runs before ownership is taken, on the `av_hwdevice_ctx_init` failure ahead of `from_raw`. The Windows gate then caught something worth keeping: `field hw_device is never read`, in all three decoders. It was true and it was new — the hand-written `Drop` used to be the field's only reader, so moving the unref into the type left a field that is *purely* an owner. The compiler cannot express that, so each one now carries a doc comment saying what it is and an explicit `#[allow(dead_code)]`. Deleting the field would free the device early; renaming it `_hw_device` would silence the lint by hiding what it holds. Neither is what we mean. Verified on BOTH platforms this time. Windows runner .133 (the toolchain env from the runner notes, plus `PF_FFVK_VULKAN_INCLUDE` for pf-ffvk): `cargo check -p pf-client-core --all-targets` at EXITCODE=0, clean of the dead-code warnings. Linux .21: `cargo check --workspace --all-targets` exit 0 / zero errors, pf-client-core 34 passed / 0 failed, pf-encode 33 passed / 0 failed.	2026-07-28 21:31:41 +02:00
enricobuehler	a42ab075a8	refactor(client): the hardware decoders own their hwdevice ref ... `VaapiDecoder::new` and `VulkanDecoder::new` create a hwdevice and then do several more fallible things with it — resolve `vkWaitSemaphores`, find the decoder, alloc the context, open it — and every one of those branches unref'd the device by hand, with a `Drop` doing it once more. Six hand-written unrefs between them, each one a line somebody has to remember when adding a step. `video_libav::AvBuffer` owns it instead, so an early `bail!` releases it and the branches carry nothing. It is a deliberate second copy of `pf-encode`'s type: the crates do not depend on each other (host encode and client decode share no code path), and this one needs something the host's does not — see below — so hoisting it to a shared crate would mean giving that crate an ffmpeg dependency and both sets of semantics to save about twenty lines. That extra piece is `into_raw`. `pick_vulkan` hands its frames context to the codec (`(*ctx).hw_frames_ctx = fr` — the codec unrefs it when the context closes), so the wrapper must give up ownership rather than drop. Making the transfer explicit is the point: dropping an `AvBuffer` there too would be exactly the double-unref this type exists to prevent. Two `av_buffer_unref` calls survive in `video_vulkan.rs` on purpose. One runs before ownership is taken (the `av_hwdevice_ctx_init` failure, ahead of `from_raw`); the other releases the codec's OWN pre-existing frames ctx before we replace it, which was never ours to model. Field order preserved: `hw_device` stays declared after `ctx`, so it still releases after each `Drop` frees packet/frame/context — the order the hand-written unref had. Verified on .21 (CachyOS, FFmpeg 62): `cargo check --workspace --all-targets` clean at exit 0 with zero errors — the workspace-wide check owed since the AvBuffer commit — plus pf-client-core 34 passed / 0 failed and pf-encode 33 passed / 0 failed. The decoders themselves still need real VAAPI/Vulkan playback to exercise.	2026-07-28 21:31:41 +02:00
enricobuehlerandClaude Opus 4.8	570ff504ad	refactor(client-core/W8): split video.rs into flat decoder-backend siblings ... Break the 1974-line pf-client-core/src/video.rs into flat sibling modules (matching the crate's video_d3d11.rs / video_pyrowave.rs convention), leaving video.rs as the contract + Decoder dispatch facade: - video_color.rs : ColorDesc + csc_rows (the Y'CbCr->RGB matrix) - video_software.rs : the libavcodec/swscale SoftwareDecoder - video_vaapi.rs : the Linux-only VAAPI/DRM-PRIME backend (mod is cfg(linux)) - video_vulkan.rs : the FFmpeg Vulkan Video backend Every crate::video::X / video::X path stays byte-stable (ColorDesc + csc_rows re-exported from video.rs; frame POD, VulkanDecodeDevice, QueueLock, Decoder, decodable_codecs*, ffmpeg_codec_id, fourcc/drm_fourcc_for all stay in video.rs). Code-driven placements: averr, AVERROR_EAGAIN, frame_is_keyframe stay in video.rs (shared by all three decoders); DrmFrameGuard's field + drm_fourcc_for + Software/Vaapi/VulkanDecoder ctors/decode became pub(crate) (sibling access); the test module split three ways (software tests need private decoder internals). Pure move; no behavior change. Verified on Linux (home-worker-5): cargo clippy -p pf-client-core (default [pyrowave] + --no-default-features, --all-targets -D warnings) + cargo test. Windows verify BLOCKED environmentally: pf-client-core -> sdl3 build-from-source -> CMake/CL.exe fails on winbox's non-ASCII home path (fails the baseline too, independent of this split); the split's Windows surface (facade cfg(windows) bits + video_d3d11) is verbatim-preserved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-17 14:06:57 +02:00