Skip to content

fix(core): preview playback moves every frame between an audio track's time updates - #7373

Merged
miguel-heygen merged 1 commit into
mainfrom
dperf/clock-between-media-steps
Nov 7, 2026
Merged

miguel-heygen merged 1 commit into
mainfrom
dperf/clock-between-media-steps

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Nov 7, 2026 •

Copy link
Copy Markdown
Collaborator

What

While an audio or video element drives the preview clock, the playhead now moves on every display frame between the element's currentTime updates, instead of holding a frame and then jumping when the next update arrives.

Why

TransportClock.now() returned the element's currentTime as it was (clock.ts, the audio-master branch). Chromium does not move currentTime continuously: it moves in steps, about 20 ms in headless Chromium and 30-50 ms or more on many machines (the comment in init.ts already calls the element tier "~33ms"). A film at 30 fps on a 60 Hz display then shows one frame for three display frames and skips the next whenever a step is longer than a film frame. Viewers see judder, and the runtime's playback health counts the skipped frames as dropped, so the preview reads as choppy on films with a soundtrack even when the page has time to spare.

To reproduce: play any film with an <audio> clip in Chromium and log window.__player.getTime() once per animation frame. It stays the same for 2-3 frames, then jumps by a whole step.

Related work

Refs the audio-master clock tiers in refreshTransportClockSource; no overlapping PR.

How

  • TransportClock keeps a reading of the element it follows (_reading): its last distinct time, its last 8 steps, and where the clock's own run started.
  • Between updates, now() runs on at the play rate, and is only pulled back into a band: no earlier than the element's time, and no later than the element's time plus its largest recent step (steps over 250 ms are jumps, not steps). It never goes back: a pull below where the clock has already been holds the playhead until the element catches up.
  • Inside the band nothing corrects the clock, so frames advance evenly; re-anchoring on each update would carry the up-to-one-frame delay between an update and the clock reading it, and show some frames for three display frames. An element that stops moving stops the clock within one step. An element that plays slightly slow pulls the clock back into the band, so no lead builds up.
  • Media sync measures the element the clock follows against that element's own time: TransportClock.aheadOf(el) says how far the last read ran ahead of it, and syncRuntimeMedia (clockAhead) takes that off the playhead for that element only. Without this, the lead looks like drift: past the 40 ms tolerance steerMedia sped the followed audio to 1.03x, and the playhead, which follows it, ran fast with it. Every other element is still synced to the playhead.
  • A reading further behind the playhead than one step is not a step: it takes the existing path (a late clip, a loop wrap, a buffering hold), unchanged.
  • The reading is dropped on play, pause, seek, rate change, detach, a different element, and while the clock waits for audio output to start. WebAudio sources (already sample-accurate) and the buffering freeze source are not interpolated.

Test plan

  • Unit tests added: clock.test.ts
    • with currentTime moving in 30, 40 and 50 ms steps read every 1/60 s, no 30 fps film frame is skipped, no frame is shown for more than two display frames, no read goes back, and the clock is never more than one step ahead of the element;
    • an element that stops moving stops the clock within one step;
    • an element playing at 0.98x stays within one step for 10 s.
  • clock.steer.test.ts (new): the clock, syncRuntimeMedia and an audio element whose currentTime moves in 45-90 ms steps, played at the rate media sync gives it, at 60, 120 and 144 Hz for 10 s: no rate writes, no seeks, and the film keeps its speed within 0.1%. With media.ts not taking the lead off, it fails (45 ms steps at 60 Hz: rate writes: expected 69 to be +0). It passes 3 runs in a row.
  • Fails without the fix: with clock.ts from main, "moves on every display frame between the element's time updates" fails (30 ms steps: ticks a frame is held: expected 3 to be less than or equal to 1).
  • packages/core src/runtime: 122 files, 2868 tests pass; the clock files pass 3 runs in a row.
  • Manual: the runtime built from main and from this branch, playing a 10 s film with a moving box and an <audio> tone in headless Chromium, the element's currentTime rounded down to 30, 40 or 50 ms to stand in for machines whose updates come that coarse (and once at Chromium's own steps). Per run: film frames skipped, frames held past two display frames, and the runtime's own playback-health readings. The rig measures frame cadence only; the element's playback rate is covered by clock.steer.test.ts. Runs where the page itself missed display frames are left out, since they drop frames whatever the clock does.

5 runs per case; "clean runs" are those where the page drew every display frame (481 in 8 s), the rest left out because a busy machine dropped frames whatever the clock did. Frames counted after the first second, the runtime's own warm-up.

currentTime steps Build Clean runs Film frames skipped, per run Frames shown 3+ display frames, per run Choppy health readings
Chromium's own (~19 ms) main 3 of 5 0, 0, 0 33, 50, 17 0 of 4, 0 of 4, 0 of 4
Chromium's own (~19 ms) this PR 2 of 5 0, 0 1, 1 0 of 4, 0 of 4
30 ms main 2 of 5 0, 0 26, 23 0 of 4, 0 of 4
30 ms this PR 1 of 5 0 11 0 of 4
40 ms main 2 of 5 35, 35 71, 69 0 of 4, 0 of 4
40 ms this PR 3 of 5 0, 0, 0 0, 0, 2 0 of 4, 0 of 4, 0 of 4
50 ms main 3 of 5 70, 70, 70 140, 133, 140 4 of 4, 4 of 4, 4 of 4
50 ms this PR 2 of 5 0, 1 1, 4 0 of 4, 0 of 4

At 50 ms steps on main every playback-health reading is choppy; with this PR none is.

  • Documentation updated (if applicable): no docs describe the clock tiers' timing.
  • Comments follow CONTRIBUTING.md "Comments": the new method's comment says what the clock must do and why.

Size

One method, two small helpers and their reset points in clock.ts, the lead hand-off to media sync (3 lines across media.ts and init.ts), and four tests. It is a lone fix in the runtime's clock: no open PR touches this file to carry it.

@github-actions

github-actions Bot commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 267 (base branch 267), smooth 102 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (8)

Unstable (1)

  • seqrepeat-none-px-r0-root-z100: tracking 0.05, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true / tracking 0.05, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true / tracking 0.05, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verdict: REQUEST CHANGES · 1 blocker · 0 important · 0 nits · head 2a7705d7

The clock now runs up to one currentTime step ahead of the element it follows. syncRuntimeMedia still rate-steers that same element against the clock, so on the coarse-step machines this PR targets, the followed audio track is sped up and the film runs fast. Details inline.

Scope

Read clock.ts/clock.test.ts end to end, plus the paths that consume the clock: media.ts drift/steer (L660–790), mediaSteer.ts, and the per-frame syncMediaForCurrentState in init.ts. Checked the fails without the fix: claim: with main's clock.ts, the new test fails (30 ms steps: ticks a frame is held: expected 3 to be less than or equal to 1). With the PR, clock.test.ts and mediaSteer.test.ts pass (64/64). No earlier reviews. CI not read: this identity can't read check-runs, and mergeStateStatus is BLOCKED.

}
noteStep(last, time);
const run = last.from + ((nowMs - last.fromMs) / 1000) * this._rate;
const held = Math.min(Math.max(run, time), time + furthestAhead(last));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

blocker: this clock now runs up to one step ahead of the element it follows, but syncRuntimeMedia still rate-steers that element against the clock. Each frame, media.ts:682 computes offset = relTime − el.currentTime, which is now up to one full step. When it passes MEDIA_SYNC_TOLERANCE_SECONDS (40 ms), steerMedia (mediaSteer.ts:55) sets the followed <audio> to 1.03x. That speeds up the element the band is anchored to, so the playhead speeds up with it. On main the offset is about 0, because the clock is the element's time.

To check this, I ran a throwaway vitest probe: this TransportClock plus the real steerMedia, with the element's position moving at its written playbackRate and currentTime floored to N ms steps. Over 10 s of wall time:

steps ticks steered ticks rate writes audio / film speed
45 ms 60 Hz 96/600 64 1.0048x
50 ms 144 Hz 263/1440 398 1.0055x
60 ms 60 Hz 224/600 241 1.0112x
70 ms 60 Hz 275/600 223 1.0137x
90 ms 120 Hz 624/1200 221 1.0156x

Main gives 0 steered ticks and 1.0000x in every case. At 20/30 ms steps, and at 40/50 ms on 60 Hz, the PR is effectively clean, because the lead stays under 40 ms. That matches your manual table, which measured frame cadence but not the element's playbackRate. The body says steps are "30-50 ms or more on many machines", so this is the population the fix targets. They would get audible 1.0↔1.03 rate flips (up to ~40 writes/s at 144 Hz) and a film running 0.5–1.5% fast.

A possible fix: don't steer the clock's own element (clock.audioElement()) by its offset against the clock. Or give media sync the raw element time for that element. Then add a test that runs the clock and steerMedia together, so the two can't drift apart again.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Fixed in 8c55d88: clockAhead takes the lead off for the followed element only. clock.steer.test.ts fails against the old media.ts (45 ms @ 60 Hz: 69 rate writes) and passes now.

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verdict: APPROVE · 1 resolved, 0 open, 0 new · 2a7705d → 8c55d88

Checked

With the old media.ts, the new clock.steer.test.ts fails (45 ms @ 60 Hz: 69 rate writes). On this head, clock, media and mediaSteer tests pass (12 files, 398 tests). _ahead cannot leak stale into a later sync: now() clears it, and seek, pause, halt and stop-at-end all re-read now() (or detach first) before syncing. CI not read: this identity can't read check-runs.

@miguel-heygen
miguel-heygen force-pushed the dperf/clock-between-media-steps branch from ad02236 to ca4995c Compare November 7, 2026 22:33

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verdict: APPROVE · 0 resolved, 0 open, 0 new · 8c55d88 → ca4995c

Rebased onto main after #7383 (Safari pre-roll revert). clock.ts, clock.test.ts and clock.steer.test.ts are byte-identical to the approved 8c55d88. The media.ts conflict resolution keeps main's single relTime = mediaSourceTimeAt(clip, …) (the pre-roll ternary is gone) with the clockAhead lead subtracted, and that is still the only relTime site. src/runtime: 122 files, 2867 tests pass locally. CI on this head is still running.

@miguel-heygen
miguel-heygen added this pull request to the merge queue Nov 7, 2026
Merged via the queue into main with commit 942893e Nov 7, 2026
98 of 100 checks passed
@miguel-heygen
miguel-heygen deleted the dperf/clock-between-media-steps branch November 7, 2026 23:03
SanHsien added a commit to SanHsien/hyperframes that referenced this pull request Nov 9, 2026
Port the adopted upstream fixes from f5bde95 to the current upstream tip:
heygen-com#7375 multipart cloud upload, heygen-com#7382 minParallelChunks, heygen-com#7349 HLG header SEI,
heygen-com#7369 skip unmounted compositions in lint/check, heygen-com#7381 filesystem-path media
in the cloud archive, heygen-com#7383 Safari cut-point pre-roll revert, heygen-com#7373 clock
steering between audio time updates, heygen-com#7384 injector test isolation, heygen-com#7378
preview seek retention, heygen-com#7390 virtual timer schedule per worker, heygen-com#7394 CDN
script failure naming, heygen-com#7395 menu row radius, heygen-com#7119 decode timeout caller
test, heygen-com#7397 authored tween order, heygen-com#7402 capture page clip loading, heygen-com#7412 muted
video audio track at cuts, heygen-com#6150 template root variable test, heygen-com#7442 held tail
seek once.

Deliberate local differences: heygen-com#7067's audio-start hold is not carried into
clock.ts, mountedSources comes from heygen-com#6827 as a helper only, cloud/_gen/types.ts
omits the image-request types the fork does not generate, and the lint
symlink test skips through symlinkOrSkip where the account cannot create links.

Co-Authored-By: Claude Sonnet <noreply@anthropic.com>
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.

1 participant