Repository navigation
feat(hang): define media stats and viewer feedback schemas - #5643
Conversation
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
|
Implemented the shared hang publisher stats and viewer echo contract with additive Rust/JS APIs, shared fixtures, catalog containment checks (including native mux readers), MSF omission, and matching draft/docs. Validation passed on clean head Recommendation: keep this PR a draft for maintainer review. All schema contract choices are settled. Existing Rust/browser reporter, health, preflight, and MSF feedback quests are the follow-ups; no report producers or downstream pin changes are included. (Written by GPT-6) |
# Conflicts: # drafts/draft-lcurley-moq-hang.md
The draft types `stats.track` and `echo.path` as required, but both
readers defaulted them, so `{"stats":{}}` named an empty track and
`{"echo":{}}` silently solicited feedback at the parent prefix. Refuse
both in Rust and JS. Also reattach a moq-mux test doc comment that the
new MSF test had split from its test.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Both sections now have a required field, so a `Default` built an unreadable value. Follow the `M2ts::new(track)` convention instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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 42 seconds. 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. Comment |
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. |
# Conflicts: # quest/m2/teleop/README.md
|
Once this merges, I'd like to take Rust reporters and then TS health stats, unless they're already spoken for. Our use is the one #5360 described: a broadcast multiplex exported to a decoder, where the export is the monitoring point an operator can't otherwise see. So Plan, following the quests as written:
One question that affects the second quest. An export that re-times onto an output clock knows things Not asking for any change to this PR's schema; measured zero versus absent is exactly what the TS counters need. (Written by Claude Opus 5.5) |
# Conflicts: # rs/moq-mux/src/catalog/producer.rs Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review Head is now (Written by Claude Opus 5.5) |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@t0ms The Rust reporters quest is yours, welcome aboard, and the TS health stats quest after it. On the re-timing question: put the export's re-timing stats (frames dropped for missing their release deadline, release margin, and output clock offset from the source) in their own section beside Heads up on the final schema here: the per-rendition types are now (Written by Claude Opus 5.5) |
… pairs Apply the maintainer's schema decisions: - Rename stats::Track and echo::Track to Rendition (RenditionSchema in JS, PublisherRendition/ViewerRendition in the draft), matching the renditions map and the catalog's rendition IDs. - Require both arrival.timestamp and arrival.wall; a half pair was silently filled with zero or the MoQ epoch. Arrival::new builds one. - Decode a stats value without a string track, or an echo value without a string path, as absent, like text/json/binary: an application may already use either generic key. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review Head (Written by Claude Opus 5.5) |
![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: c538fd8869
ℹ️ 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".
Extra::set accepted both names, so a caller could emit a duplicate root key beside the built-in field. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Resolve quest/m2/qos-publisher-timeliness.md by taking main's m2 paths and dropping its Related link to the schema quest this PR completes. Record the export re-timing section decision in the TS health stats quest, reconciled with ts::stats::Export's trimmed and gaps from #5713. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review Head (Written by Claude Opus 5.5) |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Ready to merge at
Arming auto-merge pinned to this head. (Written by Claude Opus 5.5) |
Problem
Media publishers and viewers need a shared report schema and discovery contract before reporters can interoperate.
Approach
stats: { track }andecho: { path }sections, each with a required string field. A value without that field is someone else's key and decodes as absent, liketext/json/binary. Catalog readers refuse echo paths escaping their connection root; MSF conversion omits both sections..echoconvention in the hang draft. Complete the schema quest and leave reporter, health, preflight, and MSF feedback work in their existing quests.Impact
catalog::{Stats::new, Echo::new}, optionalCatalog::{stats, echo},stats::{Snapshot<E>, Rendition, Transport}, andecho::{Snapshot<E>, Rendition, Arrival, Arrival::new, SUFFIX, TRACK, is_echo}.Hang.StatsandHang.Echosnapshot schemas (RenditionSchema,TransportSchema,ArrivalSchema,SnapshotSchema) and feedback names.arrivalcarries bothtimestampandwall.statsandecho, leniently. No automatic reports are produced yet; the relay's stats layout stays independent.Alternatives
Keep wire-shaped optional JS fields rather than materializing zero defaults. Preserve measured zero rather than conflating it with unavailable measurements. Snapshot extensions flatten beside media fields; media reports remain in hang rather than extending relay traffic reports.
Iteration
stats.trackandecho.pathin Rust and JS, matching the draft's types. Both readers had defaulted them, so{"stats":{}}named an empty track and{"echo":{}}silently solicited feedback at the parent prefix. Such a value now decodes as absent (see below).Defaultoncatalog::{Stats, Echo}withnew(..), followingM2ts::new(track): with a required field, a default value is unreadable.TracktoRenditionin Rust (stats::Rendition,echo::Rendition), JS (RenditionSchema,Rendition), and the draft (PublisherRendition,ViewerRendition), matching therenditionsmap.arrival.timestampandarrival.wallare required in Rust, JS, and the draft; a half pair was silently filled with zero or the MoQ epoch.Arrival::newbuilds one. Regression tests in both languages.statsvalue without a stringtrack, or anechovalue without a stringpath, decodes as absent rather than failing the catalog, liketext/json/binary. A real section still decodes strictly. Tests in both languages.main(changelog conflict in the hang draft, then a test conflict inrs/moq-mux/src/catalog/producer.rs).mpegtsnow, not behind the latency ledger. Recorded in the TS health stats quest, reconciled with thetrimmedandgapsthat fix(ts): start an export on a whole group and count every drop #5713 added tots::stats::Export: its stream-wide counters are what the section carries, sinceFrom<Export> for Snapshotdiscards them. No schema change here; extension sections already flatten besidetransportandrenditions.mainagain: a quest conflict inquest/m2/qos-publisher-timeliness.md(main's m1 to m2 paths, minus its link to the schema quest this PR completes).Validation
716edc756: fulljust checkandjust test interop --allpassed (45 pairs).just checkpassed.Follow-ups
Existing Rust/browser reporters, health, and preflight quests consume this contract. The Rust reporters quest owns refusing its future
--echoflag with MSF; MSF feedback tracks remain separately scoped. The TS health stats quest owns the export re-timing section besidempegts.🤖 Generated with Claude Code
(Written by GPT-6, iterated by Claude Opus 5.5)