feat(editor): audition speed (pitch-preserving slow) + latency-synced playhead - #247
Conversation
… playhead Two coupled changes to the transport's visual clock. AUDITION SPEED (design slice 5 / P2-9). A transport speed control (100/75/50%) slows the reference for practice without shifting pitch — playback only, never touches source time, the tempo map, exported audio or dirty state, resets to 100% on song load. The transport clock gains a `rate` factor (default 1, so rate-1 is bit-identical): _transportChartTimePure scales chart time by rate and the guide/metronome scheduler uses the exact inverse (_guideChartToCtxPure), so claps/clicks stay aligned with the slowed audio. Engine: the sample-accurate AudioBufferSource path is UNCHANGED at rate 1; only rate<1 reroutes the reference onto a MediaElement (preservesPitch) into the same _refGain, slaved to the ctx clock with a 30ms resync. Placement/snap always resolve from the source clock, never the element. LATENCY-SYNCED PLAYHEAD. The marker was drawn from scheduled ctx time, which leads the HEARD sound by the output latency — negligible wired, but 100-300ms on Bluetooth, where the line visibly ran ahead. The playhead now paints at ctxNow − outputLatency (sampled-and-held per pass, baseLatency fallback, NaN-guarded), scaled into chart time by rate. Display-only: S.cursorTime — and therefore snap/edit/seek/follow-scroll — is untouched, so a stopped/scrubbing playhead still sits exactly on the waveform for placement. Architecture note: a three-seat review (audio-engine, transcription, UI) confirmed Web Audio — not the desktop's JUCE graph — is the right foundation (JUCE isn't present in the browser or Docker; only Web Audio runs in every target), and that the latency fix belongs in the visual clock only. Tests: tests/audition_clock.test.mjs (5) — the rate factor (fails on main), rate-1 identity, the scheduler↔clock inverse round-trip at 0.5, and the latency·rate marker offset. compose_transport's sliced env updated for the new latency hold. 135 JS green, lint 0-err, routes.py untouched. Live-verified: the playhead advances at 0.994x wall at 1.0x and 0.496x at 0.5x, the draw time is latency-compensated, and audition resets on song change. (Subjective 50% pitch-preservation quality is an ears check, per the review.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe editor adds pitch-preserving audition speeds of 100%, 75%, and 50%. Transport, cursor, and guide timing now account for audition rate and output latency, while audio and audition state reset across playback stops, teardown, and song changes. ChangesAudition speed and timing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/audio.js`:
- Around line 392-412: Track the timeout created by _startRefMediaAt in shared
state, and have _stopRefMedia clear and reset that timeout before pausing the
element. Ensure each new delayed start replaces any prior pending timer, so
stopPlayback, teardownAudio, and _resetAuditionForNewSong reliably prevent a
deferred play() from restarting audio.
In `@src/file-ops.js`:
- Around line 121-122: Update the reset logic in loadCDLC to refresh
`#editor-audition-speed` whenever S.auditionRate is reset to 1, preferably by
routing through _resetAuditionForNewSong() or reusing its existing UI-refresh
path so the control displays 100% for the newly loaded song.
In `@src/transport-bar.js`:
- Around line 337-339: Update the audition-speed select created in the transport
UI and its integration with editorSetAuditionRate so every accepted finite rate
in (0, 1] remains representable. Add or update a current-rate fallback option
when callers set values such as 0.8, or otherwise clamp the UI consistently to
supported presets, ensuring speed.value never leaves the select without a
matching option.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ec8be19-f953-4d90-866e-c71aca04b2f9
📒 Files selected for processing (9)
CHANGELOG.mdsrc/audio.jssrc/draw.jssrc/file-ops.jssrc/main.jssrc/transport-bar.jssrc/transport.jstests/audition_clock.test.mjstests/compose_transport.test.mjs
| + `<select id="editor-audition-speed" class="editor-transport-snap" aria-label="Audition speed"` | ||
| + ` title="Audition speed — slow the recording for practice, pitch preserved. Playback only; the chart, map and export are unchanged.">` | ||
| + `<option value="1">100%</option><option value="0.75">75%</option><option value="0.5">50%</option></select>` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure first
ast-grep outline src/transport-bar.js --view expanded
# Inspect the relevant line ranges
sed -n '320,360p' src/transport-bar.js
printf '\n----\n'
sed -n '480,520p' src/transport-bar.js
# Locate the rate API and any related callers/usages
rg -n "editor(Set|AuditionRate|AuditionSpeed|audition rate|audition-speed|editor-audition-speed)" src/transport-bar.js srcRepository: got-feedBack/feedBack-plugin-editor
Length of output: 16452
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the audition-rate setter and getter
sed -n '410,455p' src/audio.js
# Check whether any other code sets audition rate values or defines presets
rg -n "auditionRate|audition rate|SetAuditionRate|setAuditionRate|0\.75|0\.5|1\)" src/audio.js src/main.js src/transport-bar.jsRepository: got-feedBack/feedBack-plugin-editor
Length of output: 6891
Keep the audition-speed select aligned with the setter. editorSetAuditionRate accepts any finite rate in (0, 1], but this control only offers 1, 0.75, and 0.5. If another caller sets 0.8, speed.value = "0.8" leaves the select with no matching option and it renders blank. Either add a fallback/current-rate option or clamp the UI to the supported presets.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/transport-bar.js` around lines 337 - 339, Update the audition-speed
select created in the transport UI and its integration with
editorSetAuditionRate so every accepted finite rate in (0, 1] remains
representable. Add or update a current-rate fallback option when callers set
values such as 0.8, or otherwise clamp the UI consistently to supported presets,
ensuring speed.value never leaves the select without a matching option.
…ck, cancellable start Adversarial review of the audition-speed slice. Five real defects: - BLOCKER: _startAudioSourceAtCursor discarded _startRefMediaAt's return AFTER stopping the BufferSource. If the MediaElement route is unavailable (no Audio ctor, or the context refuses a MediaElementSource on CORS-tainted media) the result was dead silence while the transport clock still crawled at 0.5x. Now it demotes to 100%, falls through to the BufferSource, and says why. - HIGH: the compensated paint clock was clamped at 0, not at playStartTime, so a count-in pre-roll dragged the drawn playhead backwards by preRoll*rate and swept it in over the count clicks. Extracted _cursorDrawTimePure (transport.js), which clamps exactly like the logical cursor and sanitizes the latency. - HIGH: _stopRefMedia only paused; the deferred play() from a count-in / +ve audio shift survived stop / teardown / song change and resumed audio later (CodeRabbit). The timer is now tracked, cancelled, and superseded by a re-start. - MED: _ensureRefMedia compared S.audioUrl against el.src — the GETTER, which echoes a RESOLVED absolute url, so a relative audio_url mismatched on every call and re-assigned src (reloading + stalling the element) on every seek and loop wrap. Memoise the ASSIGNED src instead. - MED: the pitched GM guide voice's sustain is in chart seconds but gmVoiceAt schedules in wall seconds — at 0.5x it rang half as long as its note. Divide by the rate, same as the chart->ctx mapping. Also: loadCDLC's audition reset now routes through _resetAuditionForNewSong() so the select follows S (CodeRabbit); the anchor re-seats cursorDrawTime so a loop wrap can't paint one frame at the old position; audioUrl/auditionRate/cursorDrawTime are declared in state.js. tests/audition_clock.test.mjs: +5 regression tests, all failing pre-fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/audio.js (1)
408-423: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle rejected
play()on the slow path
_startRefMediaAt()returnstrueas soon as_ensureRefMedia()succeeds, and theplay()promise is swallowed. Ifplay()rejects after the delayed start, the BufferSource is already stopped, so audition can go silent with no fallback to 100%.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audio.js` around lines 408 - 423, Update _startRefMediaAt so rejected play() calls on both immediate and delayed starts trigger the existing 100% audition fallback instead of being silently swallowed. Attach rejection handling inside the go callback and preserve the delayed timer behavior, ensuring fallback occurs only after play() actually rejects.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/audio.js`:
- Around line 408-423: Update _startRefMediaAt so rejected play() calls on both
immediate and delayed starts trigger the existing 100% audition fallback instead
of being silently swallowed. Attach rejection handling inside the go callback
and preserve the delayed timer behavior, ensuring fallback occurs only after
play() actually rejects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: de27f6e0-0ab0-4bd1-85de-f58e6d773fd5
📒 Files selected for processing (5)
src/audio.jssrc/file-ops.jssrc/state.jssrc/transport.jstests/audition_clock.test.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/transport.js
- src/file-ops.js
…speed # Conflicts: # CHANGELOG.md
Two coupled changes to the transport's visual clock — a slow-practice feature and the A/V-sync fix it surfaced.
1. Audition speed (design slice 5 / P2-9)
A transport speed control (100 / 75 / 50%) slows the reference for practice without shifting pitch. Playback only — never touches source time, the tempo map, exported audio, or dirty state; one click back to 100%; resets to full speed on song load.
_transportChartTimePuregains aratefactor (default 1 → rate-1 is bit-identical); the guide/metronome scheduler uses the exact inverse_guideChartToCtxPure, so claps/clicks stay locked to the slowed audio.AudioBufferSourcepath is unchanged at rate 1; onlyrate < 1reroutes the reference onto aMediaElement(preservesPitch) into the same_refGain(fader / A-B / fade all keep working), slaved to the ctx clock with a 30 ms resync. Placement/snap always resolve from the source clock, never the element.2. Latency-synced playhead (the "triple sure" fix)
The playhead was drawn from scheduled ctx time, which leads the heard sound by the output latency — negligible wired, but 100–300 ms on Bluetooth, where the line visibly ran ahead of the music. It now paints at
ctxNow − outputLatency(sampled-and-held per pass,baseLatencyfallback, NaN-guarded), scaled into chart time byrate. Display-only:S.cursorTime— and thus snap/edit/seek/follow-scroll — is untouched, so a stopped/scrubbing playhead still sits exactly on the waveform for placement. All heard audio (reference, claps, metronome) shares the latency, so one offset re-aligns the line to everything.Architecture review
A three-seat review (audio-engine, transcription, UI) independently confirmed Web Audio — not the desktop's JUCE graph — is the right foundation: JUCE isn't present in the browser or Docker, only Web Audio runs in every target (web testbed + Docker/server + desktop), and it already gives sample-accurate scheduling. All three agreed the latency fix belongs in the visual clock only, and that the real future lever is time-stretch quality at 50% (a someday WASM/native swap), not sync — so the ≥50% floor stays.
Tests
tests/audition_clock.test.mjs(5) — the rate factor (fails on main: 4th arg ignored), rate-1 identity, the scheduler↔clock inverse round-trip at 0.5×, and thelatency·ratemarker offset.compose_transport's sliced env updated for the new latency hold.routes.pyuntouched.cursorDrawTimeis latency-compensated, and audition resets 0.5→1 on song change.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes