Skip to content

Conversation

@kixelated
Copy link
Collaborator

@kixelated kixelated commented Sep 5, 2025

require wasn't actually working on Safari. I ended up forking the libav polyfill just to fix Typescript errors. And I fixed the support window to show "partial" instead of "none" on safari.

Summary by CodeRabbit

  • New Features

    • “Partial” support label changed to “Polyfill.”
    • Added VP8/VP9 codec visibility and richer per-codec detection.
  • Bug Fixes

    • Safer capability checks to avoid errors when WebCodecs APIs are absent.
    • More reliable transport/core support detection.
  • Chores

    • Package bumped to 0.4.2; two dependencies renamed and loading switched to dynamic imports.
    • Audio track settings made optional; encoder now derives values from runtime worklet.
    • Workspace build list updated.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 5, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

Bumps js/hang version 0.4.1 → 0.4.2 and renames dependencies (@libav.js/variant-opus → @libav.js/variant-opus-af; libavjs-webcodecs-polyfill → @kixelated/libavjs-webcodecs-polyfill). Support detection now always provides audio.encoding and audio.decoding (no undefined), with new helper return types and guards for missing WebCodecs; UI shows "🟡 Polyfill" for partial webtransport. Replaces require() with dynamic import() when loading LibAV and the polyfill. Vite optimizeDeps updated to the new opus package name. AudioTrackSettings properties made optional and encoder derives settings from the worklet. Adds oxc-resolver to pnpm workspace.


📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a9a8281 and 47f8bcc.

📒 Files selected for processing (2)
  • js/hang/src/publish/audio/index.ts (4 hunks)
  • js/hang/src/support/element.ts (4 hunks)
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch aac-opus

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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
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: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
js/hang/src/util/libav.ts (1)

5-7: Don’t short‑circuit when WebCodecs exist but Opus isn’t supported (Safari). Load the polyfill if Opus encode/decode isn’t supported.

Right now polyfill() returns early whenever AudioEncoder/AudioDecoder exist, which skips the Opus polyfill on Safari where the APIs exist but don’t support Opus. This breaks the fallback path.

Apply:

 export async function polyfill(): Promise<boolean> {
-  if (globalThis.AudioEncoder && globalThis.AudioDecoder) {
-    return true;
-  }
+  const hasEncoder = typeof globalThis.AudioEncoder !== "undefined";
+  const hasDecoder = typeof globalThis.AudioDecoder !== "undefined";
+  if (hasEncoder && hasDecoder) {
+    try {
+      const [enc, dec] = await Promise.all([
+        globalThis.AudioEncoder.isConfigSupported({ codec: "opus", numberOfChannels: 2, sampleRate: 48000 }),
+        globalThis.AudioDecoder.isConfigSupported({ codec: "opus", numberOfChannels: 2, sampleRate: 48000 }),
+      ]);
+      if (enc.supported && dec.supported) return true; // native Opus is available
+    } catch {
+      // fall through to polyfill
+    }
+  }
 
   if (!loading) {
-    console.warn("using Opus polyfill; performance may be degraded");
+    // Only warn when we actually load the polyfill.
+    console.warn("using Opus polyfill; performance may be degraded");
 
     // Load the polyfill and the libav variant we're using.
     // TODO build with AAC support.
-    // I forked libavjs-webcodecs-polyfill to avoid Typescript errors; there's no changes otherwise.
+    // I forked libavjs-webcodecs-polyfill to avoid Typescript errors; no functional changes.

Also applies to: 15-16

js/hang/src/support/element.ts (2)

119-138: Fix summary aggregation logic: “all” should be partial if any dimension is partial

Currently “all” returns full when one of watch/publish is partial. Also, core=partial should cap the summary to partial unless any dimension is none.

Apply:

  #getSummary(support: Full, mode: SupportMode): "full" | "partial" | "none" {
-    const core = support.webtransport;
-    if (core === "none" || mode === "core") return core;
+    const core = support.webtransport;
+    if (mode === "core") return core;
+    if (core === "none") return "none";

     if (mode === "watch") {
       return this.#getWatchSupport(support);
     }

     if (mode === "publish") {
       return this.#getPublishSupport(support);
     }

     const watch = this.#getWatchSupport(support);
     const publish = this.#getPublishSupport(support);

-    if (watch === "none" || publish === "none") return "none";
-    if (watch === "partial" && publish === "partial") return "partial";
-
-    return "full";
+    if (watch === "none" || publish === "none") return "none";
+    if (watch === "full" && publish === "full") return "full";
+    return "partial";
   }

140-165: Audio checks use truthiness and can misclassify “none” as supported

Object.values(...).some(v => v) and .every(v => v) break when mixing booleans (AAC) and string enums (Opus). “none” is truthy, causing false positives. Use explicit checks.

Apply:

   #getWatchSupport(support: Full): "full" | "partial" | "none" {
     if (!support.audio.decoding || !support.video.decoding) return "none";
     if (!support.audio.render || !support.video.render) return "none";
-
-    if (!Object.values(support.audio.decoding).some((v) => v)) return "none";
-    if (!Object.values(support.video.decoding).some((v) => v.software || v.hardware)) return "none";
-
-    if (!Object.values(support.audio.decoding).every((v) => v)) return "partial";
-    if (!Object.values(support.video.decoding).every((v) => v.software || v.hardware)) return "partial";
+    const hasAudioDec =
+      support.audio.decoding.aac === true ||
+      support.audio.decoding.opus === "full" ||
+      support.audio.decoding.opus === "partial";
+    if (!hasAudioDec) return "none";
+    if (!Object.values(support.video.decoding).some((v) => !!(v?.software || v?.hardware))) return "none";
+
+    const allAudioFull =
+      support.audio.decoding.aac === true &&
+      support.audio.decoding.opus === "full";
+    if (!allAudioFull) return "partial";
+    if (!Object.values(support.video.decoding).every((v) => v?.software || v?.hardware)) return "partial";
 
     return "full";
   }
 
   #getPublishSupport(support: Full): "full" | "partial" | "none" {
     if (!support.audio.encoding || !support.video.encoding) return "none";
     if (!support.audio.capture) return "none";
-
-    if (!Object.values(support.audio.encoding).some((v) => v)) return "none";
-    if (!Object.values(support.video.encoding).some((v) => v.software || v.hardware)) return "none";
+    const hasAudioEnc =
+      support.audio.encoding.aac === true ||
+      support.audio.encoding.opus === "full" ||
+      support.audio.encoding.opus === "partial";
+    if (!hasAudioEnc) return "none";
+    if (!Object.values(support.video.encoding).some((v) => !!(v?.software || v?.hardware))) return "none";
 
     if (support.video.capture === "partial") return "partial";
-
-    if (!Object.values(support.video.encoding).some((v) => v.hardware)) return "partial";
+    const allAudioFull =
+      support.audio.encoding.aac === true &&
+      support.audio.encoding.opus === "full";
+    if (!allAudioFull) return "partial";
+    if (!Object.values(support.video.encoding).some((v) => v?.hardware)) return "partial";
 
     return "full";
   }
🧹 Nitpick comments (3)
js/hang/src/support/index.ts (2)

125-132: Compute audio support in parallel to reduce latency of isSupported().

These await calls are sequential; run them concurrently to speed up feature detection.

-      encoding: {
-        aac: await audioEncoderSupported("aac"),
-        opus: (await audioEncoderSupported("opus")) ? "full" : "partial",
-      },
-      decoding: {
-        aac: await audioDecoderSupported("aac"),
-        opus: (await audioDecoderSupported("opus")) ? "full" : "partial",
-      },
+      ...(await (async () => {
+        const [aacEnc, opusEnc, aacDec, opusDec] = await Promise.all([
+          audioEncoderSupported("aac"),
+          audioEncoderSupported("opus"),
+          audioDecoderSupported("aac"),
+          audioDecoderSupported("opus"),
+        ]);
+        return {
+          encoding: { aac: aacEnc, opus: opusEnc ? "full" : "partial" as const },
+          decoding: { aac: aacDec, opus: opusDec ? "full" : "partial" as const },
+        };
+      })()),

24-38: Semantics mismatch in Audio type (boolean for AAC vs Partial for Opus).

Intentional or not, this mixes boolean and tri-state in one type. If UI relies on uniform rendering, consider normalizing to a single representation (e.g., both as Partial, with AAC always “full”/“none”). Low priority if consumers already account for this.

js/hang/src/support/element.ts (1)

239-244: Make the “Polyfill” label context-aware (don’t call WebTransport “Polyfill”)

Changing the generic partial label to “Polyfill” affects WebTransport and capture rows, which aren’t polyfilled. Show “Partial” there, and keep “Polyfill” for Opus.

Apply:

-    const partial = (value: Partial | undefined) =>
-      value === "full" ? "🟢 Full" : value === "partial" ? "🟡 Polyfill" : "🔴 None";
+    const partial = (value: Partial | undefined, label: "Partial" | "Polyfill" = "Partial") =>
+      value === "full" ? "🟢 Full" : value === "partial" ? `🟡 ${label}` : "🔴 None";

And update usages:

-    addRow("WebTransport", "", partial(support.webtransport));
+    addRow("WebTransport", "", partial(support.webtransport, "Partial"));
@@
-        addRow("", "Video", partial(support.video.capture));
-        addRow("Encoding", "Opus", partial(support.audio.encoding?.opus));
+        addRow("", "Video", partial(support.video.capture, "Partial"));
+        addRow("Encoding", "Opus", partial(support.audio.encoding?.opus, "Polyfill"));
@@
-        addRow("Decoding", "Opus", partial(support.audio.decoding?.opus));
+        addRow("Decoding", "Opus", partial(support.audio.decoding?.opus, "Polyfill"));

Also applies to: 282-306

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 0fb4823 and 61125ba.

⛔ Files ignored due to path filters (1)
  • js/pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (4)
  • js/hang/package.json (2 hunks)
  • js/hang/src/support/element.ts (2 hunks)
  • js/hang/src/support/index.ts (6 hunks)
  • js/hang/src/util/libav.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{rs,js}/hang/**

📄 CodeRabbit inference engine (CLAUDE.md)

All media logic belongs in the hang layer (rs/hang and js/hang)

Files:

  • js/hang/src/util/libav.ts
  • js/hang/package.json
  • js/hang/src/support/element.ts
  • js/hang/src/support/index.ts
js/**

📄 CodeRabbit inference engine (CLAUDE.md)

Place JS/TS packages under the js/ directory

Files:

  • js/hang/src/util/libav.ts
  • js/hang/package.json
  • js/hang/src/support/element.ts
  • js/hang/src/support/index.ts
🧬 Code graph analysis (1)
js/hang/src/support/index.ts (2)
js/hang/src/publish/audio/index.ts (1)
  • Audio (63-287)
js/hang/src/watch/audio/index.ts (1)
  • Audio (39-234)
🔇 Additional comments (4)
js/hang/package.json (2)

4-4: Version bump looks good.

0.4.2 accurately reflects the Opus fallback change scope.


47-47: Verified: no stale imports and lockfile reflects the new scoped package.
All code references to libavjs-webcodecs-polyfill have been updated to @kixelated/libavjs-webcodecs-polyfill, and your pnpm-lock.yaml contains only the new package.

js/hang/src/support/index.ts (2)

76-94: LGTM on the video decoder capability shape.

Firefox hardware detection caveat is handled via unknown. Clean and explicit return type.


96-118: LGTM on the video encoder capability shape.

Consistent with decoder path; explicit hardware unknown handling is good.

Comment on lines +52 to +54
async function audioDecoderSupported(codec: keyof typeof CODECS): Promise<boolean> {
if (!globalThis.AudioDecoder) return false;

Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Guard against thrown errors from isConfigSupported (return false on exceptions).

Some browsers throw on unknown codec configs. Add try/catch to keep isSupported() resilient.

 async function audioDecoderSupported(codec: keyof typeof CODECS): Promise<boolean> {
   if (!globalThis.AudioDecoder) return false;
-  const res = await AudioDecoder.isConfigSupported({
-    codec: CODECS[codec],
-    numberOfChannels: 2,
-    sampleRate: 48000,
-  });
-  return res.supported === true;
+  try {
+    const res = await AudioDecoder.isConfigSupported({
+      codec: CODECS[codec],
+      numberOfChannels: 2,
+      sampleRate: 48000,
+    });
+    return res.supported === true;
+  } catch {
+    return false;
+  }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async function audioDecoderSupported(codec: keyof typeof CODECS): Promise<boolean> {
if (!globalThis.AudioDecoder) return false;
async function audioDecoderSupported(codec: keyof typeof CODECS): Promise<boolean> {
if (!globalThis.AudioDecoder) return false;
try {
const res = await AudioDecoder.isConfigSupported({
codec: CODECS[codec],
numberOfChannels: 2,
sampleRate: 48000,
});
return res.supported === true;
} catch {
return false;
}
}
🤖 Prompt for AI Agents
In js/hang/src/support/index.ts around lines 52 to 54, the call to
AudioDecoder.isConfigSupported can throw for unknown/invalid codec configs; wrap
the isConfigSupported call in a try/catch and return false if it throws (keep
the existing early return if globalThis.AudioDecoder is missing), so the
function remains resilient and never propagates exceptions from
isConfigSupported.

Comment on lines +64 to +66
async function audioEncoderSupported(codec: keyof typeof CODECS): Promise<boolean> {
if (!globalThis.AudioEncoder) return false;

Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Mirror the try/catch in encoder support for symmetry and robustness.

 async function audioEncoderSupported(codec: keyof typeof CODECS): Promise<boolean> {
   if (!globalThis.AudioEncoder) return false;
-  const res = await AudioEncoder.isConfigSupported({
-    codec: CODECS[codec],
-    numberOfChannels: 2,
-    sampleRate: 48000,
-  });
-  return res.supported === true;
+  try {
+    const res = await AudioEncoder.isConfigSupported({
+      codec: CODECS[codec],
+      numberOfChannels: 2,
+      sampleRate: 48000,
+    });
+    return res.supported === true;
+  } catch {
+    return false;
+  }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async function audioEncoderSupported(codec: keyof typeof CODECS): Promise<boolean> {
if (!globalThis.AudioEncoder) return false;
async function audioEncoderSupported(codec: keyof typeof CODECS): Promise<boolean> {
if (!globalThis.AudioEncoder) return false;
try {
const res = await AudioEncoder.isConfigSupported({
codec: CODECS[codec],
numberOfChannels: 2,
sampleRate: 48000,
});
return res.supported === true;
} catch {
return false;
}
}
🤖 Prompt for AI Agents
In js/hang/src/support/index.ts around lines 64 to 66, the audioEncoderSupported
function only checks for globalThis.AudioEncoder and returns false but lacks the
try/catch used in the encoder support path; update the function to mirror the
encoder support: wrap the AudioEncoder usage/instantiation and capability check
in a try/catch, return false on any thrown error and true only when the
capability check succeeds, and ensure any temporary instances are closed/cleaned
up in the try block.

Comment on lines 15 to 16
loading = Promise.all([import("@libav.js/variant-opus"), import("@kixelated/libavjs-webcodecs-polyfill")]).then(
async ([opus, libav]) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Dynamic import default interop: likely passing the module namespace instead of the LibAV factory.

@libav.js/variant-opus typically default-exports the factory. With dynamic import, you likely need opus.default. Similarly, ensure load is taken from the right export on the polyfill module.

Apply:

- loading = Promise.all([import("@libav.js/variant-opus"), import("@kixelated/libavjs-webcodecs-polyfill")]).then(
-   async ([opus, libav]) => {
-     await libav.load({
-       LibAV: opus,
-       polyfill: true,
-     });
+ loading = Promise.all([
+   import("@libav.js/variant-opus"),
+   import("@kixelated/libavjs-webcodecs-polyfill"),
+ ]).then(async ([opusMod, polyfillMod]) => {
+   const LibAV: any = (opusMod as any).default ?? opusMod;
+   const load: any = (polyfillMod as any).load ?? (polyfillMod as any).default?.load;
+   if (typeof load !== "function") throw new Error("Invalid polyfill module: missing load()");
+   await load({
+     LibAV,
+     polyfill: true,
+   });
     return true;
-   },
- );
+ });
🤖 Prompt for AI Agents
In js/hang/src/util/libav.ts around lines 15-16, the dynamic import is likely
passing module namespace objects instead of the actual default-exported factory
and load function; change the code to use the default export from
@libav.js/variant-opus (e.g., use opus.default as the LibAV factory) and extract
the correct export (default or named) from the polyfill module (e.g.,
libav.default or libav.load) so you pass the actual factory and loader instead
of the module namespace when calling/initializing LibAV.

@kixelated kixelated changed the title Fix the Opus fallback Fix the Opus fallback for Safari Sep 5, 2025
@kixelated kixelated enabled auto-merge (squash) September 5, 2025 17:36
@kixelated kixelated merged commit 7c7e61b into main Sep 5, 2025
1 check passed
@kixelated kixelated deleted the aac-opus branch September 5, 2025 17:39
Copy link
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

♻️ Duplicate comments (1)
js/hang/src/util/libav.ts (1)

15-24: Dynamic import default interop bug: using module namespaces instead of the LibAV factory and polyfill loader.

With dynamic import, you must extract default exports. As-is, LibAV: opus and libav.load(...) will pass module objects on most bundlers and fail at runtime.

Apply:

-        loading = Promise.all([
-            import("@libav.js/variant-opus-af"),
-            import("@kixelated/libavjs-webcodecs-polyfill"),
-        ]).then(async ([opus, libav]) => {
-            await libav.load({
-                LibAV: opus,
-                polyfill: true,
-            });
-            return true;
-        });
+        loading = Promise.all([
+            import("@libav.js/variant-opus-af"),
+            import("@kixelated/libavjs-webcodecs-polyfill"),
+        ])
+            .then(async ([opusMod, polyfillMod]) => {
+                const LibAV: any = (opusMod as any).default ?? opusMod;
+                const load: any =
+                    (polyfillMod as any).load ??
+                    (polyfillMod as any).default?.load;
+                if (typeof load !== "function") {
+                    throw new Error("Invalid polyfill module: missing load()");
+                }
+                await load({ LibAV, polyfill: true });
+                return true;
+            })
+            .catch((err) => {
+                console.error("failed to load Opus polyfill", err);
+                loading = undefined;
+                return false;
+            });

Additionally (outside the changed range): make the early-return check codec-aware so Safari with partial WebCodecs still loads the Opus polyfill.

// Replace Lines 5-7:
if (globalThis.AudioEncoder?.isConfigSupported) {
  try {
    const { supported } = await globalThis.AudioEncoder.isConfigSupported({
      codec: "opus",
      sampleRate: 48000,
      numberOfChannels: 2,
    });
    if (supported) return true;
  } catch {
    // fall through
  }
}

To confirm export shapes without running the app:

What are the runtime exports of "@libav.js/variant-opus-af" and "@kixelated/libavjs-webcodecs-polyfill"? Do they default-export the LibAV factory and expose a named `load()` respectively?
🧹 Nitpick comments (4)
js/hang/src/publish/audio/index.ts (4)

141-142: Force explicit channel layout on the worklet for cross-browser consistency.

Safari can ignore channelCount unless channelCountMode is "explicit". Set both.

Apply:

-                channelCount: settings.channelCount ?? root.channelCount,
+                channelCount: settings.channelCount ?? root.channelCount,
+                channelCountMode: "explicit",
+                channelInterpretation: "speakers",

181-186: Guard configure() with isConfigSupported and tune bitrate per channel.

Avoid runtime errors and pick sane defaults per channel (e.g., 48–64 kbps). Also carry description when available.

Apply:

-        const config = {
+        const config = {
           // TODO get codec and description from decoderConfig
           codec: "opus",
           sampleRate: u53(worklet.context.sampleRate),
           numberOfChannels: u53(worklet.channelCount),
-          // TODO configurable
-          bitrate: u53(worklet.channelCount * 32_000),
+          // TODO configurable
+          bitrate: u53(worklet.channelCount * 48_000),
           // TODO there's a bunch of advanced Opus settings that we should use.
         };
@@
-        encoder.configure(config);
+        try {
+          const { supported, config: normalized } =
+            (AudioEncoder as any).isConfigSupported
+              ? await AudioEncoder.isConfigSupported(config as any)
+              : { supported: true, config };
+          if (!supported) throw new Error("Opus config not supported");
+          encoder.configure(normalized as any);
+        } catch (e) {
+          console.error("unsupported Opus encoder config", e, config);
+          return;
+        }

Also applies to: 220-222


214-216: Consider surfacing encoder errors to the UI/metrics.

Log is fine; additionally emit a user-facing signal/metric to aid debugging in demos.


224-225: Slice with explicit min to avoid surprises if worklet reports 0 channels.

Defensive, cheap, and avoids runtime errors if upstream misreports.

Apply:

-                const channels = data.channels.slice(0, worklet.channelCount);
+                const channels = data.channels.slice(0, Math.max(1, worklet.channelCount));
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 61125ba and a9a8281.

⛔ Files ignored due to path filters (1)
  • js/pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (5)
  • js/hang-demo/vite.config.ts (1 hunks)
  • js/hang/package.json (2 hunks)
  • js/hang/src/publish/audio/index.ts (4 hunks)
  • js/hang/src/util/libav.ts (1 hunks)
  • js/pnpm-workspace.yaml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • js/hang/package.json
🧰 Additional context used
📓 Path-based instructions (3)
js/**

📄 CodeRabbit inference engine (CLAUDE.md)

Place JS/TS packages under the js/ directory

Files:

  • js/hang-demo/vite.config.ts
  • js/pnpm-workspace.yaml
  • js/hang/src/util/libav.ts
  • js/hang/src/publish/audio/index.ts
js/pnpm-workspace.yaml

📄 CodeRabbit inference engine (CLAUDE.md)

Use pnpm workspaces for JS/TS; manage package layout via js/pnpm-workspace.yaml

Files:

  • js/pnpm-workspace.yaml
{rs,js}/hang/**

📄 CodeRabbit inference engine (CLAUDE.md)

All media logic belongs in the hang layer (rs/hang and js/hang)

Files:

  • js/hang/src/util/libav.ts
  • js/hang/src/publish/audio/index.ts
🧠 Learnings (1)
📚 Learning: 2025-08-13T21:30:35.001Z
Learnt from: CR
PR: kixelated/moq#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-13T21:30:35.001Z
Learning: Applies to js/pnpm-workspace.yaml : Use pnpm workspaces for JS/TS; manage package layout via js/pnpm-workspace.yaml

Applied to files:

  • js/pnpm-workspace.yaml
🔇 Additional comments (2)
js/pnpm-workspace.yaml (1)

14-14: Keep oxc-resolver in onlyBuiltDependencies – its published package.json defines a postinstall script (napi-postinstall oxc-resolver 11.7.1 check), so install-time build is required.

js/hang/src/publish/audio/index.ts (1)

40-49: No further action needed – all optional audio settings are either unused or accessed with a fallback (channelCount: settings.channelCount ?? root.channelCount), so there are no unchecked direct reads of autoGainControl, echoCancellation, noiseSuppression, or sampleSize.

Comment on lines +24 to 26
// No idea why this needs to be done, but I don't want to figure it out.
exclude: ["@libav.js/variant-opus-af"],
},
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Replace “No idea why…” with rationale and exclude both dynamic-imported libs.

Document why Vite pre-bundling breaks here and exclude the forked polyfill too to avoid accidental optimizeDeps scanning/pre-bundling.

Apply:

-    // No idea why this needs to be done, but I don't want to figure it out.
-    exclude: ["@libav.js/variant-opus-af"],
+    // Prevent Vite from trying to pre-bundle LibAV/polyfill; they must be loaded at runtime.
+    // Dynamic import + ESM default interop are required by the polyfill loader.
+    exclude: ["@libav.js/variant-opus-af", "@kixelated/libavjs-webcodecs-polyfill"],

Run to verify both are present in the lockfile:


🏁 Script executed:

#!/bin/bash
rg -n "@libav.js/variant-opus-af|@kixelated/libavjs-webcodecs-polyfill" js -g "!**/dist/**"

Length of output: 847


Provide rationale and exclude both dynamic-imported LibAV modules in vite.config.ts

Replace the comment and update exclude in js/hang-demo/vite.config.ts:

-    // No idea why this needs to be done, but I don't want to figure it out.
-    exclude: ["@libav.js/variant-opus-af"],
+    // Prevent Vite from pre-bundling LibAV and its polyfill loader; both must load at runtime via dynamic import + ESM interop.
+    exclude: ["@libav.js/variant-opus-af", "@kixelated/libavjs-webcodecs-polyfill"],
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// No idea why this needs to be done, but I don't want to figure it out.
exclude: ["@libav.js/variant-opus-af"],
},
// Prevent Vite from pre-bundling LibAV and its polyfill loader; both must load at runtime via dynamic import + ESM interop.
exclude: ["@libav.js/variant-opus-af", "@kixelated/libavjs-webcodecs-polyfill"],
},
🤖 Prompt for AI Agents
In js/hang-demo/vite.config.ts around lines 24 to 26, replace the vague "// No
idea why this needs to be done..." comment with a brief rationale explaining
that libav variant modules are dynamically imported and must be excluded from
Vite's dependency pre-bundling to avoid runtime resolution/bundle issues, and
update the exclude array to include both dynamic-imported modules, e.g.
"@libav.js/variant-opus" and "@libav.js/variant-opus-af".

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