Skip to content

Add audio encoder reconfiguration#1362

Merged
kixelated merged 4 commits into
moq-dev:mainfrom
Qizot:audio-encoder-reconfigure
May 21, 2026
Merged

Add audio encoder reconfiguration#1362
kixelated merged 4 commits into
moq-dev:mainfrom
Qizot:audio-encoder-reconfigure

Conversation

@Qizot
Copy link
Copy Markdown
Contributor

@Qizot Qizot commented Apr 29, 2026

For some platforms (looking at you Apple) we can't reliably get the channel count from the media track, this information becomes available only when looking at the data inside of audio worklet callback.

The following changes allow for config creation only after receiving the first batch of samples.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 29, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 29080d33-41c8-4c5f-9dbf-5d0f2d793795

📥 Commits

Reviewing files that changed from the base of the PR and between fb72613 and b704c02.

📒 Files selected for processing (1)
  • js/publish/src/audio/encoder.ts

Walkthrough

The audio encoder now creates its AudioConfig lazily from the first captured audio frame: the worklet message handler reads data.channels.length and calls a new #createConfig(). The prior #runConfig reactive path is removed. serve() defers encoder setup until #config exists and per-frame handling reads the full data.channels array, recomputing #config and resetting lastKeyframe when channel count changes, and skips encoding the first frame of a new configuration. Two Cargo.toml dependency feature lists were reformatted (no behavioral change).

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Add audio encoder reconfiguration' clearly summarizes the main change: enabling the encoder to be reconfigured after receiving initial audio data, which directly addresses the PR's core objective of deferring config creation until the first audio samples arrive.
Description check ✅ Passed The description is directly related to the changeset, explaining the platform-specific challenge with obtaining channel count and how the changes defer encoder configuration until audio data is available, which matches the implementation in the encoder.ts changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified code

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


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

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
js/publish/src/audio/encoder.ts (1)

135-145: Extract the Opus defaults into named constants.

32_000 and 20 are now part of the encoder policy, so keeping them inline makes later tuning harder and hides intent.

♻️ Suggested refactor
+const OPUS_BITRATE_PER_CHANNEL = 32_000;
+const OPUS_FRAME_DURATION_MS = 20 as Time.Milli;
+
 `#createConfig`(worklet: AudioWorkletNode, channelCount: number): Catalog.AudioConfig {
 	return {
 		codec: "opus",
 		sampleRate: Catalog.u53(worklet.context.sampleRate),
 		numberOfChannels: Catalog.u53(channelCount),
-		bitrate: Catalog.u53(channelCount * 32_000),
+		bitrate: Catalog.u53(channelCount * OPUS_BITRATE_PER_CHANNEL),
 		container: { kind: "legacy" } as const,
 		// TODO parse the actual frame duration instead of assuming 20ms.
 		// Opus supports 2.5–60ms but 20ms is the real-time default.
-		jitter: Catalog.u53(20),
+		jitter: Catalog.u53(OPUS_FRAME_DURATION_MS),
 	};
 }
As per coding guidelines, "Avoid using magic numbers; use named constants instead."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@js/publish/src/audio/encoder.ts` around lines 135 - 145, Extract the magic
numbers in `#createConfig` into named constants (e.g.,
DEFAULT_OPUS_BITRATE_PER_CHANNEL = 32000 and DEFAULT_OPUS_FRAME_MS = 20) defined
near the top of the module and replace the inline literals (32_000 and 20) in
the bitrate and jitter calculations with those constants (use
Catalog.u53(DEFAULT_OPUS_BITRATE_PER_CHANNEL * channelCount) for bitrate and
Catalog.u53(DEFAULT_OPUS_FRAME_MS) for jitter) so the encoder policy values are
explicit and easier to tune later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@js/publish/src/audio/encoder.ts`:
- Around line 122-125: The cleanup currently clears the encoder config via
this.#config.set(undefined) but leaves the derived catalog stale; update the
effect.cleanup block (the same place that nulls worklet.port.onmessage and calls
this.#config.set(undefined)) to also reset the derived catalog produced by
`#runCatalog` — e.g., call the catalog-reset method/property (the internal
`#catalog` setter/clear function) so that `#catalog` no longer exposes the previous
rendition after source teardown or reinit.

---

Nitpick comments:
In `@js/publish/src/audio/encoder.ts`:
- Around line 135-145: Extract the magic numbers in `#createConfig` into named
constants (e.g., DEFAULT_OPUS_BITRATE_PER_CHANNEL = 32000 and
DEFAULT_OPUS_FRAME_MS = 20) defined near the top of the module and replace the
inline literals (32_000 and 20) in the bitrate and jitter calculations with
those constants (use Catalog.u53(DEFAULT_OPUS_BITRATE_PER_CHANNEL *
channelCount) for bitrate and Catalog.u53(DEFAULT_OPUS_FRAME_MS) for jitter) so
the encoder policy values are explicit and easier to tune later.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 303c3d0b-f618-4cc8-8dc4-0ca6da4ac682

📥 Commits

Reviewing files that changed from the base of the PR and between 2083546 and fb88a5c.

📒 Files selected for processing (1)
  • js/publish/src/audio/encoder.ts

Comment thread js/publish/src/audio/encoder.ts
Comment thread js/publish/src/audio/encoder.ts Outdated
Comment thread js/publish/src/audio/encoder.ts Outdated
Comment thread js/publish/src/audio/encoder.ts
Qizot added 3 commits May 11, 2026 13:06
For some platforms (looking at you Apple) we can't reliably get the
channel count from the media track, this information becomes available
only when looking at the data inside of audio worklet callback.

The following changes allow for config creation only after receiving the
first batch of samples.
@Qizot Qizot force-pushed the audio-encoder-reconfigure branch from fb88a5c to fb72613 Compare May 11, 2026 12:36
@Qizot Qizot requested a review from kixelated May 11, 2026 12:37
…gure

# Conflicts:
#	js/publish/src/audio/encoder.ts
@kixelated
Copy link
Copy Markdown
Collaborator

yolo

@kixelated kixelated merged commit b876669 into moq-dev:main May 21, 2026
1 check passed
This was referenced May 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants