feat(core): pitch shift — a granular shifter as the fifth FX worklet - #3276
Conversation
miga-heygen
left a comment
There was a problem hiding this comment.
Review
Approve. Independent read at exact head.
DSP is correct: dual-tap granular delay line, 100ms grain, taps 180° apart with sin crossfade. Rate derived from 2^(semitones/12). write/phase advanced per-sample outside the channel loop — correct, prevents 2x/4x speed on stereo/quad. Ring buffer lazy-initialized per channel, sized to grain * 2.
Follows every existing worklet convention: workletBuilder("hf-pitchshift") in BUILDERS, __hfDispose message handling, parse/serialize allow-list entry in HF_AUDIO_FX with correct param ranges (semitones ±12, mix 0–1). FX copy, summary, and tail all added. Tail of 0.2s matches the two 100ms grains.
Tail code refactored (reverb/delay/chorus extracted to named functions) — clean and consistent with the new pitchshiftTail.
Tests: identity at 0 semitones (exact delay = grain/2 + 1 verified sample-by-sample), octave up at +12, octave down at -12, plus a browser-render integration test with measurement window bounded to avoid tail dilution. Solid coverage.
No issues. Ship it.
— Miga
terencecho
left a comment
There was a problem hiding this comment.
Approve — independent read at head 0ec9de40, differentiated from Miga's prior approve.
DSP correctness (things I verified end-to-end)
Overlap-add gain is equal-power, not equal-amplitude — and that's correct here.
With gA = sin(π·phase), gB = sin(π·(phase+0.5)) = cos(π·phase), we get gA² + gB² = sin²α + cos²α = 1 for all phases. Since the two taps read different parts of the ring buffer (delays offset by grain/2), their outputs are effectively decorrelated audio — powers sum, so the wet path holds constant loudness across the crossfade. The only degenerate case is semitones=0: inc=0 pins phase=0, which pins gA=0, gB=1, collapsing to a single tap at grain/2 + 1 — exactly what the unit test asserts to <1e-6.
Ring-buffer indexing has no negative-modulo footgun.
readTap: pos = (write - 1 - delaySamples + len) % len with delaySamples ∈ [0, grain) and len = 2·grain. Worst-case dividend is write=0, delaySamples=grain-1 → 0 - 1 - (grain-1) + 2·grain = grain > 0. Always positive before the %, so JS's sign-preserving % behaves.
Negative-phase wrap is handled.
For ratio > 1 (shift up), inc = (1-ratio)/grain < 0, so phase decrements. phase -= Math.floor(phase) uses floor-toward-negative-infinity, so -0.3 - (-1) = 0.7 — phase stays in [0, 1) correctly. phaseB = (phase + 0.5) % 1 is likewise safe because phase + 0.5 ≥ 0.5.
Block-level state scoping — the stereo/quad correctness comment is load-bearing and honored.
write and phase are hoisted out of the channel loop and advanced once per sample across all channels, not per-channel. Confirmed by reading the loop: the for (let s = 0; s < n; s++) block advances phase += inc and write = (write + 1) % ringLen outside the for (let ch...) inner loop. Advancing them inside would move the taps N× too fast on an N-channel input (as the comment warns).
Worklet contract holds on empty / degenerate frames.
if (!i || !i.length) return true; catches the no-input case. If i[0] exists but is length 0, n = 0 and the sample loop no-ops — this.write/this.phase are untouched, so downstream state is preserved. dead short-circuits after __hfDispose. No NaN/Infinity risk in the hot path: semitones and mix are clamped, Math.pow(2, clamped/12) is finite, sin(π·phase) is finite. Ring buffer starts zero-initialized.
Concur with Miga
Dual-tap granular delay, 100ms grain, taps 180° apart with sine crossfade, ratio = 2^(semitones/12). Correct algorithm. Tail = 0.2s = 2 × grain, matches the chain-tail model.
Integration + tests
- Registry (
audioFxGraph.ts), FX definition (audioFx.ts), copy (audioFxCopy.ts), tail (audioFxTail.ts) all follow the four pre-existing worklet patterns exactly. audioFxTail.tsrefactor from inline switch cases to per-effect helpers is a clean lift — no semantic change, and the PR description flags it was done to keep complexity under threshold.- Unit tests are behavioral, not just presence:
semitones=0verifies bit-exact passthrough at the expected delay,±12verifies frequency doubling/halving via zero-crossing rate on real 440/220 Hz sines. Browser-render test empirically proves preview↔render parity (not just structural equivalence) by measuring output frequency after an octave-up shift on a real WAV. estimateFreqin the browser test correctly bounds the measurement window to[0.05s, 0.45s)— avoids the 0.2s tail past the input diluting the crossing count. This is a subtle-but-right correction that pairs withpitchshiftTail.
CI
26 required checks green at head 0ec9de40; mergeStateStatus: CLEAN; mergeable: MERGEABLE. Stack-child so this is the reduced matrix, but Preflight/regression-shards×9/Perf/preview-parity all passed.
Minor (non-blocking) — for later polish, not for this PR
semitonesis integer-stepped (step: 1). Fine for a classic UI, but future work might want cents granularity — trivial change (widen tostep: 0.5orstep: 0.01) since the math already uses continuousratio = 2^(semitones/12).- Parameter changes via
port.onmessageland at block boundaries with no smoothing. Zipper-free for slow user-driven changes (which is the actual use case), but if a lane ever automatessemitonesat UI rate, could hear discontinuities. Same tradeoff every other FX worklet in this rack made.
Ship.
— Review by tai (pr-review)
The base branch was changed.
Adds hf-pitchshift alongside the four existing dynamics worklets: a dual-tap granular delay line, 100 ms grain, taps 180° apart so one is always crossfading in as the other resets — hides the splice each tap makes on wrap. Read-tap speed relative to the write head tracks the semitone ratio, so pitch shifts without changing duration. Registered through the same workletBuilder/dispose-message path the other four use (so shapeOf never rebuilds on a param tweak, and a chain drop retires it), wired into the registry with a plain-language copy entry and a ~0.2s chain tail (two grains). One implementation, shared by preview (Web Audio in the page) and render (the same worklet run inside an OfflineAudioContext in the headless browser) — confirmed by a browser-render test that measures the actual output frequency, not just that it differs from input. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
0ec9de4 to
1fb7d0a
Compare
Summary
hf-pitchshift, a fifth AudioWorklet FX processor alongside compressor/limiter/gate/bitcrush: a dual-tap granular delay line (100 ms grain, taps 180° apart) that shifts pitch without changing playback duration.audioFx.ts), the graph builder (audioFxGraph.ts, sameworkletBuilder/dispose-message contract as the other four worklets), plain-language copy (audioFxCopy.ts, no speech words per the audit test), and the chain-tail model (audioFxTail.ts, ~0.2s for two grains).OfflineAudioContextin the headless browser) — no separate ffmpeg filter needed, matching how the other four worklet effects already work.nodeTail's switch into small per-effect helper functions while adding thepitchshiftcase, to keep it under the complexity threshold (fallow was already close before this change).Test plan
bun run build(regeneratespackages/core/src/generated/audio-fx-runtime-inline.tsfrom source — never hand-edited)cd packages/core && bun run test— 2344/2344 pass, including newHfPitchshiftunit tests (semitones:0 exact passthrough at the grain/2 delay, semitones:±12 octave shift via zero-crossing frequency estimate)cd packages/engine && bunx vitest run src/services/audioFxRender.test.ts— 16/16 pass, including a new browser-render test that measures the actual output frequency after an octave-up shift (proves render/preview parity empirically, not just structurally)cd packages/engine && bun run test— 1527 passed, 3 skipped (pre-existing), no regressionsbunx oxfmt/bunx oxlintclean on all touched filesStacks on #3275 (A2) and #3274 (A1) — should merge after both.
🤖 Generated with Claude Code