build(lfs): track tests/*/src/*.png via Git LFS#376
build(lfs): track tests/*/src/*.png via Git LFS#376vanceingalls wants to merge 6 commits intovance/regen-window-ffrom
Conversation
5a9bccf to
becf52d
Compare
05d1fdf to
6d042d3
Compare
becf52d to
f4e6cc1
Compare
6d042d3 to
871f1ad
Compare
871f1ad to
bc32a77
Compare
5bfb45e to
f6b35f6
Compare
bc32a77 to
c20707c
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
LFS track for tests/*/src/*.png is the right call — the hdr-regression fixture's hdr-photo-pq.png + hdr-clip.mp4 are chunky binaries that would bloat the repo's .pack files without LFS. Updating every checkout step in both ci.yml and windows-render.yml to use lfs: true is correct; the only gotcha is that lfs: true on actions/checkout@v4 requires the LFS data to be on the remote (not just the pointer files), which Graphite should be handling but worth confirming once post-merge that the suites still find their source PNGs.
Two things I'd verify:
-
Existing tracked PNGs are migrated.
git lfs migrate import --include="packages/producer/tests/*/src/*.png" --everything— or however your LFS migration flow works. If any existing PNG was committed to the repo before this.gitattributeschange, it's still stored as a regular blob andlfs: truewon't help it. Thegit lfs ls-filesoutput before/after should show the count going up, not staying flat. -
Windows runner LFS support.
actions/checkout@v4withlfs: trueworks onwindows-latestbut requires Git LFS to be installed in the runner image. The GitHub-hosted Windows image does include it, but self-hosted runners may not. Worth a one-time CI run to confirm.
The stack-wide Format and Render on windows-latest failures pre-date this PR so they're not caused by the LFS change.
Approved.
— Rames Jusso
dd9b53a to
4d04942
Compare
0d422dd to
0f40c30
Compare
4d04942 to
325343d
Compare
0f40c30 to
1e22254
Compare
325343d to
d3efe6b
Compare
|
Thanks @jrusso1020 — both items are verified: 1. Existing tracked PNGs are migrated. Confirmed via `git lfs ls-files` on the post-merge state — the chunky binaries you flagged are LFS-tracked, not regular blobs: ``` The original commit added them directly via `.gitattributes` + `git add` rather than `git lfs migrate`, but the net effect is the same — the blobs in HEAD are LFS pointers, and 44 fixtures total are now LFS-resident across the repo. 2. Windows runner LFS support. Confirmed working on `windows-latest` — the upstack PRs in the same stack (#375 in particular) show `Render on windows-latest` and `Tests on windows-latest` both passing at ~3m, which means the runner successfully smudged the LFS pointers for `hdr-photo-pq.png` / `hdr-clip.mp4` and used the actual binaries to build the renders. GitHub-hosted Windows runner ships with LFS as expected. Both verification gates green. The `Format` / `Render on windows-latest` failures you noted are unrelated (style nits / pre-existing flake), tracked separately. |
d3efe6b to
99880f6
Compare
5d24059 to
64182b9
Compare
320bee7 to
9e46264
Compare
73e4e69 to
a0f4d82
Compare
9e46264 to
66c7d0c
Compare
a0f4d82 to
215ed11
Compare
cc59ffc to
b8e8f36
Compare
90d7be8 to
0482376
Compare
b8e8f36 to
14816cd
Compare
… transfer
Chunk 3 of HDR follow-ups. Three independent fixes that share a common
thread: HDR config flowing correctly from the EngineConfig down through
the encoders.
3A. chunkEncoder respects options.hdr (BT.2020 + mastering metadata)
Previously buildEncoderArgs hard-coded BT.709 color tags and the
bt709 VUI block in -x265-params, even when callers passed an HDR
EncoderOptions. Today this is harmless because renderOrchestrator
routes native-HDR content to streamingEncoder and only feeds
chunkEncoder sRGB Chrome screenshots — but the contract was a lie.
Now: when options.hdr is set, the libx265 software path emits
bt2020nc + the matching transfer (smpte2084 for PQ,
arib-std-b67 for HLG) at the codec level *and* embeds
master-display + max-cll SEI in -x265-params via
getHdrEncoderColorParams. libx264 still tags BT.709 inside
-x264-params (libx264 has no HDR support) but the codec-level color
flags flip so the container describes pixels truthfully. GPU
H.265 (nvenc/videotoolbox/qsv/vaapi) gets the BT.2020 tags but no
-x265-params block, so static mastering metadata is omitted —
acceptable for previews, not HDR-aware delivery.
3B. convertSdrToHdr accepts a target transfer
videoFrameExtractor.convertSdrToHdr was hard-coded to
transfer=arib-std-b67 (HLG) regardless of the surrounding
composition's dominant transfer. extractAllVideoFrames now calls
analyzeCompositionHdr first, then passes the dominant transfer
("pq" or "hlg") into convertSdrToHdr so an SDR clip mixed into a PQ
timeline gets converted with smpte2084, not arib-std-b67.
3C. EngineConfig.hdr type matches its declared shape
The IIFE for the hdr field returned undefined when
PRODUCER_HDR_TRANSFER wasn't "hlg" or "pq", but the field is typed
as { transfer: HdrTransfer } | false. Returning false matches the
type and avoids a downstream undefined check.
Tests
- chunkEncoder.test.ts: replaced the previous "HDR options ignored"
assertions with 8 new specs covering BT.2020 + transfer tagging,
master-display/max-cll embedding, libx264 fallback behavior, GPU
H.265 + HDR (tags but no x265-params), and range conversion for
both SDR and HDR CPU paths.
- All 313 engine unit tests pass (5 new HDR specs).
Follow-ups (separate PRs):
- Producer regression suite runs in CI; not exercising HDR-tagged
chunkEncoder yet because no live caller sets options.hdr there.
…d-transfer caller error PR #370 review feedback (jrusso1020): - chunkEncoder: when codec=h264 and hdr is set, log a warning and strip hdr instead of emitting a half-HDR file (BT.2020 container tags + BT.709 VUI inside the bitstream). libx264 has no HDR support; the only honest output is SDR/BT.709. Caller is told to use codec=h265. - videoFrameExtractor: comment at the convertSdrToHdr call site clarifying that dominantTransfer is majority-wins; mixing PQ and HLG sources in a single composition is caller-error and the minority transfer's videos will be converted with the wrong curve. Render two compositions if you need both transfers. - docs/guides/hdr.mdx: limitations section now documents (a) H.264 + HDR is rejected at the encoder layer, and (b) GPU H.265 (nvenc, videotoolbox, qsv, vaapi) emits BT.2020 + transfer tags but does NOT embed master-display or max-cll SEI, since ffmpeg won't pass x265-params through hardware encoders. Acceptable for previews, not for HDR10 delivery.
Address jrusso1020's review on PR #371: add a path.win32 test suite that exercises isPathInside on Linux/macOS CI to catch accidental Unix-only assumptions (e.g. only splitting on "/") that would silently regress for Windows users. - isPathInside now accepts an optional `pathModule` (defaults to node:path) so tests can inject path.win32 / path.posix without changing call sites. - New describe block covers equality, direct/deep children, sibling-prefix rejection, traversal escapes, trailing-backslash normalization, and cross-drive rejection.
Migrates the six existing PNG fixtures (1.6 MB combined: hdr-photo-pq.png plus heygen-promo-preview-assets screenshots) onto Git LFS to mirror the existing policy for golden videos and fixture .mp4s. Without this, regression suites that grow PNG fixtures over time would bloat the working-tree history and slow shallow clones. Addresses Chunk 11C in plans/hdr-followups.md.
14816cd to
f3dd388
Compare
0482376 to
b7ce1ea
Compare

Summary
Track
tests/*/src/*.pngvia Git LFS to mirror the existing policy for golden videos and.mp4fixtures.Why
Chunk 11Cofplans/hdr-followups.md. Without this rule, regression suites that grow PNG fixtures over time would bloat the working-tree history and slow shallow clones.What changed
.gitattributes: addtests/*/src/*.pngto the LFS-tracked patterns.hdr-photo-pq.pngplusheygen-promo-preview-assets/screenshots) onto LFS in the same commit so the rule applies retroactively.Test plan
git lfs ls-filesincludes the HDR PNG fixtures after commit.Stack
Chunk 11C of
plans/hdr-followups.md. Independent of all code changes.