Repository navigation
test(audio-quality): grade steady-state lag, the player's own trims, and recovery from congestion - #8459
Conversation
Quest-Claim: 758433f7b4eba31e
…tion rows Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 2 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit c008a8c. No actionable correctness findings.
Overall direction: the grading changes consistently replace inferred skips with player counters, add lag gates, and report recovery without incentivizing discarded audio. Schema, analyzer, grade table, budgets, and harness matrix agree. The new congestion profile matches the shaper's rate-step behavior; the replay preserves every frame while applying the documented stall and twice-speed backlog. Provisional calibration and exact replay budgets remain explicitly documented.
Verification limits: GitHub-only static review, including the replay consumer and shaper configuration paths. No builds, tests, live audio runs, or independent reproduction of the reported calibration measurements.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ Two minor suggestions — nothing blocking.
Reviewed changes
- Dropped audio is now the player's own count:
skip_aheads/discarded_msare deleted from the schema,analyze.ts(skipAheads,SKIP_MS,SKIP_WINDOW), its tests, the grade columns, and every budget;player_skips_per_minandtrimmed_ms_per_minare budgeted on all 34 rows instead, with the exception of the new stall trace. - Steady-state and through-stall lag:
av_lag_ms_p50andav_lag_ms_p95ceilings now exist on all 28 live rows (replay rows stay exact). I re-derived every new lag ceiling from the PR's measurement tables with the note's rule (worst × 1.25 + hiccup, plus one 20 ms bucket for the signed lag) and they all reproduce, as do the congestion rows' skip, trim, target, and underrun ceilings. recover_ms: a new reported-only metric built onlongestAway(): the longest run from a stalled sample, or one more thanBUCKET_MSbehind the window's median lag, to the next sample back. It is null only when no playing sample has a lag, which theclean()test covers.- Congestion, twice:
congestion.toml(1 Mbit/s behind a one second queue, collapsing to 16 kbit/s for 25–26.5 s) adds four live rows, and the derivedrelay-mic-stalltrace adds one replay row. The trace's rewrite is faithful: 151 arrivals moved from 15.0–18.0 s into 16.5–18.0 s, oldest first at twice real time, still sorted byatand preserving media timestamps. - Docs and counts: the audio-quality README, budget note,
nightly.yml,test/justfile, ptt comments, and quest links move to the new metrics and 28 live rows / 6 replays;grading.mdis deleted with no dangling links left.
I ran bun test test/audio-quality/clients/js/src/analyze.test.ts (14 pass) and src/run.test.ts (3 pass), and diffed the old and new budgets.json: 29 to 34 rows, no pre-existing ceiling moved apart from the removed keys.
ℹ️ The shaper README no longer lists every profile the lane uses
rs/moq-shaper/README.md describes the audio-quality lane as using bursty and step "plus its own flag profiles". This PR adds a third kind, a profile file at test/audio-quality/congestion.toml, so a reader of the shaper crate would not learn that --profile <path> is exercised in-tree or where the new treatment lives. That file is not in the diff, hence the note here.
Technical details
# Shaper README profile list
## Affected sites
- `rs/moq-shaper/README.md` — the paragraph under "Profiles" after the TOML example enumerates the lane's profiles and omits `congestion.toml`.
## Required outcome
- The sentence names the file profile (or points at `test/audio-quality/README.md`) so all three kinds the lane uses are accounted for.deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
| valid "$p" profile "${ALL_PROFILES[@]}" "${ALL_TRACES[@]}" | ||
| if [[ "$p" == step ]] && ((DURATION <= 30)); then | ||
| echo "error: --duration must exceed the step at 30s" >&2 | ||
| if [[ "$p" == step || "$p" == congestion ]] && ((DURATION <= 30)); then |
There was a problem hiding this comment.
congestion is guarded like step, but its change is a 1.5 s stall ending at 26.5 s and the row exists to report the recovery after it. The PR's own measurements put that recovery at 2.5–4.3 s (browser) and 3.5–17.75 s (native), so a --duration 31 row stops before the backlog clears: recover_ms under-reports and av_lag_ms_p95 is sampled mid-recovery. Since the new test blesses 31 s as a valid congestion row, either require enough duration past the change for the recovery or state that only the 60 s default measures it.
Technical details
# Congestion rows stop before their own recovery
## Affected sites
- `test/audio-quality/run.sh:144` — the `DURATION <= 30` guard is shared with `step`, whose change is a single event at 30 s.
- `test/audio-quality/clients/js/src/run.test.ts:13` — asserts `--duration 31` is a valid `congestion` row.
- `test/audio-quality/README.md` — says `--duration` must exceed 30 s for `congestion`.
## Required outcome
- A congestion row's playback window covers the measured recovery (the default 60 s does), or the docs and test stop presenting 31 s as adequate for it.
## Suggested approach (optional)
- Give `congestion` its own lower bound (the change ends at 26.5 s, recovery up to ~18 s on native) and update the test's accepted duration, or keep 31 s and note that a narrowed congestion run only grades trimming, not recovery.
Problem
Some of the audio harness's gates reward a player for moving away from the epic's model, where audio rests on the shared clock, trails it through a stall, and catches up after:
skip_aheadsanddiscarded_ms, on all 29 rows, infer a skip from a step in the playhead's lag. A time stretch catching up reads as dropped audio.Completes
quest/m1/audio-jitter-target/grading.md(deleted here).Approach
skip_aheadsanddiscarded_ms(schema, analyzer, tests, grade table, every budget). Every row now budgetsplayer_skips_per_minandtrimmed_ms_per_min.av_lag_msp50 (steady state) and p95 (through a stall). The ceilings have measured headroom, and none is fixed at zero.recover_ms(max): the longest stretch audio spends stalled or more than one 20 ms bucket behind the window's median lag. It is reported, not budgeted, because it is the time a stall costs, and a player may trade that time against the audio it keeps.congestionlive profile (congestion.toml) is 1 Mbit/s down behind a 1 s queue. The rate collapses to 16 kbit/s from 25 s to 26.5 s, so the queue fills and then drops, per the bufferbloat rule. When the rate comes back, the transport delivers the backlog. This adds 4 rows (browser and native, Opus and AAC).relay-mic-stallreplay trace isrelay-micwith nothing arriving from 15 s to 16.5 s. The held frames then trickle in at twice real time until 18 s.test/justfileand thenightly.ymlcomments, and the run.sh duration guard, which now coverscongestiontoo.How the new ceilings were measured
All ceilings use the note's rule: the worst run times 1.25, plus the documented hiccup, rounded up to two significant figures.
av_lag_msis signed, so its ceiling is the worst run plus a quarter of its size plus one 20 ms bucket. All new ceilings are provisional until calibrate re-measures every row from seven Nightlies with the model.mainthat have today's browser player and its trim counter: 2026-11-09 (42659882487) and 2026-11-10 (42842827718). The skip and trim ceilings take the old skip hiccup: 3.3 skips/min and 280 ms/min.--replays --enforce. The existing five did not move.Measured
av_lag_msp50 / p95 (ms):Congestion rows, three local runs each:
relay-mic-stallreplay: today's browser player trims 2152.5 ms/min and skips 6.1 times a minute. Its target p95 rises to 1540 ms, and audio takes 3000 ms to return to the clock.What the numbers already show:
steprows rest 30 to 40 ms behind the clock after the step. That is the stale-reference effect the deferred clock-step decision is about.Impact
Summarymetrics:skip_aheads_*anddiscarded_ms_*are gone, andrecover_ms_maxis new. Anything reading oldsummary.jsonkeys must switch toplayer_skipsandtrimmed_ms.Choices made without asking
recover_msis reported, not budgeted. A ceiling on it would reward trimming over stretching.mildandstepexceeded their old provisional underrun ceilings once each in three runs. Native rows are report-only, so this is noted here, not re-budgeted.test/audio-quality/rather than as amoq-shaperbuilt-in, since only this harness uses it.relay-mic-stallis derived fromrelay-micrather than recorded, so the stall is exact and repeatable. Itssourcefield describes the rewrite.Alternatives
recover_ms: rejected (see above).Follow-ups
Models
Size: M
Implementer: Claude Code, Claude Opus 5.5, medium effort
(Written by Claude Opus 5.5 with medium)
Review
Rewritten by
quest reviewafter each round.01a1c784-d75c-7152-a74e-d95404a8ca20, round 1The grading changes and congestion coverage appear coherent from source review with no actionable defects identified; builds and tests were not run as instructed.