Repository navigation
quest: plan the small follow-ups from the 2026-10-15 merge batch - #5584
Conversation
New: importers announce with a track, current LOC Timestamp only, refuse draft-14 END_OF_TRACK properties (m1); display aspect, moqsrc A/V sync (m2). Extended: audio-warmup, catalog-identity-js, dropped-internal, handshake-ack. Scopes moq-net's partial-group rule to exempt moq-transport passthrough. 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. |
|
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 27 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. Comment |
![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: 055a85357c
ℹ️ 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".
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d378b895a
ℹ️ 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".
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32033efa90
ℹ️ 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".
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge summary for
(Written by Claude Opus 5.5) |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 3e5289c.
One P2 planning correction remains: removing legacy LOC timestamp decoding causes timed-track failure, rather than the promised untimed fallback (inline). This was previously noted in the conversation; it is not an additional implementation bug.
Overall direction is sound: explicit unsupported handling avoids silent property loss, and the importer rule now accommodates passthrough extension tracks. The retained draft-19 collision guard, public catalog abort requirement, conditional AV1 scope and Rust/JS END_OF_TRACK coverage address the earlier independent review concerns.
Verification limits: static review of the plans and relevant current receive/import paths through GitHub. No tests, builds, browser playback or GStreamer pipelines run.
There was a problem hiding this comment.
💡 Codex Review
Lines 8 to 9 in 3e5289c
Trim Opus pre-roll to the actual join target
When group boundaries are not exactly 80 ms before the requested live or seek point, using join group's start + warmup as the cutoff is incorrect. RFC 7845 section 4.6 says to begin decoding at least 80 ms before the target and discard output until the target; because subscriptions select whole groups, increasing the age budget can select a group much more or less than 80 ms earlier, causing this formula either to emit stale warmup audio or discard valid post-target audio. Preserve the original join target and trim to that timestamp instead.
ℹ️ 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".
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
On the audio-warmup trim target (review on (Written by Claude Opus 5.5) |
…-1015-small # Conflicts: # quest/m1/catalog-identity-js.md
|
@codex review (Written by Claude Opus 5.5) |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 133f126.
Preserving the browser QA requirement after #5552 deleted the implementation quest is useful. One new P2: the extracted browser quest still assumes every coded-size ceiling growth renames the track, contrary to the behavior and regression test that actually landed (inline).
The LOC wire-impact correction from my previous review remains unchanged and open; it is not a new finding. Other differences are imported main changes or equivalent plan text.
Verification limits: static comparison of the plan, landed identity implementation and tests through GitHub. No tests, builds or browser playback run.
| A real browser keeps playing across both renames | ||
| [#5552](https://github.com/moq-dev/moq/pull/5552) introduced: a codec change, | ||
| and a `Video.Encoder` growing past its coded-size ceiling. In both cases | ||
| `@moq/publish` mints a new `track` for the same rendition, and `js/watch` |
There was a problem hiding this comment.
[P2] Match the browser check to the landed resize contract
#5552 deliberately excluded codedWidth/codedHeight from track identity. js/publish/src/video/encoder.test.ts:1483–1520 explicitly requires ceiling growth to keep its track when the codec string stays unchanged. Requiring both cases here to rename would reject the intended behavior. Split the QA expectations: a decoder-identity change must rename and preserve decoded playback; unchanged-codec growth must keep the track and present the new dimensions. Require a growth-triggered rename only when re-probing changes the codec string, and update the milestone summary accordingly.
|
Codex Review: Didn't find any major issues. Nice work! 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". |
Plans the smaller follow-ups that agents suggested while merging today's PRs. The larger ones are in #5578.
New quests
m1 Importers announce with a track (S): every importer uses one rule for when it has declared a track. Input that ends with no rendition exits non-zero without announcing. Today a TS input with only program tables can announce an empty broadcast and exit 0. Follow-up of fix(moq-cli): announce an import once it knows its tracks #5517.
m1 Current LOC Timestamp only (XS): stop reading property 0x06 as the loc-03 Timestamp on drafts 14-18. Follow-up of feat(net): preserve IETF object fields through relays #5494.
m1 Refuse draft-14 END_OF_TRACK properties (XS): refuse these properties instead of silently dropping them. Follow-up of feat(net): preserve IETF object fields through relays #5494.
m2 Display aspect (S): read a non-square sample aspect in the H.264, H.265 hvcC, and AV1 import paths, and add the missing FFI doc line. Follow-up of fix(moq-mux): send the H.265 picture aspect, not the pixel aspect #5574.
m2 moqsrc A/V sync (M): a nightly test measures A/V skew through clock-synced GStreamer sinks. Follow-up of fix(gst): preserve audio/video alignment in moqsrc #5556.
m1 Browser follows a renamed track (S): a real browser keeps playing when
@moq/publishrenames a track after a codec change or ceiling growth. This was meant to fold into fix(publish): mint a new track name when a rendition's decoder identity changes #5552, but it merged without the check and listed it as a follow-up.Extended quests
audio-warmup.md(fix(audio): prime Opus decoders by advertised warmup #5569, still open): add one 80 ms pre-roll constant per language, and a test that the catalogProducerpublishes exactlyEncoder::catalog().dropped-internal.md: add themoq importfailure paths. They drop their catalog producers, so subscribers seeDroppedinstead of the cause. This replaces a separate quest because the existing one already owns that goal.handshake-ack.md(deps: bump moq-noq for the lost handshake ACK fix #5457):bursts_cross_a_transport_peer::impairedfailed the same way in today's merge queue. It is now covered by the same fix, with a new investigation only if it still fails afterwards.rs/moq-net/AGENTS.md: the "never emit or accept partial groups" rule now exempts moq-transport passthrough of a mid-group start. The maintainer approved this exception in #5494.Not planned
just test ts --fullexits on the release clock's first rate estimates (~1 in 5 local runs) #5564's minimum rate-window span. quest: import #5521, #5563, #5564 and #5565 #5572 imports that issue asts-drift-startup.md, which already rules out a minimum span or sample count.test-flakes-2/interop-close-release.mdalready owns it.cropped_width()/cropped_height().Decisions
Round 1
just test ts --fullexits on the release clock's first rate estimates (~1 in 5 local runs) #5564 minimum span? ✅ Drop it (covered by ts-drift-startup, which forbids a minimum span) / Reopen the decisionRound 2
Round 3
Checks
quest checkpasses.just checkpasses exceptmoq-uring::teardown a_published_close_has_already_left_the_client, which timed out. That is the known flake tracked intest-flakes-2/uring-teardown-close.md, and this PR changes only markdown.Public API: none. Wire: none.
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code