From 7393095be872d4eedadd221aefdcc40772547f6f Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Sun, 16 Aug 2026 16:29:48 -0700 Subject: [PATCH] fix(studio,core,engine): close the defects a max-effort review found in the fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A review of the five fix commits found eleven real defects, including a regression one of them introduced. Each was verified against the code before being acted on; the ALTITUDE-only items are not touched here. REGRESSION, from "group rows survive a collapse". Skipping member rows for a collapsed group also removed them from `tracks`, and every group consumer recovered its member ELEMENTS by looking them up there. Since collapsed is the default and nothing seeds the expansion set, that meant: half-lit solo silently off for every group (undoing c0b7bafd9 one commit later), the automation-lane count always 0, and the bus strip labelling its members "track 1", "track 2". Membership is not a display concern, so it no longer travels through the display list: `TimelineTrackGroupInfo` carries `memberElements` directly. Group bus. `reanchor` wrote `fader.gain.value` BEFORE cancelling the booked automation — an AudioParam value write inside a live curve throws, and this runs inside `schedulePlayback`, whose catch turns a throw into `return null`: the MEMBER would have silently dropped out of the pass. Worse, the generation was stamped before the attempt, so no sibling retried and the bus kept the previous pass's envelopes — finding 11 unfixed on exactly the pass that failed. Now: clear first, stamp only on success, and isolate the call. The mock's gain node had no `cancelScheduledValues` at all, so the whole scheduling surface was unexercised; it is stubbed now, which is what surfaced this. `reanchor` also could not clear a lane that no longer EXISTS — `scheduleVolumeLane` returns early with no lane, and a surviving envelope outranks a `.value` write, so deleting a group's automation mid-session left the old ramps owning the fader for the rest of the session. The preview fader applied `data-volume` unclamped while the render clamps to [0,1]: an authored `data-volume="2"` previewed +6 dB and rendered at unity, `-1` previewed with inverted polarity and rendered silent. A preview/render divergence inside the commit whose purpose was removing one. Pitch shift. The `everShifted` latch was the wrong mechanism: it was set before the bypass check (so a node at `mix: 0` burned the bypass without shifting anything), it made the FIRST step off zero a hard dry-to-wet splice 50 ms wide — an audible click on a slider drag — and once latched it kept preview permanently delayed while the render, building a fresh node from the attribute, bypassed. Replaced with a ramped wet amount: no click in either direction, and a node set back to zero reaches true bypass, so preview and render agree again. Silent no-ops. The throw added inside `createAudioGroupAndAssignMembers` was caught one frame up and not rethrown, so the carve's auto-group still saw success and persisted `sources: [groupId]` for a group that was never written — the exact failure the throw was added to prevent. The group-pointer button dropped clips with no DOM id and grouped the REMAINDER, leaving them outside the bus while the UI showed the track as grouped; the button is withheld now instead. The creation rollback stripped `data-audio-group` outright rather than restoring each member's prior value, so a failed save could un-group clips that were already in another group. `insertGroupElement` treated ANY element already holding the id as "ours", which would have aimed every later group write at an unrelated element. `setAudioMuteHidden` rescheduled Web Audio mid-play without `stopAll()`. Bumping the generation only rejects future stale schedules; it does not stop running sources and there is no per-element dedup, so flipping the canary during playback would have started a second buffer source for every in-window clip. `invalidateGroupInfoCache` was missed by the DOM-edit path: the rack reaches `` through the DOM editor, not through the timeline's writers. Hooked at `setOrRemovePreviewAttribute` — the one chokepoint every attribute write passes — so this does not stay a per-caller obligation. Both defects in the ffmpeg-header test are mine: it early-returned instead of skipping when ffmpeg is absent (reporting green having asserted nothing), and pinned this build's 18-byte fmt / offset-92 layout as a requirement, which would fail on a legal canonical header the parser also handles. Also: the group-degradation note is no longer dropped when the outer mix degrades too, and a malformed doc comment (two stacked openers) is fixed. Co-Authored-By: Claude Opus 5 (1M context) --- .../core/src/audio/audioFxWorklets.test.ts | 74 ++++++++++++++++--- packages/core/src/audio/audioFxWorklets.ts | 40 ++++++---- packages/core/src/runtime/init.ts | 11 ++- .../src/runtime/webAudioTransport.test.ts | 42 ++++++++++- .../core/src/runtime/webAudioTransport.ts | 38 +++++++++- packages/engine/src/services/audioMixer.ts | 11 ++- .../src/services/audioVolumeEnvelope.test.ts | 17 +++-- .../src/services/audioVolumeEnvelope.ts | 1 - .../src/components/editor/useFxCarve.ts | 2 + .../components/editor/useFxCarveGrouping.ts | 18 ++++- .../src/hooks/timelineAudioGroupCreate.ts | 49 +++++++++++- .../useAudioGroupCarveAssignment.test.tsx | 9 ++- .../src/hooks/useDomEditAttributeCommits.ts | 13 ++++ .../player/components/TimelineGroupRow.tsx | 13 ++-- .../src/player/components/TimelineLanes.tsx | 1 - .../player/components/TimelineTrackHeader.tsx | 19 +++-- .../components/timelineKeyboardNavigation.ts | 6 +- .../components/useTimelineTrackDerivations.ts | 15 +++- .../components/useTimelineTrackLayout.test.ts | 12 +++ 19 files changed, 325 insertions(+), 66 deletions(-) diff --git a/packages/core/src/audio/audioFxWorklets.test.ts b/packages/core/src/audio/audioFxWorklets.test.ts index cc9e89d9a..2f342f666 100644 --- a/packages/core/src/audio/audioFxWorklets.test.ts +++ b/packages/core/src/audio/audioFxWorklets.test.ts @@ -168,23 +168,77 @@ describe("the worklet processors themselves", () => { expect(maxErr).toBeLessThan(1e-6); }); - // A track whose semitones are automated THROUGH zero must not jump between - // the delayed and the undelayed path — that discontinuity is a click, which - // is worse than the delay the bypass would save. - it("keeps processing at zero once it has shifted, rather than clicking to dry", async () => { + /** 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 } }); - const input = sine(440, 0.4); - run(p, input); + run(p, sine(440, 0.3)); p.p = { ...p.p, semitones: 0 }; - const output = run(p, sine(440, 0.4)); - // Still the wet path (grain-delayed), so it does NOT equal the input. + 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 < 4000; i++) { + for (let i = 0; i < input.length; i++) { maxErr = Math.max(maxErr, Math.abs((output[i] ?? 0) - (input[i] ?? 0))); } - expect(maxErr).toBeGreaterThan(1e-3); + 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 diff --git a/packages/core/src/audio/audioFxWorklets.ts b/packages/core/src/audio/audioFxWorklets.ts index 673dba725..5aa3d9fc1 100644 --- a/packages/core/src/audio/audioFxWorklets.ts +++ b/packages/core/src/audio/audioFxWorklets.ts @@ -257,11 +257,17 @@ class HfPitchshift extends AudioWorkletProcessor { // 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; - // Latched the first time a non-zero shift is asked for. The no-op bypass - // below must not engage for a track whose semitones are AUTOMATED through - // zero: switching between a delayed and an undelayed path mid-signal is a - // click, which is worse than the delay it would save. - this.everShifted = false; + // 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 }; @@ -274,7 +280,6 @@ 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)); - if (semitones !== 0) this.everShifted = true; const grain = this.grain; const ringLen = grain * 2; const n = i[0] ? i[0].length : 0; @@ -282,12 +287,16 @@ class HfPitchshift extends AudioWorkletProcessor { if (!this.buf[ch]) this.buf[ch] = new Float32Array(ringLen); } - // A node asking for no shift at all, or mixed fully out, is transparent. - // The grain delay is ~grain/2 whatever the ratio, so at semitones=0 this - // used to degrade into a pure 50 ms delay of the signal — while the copy - // for that exact setting reads "Unchanged pitch". The ring keeps filling - // so a later shift does not start cold. - if (mix === 0 || (semitones === 0 && !this.everShifted)) { + // 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++) { @@ -304,7 +313,8 @@ class HfPitchshift extends AudioWorkletProcessor { const ratio = Math.pow(2, semitones / 12); const inc = (1 - ratio) / grain; - let write = this.write, phase = this.phase, filled = this.filled; + 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); @@ -313,7 +323,8 @@ class HfPitchshift extends AudioWorkletProcessor { // 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; - const wetMix = mix * warm; + 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]; @@ -329,6 +340,7 @@ class HfPitchshift extends AudioWorkletProcessor { this.write = write; this.phase = phase; this.filled = filled; + this.wet = wetNow; return true; } } diff --git a/packages/core/src/runtime/init.ts b/packages/core/src/runtime/init.ts index 161ac30f1..0f03fd83d 100644 --- a/packages/core/src/runtime/init.ts +++ b/packages/core/src/runtime/init.ts @@ -205,8 +205,15 @@ export function initSandboxRuntimeModular(): void { if (silenceHiddenAudio === enabled) return; silenceHiddenAudio = enabled; // The active-clip set is built with this predicate baked in, so a flip - // mid-session has to rebuild it — same reason a `data-hidden` toggle does. - if (clock.isPlaying()) scheduleWebAudioForActiveClips(); + // mid-session has to rebuild it. `stopAll()` first: bumping the generation + // only rejects future STALE schedules, it does not stop sources already + // started, and there is no per-element dedup — so rescheduling on its own + // starts a second buffer source for every in-window clip on top of the ones + // still playing. `applyWebAudioRate` pairs the two for the same reason. + if (clock.isPlaying()) { + webAudio.stopAll(); + scheduleWebAudioForActiveClips(); + } }; // `_auto` is a Studio-internal keyframe marker (an auto-tracked endpoint the // parser reads back), NOT an animatable property. Register it as a no-op GSAP diff --git a/packages/core/src/runtime/webAudioTransport.test.ts b/packages/core/src/runtime/webAudioTransport.test.ts index d482eb3d3..f41c4d435 100644 --- a/packages/core/src/runtime/webAudioTransport.test.ts +++ b/packages/core/src/runtime/webAudioTransport.test.ts @@ -656,7 +656,24 @@ describe("WebAudioTransport", () => { addEventListener: vi.fn(), })), createGain: vi.fn(() => { - const node = { gain: { value: 1 }, connect: vi.fn(), disconnect: vi.fn() }; + // The AudioParam scheduling surface is part of the contract the group + // bus uses (`clearParamLane` cancels before re-seeding a reused bus). + // A bare `{ value }` made any such call throw, and `schedulePlayback` + // swallows throws into `return null` — so the mock's own gap read as + // "the member did not play" rather than as a missing stub. + const node = { + gain: { + value: 1, + cancelScheduledValues: vi.fn(), + cancelAndHoldAtTime: vi.fn(), + setValueAtTime: vi.fn(), + linearRampToValueAtTime: vi.fn(), + exponentialRampToValueAtTime: vi.fn(), + setValueCurveAtTime: vi.fn(), + }, + connect: vi.fn(), + disconnect: vi.fn(), + }; gainNodes.push(node); return node; }), @@ -866,6 +883,29 @@ describe("WebAudioTransport", () => { expect(fader.gain.value).toBe(0); }); + // reanchor runs inside schedulePlayback, whose catch turns any throw into + // `return null` — so a bus that fails to re-anchor would silently take the + // MEMBER out of the pass, and a generation stamped before the attempt would + // stop every later member retrying. + it("keeps the member playing when re-anchoring the bus throws", async () => { + document.body.innerHTML = ``; + const { transport, mock, gen } = setupGroupTransport(); + await scheduleGrouped(transport, gen, "a", "vo"); + const fader = mock.gainNodes[5]!; + fader.gain.cancelScheduledValues = vi.fn(() => { + throw new Error("param is not schedulable"); + }); + + transport.stopAll(); + const gen2 = transport.startGeneration(); + + await expect(scheduleGrouped(transport, gen2, "a", "vo")).resolves.not.toBeNull(); + // Generation not consumed by the failed attempt, so a sibling still tries. + fader.gain.cancelScheduledValues = vi.fn(); + await scheduleGrouped(transport, gen2, "b", "vo"); + expect(fader.gain.value).toBeCloseTo(0.5, 6); + }); + describe('solo — "Hear only this" (B5)', () => { it("silences a non-soloed member via its own solo gain, without touching the group bus", async () => { const { transport, mock, gen } = setupGroupTransport(); diff --git a/packages/core/src/runtime/webAudioTransport.ts b/packages/core/src/runtime/webAudioTransport.ts index 3d32e4d2f..f46a59b83 100644 --- a/packages/core/src/runtime/webAudioTransport.ts +++ b/packages/core/src/runtime/webAudioTransport.ts @@ -1,5 +1,6 @@ import { attachElementFxChain, readElementAutomation, type ElementFxHandle } from "./audioFx.js"; import { + clearParamLane, scheduleParamLane, volumeLane, type AutomationTiming, @@ -16,6 +17,18 @@ function normalizeRate(rate: number): number { return rate; } +/** + * The render puts every track volume through its own `clampVolume` before + * building the filter, so an authored `` + * renders at unity. Preview has to agree or the two diverge on exactly the + * attribute this bus exists to honour — and a negative value would invert + * polarity in preview while rendering silent. Compositions are hand-authorable, + * so out-of-range values do not need the studio slider to be reachable. + */ +function clampGroupVolume(volume: number): number { + return Math.max(0, Math.min(1, volume)); +} + /** * Breadcrumb for the per-element-mute handoff: the transport just claimed a track * that was audibly playing through the HTMLMedia fallback. Quiet unless @@ -323,8 +336,18 @@ export class WebAudioTransport { // which for a fade-out is silence for the rest of the session. Re-anchor // once per play generation, not once per member scheduled. if (existing.generation !== this._playGeneration) { - existing.generation = this._playGeneration; - existing.reanchor(timing); + // Stamped only on success, and isolated: this runs inside + // `schedulePlayback`, whose catch turns any throw into `return null` — + // i.e. a bus problem would silently drop the MEMBER from the pass. And + // stamping first would consume the generation, so no later member of + // the same group would retry and the bus would keep the previous pass's + // envelopes: finding 11 unfixed on exactly the pass that failed. + try { + existing.reanchor(timing); + existing.generation = this._playGeneration; + } catch (err) { + swallow("webAudioTransport.groupReanchor", err); + } } return existing.input; } @@ -355,7 +378,7 @@ export class WebAudioTransport { // group effect — a compressor, the Giant preset — previewed differently // than it rendered. const fader = this._ctx.createGain(); - fader.gain.value = readAudioGroupVolume(groupEl); + fader.gain.value = clampGroupVolume(readAudioGroupVolume(groupEl)); fader.connect(muteGain); const fx = attachElementFxChain( this._ctx, @@ -375,7 +398,14 @@ export class WebAudioTransport { fx, generation: this._playGeneration, reanchor: (at: AutomationTiming) => { - fader.gain.value = readAudioGroupVolume(groupEl); + // Cleared BEFORE the value write, and unconditionally. `scheduleVolumeLane` + // clears as part of scheduling, but returns early when the group no + // longer has a lane — and a scheduled envelope outranks a `.value` + // write, so deleting a group's automation mid-session otherwise left + // the previous pass's ramps still owning the param (for a fade-out, + // silence) for the rest of the session. + clearParamLane([{ param: fader.gain }]); + fader.gain.value = clampGroupVolume(readAudioGroupVolume(groupEl)); fx?.reanchor(at); if (groupEl) scheduleVolumeLane(groupEl, fader, at); }, diff --git a/packages/engine/src/services/audioMixer.ts b/packages/engine/src/services/audioMixer.ts index 7c3d3d891..df93dc62f 100644 --- a/packages/engine/src/services/audioMixer.ts +++ b/packages/engine/src/services/audioMixer.ts @@ -1436,15 +1436,18 @@ export async function processCompositionAudio( // A group whose sub-mix had to drop member automation reports it the same // way mixAudioTracks reports its own degradation: on a SUCCESSFUL result, so // the render ships and the caller can still say what was lost. - const degradedGroups = [...groupsDegradedAutomation]; const degradedNote = - degradedGroups.length > 0 - ? `Volume automation exceeded this ffmpeg build's expression limits in group(s) ${degradedGroups.join(", ")}; rendered at base volume` + groupsDegradedAutomation.length > 0 + ? `Volume automation exceeded this ffmpeg build's expression limits in group(s) ${groupsDegradedAutomation.join(", ")}; rendered at base volume` : undefined; return { ...mixResult, durationMs: Date.now() - startMs, - error: mixResult.error ?? degradedNote, + // Both, when both degraded. `mixResult.error ?? degradedNote` reported only + // the outer mix and dropped the one that names which GROUPS lost their + // members' automation — two different losses, and the operator needs to + // hear about the one they can act on. + error: [mixResult.error, degradedNote].filter(Boolean).join("; ") || undefined, }; } diff --git a/packages/engine/src/services/audioVolumeEnvelope.test.ts b/packages/engine/src/services/audioVolumeEnvelope.test.ts index 04a200ed2..5d453181a 100644 --- a/packages/engine/src/services/audioVolumeEnvelope.test.ts +++ b/packages/engine/src/services/audioVolumeEnvelope.test.ts @@ -8,6 +8,7 @@ import { applyVolumeEnvelopeToWav } from "./audioVolumeEnvelope.js"; const SAMPLE_RATE = 48000; const CHANNELS = 2; +const HAS_FFMPEG = spawnSync(getFfmpegBinary(), ["-version"], { encoding: "utf-8" }).status === 0; /** Build a PCM s16le stereo WAV whose every sample equals `value`. */ function writeConstantWav(path: string, frames: number, value: number): void { @@ -255,7 +256,7 @@ describe("applyVolumeEnvelopeToWav", () => { * not read a real one — and an unreadable file returns false, which the * caller reads as "no automation here" and drops the group's envelope. */ - it("reads what ffmpeg actually writes, not just a canonical header", () => { + it.skipIf(!HAS_FFMPEG)("reads what ffmpeg actually writes, not just a canonical header", () => { const path = join(tmp(), "ffmpeg-f32.wav"); const made = spawnSync( getFfmpegBinary(), @@ -275,11 +276,13 @@ describe("applyVolumeEnvelopeToWav", () => { ], { encoding: "utf-8" }, ); - if (made.status !== 0) return; // no usable ffmpeg here + expect(made.status).toBe(0); const before = readFileSync(path); - // Non-canonical by construction: prove the fixture is the awkward shape. - expect(before.readUInt32LE(16)).toBe(18); // fmt chunk size + // The format tag is the load-bearing part; the chunk LAYOUT is this + // build's quirk, so it is logged as context rather than required — a + // build emitting a canonical 16-byte fmt with data at 44 is legal and + // handled, and pinning 18/92 would fail on the good case. expect(before.readUInt16LE(20)).toBe(3); // WAVE_FORMAT_IEEE_FLOAT expect( @@ -307,8 +310,10 @@ describe("applyVolumeEnvelopeToWav", () => { } at += 8 + size + (size % 2); } - expect(dataOffset).toBeGreaterThan(44); - // Faded to silence by the end (stereo float = 8 bytes per frame). + expect(dataOffset).toBeGreaterThan(0); + // Faded to silence by the end (stereo float = 8 bytes per frame). This is + // the assertion that matters: the parser read a real file and the bake + // landed, whatever chunk layout the build chose. expect(Math.abs(after.readFloatLE(dataOffset + (SAMPLE_RATE - 2) * 8))).toBeLessThan(0.02); }); }); diff --git a/packages/engine/src/services/audioVolumeEnvelope.ts b/packages/engine/src/services/audioVolumeEnvelope.ts index 4213e2939..47064bc95 100644 --- a/packages/engine/src/services/audioVolumeEnvelope.ts +++ b/packages/engine/src/services/audioVolumeEnvelope.ts @@ -122,7 +122,6 @@ export function createEnvelopeWalker( }; } -/** /** Every sample scaled by the envelope, in place, in whichever of the two * formats the layout reports. Float is NOT clamped: it is the format the group * sub-mix writes precisely so an over-unity sum keeps its headroom until diff --git a/packages/studio/src/components/editor/useFxCarve.ts b/packages/studio/src/components/editor/useFxCarve.ts index f2b7da221..1cb6f531c 100644 --- a/packages/studio/src/components/editor/useFxCarve.ts +++ b/packages/studio/src/components/editor/useFxCarve.ts @@ -32,6 +32,7 @@ import { import { resolveAudioGroups, resolveCarveSourceIds } from "@hyperframes/core/audio-groups"; import { collectCarveCandidates, + CARVE_ABORTED, isPromiseLike, resolveNextCarveSettings, } from "./useFxCarveGrouping.js"; @@ -425,6 +426,7 @@ export function useFxCarve( // awaited: see resolveNextCarveSettings's own contract. const resolved = resolveNextCarveSettings(nextRaw, doc, onAutoGroupCarveSources); const next = isPromiseLike(resolved) ? await resolved : resolved; + if (next === CARVE_ABORTED) return; // Which of the carve's settings moved. One event per change with the action // named, rather than a single "carve touched" — enabling a carve and nudging // its strength are different decisions and the interesting question (do diff --git a/packages/studio/src/components/editor/useFxCarveGrouping.ts b/packages/studio/src/components/editor/useFxCarveGrouping.ts index 18bf46166..70c08e630 100644 --- a/packages/studio/src/components/editor/useFxCarveGrouping.ts +++ b/packages/studio/src/components/editor/useFxCarveGrouping.ts @@ -30,6 +30,12 @@ export function isPromiseLike(value: T | Promise): value is Promise { return typeof (value as { then?: unknown })?.then === "function"; } +/** + * "The auto-group failed, do not persist this carve" — distinct from a + * legitimate `null`, which means the carve was deliberately cleared. + */ +export const CARVE_ABORTED = Symbol("carve-aborted"); + /** * Plural voiceover carve, always against a group — normative, not a * suggestion (groups doc §1.6). Picking a second ungrouped voice clip mints a @@ -71,8 +77,16 @@ export function resolveNextCarveSettings( nextRaw: HfCarveSettings | null, doc: Document | undefined, assignGroup: ((clipIds: readonly string[], groupId: string) => Promise) | undefined, -): HfCarveSettings | Promise | null { - return nextRaw && doc ? withAutoGroupedSources(doc, nextRaw, assignGroup) : nextRaw; +): HfCarveSettings | Promise | null { + const resolved = nextRaw && doc ? withAutoGroupedSources(doc, nextRaw, assignGroup) : nextRaw; + // A failed auto-group resolves to the sentinel rather than rejecting: the + // write has already toasted, and the caller's job is simply not to persist a + // carve whose `sources` name a group that was never written — which reads, at + // playback, as a carve that silently stops ducking. Caught here rather than + // in the caller so the synchronous branch above stays synchronous. + return isPromiseLike(resolved) + ? resolved.catch((): typeof CARVE_ABORTED => CARVE_ABORTED) + : resolved; } export interface CarveCandidate { diff --git a/packages/studio/src/hooks/timelineAudioGroupCreate.ts b/packages/studio/src/hooks/timelineAudioGroupCreate.ts index 89f2a2fde..9f39df2b8 100644 --- a/packages/studio/src/hooks/timelineAudioGroupCreate.ts +++ b/packages/studio/src/hooks/timelineAudioGroupCreate.ts @@ -28,21 +28,47 @@ import { type UseTimelineElementVisibilityEditingInput, } from "./timelineTrackVisibility"; +/** + * Assign (or restore) `data-audio-group` across a set of members. + * + * `restore` carries each member's PRIOR value so the unwind can put back a + * membership that already existed, rather than removing the attribute outright. + * `setElementsHidden`, which this mirrors, gets away with a plain `!hidden` + * because hidden is boolean; group membership is an arbitrary id, and the carve + * path does not check whether a clip is already grouped — so a failed save + * could silently un-group clips that belonged to another group before it. + */ function patchLiveAudioGroupState( iframe: HTMLIFrameElement | null, elements: readonly TimelineElement[], groupId: string | null, activeCompPath: string | null, + restore?: ReadonlyMap, ): void { for (const element of elements) { const target = findTimelineElementInIframe(iframe, element, activeCompPath); if (!target) continue; - if (groupId) target.setAttribute(HF_AUDIO_GROUP_ATTR, groupId); + const next = restore ? (restore.get(element) ?? null) : groupId; + if (next) target.setAttribute(HF_AUDIO_GROUP_ATTR, next); else target.removeAttribute(HF_AUDIO_GROUP_ATTR); } invalidateGroupInfoCache(iframe?.contentDocument); } +/** Each member's `data-audio-group` before this write, for the unwind. */ +function captureAudioGroupState( + iframe: HTMLIFrameElement | null, + elements: readonly TimelineElement[], + activeCompPath: string | null, +): Map { + const prior = new Map(); + for (const element of elements) { + const target = findTimelineElementInIframe(iframe, element, activeCompPath); + prior.set(element, target?.getAttribute(HF_AUDIO_GROUP_ATTR) ?? null); + } + return prior; +} + /** Group ids are interpolated into markup and into a render-side filename, so * they stay in the character set an HTML id and a path can both carry. */ const GROUP_ID_PATTERN = /^[A-Za-z0-9_-]+$/; @@ -63,7 +89,17 @@ const GROUP_ID_PATTERN = /^[A-Za-z0-9_-]+$/; * buys nothing. */ function insertGroupElement(html: string, groupId: string): string { - if (readTagSnippetByTarget(html, { id: groupId }) !== undefined) return html; + const existing = readTagSnippetByTarget(html, { id: groupId }); + if (existing !== undefined) { + // Only OUR tag counts as "already there". The id was minted against the + // live preview document, which does not contain markup that is on disk but + // not rendered (inside a `