Repository navigation
fix(cli): snapshot stops Chrome decoding videos it already replaces with extracted frames - #6949
Conversation
8eab027 to
6811e31
Compare
![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 6811e316
The memory fix is the right move, but snapshot holds these videos without the intrinsic-size hints that render applies alongside the same hold, so a local video with height:auto now lays out at the wrong size. ↓ inline
Scope
- Read the full diff (5 files), plus
renderFrameVideoHold.ts,isRenderFrameVideoin the runtime, render's hold and hint pairing (extractVideosStage.ts,frameCapture.tsinstallMediaPageHooks/applyVideoMetadataHints), andcaptureCompositionFrame.tsbeforeNavigate. - The ids match on both sides:
localVideoRenderIdsusesdata-hf-render-id || id, the same asholdRenderFrameVideosand__hfMediaId. - The body's
fails without the fix:line names snapshot.test.ts › "holds the project's local videos out of Chrome and reads their durations from FFprobe". That test greps the source, so it does fail without the change. - CI on this head was still running (CI, CodeQL, regression, Windows, preview-regression) when I reviewed.
6811e31 to
cc48b47
Compare
Fallow audit reportQuality gate failed Found 1 finding. Health (1)
Generated by fallow.
|
There was a problem hiding this comment.
Verdict: APPROVE — 1 resolved, 0 open, 0 new · 6811e316 → cc48b474
CI note
The optional Comments check fails because the new comment line at captureCompositionFrame.ts:129 raises that file's comment share from 6.9% to 7.0%. It isn't a required check, so it doesn't block. Deleting that line clears it. The required checks were still running when I approved.
There was a problem hiding this comment.
Verdict: APPROVE — 0 resolved, 0 open, 0 new · cc48b474 → 9478dc16 (one comment reworded in snapshotHeldVideos.ts; the earlier commits are unchanged)
CI note
The optional Comments check still fails on this head. The ratchet is flagging captureCompositionFrame.ts (the added // Runs after navigation and before the render-ready wait. line, 6.9% → 7.0%), not snapshotHeldVideos.ts. Removing that one line clears it. It isn't a required check.
What
hyperframes snapshotno longer crashes Chrome on a project with many clips of one 4K source. Chrome stops decoding the project's own video files, because snapshot already replaces every active video with a frame it extracted with FFmpeg.Why
Three reports (about 23, 36 and 48 clips of one 4K 10-bit 4:2:2 H.264 file with a rotation flag, and
checkpassing on the same project) end invideo frame injection failed, thenAttempted to use detached FrameorTarget closed. The page process runs out of memory: each active<video>gets its own decoder, about 0.5 GB for this source. Marking the<video>tagsdata-hiddenmade the snapshot work, which matches.Render already avoids this: a video it replaces with extracted frames is held at
preload="none"from parse (holdRenderFrameVideos). Snapshot replaced its videos the same way but never held them.Change
localVideoRenderIds) with the same engine function. Remote clips are unchanged: Chrome still loads them, and snapshot still downloads a local copy for FFmpeg.data-duration, loops, and the end-of-source clamp. Those durations now come from FFprobe (probeHeldVideo, once per file), together with the size.applyVideoMetadataHints, run before the readiness wait through the page-loaded hook), from the same FFprobe read with a rotation swap. Without it awidth:100%;height:autoclip with no poster lays out at 300x150 and the injected frame lands in a squashed box.projectLocalFile, used by both.holdRenderFrameVideosandapplyVideoMetadataHintsare exported from the engine.Measured
Linux x64, 4K 10-bit 4:2:2 H.264 with a 90 degree rotation flag, every clip active at once, Chrome process memory at its peak:
The still for 6 clips is byte-identical to the published CLI's. The remaining process-tree memory (about 5.2 GB) is the four FFmpeg decodes snapshot already caps.
fails without the fix: packages/cli/src/commands/snapshotHeldVideos.test.ts > "gives a held height:auto clip its intrinsic size, as render does, and skips what has none"; packages/cli/src/commands/snapshot.test.ts > "holds the project's local videos out of Chrome and reads their durations and sizes from FFprobe"