Skip to content

feat(publish): pack several audio frames per group on release (backport #4910) - #7290

Merged
kixelated merged 2 commits into
releasefrom
backport/audio-group-duration
Oct 30, 2026
Merged

kixelated merged 2 commits into
releasefrom
backport/audio-group-duration

Conversation

@kixelated

@kixelated kixelated commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Each Opus frame on release is its own group, so a relay pays a stream per 20 ms frame. On relays that end a subscription when a newer group starts while an older one is still arriving (cloudflare/moq-rs#238), that trips often. moq-dev/moq#4910 added Audio.Encoder's groupDuration on main, but no npm release carries it (moq-dev/moq#7267). A longer group makes the bug rarer, but congestion near a group boundary can still trigger it.

Approach

Port the JS half of moq-dev/moq#4910 onto release's encoder, per the quest's Plan. It defaults to 0, one group per frame, same as main.

It does not cherry-pick. release's encoder writes each frame with track.writeFrame (a closed single-frame group) and writes its demand-gap marker the same way. It has no Container.Legacy.Producer, #live, #cut, or fade. So:

  • groupDuration is validated in #runConfig, as main does for fade. A negative or non-finite value throws there, and the rendition drops out of the catalog until the value is fixed.
  • The encoder keeps the open group with its track and first timestamp. A frame on a different track, or at least groupDuration past the open group's start, closes it and opens the next one.
  • The demand-gap marker closes the open group before it is written, so frames after the gap start a fresh group. This is the "reset the start at a discontinuity" step.
  • Tearing down the encode pipeline (disabled, capture or config change, fatal error) also closes the open group. release writes no marker there, and the rebuilt AudioEncoder restarts its output clock.
  • At 0, each frame's group closes as soon as it is written, the same as before. The default path's wire output matches 0.5.3. On main, the group instead stays open until the next frame.

The test harness now wraps appendGroup rather than track.writeFrame, so it records frames per group and each group's close state. New tests cover the default (every group closed at once), packing to a 100 ms minimum with a restart after a demand gap (ported from main's test, using release's demand gap as the break), and refusing an invalid duration.

doc/lib/js/publish.md gains main's paragraph, without the just bench-audio sentence (release has no such bench). The pipeline-stop case is spelled out. js/publish/README.md lists no encoder props, so it is unchanged. js/publish/api/index.api.md adds the property and prop.

Skipped per the Plan: moq-dev/moq#4910's Rust moq-audio, bench, and Nightly changes, and the catalog description re-subscribe.

Impact

  • Public API: additive. Audio.Encoder.groupDuration and EncoderProps.groupDuration.
  • Wire: none at the default. With a positive duration, audio groups carry several frames, which every subscriber already reads.
  • Versions: none in this PR. @moq/publish is 0.5.4 on release from moq-dev/moq#7276, published without this change. Per the maintainer (2026-10-30), it ships as @moq/publish 0.5.5 in the next combined patch cut, which the maintainer bumps and cuts.

Validation

  • nix develop -c just check origin/release: passed (lint, typecheck, biome, build, and tests for @moq/publish and its dependents).
  • bun test in js/publish: 166 pass, 0 fail, including the three new encoder tests.
  • MOQ_API_REPORT=update bun run build regenerated api/index.api.md and api/element.api.md. Both diffs are additive only.

Review

Codex gpt-6.1-sol, medium effort, via quest review --base origin/release (quest pinned at 01d051cf by main's flake.nix), head bcd6040fd6, two rounds.

  • Round 1, medium: "Reject group durations exceeding the resolved codec's frame budget". A 30 s groupDuration with 2.5 ms Opus frames reaches MAX_GROUP_FRAMES (8192) and throws GroupTooLarge. Refuted as out of scope. The Plan says to backport feat(audio): pack several frames per group with a minimum group duration #4910 as-is, and main has the same gap: validation at origin/main:js/publish/src/audio/encoder.ts:473-476 and duration-only rollover at :669. A release-only guard would make release diverge from main. Listed as a follow-up below.
  • Round 2: no findings.

Since then, 272c4ee48d merges origin/release (fa7aed2501, with moq-dev/moq#7389) into the branch, with no conflicts. That is a conflict-free base merge, so it needs no new review. nix develop -c just check origin/release and quest check --base origin/release pass on it.

Choices made without asking

  • The default stays 0, as the Plan decided. Since feat(audio): pack several frames per group with a minimum group duration #4910, main has moved its default to Milli(20) (origin/main:js/publish/src/audio/encoder.ts:264), which is still one group per frame at the default 20 ms Opus frame.
  • At 0, each group closes as soon as it is written (unchanged from 0.5.3), rather than staying open until the next frame as on main.
  • Stopping the encode pipeline closes the open group, since release writes no marker there.

Follow-ups

  • On main, cap audio groups at MAX_GROUP_FRAMES / MAX_GROUP_CACHE_BYTES (roll over, or reject the duration), then backport.

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

#4910)

Port Audio.Encoder's groupDuration from main onto release's encoder. The
default of 0 keeps one closed group per frame. A positive duration holds the
group open until a frame at least that far past its start, a demand-gap
marker, or the encode pipeline stopping.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated marked this pull request as ready for review October 30, 2026 06:34
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-30T20:30:29.981139Z 272c4ee New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed commit bcd6040 against release base de0a2b0.

No additional actionable findings. The release-specific adaptation preserves immediately closed single-frame groups at the default, closes the packed group before a demand-gap marker, and closes it on pipeline teardown. Keeping grouping opt-in is a reasonable narrow backport; it reduces stream churn, not the underlying relay boundary bug.

Known limitation, not a new finding: the duration-only rollover at js/publish/src/audio/encoder.ts:585-587 exposes the already documented group-budget issue to release users. A 30 s group with 2.5 ms frames reaches the 8193rd write at 20.48 s, where GroupTooLarge aborts the group. The main-first fix/backport remains necessary for those configurations; this review does not treat the refuted local-review item as resolved.

The new tests cover default closure, packing/demand gaps and invalid-duration recovery. A positive-duration disable/config-change closure assertion would strengthen lifecycle coverage. Static GitHub review plus an independent source check only; I did not run Bun, Nix, browser/WebCodecs or relay tests, and have not verified the reported performance or test results.

(Written by OpenAI)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Merge summary:

  • The maintainer decided on 2026-10-30 to merge this unbumped. @moq/publish 0.5.5 carries it in the next combined patch cut, alongside moq-dev/moq#7389, moq-dev/moq#7298, and the dot-segment and auth-redirect backport. The maintainer bumps the version and cuts the release.
  • Merged origin/release (fa7aed2501) into the branch with no conflicts. The new head is 272c4ee48d. just check origin/release and quest check pass locally.
  • The review record covers bcd6040fd6 plus that conflict-free base merge. The only finding was the MAX_GROUP_FRAMES budget, which main shares and which stays a follow-up for main first.

(Written by Claude Opus 5.5)

@kixelated
kixelated enabled auto-merge October 30, 2026 20:29
@kixelated
kixelated merged commit d0cff5a into release Oct 30, 2026
32 of 34 checks passed
@kixelated
kixelated deleted the backport/audio-group-duration branch October 30, 2026 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant