Read/write the cross-process diagnostic counters atomically
present_calls, frames_dropped (VideoShare) and frames_rendered (AudioStreamInfo) were plain `+= 1` / stores in the DLL, read by the host cross-process. On an x86 DLL a 64-bit store is two halves, so the x64 host could read a torn value during a carry. Benign (display-only), but a real data race. Use std::atomic_ref at the access sites rather than changing the field types: the structs stay plain POD so the layout/offset asserts are unchanged and AudioStreamInfo stays trivially copyable (it's published/read as a whole struct). The DLL writers (note_present / note_video_dropped / note_audio_frames) and the host readers (IpcServer::video_share / hook_status) now use relaxed atomic_ref; hook_status reloads frames_rendered atomically after the wholesale struct copy. The dev-tool readers (vk_validate, audio_probe) keep plain reads -- diagnostics of diagnostics, and same-bitness in practice. Validated by present_hook_test (present_calls via atomic_ref) and audio_hook_test (frames_rendered) -- also confirms no atomic_ref alignment fault. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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 -------------------------------------------
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -1,8 +1,22 @@
|
||||
#include "ipc/ipc_server.hpp"
|
||||
|
||||
#include <atomic>
|
||||
#include <cstdint>
|
||||
|
||||
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<std::uint64_t&>(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;
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user