fix(audio): code-review fixes — clip-before-duck, phaser lanes, rate reschedule

.fallowrc.jsonc

@@ -149,13 +149,6 @@
       "file": "packages/studio/src/utils/studioHelpers.ts",
       "exports": ["resolveDroppedAssetDimensions"],
     },
-    // Audio FX worklets sit near the bottom of the audio stack: the worklet
-    // source and its test reset are consumed by the runtime and engine PRs
-    // upstack, so a per-PR audit against the merge base sees them as unused.
-    {
-      "file": "packages/core/src/audio/audioFxWorklets.ts",
-      "exports": ["AUDIO_FX_WORKLET_SOURCE", "__resetAudioFxWorkletsForTests"],
-    },
     {
       "file": "packages/core/src/audio/audioFxGraph.ts",
       "exports": ["ensureAudioFxWorklets"],

packages/core/package.json

@@ -549,7 +549,7 @@
   },
   "scripts": {
     "build": "bun run build:hyperframes-runtime && bun run build:position-edits-render && bun run build:audio-fx-runtime && tsc && tsx scripts/rewrite-esm-extensions.ts",
-    "test": "bun run check:position-edits-render && vitest run",
+    "test": "bun run check:position-edits-render && bun run build:audio-fx-runtime && vitest run",
     "test:watch": "vitest",
     "test:coverage": "vitest run --coverage",
     "test:runtime-coverage": "vitest run --coverage src/runtime",

packages/core/src/audio/audioFxGraph.test.ts

@@ -155,6 +155,16 @@ describe("buildFxNode", () => {
     expect(workletNodes[0]!.messages[0]).toMatchObject({ threshold: -30 });
   });
 
+  it("tells a worklet processor to retire on dispose, not just disconnect it", () => {
+    // Disconnecting leaves the processor alive — it lives until `process()`
+    // returns false — so every rebuild that dropped a worklet effect left one
+    // running on the audio thread for the rest of the session.
+    workletNodes.length = 0;
+    const h = buildFxNode(asCtx(ctx()), "compressor", defaultAudioFxParams("compressor"));
+    h.dispose();
+    expect(workletNodes[0]!.messages).toEqual([{ __hfDispose: true }]);
+  });
+
   it("rebuilds the saturation curve for the selected shape", () => {
     const c = ctx();
     buildFxNode(asCtx(c), "saturate", { ...defaultAudioFxParams("saturate"), type: "hard" });
@@ -206,6 +216,39 @@ describe("buildFxChain", () => {
     expect(c.created.find((x) => x.kind === "biquad")!.frequency.value).toBe(2000);
   });
 
+  it("lines new values up against the built nodes, skipping a bypassed one", () => {
+    // A bypassed node is not in the graph, so the update has to walk the
+    // ENABLED nodes to stay aligned with what was built. Walking `next.nodes`
+    // instead shifts everything after the bypass by one and pushes each node's
+    // parameters into its neighbour — and the shape is unchanged either way,
+    // so nothing forces a rebuild that would hide it.
+    const c = ctx();
+    const withBypass = (frequency: number): HfAudioFxChain => ({
+      version: 1,
+      nodes: [
+        {
+          type: "highpass",
+          enabled: true,
+          params: { ...defaultAudioFxParams("highpass"), frequency: 100 },
+        },
+        { type: "peaking", enabled: false, params: defaultAudioFxParams("peaking") },
+        {
+          type: "lowpass",
+          enabled: true,
+          params: { ...defaultAudioFxParams("lowpass"), frequency },
+        },
+      ],
+    });
+    const h = buildFxChain(asCtx(c), withBypass(8000));
+    const biquads = c.created.filter((n) => n.kind === "biquad");
+    expect(biquads).toHaveLength(2);
+
+    expect(h.update(withBypass(3000))).toBe(true);
+    // The lowpass took the new cutoff; the highpass was left where it was.
+    expect(biquads[0]!.frequency.value).toBe(100);
+    expect(biquads[1]!.frequency.value).toBe(3000);
+  });
+
   it("reports that a rebuild is needed when the chain shape changes", () => {
     const c = ctx();
     const h = buildFxChain(asCtx(c), chain("peaking"));
@@ -438,3 +481,54 @@ describe("automatable parameters", () => {
     expect(onePole.automation?.frequency).toBeUndefined();
   });
 });
+
+describe("phaser automation targets", () => {
+  /**
+   * `in_gain` and `out_gain` trim the signal entering and leaving the effect,
+   * which the builder drives through inTrim/outTrim while pinning wet and dry
+   * to 1. The automation map used to aim both lanes at wet/dry — so an envelope
+   * modulated a constant, the trim it was supposed to move stayed frozen, and
+   * "fade the phaser out" left the dry leg playing at full level.
+   *
+   * Asserted by VALUE rather than by node identity: the trims are internal, and
+   * the only honest question is whether the param a lane would drive is the one
+   * the knob sets.
+   */
+  it("drives the trims a lane is named for, not the pinned wet/dry pair", () => {
+    const handle = buildFxNode(ctx() as unknown as BaseAudioContext, "phaser", {
+      ...defaultAudioFxParams("phaser"),
+      in_gain: 0.25,
+      out_gain: 0.5,
+    });
+    expect(handle.automation?.in_gain?.[0]?.param.value).toBeCloseTo(0.25, 6);
+    expect(handle.automation?.out_gain?.[0]?.param.value).toBeCloseTo(0.5, 6);
+  });
+});
+
+describe("chain update keeps ids with their effects", () => {
+  const band = (id: string, frequency: number) => ({
+    type: "peaking",
+    id,
+    enabled: true,
+    params: { ...defaultAudioFxParams("peaking"), frequency },
+  });
+
+  /**
+   * Reordering two effects of the same type leaves the shape string identical,
+   * so the chain updates in place rather than rebuilding — correct for the
+   * audio, since the params move with the position. The ids have to move too:
+   * a lane addresses its effect by id, and an id captured at build time names
+   * whichever effect used to occupy that slot. The scheduler would then drive
+   * `fx.n2.frequency` into the band that is now n1 — the exact swap that
+   * HfAudioFxNode.id documents itself as preventing, and the one the voiceover
+   * carve's all-peaking chains make easy to hit.
+   */
+  it("moves an id with its slot when same-type effects are reordered", () => {
+    const chain: HfAudioFxChain = { version: 1, nodes: [band("n1", 200), band("n2", 4000)] };
+    const handle = buildFxChain(ctx() as unknown as BaseAudioContext, chain);
+
+    const swapped: HfAudioFxChain = { version: 1, nodes: [band("n2", 4000), band("n1", 200)] };
+    expect(handle.update(swapped)).toBe(true);
+    expect(handle.nodes.map((n) => n.id)).toEqual(["n2", "n1"]);
+  });
+});

packages/core/src/audio/audioFxGraph.ts

@@ -9,6 +9,7 @@
  */
 
 import {
+  enabledAudioFxNodes,
   getAudioFxDef,
   normalizeAudioFxParams,
   type HfAudioFxChain,
@@ -176,7 +177,17 @@ function workletBuilder(processor: string): Builder {
       input: node,
       output: node,
       update: (v) => node.port.postMessage({ ...v }),
-      dispose: () => node.disconnect(),
+      dispose: () => {
+        // Disconnecting is not enough to retire an AudioWorkletProcessor: it
+        // lives until its `process()` returns false, and these all returned
+        // true unconditionally. So every chain rebuild that dropped a limiter,
+        // compressor, gate or bitcrush left it running on the audio thread for
+        // the rest of the session, and a few edits to a carved bed accumulated
+        // a stack of them. The processors treat this message as their cue to
+        // stop.
+        node.port.postMessage({ __hfDispose: true });
+        node.disconnect();
+      },
     };
   };
 }
@@ -360,8 +371,13 @@ const allpassPhaser: Builder = (ctx, p) => {
     // frequency at once — not one knob, one param — so they stay unautomated.
     automation: {
       speed: [{ param: lfo.frequency }],
-      in_gain: [{ param: dry.gain }],
-      out_gain: [{ param: wet.gain }],
+      // The trims, not wet/dry. apply() drives inTrim/outTrim from these knobs
+      // and pins wet and dry to 1 — so a lane aimed at wet/dry modulated a
+      // constant and left the trim frozen, and the next values-only edit slammed
+      // it back over the running envelope. The comment above records that this
+      // wiring was already moved once; the automation map was missed.
+      in_gain: [{ param: inTrim.gain }],
+      out_gain: [{ param: outTrim.gain }],
     },
     dispose: () => {
       try {
@@ -465,8 +481,7 @@ export interface FxChainHandle {
  * into the running nodes; when it changes, the caller rebuilds.
  */
 function shapeOf(chain: HfAudioFxChain): string {
-  return chain.nodes
-    .filter((node) => node.enabled !== false)
+  return enabledAudioFxNodes(chain)
     .map((node) => {
       const p = normalizeAudioFxParams(node.type, node.params);
       const poles = p.poles !== undefined ? `:${p.poles}` : "";
@@ -491,28 +506,38 @@ export function buildFxChain(ctx: BaseAudioContext, chain: HfAudioFxChain): FxCh
   const handles: { id?: string; type: string; handle: FxNodeHandle }[] = [];
 
   let tail: AudioNode = input;
-  for (const node of chain.nodes) {
-    if (node.enabled === false) continue;
+  for (const node of enabledAudioFxNodes(chain)) {
     const handle = buildFxNode(ctx, node.type, node.params ?? {});
     tail.connect(handle.input);
     tail = handle.output;
     handles.push({ ...(node.id ? { id: node.id } : {}), type: node.type, handle });
   }
   tail.connect(output);
 
-  let shape = shapeOf(chain);
+  const shape = shapeOf(chain);
 
   return {
     input,
     output,
     nodes: handles,
     update(next) {
       if (shapeOf(next) !== shape) return false;
-      const active = next.nodes.filter((node) => node.enabled !== false);
-      active.forEach((node, i) => {
-        handles[i]?.handle.update(normalizeAudioFxParams(node.type, node.params));
+      enabledAudioFxNodes(next).forEach((node, i) => {
+        const held = handles[i];
+        if (!held) return;
+        held.handle.update(normalizeAudioFxParams(node.type, node.params));
+        // The id follows the position, because the params just did. Reordering
+        // two effects of the same type leaves the shape identical, so the graph
+        // is updated in place — but a lane addresses its effect BY id, and an id
+        // captured at build time then names whichever effect used to be here.
+        // The scheduler would drive `fx.n2.frequency` into the band that is now
+        // n1: exactly what HfAudioFxNode.id documents itself as preventing.
+        if (node.id === undefined) delete held.id;
+        else held.id = node.id;
       });
-      shape = shapeOf(next);
+      // `shape` is not reassigned: the early return above already established
+      // that `shapeOf(next)` equals it, so recomputing was a whole normalise +
+      // join per observer tick to write back the string that was already there.
       return true;
     },
     dispose() {

packages/core/src/audio/audioFxWorklets.test.ts

@@ -0,0 +1,103 @@
+import { describe, expect, it, vi } from "vitest";
+import { audioFxWorkletsReady, ensureAudioFxWorklets } from "./audioFxWorklets.js";
+
+/** Just enough of a BaseAudioContext for the registration cache to key on. */
+const contextWith = (addModule: (url: string) => Promise<void>): BaseAudioContext =>
+  ({ audioWorklet: { addModule } }) as unknown as BaseAudioContext;
+
+describe("ensureAudioFxWorklets", () => {
+  it("registers once per context and reuses the result", async () => {
+    const addModule = vi.fn(async () => undefined);
+    const ctx = contextWith(addModule);
+
+    await ensureAudioFxWorklets(ctx);
+    await ensureAudioFxWorklets(ctx);
+
+    expect(addModule).toHaveBeenCalledTimes(1);
+    expect(audioFxWorkletsReady(ctx)).toBe(true);
+  });
+
+  it("retries after a failure instead of replaying it forever", async () => {
+    // The rejected promise used to stay in the cache, so every later attempt
+    // got the same rejection back — the limiter, compressor, gate and bitcrush
+    // were silent for the life of the context after one transient failure.
+    const addModule = vi
+      .fn<(url: string) => Promise<void>>()
+      .mockRejectedValueOnce(new Error("module load failed"))
+      .mockResolvedValue(undefined);
+    const ctx = contextWith(addModule);
+
+    await expect(ensureAudioFxWorklets(ctx)).rejects.toThrow("module load failed");
+    expect(audioFxWorkletsReady(ctx)).toBe(false);
+
+    await expect(ensureAudioFxWorklets(ctx)).resolves.toBeUndefined();
+    expect(addModule).toHaveBeenCalledTimes(2);
+    expect(audioFxWorkletsReady(ctx)).toBe(true);
+  });
+
+  it("refuses a context with no AudioWorklet rather than hanging", async () => {
+    const ctx = {} as BaseAudioContext;
+    await expect(ensureAudioFxWorklets(ctx)).rejects.toThrow(/secure context/);
+  });
+});
+
+/**
+ * A processor lives until its `process()` returns false — disconnecting the
+ * node does not retire it. These all returned true unconditionally, so every
+ * chain rebuild that dropped a worklet effect left it running on the audio
+ * thread for the rest of the session.
+ *
+ * The source is taken from the data: URL registration actually hands to
+ * `addModule`, so this also proves the URL carries what it claims to.
+ */
+describe("the worklet processors themselves", () => {
+  /** Evaluate the registered module and hand back the processor classes by name. */
+  async function loadProcessors(): Promise<Map<string, new (o: unknown) => Processor>> {
+    let moduleSource = "";
+    await ensureAudioFxWorklets(
+      contextWith(async (url: string) => {
+        moduleSource = atob(url.replace("data:text/javascript;base64,", ""));
+      }),
+    );
+    const made = new Map<string, new (o: unknown) => Processor>();
+    class Base {
+      port = {
+        onmessage: null as ((e: { data: unknown }) => void) | null,
+        postMessage: (data: unknown) => this.port.onmessage?.({ data }),
+      };
+    }
+    new Function("AudioWorkletProcessor", "registerProcessor", "sampleRate", moduleSource)(
+      Base,
+      (name: string, cls: new (o: unknown) => Processor) => made.set(name, cls),
+      48000,
+    );
+    return made;
+  }
+
+  interface Processor {
+    port: { postMessage(data: unknown): void };
+    process(inputs: Float32Array[][], outputs: Float32Array[][]): boolean;
+  }
+
+  const block = (): Float32Array[][] => [[new Float32Array(128)]];
+
+  it("every processor keeps running until it is told to stop, then retires", async () => {
+    const processors = await loadProcessors();
+    expect([...processors.keys()]).toEqual([
+      "hf-compressor",
+      "hf-limiter",
+      "hf-gate",
+      "hf-bitcrush",
+    ]);
+
+    for (const [name, Cls] of processors) {
+      const p = new Cls({ processorOptions: {} });
+      expect(p.process(block(), block()), `${name} retired before it was disposed`).toBe(true);
+      p.port.postMessage({ __hfDispose: true });
+      expect(p.process(block(), block()), `${name} kept running after dispose`).toBe(false);
+      // And it stays retired — a later parameter update must not revive it.
+      p.port.postMessage({ mix: 0.5 });
+      expect(p.process(block(), block()), `${name} came back to life`).toBe(false);
+    }
+  });
+});