Merge main into dev - #2883
Conversation
…#2823) Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Codex GPT-5.6 <noreply@openai.com>
Use the shared VP8 and VP9 importers so RTC publishers advertise coded dimensions from keyframes. Defer unresolved catalog reservations until media arrives, and preserve the session abort cause if importer construction fails.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: GPT-5 <noreply@openai.com>
Co-authored-by: sreese <shayne.reese@mlb.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Resolve catalog broadcast references against the catalog parent while preserving empty self-references and valid root references. Ignore above-root references before media selection, export, and SDP codec discovery, and normalize transcoder-derived references consistently. Co-Authored-By: GPT-5 <noreply@openai.com>
Co-authored-by: GPT-5 <noreply@openai.com>
Co-authored-by: moq-bot[bot] <186640430+moq-bot[bot]@users.noreply.github.com>
Co-authored-by: GPT-5 <noreply@openai.com>
Resolves 27 conflicts from the 11 commits `main` gained since #2852. The non-mechanical ones: **Catalog broadcast references.** Two independent changes, both kept. main #2855 changed the *resolution algorithm* to relative-URL semantics (a non-empty reference replaces the base's last segment, then `.`/`..` apply); dev #2630 changed *escape handling* to reject the whole catalog. main already ships both `resolve` (clamping) and `try_resolve` (`Option`), so its `path.rs` is taken wholesale and dev's callers point at `try_resolve`. This shifts what escapes by one segment, which is why several tests move from `../x` to `./x`. The escape policy is two-sided rather than one choice. main's `retain_valid`/`resolve_reference` is publisher-side sanitisation with 9 call sites across moq-mux, moq-hls, and moq-rtc: kept. dev's `EscapingBroadcast` plus `Source::target` returning `Result` is consumer-side rejection: also kept, now layered on `try_resolve`. main's duplicate `InvalidBroadcastReference` variant is dropped for dev's better-documented `EscapingBroadcast`. `Source` grows a `request`/`try_request` pair mirroring `resolve`/`try_resolve`, so the exporters keep skipping one bad rendition while consumers keep reporting the fault. `js/watch` is a pure consumer, so it takes the reject policy alone; main's `filterCatalog` is dropped (it also missed the `text` section). Its `findEscaping` had to move to `Path.tryResolve`, since main's `resolve` clamps and would have silently disabled the check. **`Connecting` removal.** main #2856 deletes the mechanism; dev had rewritten the same thing into its poll driver (`SubscriberDriver`, `AnnouncePrefix`, 27 references). main's removal wins, applied on top of dev's driver: the per-prefix producer, the `initial_count`/`initial_remaining` bookkeeping that existed only to release it, and the `client.rs` wait sites all go, keeping dev's `goaway` plumbing. **Fixes carried across a rewrite.** main #2862's live-edge cursor fix landed in an async `run_track` that dev deleted, so it is reapplied to dev's poll-based `TrackServe::new`; its regression test now drives that. main #2841's WebSocket `SessionInputs` refactor gains dev's `shutdown` field. main #2874's target-carrying `DialSources` gains dev's mDNS source, and `run_mdns` moves to `upsert`/`release` (dev's 2-arg `release` was superseded). The hang draft and docs state main's URL resolution semantics *and* dev's reject-the-catalog rule. `just check` and `just test` pass: 3185 tests, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`retain_valid`/`retain_valid_media` came from main, which has no `text` catalog section; `text` is dev's captions work. Merging the two left the publisher-side containment filter covering video and audio only, while the consumer-side `check_resolvable` already covered all three. That is exactly the drift dev's own comment warns about: a section left out silently exempts its renditions from the check. Latent rather than live today, since no exporter consumes text renditions yet and the consumer rejects such a catalog outright. It would have surfaced as a silent hole the moment text export landed. Co-Authored-By: Claude Opus 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: 4ce12d5d9a
ℹ️ 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".
| broadcast: active, | ||
| track, | ||
| config, | ||
| config: identity.decoder, |
There was a problem hiding this comment.
Preserve rendition jitter outside the decoder identity
When a rendition declares jitter or only framerate, identity.decoder contains neither field, but DecoderTrack still calls renditionJitter(props.config). It therefore always publishes undefined for video jitter, so video-only streams with grouped or delayed frames lose the catalog-derived buffer allowance and can repeatedly underrun. Keep the reload identity narrow, but pass or derive the full rendition's jitter metadata separately.
Useful? React with 👍 / 👎.
| pub struct VideoConfig { | ||
| /// Optional reference to another broadcast that publishes this track, expressed | ||
| /// relative to the broadcast that served this catalog (e.g. `../source`). If unset, | ||
| /// relative to the broadcast that served this catalog (e.g. `./source`). If unset, |
There was a problem hiding this comment.
Point catalog consumers at the rejecting resolver
When an external consumer follows this broadcast field's adjacent Path::resolve documentation, excessive .. segments no longer produce no broadcast: this commit changed resolve to clamp and introduced try_resolve for containment checks. That can silently select an unrelated root broadcast instead of rejecting the catalog, so both VideoConfig and AudioConfig should direct consumers to Path::try_resolve.
AGENTS.md reference: AGENTS.md:L97-L99
Useful? React with 👍 / 👎.
Merges the 11 commits
maingained since #2852. 27 conflicts; the non-mechanical decisions are below.Catalog broadcast references
Two independent changes, both kept:
./..apply.main already ships both
resolve(clamping) andtry_resolve(Option), so itspath.rsis taken wholesale and dev's callers point attry_resolve. main itself had zero callers of the clampingresolve, so this moves no behavior.This shifts what counts as escaping by one segment, which is why a number of tests move from
../xto./x. From a two-segment base likea/pub,..now lands on the root and../..escapes.The escape policy is deliberately two-sided, not one choice:
Source::retain_valid/resolve_referencedrop the one bad rendition. 9 call sites across moq-mux, moq-hls, moq-rtc.Error::EscapingBroadcastrejects the whole catalog, which is what the hang draft now mandates.main's duplicate
InvalidBroadcastReferencevariant is dropped for dev's better-documentedEscapingBroadcast.Sourcegrows arequest/try_requestpair mirroringresolve/try_resolve, so exporters keep skipping one bad rendition while consumers keep reporting the fault.js/watchis a pure consumer, so it takes the reject policy alone; main'sfilterCatalogis dropped. ItsfindEscapinghad to move toPath.tryResolve— main'sresolveclamps and never returnsundefined, so leaving it would have compiled, type-checked, and silently disabled the containment check.Connectingremovalmain #2856 deletes the mechanism; dev had rewritten the same thing into its poll driver (
SubscriberDriver,AnnouncePrefix, 27 references). main's removal wins, applied on top of dev's driver: the per-prefix producer, theinitial_count/initial_remainingbookkeeping that existed only to release it, and theclient.rswait sites all go, keeping dev'sgoawayplumbing.Fixes carried across a rewrite
Two main fixes landed in code dev had deleted, so they were reapplied by hand rather than lost:
run_trackinto dev's poll-basedTrackServe::new. Same lifecycle point: after SUBSCRIBE_OK, once per subscribe, before any group is consumed. Its regression test now drives the poll path.DialSourcesabsorbed dev's mDNS source, withrun_mdnsmoved ontoupsert/release(dev's 2-argreleasewas superseded). mDNS gains the fallback-to-another-source behavior the other discovery sources already had.main #2841's WebSocket
SessionInputsrefactor gains dev'sshutdownfield.Docs
The hang draft and
/docstate main's URL resolution semantics and dev's reject-the-catalog rule.Follow-up fix in this PR
retain_validcame from main, which has notextcatalog section —textis dev's captions work. The merge left the publisher-side filter covering video and audio only, while the consumer side already covered all three: exactly the drift dev's comment warns about. Latent today (no exporter consumes text renditions, and the consumer rejects such a catalog anyway), fixed with a regression test that fails without it.Verification
just checkandjust testpass: 3186 tests, 0 failures.Adversarially reviewed by Codex against the mechanical auto-merge as the base, so the diff under review was purely the human conflict resolutions rather than main's already-reviewed commits. Verdict: approve, no material findings. The
textgap above came from a separate manual pass.(written by Opus 5)