Repository navigation
fix(core): a video held on its tail is seeked once, not every frame - #7442
Conversation
09261f0 to
1be58d8
Compare
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes at 1be58d88. There are two blockers.
1. A held tail moved by other code is never re-seeked. mediaSourceTimeAt clamps a non-loop video to sourceDuration. That makes relTime the same value on every held tick. So once heldTailSeekedTo is set, tailSettled is true on every later held tick, whatever el.currentTime actually is. The map is only cleared by a non-tail seek or by evictMediaSyncState. Other code writes currentTime on these elements while they are held: pausedFrames.clear (init.ts ~5086), the render prepare path (~5111, followed by a forceSync pass that this change now skips), hardSyncAllMedia, the color-grading borrow, and author code. Any of these can leave the clip off its last frame for the rest of its hold.
I confirmed this locally with a probe next to your test. It uses the same clamped currentTime mock: one sync at t=2, then time = 0 (the element is moved elsewhere), then a sync at t=2.02. It expected 2 seeks and got 1 ([0.25001]).
Suggested fix: after the seek, remember where the element landed (el.currentTime), not only the target. Skip only while el.currentTime is still within a frame or so of that landing point (or while drift is within a frame-sized tolerance rather than > 0.001). Please add a regression test for the moved-element case.
2. The body needs a fails without the fix: line naming the test. The test plan says it fails with 5 seeks, but this lane requires an explicit fails without the fix: <test name> line. I confirmed the claim: against the parent media.ts, "seeks a held video tail once, though the browser lands short of the source end" fails with 5 seeks.
Otherwise the change is fine: the scope is tight, eviction clears the new map, and a non-tail seek deletes the entry.
There was a problem hiding this comment.
Approved at 96802ca4. Both blockers are resolved.
- Moved element: now the seek is skipped only while playing, while the target is the same, and while
el.currentTimeis within 0.05s of where the seek landed. If other code moves the element, it re-seeks. Paused andforceSyncpasses, including a render's prepare, seek as before. My earlier probe is now covered by "seeks a held video tail again when something else moved the element off it". fails without the fix:line: it's present and accurate. Against main'smedia.ts, "seeks a held video tail once…" fails (5 seeks), and the paused-pass test passes, so it guards the paused path. With the PR,media.test.tsandinit.test.tspass locally (473 tests).
Not blocking: landed is read right after the assignment. A real browser reports the requested time there, not where the seek actually lands. The 0.05s tolerance absorbs a last-frame clamp at 24fps or faster. For lower-fps sources it falls back to today's seek-every-tick behaviour, so it's no worse than main. You could record landed on seeked later if that matters.
There was a problem hiding this comment.
I'm replacing my approval at 96802ca4 with a request for changes. The code is right, but CI is red because of this PR. regression fails at Preflight oxfmt --check, on packages/core/src/runtime/media.ts and media.test.ts. Both files pass on main.
Fix: run bunx oxfmt packages/core/src/runtime/media.ts packages/core/src/runtime/media.test.ts. That's two line wraps and no change in behaviour:
- media.ts ~851:
if (isHeldVideoTail)thenheldTailSeekedTo.set(...)on the next line - media.test.ts ~1334: the paused-pass
syncRuntimeMedia({...})call split one property per line
My code review from the previous comment still holds. I'll approve a head that differs from this one only by formatting once its CI is green.
There was a problem hiding this comment.
Requesting changes at bfae14b6. The formatting fix is clean: the diff from 96802ca4 is exactly oxfmt's output, and oxfmt --check passes on both files. But CI is still red because of this PR, now in Comments (the comment ratchet):
RAISED comment share: packages/core/src/runtime/media.ts is 23.0% (216/939), at the base 23.0% (212/923)
4 comment line(s) added. ... A file's comment share may only go down.
With 12 code lines added, at most 3 comment lines keep the file's share at or below the base (212/923). Cutting one line is enough. The simplest is the line above const heldTail = … ("Already sent to this tail time: the browser landed short of it, and asking again would seek every tick."). It repeats the comment on the heldTailSeekedTo declaration. Please keep the "Only while playing…" line, since it explains the paused/render carve-out.
I'll approve a head that changes only comments, once its CI is green.
There was a problem hiding this comment.
Approved at d825c2f9. Since bfae14b6, the only change is deleting the duplicate comment line; the 'Only while playing…' line is kept. All 10 workflows on this head are green, including Comments, regression (preflight format) and CI. My code review from 96802ca4 still holds.
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>
What
A video held on its last frame is sent to that frame once, instead of every frame it is held.
Why
When a clip's media position is past the end of its source, the runtime holds the video on its tail by seeking to the source end. A browser lands a seek to the end a few milliseconds short of it, so the clip still read as out of sync and the next frame seeked again, about 90 seeks per clip on a real phone run (four such clips in one film). Each seek resets the decoder.
Related work
None open that touch the tail hold.
How
media.tsremembers the source time a held tail was last sent to and where the browser put the element (heldTailSeekedTo, cleared with the rest of the clip's sync state inevictMediaSyncState). While the transport is playing, the clip is held at that same time and the element is still within 0.05 s of where the seek left it, the hard, strict and forced seeks are skipped. A paused pass (a user seek, a render's prepare), a new tail time, or an element moved by anything else (a reload resets it to 0) seeks as before, so nothing that relies on a forced pass changes.Test plan
media.test.ts: "seeks a held video tail once, though the browser lands short of the source end", "seeks a held video tail again when something else moved the element off it", "seeks a held video tail on every paused pass, as a render's prepare relies on". The three tests, andinit.test.ts, pass (473 in all).Size
79 changed lines, under the 100-line floor, on purpose: a lone fix in
media.tsthat no open PR can carry. The only other open Safari work (#7446, #7447) is draft and measure-only, and #7436 is on hold, so folding it in would hold a correct fix behind unfinished experiments.