From fb0970ccd9d62c6482520e235b08ec3491d271be Mon Sep 17 00:00:00 2001 From: Thales Pereira <31625914+thcp@users.noreply.github.com> Date: Tue, 8 Sep 2026 14:11:39 +0100 Subject: [PATCH 1/3] feat(export): carry the mixer's transpose into the exported file Transposition happens in the browser, in a SoundTouch AudioWorklet. The server has no pitch stage at all, so a mix exported from a transposed session came back in the original key with nothing to say it had. The export now takes a per-lane semitone shift and applies it with ffmpeg's rubberband filter. Measured on the bundled build: duration holds to the microsecond across the whole -6..+6 range and the pitch lands within 1 Hz of target, which matters because a lane whose length moved would desync the mix. asetrate/atempo was not used for that reason. Drums are never transposed, on the server as on the client. Resampling a snare does not move it to another key, it makes it a different drum, and effective_pitch here mirrors effectivePitch in static/js/pitchBus.js so an export cannot contain something the studio refuses to play. "original" needed more than a filter. It is the complement lane, so it usually has drums summed into it, and shifting that one file would resample them. Playback never does: buildPlaybackStems rebuilds the lane from the component stems and routes drums around the pitch stage. The export now does the same, and falls back to leaving the lane at its original key when a component is missing -- an older job that kept only the rendered mix -- which is the same conservative choice playback makes there. librubberband is GPL and plenty of builds omit it: the bundled macOS ffmpeg has it, the Homebrew one on a dev machine does not. So it is probed once per process, and a transpose asked of a build that cannot do it is refused with a reason rather than served in the original key, which would look exactly like the bug this fixes. An export with no transpose never probes at all and renders exactly as it did before. The video mux takes the same parameter. An MP4 whose audio ignored the transpose would be this issue reported a second time. The shift enters the render cache key only when something is actually shifted, so every export made before this keeps the entry it already had, and a client that predates it omits the parameter and is unchanged. Closes #592 --- app/api/stems.py | 217 +++++++++++++++++++++++++++++++-- static/js/player.js | 14 ++- tests/test_transpose_export.py | 208 +++++++++++++++++++++++++++++++ 3 files changed, 428 insertions(+), 11 deletions(-) create mode 100644 tests/test_transpose_export.py diff --git a/app/api/stems.py b/app/api/stems.py index c5743557..6f4cab1c 100644 --- a/app/api/stems.py +++ b/app/api/stems.py @@ -119,6 +119,7 @@ def _mixdown_cache_key( start: float | None, end: float | None, click_lane: tuple[Path, float] | None = None, + pitches: list[int] | None = None, ) -> str: """Every render input is in the key, so a different mixer state, region, or a Settings change (export sample rate) misses cleanly -- never a stale @@ -139,6 +140,10 @@ def _mixdown_cache_key( # The click's own cache key already encodes the grid, rate and # accent mode, so its filename plus gain fully identifies it. "" if click_lane is None else f"{click_lane[0].name}:{click_lane[1]:.6f}", + # Appended only when something is actually transposed, so every + # export made before #592 keeps the exact key it already had and + # the cache survives the change. + "" if not (pitches and any(pitches)) else "p" + ",".join(str(p) for p in pitches), ] ) # Cache key, not a security context -- usedforsecurity=False documents @@ -394,6 +399,97 @@ def _read_beat_grid(job_id: str) -> dict | None: return grid +# Transpose on export (#592). The studio shifts pitch in the browser with a +# SoundTouch AudioWorklet; the server has no such stage, so an export came out +# in the original key however the mixer was set. +# +# PITCH_MIN/MAX mirror static/js/pitchBus.js. Keep them in step: a UI that can +# ask for a shift the export rejects is worse than one that cannot ask. +PITCH_MIN = -6 +PITCH_MAX = 6 + +# Probed once per process; the ffmpeg binary cannot change mid-run. None until +# the first export needs to know. +_rubberband_cached: bool | None = None + + +def _rubberband_available() -> bool: + """Whether this ffmpeg can pitch-shift. + + librubberband is GPL and plenty of builds omit it -- the Homebrew ffmpeg on + a dev machine does, while the bundled macOS build does not -- so this is + probed rather than assumed. Any probe problem reads as "no": an export that + silently used a different algorithm would be worse than one that says it + cannot transpose. + + rubberband specifically, not asetrate/atempo. Measured on the bundled build: + rubberband holds duration to the microsecond across -6..+6 and lands within + 1 Hz of the target, and a lane whose length moved would desync the mix. + """ + global _rubberband_cached + if _rubberband_cached is None: + _rubberband_cached = False + try: + probe = subprocess.run( + [ffmpeg_executable(), "-hide_banner", "-filters"], + capture_output=True, + text=True, + timeout=10, + ) + _rubberband_cached = bool(re.search(r"\brubberband\b", probe.stdout)) + except (OSError, subprocess.SubprocessError): + pass + return _rubberband_cached + + +def effective_pitch(name: str, semitones: int) -> int: + """The shift a lane actually gets, mirroring effectivePitch in + static/js/pitchBus.js. + + Drums are never transposed, whatever the request says: resampling a snare + does not move it to another key, it makes it a different drum. The client + refuses this too, so honouring a drums pitch here would export something the + studio will not play. + """ + if name == "drums": + return 0 + return max(PITCH_MIN, min(PITCH_MAX, int(semitones))) + + +def _parse_pitches(pitches: str, names: list[str]) -> list[int]: + """Parse semitone shifts parallel to `names`, or all zeros when absent. + + Empty means "no transpose", which is what every client that predates #592 + sends by omitting the parameter -- so an old caller keeps its exact + behaviour rather than needing to opt out. + """ + if not pitches: + return [0] * len(names) + raw = [p for p in pitches.split(",") if p != ""] + if len(raw) != len(names): + raise HTTPException(status_code=422, detail="pitches must be the same length as stems") + try: + parsed = [int(p) for p in raw] + except ValueError: + raise HTTPException(status_code=422, detail="pitches must be whole semitones") from None + if any(p < PITCH_MIN or p > PITCH_MAX for p in parsed): + raise HTTPException( + status_code=422, detail=f"pitch out of range ({PITCH_MIN}..{PITCH_MAX})" + ) + return [effective_pitch(n, p) for n, p in zip(names, parsed, strict=True)] + + +def _pitch_filter(semitones: int) -> str: + """The rubberband stage for a lane, or "" when it is not being shifted. + + Trailing comma so it composes into the existing per-lane chain. Pitch is a + frequency ratio, hence the twelfth root of two. + """ + if semitones == 0: + return "" + return f"rubberband=pitch={2 ** (semitones / 12):.9f}," + + def _parse_lane_gains(stems: str, gains: str) -> tuple[list[str], list[float]]: """Parse and validate parallel comma-separated lane names and linear gains. Shared by the audio mixdown and the MP4 video mux. Raises HTTPException @@ -420,6 +516,70 @@ def _parse_lane_gains(stems: str, gains: str) -> tuple[list[str], list[float]]: return names, parsed_gains +class _RenderLane(NamedTuple): + """One ffmpeg input in the mixdown graph: a file, its gain, its shift.""" + + path: Path + gain: float + pitch: int + + +def _expand_render_lanes( + job_id: str, + names: list[str], + gains: list[float], + pitches: list[int], +) -> tuple[list[_RenderLane], list[str]]: + """Lanes to render, with "original" rebuilt from its parts when it is + transposed. Returns the lanes and the names of any lane left unpitched. + + "original" is the complement lane: whatever the user did not select, already + summed into one file -- which means it usually contains drums. Shifting that + file would resample the drums, so playback never does it. buildPlaybackStems + in static/js/playbackStems.js rebuilds the lane from the component stems + instead, routing drums around the pitch stage, and this is the same move on + the export side. + + When a component is missing -- an older job that kept only the rendered mix + -- the lane cannot be taken apart, so it stays at its original key. That is + deliberately the same conservative choice playback makes: melodic content + stays in the old key rather than the drums being resampled. + """ + lanes: list[_RenderLane] = [] + unpitched: list[str] = [] + selected = set(names) + # A completed lead/backing split stands in for the base vocals source, so + # vocals is not also part of the complement (mirrors buildPlaybackStems). + if {"lead_vocals", "backing_vocals"} <= selected: + selected.add("vocals") + + for name, gain, pitch in zip(names, gains, pitches, strict=True): + if name != "original" or pitch == 0: + lanes.append(_RenderLane(_validate_stem_path(job_id, name), gain, pitch)) + continue + + components = [n for n in STEM_NAMES if n not in selected] + paths: list[Path] = [] + for comp in components: + path = (JOBS_DIR / job_id / "stems" / f"{comp}.wav").resolve() + if not path.is_file() or not path.is_relative_to(JOBS_DIR.resolve()): + paths = [] + break + paths.append(path) + + if not paths: + lanes.append(_RenderLane(_validate_stem_path(job_id, name), gain, 0)) + unpitched.append(name) + continue + + # Each component carries the lane's own gain; amix sums them back to + # what the single file would have been. effective_pitch keeps drums at 0. + for comp, path in zip(components, paths, strict=True): + lanes.append(_RenderLane(path, gain, effective_pitch(comp, pitch))) + + return lanes, unpitched + + async def _drain_stderr(stream: asyncio.StreamReader, sink: deque[str]) -> None: """Collect ffmpeg stderr lines into a bounded deque. Draining is mandatory once stderr is a pipe -- an undrained full pipe would deadlock ffmpeg.""" @@ -822,6 +982,14 @@ async def get_mixdown( ext: str, stems: str = Query(..., description="Comma-separated lane names to sum"), gains: str = Query(..., description="Comma-separated linear gains, parallel to stems"), + pitches: str = Query( + default="", + max_length=128, + description=( + "Comma-separated semitone shifts parallel to stems (-6..6). " + "Empty means no transpose, which is what a client that predates this sends." + ), + ), start: float | None = Query(default=None, ge=0, description="Trim start in seconds"), end: float | None = Query(default=None, gt=0, description="Trim end in seconds"), click: bool = Query(default=False, description="Mix the click track into the export"), @@ -863,6 +1031,7 @@ async def get_mixdown( raise HTTPException(status_code=404, detail="not found") names, parsed_gains = _parse_lane_gains(stems, gains) + parsed_pitches = _parse_pitches(pitches, names) if (start is None) != (end is None) or (start is not None and start >= end): raise HTTPException( status_code=422, @@ -870,11 +1039,21 @@ async def get_mixdown( ) _validate_trim_range(job_id, start, end) + # Refuse rather than export in the wrong key. A build without librubberband + # cannot pitch-shift at all, and quietly returning the original key would + # look like the bug this feature exists to fix (#592). + if any(parsed_pitches) and not await asyncio.to_thread(_rubberband_available): + raise HTTPException( + status_code=422, + detail="this ffmpeg build cannot transpose (no rubberband filter)", + ) + # Validates job_id (404), job done (404), and path traversal (404) per # stem -- deliberately before the cache lookup below, so a deleted or # not-yet-done job 404s the same way it always has instead of serving a # stale cache entry from before the job was removed. - paths = [_validate_stem_path(job_id, name) for name in names] + render_lanes, unpitched = _expand_render_lanes(job_id, names, parsed_gains, parsed_pitches) + paths = [lane.path for lane in render_lanes] media_type = MIXDOWN_MEDIA_TYPES[ext] click_lane = await asyncio.to_thread( @@ -889,7 +1068,9 @@ async def get_mixdown( end=end, groups=_parse_groups(click_groups, click_accent), ) - cache_key = _mixdown_cache_key(job_id, ext, names, parsed_gains, start, end, click_lane) + cache_key = _mixdown_cache_key( + job_id, ext, names, parsed_gains, start, end, click_lane, parsed_pitches + ) cache_path = _MIXDOWN_CACHE_DIR / f"{cache_key}.{ext}" if cache_path.is_file(): return FileResponse( @@ -929,9 +1110,11 @@ async def get_mixdown( # click lane is never delayed -- its lead-in is baked in. A single audible # lane skips amix (a 1-input amix is a no-op). filters = [] - for i, g in enumerate(parsed_gains): + for i, lane in enumerate(render_lanes): delay = f"adelay={delay_ms}:all=1," if delay_ms > 0 else "" - filters.append(f"[{i}:a]{delay}volume={g:.6f}[a{i}]") + # Shift before the delay: rubberband would otherwise chew through the + # silent lead-in padding for nothing. + filters.append(f"[{i}:a]{_pitch_filter(lane.pitch)}{delay}volume={lane.gain:.6f}[a{i}]") if click_lane is not None: filters.append(f"[{stem_count}:a]volume={click_lane.gain:.6f}[a{stem_count}]") n = stem_count + (1 if click_lane is not None else 0) @@ -1000,6 +1183,11 @@ async def get_video_mixdown( job_id: str, stems: str = Query(..., description="Comma-separated lane names to sum"), gains: str = Query(..., description="Comma-separated linear gains, parallel to stems"), + pitches: str = Query( + default="", + max_length=128, + description="Comma-separated semitone shifts parallel to stems (-6..6). Empty = none.", + ), click: bool = Query(default=False, description="Mix the click track into the export"), click_mult: float = Query(default=1.0, description="Click rate: 0.5, 1 or 2"), click_accent: int = Query(default=-1, ge=-1, le=32, description="-1 auto, 0 off, N per bar"), @@ -1025,8 +1213,18 @@ async def get_video_mixdown( raise HTTPException(status_code=404, detail="no video track for this job") names, parsed_gains = _parse_lane_gains(stems, gains) - # Validates job_id (404), job done (404), and path traversal (404) per stem. - paths = [_validate_stem_path(job_id, name) for name in names] + parsed_pitches = _parse_pitches(pitches, names) + # Same refusal as the audio export: a video whose audio came back in the + # original key would be the same complaint reported again (#592). + if any(parsed_pitches) and not await asyncio.to_thread(_rubberband_available): + raise HTTPException( + status_code=422, + detail="this ffmpeg build cannot transpose (no rubberband filter)", + ) + # Validates job_id (404), job done (404), and path traversal (404) per stem, + # and rebuilds a transposed "original" lane from its parts. + render_lanes, _unpitched = _expand_render_lanes(job_id, names, parsed_gains, parsed_pitches) + paths = [lane.path for lane in render_lanes] # Click is one more audio input. It must be appended before the video input # so the audio indices the filter graph references stay contiguous from 0. @@ -1035,7 +1233,7 @@ async def get_video_mixdown( ) if click_lane is not None: paths = [*paths, click_lane[0]] - parsed_gains = [*parsed_gains, click_lane[1]] + render_lanes = [*render_lanes, _RenderLane(click_lane[0], click_lane[1], 0)] cmd: list[str] = [ffmpeg_executable(), "-nostdin", "-loglevel", "error"] for p in paths: @@ -1044,7 +1242,10 @@ async def get_video_mixdown( video_idx = len(paths) # Per-lane gain then amix (normalize=0 keeps levels faithful). A single audible # lane skips amix (a 1-input amix is a no-op), matching get_mixdown. - filters = [f"[{i}:a]volume={g:.6f}[a{i}]" for i, g in enumerate(parsed_gains)] + filters = [ + f"[{i}:a]{_pitch_filter(lane.pitch)}volume={lane.gain:.6f}[a{i}]" + for i, lane in enumerate(render_lanes) + ] n = len(paths) if n > 1: labels = "".join(f"[a{i}]" for i in range(n)) diff --git a/static/js/player.js b/static/js/player.js index 091581d6..a3591869 100644 --- a/static/js/player.js +++ b/static/js/player.js @@ -31,6 +31,7 @@ import { import { createAudioEngine, estimateDecodedBytes } from "./audioEngine.js"; import { createChunkedAudioEngine } from "./chunkedAudioEngine.js"; import { createPlaybackContext } from "./audioContext.js"; +import { effectivePitch } from "./pitchBus.js"; import { addVisualOnlyStems, buildPlaybackStems } from "./playbackStems.js"; import { vuLevel } from "./vuScale.js"; import { createMetronome } from "./metronome.js"; @@ -1804,6 +1805,7 @@ function _effectiveMixGains() { const anySolo = _currentStems.some((s) => mixerState[s.name]?.soloed); const names = []; const gains = []; + const pitches = []; for (const s of _currentStems) { if (s.name === "mix") continue; const m = mixerState[s.name]; @@ -1812,8 +1814,12 @@ function _effectiveMixGains() { if (g <= 0) continue; names.push(s.name); gains.push(Math.max(0, Math.min(LANE_VOLUME_MAX, g))); + // The lane's own key, the same number the stepper shows. The server + // re-applies effectivePitch, so drums are refused there too rather than + // this being the only thing standing between a snare and a resampler. + pitches.push(effectivePitch(s.name, m.pitch ?? 0, undefined)); } - return { names, gains }; + return { names, gains, pitches }; } // Click-track export params. The click is synthesised in the browser during @@ -1866,11 +1872,12 @@ export function setExportClickAvailable(on) { // when every lane is silenced; `region` appends the loop bounds. function _mixdownUrl(ext, region) { if (!currentJobId) return null; - const { names, gains } = _effectiveMixGains(); + const { names, gains, pitches } = _effectiveMixGains(); if (!names.length) return null; const q = new URLSearchParams({ stems: names.join(","), gains: gains.map((g) => g.toFixed(3)).join(","), + ...(pitches.some((p) => p !== 0) ? { pitches: pitches.join(",") } : {}), }); if (region) { q.set("start", loopStart.toFixed(3)); @@ -1916,11 +1923,12 @@ export function downloadCurrentMix(ext = "wav", onTransferStart) { // there's no video track or every lane is muted. export function downloadCurrentVideo(onTransferStart) { if (!currentJobId || !_currentHasVideo) return false; - const { names, gains } = _effectiveMixGains(); + const { names, gains, pitches } = _effectiveMixGains(); if (!names.length) return false; const q = new URLSearchParams({ stems: names.join(","), gains: gains.map((g) => g.toFixed(3)).join(","), + ...(pitches.some((p) => p !== 0) ? { pitches: pitches.join(",") } : {}), }); _clickParams(q); const safe = _currentTitle diff --git a/tests/test_transpose_export.py b/tests/test_transpose_export.py new file mode 100644 index 00000000..ea4182d8 --- /dev/null +++ b/tests/test_transpose_export.py @@ -0,0 +1,208 @@ +"""Transpose on export (#592). + +The studio shifts pitch in the browser with a SoundTouch AudioWorklet. The +server has no such stage, so an export came out in the original key however the +mixer was set. These pin the rules the export now follows, and in particular the +two it must never break: drums are not resampled, and an export never silently +returns the original key when it was asked for a different one. +""" + +from __future__ import annotations + +import pytest +from fastapi.testclient import TestClient + +from app.api.stems import ( + PITCH_MAX, + PITCH_MIN, + _expand_render_lanes, + _mixdown_cache_key, + _parse_pitches, + _pitch_filter, + effective_pitch, +) +from app.core.models import Job +from app.core.registry import _jobs + +JOB = "abcdefabcdef" + + +@pytest.fixture +def client(tmp_path, monkeypatch): + from app.api import stems as stems_mod + + monkeypatch.setattr(stems_mod, "JOBS_DIR", tmp_path) + monkeypatch.setattr(stems_mod, "_MIXDOWN_CACHE_DIR", tmp_path / "cache" / "mixdown") + monkeypatch.setattr(stems_mod, "_CLICK_CACHE_DIR", tmp_path / "cache" / "click") + from app.main import app + + return TestClient(app) + + +def _setup_job(tmp_path, names=("vocals", "drums", "bass", "guitar", "piano", "other")): + job = Job(id=JOB) + job.status = "done" + _jobs[job.id] = job + stems = tmp_path / JOB / "stems" + stems.mkdir(parents=True, exist_ok=True) + for n in names: + (stems / f"{n}.wav").write_bytes(b"RIFF") + return stems + + +# ─── The rule that matters most ────────────────────────────────────────── + + +def test_drums_are_never_transposed_whatever_is_asked(): + """Resampling a snare does not move it to another key, it makes it a + different drum. static/js/pitchBus.js refuses this on the playback side, so + honouring it here would export something the studio will not play.""" + for st in (-6, -1, 1, 6): + assert effective_pitch("drums", st) == 0 + assert _parse_pitches("4,4", ["vocals", "drums"]) == [4, 0] + + +def test_every_other_lane_is_shifted_as_asked(): + for name in ("vocals", "bass", "guitar", "piano", "other", "original"): + assert effective_pitch(name, 3) == 3 + assert effective_pitch(name, -3) == -3 + + +def test_a_shift_beyond_the_range_is_clamped_not_honoured(): + assert effective_pitch("vocals", 99) == PITCH_MAX + assert effective_pitch("vocals", -99) == PITCH_MIN + + +# ─── Parsing ───────────────────────────────────────────────────────────── + + +def test_no_pitches_means_no_transpose_so_old_clients_are_unchanged(): + """A caller that predates #592 omits the parameter entirely. It must keep + its exact behaviour rather than having to opt out of a new feature.""" + assert _parse_pitches("", ["vocals", "drums"]) == [0, 0] + + +def test_pitches_must_line_up_with_stems(): + with pytest.raises(Exception) as e: + _parse_pitches("1", ["vocals", "drums"]) + assert e.value.status_code == 422 + + +@pytest.mark.parametrize("bad", ["7,0", "-7,0", "x,0", "1.5,0"]) +def test_a_pitch_that_is_not_a_whole_semitone_in_range_is_refused(bad): + with pytest.raises(Exception) as e: + _parse_pitches(bad, ["vocals", "drums"]) + assert e.value.status_code == 422 + + +def test_the_filter_is_a_frequency_ratio_and_absent_at_zero(): + assert _pitch_filter(0) == "" + assert _pitch_filter(12).startswith("rubberband=pitch=2.0") + assert _pitch_filter(-12).startswith("rubberband=pitch=0.5") + + +# ─── The "original" lane ───────────────────────────────────────────────── + + +def test_the_original_lane_is_rebuilt_from_parts_so_its_drums_survive(tmp_path): + """ "original" is the complement lane and usually contains drums, so shifting + that one file would resample them. Playback rebuilds the lane from + components for exactly this reason (static/js/playbackStems.js); the export + has to do the same or a transposed mix comes back with pitched drums.""" + _setup_job(tmp_path) + from app.api import stems as stems_mod + + stems_mod.JOBS_DIR = tmp_path + lanes, unpitched = _expand_render_lanes(JOB, ["vocals", "original"], [1.0, 1.0], [2, 2]) + + assert unpitched == [] + # vocals, then the five components that make up the complement. + assert len(lanes) == 6 + by_name = {lane.path.stem: lane.pitch for lane in lanes} + assert by_name["vocals"] == 2 + assert by_name["drums"] == 0, "the complement's drums must not be resampled" + for n in ("bass", "guitar", "piano", "other"): + assert by_name[n] == 2 + + +def test_an_unshifted_original_lane_is_left_as_one_file(tmp_path): + """Nothing to take apart when nothing is being shifted -- the cheap path + stays cheap, and an export with no transpose renders exactly as it always + did.""" + _setup_job(tmp_path) + from app.api import stems as stems_mod + + stems_mod.JOBS_DIR = tmp_path + (tmp_path / JOB / "stems" / "original.wav").write_bytes(b"RIFF") + lanes, unpitched = _expand_render_lanes(JOB, ["vocals", "original"], [1.0, 1.0], [0, 0]) + assert len(lanes) == 2 + assert unpitched == [] + + +def test_an_original_lane_that_cannot_be_taken_apart_keeps_its_key(tmp_path): + """An older job kept only the rendered mix. Playback makes the same + conservative choice: melodic content stays in the old key rather than the + drums inside it being resampled.""" + _setup_job(tmp_path, names=("vocals", "original")) + from app.api import stems as stems_mod + + stems_mod.JOBS_DIR = tmp_path + lanes, unpitched = _expand_render_lanes(JOB, ["vocals", "original"], [1.0, 1.0], [2, 2]) + assert unpitched == ["original"] + assert len(lanes) == 2 + assert {lane.path.stem: lane.pitch for lane in lanes}["original"] == 0 + + +# ─── Cache ─────────────────────────────────────────────────────────────── + + +def test_a_transposed_export_never_serves_the_untransposed_render(): + args = (JOB, "wav", ["vocals"], [1.0], None, None, None) + assert _mixdown_cache_key(*args) != _mixdown_cache_key(*args, [2]) + assert _mixdown_cache_key(*args, [2]) != _mixdown_cache_key(*args, [-2]) + + +def test_an_untransposed_export_keeps_the_cache_entry_it_already_had(): + """Every export rendered before #592 must still hit its existing entry.""" + args = (JOB, "wav", ["vocals"], [1.0], None, None, None) + assert _mixdown_cache_key(*args) == _mixdown_cache_key(*args, None) + assert _mixdown_cache_key(*args) == _mixdown_cache_key(*args, [0]) + + +# ─── Endpoint ──────────────────────────────────────────────────────────── + + +def test_asking_for_a_transpose_an_ffmpeg_cannot_do_says_so(client, tmp_path, monkeypatch): + """librubberband is GPL and plenty of builds omit it. Returning the original + key would look exactly like the bug this feature exists to fix, so the + export refuses and says why instead.""" + from app.api import stems as stems_mod + + _setup_job(tmp_path) + monkeypatch.setattr(stems_mod, "_rubberband_available", lambda: False) + r = client.get(f"/api/jobs/{JOB}/mixdown.wav?stems=vocals&gains=1.0&pitches=2") + assert r.status_code == 422 + assert "transpose" in r.json()["detail"] + + +def test_an_export_with_no_transpose_never_probes_for_rubberband(client, tmp_path, monkeypatch): + """A build with no pitch support must keep exporting normally.""" + from app.api import stems as stems_mod + + _setup_job(tmp_path) + called = [] + monkeypatch.setattr(stems_mod, "_rubberband_available", lambda: called.append(1) or False) + r = client.get(f"/api/jobs/{JOB}/mixdown.wav?stems=vocals&gains=1.0") + assert r.status_code != 422 + assert called == [], "the probe ran for an export that was never transposed" + + +def test_a_drums_only_transpose_is_not_treated_as_a_transpose(client, tmp_path, monkeypatch): + """Asking to shift drums resolves to no shift at all, so the export must not + then refuse itself on a build that cannot pitch-shift.""" + from app.api import stems as stems_mod + + _setup_job(tmp_path) + monkeypatch.setattr(stems_mod, "_rubberband_available", lambda: False) + r = client.get(f"/api/jobs/{JOB}/mixdown.wav?stems=drums&gains=1.0&pitches=3") + assert r.status_code != 422 From 1e76e8b24c33051dce26d0f7bac7e1b6d875da1e Mon Sep 17 00:00:00 2001 From: Thales Pereira <31625914+thcp@users.noreply.github.com> Date: Tue, 8 Sep 2026 17:31:34 +0100 Subject: [PATCH 2/3] fix(export): two bugs in the transposed "original" lane, found on real audio Both surfaced by exporting from an actual library rather than a fixture, and neither was visible to the tests as they stood. Validity depended on the transpose. `_expand_render_lanes` rebuilt the "original" lane from component stems without first checking the lane exists at all, so ?stems=vocals,original 404'd untransposed and rendered when transposed. Whether a lane exists is a property of the job, not of the request. The lane is now validated before anything else, and the reconstruction is a decision made after that. The fallback pointed at the wrong file. The component loop bound its path to `path`, the same name already holding the validated original.wav, so a job whose components are missing fell back to whichever component was tried last -- drums.wav -- instead of the lane the caller asked for. Renamed, with a comment saying why the name is not reused. Verified against a real 224 s track rather than only in tests, measuring the shift by per-frame median log-spectral correlation and validating that method against a known-good direct rubberband render first: bass pitches=+2 -> +2.00 semitones bass pitches=-3 -> -2.96 semitones drums pitches=+2 -> byte-identical to no transpose MP3 and WAV both export; duration is unchanged in every case. Refs #592 --- app/api/stems.py | 19 ++++++++++++++----- tests/test_transpose_export.py | 21 +++++++++++++++++++++ 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/app/api/stems.py b/app/api/stems.py index 6f4cab1c..ec364c6e 100644 --- a/app/api/stems.py +++ b/app/api/stems.py @@ -554,21 +554,30 @@ def _expand_render_lanes( selected.add("vocals") for name, gain, pitch in zip(names, gains, pitches, strict=True): + # Validate first, always. Whether a lane exists is a property of the + # job, not of the request: without this an "original" lane the job + # never produced would 404 untransposed and be synthesised from + # components when transposed, so the same URL was valid or not + # depending on the pitch attached to it. + path = _validate_stem_path(job_id, name) if name != "original" or pitch == 0: - lanes.append(_RenderLane(_validate_stem_path(job_id, name), gain, pitch)) + lanes.append(_RenderLane(path, gain, pitch)) continue components = [n for n in STEM_NAMES if n not in selected] + # Deliberately not `path`: that name holds the validated "original.wav" + # this lane falls back to, and reusing it here left the fallback + # pointing at whichever component was tried last. paths: list[Path] = [] for comp in components: - path = (JOBS_DIR / job_id / "stems" / f"{comp}.wav").resolve() - if not path.is_file() or not path.is_relative_to(JOBS_DIR.resolve()): + comp_path = (JOBS_DIR / job_id / "stems" / f"{comp}.wav").resolve() + if not comp_path.is_file() or not comp_path.is_relative_to(JOBS_DIR.resolve()): paths = [] break - paths.append(path) + paths.append(comp_path) if not paths: - lanes.append(_RenderLane(_validate_stem_path(job_id, name), gain, 0)) + lanes.append(_RenderLane(path, gain, 0)) unpitched.append(name) continue diff --git a/tests/test_transpose_export.py b/tests/test_transpose_export.py index ea4182d8..45ff2977 100644 --- a/tests/test_transpose_export.py +++ b/tests/test_transpose_export.py @@ -110,6 +110,9 @@ def test_the_original_lane_is_rebuilt_from_parts_so_its_drums_survive(tmp_path): components for exactly this reason (static/js/playbackStems.js); the export has to do the same or a transposed mix comes back with pitched drums.""" _setup_job(tmp_path) + # A job that shows an "original" lane has the rendered file too; the + # reconstruction is about not *shifting* that file, not about its absence. + (tmp_path / JOB / "stems" / "original.wav").write_bytes(b"RIFF") from app.api import stems as stems_mod stems_mod.JOBS_DIR = tmp_path @@ -139,6 +142,24 @@ def test_an_unshifted_original_lane_is_left_as_one_file(tmp_path): assert unpitched == [] +def test_a_lane_the_job_never_produced_is_refused_whether_or_not_it_is_shifted(tmp_path): + """Whether a lane exists is a property of the job, not of the request. + + Found by exporting against a real library: "original" only exists when the + user picked a strict subset, and the reconstruction path was happy to + synthesise one from components. So the same URL 404'd untransposed and + rendered when transposed -- validity depending on the pitch attached to it. + """ + _setup_job(tmp_path) # no original.wav + from app.api import stems as stems_mod + + stems_mod.JOBS_DIR = tmp_path + for pitch in (0, 2): + with pytest.raises(Exception) as e: + _expand_render_lanes(JOB, ["vocals", "original"], [1.0, 1.0], [0, pitch]) + assert e.value.status_code == 404, f"pitch={pitch} disagreed with the other" + + def test_an_original_lane_that_cannot_be_taken_apart_keeps_its_key(tmp_path): """An older job kept only the rendered mix. Playback makes the same conservative choice: melodic content stays in the old key rather than the From b816b83ecc4e142cd2aabafc56b241b03fd5113e Mon Sep 17 00:00:00 2001 From: Thales Pereira <31625914+thcp@users.noreply.github.com> Date: Tue, 8 Sep 2026 17:55:05 +0100 Subject: [PATCH 3/3] fix(export): say why the rubberband probe failed instead of swallowing it A probe that raised was silently treated as "no pitch support". That is the right answer, but it collapses two different situations into one message: a build genuinely without the filter and an ffmpeg that would not run at all both reach the user as "this ffmpeg build cannot transpose", with nothing to tell them apart. Logs which it was at debug level. Raised by the code-quality review on #599. Refs #592 --- app/api/stems.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/app/api/stems.py b/app/api/stems.py index ec364c6e..0f800c9b 100644 --- a/app/api/stems.py +++ b/app/api/stems.py @@ -438,7 +438,14 @@ def _rubberband_available() -> bool: ) _rubberband_cached = bool(re.search(r"\brubberband\b", probe.stdout)) except (OSError, subprocess.SubprocessError): - pass + # Treated as "no pitch support", which is the safe answer, but the + # two cases are not the same: a build genuinely without the filter + # and an ffmpeg that would not run at all both surface to the user + # as "this build cannot transpose". Log which one it was, or that + # message is the only thing anybody has to go on. + logger.debug( + "rubberband probe failed; treating transpose as unavailable", exc_info=True + ) return _rubberband_cached