Skip to content

fix(core): stop script loading time from shifting WAAPI frames - #5017

Merged
jrusso1020 merged 1 commit into
heygen-com:mainfrom
cooleryu:fix/4559-frame-stable
Nov 5, 2026
Merged

jrusso1020 merged 1 commit into
heygen-com:mainfrom
cooleryu:fix/4559-frame-stable

Conversation

@cooleryu

@cooleryu cooleryu commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

What

Keep initial WAAPI frames on composition time, rather than on how long the page took to load.

Why

The first baseline currently retains a running animation's currentTime. If the animation starts before the runtime and more scripts load afterward, that elapsed wall-clock time becomes a permanent seek offset. Repeated exports can therefore put the same object at different positions in the same frame.

In the original October 4 reproduction, I checked the producer's HTML compilation, capture, MP4 encoding, and decoded-frame path, not just the adapter. A 4-second, 100 px translation with an initial left edge of 40 px should land at 90 px at 2 seconds:

Local scripts loaded after creation Before: three exports After: three exports
0 90, 90, 90 px 90, 90, 90 px
8 92, 92, 92 px 90, 90, 90 px
32 98, 92, 92 px 90, 90, 90 px

The scripts are tiny local files, with no artificial sleep. Paused-offset controls retain their authored position. These are macOS/Chrome measurements, not a cross-platform performance claim.

Related work

Refs #4559. Proposed contract and reproduction: #4559 (comment)

How

At initial discovery only, anchor running or already-finished JavaScript animations on document.timeline at normal playback rate to composition zero. Including finished animations lets short, filled effects rewind after a slow load. Keep existing baselines for paused animations, CSS animations/transitions, other timelines, non-default playback rates, late creation, and rediscovery.

Compatibility: a pre-runtime currentTime assignment left running is indistinguishable from elapsed loading time. This change does not retain it as a baseline offset. Use positive/negative delay, or pause() followed by currentTime, for an explicit offset. There is a browser test for this behavior; it is not an accidental side effect. No new API or timing heuristic is introduced.

Review follow-up

  • Rebased onto dd7290ae and dropped the edit to the retired WAAPI adapter guide. The current skills/hyperframes/references/runtimes.md already describes document time and explicit offsets; no generated manifest change is needed.
  • Extracted the initial-origin predicate and reused isBrowserCreatedCss, keeping the same timing contract without pushing ensureBaseline over the Fallow complexity threshold.
  • The PR remains three files: the adapter, its initial-baseline tests, and the browser regressions.

Test plan

  • Added 21 adapter cases: initial progress, direct seek/pause, finished/idle states, paused offsets, exclusions, repeated discovery, late creation, and revert. October 28: 326/326 adapter tests pass; a final rebase check of the two WAAPI files also passes 46/46.
  • Added 9 real-Chrome cases using the built runtime and both public seek methods. Seven failed against the original artifact in the initial comparison. October 28: the complete browser file passes 90/90, including all nine cases.
  • Full test:hyperframe-runtime-ci passes: 107 files / 2,635 tests, runtime type checking, contract/behavior/seek/duration/parity/security checks, coverage gates, and linter.
  • Producer type checking and test classification pass.
  • Fallow audit reports no findings; skills manifest is in sync; changed-file lint/format and diff whitespace checks pass.
  • Original October 4 integration experiment: 36 browser contexts / 288 seek samples, plus 18 final MP4 exports / 720 decoded frames, all within 1 px of expected positions. Across 0/8/32 scripts and three repetitions, each paused/running group had identical decoded RGB output. These exports were not rerun for this review follow-up; the committed browser regressions were.
VITEST_MAX_WORKERS=2 bun run --cwd packages/core test:hyperframe-runtime-ci
bun run --cwd packages/core test src/runtime/adapters
bun run --cwd packages/producer test:classification
bunx tsc --noEmit -p packages/producer/tsconfig.json
bun run --cwd packages/cli gen:skills-manifest --check
bunx fallow audit --base origin/main --fail-on-issues --format pr-comment-github
cd packages/producer
PUPPETEER_EXECUTABLE_PATH='/Applications/Google Chrome.app/Contents/MacOS/Google Chrome' VITEST_MAX_WORKERS=2 bunx vitest run src/services/coreRuntimeBrowser.test.ts

The browser suite needs Chrome. Initial broad local runs hit default test timeouts; reruns with two workers passed without changing deadlines or assertions. The last upstream commit only changes Studio copy; the tested runtime, producer, dependencies and scripts are unchanged. I have not validated Linux BeginFrame/GPU capture, distributed workers, or the Studio UI. This is not a general fix for non-default playback rates or custom timelines.

@cooleryu
cooleryu marked this pull request as ready for review October 4, 2026 13:55

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm commenting rather than approving because the code is right but this sets a timing contract, and a maintainer should agree to that contract first.

The code (looks correct)

  • packages/core/src/runtime/adapters/waapi.ts:57-73: the zero anchor applies only on first discovery, at composition time 0, to running or finished animations with playbackRate === 1 on document.timeline. CSS animations and transitions are excluded. Paused, idle, other-timeline, late-created and rediscovered animations keep their old baselines.
  • waapi.ts:155-159 moves didDiscover = true after the initial trackAnimations. I checked whether this changes the old normalizeInitialAnimationTimeMs path. The first runAdapters("discover") in init.ts runs at time 0, and rediscovery runs with didDiscover already true. Only one case changes: an animation created after a pre-discover seek(t>0) but before discover now keeps its raw time instead of the normalized one. I don't think the runtime reaches that case today. Mentioning it only so it's a known edge.

The decision for a maintainer
A running animation that had currentTime assigned before the runtime loads no longer keeps that value as an offset. The skill doc (skills/hyperframes-animation/adapters/waapi.md) now directs authors to delay or to pause() + currentTime. I agree it can't be told apart from load time. But it changes behavior for existing compositions, and the issue explicitly left "where script-created animations start" as an open decision. A maintainer should sign off on that contract before this merges.

What I ran

  • vitest run src/runtime/adapters/waapi: 43/43 pass.
  • Mutation check: with the base waapi.ts restored, 9 of the new adapter cases fail (wall-clock progress at 16/90/300/1500 ms, seek/pause before discover, finished rewind, repeated discovery, revert).
  • Not run: the 9 real-Chrome cases in coreRuntimeBrowser.test.ts. CI hasn't run them either, because the fork workflows still need approval.

— Rames

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This needs a rebase, but only for a docs file. The code still merges cleanly.

Reviewed at c1ac9c5d, the same head as my earlier comment. Since then #5651 retired skills/hyperframes-animation/, so adapters/waapi.md no longer exists on main, and that modify/delete is the whole conflict. The WAAPI guidance now lives in one row of skills/hyperframes/references/runtimes.md, which already tells authors to call pause() and use delay for offsets. That matches the contract this PR proposes, so dropping the doc hunk is probably enough. If you want to keep the "pause() then currentTime" note, a short addition to that row is the place for it.

I checked the code against current main. waapi.ts merges without conflict next to #5503's holdAnimationAt change, which fixed a different bug (pause ordering at the first seek, not the initial baseline). On a local merge with the doc file dropped, all 326 adapter tests in packages/core/src/runtime/adapters/ pass, including your 21 new ones.

Two things unchanged from my earlier comment:

  • The timing contract (a running animation's pre-runtime currentTime is no longer kept as an offset) still needs a maintainer to sign off before this merges.
  • The commit is unsigned, and main requires signed commits, so please re-sign it when you rebase.

Need: yes, load time leaks into WAAPI frame positions (#4559) · Size: ok · Reuse: extends the existing baseline logic in the WAAPI adapter · Simpler: already minimal

— Rames

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Adding one thing to the earlier review, which already covers the conflict, the timing contract question and commit signing.

CI did run on this commit, and two checks are red besides the conflict:

  • Skills: manifest in sync fails because the adapters/waapi.md change makes the hyperframes-animation entry in skills-manifest.json stale. Dropping that doc hunk when you rebase, as suggested above, fixes it. If you move the note into skills/hyperframes/references/runtimes.md instead, run bun run --cwd packages/cli gen:skills-manifest and commit the result.
  • Fallow audit reports new issues, but on a fork the job cannot post its findings as a PR comment, so they are not visible here. Running bunx fallow audit --base origin/main locally after the rebase shows the same list.

Verdict: COMMENT
Reasoning: The code still looks right and the open items are the rebase, the timing contract sign-off and signing, plus two red checks the author should clear on the rebased commit.

@miguel-heygen

Copy link
Copy Markdown
Collaborator

On the timing contract: yes. A currentTime set on a running animation before the runtime loads can't be told apart from load time, and delay, or pause() then currentTime, already covers a deliberate offset, which is what skills/hyperframes/references/runtimes.md tells authors. What's left is the rebase (drop the deleted doc hunk) and a signed commit.

@cooleryu
cooleryu force-pushed the fix/4559-frame-stable branch from c1ac9c5 to 31095ca Compare October 28, 2026 10:57
@cooleryu

Copy link
Copy Markdown
Contributor Author

Thanks for confirming the timing contract. Updated in 31095ca:

  • Rebased onto dd7290a, dropped the retired WAAPI guide edit, and replaced the commit with a GitHub-verified signed commit. The diff is still three files.
  • Extracted the initial-origin predicate and reused the existing CSS-animation helper; the timing behavior is unchanged and Fallow now reports no findings.
  • Local validation passes: full runtime CI (107 files / 2,635 tests, including 326 adapter tests), all 90 real-Chrome browser cases, producer type checking and test classification, skills manifest sync, and changed-file lint/format. Local reruns used two workers without changing test deadlines or assertions.

The PR remains ready for review and is mergeable. All nine upstream workflow runs for this head currently show action_required; could you approve those runs when convenient? The earlier MP4 export measurements remain historical, not a new export run.

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed at 31095cadf4a4c49b075ead56b3b1b75bb74cb6f1.

The initial-origin predicate excludes paused, non-unit-rate, custom-timeline and CSS animations. Miguel explicitly accepted the timing contract, the rebase drops the retired guide and the independent runtime pass found no source blocker.

The exact-head new WAAPI unit suite passed 21 cases. The producer browser suite did not collect because the donor core lacks compiler/html-document. No real-Chrome render in this sweep.

Need: yes · Size: ok · Reuse: existing implementation and tests · Simpler: already bounded · Structure: fine
Risk: medium - initial WAAPI clock anchoring and seek semantics.

Verdict: APPROVE

Reasoning: The initial-origin contract is explicit and the adapter controls pass; no remaining blocker found.

@jrusso1020
jrusso1020 enabled auto-merge November 5, 2026 02:16
@jrusso1020
jrusso1020 added this pull request to the merge queue Nov 5, 2026
Merged via the queue into heygen-com:main with commit 9820703 Nov 5, 2026
86 of 97 checks passed
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