Skip to content

fix(runtime): make hard-sync and preload boundaries inclusive at clip end#1339

Open
calcarazgre646 wants to merge 1 commit into
heygen-com:mainfrom
calcarazgre646:fix/runtime-media-boundary-inclusive
Open

fix(runtime): make hard-sync and preload boundaries inclusive at clip end#1339
calcarazgre646 wants to merge 1 commit into
heygen-com:mainfrom
calcarazgre646:fix/runtime-media-boundary-inclusive

Conversation

@calcarazgre646

Copy link
Copy Markdown
Contributor

Follow-up to #1173, which made the audio clock and syncRuntimeMedia treat t = end as active to match the visibility change from #1166. Two sites in the runtime still use the exclusive end:

hardSyncAllMedia (init.ts) - observable bug

hardSyncAllMedia skips elements when timeSeconds >= end, so pausing exactly at a clip's end boundary skips the seek for that clip. This matters because the skip is not covered by any other path: syncRuntimeMedia deliberately avoids strict/force sync while a video is playing (decoder-reset cost, see the drift-correction tiers in media.ts), so drift under 0.5s on a playing video is only ever corrected by the play/pause hard sync. Pause exactly at t = end and the paused video shows its drifted frame instead of the final one, while the element is still visible (inclusive since #1166).

getClipsInWindow (mediaPreloader.ts) - consistency only

The preloader's active predicate also used the exclusive end. To be transparent: this one has no observable behavior change, because a clip at exactly t = end stays in the preload window via the lookbehind term on the next line. Updated so the predicate matches the runtime-wide definition of active rather than leaving the last exclusive comparison in the runtime.

Tests

Two tests in init.test.ts driving the real player through the manual-RAF harness:

  • pause exactly at t = end hard-syncs the element to its final frame (fails without the fix: currentTime stays at the drifted value)
  • pause past t = end does not seek the element (guards the upper bound)

The fixture pins drift at 0.3s, below the 0.5s hard-sync tier and above the force-sync threshold, so the assertion isolates hardSyncAllMedia from syncRuntimeMedia's own corrections.

Full core suite green (73 files, 1439 passed).

… end

heygen-com#1166 made visibility inclusive at t = end and heygen-com#1173 aligned the audio
clock and syncRuntimeMedia. Two sites still treated the exact clip end
as inactive:

- hardSyncAllMedia skipped the seek at t = end, so pausing exactly at
  the boundary left a playing video on its drifted frame (drift under
  0.5s is deliberately not corrected by syncRuntimeMedia while a video
  is playing, so the play/pause hard sync is the only path that can fix
  it).
- the media preloader's active predicate used the exclusive end. No
  observable change there (a clip at t = end stays in the window via
  lookbehind), updated for consistency with the runtime-wide definition
  of active.

@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.

Clean fix — two one-character changes (`>=` → `>`, `<` → `<=`) aligning the last two exclusive-end sites with the inclusive-end contract from #1173/#1166.

init.ts `hardSyncAllMedia` — the observable bug. Changing `timeSeconds >= end` to `timeSeconds > end` means pausing exactly at a clip's end boundary now seeks the video to its final frame instead of skipping it. The test proves the fix: clip [2,5], pause at t=5, video.currentTime lands on 3 (correct final relative time) instead of staying at the drifted 3.2.

mediaPreloader.ts `getClipsInWindow` — consistency. `timeSeconds < clip.end` → `timeSeconds <= clip.end` matches the inclusive semantics everywhere else. No user-visible impact but prevents future confusion.

Tests are solid: boundary fixture with deliberate sub-threshold drift (3.2 vs 3.0 — under the 0.5s hard-sync tier), one test at exact boundary (t=end, expects correction), one test past boundary (t=end+0.5, expects no correction).

Ship it.

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.

2 participants