Repository navigation
quest(m1): plan the audio-quality and wire-compat follow-ups - #5445
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8e3ea1a0e
ℹ️ 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".
| Compare what each ring reports as arrival and consumption to the estimator: | ||
| an isolated ring that under-reports its fill, reads the worklet's position | ||
| late, or misses a resize would underrun on its own and feed that back as | ||
| measured jitter. Fix it at the source with a regression that fails without |
There was a problem hiding this comment.
Separate ring underruns from target inflation
The ring cannot feed its state back into the measured jitter as claimed here: Decoder.#runLegacyDecoder derives the target from consumer.spread, while Container.Consumer.#runGroup records that spread directly from transport frame arrivals before ring consumption. Under-reported fill, a late worklet position, or a missed resize can explain underruns, but not the 100–800 ms target_ms_p95; the quest should first compare captured arrivals and configuration between the two browser rows rather than directing the target investigation into the ring.
AGENTS.md reference: AGENTS.md:L16-L16
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed: consumer.spread is recorded in the container consumer before the ring, so a ring defect explains underruns but not the 100-800 ms target. bd34293 splits the two: compare the rows' captured arrivals and configuration first for the target, and look at the ring for the underruns.
(Written by Claude Opus 5.5)
|
|
||
| `just test wire-compat` runs at least one current-to-current and one | ||
| released-to-released media cell through the compat reader | ||
| (`test/interop/compat/client.ts`, selected by `INTEROP_COMPAT_CLIENT`), so a |
There was a problem hiding this comment.
Point the control cells at the real media reader
Neither this selector nor the described reader exists: test/interop/compat/client.ts has no media-subscribe command, and the harness selects the staged media reader at test/interop/clients/js-native/subscribe.ts via INTEROP_NATIVE_CLIENT. Implementing the quest literally therefore cannot exercise the reader's media path; name the existing subscriber and selector, or explicitly plan the new selector and reader wiring.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed bd34293 against 52dbffa, including all six planning files and relevant audio/interop code.
No remaining actionable bugs found. The previously identified ring-to-estimator feedback claim is fixed: the plan now compares captured arrivals/configuration first and separates target inflation from playback underruns. This also addresses the independent finding: #5445 (comment).
The #5442/#5444 prerequisites are explicit; #5444 actually supplies the named compat reader and selector, so the separate missing-selector finding does not apply after that prerequisite. Same-version control cells with a deliberately broken decoder and reproducible audio cases are appropriately scoped, without a second harness.
Nonblocking: treat the isolated-page explanation as a hypothesis; the remaining introductory 'not the estimator' wording is stronger than the evidence from separately run rows.
Verification: static GitHub-only plan/source review. No tests run or causal explanation independently reproduced.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge summary at b3e8785.
The scoping decisions in the description were the maintainer's. CI passes; (Written by Claude Opus 5.5) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughAdds quest documents for wire-compat media coverage, AAC bursty-delivery re-stalls, and the Opus isolated-ring target. Adds required-quest links for these items. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
Plans follow-ups from #5442 (audio-quality budgets) and #5444 (wire compat on main).
quest/m1/audio-jitter-target/aac-bursty-restall.md[M]: AAC bursty rows stall 19-43% of every nightly window.quest/m1/audio-jitter-target/opus-isolated-target.md[S]: Opus near-zero on the isolated ring reaches a 100-800 ms target while the postMessage ring holds 60 ms.quest/m1/wire-compat-media.md[S]: same-version media cells keep the compat reader's media path covered while planned breaks skip the cross-version ones.Watch now requires the two audio quests, and the audio-jitter-target epic lists them.
Decisions
Typecheck compat/*.ts follow-up (already typechecked against checkout packages by
just js check)AAC bursty re-stall and Opus isolated-ring blow-up overlap Watch. How to scope them?
Same-version media cell: confirm the goal and act
Public API: none. Wire: none.
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.