diff --git a/README.md b/README.md index 4f5b344..e7139b3 100644 --- a/README.md +++ b/README.md @@ -114,9 +114,6 @@ From an in-depth review pass. Each item is fixed test-first (a failing test, the as its own commit; "verify" items are confirmed real before any change, and dropped if not. Code quality: -- **Stale comments** — `hook_guard.hpp` top block (still the old destroy-on-remove model), - `input_source.hpp` ("future" Steam source), `audio_ring.hpp` (v1/reserved), `shared_memory.hpp` / - `audio_ring.hpp` ("20-byte pads"). - **Consolidate `VtableHook`** — remove the duplicate copy in `audio_hook.cpp`. ### Future work diff --git a/common/include/coop/audio_ring.hpp b/common/include/coop/audio_ring.hpp index 4fd3b47..e7fc65a 100644 --- a/common/include/coop/audio_ring.hpp +++ b/common/include/coop/audio_ring.hpp @@ -2,9 +2,9 @@ // // The injected hook (coop_hook.dll) captures the game's WASAPI render frames and // is the sole *producer*; the host (coop_host.exe) is the sole *consumer* and -// re-renders the frames for Steam Remote Play Together. This is a separate, -// larger mapping from the input/status SharedBlock (which is only 20-byte pads -// and can't hold PCM): a header followed by a byte ring of `capacity` bytes. +// re-renders the frames for Steam Remote Play Together. This is a separate, larger mapping from the +// input/status SharedBlock (which holds fixed-size POD state, not bulk PCM): a header followed by a +// byte ring of `capacity` bytes. // // Lock-free SPSC with free-running 64-bit positions (release on publish, acquire // on read) — the same cross-process atomic model as the input seqlock. POD and @@ -58,9 +58,9 @@ struct AudioRingHeader // (stream counting in HookStatus still runs regardless of this flag). std::atomic capture_enabled; - // Producer publishes the captured stream's format once, then sets - // format_valid=1 (release). format_generation is reserved so a future - // mid-session device re-init can be made forward-compatible; v1 sets once. + // Producer publishes the captured stream's format, then sets format_valid=1 (release). + // format_generation bumps on every (re)publish (audio_ring_set_format), so the host can notice a + // mid-session format change (a device re-init, or a measured-rate / override update). std::atomic format_valid; std::atomic format_generation; @@ -92,8 +92,8 @@ struct AudioRingHeader // capture both the hook (pre-mix) and a parallel process-loopback (post-mix) of the same audio // and cross-correlate them to recover the true sample rate (and, in step b, channels/bit-depth) // from ground truth instead of guessing. Inert (0) by default -- normal capture is unaffected, - // so it never changes the shipping no-echo path. Repurposed from `reserved`, so the layout and - // size are unchanged (old builds saw it as a zero reserved byte). + // so it never changes the shipping no-echo path. It's a 4-byte atomic carved out of the header's + // reserved space; the version gate (kAudioRingVersion) rejects any layout that doesn't match. std::atomic verify_capture; std::uint8_t reserved[36]; diff --git a/hook/src/hook_guard.hpp b/hook/src/hook_guard.hpp index 0dd5bb8..9fb43aa 100644 --- a/hook/src/hook_guard.hpp +++ b/hook/src/hook_guard.hpp @@ -7,13 +7,14 @@ // use-after-free -> the game crashes (the "spamming Mirror video crashed Brotato" bug). // // The fix mirrors the audio hooks' epoch+drain pattern, generalised for inline hooks: -// 1. Restore/disable the hook FIRST so no NEW detour can start. For a SafetyHook inline hook -// that's `hook = {}` (reset): it restores the original bytes under thread suspension, and -// its mutex-guarded call wrappers make any in-flight trampoline call safe. For a hook the -// game reaches by a cached pointer (Vulkan present, the WNDPROC subclass) it's clearing an -// atomic gate / restoring the window proc. -// 2. drain() -- wait (bounded) for detour BODIES already running to finish, since the reset -// above does NOT wait for the part of the detour that runs before it calls the trampoline. +// 1. Disable the hook FIRST so no NEW detour can start. For a SafetyHook inline hook that's +// `disable_for_removal(hook)` (disable, NOT `= {}` destroy): it restores the original bytes but +// keeps the trampoline alive -- hooks are PERSISTENT, never destroyed mid-session, so an in-flight +// detour about to call the trampoline never finds it freed (see hook_install.hpp). For a hook the +// game reaches by a cached pointer (Vulkan present, the WNDPROC subclass) it's clearing an atomic +// gate / restoring the window proc instead. +// 2. drain() -- wait (bounded) for detour BODIES already running to finish, since the disable above +// does NOT wait for the part of the detour that runs before it calls the trampoline. // 3. Only THEN free the shared state the detour was reading. // // Each detour wraps its whole body in a DetourGate::Guard (an RAII active-count). drain() spins diff --git a/host/src/audio/audio_format_verifier.cpp b/host/src/audio/audio_format_verifier.cpp index 51ce5f1..5085ead 100644 --- a/host/src/audio/audio_format_verifier.cpp +++ b/host/src/audio/audio_format_verifier.cpp @@ -132,7 +132,6 @@ ChunkedCapture parse_chunks(const std::vector& raw, unsigned stride) FormatVerification verify_stream_format(DWORD pid, AudioRingHeader* ring, unsigned window_ms, bool recover_layout) { - (void)recover_layout; // step (b) extends this; step (a) recovers the rate only FormatVerification result; if (ring == nullptr) { diff --git a/host/src/input/input_source.hpp b/host/src/input/input_source.hpp index 157ea13..145d414 100644 --- a/host/src/input/input_source.hpp +++ b/host/src/input/input_source.hpp @@ -1,9 +1,10 @@ // Abstraction over "where controller input comes from" on the host side. // -// Phase 0/1 use XInputSource: Remote Play Together delivers each guest's gamepad -// to the focused window (our host) as a virtual XInput controller, so reading -// XInput is enough to see guests. A future SteamInputSource can implement this -// same interface for cleaner per-guest handles once the Steamworks SDK is wired. +// XInputSource is the primary/default: Remote Play Together delivers each guest's gamepad to the +// focused window (our host) as a virtual XInput controller, so reading XInput is enough to see guests. +// SteamInputSource (steam_input_source.cpp) implements this same interface over Steam Input when the +// Steamworks SDK is present; it's opt-in (initializing Steam Input suppresses XInput -- see the +// Controllers panel and Lessons learned). #pragma once #include