highway: phase-lock the render clock to stop backward frames#974
highway: phase-lock the render clock to stop backward frames#974Kilgannon2113 wants to merge 1 commit into
Conversation
Fixes got-feedBack#957. Signed-off-by: Kilgannon2113 <kievkillgo@gmail.com>
📝 WalkthroughWalkthroughHighway now uses a monotone phase-locked render clock, supports an optional transport-clock source for JUCE mode, updates timing at draw time, resets PLL state on stop, and adds simulations covering frame rates, stalls, and lifecycle behavior. ChangesHighway clock stabilization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AppLoop
participant Highway
participant AudioSource
participant Renderer
AppLoop->>Highway: setClockSource(audio time or NaN)
Highway->>AudioSource: sample transport time
Highway->>Highway: update PLL and chartTime
Highway->>Renderer: draw with frame-time clock
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Notes for review:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@static/highway.js`:
- Around line 1216-1219: Update the PLL correction logic using _PLL_GAIN so
phase adjustment is scaled by elapsed time rather than applied as a fixed
per-frame amount, preserving frame-rate-independent convergence. Clamp normal
phase correction so the corrected playback position never moves backward within
a frame, while retaining _PLL_MAX_ERR_SEC as a separate hard-resync path for
intentional seeks. Apply the same behavior to the related correction block
around the observed-rate update.
In `@tests/js/highway_render_clock.test.js`:
- Around line 106-112: Update the simulation loop around nextTick in the clock
test to schedule transport polls on a fixed 60 Hz timeline, independent of the
render frame interval frameMs. Advance nextTick by tickMs from its prior
scheduled value rather than deriving it from the current render timestamp, while
preserving the existing setTime(audioAt(t)) behavior.
🪄 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: fbfdc8e2-df6f-4ed7-aa5a-4a1390afa331
📒 Files selected for processing (4)
CHANGELOG.mdstatic/app.jsstatic/highway.jstests/js/highway_render_clock.test.js
| const _PLL_GAIN = 0.10; // phase-correction gain per frame | ||
| const _PLL_RATE_ALPHA = 0.05; // EMA weight for the noisy observed rate | ||
| const _PLL_MAX_ERR_SEC = 0.25; // beyond this, it's a seek → hard resync | ||
| const _PLL_RESYNC_MS = 120; // loop went stale (paused/hidden) → resync |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the phase correction monotonic and time-based.
At 200 FPS, dt=5ms with err=-0.2s advances 5ms then subtracts 20ms, producing a 15ms backward frame while remaining below the 250ms resync threshold. The fixed per-frame gain also settles much faster at high refresh rates, contradicting frame-rate independence.
Proposed correction
- const _PLL_GAIN = 0.10; // phase-correction gain per frame
+ const _PLL_GAIN_60FPS = 0.10; // equivalent correction at 60 fps
...
const dt = (nowP - hwState._pllAt) / 1000;
hwState._pllAt = nowP;
+ const previous = hwState._pllTime;
hwState._pllTime += hwState._pllRate * dt;
const err = target - hwState._pllTime;
if (Math.abs(err) > _PLL_MAX_ERR_SEC) {
hwState._pllTime = target;
} else {
- hwState._pllTime += err * _PLL_GAIN;
+ const gain = 1 - Math.pow(1 - _PLL_GAIN_60FPS, dt * 60);
+ hwState._pllTime = Math.max(
+ previous,
+ hwState._pllTime + err * gain
+ );
}Seek detection should remain a separate hard-resync path so intentional backward seeks are not clamped.
Also applies to: 1263-1271
🤖 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 `@static/highway.js` around lines 1216 - 1219, Update the PLL correction logic
using _PLL_GAIN so phase adjustment is scaled by elapsed time rather than
applied as a fixed per-frame amount, preserving frame-rate-independent
convergence. Clamp normal phase correction so the corrected playback position
never moves backward within a frame, while retaining _PLL_MAX_ERR_SEC as a
separate hard-resync path for intentional seeks. Apply the same behavior to the
related correction block around the observed-rate update.
| let nextTick = 0; | ||
| let prev = null; | ||
| const deltas = []; | ||
| for (let t = 0; t < seconds * 1000; t += frameMs) { | ||
| now = t; | ||
| if (t >= nextTick) { sb.setTime(audioAt(t)); nextTick = t + tickMs; } | ||
| const c = sb.renderClock(t); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the simulated transport poll fixed at 60 Hz.
nextTick = t + tickMs quantizes polling to render frames. For example, 30 FPS receives only 30 Hz anchors and 90 FPS receives roughly 45 Hz, so the cross-refresh tests do not model the stated 60 Hz producer.
Proposed fix
for (let t = 0; t < seconds * 1000; t += frameMs) {
- now = t;
- if (t >= nextTick) { sb.setTime(audioAt(t)); nextTick = t + tickMs; }
+ while (nextTick <= t) {
+ now = nextTick;
+ sb.setTime(audioAt(nextTick));
+ nextTick += tickMs;
+ }
+ now = t;
const c = sb.renderClock(t);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let nextTick = 0; | |
| let prev = null; | |
| const deltas = []; | |
| for (let t = 0; t < seconds * 1000; t += frameMs) { | |
| now = t; | |
| if (t >= nextTick) { sb.setTime(audioAt(t)); nextTick = t + tickMs; } | |
| const c = sb.renderClock(t); | |
| let nextTick = 0; | |
| let prev = null; | |
| const deltas = []; | |
| for (let t = 0; t < seconds * 1000; t += frameMs) { | |
| while (nextTick <= t) { | |
| now = nextTick; | |
| sb.setTime(audioAt(nextTick)); | |
| nextTick += tickMs; | |
| } | |
| now = t; | |
| const c = sb.renderClock(t); |
🤖 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 `@tests/js/highway_render_clock.test.js` around lines 106 - 112, Update the
simulation loop around nextTick in the clock test to schedule transport polls on
a fixed 60 Hz timeline, independent of the render frame interval frameMs.
Advance nextTick by tickMs from its prior scheduled value rather than deriving
it from the current render timestamp, while preserving the existing
setTime(audioAt(t)) behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/js/highway_render_clock.test.js (1)
51-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPLL tuning constants are hardcoded in the sandbox, not sourced from
highway.js.
_PLL_GAIN,_PLL_RATE_ALPHA,_PLL_MAX_ERR_SEC,_PLL_RESYNC_MSare duplicated here as literals that must be kept in sync by hand with theconstdeclarations instatic/highway.js(Lines 1216-1219). Only the function bodies of_anchoredChartTime/_renderClockare extracted from source viaextractBlock; the tunables they close over are test-authored copies. If someone retunes the PLL inhighway.jswithout updating this file, the test silently exercises stale constants — it would keep passing/failing based on old tuning rather than the real implementation's behavior.Consider extracting the
const _PLL_* = ...;declarations fromhighwaySources()(e.g. a small regex or a brace-free line-scan) instead of re-declaring the values here, so the test stays coupled to the actual tuning.🤖 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 `@tests/js/highway_render_clock.test.js` around lines 51 - 75, Update buildRenderClockSandbox and highwaySources so the PLL tuning constants _PLL_GAIN, _PLL_RATE_ALPHA, _PLL_MAX_ERR_SEC, and _PLL_RESYNC_MS are extracted from static/highway.js and injected into the sandbox instead of duplicated literals. Preserve the existing extracted function bodies and ensure the test uses the source declarations’ current values.static/highway.js (3)
1221-1229: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated by
getTime()'s fallback branch.This exact interpolation (anchor + observed-rate × elapsed, with the two staleness checks) is re-implemented almost verbatim in
getTime()at Lines 2538-2556, just inliningperformance.now()fornowP. SincegetTime()already runs in the same closure as_anchoredChartTime, it can call_anchoredChartTime(performance.now())directly instead of re-deriving the same math — reduces the risk of the two copies drifting apart (which is exactly the kind of subtle divergence this PR is trying to eliminate).♻️ Proposed dedup
getTime() { if (typeof hwState._clockSource === 'function') { let _t; try { _t = hwState._clockSource(); } catch (_) { _t = NaN; } if (Number.isFinite(_t)) return _t + hwState.songOffset; } if (Number.isFinite(hwState._pllTime) && (performance.now() - hwState._pllAt) <= 120) { return hwState._pllTime; } - if (Number.isNaN(hwState._chartAnchorPerfNow)) return hwState.chartTime; - const nowP = performance.now(); - if (nowP - hwState._chartLastAdvanceAt > _CHART_MAX_INTERP_MS) return hwState.chartTime; - const elapsedMs = nowP - hwState._chartAnchorPerfNow; - if (elapsedMs > _CHART_MAX_INTERP_MS) return hwState.chartTime; - return hwState._chartAnchorAudioT + (hwState._chartObservedRate * elapsedMs) / 1000 + hwState.songOffset; + return _anchoredChartTime(performance.now()); },🤖 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 `@static/highway.js` around lines 1221 - 1229, Update getTime()’s fallback branch to call _anchoredChartTime(performance.now()) instead of duplicating the anchor interpolation and staleness checks. Preserve the existing fallback behavior while making _anchoredChartTime the single implementation of this calculation.
1198-1211: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
_clockNow()permanently disables the clock source on any transient error.If
src()throws once (e.g. a momentary null-property access during JUCE IPC setup),hwState._clockSourceis nulled forever — there is no way to recover except callingsetClockSource()again. A single transient hiccup silently and permanently degrades JUCE mode back to the browser PLL clock for the rest of the session. Consider only skipping the current frame (return NaN) without uninstalling the source, or retrying after a cooldown.🤖 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 `@static/highway.js` around lines 1198 - 1211, Update _clockNow() so a thrown error from the clock source only skips the current reading by returning NaN; do not clear hwState._clockSource in the catch block, allowing later frames to retry the same source without requiring setClockSource() again.
2519-2523: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClock-source error handling here diverges from
_clockNow()'s.This inline
try/catchduplicates_clockNow()'s job (safely callinghwState._clockSource()) but, unlike_clockNow(), does not null outhwState._clockSourceon a throw — so a throwing clock source is "disabled" from the renderer's perspective (via_clockNow()indraw()) but stays "installed" fromgetTime()'s perspective (plugins would keep hitting the same exception, silently swallowed, every call). Reusing_clockNow()here would unify the behavior and remove the second copy of this logic.♻️ Proposed fix
if (typeof hwState._clockSource === 'function') { - let _t; - try { _t = hwState._clockSource(); } catch (_) { _t = NaN; } - if (Number.isFinite(_t)) return _t + hwState.songOffset; + const _t = _clockNow(); + if (Number.isFinite(_t)) return _t + hwState.songOffset; }🤖 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 `@static/highway.js` around lines 2519 - 2523, Replace the inline _clockSource try/catch in getTime with a call to _clockNow(), reusing its existing error handling and disabling behavior. Preserve the songOffset adjustment and finite-time fallback behavior while removing the duplicated clock-source invocation logic.
🤖 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 `@static/highway.js`:
- Around line 2513-2533: Replace the hardcoded 120 ms freshness threshold in the
getTime PLL-time check with the existing _PLL_RESYNC_MS constant, preserving the
current finite-value and performance.now()-based staleness logic.
---
Nitpick comments:
In `@static/highway.js`:
- Around line 1221-1229: Update getTime()’s fallback branch to call
_anchoredChartTime(performance.now()) instead of duplicating the anchor
interpolation and staleness checks. Preserve the existing fallback behavior
while making _anchoredChartTime the single implementation of this calculation.
- Around line 1198-1211: Update _clockNow() so a thrown error from the clock
source only skips the current reading by returning NaN; do not clear
hwState._clockSource in the catch block, allowing later frames to retry the same
source without requiring setClockSource() again.
- Around line 2519-2523: Replace the inline _clockSource try/catch in getTime
with a call to _clockNow(), reusing its existing error handling and disabling
behavior. Preserve the songOffset adjustment and finite-time fallback behavior
while removing the duplicated clock-source invocation logic.
In `@tests/js/highway_render_clock.test.js`:
- Around line 51-75: Update buildRenderClockSandbox and highwaySources so the
PLL tuning constants _PLL_GAIN, _PLL_RATE_ALPHA, _PLL_MAX_ERR_SEC, and
_PLL_RESYNC_MS are extracted from static/highway.js and injected into the
sandbox instead of duplicated literals. Preserve the existing extracted function
bodies and ensure the test uses the source declarations’ current values.
🪄 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: fbfdc8e2-df6f-4ed7-aa5a-4a1390afa331
📒 Files selected for processing (4)
CHANGELOG.mdstatic/app.jsstatic/highway.jstests/js/highway_render_clock.test.js
| // Prefer the frame-time clock source when one is installed, so | ||
| // plugins and the renderer read the SAME clock. Guarded on hwState | ||
| // only, so the source-extraction sandbox in | ||
| // tests/js/highway_monotonic_clock.test.js (which supplies a bare | ||
| // hwState with no _clockSource) falls straight through to the | ||
| // interpolator below and keeps testing it. | ||
| if (typeof hwState._clockSource === 'function') { | ||
| let _t; | ||
| try { _t = hwState._clockSource(); } catch (_) { _t = NaN; } | ||
| if (Number.isFinite(_t)) return _t + hwState.songOffset; | ||
| } | ||
| // Phase-locked render clock, when the draw loop is live (browser | ||
| // mode). Keeps plugins and the renderer on ONE clock. Guarded on | ||
| // hwState alone so the source-extraction sandbox in | ||
| // tests/js/highway_monotonic_clock.test.js — which supplies a bare | ||
| // hwState with no _pllTime — falls through to the interpolator below | ||
| // and keeps testing it. | ||
| if (Number.isFinite(hwState._pllTime) | ||
| && (performance.now() - hwState._pllAt) <= 120) { | ||
| return hwState._pllTime; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Staleness threshold 120 should reference _PLL_RESYNC_MS instead of a bare literal.
_PLL_RESYNC_MS (declared at Line 1219, in scope here) already encodes this exact 120 ms staleness budget for the render clock's own resync decision. Hardcoding 120 again here means getTime()'s "is the PLL time still fresh enough to serve plugins" check can silently diverge from _renderClock()'s own resync threshold if _PLL_RESYNC_MS is ever tuned — precisely the kind of two-clocks-disagree bug this PR sets out to eliminate.
🛠️ Proposed fix
if (Number.isFinite(hwState._pllTime)
- && (performance.now() - hwState._pllAt) <= 120) {
+ && (performance.now() - hwState._pllAt) <= _PLL_RESYNC_MS) {
return hwState._pllTime;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Prefer the frame-time clock source when one is installed, so | |
| // plugins and the renderer read the SAME clock. Guarded on hwState | |
| // only, so the source-extraction sandbox in | |
| // tests/js/highway_monotonic_clock.test.js (which supplies a bare | |
| // hwState with no _clockSource) falls straight through to the | |
| // interpolator below and keeps testing it. | |
| if (typeof hwState._clockSource === 'function') { | |
| let _t; | |
| try { _t = hwState._clockSource(); } catch (_) { _t = NaN; } | |
| if (Number.isFinite(_t)) return _t + hwState.songOffset; | |
| } | |
| // Phase-locked render clock, when the draw loop is live (browser | |
| // mode). Keeps plugins and the renderer on ONE clock. Guarded on | |
| // hwState alone so the source-extraction sandbox in | |
| // tests/js/highway_monotonic_clock.test.js — which supplies a bare | |
| // hwState with no _pllTime — falls through to the interpolator below | |
| // and keeps testing it. | |
| if (Number.isFinite(hwState._pllTime) | |
| && (performance.now() - hwState._pllAt) <= 120) { | |
| return hwState._pllTime; | |
| } | |
| // Prefer the frame-time clock source when one is installed, so | |
| // plugins and the renderer read the SAME clock. Guarded on hwState | |
| // only, so the source-extraction sandbox in | |
| // tests/js/highway_monotonic_clock.test.js (which supplies a bare | |
| // hwState with no _clockSource) falls straight through to the | |
| // interpolator below and keeps testing it. | |
| if (typeof hwState._clockSource === 'function') { | |
| let _t; | |
| try { _t = hwState._clockSource(); } catch (_) { _t = NaN; } | |
| if (Number.isFinite(_t)) return _t + hwState.songOffset; | |
| } | |
| // Phase-locked render clock, when the draw loop is live (browser | |
| // mode). Keeps plugins and the renderer on ONE clock. Guarded on | |
| // hwState alone so the source-extraction sandbox in | |
| // tests/js/highway_monotonic_clock.test.js — which supplies a bare | |
| // hwState with no _pllTime — falls through to the interpolator below | |
| // and keeps testing it. | |
| if (Number.isFinite(hwState._pllTime) | |
| && (performance.now() - hwState._pllAt) <= _PLL_RESYNC_MS) { | |
| return hwState._pllTime; | |
| } |
🤖 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 `@static/highway.js` around lines 2513 - 2533, Replace the hardcoded 120 ms
freshness threshold in the getTime PLL-time check with the existing
_PLL_RESYNC_MS constant, preserving the current finite-value and
performance.now()-based staleness logic.
1 participant
Fixes #957.
What
Phase-locks the highway render clock so it stops running backward during playback.
The chart-clock interpolator estimated playback rate from a single quantised sample gap —
audio.currentTimesteps ~23 ms whilesetTime()re-anchors on a 60 Hz poll, so consecutive anchors sit 16.7 ms or 33.3 ms apart and the rate estimate alternated between ~1.38x and ~0.69x — and then snapped its output onto each stale anchor. Together those drove the chart clock backward on ~10% of frames (measured -52 ms on a 200 Hz display), which shows up as shimmer/vibration on the note highway. It's not a draw-cost problem: it reproduced with the highway drawing in 0.8 ms and the adaptive scaler fully disengaged.The render clock now free-runs at a smoothed rate and is pulled gently toward the anchored estimate (proportional correction), hard-resyncing only on a genuine seek. Output is monotone and frame-rate independent (verified 30–360 fps), and plugins and the renderer now read one clock.
Measured on the affected machine:
before: mean 4.96 | min -52.42 | max 36.73 | stddev 11.99 | BACKWARD 42/399
after: mean 5.00 | min 2.60 | max 6.59 | stddev 0.77 | BACKWARD 0/399
Full root-cause analysis, the frame-rate sweep, and the console probe used to measure this are in #957.
feedpak surface
Checklist
CHANGELOG.md[Unreleased]updated (user-visible changes)git commit -s)Summary by CodeRabbit
Bug Fixes
New Features