feat(cli): snapshot --against pairs each frame with a reference video - #3637
Conversation
Rebuilding footage means checking the render against the source at the same timestamps. Until now that meant hand-rolling it: ffmpeg frame grabs, an image tool for the side-by-side sheet, a fresh script per round; `compare` only puts projects next to each other. `--against <video>` saves the reference frame at every captured time (frame-exact seek: -ss after -i, so a keyframe snap never reads as a mismatch) plus a labelled render|reference pair sheet built with the existing contact-sheet helper. No new dependencies.
880a698 to
2580cc2
Compare
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Approved at 2580cc29ce765430ab48deb4d1642be5521b5237.
--against uses a local resolved path, validates its existence before capture, and uses accurate FFmpeg seeking (-ss after -i) for the reference frame. Each successful reference write is paired in render/reference order with stable labels; missing reference frames warn and preserve the render snapshot. Existing timestamp, cleanup, and command-error paths remain intact. Current checks are green.
Review by Jerrai
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at head 2580cc29ce765430ab48deb4d1642be5521b5237. Additive to @jerrai-bot-heygen's approval - they covered the reference-path validation, the accurate-seek flag order, label ordering, and the missing-frame warning, and I agree with all four. One gap below that their review does not cover.
Strengths
snapshot.ts:145-160gets the seek order right, and the claim "frame-exact" is accurate:accurateSeekemits-i <path> -ss <t>(decode from the start) while the fast path keeps-ss <t> -i <path>(keyframe snap). The call site passestrue(snapshot.ts:597). The existing comment is extended rather than replaced, so the tradeoff stays documented at the point of use.- The output filenames were chosen so the existing summary sheet ignores them.
createSnapshotContactSheetselects onf.startsWith("frame-") && f.endsWith(".png")(packages/cli/src/capture/contactSheet.ts), soref-*.pngandpair-*.jpgare both excluded andcontact-sheet.jpgis unaffected. That is easy to get wrong by picking aframe-ref-…style name, and it is right here. - The degraded case is handled at the right granularity (
snapshot.ts:598-602): a reference video with no frame at a given time warns and skips only that pair, keeping the render snapshot. - I verified all four assertions in the new test are present in the head source, so the test claim holds without running the suite.
important - savedPaths gains the derived pair sheet, which changes an unrelated default-on feature
snapshot.ts:614 pushes pairPath into saved, while refPath is written to disk and never pushed. savedPaths is returned as paths and has three consumers, and two of them misread it:
- The count is wrong.
snapshot my-project --at 1.5,4.3 --against ref.mp4prints "4 snapshots saved" for two requested times (snapshot.ts:802), then lists the pair sheets as if they were captures. --describeruns overpathsand is on by default.describeArgis"true"whenever--describeis omitted (snapshot.ts:750-752), skipped only when noGEMINI_API_KEYis present. So with a key set, everypair-*.jpgis sent to Gemini (snapshot.ts:865) under the single-frame prompt "Describe this video composition frame in 1-2 sentences ... is the frame blank/black/loading" (snapshot.ts:838). That doubles the calls on an--againstrun and asks a single-frame question of a side-by-side comparison image.- The documented output is the one missing from the list.
docs/packages/cli.mdxadvertisesref-XX-….pngas a saved artifact, but it never appears in the printed paths, while the sheet derived from it does.
The repo already settles this: contact-sheet.jpg is a derived artifact and is deliberately not in savedPaths (there is exactly one push site, snapshot.ts:622). The pair sheet is the same class of thing.
Smallest fix consistent with that convention: drop saved.push(pairPath). If the reference frame should be listed - which would match the docs - push refPath instead, so the count stays one-per-captured-time and --describe keeps receiving single frames.
note, not a finding - the new test asserts on source substrings rather than behaviour, so it pins the wiring rather than the capture. That matches this file's established convention (the sibling test at snapshot.test.ts:78 does the same with opts.autoProxy), so I am not filing it.
Audited: packages/cli/src/commands/snapshot.ts (read end-to-end at head), packages/cli/src/capture/contactSheet.ts (createSnapshotContactSheet selection logic), packages/cli/src/commands/snapshot.test.ts, docs/packages/cli.mdx.
Trusting: the PR-body claim that two snapshot lint preflight failures are pre-existing and unchanged on main - I did not run the suite at both refs, and a claim of that shape only means something with a control at each ref, which is heavier than this change warrants. Flagging it as unverified rather than implying otherwise.
Verdict: APPROVE
Reasoning: The feature is well-built and the seek semantics match the claim. The savedPaths membership is a real defect with a one-line fix, but it degrades output quality rather than correctness of the capture, so it reads as important rather than blocking. Happy to re-read after that change.
- Rames Jusso
Review follow-up: the pair sheet was pushed into savedPaths, so the summary counted it as a capture and --describe (on by default with a Gemini key) sent side-by-side sheets to a single-frame prompt. Reference frames and pair sheets are derived artifacts like contact-sheet.jpg; they now stay out of the list and the summary names them on one line.
|
Addressed in |
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Re-approved at eb6e6e6467fa096f9eb4aa1643bad47e19654744.
The follow-up correctly keeps both derived --against artifacts—the reference PNG and the render/reference pair sheet—out of savedPaths. The terminal count/list and default Gemini description path therefore continue to operate only on captured render frames; the CLI still prints where the derived artifacts were written. The prior accurate-seek, ordered-pair, validation, and partial-reference behavior remains intact. Required checks are still running; this is a code-quality approval only.
Review by Jerrai
jrusso1020
left a comment
There was a problem hiding this comment.
Re-reviewed at head eb6e6e6467fa096f9eb4aa1643bad47e19654744 - the fix for the savedPaths finding in my previous review. Additive to @jerrai-bot-heygen's re-approve at this same head. I checked the delta since 2580cc29 rather than making a fresh pass over the whole PR, so read this as scoped to that.
The fix is correct, and the option chosen is better than the one I suggested. I offered "drop saved.push(pairPath), or push refPath instead to match the docs." Excluding both and naming them in a summary line is a third option and it is the right one: it keeps savedPaths meaning one thing regardless of flags, instead of meaning "captures" normally and "captures plus some derived artifacts" under --against.
Verified all three consumers at this head:
snapshot.ts:800- the count readspaths.length, now one entry per capture, so two requested times print "2 snapshots saved". Matches the smoke test described on the PR.snapshot.ts:802-804- the listing shows frame files only, and the new dim line at :806 names the derived artifacts without inflating the count. It sits directly above the existingcontact-sheet.jpgdim line at :820, so it reads as the same convention it cites.snapshot.ts:865---describemaps overpaths, so onlyframe-*.pngreach Gemini under the single-frame prompt. That was the substantive half of the original finding and it is gone.
Also confirmed there is no fallout from removing the saved local: refPath (:606) and pairPath (:607) are both still consumed by createContactSheet at :609, so no unused-variable problem, and savedPaths now has exactly one push site (:622).
nit - snapshot.ts:805: the summary line is gated on against alone, not on whether any pair was actually written. Point --against at a video shorter than the requested times and every frame logs --against has no frame at ... - reference pair skipped, then the summary still says ref-*.png + pair-*.jpg sit beside each frame. The per-time warnings keep that from being genuinely misleading, so it is cosmetic. Gating the line on at least one successful pair would close it.
Verdict: APPROVE
Reasoning: The delta since my last review does exactly what the finding asked, the three consumers that misread savedPaths now read it correctly, and the one item left is cosmetic.
- Rames Jusso
What
hyperframes snapshot --against <video>: for every captured time, also save the reference video's frame at that time and a labelled render | reference pair sheet.Why
Rebuilding footage (a 1:1 recreation, a re-cut, a re-render of a reference) means checking the render against the source at the same timestamps. Until now that was hand-rolled every time: ffmpeg frame grabs, an image tool for the side-by-side sheet, a fresh script per round.
compareonly puts projects next to each other; nothing in the CLI compared a composition to a video.How
extractVideoFrameToBuffer, with a newaccurateSeekoption that puts-ssafter-i. The fast keyframe seek is ±1 frame, which would read as a real mismatch in a pair sheet.createContactSheet(2 columns, 960 px cells, labelsrender/reference). No new dependencies.contact-sheet.jpgstill only picks upframe-*.png.--againstpointing at a missing file fails the command up front.Test plan
snapshot.test.ts, source contract for the flag, the accurate seek and the pair labels). Two pre-existing "snapshot lint preflight" failures in that suite are unchanged onmain.docs/packages/cli.mdx)