diff --git a/README.md b/README.md index 0d678c3..ae1c3da 100644 --- a/README.md +++ b/README.md @@ -88,13 +88,12 @@ default** and covers anything the hooked path doesn't. for the common case (engines render stereo float, matching the endpoint, differing only in rate). A game rendering a *different* channel count or bit depth than the device is mirrored with the wrong layout (garbled audio) on the hooked path, but never - an over-read/crash: a guessed stream is **captured but not silenced** (so it stays - audible locally — an echo), because zeroing it could over-*write* past the real buffer - (zeroing 8-channel-worth into a 2-channel buffer corrupts adjacent audio memory). Only - an **exact / override** format gets the no-echo silence (its frame size is known), so - the no-echo experience comes from an early (auto-attach) exact format or an operator - override. The loopback fallback is always format-correct. The Audio panel shows each - stream's + an over-read/crash: the capture copy is clamped to the readable region (`VirtualQuery`), + and the local **mute** is done with `AUDCLNT_BUFFERFLAGS_SILENT` (which makes WASAPI + ignore the buffer's contents), so it never *writes* the wrongly-sized buffer either. So + every captured stream is silenced locally — guessed or exact — and there is no echo on + the hooked path. The loopback fallback is always format-correct. The Audio panel shows + each stream's format provenance (*known* / *measuring* / *measured rate* / *low-confidence* / *override*) so the assumption is visible, and (under Debug details) lets the operator **re-measure** the rate or **override** the format when the guess is wrong. Overrides @@ -170,6 +169,14 @@ clang-cl driver resolves the MSVC / Windows SDK system includes on its own. ctest --test-dir build -C Debug --output-on-failure ``` +> **Fix bugs test-first.** Every bug fix must begin with a test that *reproduces* the bug +> and **fails** on the unfixed code — run it, watch it fail, and confirm it fails for the +> right reason. Only then write the fix, and confirm the same test now passes. A fix without +> a first-failing test is not done: the test is what proves the bug existed, that the change +> addresses it, and that it can't silently come back. (The hooked-audio double-play bug is the +> worked example: `audio_hook_test`'s guessed-path mute assertion was added and seen to fail +> before the one-line mute fix landed.) + - **`hook_selftest`** — in-process check of the IPC + XInput hook core (no game, no controller needed). - **`audio_ring_test`** — unit test of the shared audio ring (lock-free SPSC @@ -184,6 +191,16 @@ ctest --test-dir build -C Debug --output-on-failure - **`audio_overrides_test`** — unit test of the per-game audio override store (persist/reload, case-insensitive lookup by image name, and the differing-overwrite detection that drives the warning). No device. +- **`tone_analysis_test`** — unit test of the audio fidelity analyzer used by + `coop_audio_validate` (pitch error in cents, SNR/THD, click + dropout detection) and the WAV + reader/writer. Synthesizes a clean tone, a wrong-rate (pitch-shifted) tone, a tone with + injected clicks, and one with silence gaps, and asserts each metric matches what was injected + (e.g. 44100 played as 48000 → +147 cents). Pure header logic, no device. +- **`render_pacer_test`** — unit test of the mirror's render-feed pacing policy + (`host/src/audio/render_pacer.hpp`). Simulates a producer/consumer device timeline and asserts + the shipping `RenderPacer` rides producer jitter that makes the old re-prime-on-partial-fill + policy withhold available data ~168× and drain the buffer to the brink of silence (the + under-run / "metallic" bug `coop_audio_validate` found). No device. - **`audio_hook_test`** — in-process self-test of the WASAPI render-hook's **format detection**, the part that gets pitch right. Using a shared configurable `ToneSource` (the same render helper `coop_tone` uses), it renders tones at a matrix @@ -192,7 +209,9 @@ ctest --test-dir build -C Debug --output-on-failure code paths: **see-init** (hooks installed first → exact `Initialize` format) and **guess** (render client pre-exists → device-mix guess whose true rate is measured from the cadence, the Brotato/Godot case). Also checks the frames reached the ring - non-silent. Skips cleanly with no audio endpoint. + non-silent **and that the hook muted the game's local playback** (the no-echo guarantee) + for *both* paths — the guessed-path mute assertion is the regression guard for the + double-audio bug. Skips cleanly with no audio endpoint. - **`srgb_format_test`** — unit test of the `srgb_to_unorm` mapping the hooked video path uses so `*_SRGB`-backbuffer games aren't darkened. No device. - **`opengl_hook_test`** — in-process self-test of the OpenGL capture path: @@ -245,6 +264,19 @@ game, then drains the ring and prints per-stream format, captured-frame counts, peak amplitude (proves the audio is real, not silence), and overruns. It enables the hook's file trace (`%TEMP%\coop_hook.log`) for the run. +[`tools/audio_validate`](tools/audio_validate) (`coop_audio_validate.exe`) quantifies +audio-capture **fidelity** — it turns "the audio sounds slightly off" into numbers. With no +args it plays a known sine (`coop_tone` at 44100 Hz on a 48000 Hz endpoint, the Godot/Brotato +case), injects the hook exactly as the host does (late attach), captures the ring, and runs the +analyzer ([`common/include/coop/tone_analysis.hpp`](common/include/coop/tone_analysis.hpp)): +**pitch error in cents** (detects a mis-measured rate), **SNR/THD**, and **click / dropout** +counts. It also dumps a `.wav` so the capture can be *listened* to. Modes: `--render` drives the +real `AudioMirror` and measures its *rendered* output (surfaces the under-run / re-prime gaps the +capture side can't show — see Lessons learned); `--baseline` / `--selfcheck` establish the +measurement floor; `--listen ` passively records a live process's output (point it at a +running `coop_host` to hear/quantify exactly what a guest gets on a real game); `--wav ` +analyzes any recording. + [`tools/input_probe`](tools/input_probe) (`coop_input_probe.exe [seconds] [disable_mask]`) does the same for input: it injects, reports one connected pad, and toggles a button each second so the game's @@ -450,6 +482,36 @@ Non-obvious things that cost time and constrain the design: estimate as explicitly *low-confidence* (shown red). The operator can also re-measure or override the format via a per-stream `AudioRingHeader` op channel; the host rebuilds its render client when `format_generation` bumps, so it takes effect live. +- **Re-priming the render feed on a *partial* fill manufactures the gap it's avoiding.** The + mirror re-renders the captured ring to the output device. The original feed loop re-primed + (withheld the feed until ~30 ms had rebuffered) whenever it couldn't completely fill the free + buffer that tick (`to_write < avail`). But a partial fill is *normal* producer jitter — and + withholding the feed drains the device, so a one-frame ring dip became a full ~30 ms drop-out. + On a jittery game it fired constantly → choppy, "metallic" mirror audio, intermittent by + buffer phase. Fix: feed whatever is available every tick; re-prime **only** on a genuine + starvation (device buffer empty *and* ring empty). The policy is factored into a pure + `RenderPacer` reused by the loop and unit-tested headlessly (`render_pacer_test`: on a jittery + schedule the old policy withholds available data ~168× and drains the cushion to one period + from silence; the new one never withholds). **Diagnosing it needed device-side capture, not a + write-side tap** — the artifact is silence the device *plays* during an under-run, not bytes + the loop writes, so a tap on the write would look clean. `coop_audio_validate` quantifies it + by loopback-capturing the *rendered* output (pitch error in cents, SNR, click/dropout counts) + and contrasts the capture ring (pristine), the measurement floor, and the render path. +- **Mute the game with the SILENT flag, not a `memset` — they are not the same thing.** The + hooked path's whole point is "no echo": capture the game's frames into the ring AND silence + its local playback, so the only audio is the host's re-render. The mute was implemented by + *zeroing the buffer* and releasing it with `AUDCLNT_BUFFERFLAGS_SILENT`. But zeroing + `num_frames * block` is only safe when `block` is the real frame size — for a **guessed** + format (late attach, the Brotato case) the guessed block can exceed the real buffer, so the + zero would over-write adjacent memory. The original code's conservative answer was to skip the + whole mute for guessed streams — which left the game **audible**: it played locally *and* the + mirror re-rendered the same audio a few ms later = a metallic, slightly-out-of-sync double + (exactly the reported symptom). The fix: `AUDCLNT_BUFFERFLAGS_SILENT` already makes WASAPI + *ignore the buffer contents* and play silence — it mutes with **no write at all**, so it's safe + for any format. Decouple the two: always mute via the flag; keep the `memset` only for an exact + format (belt-and-suspenders). `audio_hook_test` now asserts the mute engages for both the + exact and the guessed path (the guessed assertion fails on the old code — write the failing + test first), and the byte-incompatible cases prove the flag-mute never over-writes. - **Panels must *fit* their assigned size at max info -- measure it, don't eyeball it.** The overlay opens panels at fixed sizes that scale with the monitor, so with Debug details on a dense panel can overflow and scroll content out of view. `ui_fit_test` drives the real diff --git a/hook/src/audio_hook.cpp b/hook/src/audio_hook.cpp index 17e3d29..aa2b5c7 100644 --- a/hook/src/audio_hook.cpp +++ b/hook/src/audio_hook.cpp @@ -208,6 +208,7 @@ std::uint32_t g_registered = 0; // slots filled (<= kMaxAudioStreams) std::atomic g_streams_seen{0}; // total distinct clients ever seen std::atomic g_frames_captured{0}; // total frames captured across streams +std::atomic g_frames_silenced{0}; // total frames whose local playback we muted (SILENT) // When we attach to an already-running game we never saw its IAudioClient::Initialize, // so a render client discovered on the hot path gets the device mix format as a best @@ -368,18 +369,25 @@ HRESULT STDMETHODCALLTYPE hk_ReleaseBuffer(IAudioRenderClient* self, UINT32 num_ if (block != 0 && audio_ring_push(*ring, t_gb_data, bytes, num_frames)) { g_frames_captured.fetch_add(num_frames, std::memory_order_relaxed); - // Only silence (zero the buffer) for an EXACT/override format, where `block` - // is the real frame size so the memset stays in-bounds. For a guessed format - // the block can exceed the real buffer, so zeroing it would over-WRITE into - // adjacent audio memory (an intermittent crash the stress test caught) -- so we - // capture but leave the game audible (echo). The no-echo path is reached via an - // exact format (auto-attach early) or an operator override. + // Mute the game's local playback so the only audio is the host's re-render. + // Otherwise the game plays locally AND the mirror re-renders the same audio a + // few ms later = a metallic double (the Brotato symptom). AUDCLNT_BUFFERFLAGS_SILENT + // tells WASAPI to treat the buffer as silence and IGNORE its contents, so it + // mutes WITHOUT writing the buffer -- safe even for a guessed-format stream whose + // true frame size we don't know. (Muting used to be tied to the memset below, + // which is unsafe for a guessed block, so guessed streams -- the late-attach / + // Brotato case -- were captured but left audible. The flag is the actual mute; + // the memset is not needed for it.) For an exact/override format we additionally + // zero the buffer (belt-and-suspenders; `block` is the real frame size there, so + // it stays in-bounds). Only mutes once the frames made the ring (above) -- a + // stalled host degrades to echo, never to dead silence. if (!guessed) { std::memset(t_gb_data, 0, bytes); - return g_vh_releasebuffer.original()( - self, num_frames, flags | AUDCLNT_BUFFERFLAGS_SILENT); } + g_frames_silenced.fetch_add(num_frames, std::memory_order_relaxed); + return g_vh_releasebuffer.original()( + self, num_frames, flags | AUDCLNT_BUFFERFLAGS_SILENT); } } } @@ -898,6 +906,7 @@ void remove_audio_hooks() g_registered = 0; g_streams_seen.store(0, std::memory_order_relaxed); g_frames_captured.store(0, std::memory_order_relaxed); + g_frames_silenced.store(0, std::memory_order_relaxed); for (std::uint32_t i = 0; i < kMaxAudioStreams; ++i) { g_streams[i].client.store(nullptr, std::memory_order_relaxed); @@ -937,6 +946,11 @@ void shutdown_audio_hooks() g_have_mix_format.store(0, std::memory_order_relaxed); } +std::uint64_t audio_frames_silenced() +{ + return g_frames_silenced.load(std::memory_order_relaxed); +} + std::uint64_t audio_frames_captured() { return g_frames_captured.load(std::memory_order_relaxed); diff --git a/hook/src/audio_hook.hpp b/hook/src/audio_hook.hpp index c195c8e..22a52fb 100644 --- a/hook/src/audio_hook.hpp +++ b/hook/src/audio_hook.hpp @@ -47,6 +47,11 @@ void shutdown_audio_hooks(); // Cumulative frames the primary path copied to the ring and silenced locally. std::uint64_t audio_frames_captured(); +// Cumulative frames whose local playback the hook muted (released to WASAPI with +// AUDCLNT_BUFFERFLAGS_SILENT). The "no echo" guarantee: this must advance for every +// captured stream, including a guessed-format (late-attach) one. Used by the self-test. +std::uint64_t audio_frames_silenced(); + // Distinct render streams ever observed (may exceed kMaxAudioStreams). std::uint32_t audio_streams_seen(); diff --git a/tests/audio_hook_test.cpp b/tests/audio_hook_test.cpp index 53e8768..ade65e3 100644 --- a/tests/audio_hook_test.cpp +++ b/tests/audio_hook_test.cpp @@ -101,6 +101,7 @@ void test_see_init(hook::IpcClient& ipc, AudioRingHeader* ring, SharedBlock* blo const ToneFormat& f = tone.format(); // Exact format publishes at registration; render briefly so capture fills the ring. + const std::uint64_t silenced_before = hook::audio_frames_silenced(); const DWORD end = GetTickCount() + 300; while (GetTickCount() < end) { @@ -116,6 +117,8 @@ void test_see_init(hook::IpcClient& ipc, AudioRingHeader* ring, SharedBlock* blo fail += expect(s.sample_rate == f.rate, "see-init: HookStatus rate == exact rate"); fail += expect(s.format_state == AudioFormat_Exact, "see-init: provenance == Exact"); fail += expect(ring_has_nonsilent(ring), "see-init: non-silent audio captured"); + fail += expect(hook::audio_frames_silenced() > silenced_before, + "see-init: local playback muted (no echo)"); std::printf(" %s see-init %s -> ring %uHz/%uch/%ubit state=%u\n", fail == 0 ? "PASS" : "FAIL", fmt_desc(f, d, sizeof(d)), ring->sample_rate, ring->channels, ring->bits, s.format_state); @@ -165,6 +168,8 @@ void test_guess(hook::IpcClient& ipc, AudioRingHeader* ring, SharedBlock* block, tone.render_step(30); hook::republish_audio_format(); } + // Format is published; from here the hook must capture AND mute (the no-echo path). + const std::uint64_t silenced_before = hook::audio_frames_silenced(); const DWORD cap_end = GetTickCount() + 200; while (GetTickCount() < cap_end) { @@ -178,6 +183,11 @@ void test_guess(hook::IpcClient& ipc, AudioRingHeader* ring, SharedBlock* block, fail += expect(s.sample_rate == f.rate, "guess: HookStatus rate == rendered rate"); fail += expect(s.format_state == AudioFormat_Measured, "guess: provenance == Measured"); fail += expect(ring_has_nonsilent(ring), "guess: non-silent audio captured"); + // THE BROTATO BUG: a guessed (late-attach) stream is byte-compatible here (device + // channels/bits), so the hook must mute the game's local playback too -- otherwise the + // game plays locally AND the mirror re-renders it, slightly delayed = a metallic double. + fail += expect(hook::audio_frames_silenced() > silenced_before, + "guess: local playback muted (no echo) -- the Brotato double-audio bug"); std::printf(" %s guess %u Hz (device %uch/%ubit) -> measured %uHz state=%u\n", fail == 0 ? "PASS" : "FAIL", rate, f.channels, f.bits, ring->sample_rate, s.format_state);