Skip to content

fix(player): correct playback rate for direct-timeline and audio-clock paths#849

Merged
terencecho merged 2 commits into
mainfrom
fix/playback-rate-speed
May 14, 2026
Merged

fix(player): correct playback rate for direct-timeline and audio-clock paths#849
terencecho merged 2 commits into
mainfrom
fix/playback-rate-speed

Conversation

@terencecho
Copy link
Copy Markdown
Contributor

Summary

  • Direct-timeline path (GSAP compositions with window.__timelines): The player drives these via DirectTimelineAdapter, bypassing postMessage entirely. Rate changes sent set-playback-rate to the iframe but had no receiver — GSAP's timeScale() was never called. Fix: add optional timeScale? to DirectTimelineAdapter and call this._directTimelineAdapter?.timeScale?.(rate) in attributeChangedCallback. GSAP timelines expose timeScale natively, no composition changes required.

  • Audio-clock path (compositions with audio): Three bugs caused TransportClock to always run at 1x when an audio element or WebAudio context drove the clock:

    1. schedulePlayback was called without the playbackRate arg (defaulted to 1).
    2. onSetPlaybackRate and player.setPlaybackRate didn't call webAudio.setRate().
    3. TransportClock.attachAudioSource divided by this._rate instead of el.playbackRate, cancelling the rate multiplier.
  • Adds 2 regression tests to clock.test.ts covering the corrected audio-clock formula.

Test plan

  • Unit tests: bun run --cwd packages/core test — 861/861 pass
  • Browser verification (Playwright headless, GSAP direct-timeline composition):
    • 1x speed → ratio 0.972 ✓
    • 2x speed → ratio 1.965 ✓
    • 0.5x speed → ratio 0.490 ✓

🤖 Generated with Claude Code

…-clock paths

Two separate bugs caused playback rate changes to have no effect:

**Direct-timeline path (GSAP compositions with `window.__timelines`):**
The player drives these compositions directly via `DirectTimelineAdapter` — the
postMessage bridge is bypassed entirely. Rate changes only sent a
`set-playback-rate` control message (ignored with no runtime), but never called
GSAP's `timeScale()`. Fix: add optional `timeScale?` to `DirectTimelineAdapter`
and call `this._directTimelineAdapter?.timeScale?.(rate)` in `attributeChangedCallback`.
GSAP timelines expose `timeScale` natively, so no composition changes are needed.

**Audio-clock path (compositions with audio):**
Three bugs in `init.ts` and `clock.ts` caused `TransportClock` to always use
rate=1 when an audio element or WebAudio context drove the clock:
1. `schedulePlayback` was called without the `playbackRate` arg (defaulted to 1).
2. `onSetPlaybackRate` and `player.setPlaybackRate` didn't call `webAudio.setRate()`.
3. `TransportClock.attachAudioSource` divided by `this._rate` instead of
   `el.playbackRate`, cancelling the rate multiplier and locking speed at 1x.

Adds two regression tests to `clock.test.ts` covering the audio-clock formula.

Browser verification (Playwright headless): 1x=0.972 ✓, 2x=1.965 ✓, 0.5x=0.490 ✓

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@miguel-heygen miguel-heygen self-requested a review May 14, 2026 22:10
Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Playback speed fixes

Four targeted fixes for non-1x playback rates — all correct:

  1. Clock formula (clock.ts:51): Old formula (el.currentTime - mediaStart) / this._rate cancels out the speed-up. Fix correctly converts audio-element time to wall-clock elapsed, then scales by composition rate.

  2. Missing webAudio.setRate() calls (init.ts:1604,2016): Both rate-change handlers updated the clock but never told WebAudio transport. Now wired.

  3. Missing state.playbackRate arg (init.ts:1924): 8th param defaulted to 1, so newly-scheduled audio always played at 1x. Now passes actual rate.

  4. Direct-timeline path (hyperframes-player.ts:180): set-playback-rate postMessage had no receiver in GSAP direct-timeline mode. timeScale(rate) is the correct GSAP API.

Two new clock tests cover the formula fix at 2x and mismatched rates. No regressions expected — all fixes only affect non-1x paths, which were already broken. LGTM.

Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — three real bugs identified and fixed coherently, tests pin the math.

Audited

  • Clock formula (clock.ts:51) — old (el.currentTime - mediaStart) / this._rate + compositionStart is inverted: dividing by this._rate cancels the multiplier exactly when el.playbackRate === this._rate (the common case where the audio element and the clock rate match), yielding 1x regardless. New formula ((el.currentTime - mediaStart) / (el.playbackRate || 1)) * this._rate + compositionStart is the right decomposition — divide by the audio element's local rate to get wall-clock elapsed, multiply by clock rate to project into composition time. Both new tests cover the corrected algebra (matched-rate and mismatched-rate cases). ✓
  • Direct-timeline path (hyperframes-player.ts:179, timeline-adapters.ts:27) — optional timeScale? on the adapter + optional-chained call in attributeChangedCallback cleanly extends the adapter contract without forcing non-GSAP adapters to implement it. GSAP's native timeScale() is the right primitive to map onto. ✓
  • Audio-rate propagation — three sites that all touch rate now stay in sync: attributeChangedCallback (postMessage + direct-timeline), onSetPlaybackRate (clock + webAudio), player.setPlaybackRate (clock + webAudio), and schedulePlayback now receives the rate arg. ✓
  • Tests (clock.test.ts) — both new tests assert exact composition-time values from the corrected formula. Future re-inversions of the division will fail these. Good regression pin. ✓

Non-blocking nit

el.playbackRate || 1: a playbackRate of 0 is technically valid for HTMLMediaElement (means paused) — || 1 would defensively coerce that to 1. In practice the !el.paused && Number.isFinite(el.currentTime) guard above this line gates entry, and a playbackRate=0 element generally won't have paused=false, so the bypass is unreachable. Worth a brief comment noting the divisor is guaranteed non-zero by the surrounding guard, or el.playbackRate > 0 ? el.playbackRate : 1 if you'd rather make the guard explicit at the divide site.

CI

Test, Typecheck, Lint, Build, CodeQL, Test: runtime contract, Perf: drift/load/fps/scrub all green. Regression shards + CLI smoke + Windows still running. mergeable_state: blocked is reviewer-gate, not CI-gate.

Review by Rames Jusso (pr-review)

…site

The || 1 form coerces any falsy value but obscures intent. The explicit
> 0 guard makes clear we're protecting against division by zero / negative
rates, and correctly handles negative playbackRate (reverse playback, valid
per spec) without silently treating it as 1x.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving on 2bf2b9a. Nit addressed — el.playbackRate > 0 ? el.playbackRate : 1 makes the non-zero divisor invariant explicit at the divide site. Math and tests unchanged. CI green on required checks (Test/Typecheck/Lint/Build/CodeQL/Test:runtime/Windows tests + render/Preview parity/Perf:drift/load/fps/scrub/parity); regression shards still running but not blocking the review verdict.

Re-review by Rames Jusso (pr-review)

@terencecho terencecho merged commit 9abf65a into main May 14, 2026
42 checks passed
@terencecho terencecho deleted the fix/playback-rate-speed branch May 14, 2026 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants