Repository navigation
feat(publish): mark a discontinuity when an encoder pauses for demand - #3982
Conversation
The browser's demand-gated encoders only cut the open group when the last subscriber left. The group before the pause then reached up to the keyframe that resumed it, so it never expired and a subscriber joining after the resume was served stale media as live. Add `Legacy.Producer.discontinuity()`, mirroring moq-mux: close the open group and publish a marker group of one empty frame at the live edge. The video and audio encoders now mark every demand gap. The watch audio decoder kept a marker's endpoint until the next playhead event, so every sample resumed after an audio marker was trimmed to silence (also hit by Rust publishers such as moq-boy). An endpoint now bounds only the terminal packets in its own group. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 11 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
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. |
|
MERGE Positive improvement. Demand-gated JS encoders were cutting the open group on pause but not bounding it, so under lite-06 the pre-pause group reached to the resume keyframe and looked live to anyone joining after the gap. That matches the Smoke "after rejoin" flake story. Shipping a hang marker group (and teaching the audio Terminal that an endpoint only covers terminal packets in its own group) is the right fix, and it brings browser publish in line with Worth the complexity. The new surface is one method plus a small Terminal rule change; no wire-format change. Draft SHOULD + audio endpoint clarification are appropriate. Tests hit the important contracts: marker shape/idempotence, pre-pause group no longer reading as live under No better approach jumps out. A zero-frame group cannot bound reach; video-only markers would leave audio's last frame stretching across the gap; keeping Nits, not blockers:
Ship it (ideally with #3977 as noted for the reattach Smoke path). This is an automated review, not the maintainer's decision |
An explicit cut now both closes the group and marks the break, at the caller's end or the live edge. The keyframe rollover and close() close the group without a marker. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![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: d5258854bf
ℹ️ 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".
| this.#resetEpoch(); | ||
| } | ||
| // A frame from a later group is new media, e.g. resuming after a discontinuity marker. | ||
| if (next.frame && next.group !== this.#endGroup) this.#end = undefined; |
There was a problem hiding this comment.
When an audio endpoint is followed by terminal codec packets in group G and group G+1 is already buffered, #runLegacyDecoder can queue those packets with decoder.decode() and consume G+1 before the asynchronous WebCodecs output callbacks invoke Terminal.span(). Clearing #end as soon as G+1's encoded frame is read means the still-pending output for G is no longer clipped, so codec padding past the declared endpoint can be rendered. Associate the bound with queued output or drain the decoder before clearing it at the group boundary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining this one.
#end is cleared when a frame from another group is read, and span() does run later from the AudioDecoder output callback. That only drops the trim if group G still has terminal packets queued in decode() when a frame from group G+1 is already buffered.
That is not a shape this PR publishes, and it is not what moq-audio publishes either:
- A demand-gap marker is its own group: one empty frame, and nothing from it is submitted to the decoder. The resume frame is supposed to clear
#end. Keeping the marker endpoint was trimming that next group to silence, which is the bug this PR fixes. The marker also bumps discontinuity, and#runLegacyDecodercallsdecoder.reset()on that jump, so outputs still queued from the pre-pause groups are aborted instead of spanned after#endis cleared. - The endpoint followed by terminal codec packets in the same group is
moq-audio'sfinish()path. That call ends the track. No later group is buffered, sonextMediablocks until the track ends and the pending output callbacks run while#endis still set.
Stamping the bound onto queued output, or flushing before clearing it, would matter if a publisher continued after a terminal group. Nothing does, and a flush on the live path is not worth landing here.
(written by Grok 4.7)
|
Squash auto-merge enabled. Demand-gated encoders now bound the open group on pause. The hang draft ships with this. A publisher that stops producing and may resume on the same track SHOULD publish a discontinuity marker, and an audio endpoint bounds only the terminal packets that follow it in the same group. No wire-format change.
CI: Check and Test passed on (written by Grok 4.7) |
Merges the 49 upstream commits in 7ee2b02..ffa5b81 onto PR #4 at 0bd542a. Breaking upstream changes this brings: none carries a `!` marker. Behavior changes worth knowing: - moq-dev#3982 Container.Legacy.Producer.cut() now publishes a discontinuity marker group, and the audio encoder declares a demand gap with an endpoint marker. - moq-dev#3973 the container consumer plays the head group once the gap from where presentation left off exceeds the age budget. - moq-dev#3934 a refused getUserMedia is terminal until settings, devices or permission change; a camera source announces only once every enabled track is live. - moq-dev#3935 catalog readers refuse updates with more than 64 renditions. - moq-dev#3963 test/smoke is test/interop: `just test interop [--all]` and `just test media` replace `smoke` and `smoke-media`. Upstream landed no audio playout target work in this range: the audio-jitter-target quest only moved from quest/main to quest/m0, and jitter.ts, rs/moq-audio's playout and doc/concept/audio-jitter.md are untouched upstream, so the fork's estimator, rings, stall monitor and corpus stay byte for byte as they were. Conflicts: - bun.lock: upstream's version, see the last line. - doc/lib/js/net.md: upstream's Discovery line; keeps the fork's Error.Expired in Subscriptions. - doc/lib/js/publish.md: announce row carries upstream's camera wait and the fork's latch; the refusal paragraph gains the busy-device retry. - drafts/draft-lcurley-moq-hang.md: upstream's "until a later group begins", then the fork's mute endpoint rules, the last one shortened so the draft does not say the untrimmed next group twice. - js/hang/src/container/consumer.test.ts: upstream's rewrite of the shared waited-out-gap test, whose expectation (resume at B) matches the fork's. - js/justfile: upstream's interop path plus the fork's playout corpus. - js/publish/src/audio/encoder.ts: see audio below. - js/publish/src/element.ts: the fork's announce latch now closes only once upstream's readiness holds (every enabled track for a camera), so a refused microphone still withholds the broadcast and a device swap still keeps every subscriber. - js/publish/src/source/camera.ts, microphone.ts: the fork's release wait and Attempt shape; a refusal goes through Retry.refused (below), and upstream's per-attempt error clear is dropped because the fork's Retry keeps a repeating busy error set until capture runs, which that clear would toggle once a retry. - js/publish/src/source/retry.test.ts: the fake keeps both the fork's hold and upstream's denied switch. - js/watch/src/audio/decoder.ts: the fork's #declareEnd and #onNext, with upstream's `group` in #onNext's argument. - js/watch/src/audio/terminal.test.ts: both endpoint tests. - package.json: the audio-quality workspace plus upstream's interop ones. - rs/moq-cli/src/play/media.rs: upstream's shared engine and tail drain, on the fork's jitter buffer and device cushion. - test/interop/clients/js-native/tsconfig.json: the fork's file, at the renamed path. Audio, per file: - js/publish/src/audio/encoder.ts: upstream's #end field and demand-gap marker effect stay as written. The fork's mute endpoint now reads and clears the same #end, so a pause and the demand gap after it are declared once, and #end keeps the fork's frame-duration fallback. The demand-gate comments said a demand gap is left for the subscriber to find; they now say it is declared. - js/watch/src/audio/terminal.ts: upstream's endGroup rule merged clean; the fork's epoch and hole handling is untouched. - js/hang/src/container/consumer.ts: upstream's promotion (moq-dev#3973) and the fork's walk in #checkMaxAge (c1f899e) fix the same held picture with different measures, and both are kept. Upstream's measures from where presentation left off, so it fires first on a lone head; the fork's measures from the head's own start and still covers a hole with nothing presented yet, or one only later groups prove. Comments on both say which covers what. - js/hang/src/container/legacy.ts: upstream's cut() would throw writing its marker into a closed track; it now returns first, as the fork's #close already does, so cutting after the track closed stays a no-op. Adapted outside the conflict markers: - js/hang/src/container/consumer.test.ts: the fork's lone-group test starts from an empty first group, the case only the fork's walk covers; its old setup now resumes on upstream's rule at the first live frame. - js/publish/src/audio/encoder.test.ts: the fake track has `closed`, a demand gap now expects its endpoint on both the fake and the real broadcast path, and a new test pins one marker for a pause and the demand gap after it. - js/publish/src/source/retry.ts: Retry.refused retries a busy device (NotReadableError, AbortError) as the fork did and makes any other refusal terminal as upstream does; the fork's still-being-released camera test fails if every refusal is terminal. - js/watch/src/audio/terminal.test.ts, js/watch/src/sync.replay.test.ts: the fork's Terminal.update calls carry `group`. - doc/bin/cli.md: the retired rendition plays out all it buffered before the replacement starts; only the device cushion overlaps its fill. - js/publish/README.md: busy devices are retried. - test/audio-quality/clients/js/driver.ts, safari.ts: import the harness from test/interop. - test/interop/clients/js/harness.ts, .github/workflows/nightly.yml, js/hang/src/container/stall.test.ts, js/watch/src/audio/decoder.test.ts: smoke naming follows the rename. bun.lock was taken from upstream and must be regenerated with bun install. 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>
1 participant
Problem
The browser encoders (
js/publishvideo and audio) only encode while there's demand. When the last subscriber left they cut the open group and went quiet. Under lite-06 expiration a group's reach runs to its successor's first frame. The pre-pause group therefore reached all the way to the keyframe that resumed seconds later: it never expired, and a subscriber joining after the resume was served it as live. This is what surfaced in the Smoke "after rejoin" flake (#3973, #3977).Rust already handles this:
moq-muxProducer::discontinuity()publishes a marker group, andmoq-video,moq-audioandmoq-boycall it when capture stops. JS had no equivalent.Approach
@moq/hang:Legacy.Producer.cut(end?)now does both jobs. It closes the open group and writes a marker group of one empty frame atend, or at the live edge without one. No marker is written until a new frame follows, and data tracks only close the group. The keyframe rollover andclose()close the group without a marker. WithmaxAge: 1safter a 5s pause, a subscriber is now served[marker, live]instead of[stale, marker-less gap, live].@moq/publish: the video encoder'scut()on a demand gap now marks the break. The audio encoder writes one group per frame straight to the track, so it writes the marker itself at the last frame's exclusive end.@moq/watch: the audioTerminalkept a marker's endpoint until the next playhead event. Audio resumed after a marker group was therefore trimmed to silence, which also affected Rust publishers such as moq-boy. An endpoint now bounds only the terminal packets in its own group, which is how moq-audio writes them onfinish().Impact
@moq/hangLegacy.Producer.cut()now also publishes a marker group, a behavior change for callers that used it only to close a group early. No new API. Rustmoq-muxstill splits this intocut()anddiscontinuity().Testing
Legacy.Producermarker shape, idempotence, and data/empty cases. A pre-pause group no longer reads as live after resume; this test fails withcut()alone.Terminal: resumed media in a later group is not trimmed. Fails before the fix.cut(end)places the marker at the caller's end.cut().just checkandjust testpass.just test smoke-mediapassed 3/3 combined with fix(relay): keep only finished groups warm when a track goes idle #3977. Without fix(relay): keep only finished groups warm when a track goes idle #3977 one of two runs failed "video monotonic after reattach" on the reattach path that PR fixes. The audio encoder's marker has no unit test; it would need a fake AudioEncoder pipeline, and smoke exercises it (the first cut failed smoke with silent audio until theTerminalfix).Alternatives
Follow-ups
moq-muxkeepscut()anddiscontinuity()separate. Folding them together to match is a breaking change fordev. It would also give themoq-ffi/libmoqencoders, which only exposecut(), a way to mark a pause.🤖 Generated with Claude Code
(written by Claude Opus 5.5)