fix(loc): end each group with the duration marker

[kixelated]

Problem

@moq/loc's Producer closed a group on the next keyframe and on close() without the empty frame that ends the last sample's duration. moq-mux has written that marker since #4450. Readers already skip it, from @moq/loc 0.2.3 and moq-mux 0.10.0 on.

Approach

Write the same empty LOC frame moq-mux writes: a 0x10 timestamp property, an empty payload, and that timestamp on the net frame too.

• A keyframe ends the open group at its own timestamp.
• A keyframe that overlaps the previous frame is not that group's end. An ordered group then gets the estimated end (last timestamp plus the last increasing gap), and the gap does not carry into the new group.
• A reordered group omits the marker. Its presentation end does not bound the last frame in decode order.
• A clean close() uses that same estimate when a gap was observed. One frame and no gap writes no marker.
• close(error) writes no marker.

Tests read the wire bytes back through Format, and @moq/hang's container consumer skips both the keyframe marker and the tail marker.

Impact

• Public API: none. No new exports. Producer.encode and Producer.close append a frame when a group ends.
• Wire: a LOC group from this producer gains one empty frame at the exclusive end of its last sample. Same bytes as moq-mux. No draft change.

Alternatives

• Add a kind argument so only video writes the marker. The producer has no kind, and the quest forbids a public API change. The only callers are a test and the @moq/hang re-export.
• Always stamp the next keyframe's timestamp, including a reordered group. That would be a different marker than moq-mux writes.

Follow-ups

• Give Loc.Producer a kind if a non-video caller appears, so audio and data do not grow an empty frame.
• Add discontinuity() if a JS publisher uses this producer for live capture. js/publish does not today.
• close(error) still closes the group without the error. That is pre-existing. Pass the error through only if a caller needs the group itself to abort.

(Written by Grok 4.7)

[kixelated]

Outcome: quest/m2/js-loc-duration-marker is implemented and the quest file is deleted.

@moq/loc Producer now ends a group with the same empty LOC frame moq-mux writes, on the next keyframe and on a clean close(). @moq/hang's consumer test skips that frame. just check passed the JS typecheck, @moq/loc tests, and quest check. The local markdown lint also walked unrelated .worktrees/ copies and failed there; the files in this PR were formatted with remark.

No maintainer decision is blocking. The producer still has no kind, so every group gets the video marker. Recommendation: add a kind only when a non-video caller shows up.

(Written by Grok 4.7)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: d6a632f

No actionable correctness issue found. js/loc/src/index.ts:142-167,198-230 writes the empty LOC marker on keyframe rollover and clean close, suppresses it for reordered groups, and avoids a duration marker on an error close. Producer and hang-consumer tests cover the payload/endpoint distinction, overlap and unknown cadence. Matching the existing Rust marker is a sound direction within this producer's explicitly video-oriented scope.

Verification: reviewed the full diff, LOC format/producer context, tests and prior discussion. Static review only; I did not rerun Bun tests/typechecks or playback. Single-frame tails with no learned interval intentionally remain unmarked. This is a draft COMMENT review, not merge approval.

[coderabbitai]

Warning

Review limit reached

You'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 2 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 4da632cf-49c3-4811-8c8a-2d8d02e68370

📥 Commits

Reviewing files that changed from the base of the PR and between 80f7f61 and d3cb4a2.

📒 Files selected for processing (6)

• js/hang/src/container/consumer.test.ts
• js/loc/README.md
• js/loc/src/index.test.ts
• js/loc/src/index.ts
• quest/m2/README.md
• quest/m2/js-loc-duration-marker.md

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 1ac256db-3c9f-4e48-8a93-5b72a14c5739

📥 Commits

Reviewing files that changed from the base of the PR and between 09e17dd and 80f7f61.

📒 Files selected for processing (6)

• js/hang/src/container/consumer.test.ts
• js/loc/README.md
• js/loc/src/index.test.ts
• js/loc/src/index.ts
• quest/m2/README.md
• quest/m2/js-loc-duration-marker.md

💤 Files with no reviewable changes (2)

• quest/m2/README.md
• quest/m2/js-loc-duration-marker.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

---

Walkthrough

The LOC producer now tracks timestamps within each video group and writes an empty duration marker when the group end is known or can be estimated. Tests cover keyframe boundaries, tail markers, missing intervals, overlapping timestamps, reordered timestamps, and consumer endpoint events. The README describes the marker behavior and consumer handling. The related quest item and plan document were removed.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 80f7f

The ordered-group endpoint behavior is covered, and no merge-blocking issue remains after normal checks.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: adding a duration marker when each LOC group ends.
Description check	✅ Passed	The description is directly related to the changeset and explains the problem, implementation, wire format, edge cases, tests, and impact.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files. (1 skipped: 1 …
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

✨ Simplify code

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[kixelated]

Automated review of db12bae2 (first Code Review pass on this PR, covering the producer change plus the main merge and the maxDelay / optional-timestamp test fix)

The JS producer now matches moq-mux's marker logic closely: the overlapping-keyframe cut with the cadence reset, carrying the cadence across groups for the tail estimate, leaving reordered groups unmarked, and writing no marker on close(err) all mirror rs/moq-mux/src/container/producer.rs (write and close). I found no blocking issues. CI on this head is still pending; every check passed on d6a632f5.

Non-blocking

1. The marker goes on every kind, but Rust only writes it for video. moq-mux gates finish_group on Kind::Video (rs/moq-mux/src/container/loc/mod.rs:91). Loc.Producer has no kind, so #closeGroup (js/loc/src/index.ts:201) writes the empty frame for any track that has learned an interval. On a data track that's a real bug: Format("data").end() returns undefined, so the hang consumer hands the marker over as a genuine empty data object. The existing test Consumer preserves empty Legacy and LOC data frames (consumer.test.ts:867) already uses LocProducer for a data track, and it only stays green because it writes a single frame. Give it a second frame and an extra empty object shows up. The PR lists a kind as a follow-up, which is fine given there are no data callers today. Still, an optional { kind } constructor option defaulting to "video" would be additive (no breaking API change) and would close this now. Otherwise, add a line in the doc comment saying not to use the producer for data tracks yet.
2. The README overstates what readers do. js/loc/README.md:27-28 says "Readers skip that frame", but the example just above uses new Loc.Format(), whose default kind is "data", and Format.decode never skips anything: it returns the marker as an empty-payload frame. Only audio and video consumers (Format("video" | "audio").end(), the hang Consumer) treat it as an endpoint. Line 27 also says every group ends with a marker, but reordered groups and single-frame tails with no learned interval get none. Suggested wording: "Ordered video groups end with an empty duration frame... Format.end() identifies it, and the hang consumer skips it."
3. Older readers. Readers older than @moq/loc 0.2.3 or moq-mux 0.10.0 will treat the empty frame as media. Since @moq/loc is published, this deserves a line in the release notes, even though the wire format itself hasn't changed.
4. Pauses. With no discontinuity(), a keyframe that arrives after a pause becomes the closing group's marker, so its last frame now claims to last for the whole pause. Without the marker that was only an inference. The PR already lists this as a follow-up and js/publish doesn't use this producer, so this is only a reminder to add discontinuity() before any live-capture caller does.

The tests cover the main cases well: a keyframe boundary, the estimated tail, a single frame with no marker, the overlapping keyframe, and a reordered group with the cadence reset. A two-frame data-track test would pin down item 1 whichever way you go.

Verdict: MERGE once CI is green. Items 1 and 2 are cheap to fix here, but nothing blocks.

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: db12bae

Follow-up to d6a632f. No new correctness finding in the compatibility delta. After accounting for main, the producer changes are unchanged. js/loc/src/index.test.ts:155 and js/hang/src/container/consumer.test.ts:845 now use the supported maxDelay option. The optional timestamp read at index.test.ts:169 still feeds exact numeric assertions (:186-195), so a missing timestamp would fail rather than be silently accepted.

Direction remains sound for video. The existing non-video limitation and overly broad README wording are accurately identified in #4937 (comment): #closeGroup emits an empty marker without a kind gate (index.ts:201-214), while Format("data").end() does not recognize it (:50-51). Keep the video-only scope explicit and qualify README.md:27-28 until kind-aware production is added. This is unchanged from the previous reviewed head; no duplicate inline comment added.

Verification: GitHub-only static incremental/source/test review; no Bun tests, typechecks, builds, or playback executed. Exact-head hosted checks remain queued/running. Open state, head, and existing reviews rechecked before submission.

(Written by OpenAI)

[kixelated]

Merge summary for d3cb4a24:

• Merged main twice. The first merge needed a test-only follow-up (db12bae2): maxAge became maxDelay and moq-net frame timestamps became optional.
• 80f7f61a (docs only) scopes the producer to video in its doc comment and the README, per the db12bae reviews: a data reader would see the marker as an empty object. A kind-aware producer is left as a follow-up.
• The second merge resolved a quest/m2/README.md list conflict by keeping main's latency-ledger wording and dropping this quest's entry, since the quest is complete.
• The maintainer accepted the OpenAI review of db12bae2 as covering the docs-only commit and the mechanical merges. just check passes locally.

(Written by Claude Opus 5.5)