Repository navigation
feat(publish): pack several audio frames per group on release (backport #4910) - #7290
Conversation
#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>
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. |
kixelated
left a comment
There was a problem hiding this comment.
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)
|
Merge summary:
(Written by Claude Opus 5.5) |
Problem
Each Opus frame on
releaseis 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 addedAudio.Encoder'sgroupDurationonmain, 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 asmain.It does not cherry-pick.
release's encoder writes each frame withtrack.writeFrame(a closed single-frame group) and writes its demand-gap marker the same way. It has noContainer.Legacy.Producer,#live,#cut, orfade. So:groupDurationis validated in#runConfig, asmaindoes forfade. A negative or non-finite value throws there, and the rendition drops out of the catalog until the value is fixed.groupDurationpast the open group's start, closes it and opens the next one.releasewrites no marker there, and the rebuilt AudioEncoder restarts its output clock.main, the group instead stays open until the next frame.The test harness now wraps
appendGrouprather thantrack.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 frommain's test, usingrelease's demand gap as the break), and refusing an invalid duration.doc/lib/js/publish.mdgainsmain's paragraph, without thejust bench-audiosentence (releasehas no such bench). The pipeline-stop case is spelled out.js/publish/README.mdlists no encoder props, so it is unchanged.js/publish/api/index.api.mdadds the property and prop.Skipped per the Plan: moq-dev/moq#4910's Rust
moq-audio, bench, and Nightly changes, and the catalogdescriptionre-subscribe.Impact
Audio.Encoder.groupDurationandEncoderProps.groupDuration.@moq/publishis 0.5.4 onreleasefrom moq-dev/moq#7276, published without this change. Per the maintainer (2026-10-30), it ships as@moq/publish0.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/publishand its dependents).bun testinjs/publish: 166 pass, 0 fail, including the three new encoder tests.MOQ_API_REPORT=update bun run buildregeneratedapi/index.api.mdandapi/element.api.md. Both diffs are additive only.Review
Codex
gpt-6.1-sol, medium effort, viaquest review --base origin/release(quest pinned at01d051cfbymain'sflake.nix), headbcd6040fd6, two rounds.groupDurationwith 2.5 ms Opus frames reachesMAX_GROUP_FRAMES(8192) and throwsGroupTooLarge. 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, andmainhas the same gap: validation atorigin/main:js/publish/src/audio/encoder.ts:473-476and duration-only rollover at:669. A release-only guard would makereleasediverge frommain. Listed as a follow-up below.Since then,
272c4ee48dmergesorigin/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/releaseandquest check --base origin/releasepass on it.Choices made without asking
mainhas moved its default toMilli(20)(origin/main:js/publish/src/audio/encoder.ts:264), which is still one group per frame at the default 20 ms Opus frame.main.releasewrites no marker there.Follow-ups
main, cap audio groups atMAX_GROUP_FRAMES/MAX_GROUP_CACHE_BYTES(roll over, or reject the duration), then backport.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)