Repository navigation
test(audio-quality): record real arrival traces and grade their replay per PR - #4525
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Records three arrival traces in headless Chromium (the public relay's bbb.hang, a local relay's AAC tone, and a browser publisher through the public relay), stamping each frame as it comes off its group's stream. The harness replays them through the player's real rings on a simulated clock at the delay Sync resolves, and grades them as exact replay rows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…EADME' into quest/m0/audio-quality-harness/traces
|
Outcome, from the background quest agent. It could not ask questions, so it is leaving this as a draft. Open decisions
Suggested follow-ups
(Written by Claude Opus 5.5) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ae5051fa1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const arrivals = trace.arrivals.map(([at, timestamp]) => ({ at, timestamp })); | ||
| for (const quantum of replay(arrivals, { ring: row.ring === "isolated" ? "shared" : "post", rate, delay })) { |
There was a problem hiding this comment.
When groups arrive out of order, as they do after warmup in every checked-in trace, this mapping discards the group sequence and replay() inserts each frame directly into the ring. The production decoder instead passes groups through Container.Consumer, which orders them and applies the delay-derived maxAge skip logic. Consequently these budgets grade raw timestamped ring writes rather than the documented player behavior, so they can report a pass while changes to group waiting, skipping, or discontinuity handling regress real playback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 69c1430, per the maintainer's call (replay scope: through the consumer). replay() now writes each arrival into its group on a real Track.Producer and reads through Container.Consumer at the delay's max age, so group ordering, holds, max-age skips, and discontinuity ring resets are the player's own. A new test drops one group and asserts the ring runs dry while the later groups wait; the ring-only replay played straight through it. local-aac's budgets moved with it (the PR body says why).
(Written by Claude Opus 5.5)
| for profile in "${PROFILES[@]}"; do | ||
| if [[ " ${ALL_TRACES[*]} " == *" $profile "* ]]; then | ||
| TRACES+=("$profile") | ||
| continue |
There was a problem hiding this comment.
When every selected profile is moved into TRACES here, ROWS is empty, but the script still requires cargo and ffmpeg, builds three Rust binaries and the Vite pages, installs Chromium, starts a relay, and launches publishers before reaching the replay section. Thus the newly documented replay-only command can fail on missing browser/live-test tooling and is not the seconds-long deterministic path it advertises; gate the live setup and publishers on ROWS being nonempty.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 69c1430. With no browser rows, run.sh only needs bun: it installs the workspace and replays, skipping cargo, ffmpeg, Playwright, the page build, the relay, and the publishers. --replays selects every trace.
(Written by Claude Opus 5.5)
| delay: input.delay, | ||
| probe: input.rtt === undefined ? undefined : { rtt: Time.Milli(input.rtt) }, | ||
| }); | ||
| const unregister = sync.register(new Signal<Time.Milli | undefined>(playbackJitter(input.config))); |
There was a problem hiding this comment.
The new target unit tests always pass through this real setTimeout(0), making completion depend on the host scheduler instead of deterministic effect flushing. Replace it with a mocked/scoped clock or a deterministic signal flush so the test does not wait on wall-clock timing, as required for unit tests in this repository.
AGENTS.md reference: AGENTS.md:L18-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 69c1430. target() now awaits sync.out.delay.changed() until the delay equals the resolved jitter plus the registered rendition's, with no timer. The replay itself uses setImmediate as a microtask barrier after writing arrivals: nothing on the consumer path arms a timer, so it orders the replay behind the consumer rather than waiting on time, and repeated runs write byte-identical output.
(Written by Claude Opus 5.5)
Replays run in seconds without a browser, so they gate PRs while the rest of the matrix stays nightly. The watch quest's manual run re-records relay-mic with a real microphone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: caa7635a3d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }).then((first) => arrivals.push(...first)); | ||
|
|
||
| const start = arrivals[0][0]; | ||
| while ((arrivals.at(-1)?.[0] ?? start) < start + durationMs) { |
There was a problem hiding this comment.
When a broadcast stops producing frames after recording begins but leaves the track open, arrivals.at(-1) never advances, so this loop runs forever even though --duration has elapsed. Temporary outages also make the command wait until the next frame rather than preserving the outage at the end of the trace. Base termination on elapsed wall-clock time from the first arrival so a silent interval is recorded instead of hanging the recorder.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 69c1430. The loop now ends on the page's performance.now(), the arrivals' own clock, and drains once more after it, so a quiet publisher is recorded as silence instead of holding the recording open.
(Written by Claude Opus 5.5)
| .sort() | ||
| .map((f) => ({ profile: basename(f, ".json"), trace: JSON.parse(readFileSync(join(dir, f), "utf8")) as Trace })); | ||
| const profiles = values.profiles === undefined ? undefined : split(values.profiles); | ||
| const rings = split(values.rings) as Ring[]; |
There was a problem hiding this comment.
When replay.ts is invoked directly with a misspelled --rings value, this cast accepts it and emits a row bearing that invalid ring name, while line 98 silently runs the postMessage ring for every value other than isolated. For example, --rings typo --list succeeds and a normal run produces mislabeled results before analysis eventually rejects the row key. Validate selectors against isolated and plain at argument parsing time.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 69c1430. replay.ts rejects an unknown ring or codec at parse time and exits 2.
(Written by Claude Opus 5.5)
…on them The replay now writes each recorded arrival into its group and lets the player's Container.Consumer order, hold, and skip groups at the delay's max age, resetting the ring on a discontinuity as the decoder does. local-aac's budgets move with it: its relay flushes groups newest-first, so the consumer holds a burst behind its oldest frame and the ring lands one quantum later. target() waits on the delay signal instead of a timer, run.sh skips the browser, relay, and publishers when only replays run (--replays), the recorder stops on the page clock, and replay.ts rejects unknown rings. .github/workflows/audio-quality.yml grades the replays on PRs touching js/watch, js/hang, or the harness. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69c14304d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let next = 0; | ||
|
|
||
| try { | ||
| for (let n = 1; first.at + (n - 1) * step <= last.at; n++) { |
There was a problem hiding this comment.
When a publisher stops before the requested duration, record.ts now continues polling, but this loop still stops at last.at, and the serialized Trace contains no requested end time. The trailing outage therefore vanishes from replay and from the silence and stall budgets instead of being graded. This is fresh evidence beyond the earlier recorder finding: wall-clock capture was added, but its end is never passed to replay().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving this as is. After the last arrival nothing more is delivered, so every quantum past it underruns no matter what the player decides. Grading that tail would score the publisher stopping, not the consumer or the rings. Outages in the middle of a trace stay in, because later arrivals bound them. A requested end would also mean a new Trace field and a re-baseline for a tail that no checked-in trace has (each ends within about 150 ms of its 35 s).
(Written by Claude Opus 5.5)
There was a problem hiding this comment.
The maintainer decided to leave this for a follow-up: grading the silent tail from the recording's requested end. It's listed under Follow-ups in the PR description.
(Written by Claude Opus 5.5)
| - "js/watch/**" | ||
| # The container consumer decides what a replay delivers, waits for, and skips. | ||
| - "js/hang/**" |
There was a problem hiding this comment.
The path filter covers js/watch and js/hang but not js/net or js/signals. A PR that only changes Group/Track scheduling or signal effect delivery can change replay results, yet this workflow is skipped; these are direct runtime dependencies because replay.ts imports Group, Track, Time, and Signal. Include those paths so deterministic budget regressions block the affected PR rather than waiting for the nightly run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e7334db: the workflow also runs on js/net/** and js/signals/**.
(Written by Claude Opus 5.5)
| const durationMs = Number.parseFloat(values.duration) * 1000; | ||
| if (!pageDir || !url || !broadcast || !source || !description || !out || !(durationMs > 0)) { |
There was a problem hiding this comment.
When invoked with --duration Infinity, Number.parseFloat returns infinity and this validation accepts it, so the wall-clock recording loop never terminates; prefixes such as 35oops are also silently accepted as 35 seconds. Validate the complete value and require it to be finite and positive so malformed manual recording commands fail immediately.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e7334db: --duration is parsed with Number and must be finite and positive, so Infinity and 35oops are rejected.
(Written by Claude Opus 5.5)
…e recording duration Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7334dba23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const { profile, trace } of traces) { | ||
| if (trace.version !== 1) throw new Error(`${profile}: trace version ${trace.version}, expected 1`); | ||
| const codec = traceCodec(trace); | ||
| if (profiles && !profiles.includes(profile)) continue; |
There was a problem hiding this comment.
When this CLI is invoked directly with a misspelled selector such as --profiles relay-bb, every trace is skipped here and the command exits successfully without listing or writing any rows. Validate each requested profile against the loaded trace names so a typo cannot silently turn a targeted replay into a no-op.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not fixing this here. run.sh already rejects an unknown profile before it calls replay.ts (the valid check against the trace names), so this only affects running replay.ts by hand. It's cheap to add if the maintainer wants it.
(Written by Claude Opus 5.5)
There was a problem hiding this comment.
Fixed in d360b87, per the maintainer: replay.ts now refuses a profile that matches no trace and exits 2, the same way it handles rings and codecs.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…EADME' into land-4525
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3004f8a3be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const traces = readdirSync(dir) | ||
| .filter((f) => f.endsWith(".json")) | ||
| .sort() | ||
| .map((f) => ({ profile: basename(f, ".json"), trace: JSON.parse(readFileSync(join(dir, f), "utf8")) as Trace })); |
There was a problem hiding this comment.
When --traces contains a malformed trace whose config.sampleRate is missing or nonnumeric, this unchecked cast passes it to playbackJitter, producing NaN. The equality on target() line 74 can then never succeed, and after the delay signal settles, changed() never resolves, so the replay hangs until the 20-minute workflow timeout instead of rejecting the trace. Validate that the sample rate is finite and positive while loading each trace.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
0f024e3
into
quest/m0/audio-quality-harness/README
1 participant
Records real audio arrival traces, checks them in, and grades a replay of each next to the harness's synthetic profiles, on every PR that touches the player. Completes
quest/m0/audio-quality-harness/traces.mdandreplay-ci.md.Problem
The harness's shaper profiles are synthetic. The player needs grading against real arrival shapes (the #3477 flush-span bursts), and those traces are gone.
Approach
test/audio-quality/clients/js/record.ts,src/recorder.ts): headless Chromium subscribes over WebTransport and stamps each audio frame as it comes off its group's stream, which is whereContainer.Consumerreceives it. It uses only public@moq/net+@moq/hang, and stops on the page clock, so a quiet publisher is recorded as silence.test/audio-quality/traces/, 35 s each, recorded 2026-09-29 withjust test audio-quality-record):relay-bbb:cdn.moq.dev/demobbb.hang. One AAC frame per group, flushed in bursts of about 7 (p95 spread 157 ms).local-aac: the harness's AAC publisher through a local relay, the shallow control.relay-mic:<moq-publish>publishing Chromium's fake capture device throughcdn.moq.dev/anon. It includes a real 390 ms stall.js/watch/src/audio/replay.ts,@internal): writes each recorded arrival into its group on a realTrack.Producerand reads it through the player'sContainer.Consumerat the delay's max age, on a simulated clock. The consumer's group ordering, holds, max-age skips, and discontinuities are the player's own; a discontinuity resets the ring as the decoder does. Delivered frames go into the realSharedRingBuffer/AudioRingBuffer, sized the way the decoder sizes them. The delay comes from a realSyncfed the recorded catalog config and PROBE RTT.target()waits on the delay signal, and the replay orders itself behind the consumer with a microtask barrier, so neither waits on time.replay-<codec>-<rate>-<trace>-<ring>row (6 rows, about 2 s).just test audio-quality --replaysruns only those and needs only bun: no cargo, ffmpeg, browser, relay, or publishers. Their budgets are exactly what was measured, with no headroom..github/workflows/audio-quality.ymlruns--replays --enforceon PRs touchingjs/watch,js/hang,js/net,js/signals,test/audio-quality,test/lib, ortest/justfile. Nightly still runs the full matrix.Current replay grades at the "auto" delay:
Why local-aac moved (from 101.7 underruns/min, 287.3 gap ms/min, 0.004 silence to 103.7, 293.2, 0.005): the local relay delivers each burst newest-group-first (for example 498, 502, 501, 500, 499, then 497). The ring-only replay played the newer frames while 497 was still missing, which let the playhead step past 497's start. The consumer holds the burst until 497 arrives, so the ring runs dry and waits instead. Playback then lands about 3 ms later, and a later gap goes one quantum deeper. relay-bbb and relay-mic are unchanged.
Decisions
Maintainer decisions, 2026-09-29, via /quest-plan:
Container.Consumeron the simulated clock, not straight into the ring, so the grade covers the player's real skip and wait decisions. (Alternative: ring-only, documented.) This resolves the Codex P1.@moq/net/@moq/hangAPI: no@moq/watchhook, no CLI flag.Sync, with the 100 ms fallback where the relay reported no PROBE RTT. Revisit when the watch quest moves the target to the arrival estimator.--profilesvalue inreplay.ts(fixed); grading the silent tail is a follow-up.js/watchand the harness, not only nightly, since they are deterministic and fast. Wired with path filters in a new workflow.Impact
replay.tsis@internaland not reachable through@moq/watch's exports map.--replaysflag onjust test audio-quality.Audio quality / Replayjob on PRs.Alternatives
just check, asreplay-ci.mdplanned. The maintainer chose a path-filtered workflow instead.Not covered
relay-micuses Chromium's fake capture device, not a real microphone. A headless agent has no mic to capture, and publishing a real one to a public relay needs a person's consent.Follow-ups
Traceand replay up to it, so a publisher that stops early is graded to the end (Codex finding; maintainer deferred it).relay-micwith a real microphone in the watch quest's manual run (already inquest/m0/audio-jitter-target/watch.md).main, which has fix(watch): hold video frames and reject older decode groups #4483 (reject older decode groups). Mergingmaininto the questline may move the replay numbers; re-baseline them then.Interopfails on the TSpcr-monotoniccheck here and on unrelated branches (quest/m1/watch-refusal,quest/m1/interop-audio-cold-start). That is a base failure, not this PR. fix(moq-mux): one TS re-anchor shift per program #4513 may cover it.quest checkfails onmain:quest/m2/ts-import-health.mdlinks the deleted/quest/m1/srt-import-stats.md. Unrelated.Checks
just checkpasses except for that unrelatedquest checklink.just test audio-quality --replays --enforcegrades all 6 rows within budget. The output is byte-identical across runs.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code