Fix Brotato hooked-audio double-play: mute guessed streams via SILENT flag

The hooked path must capture the game's frames AND mute its local playback
("no echo"). The mute was implemented as zero-the-buffer (memset) + release
with AUDCLNT_BUFFERFLAGS_SILENT. Zeroing num_frames*block is only safe when
block is the real frame size; for a guessed format (late attach -- the
Brotato case, where we never saw Initialize) the guessed block can exceed
the real buffer, so the conservative code skipped the whole mute for guessed
streams. That left the game audible: it played locally AND the mirror
re-rendered the same audio a few ms later = a metallic, out-of-sync double.

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/override format (belt-and-suspenders). One-line behavior change;
the byte-incompatible cases confirm the flag-mute never over-writes.

Test-first (now a documented rule, README "Tests"): added an
audio_frames_silenced() counter and a mute assertion to audio_hook_test for
both the exact and guessed paths. The guessed assertion FAILS on the unfixed
code (3 rates) and passes after the fix -- the regression guard for this bug.
README also updates the now-correct no-echo limitation and adds a
lessons-learned writeup.

ctest 17/17. Brotato confirmed fixed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

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 <pid>` 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 <file>`
+analyzes any recording.
+
 [`tools/input_probe`](tools/input_probe)
 (`coop_input_probe.exe <pid> [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

hook/src/audio_hook.cpp

@@ -208,6 +208,7 @@ std::uint32_t g_registered = 0;            // slots filled (<= kMaxAudioStreams)
 std::atomic<std::uint32_t> g_streams_seen{0}; // total distinct clients ever seen
 
 std::atomic<std::uint64_t> g_frames_captured{0}; // total frames captured across streams
+std::atomic<std::uint64_t> 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,21 +369,28 @@ 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);
+					}
+					g_frames_silenced.fetch_add(num_frames, std::memory_order_relaxed);
 					return g_vh_releasebuffer.original<ReleaseBufferFn>()(
 					    self, num_frames, flags | AUDCLNT_BUFFERFLAGS_SILENT);
 				}
 			}
 		}
-		}
 		break;
 	}
 	return g_vh_releasebuffer.original<ReleaseBufferFn>()(self, num_frames, flags);
@@ -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);

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();
 

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);