diff --git a/README.md b/README.md index 21e5bc5..8a952c4 100644 --- a/README.md +++ b/README.md @@ -113,10 +113,6 @@ default** and covers anything the hooked path doesn't. From an in-depth review pass. Each item is fixed test-first (a failing test, then the fix) and lands as its own commit; "verify" items are confirmed real before any change, and dropped if not. -Correctness (verify, then fix if real): -- **Non-atomic cross-process diagnostic counters** — `present_calls`, `frames_dropped`, - `frames_rendered` are plain `+=`/stores read by the host. Make them atomic. - Performance (failing perf-regression test first): - **D3D9 capture readback off the present thread** — move the swizzle/flip to an off-thread reaper (the inline-readback pattern that caused the Vulkan 144→3 FPS stall), add a present-thread overhead diff --git a/common/include/coop/protocol.hpp b/common/include/coop/protocol.hpp index b0e283a..cf3658b 100644 --- a/common/include/coop/protocol.hpp +++ b/common/include/coop/protocol.hpp @@ -47,10 +47,11 @@ static_assert(sizeof(CoopPadState) == 20, "CoopPadState layout must stay stable // first ("primary"); the rest are surfaced so a multi-stream game is visible. inline constexpr std::uint32_t kMaxAudioStreams = 4; -// One render stream the hook observed, for the Audio panel's debug view. Plain -// POD (no atomics): diagnostics tolerate benign cross-process races like the -// other HookStatus counters. frames_rendered is cumulative; the host derives -// "live vs idle" from successive deltas. +// One render stream the hook observed, for the Audio panel's debug view. Plain POD so it stays +// trivially copyable (it's published/read as a whole struct). frames_rendered is cumulative and +// updated per audio buffer, so it's read/written via std::atomic_ref at its hot sites to avoid a +// torn cross-process read (the other fields change rarely, at stream discovery). The host derives +// "live vs idle" from successive frames_rendered deltas. // How confidently the hook knows a render stream's format. A stream that existed before // we injected (the common case) was never seen at Initialize, so its format starts as a // guess (the device mix format) and its true sample rate is measured from the render @@ -191,6 +192,9 @@ struct VideoShare std::uint64_t frames_dropped; // cumulative captures skipped because the shared // keyed mutex was busy (host mid-copy) -- a frame // the game produced that never reached the mirror + // present_calls / frames_dropped stay plain uint64_t (POD layout) but are read/written via + // std::atomic_ref so the host's cross-process read isn't torn (an x86 DLL stores 64 bits in two + // halves). Kept as fields, not std::atomic, only so the layout/offset asserts stay simple. }; // --- Mouse + keyboard forwarding ------------------------------------------- diff --git a/hook/src/ipc_client.hpp b/hook/src/ipc_client.hpp index 48236d3..c582c9a 100644 --- a/hook/src/ipc_client.hpp +++ b/hook/src/ipc_client.hpp @@ -200,7 +200,10 @@ public: { if (block_ != nullptr && slot < kMaxAudioStreams) { - block_->status.audio_streams[slot].frames_rendered = frames; + // atomic_ref so the host's cross-process read isn't torn (notably an x86 DLL -> x64 host, + // where a plain 64-bit store is two halves). The field stays plain POD so AudioStreamInfo + // remains trivially copyable for the wholesale publishes elsewhere. + std::atomic_ref(block_->status.audio_streams[slot].frames_rendered).store(frames, std::memory_order_relaxed); } } @@ -211,7 +214,7 @@ public: { if (block_ != nullptr) { - block_->video.present_calls += 1; + std::atomic_ref(block_->video.present_calls).fetch_add(1, std::memory_order_relaxed); } } @@ -221,7 +224,7 @@ public: { if (block_ != nullptr) { - block_->video.frames_dropped += 1; + std::atomic_ref(block_->video.frames_dropped).fetch_add(1, std::memory_order_relaxed); } } diff --git a/host/src/ipc/ipc_server.cpp b/host/src/ipc/ipc_server.cpp index b98a430..9796fe0 100644 --- a/host/src/ipc/ipc_server.cpp +++ b/host/src/ipc/ipc_server.cpp @@ -1,8 +1,22 @@ #include "ipc/ipc_server.hpp" +#include +#include + namespace coop { +namespace +{ +// The hook writes these cumulative diagnostic counters cross-process (an x86 DLL can do a 64-bit +// store in two halves), so read them atomically to avoid a torn value. The shared mapping is +// genuinely mutable -- the const here is just our read-only view -- so const_cast for atomic_ref. +std::uint64_t atomic_load_u64(const std::uint64_t& field) +{ + return std::atomic_ref(const_cast(field)).load(std::memory_order_relaxed); +} +} // namespace + bool IpcServer::start(unsigned long target_pid) { std::scoped_lock lock(mutex_); @@ -83,6 +97,7 @@ HookStatusView IpcServer::hook_status() const for (std::uint32_t i = 0; i < kMaxAudioStreams; ++i) { view.audio_streams[i] = s.audio_streams[i]; + view.audio_streams[i].frames_rendered = atomic_load_u64(s.audio_streams[i].frames_rendered); } view.hook_entry_count = s.hook_entry_count; for (std::uint32_t i = 0; i < kMaxHookEntries; ++i) @@ -111,9 +126,9 @@ VideoShareView IpcServer::video_share() const v.width = s.width; v.height = s.height; v.format = s.format; - v.present_calls = s.present_calls; + v.present_calls = atomic_load_u64(s.present_calls); v.present_qpc = s.present_qpc; - v.frames_dropped = s.frames_dropped; + v.frames_dropped = atomic_load_u64(s.frames_dropped); return v; }