Skip to content

fix(snapshot): video frames keep the colours render gives the same footage - #6425

Merged
miguel-heygen merged 4 commits into
mainfrom
fix/snapshot-srgb-retag
Oct 26, 2026
Merged

miguel-heygen merged 4 commits into
mainfrom
fix/snapshot-srgb-retag

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #6157

What was wrong

Render retags SDR footage as sRGB when it extracts frames, and snapshot did not, so a snapshot's video frames kept the source's BT.709 transfer tag. Chrome 152 colour-manages that tag (Chrome 131 ignored it), so snapshot stills came out about 4% darker than the same composition rendered.

Fix

The retag now has one owner, sdrCanvasColourFilters in the engine's frame extractor. Render uses it where it used to inline the filters, and snapshot applies it to the frame it injects over each <video>. The snapshot frame is probed first, and the helper owns the HDR decision: HDR yields no retag unless it was tone-mapped, which render does and snapshot does not.

Design calls:

  • Share the function, not the constant: the untagged-matrix guess that render applies first depends on the probed codec and height, so snapshot needs the probe, not a copied filter string.
  • No ffprobe: render has no path without it (it fails), so there is nothing to copy. Snapshot warns once, "snapshot colours may differ from render: install ffmpeg/ffprobe", and leaves the frame as decoded, rather than staying silent.
  • HDR stays out of snapshot: retagging an un-tone-mapped HDR frame as sRGB would be wrong, so that gap is left as it is.
  • Existing snapshot tests compared PNG bytes; they now compare pixel data, because the retag adds colour-tag chunks that are not part of the picture.

Test

New test: a BT.709 clip's snapshot frame carries the sRGB tag. With the snapshot call removed it fails (expected false to be true); with it, snapshot.test.ts is 89 passed, 0 failed. videoFrameExtractor engine tests: 272 passed, 1 failed, and that failure is remoteTail asserting extraction concurrency above the host's CPU count (16 on the runner), which this change does not touch.

Why under 100 lines

One function moves to a shared export and one call site gains four lines; the rest of the diff is the test helper and the byte-compare to pixel-compare swap in existing tests.

@github-actions

github-actions Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 271 (base branch 271), smooth 131 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

Unstable (1)

  • seqplainkeys-none-center-r0-root-z100: tracking 0.06, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo false, teleport true / tracking 0.06, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true / tracking 0.06, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVE · 0 blocker · 0 important · 1 nit · head df780410

I checked that render's filter chain is unchanged for SDR, tone-mapped HDR and kept HDR. Snapshot now gets the same SDR retag, HDR stays untouched, and with no ffprobe it warns once.

  • nit: the colours may differ assertion in the negative-TS test depends on module-level warnedColourUnknown. Today it is the first test in the file to hit the no-ffprobe path, so it passes. Run after the extracts without ffprobe test (-t filter, reorder, retry), it sees 0 warnings and fails.
CI notes

All required checks are green. Flake gate failed 0/10, and 6/10 on a rerun. The failing test each time was the existing seeks into a stream-copied HEVC cut hitting its 20 s timeout. I ran that test with 8 busy cores: main dc56b3d2 takes ~15 s, 54bc274c ~17 s, df780410 ~15 s. So it was already close to the limit under contention, and this PR doesn't slow it. Comments (comment ratchet, not required) is red on snapshot.ts and videoFrameExtractor.ts.

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST CHANGES — 1 resolved, 0 open, 1 new · df78041 → 6f62a15

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVE — 1 resolved, 0 open, 0 new · 6f62a15 → 328ad7e

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 26, 2026
Merged via the queue into main with commit 37f14fd Oct 26, 2026
79 checks passed
@miguel-heygen
miguel-heygen deleted the fix/snapshot-srgb-retag branch October 26, 2026 18:16
SanHsien added a commit to SanHsien/hyperframes that referenced this pull request Oct 26, 2026
Brings in 10 upstream commits (ad4d3fa..15367a9): heygen-com#6425, heygen-com#6427, heygen-com#6429,
heygen-com#6431, heygen-com#6436, heygen-com#6432, heygen-com#6453, heygen-com#6457, heygen-com#6450, heygen-com#6451.

One conflict, packages/core/src/compiler/compositionScoping.ts (heygen-com#6436). This
fork passes authored composition code to Function() as a JSON string literal,
so upstream's markers around the raw source would sit outside the text its
page-side scan reads and the fix would silently not apply: the wrapper's own
__timelines use would make every composition look able to register a timeline.
The markers go inside the literal instead, begin marker on the first source
line so error line numbers do not shift. The pageBuild goldens change only by
those two marker comments. A new engine test runs upstream's scan on this
fork's real wrapper output and fails without the markers.

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
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.

snapshot: <video> frames come out ~4% darker than the render on Chrome 152 (snapshot's frame PNGs keep the BT.709 cICP tag)

1 participant