Repository navigation
fix(engine): a video clip shows a new source frame on every frame it is on screen - #7471
Conversation
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 blocker · 0 important · 0 nits · head 81cb0036
- blocker —
oxfmt --checkfails onpackages/engine/src/services/videoFrameExtractor.ts(regression› Preflight (lint + format), run 42732587326). Dropping...frameCapfrom the single-passrunFfmpeg([...])call (~L1465) probably lets it fit on one line again, and oxfmt now wants it collapsed. This is stacked onfix/short-clip-coverage, so theCIworkflow didn't run. Its requiredFormatjob will fail the same way once this targetsmain. Runbun run format.
Scope
I read the diff against #7469's head 6607d9c7: 6 files, both commits. The count works out to ceil(x − 1e-4) slots, where x is the window length in output frames. That matches the [start,end) slots isInClipWindow shows (0.27 s at 30 fps → 9, 0.266667 → 8). The coverage gate reads the same function through "slots", on the start that alignClipStartToFirstOutputFrame already moved in place. Every counted CFR window now goes through segments, which bound their own frames, so removing #7469's frameCap is safe. Only a decimal rate (count 0) keeps the -t path. The cache bump to v10 is needed. The fails-without-the-fix lines are present. Outside the format check I found no correctness blocker. I didn't run tests locally.
There was a problem hiding this comment.
Verdict: APPROVE · 1 resolved, 0 open, 0 new · 81cb003 → a5f68e8
The format fix is formatter output only. Preflight and all 10 regression shards are green. Heads-up for when this reaches main: it adds two ffmpeg cases to videoFrameExtractor.test.ts. #7469's flake gate timed out on that file at 120 s and only passed once its test moved out, so put these in a separate file too, or the gate will fail again.
a5f68e8 to
c470bdd
Compare
There was a problem hiding this comment.
Verdict: APPROVE · 0 open, 0 new · a5f68e8 → c470bdd
Range-diff against the previous head: the code matches a5f68e89. The only change is that the two ffmpeg cases moved to videoFrameExtractor.slots.test.ts, which builds its own fixture. The large test file now changes by count assertions only, and the fails-without-the-fix line points at the new file.
The base branch was changed.
c470bdd to
2c47507
Compare
Edit accuracy: accurate 267 (base branch 267), smooth 107 of thoseThe gate passes. Quarantined, measured but not gated (8)
Unstable (1)
|
…nce watermark Seventeen commits, PRs 7469-7493, issues 7473 and 7486 and twelve pin moves are recorded with their evidence in the continued ledger, DECISIONS, UPSTREAM and ROADMAP. Adopted: heygen-com#7466, heygen-com#7479, heygen-com#7482, heygen-com#7490 and the heygen-com#7441 and heygen-com#7440 docs. Deferred: heygen-com#7469, heygen-com#7471, heygen-com#7476, heygen-com#7480, heygen-com#7468, heygen-com#7487, heygen-com#7491. Not applicable: heygen-com#7470, heygen-com#7351, heygen-com#7475, heygen-com#7483. The watermark moves to 61e720b, PR heygen-com#7493, issue heygen-com#7486; thirteen new PRs are pinned. FORK.md records the subComposition test differences. The core packed-size roadmap row trigger is not met. Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
What
A video clip now gets a new source frame on every output frame it is on screen. Before this, a clip whose length was not a whole number of output frames, or that started between output frames, showed its second-to-last picture twice and never showed its last one. The coverage gate counted frames the same short way, so it never noticed.
Why
Extraction counted a constant-frame-rate window as its length in frames rounded to the nearest whole frame, which is where FFmpeg's
-tstops. The render shows a clip on every output frame inside its window (exportClipWindow+isInClipWindow), which is the count rounded up. Two common cases fall short by one:alignClipStartToFirstOutputFrame), and the shortened window's fraction is often under one halfThe render then holds the last extracted frame for the uncovered slot: one repeated picture at the end of the clip.
Reproduce on main: a 30 fps clip at
data-start="0.01"withdata-duration="0.27"over a counter video renders source frames0, 1, 2, 3, 4, 5, 6, 6. With this change it renders0, 1, 2, 3, 4, 5, 6, 7.Related work
Stacked on #7469 (same extraction lines). Review after it; the diff against #7469's branch is this PR's change.
How
extractionFrameCountForDurationcounts a CFR window as every output slot it covers. A tolerance of 1e-4 frames absorbs microsecond-rounded probe durations. It is deliberately below the export snap's 1e-3, so a window ending right at that snap edge gets a spare held copy, never a missing frame.-trounding can no longer drop the last slot. The single-pass-tpath remains only for counts it cannot make (a decimal rate)."nearest"mode is renamed"slots"), so a clip missing its last slot now fails the gate instead of passing.hfcache-v9-tohfcache-v10-: the same key now yields one more frame for these windows. Locally, without the bump, a build of this branch failed the gate on 30 entries a fix(engine): short clips from Apple-encoded videos keep their last frame #7469 build had cached.Size
Small on purpose: one count rule, one routing condition, one rename. It stays out of #7469, which fixes a different failure and is already in review.
Test plan
fails without the fix:
videoFrameExtractor.slots.test.ts› a clip's frames match the output frames it is on screen › gives a 0.01-0.28 s clip a new source frame on every output frame it is on screen (expected [ +0, 1, 2, 3, 4, 5, 6 ] to deeply equal [ +0, 1, 2, 3, 4, 5, 6, 7 ]), and the 0-0.27 s case (expected [ +0, 1, 2, 3, 4, 5, 6, 7 ] to deeply equal [ +0, 1, 2, 3, 4, 5, 6, 7, 8 ])fails without the fix:
videoFrameCoverage.test.ts› rejects a short CFR clip missing the last output slot it covers (expectedFrames7, not 8, with the old count)videoFrameExtractor.test.tschanges only by the two assertions below; 10 runs in a row at about 2 s each.ffprobe.test.tscases that read git-lfs PNG fixtures missing from my checkout (the same 4 fail on main there). Producer unit lane: 1237 passed. Producer integration lane: passes. Engine and core typecheck clean. New tests: 3 runs in a row.