Add audio encoder reconfiguration#1362
Conversation
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.
WalkthroughThe audio encoder's configuration logic has been refactored to use lazy initialization. Instead of setting configuration reactively via 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
js/publish/src/audio/encoder.ts (1)
135-145: Extract the Opus defaults into named constants.
32_000and20are now part of the encoder policy, so keeping them inline makes later tuning harder and hides intent.As per coding guidelines, "Avoid using magic numbers; use named constants instead."♻️ 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), }; }🤖 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
📒 Files selected for processing (1)
js/publish/src/audio/encoder.ts
| effect.cleanup(() => { | ||
| worklet.port.onmessage = null; | ||
| this.#config.set(undefined); | ||
| }); |
There was a problem hiding this comment.
Clear the derived catalog when the config is reset.
This cleanup drops #config, but #runCatalog() never mirrors that back to #catalog. After source teardown or reinit, catalog will still expose the last rendition until another config arrives.
🩹 Suggested fix
`#runCatalog`(effect: Effect): void {
const config = effect.get(this.#config);
- if (!config) return;
+ if (!config) {
+ effect.set(this.#catalog, undefined);
+ return;
+ }
const catalog: Catalog.Audio = {
renditions: { [Encoder.TRACK]: config },
};🤖 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 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.
There was a problem hiding this comment.
Can you make sure #runCatalog uses this #config signal?
There was a problem hiding this comment.
@kixelated, the fix is straightforward — #runCatalog already calls effect.get(this.#config) so it re-runs whenever #config changes, but the early return on a falsy config leaves #catalog stale. Clearing it before returning keeps them in sync:
`#runCatalog`(effect: Effect): void {
const config = effect.get(this.#config);
if (!config) {
effect.set(this.#catalog, undefined);
return;
}
const catalog: Catalog.Audio = {
renditions: { [Encoder.TRACK]: config },
};
effect.set(this.#catalog, catalog);
}Now when #config is reset to undefined (e.g., during source cleanup), #catalog will immediately be cleared as well instead of exposing a stale rendition.
🧠 Learnings used
Learnt from: kixelated
Repo: moq-dev/moq PR: 948
File: js/publish/vite.config.ts:14-16
Timestamp: 2026-02-13T18:36:03.315Z
Learning: In JS projects that publish web components (custom elements) implemented with a framework (e.g., Solid.js), ensure the framework is bundled into the library output rather than externalized. This means in build config (e.g., vite.config.ts) avoid marking the framework as external and configure the library build to include the framework so consumers do not need to install unused framework dependencies. Verify via build output: the published bundle contains framework code, and no externalized externals for the framework are present. This differs from libraries exporting framework components for direct consumption, where externalizing the framework can be appropriate.
|
|
||
| console.debug("encoding audio", config); | ||
| encoder.configure(config); | ||
| let config = this.#config.peek(); |
There was a problem hiding this comment.
Peek is racey.
If we really need the config, we should break early if effect.get(this.#config) is undefined. This function will rerun when it's set, then we can properly configure the encoder and stuff.
|
|
||
| // The information about channels count can be unreliable on different platforms (Apple's safari). | ||
| // Try to get the first audio frame and only then create the configuration. | ||
| worklet.port.onmessage = ({ data }: { data: Capture.AudioFrame }) => { |
There was a problem hiding this comment.
It's a little spooky setting onmessage in two places.
You shuld use
effect.event(worklet.port, "message", (data) => {It'll unregister the event on cleanup too.
| effect.cleanup(() => { | ||
| worklet.port.onmessage = null; | ||
| this.#config.set(undefined); | ||
| }); |
There was a problem hiding this comment.
Can you make sure #runCatalog uses this #config signal?
2 participants
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.