fix(core): harden audio FX and group identity (#3444)

* fix(core): harden audio FX and group identity

* fix(core): address audio group review feedback

Author: vanceingalls

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

@@ -136,25 +136,127 @@ describe("the worklet processors themselves", () => {
       return crossings / ((s.length - start) / SR);
     }
 
-    it("at semitones: 0, mix: 1 reproduces the input, delayed by exactly one grain/2", async () => {
+    // This assertion used to be that the output equalled the input DELAYED by
+    // grain/2 — the measurement was right and was written down as the contract.
+    // But the grain delay is there to shift pitch, and at semitones: 0 nothing
+    // is being shifted: the node degenerated into a pure 50 ms delay of the
+    // signal, plus a head of silence while the ring filled, under a label that
+    // reads "Unchanged pitch".
+    it("at semitones: 0, mix: 1 passes the input through untouched", async () => {
       const HfPitchshift = (await loadProcessors()).get("hf-pitchshift");
       if (!HfPitchshift) throw new Error("hf-pitchshift not registered");
       const p = new HfPitchshift({ processorOptions: { semitones: 0, mix: 1 } });
       const input = sine(440, 0.5);
       const output = run(p, input);
-      const grain = Math.round(SR * 0.1);
-      // readTap reads from `write - 1`, i.e. one sample behind the one just
-      // written in this same iteration — so the effective delay is one sample
-      // more than the nominal grain/2.
-      const delay = grain / 2 + 1;
-      // Skip the first grain while the ring buffer is still filling.
       let maxErr = 0;
-      for (let i = grain * 2; i < input.length; i++) {
-        maxErr = Math.max(maxErr, Math.abs((output[i] ?? 0) - (input[i - delay] ?? 0)));
+      for (let i = 0; i < input.length; i++) {
+        maxErr = Math.max(maxErr, Math.abs((output[i] ?? 0) - (input[i] ?? 0)));
       }
       expect(maxErr).toBeLessThan(1e-6);
     });
 
+    it("mix: 0 passes the input through untouched too", async () => {
+      const HfPitchshift = (await loadProcessors()).get("hf-pitchshift");
+      if (!HfPitchshift) throw new Error("hf-pitchshift not registered");
+      const p = new HfPitchshift({ processorOptions: { semitones: 7, mix: 0 } });
+      const input = sine(440, 0.25);
+      const output = run(p, input);
+      let maxErr = 0;
+      for (let i = 0; i < input.length; i++) {
+        maxErr = Math.max(maxErr, Math.abs((output[i] ?? 0) - (input[i] ?? 0)));
+      }
+      expect(maxErr).toBeLessThan(1e-6);
+    });
+
+    /** Largest sample-to-sample step — a splice between the dry and the
+     *  ~50 ms-delayed wet path shows up here as a discontinuity. */
+    function maxStep(s: Float32Array, from: number, to: number): number {
+      let worst = 0;
+      for (let i = from + 1; i < to; i++) {
+        worst = Math.max(worst, Math.abs((s[i] ?? 0) - (s[i - 1] ?? 0)));
+      }
+      return worst;
+    }
+
+    // Dragging the semitones slider off zero mid-playback swaps the output from
+    // x[t] to x[t-50ms]. Switched hard that is an audible click; the wet amount
+    // is ramped instead. A 440 Hz sine steps ~0.057 per sample at its steepest,
+    // so anything near the signal's own peak is a splice, not the waveform.
+    it("does not click when the shift moves off zero mid-signal", async () => {
+      const HfPitchshift = (await loadProcessors()).get("hf-pitchshift");
+      if (!HfPitchshift) throw new Error("hf-pitchshift not registered");
+      const p = new HfPitchshift({ processorOptions: { semitones: 0, mix: 1 } });
+      run(p, sine(440, 0.3)); // settled dry, ring warm
+      p.p = { ...p.p, semitones: 7 };
+      const output = run(p, sine(440, 0.3));
+      expect(maxStep(output, 0, output.length)).toBeLessThan(0.2);
+    });
+
+    it("does not click on the way back to zero either", async () => {
+      const HfPitchshift = (await loadProcessors()).get("hf-pitchshift");
+      if (!HfPitchshift) throw new Error("hf-pitchshift not registered");
+      const p = new HfPitchshift({ processorOptions: { semitones: 7, mix: 1 } });
+      run(p, sine(440, 0.3));
+      p.p = { ...p.p, semitones: 0 };
+      const output = run(p, sine(440, 0.3));
+      expect(maxStep(output, 0, output.length)).toBeLessThan(0.2);
+    });
+
+    // ...and having ramped back down it must reach TRUE bypass, not sit on a
+    // permanently latched wet path. The render builds a fresh node from the
+    // saved attribute and bypasses at semitones 0; a preview that stayed wet
+    // would carry a 50 ms delay the export does not have.
+    it("returns to true bypass after being shifted and set back to zero", async () => {
+      const HfPitchshift = (await loadProcessors()).get("hf-pitchshift");
+      if (!HfPitchshift) throw new Error("hf-pitchshift not registered");
+      const p = new HfPitchshift({ processorOptions: { semitones: 7, mix: 1 } });
+      run(p, sine(440, 0.3));
+      p.p = { ...p.p, semitones: 0 };
+      run(p, sine(440, 0.3)); // ramp down settles here
+
+      const input = sine(440, 0.3);
+      const output = run(p, input);
+      let maxErr = 0;
+      for (let i = 0; i < input.length; i++) {
+        maxErr = Math.max(maxErr, Math.abs((output[i] ?? 0) - (input[i] ?? 0)));
+      }
+      expect(maxErr).toBeLessThan(1e-6);
+    });
+
+    // A node parked at mix 0 has shifted nothing, so it must not have spent
+    // anything that stops the zero-shift bypass engaging later.
+    it("is transparent at zero after sitting mixed fully out", async () => {
+      const HfPitchshift = (await loadProcessors()).get("hf-pitchshift");
+      if (!HfPitchshift) throw new Error("hf-pitchshift not registered");
+      const p = new HfPitchshift({ processorOptions: { semitones: 7, mix: 0 } });
+      run(p, sine(440, 0.3));
+
+      p.p = { ...p.p, semitones: 0, mix: 1 };
+      const input = sine(440, 0.3);
+      const output = run(p, input);
+      let maxErr = 0;
+      for (let i = 0; i < input.length; i++) {
+        maxErr = Math.max(maxErr, Math.abs((output[i] ?? 0) - (input[i] ?? 0)));
+      }
+      expect(maxErr).toBeLessThan(1e-6);
+    });
+
+    // The ring starts empty, so the taps read zeros for the first grain. That
+    // used to come out of the head of every clip as silence; it ramps the wet
+    // path in instead, which is unshifted audio rather than no audio.
+    it("does not open with silence while the grain buffer fills", async () => {
+      const HfPitchshift = (await loadProcessors()).get("hf-pitchshift");
+      if (!HfPitchshift) throw new Error("hf-pitchshift not registered");
+      const p = new HfPitchshift({ processorOptions: { semitones: 7, mix: 1 } });
+      const input = sine(440, 0.5);
+      const output = run(p, input);
+      // Peak over the first 20 ms — well inside the old dead zone.
+      let peak = 0;
+      for (let i = 0; i < Math.round(SR * 0.02); i++)
+        peak = Math.max(peak, Math.abs(output[i] ?? 0));
+      expect(peak).toBeGreaterThan(0.5);
+    });
+
     it("at semitones: 12, doubles the fundamental (one octave up)", async () => {
       const HfPitchshift = (await loadProcessors()).get("hf-pitchshift");
       if (!HfPitchshift) throw new Error("hf-pitchshift not registered");

packages/core/src/audio/audioFxWorklets.ts

@@ -253,6 +253,21 @@ class HfPitchshift extends AudioWorkletProcessor {
     this.buf = [];
     this.write = 0;
     this.phase = 0;
+    // Samples written so far, capped at one grain. The taps read up to a grain
+    // behind the write head, so until this fills they would read the ring's
+    // zeros — the head of every clip came out attenuated or silent.
+    this.filled = 0;
+    // How much of the wet (pitch-shifted) path is currently in the output, and
+    // where it is heading. Crossing between dry and wet is a ~50 ms jump in the
+    // signal, so it is RAMPED rather than switched: a hard swap either way is a
+    // click. Ramping in both directions is also what lets a node return to true
+    // bypass at semitones 0 — a one-way latch left preview stuck with the delay
+    // that the render, building a fresh node from the attribute, does not have.
+    this.wet = 0;
+    this.wetTarget = 0;
+    // ~15 ms one-pole, short enough to feel immediate on a slider drag and long
+    // enough that the splice is inaudible.
+    this.wetCoef = Math.exp(-1 / (sampleRate * 0.015));
     this.port.onmessage = (e) => {
       if (e.data && e.data.__hfDispose) { this.dead = true; return; }
       this.p = { ...this.p, ...e.data };
@@ -265,33 +280,67 @@ class HfPitchshift extends AudioWorkletProcessor {
     const p = this.p;
     const semitones = Math.max(-12, Math.min(12, p.semitones ?? 0));
     const mix = Math.max(0, Math.min(1, p.mix ?? 1));
-    const ratio = Math.pow(2, semitones / 12);
     const grain = this.grain;
     const ringLen = grain * 2;
-    const inc = (1 - ratio) / grain;
     const n = i[0] ? i[0].length : 0;
     for (let ch = 0; ch < i.length; ch++) {
       if (!this.buf[ch]) this.buf[ch] = new Float32Array(ringLen);
     }
-    let write = this.write, phase = this.phase;
+
+    // Nothing to shift, or mixed fully out. The grain delay is ~grain/2
+    // whatever the ratio, so at semitones=0 this degenerated into a pure 50 ms
+    // delay of the signal — while the copy for that exact setting reads
+    // "Unchanged pitch".
+    this.wetTarget = semitones === 0 ? 0 : mix;
+
+    // Fully dry AND settled: take the cheap transparent path. The ring keeps
+    // filling, so a later shift does not start cold.
+    if (this.wetTarget === 0 && this.wet < 1e-4) {
+      this.wet = 0;
+      let w = this.write;
+      for (let s = 0; s < n; s++) {
+        for (let ch = 0; ch < i.length; ch++) {
+          const x = i[ch][s];
+          this.buf[ch][w] = x;
+          o[ch][s] = x;
+        }
+        w = (w + 1) % ringLen;
+      }
+      this.write = w;
+      this.filled = Math.min(grain, this.filled + n);
+      return true;
+    }
+
+    const ratio = Math.pow(2, semitones / 12);
+    const inc = (1 - ratio) / grain;
+    let write = this.write, phase = this.phase, filled = this.filled, wetNow = this.wet;
+    const target = this.wetTarget, coef = this.wetCoef;
     for (let s = 0; s < n; s++) {
       phase += inc;
       phase -= Math.floor(phase);
       const phaseB = (phase + 0.5) % 1;
       const gA = xfade(phase), gB = xfade(phaseB);
+      // Ramp the wet path in as the ring fills rather than reading zeros:
+      // 100 ms of unshifted audio at the head of a clip beats 50 ms of silence.
+      const warm = filled >= grain ? 1 : filled / grain;
+      wetNow = target + coef * (wetNow - target);
+      const wetMix = wetNow * warm;
       for (let ch = 0; ch < i.length; ch++) {
         const ring = this.buf[ch];
         const inp = i[ch], out = o[ch];
         const x = inp[s];
         ring[write] = x;
         const wet =
           readTap(ring, write, phase * grain) * gA + readTap(ring, write, phaseB * grain) * gB;
-        out[s] = x * (1 - mix) + wet * mix;
+        out[s] = x * (1 - wetMix) + wet * wetMix;
       }
       write = (write + 1) % ringLen;
+      if (filled < grain) filled++;
     }
     this.write = write;
     this.phase = phase;
+    this.filled = filled;
+    this.wet = wetNow;
     return true;
   }
 }

packages/core/src/audioCarve.test.ts

@@ -10,6 +10,8 @@ import {
   clipsOverlap,
   mixCarveSources,
   couldBeCarveSource,
+  couldBeCarveBed,
+  isNamedCarveBed,
   DEFAULT_CARVE,
   normalizeCarveSettings,
 } from "./audioCarve.js";
@@ -538,6 +540,30 @@ describe("classifyAudioName", () => {
     expect(couldBeCarveSource("sfx-explosion")).toBe(false);
   });
 
+  // The near-end rule, which nothing used to ask. `couldBeCarveSource` shipped
+  // with its own doc comment ("music and sfx are out") and no caller; the bed
+  // side had no predicate at all, so a narration clip was offered the carve and
+  // — finding one candidate — had one applied for it, against the group it was
+  // a member of.
+  it("never offers a voice track as the bed, but keeps an unnamed one eligible", () => {
+    expect(couldBeCarveBed("music-bed")).toBe(true);
+    expect(couldBeCarveBed("sfx-riser")).toBe(true);
+    expect(couldBeCarveBed("a1")).toBe(true);
+    expect(couldBeCarveBed("vo-2")).toBe(false);
+    expect(couldBeCarveBed("voiceover")).toBe(false);
+    expect(couldBeCarveBed("narration-3")).toBe(false);
+  });
+
+  // Showing the control is a suggestion; writing the attribute is a decision.
+  // A decision taken off a name that said nothing is how a carve appears that
+  // nobody remembers configuring — so `a1` may be offered but never chosen.
+  it("only self-applies to a name that positively reads as a bed", () => {
+    expect(isNamedCarveBed("music-bed")).toBe(true);
+    expect(isNamedCarveBed("sfx-riser")).toBe(true);
+    expect(isNamedCarveBed("a1")).toBe(false);
+    expect(isNamedCarveBed("vo-2")).toBe(false);
+  });
+
   it("treats underscores as separators, not word characters, for short hints", () => {
     // `\b` treats `_` as a word character, so `\bbed\b` used to miss `bed_01` —
     // an underscore-separated bed classified as "unknown" and could end up

packages/core/src/audioCarve.ts

@@ -188,6 +188,40 @@ export function couldBeCarveSource(...parts: readonly (string | null | undefined
   return kind === "voice" || kind === "unknown";
 }
 
+/**
+ * Could this track be the BED a carve is written onto?
+ *
+ * The other half of `couldBeCarveSource`, and the half nothing used to ask. A
+ * carve makes room in a bed for a voice; a voice track has no room to make for
+ * itself, and offering it the control is offering a track to duck against its
+ * own kind. Observed: a narration clip in a Voiceover group carved against that
+ * group — a member ducking the bus it feeds.
+ *
+ * Loose in the same direction as its sibling: a name that says nothing stays
+ * eligible, because a name is a hint and an author may know better. Only a name
+ * that positively reads as speech is refused.
+ */
+export function couldBeCarveBed(...parts: readonly (string | null | undefined)[]): boolean {
+  return classifyAudioName(...parts) !== "voice";
+}
+
+/**
+ * Does this track's name positively say "bed"?
+ *
+ * Stricter than `couldBeCarveBed`, for the one act the author did not ask for:
+ * applying a carve on their behalf. Offering the control on a track named `a1`
+ * is a suggestion they can ignore; writing `data-fx-carve` onto it is a decision,
+ * and a decision taken off a name that said nothing is how a carve appears that
+ * nobody remembers configuring.
+ *
+ * The same split the source side already makes between what the picker may show
+ * and what `autoSourceIds` may choose unprompted.
+ */
+export function isNamedCarveBed(...parts: readonly (string | null | undefined)[]): boolean {
+  const kind = classifyAudioName(...parts);
+  return kind === "music" || kind === "sfx";
+}
+
 export const DEFAULT_CARVE: HfCarveSettings = {
   enabled: true,
   sources: [],