feat(hang)!: name the container producer's timeline break discontinuity() - #4141
Conversation
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: 06c85c4032
ℹ️ 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".
| * is written until a frame follows the last one. Throws if `end` precedes the last video frame. | ||
| */ | ||
| cut(end?: Time.Micro) { | ||
| discontinuity(end?: Time.Micro) { |
There was a problem hiding this comment.
Reserve a skipped sequence for data discontinuities
When this newly named method is used with a data format, the branch at line 146 returns after closing the current group, so the next group receives the immediately adjacent sequence, as the changed test explicitly expects. In contrast, moq_mux::container::Producer::discontinuity() skips a sequence for data because an empty payload is valid data. Without either a marker or a sequence gap, JS consumers cannot distinguish the two epochs and treat the resumed data as continuous.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving the data-track path unchanged. discontinuity() still only closes the group, and the next sequence stays adjacent; the existing test locks that in. Rust skips a sequence because an empty payload is data, and the quest this PR finishes deferred that skip until a JS data-track caller exists. None does. Skipping a sequence here would change what gets published, which this rename does not.
(Written by Grok 4.7)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ty() Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Verdict: MERGE Clean naming alignment: JS Change is a pure rename plus call-site/test updates, quest doc retired, and it correctly targets Worth doing; no better alternative that preserves clarity. This is an automated review, not the maintainer's decision |
Co-Authored-By: Grok 4.7 <grok-4.7@x.ai>
06c85c4 to
740c41a
Compare
|
Rebased onto current (Written by Grok 4.7) |
Problem
@moq/hang'sContainer.Legacy.Producer.cut()closes the group and writes the empty marker group that tells subscribers to re-anchor. Rustmoq_mux::container::Producersplits that intocut(end)for a plain group close anddiscontinuity()for the break. The same name meant different things in the two languages.Approach
Rename the JS method to
discontinuity(end?), matching Rust. It behaves the same as before. No JS caller needs a plain group close yet, so none is made public. The publish video encoder and the tests now call the new name. Deletesquest/m2/js-discontinuity.md.Impact
@moq/hang:Container.Legacy.Producer.cut(end?)is renamed todiscontinuity(end?). This breaks the published API, so the PR targetsdev.Alternatives
cut()for the plain group close next todiscontinuity(). Skipped until a caller needs it.Follow-ups
None.
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code