diff --git a/packages/core/package.json b/packages/core/package.json index c2a6213ed..b87777821 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -559,7 +559,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", diff --git a/packages/core/src/audio/audioFxGraph.test.ts b/packages/core/src/audio/audioFxGraph.test.ts index 9c54f12bc..1a8d23519 100644 --- a/packages/core/src/audio/audioFxGraph.test.ts +++ b/packages/core/src/audio/audioFxGraph.test.ts @@ -438,3 +438,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"]); + }); +}); diff --git a/packages/core/src/audio/audioFxGraph.ts b/packages/core/src/audio/audioFxGraph.ts index 9e5172f82..f4bc818dd 100644 --- a/packages/core/src/audio/audioFxGraph.ts +++ b/packages/core/src/audio/audioFxGraph.ts @@ -364,8 +364,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 { @@ -514,7 +519,17 @@ export function buildFxChain(ctx: BaseAudioContext, chain: HfAudioFxChain): FxCh 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)); + 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); return true; diff --git a/packages/core/src/runtime/media.test.ts b/packages/core/src/runtime/media.test.ts index c6fa207ed..bf697161b 100644 --- a/packages/core/src/runtime/media.test.ts +++ b/packages/core/src/runtime/media.test.ts @@ -365,6 +365,37 @@ describe("syncRuntimeMedia", () => { expect(only).toBeCloseTo(0.55, 5); }); + /** + * The render bakes the lane at CLIP-LOCAL time: prepareAudioTrack already + * cut the wav with `-ss mediaStart`, so its t=0 is the clip's start, and + * normaliseEnvelope subtracts trackStart. Preview used to sample at MEDIA + * time — mediaStart included, scaled by playbackRate, wrapped on a loop — so + * the same envelope played somewhere else than it rendered. + */ + it("samples the lane at clip-local time, the way the render bakes it", () => { + const trimmed = (t: number) => { + const clip = createMockClip({ start: 0, end: 10, volume: 0.55, mediaStart: 30 }); + Object.defineProperty(clip.el, "readyState", { value: 4, writable: true }); + clip.el.setAttribute("data-automation", DUCK); + let seen = -1; + syncRuntimeMedia({ + clips: [clip], + timeSeconds: t, + playing: true, + playbackRate: 1, + onElementVolume: (_el, v) => { + seen = v; + }, + }); + return seen; + }; + // `data-media-start="30"` on a clip whose lane holds 0.8 until t=2 then + // ducks to 0.1 by t=3. At media time the playhead is already 30 s past the + // last point, so preview held 0.1 from the first frame and never ducked. + expect(trimmed(1)).toBeCloseTo(0.8, 5); + expect(trimmed(5)).toBeCloseTo(0.1, 5); + }); + it("supersedes keyframes probed from the timeline", () => { // Both present: the lane is the explicit one, and `lint` warns about it. const clip = createMockClip({ start: 0, end: 10, volume: 0.55 }); diff --git a/packages/core/src/runtime/media.ts b/packages/core/src/runtime/media.ts index 9a6a4b234..40ae5ec17 100644 --- a/packages/core/src/runtime/media.ts +++ b/packages/core/src/runtime/media.ts @@ -274,7 +274,15 @@ export function syncRuntimeMedia(params: { // An explicit volume lane owns the fader. It is checked before the probed // keyframes because the two would otherwise fight, and it is the one the // author drew — `lint` warns when a track carries both. - const laneGain = elementVolumeLaneGain(el, relTime); + // Clip-local, NOT `relTime`. A lane's `t` is "seconds from the start of + // the clip" (see HfAutomationPoint), and the render honours that: the wav + // is already cut with `-ss mediaStart`, so its t=0 IS the clip's start. + // `relTime` is MEDIA time — it carries mediaStart, scales by playbackRate + // and wraps on a loop — so feeding it here played the envelope at a + // different position than it renders, or ran it off the end entirely on a + // trimmed clip. The FX lanes on this same feature use clip-local elapsed; + // there is one time base, and this is it. + const laneGain = elementVolumeLaneGain(el, params.timeSeconds - clip.start); if (laneGain !== null) { authorVolume = clampVolume(laneGain); } else if (clip.volumeKeyframes && clip.volumeKeyframes.length > 0) { diff --git a/packages/core/src/runtime/webAudioTransport.test.ts b/packages/core/src/runtime/webAudioTransport.test.ts index 15f1825a1..f1597ccc8 100644 --- a/packages/core/src/runtime/webAudioTransport.test.ts +++ b/packages/core/src/runtime/webAudioTransport.test.ts @@ -388,6 +388,19 @@ describe("WebAudioTransport", () => { expect(transport.isActive()).toBe(false); }); + it("disposes the FX graph when a clip ends naturally", async () => { + // stopAll() disposes by walking _activeSources, and the splice above had + // already removed this entry — so the handle, its MutationObserver and any + // running LFO survived the clip for the rest of the session. + const { transport, mock, gen } = setupTransport(100); + await transport.schedulePlayback(mockEl, mockBuffer, 0, 0, 0, 1, gen); + + mock.sourceNode._fireEnded(); + + expect(mock.sourceNode.disconnect).toHaveBeenCalled(); + expect(mock.gainNode.disconnect).toHaveBeenCalled(); + }); + it("registers onended listener on the sourceNode", async () => { const { transport, mock, gen } = setupTransport(100); diff --git a/packages/core/src/runtime/webAudioTransport.ts b/packages/core/src/runtime/webAudioTransport.ts index 9bbe6071c..07b1af27c 100644 --- a/packages/core/src/runtime/webAudioTransport.ts +++ b/packages/core/src/runtime/webAudioTransport.ts @@ -264,6 +264,21 @@ export class WebAudioTransport { if (idx !== -1) { this._activeSources.splice(idx, 1); el.muted = priorMuted; + // The graph goes with it. Splicing alone left the FX handle alive and + // then UNREACHABLE — stopAll() disposes by walking this array, which + // the splice just emptied of this entry. Every clip that finished + // naturally leaked its MutationObserver for the session, and each one + // still answered later `data-fx-chain` edits by rebuilding a whole + // graph (impulse response, chorus/phaser oscillators started and never + // stopped) around a dead source. Not disposed when idx is -1: stopAll() + // has already done it, and `stop()` is what fired this event. + try { + sourceNode.disconnect(); + fx?.dispose(); + gainNode.disconnect(); + } catch { + // Already torn down. + } if (this._activeSources.length === 0) this._paused = true; } }); diff --git a/packages/core/stubs/audio-fx-runtime-entry.ts b/packages/core/stubs/audio-fx-runtime-entry.ts index 81f8e5f03..d6a78b943 100644 --- a/packages/core/stubs/audio-fx-runtime-entry.ts +++ b/packages/core/stubs/audio-fx-runtime-entry.ts @@ -81,6 +81,11 @@ async function render( const chain: HfAudioFxChain = parseAudioFxChain(chainJson); const channels = Math.max(1, planes.length); const frames = planes[0]?.length ?? 0; + // Nothing to process, and `new OfflineAudioContext(ch, 0, rate)` throws — an + // error the render treats as fatal. applyAudioFxChain screens empty tracks + // out before they reach the browser; this is the same guard at the point the + // constructor would actually blow up. + if (frames === 0) return planes; const parsedAutomation = automationJson ? resolveAutomation(parseAutomation(automationJson), chain) : null; diff --git a/packages/engine/src/services/audioFxRender.test.ts b/packages/engine/src/services/audioFxRender.test.ts index 0456c9488..b87fbb4e6 100644 --- a/packages/engine/src/services/audioFxRender.test.ts +++ b/packages/engine/src/services/audioFxRender.test.ts @@ -293,3 +293,25 @@ describe.skipIf(!HAS_BROWSER)("browser render", () => { expect(readWav(outPath).samples.length).toBeGreaterThan(0); }, 180_000); }); + +/** + * ffmpeg exits 0 and writes a structurally valid but EMPTY wav whenever a + * clip's trim starts past the end of its source (`-ss 10 -t 5` on a 2 s file). + * That track used to reach `new OfflineAudioContext(ch, 0, rate)`, which throws + * — and the error travels past the mixer's per-track failure collector, so one + * mis-set `data-media-start` took the whole render down. + */ +describe("an empty track", () => { + it("is handed back untouched rather than failing the render", async () => { + const input = join(dir, "empty.wav"); + writeWav(input, new Float32Array(0), SR); + const output = join(dir, "out.wav"); + + // Returns the input path, the same contract as a chain with nothing enabled + // — and without paying for a browser to decide it. + await expect( + applyAudioFxChain(input, chainOf("peaking"), output, { trackId: "t" }), + ).resolves.toBe(input); + expect(existsSync(output)).toBe(false); + }); +}); diff --git a/packages/engine/src/services/audioFxRender.ts b/packages/engine/src/services/audioFxRender.ts index 80ce1cd8e..b09fb1f85 100644 --- a/packages/engine/src/services/audioFxRender.ts +++ b/packages/engine/src/services/audioFxRender.ts @@ -205,18 +205,30 @@ export async function applyAudioFxChain( const { samples, sampleRate, channels } = readWav(inputWav); const planes = deinterleave(samples, channels); + // An empty track has nothing to process — and an OfflineAudioContext of zero + // length throws, which is fatal for the WHOLE render rather than this track: + // the error travels past the mixer's per-track failure collector. ffmpeg + // writes an empty but structurally valid WAV whenever a clip's trim starts + // past the end of its source, so one mis-set `data-media-start` used to take + // the render down. Guarded here as well as in the runtime so an empty track + // never costs a browser. + if ((planes[0]?.length ?? 0) === 0) return inputWav; - // Audio processing needs no GPU or special capture mode; a plain sandboxed - // browser is enough, and the lease pool reuses one across tracks. - const lease = await acquireBrowser([ - "--no-sandbox", - "--autoplay-policy=no-user-gesture-required", - ]); + // Both resources are taken INSIDE the try that releases them. The lease used + // to be acquired above it, with the mkdtemp between — so a failure there + // (a full disk, a read-only tmpdir) leaked a pooled browser, and a pool with + // no leases left hangs every later render rather than failing one. const hostDir = mkdtempSync(join(tmpdir(), "hf-fx-host-")); + let lease: Awaited> | null = null; try { + // Audio processing needs no GPU or special capture mode; a plain sandboxed + // browser is enough, and the lease pool reuses one across tracks. + // Checked before the lease, not after: an already-cancelled track has no + // reason to take a browser out of the pool just to hand it straight back. if (options.signal?.aborted) { throw new AudioFxRenderError(`Audio FX cancelled for track ${options.trackId}`); } + lease = await acquireBrowser(["--no-sandbox", "--autoplay-policy=no-user-gesture-required"]); const page = await lease.browser.newPage(); try { // AudioWorklet is only exposed in a secure context, and about:blank is @@ -302,7 +314,7 @@ export async function applyAudioFxChain( ); } finally { rmSync(hostDir, { recursive: true, force: true }); - await lease.release().catch(() => undefined); + await lease?.release().catch(() => undefined); } } diff --git a/packages/engine/src/services/audioMixer.ts b/packages/engine/src/services/audioMixer.ts index ead78edf9..10c6f9ad5 100644 --- a/packages/engine/src/services/audioMixer.ts +++ b/packages/engine/src/services/audioMixer.ts @@ -793,6 +793,12 @@ export async function processCompositionAudio( // be able to abort the in-flight ffmpeg runs before the finally-block removes // workDir out from under them. Chained off the caller's signal so external // cancellation still behaves as before. + // + // Every child that can outlive a sibling's failure has to be given THIS + // signal, not the caller's: the trim, the video extract and the download all + // took `signal`, so `internalController.abort()` cancelled nothing and the + // `rmSync(workDir)` on the next line ran while their ffmpeg children were + // still writing into it. const internalController = new AbortController(); const effectiveSignal = internalController.signal; if (signal) { @@ -823,9 +829,16 @@ export async function processCompositionAudio( if (isHttpUrl(srcPath)) { try { - srcPath = await downloadToTemp(srcPath, workDir, undefined, signal, undefined, { - onTelemetry: writeUrlDownloadTelemetry, - }); + srcPath = await downloadToTemp( + srcPath, + workDir, + undefined, + effectiveSignal, + undefined, + { + onTelemetry: writeUrlDownloadTelemetry, + }, + ); } catch (err: unknown) { failures.push(downloadFailure(err, element.id)); return; @@ -890,7 +903,7 @@ export async function processCompositionAudio( startTime: element.mediaStart, duration: element.end - element.start, }, - signal, + effectiveSignal, config, ); if (!extractResult.success) { @@ -916,7 +929,7 @@ export async function processCompositionAudio( trimmedPath, element.mediaStart, element.end - element.start, - signal, + effectiveSignal, config, ); if (!prepResult.success) { diff --git a/packages/lint/src/rules/media.test.ts b/packages/lint/src/rules/media.test.ts index f2574aa17..8554bad69 100644 --- a/packages/lint/src/rules/media.test.ts +++ b/packages/lint/src/rules/media.test.ts @@ -457,6 +457,27 @@ describe("audio_volume_double_automation", () => { } }); + it("does not blame the wrong element in a chained timeline", async () => { + // A chain has no semicolon until its very end, so a run that could cross `)` + // reached the `volume` in a LATER call and reported the element from an + // earlier one. Acting on the fixHint would have deleted #bgm's only real + // automation to fix a tween that is on #vo. + const res = await lintHyperframeHtml( + withScript( + LANE, + `gsap.timeline().to("#bgm", { duration: 0.6, x: 10 }).to("#vo", { volume: 1 });`, + ), + ); + expect(res.findings.some((f) => f.code === "audio_volume_double_automation")).toBe(false); + }); + + it("still catches a real tween further down the same call", async () => { + const res = await lintHyperframeHtml( + withScript(LANE, `gsap.timeline().to("#bgm", { duration: 0.6, ease: "none", volume: 0 });`), + ); + expect(res.findings.some((f) => f.code === "audio_volume_double_automation")).toBe(true); + }); + it("ignores a lane that automates something other than volume", async () => { const res = await lintHyperframeHtml( withScript( diff --git a/packages/lint/src/rules/media.ts b/packages/lint/src/rules/media.ts index 9efca4f6f..32c10d862 100644 --- a/packages/lint/src/rules/media.ts +++ b/packages/lint/src/rules/media.ts @@ -625,7 +625,14 @@ function findVolumeDoubleAutomationFindings(ctx: LintContext): HyperframeLintFin // the same call the runtime's own probe would pick up, and the rule only // warns, so a miss costs nothing. const escaped = escapeRegExp(id); - const tweened = new RegExp(`#${escaped}(?![\\w-])[^;]{0,200}?\\bvolume\\s*:`, "s").test(script); + // `[^;)]`, not `[^;]`: a chained timeline has no semicolon until the end of + // the whole chain, so a run that could cross `)` matched `volume` in a LATER + // `.to()` call and named the wrong element — and this rule's fixHint tells + // the author to delete their lane. Refusing to cross the closing paren keeps + // the match inside the call the selector belongs to. It costs a false + // negative when some other value in the same object is a call result, which + // is the safe direction for a warning that already only guesses. + const tweened = new RegExp(`#${escaped}(?![\\w-])[^;)]{0,200}?\\bvolume\\s*:`).test(script); if (!tweened) continue; findings.push({ code: "audio_volume_double_automation", diff --git a/packages/studio/src/components/editor/propertyPanelAudioFxGroup.test.tsx b/packages/studio/src/components/editor/propertyPanelAudioFxGroup.test.tsx index 7950411fc..c7587df27 100644 --- a/packages/studio/src/components/editor/propertyPanelAudioFxGroup.test.tsx +++ b/packages/studio/src/components/editor/propertyPanelAudioFxGroup.test.tsx @@ -1467,3 +1467,66 @@ describe("AudioFxGroup carve against a deleted voice", () => { ).toEqual(["fx.k1.frequency"]); }); }); + +describe("AudioFxGroup while the carve is measuring", () => { + /** + * `analyse` captures the chain and the automation before its fetch and decode, + * then rewrites the whole `data-fx-chain` from that snapshot. An effect added + * — or a knob committed — during those seconds landed first and was silently + * discarded when the analysis returned. Only the Analyse control was gated; + * every other control in the rack stayed live throughout. + */ + it("locks the rack, so an edit cannot be made against a snapshot that is moving", async () => { + // A fetch that never settles holds the panel in its analysing state, which + // is exactly the window the race lives in. + const hang = vi.fn(() => new Promise(() => {})); + vi.stubGlobal("fetch", hang); + // happy-dom has no Web Audio, and without a constructor `analyse` returns + // before it ever reaches the decode — closing the window under test. + vi.stubGlobal( + "OfflineAudioContext", + class { + decodeAudioData() { + return new Promise(() => {}); + } + }, + ); + try { + const bed = document.createElement("audio"); + bed.id = "bed"; + bed.setAttribute("src", "bed.wav"); + document.body.append(bed); + const voice = document.createElement("audio"); + voice.id = "narration"; + voice.setAttribute("src", "vo.wav"); + document.body.append(voice); + + const host = document.createElement("div"); + document.body.append(host); + await act(async () => { + createRoot(host).render( + , + ); + }); + + // The bed carves itself against its one candidate, which starts the + // decode — and the whole rack goes read-only until it lands. + expect(hang).toHaveBeenCalled(); + const controls = Array.from(host.querySelectorAll(".hf-fx-slider")); + expect(controls.length).toBeGreaterThan(0); + expect(controls.every((c) => c.disabled)).toBe(true); + } finally { + vi.unstubAllGlobals(); + } + }); +}); diff --git a/packages/studio/src/components/editor/propertyPanelAudioFxGroup.tsx b/packages/studio/src/components/editor/propertyPanelAudioFxGroup.tsx index 224d653aa..c9eb2f972 100644 --- a/packages/studio/src/components/editor/propertyPanelAudioFxGroup.tsx +++ b/packages/studio/src/components/editor/propertyPanelAudioFxGroup.tsx @@ -613,6 +613,14 @@ export function AudioFxGroup({ return ( { + document.body.innerHTML = ""; +}); + +/** A frequency knob: wide range, so a clamp to the minimum is unmistakable. */ +const FREQUENCY: HfAudioFxNumberParam = { + kind: "number", + key: "frequency", + label: "Frequency", + min: 20, + max: 20000, + step: 1, + default: 1000, + unit: "Hz", +}; + +/** React tracks its own value on the node, so a plain assignment is ignored. */ +function setInputValue(input: HTMLInputElement, text: string): void { + const setter = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, "value")?.set; + setter?.call(input, text); + input.dispatchEvent(new Event("input", { bubbles: true })); +} + +function mount(value: number) { + const onChange = vi.fn(); + const onCommit = vi.fn(); + const host = document.createElement("div"); + document.body.append(host); + let root!: Root; + act(() => { + root = createRoot(host); + root.render( + , + ); + }); + const number = () => host.querySelector(".hf-fx-number")!; + const slider = () => host.querySelector(".hf-fx-slider")!; + const type = (text: string) => { + act(() => { + number().focus(); + setInputValue(number(), text); + }); + }; + /** A new value arrives through the prop — a carve, or an undo. */ + const receive = (next: number) => { + act(() => { + root.render( + , + ); + }); + }; + return { host, number, slider, onChange, onCommit, type, receive }; +} + +describe("FxParamRow number field", () => { + it("shows what is being typed instead of snapping back to the stored value", () => { + // Every keystroke is clamped and written live, and the live write does not + // refresh the prop — so a field bound to the committed value put the old + // number straight back. Typing 5000 wrote 20 on the first keystroke and the + // knob could not be typed into at all, only dragged. + const { number, type } = mount(1000); + type("5"); + expect(number().value).toBe("5"); + type("5000"); + expect(number().value).toBe("5000"); + }); + + it("does not write the parameter minimum while the field is empty", () => { + // `Number("") === 0` passes Number.isFinite, so select-all + Delete before + // retyping used to clamp to the minimum and live-write it — 20 Hz here. + const { onChange, type } = mount(1000); + type(""); + expect(onChange).not.toHaveBeenCalled(); + type("800"); + expect(onChange).toHaveBeenLastCalledWith("frequency", 800); + }); +}); + +describe("FxParamRow commit", () => { + it("stays quiet when a gesture changed nothing", () => { + // commit() hangs off pointerup, keyup and blur, all reachable without an + // edit. Firing then costs a source patch, a selection resync, a preview + // reload and an audio restart for a gesture that moved nothing. + const { slider, number, onCommit } = mount(1000); + act(() => slider().dispatchEvent(new Event("pointerup", { bubbles: true }))); + act(() => number().dispatchEvent(new Event("blur", { bubbles: true }))); + expect(onCommit).not.toHaveBeenCalled(); + }); + + it("does not re-persist a stale value over a change that arrived meanwhile", () => { + // The scenario: open an EQ row, run the carve (or press undo), which + // rewrites the chain so this row's value becomes 400. Then click the slider + // thumb and release without moving it. `latest` was seeded at mount and only + // written by an edit, so it still held 1000 — and the release wrote it back, + // undoing the carve with no gesture that looks like an edit. + const { slider, onCommit, type, receive } = mount(1000); + // An edit happened earlier in this row's life, so `latest` holds 1000. + type("1000"); + act(() => slider().dispatchEvent(new Event("pointerup", { bubbles: true }))); + onCommit.mockClear(); + + receive(400); + act(() => slider().dispatchEvent(new Event("pointerup", { bubbles: true }))); + expect(onCommit).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/studio/src/components/editor/propertyPanelFxControls.tsx b/packages/studio/src/components/editor/propertyPanelFxControls.tsx index 4fd3254bd..63d7bfd6d 100644 --- a/packages/studio/src/components/editor/propertyPanelFxControls.tsx +++ b/packages/studio/src/components/editor/propertyPanelFxControls.tsx @@ -132,6 +132,26 @@ export function FxParamRow({ * write applied on the way down. */ const [pending, setPending] = useState(null); + /** + * What the number field is showing while it has focus. + * + * The field cannot be bound to the committed value: every keystroke is + * clamped into range and written live, and the live write does not refresh + * the prop — so React put the old number straight back and typing `5000` into + * a 20..20000 knob wrote 20 on the first keystroke and never got further. Held + * as TEXT, so a half-typed "-" or "" is a state the field can be in rather + * than a number to clamp and persist. + */ + const [typing, setTyping] = useState(null); + /** + * Did this gesture actually change anything? + * + * `commit()` hangs off pointerup, keyup and blur, all of which fire without + * an edit — clicking a slider thumb without moving it, or tabbing through the + * field. Committing then re-persisted whatever `latest` happened to hold and + * silently reverted any change that had arrived meanwhile. + */ + const edited = useRef(false); useEffect(() => { if (!dragging) setLocal(value); }, [value, dragging]); @@ -146,6 +166,7 @@ export function FxParamRow({ const p = param as HfAudioFxNumberParam; const next = Math.min(p.max, Math.max(p.min, raw)); latest.current = next; + edited.current = true; setLocal(next); onChange(param.key, next); }, @@ -154,6 +175,12 @@ export function FxParamRow({ const commit = useCallback(() => { setDragging(false); + // Nothing was edited, so there is nothing to persist. Without this a bare + // focus/blur — or a click on the slider thumb that never moved — fired a + // full persisting write: source patch, selection resync, preview reload and + // an audio restart, for a gesture that changed no value. + if (!edited.current) return; + edited.current = false; if (typeof latest.current === "number") setPending(latest.current); onCommit?.(param.key, latest.current); }, [onCommit, param.key]); @@ -228,13 +255,25 @@ export function FxParamRow({ min={param.min} max={param.max} step={param.step} - value={display(param, current)} + value={typing ?? display(param, current)} disabled={locked} + onFocus={() => setTyping(display(param, current))} onChange={(e) => { - const next = Number(e.target.value); + const text = e.target.value; + setTyping(text); + // An empty field, a lone "-", or a trailing "." are all states on the + // way to a number, not numbers. `Number("")` is 0, which passes + // Number.isFinite — so clearing the field to retype used to clamp to + // the parameter MINIMUM and write it live: 20 Hz on a cutoff, -40 dB + // on a gain. + if (text.trim() === "") return; + const next = Number(text); if (Number.isFinite(next)) handleNumber(next); }} - onBlur={commit} + onBlur={() => { + commit(); + setTyping(null); + }} onKeyDown={(e) => { if (e.key === "Enter") commit(); }} diff --git a/plans/audio-automation-lanes/SPEC.md b/plans/audio-automation-lanes/SPEC.md index 628d727ce..85e32b67a 100644 --- a/plans/audio-automation-lanes/SPEC.md +++ b/plans/audio-automation-lanes/SPEC.md @@ -16,7 +16,7 @@ Two facts make this cheaper here than in most editors: 1. **Web Audio has native envelope playback.** `AudioParam` scheduling (`linearRampToValueAtTime`, `setValueCurveAtTime`) is sample-accurate and runs on the audio thread. No per-frame JS evaluates the envelope; the studio - only *schedules* it. + only _schedules_ it. 2. **Preview and render share one graph.** The render runs the same builders in an `OfflineAudioContext`, so an envelope scheduled the same way in both places is identical by construction. No parity harness needed. @@ -50,16 +50,16 @@ panel). ## 3. UX spec (Ableton mapping) -| Ableton | Here | -| --- | --- | -| Automation triangle on track header | Expand toggle on audio track rows in the timeline gutter | +| Ableton | Here | +| --------------------------------------------- | ------------------------------------------------------------------------------------------------------ | +| Automation triangle on track header | Expand toggle on audio track rows in the timeline gutter | | One parameter per lane, selector at lane left | Same. Selector lists `Volume` + every automatable param of every chain node (`Compressor · Threshold`) | -| Breakpoint envelope over the clip | SVG envelope drawn over the existing waveform, clip-local | -| Double-click segment → add point | Same | -| Drag point (value tooltip) | Same; tooltip shows value + unit from the registry | -| Drag segment vertically → bend curvature | Same (Phase 2; format supports it from v1) | -| Delete key / right-click → remove point | Same | -| Dimmed line when no automation | Flat line at the current static value; first edit creates the lane | +| Breakpoint envelope over the clip | SVG envelope drawn over the existing waveform, clip-local | +| Double-click segment → add point | Same | +| Drag point (value tooltip) | Same; tooltip shows value + unit from the registry | +| Drag segment vertically → bend curvature | Same (Phase 2; format supports it from v1) | +| Delete key / right-click → remove point | Same | +| Dimmed line when no automation | Flat line at the current static value; first edit creates the lane | Lane height ~48 px expanded. Multiple lanes per track may be open at once (one per parameter), matching Ableton's "+" lanes — Phase 2; V1 shows one lane @@ -75,7 +75,10 @@ coalesced per gesture — free. Serialised on the element, versioned, same pattern as `data-fx-chain`: ```html - ``` - **`t`** — seconds, **clip-local** (relative to the element's `data-start`). @@ -96,7 +100,7 @@ Serialised on the element, versioned, same pattern as `data-fx-chain`: (dB for a compressor threshold, Hz for a cutoff). Volume is **linear 0..1**, consistent with `data-volume` and the existing linear-domain envelope machinery — no dB conversion enters the volume path. -- **`curve`** — optional, `-1..1`, curvature of the segment *leaving* this +- **`curve`** — optional, `-1..1`, curvature of the segment _leaving_ this point. `0`/absent = linear. Power-curve bend, Ableton-style. - **`target`** — `"volume"` or `"fx.."`. @@ -106,6 +110,7 @@ so reordering the chain never re-targets a lane. Chains without ids stay valid — they just can't be automation targets until the panel touches them. **Normalization** (`normalizeAutomation`, mirrors `normalizeAudioFxParams`): + - points sorted by `t`; duplicate `t` keeps the later point - `v` clamped to the target's registry range; non-finite → point dropped - lanes targeting a node id that no longer exists in the chain are **dropped** @@ -143,6 +148,7 @@ play / seek / rate change with the clip's `elapsed` offset: (§8) of the chain instance spliced for this source. Mechanics per lane, at schedule time: + 1. Convert clip-local envelope → context-time segments starting at `scheduledAt`, offset by `elapsed`, scaled by playback rate. 2. Linear segments → `setValueAtTime` + `linearRampToValueAtTime` (log-domain @@ -180,17 +186,18 @@ mid-playback without rescheduling the source. **Automatable in V1** (param maps to a real AudioParam): -| Effect | Params | -| --- | --- | -| Peaking / shelves | frequency, gain, Q | -| High/low-pass (2-pole) | frequency, Q | -| Delay | time (delayTime), feedback, mix | -| Chorus | rate, depth, mix | -| Phaser | rate, wet/dry gains | -| Reverb | wet, dry | -| *Volume* | (transport gainNode) | +| Effect | Params | +| ---------------------- | ------------------------------- | +| Peaking / shelves | frequency, gain, Q | +| High/low-pass (2-pole) | frequency, Q | +| Delay | time (delayTime), feedback, mix | +| Chorus | rate, depth, mix | +| Phaser | rate, wet/dry gains | +| Reverb | wet, dry | +| _Volume_ | (transport gainNode) | **Not automatable in V1**, greyed out in the selector, with reasons: + - **Worklet effects** (compressor, limiter, gate, bitcrush): params travel by `postMessage`, not AudioParams. V2 path: declare `parameterDescriptors` in the processors and read `parameters` in @@ -227,14 +234,14 @@ mid-playback without rescheduling the source. ## 11. PR breakdown (all < 1000 LOC) -| PR | Scope | Est. LOC | -| --- | --- | --- | -| A `wa-10-automation-model` | core: types, parse/normalize/serialize, `sampleAutomationLane`, curvature math, chain node ids, lint rule | ~450 | -| B `wa-11-param-exposure` | core: `automatable` flags, `FxNodeHandle.params`, invariant test | ~350 | -| C `wa-12-preview-scheduling` | core: transport + attach-path scheduling, cancel/re-schedule on live edit | ~400 | -| D `wa-13-render-scheduling` | core/engine: offline scheduling in runtime entry, volume→bake bridge, sweep fixture test | ~350 | -| E `wa-14-lane-ui` | studio: lane component, expand toggle, selector, point editing, orphan cleanup | ~800 | -| F `wa-15-curvature` (Phase 2) | studio: segment-bend drag; worklet `parameterDescriptors` migration | ~300+ | +| PR | Scope | Est. LOC | +| ----------------------------- | --------------------------------------------------------------------------------------------------------- | -------- | +| A `wa-10-automation-model` | core: types, parse/normalize/serialize, `sampleAutomationLane`, curvature math, chain node ids, lint rule | ~450 | +| B `wa-11-param-exposure` | core: `automatable` flags, `FxNodeHandle.params`, invariant test | ~350 | +| C `wa-12-preview-scheduling` | core: transport + attach-path scheduling, cancel/re-schedule on live edit | ~400 | +| D `wa-13-render-scheduling` | core/engine: offline scheduling in runtime entry, volume→bake bridge, sweep fixture test | ~350 | +| E `wa-14-lane-ui` | studio: lane component, expand toggle, selector, point editing, orphan cleanup | ~800 | +| F `wa-15-curvature` (Phase 2) | studio: segment-bend drag; worklet `parameterDescriptors` migration | ~300+ | A→B→C→D are dependency-ordered; E needs A+B (draws and writes) and benefits from C (audible while editing). F is optional polish. @@ -243,13 +250,13 @@ from C (audible while editing). F is optional polish. 1. **Volume lane display unit** — data stays linear either way; show the axis as % (matches `data-volume`) or dB (matches DAW muscle memory)? - *Default if unanswered: %.* + _Default if unanswered: %._ 2. **Curvature in V1?** Format supports it from day one regardless. Building - the bend-drag in V1 adds ~2 days to E. *Default: defer to F, straight lines - first.* + the bend-drag in V1 adds ~2 days to E. _Default: defer to F, straight lines + first._ 3. **Worklet-param automation deferral acceptable?** Compressor threshold - automation is the notable absence. *Default: defer; it's a self-contained - follow-up.* + automation is the notable absence. _Default: defer; it's a self-contained + follow-up._ 4. **Clip-envelope semantics confirmed?** Automation travels with the clip. - If you expected Ableton *arrangement* behaviour (stays put), say so now — + If you expected Ableton _arrangement_ behaviour (stays put), say so now — it changes the data model (composition-global times, stored off-element). diff --git a/plans/automation-time-selection-design.md b/plans/automation-time-selection-design.md index 8db4c4d34..884df6866 100644 --- a/plans/automation-time-selection-design.md +++ b/plans/automation-time-selection-design.md @@ -35,9 +35,9 @@ New store slice `automationSelectionSlice.ts` (own file — `playerStore.ts` sit ```ts interface AutomationSelection { elementKey: string; // which clip - target: string; // which lane ("volume" | "fx..") - t0: number; // clip-local seconds - t1: number; // > t0 + target: string; // which lane ("volume" | "fx..") + t0: number; // clip-local seconds + t1: number; // > t0 } // automationSelection: AutomationSelection | null // setAutomationSelection(sel), clearAutomationSelection() @@ -99,11 +99,11 @@ Right-click the selection rect → context menu: **Ramp up · Ramp down · Swell **Simplify**. Pure generators in `automationShapes.ts`, one shape scaled to the selection, values computed in unit space so log knobs behave: -| Shape | Points | Semantics | -| --------- | ------ | -------------------------------------------------------------------- | -| Ramp up | 2 | `range.min` at `t0` → envelope's own value at `t1` (fade in) | -| Ramp down | 2 | envelope's own value at `t0` → `range.min` at `t1` (fade out) | -| Swell | 3 | edge values, peak at `range.max` at the midpoint, `curve`-smoothed | +| Shape | Points | Semantics | +| --------- | ------ | ------------------------------------------------------------------------------ | +| Ramp up | 2 | `range.min` at `t0` → envelope's own value at `t1` (fade in) | +| Ramp down | 2 | envelope's own value at `t0` → `range.min` at `t1` (fade out) | +| Swell | 3 | edge values, peak at `range.max` at the midpoint, `curve`-smoothed | | Dip | 3 | edge values, midpoint at 25 % of the edge value in unit space (duck), smoothed | Point counts are tiny; the 512 cap is never approached. @@ -153,12 +153,12 @@ carve output and dense hand edits. Stacked on wa-14 (stack #3027), each under the 1000-LOC convention: -| PR | Content | ~LOC | -| ----- | ------------------------------------------------------------------------ | ---- | -| wa-15 | slice, drag gesture, rect render, `replaceRange`/`pointsIn`, Delete/Esc | 400 | -| wa-16 | shape generators, selection context menu (ramp/swell/dip), Simplify | 380 | -| wa-17 | clipboard, Cmd+C/V in `useAppHotkeys`, unit-space mapping | 300 | -| wa-18 | edge-handle stretch gesture | 250 | +| PR | Content | ~LOC | +| ----- | ----------------------------------------------------------------------- | ---- | +| wa-15 | slice, drag gesture, rect render, `replaceRange`/`pointsIn`, Delete/Esc | 400 | +| wa-16 | shape generators, selection context menu (ramp/swell/dip), Simplify | 380 | +| wa-17 | clipboard, Cmd+C/V in `useAppHotkeys`, unit-space mapping | 300 | +| wa-18 | edge-handle stretch gesture | 250 | wa-16/17/18 are independent once wa-15 lands. File-size note: `useAutomationLaneGestures` grows in wa-15 and wa-18 (310 lines today — headroom exists); the menu is a new file.