fix(engine): merge colour per field, detect alpha correctly, defer PNG parse - #2913
Conversation
miga-heygen
left a comment
There was a problem hiding this comment.
Stack 2/6: Colour merge, alpha regex, deferred PNG — Review
All three fixes are correct. One SSOT observation.
Per-field colour merge
The old whole-object ?? discarded the PNG cICP result whenever ffprobe returned anything — and ffprobe emits color_space: "gbr" for every PNG, so the cICP fallback never ran when ffprobe was present. Per-field merge is the right fix.
The gate colorTransfer && colorPrimaries && colorSpaceVal ? null : stillImage()?.colorSpace correctly avoids the PNG parse entirely when ffprobe provides all three fields. Clean.
Alpha regex
The old regex's anchor bug is real: /(^|[^a-z])yuva|rgba|argb.../ — | binds looser than concatenation, so (^|[^a-z]) only guarded yuva. The start-anchored replacement is correct and the ya / abgr / ayuv additions cover real ffmpeg pix_fmts.
SSOT observation: pixelFormatHasAlpha divergence
The comment says "matching studio-server's mediaMetadata.ts:137" — but the engine's pattern now includes ayuv that studio-server's doesn't have:
- Engine:
/^(?:yuva|rgba|argb|bgra|abgr|gbrap|ya|ayuv)/i - Studio-server:
/^(?:yuva|rgba|argb|bgra|abgr|gbrap|ya)/
Both packages import @hyperframes/core. Moving the predicate into core would make it single-source — and studio-server would gain the ayuv coverage for free. Not blocking this PR, but this is the kind of divergence that SSOT exists to prevent.
Deferred PNG parse
The lazy ??= memoization is the right pattern — the PNG parse was running synchronously before the first await, blocking the event loop during fan-out. Now it only fires when ffprobe fails or reports incomplete color.
Verified
- Per-field merge: cicp fallback only consulted when ffprobe is missing a colour field
- Alpha regex: start-anchored, covers the FFmpeg pix_fmt name space including abgr/ya8/ya16/ayuv64
- 10-format alpha test + 8-format opaque test cover the regex
- Deferred parse:
stillImage()called only in the catch path and whenvideoStreamis absent, never on the happy path - No observable behaviour change for the happy path (ffprobe succeeds, all three colour fields present)
Ships clean.
miguel-heygen
left a comment
There was a problem hiding this comment.
Approved exact head 0d407935d0d297c58cd4d422e7fc66f08978450b relative to stack parent #2912. Per-field colour fallback, alpha detection, and lazy PNG parsing are correct; exact-head CI is terminal green and there are no unresolved threads. Non-blocking: the alpha predicate remains duplicated with studio-server and the null fallback is not memoized. Landing remains dependent on #2912 resolving its Node 22.0/22.1 crc32 compatibility blocker; any rebase/head change needs a fresh exact-head review. No merge performed.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Independent read at 0d407935d, layered on @miga-heygen. Agree with their read of the three fixes — per-field merge is correct, alpha regex anchor bug is real (| binds looser than concatenation, so (^|[^a-z]) guarded only yuva), and the lazy ??= memoization is the right pattern (a Promise.all fan-out over 12 4K PNGs paying 2649ms of pre-await CRC walks is real event-loop starvation).
Two things to add:
Concerns
- The
pixelFormatHasAlphadivergence Miga flagged is a latent bug in studio-server, not an intentional split. Studio-server's own docstring atpackages/studio-server/src/helpers/mediaMetadata.ts:133-136explicitly names "gbrap* (planar GBR+alpha, ProRes 4444 decodes to these), ya* (gray+alpha)" as the target coverage — but the regex/^(?:yuva|rgba|argb|bgra|abgr|gbrap|ya)/missesayuv*(packed 4:4:4:4 YUV+alpha, e.g.ayuv64le) that engine now includes. Studio-server's tests atmediaMetadata.test.ts:141+coverabgrandya8but not anyayuv*variant, so the coverage gap isn't behavioural — it's untested + wrong.pixelFormatHasAlphais called bymediaCodecMap.ts:121to derivecodecFactsFor(codecName, hasAlpha)on uploaded media metadata, so a ProRes 4444 upload that ffmpeg decodes toayuv64lecurrently reportshasAlpha: falsein studio-server. Collapsing to@hyperframes/core(both packages already import it) fixes the latent studio-server bug for free AND closes the drift risk. Follow-up rather than in-scope for this PR — but worth spelling out as more than "SSOT good practice".
Questions
- Merge precedence when ffprobe reports partial + junk colour info. The new per-field merge always prefers ffprobe over cICP when the ffprobe field is truthy. For your driving case (ffprobe emits
color_space="gbr"alongside empty transfer/primaries), this correctly rescues the HDR grade — colorTransfer and colorPrimaries pull from cICP. Butmerged.colorSpacestill ends up"gbr"from ffprobe rather than the cICP-derived value. Is that intentional (ffprobe wins even for the field it's known to lie about on PNGs), or would you rather have colorSpace also fall back to cICP when the other two are missing? Non-blocking either way — depends on how downstream consumes thecolorSpacefield vs the transfer/primaries fields. Just calling it out.
Otherwise clean. LGTM from my side.
— Review by Rames D Jusso
The base branch was changed.
…G parse Three defects in how ffprobe output and the PNG fallback are combined. The cICP fallback was unreachable. `ffprobeColorSpace ?? stillImageMeta ?.colorSpace` discarded the PNG result whenever ffprobe returned ANY colour field — and ffprobe emits color_space "gbr" for every PNG, including a plain rgb24 with no colour metadata. So on the build the parser exists for (reports gbr, does not decode cICP) an HDR PQ PNG resolved colorTransfer "" , isHdrColorSpace() returned false, and the still graded SDR. Now merged per field. hasAlpha's anchor bound to one alternative. In /(^|[^a-z])yuva|rgba|.../ the `|` is looser than concatenation, so (^|[^a-z]) guarded `yuva` and nothing else. The list also omitted abgr, ya8, ya16 and ayuv64, and `gray[a-z0-9]*a` matched only gray8a/gray16a — names FFmpeg renamed to ya8/ya16 in 2013, so dead against modern builds. A ya8 grayscale-plus-alpha PNG reported hasAlpha:false, resolveFrameFormat picked jpg and the overlay flattened to an opaque rectangle. Replaced with the start-anchored form studio-server already uses, extracted as exported pixelFormatHasAlpha so the test asserts the shipped predicate rather than a copy of the pattern. The PNG parse ran eagerly and was discarded. It sat before the first await, so readFileSync plus the CRC walk executed for every file before a single ffprobe was spawned — a caller fanning out over composition.images with Promise.all serialised entirely: 12 4K PNGs took 2649 ms against 170 ms probe-only, 2.5 s of event-loop stall that also blocks Puppeteer IPC. On the happy path the value was then thrown away. Now lazily memoized behind the paths that actually consult it. Tests: 18 pix_fmt cases against the real predicate. Reverting the regex fails 4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0d40793 to
62b96c2
Compare
Stack 2/6, on top of #2912. These are pre-existing bugs, not regressions from #2740 — but they live in the code #2740 touched and one of them makes that commit's fix unreachable.
The cICP fallback never runs when ffprobe is present
ffprobeColorSpaceis non-null if any ofcolor_transfer/color_primaries/color_spaceis non-empty — and ffprobe 8.1.1 emitscolor_space: "gbr"for every PNG, including a plain 2×2 rgb24 with no colour metadata. So the whole-object??discards the PNG result unconditionally.Which means on exactly the build the parser exists for — one that reports
gbrbut does not decode cICP — an HDR PQ PNG resolvescolorTransfer: "",isHdrColorSpace()returns false, and the still grades SDR. Now merged per field.Worth noting: the guard test added in #2740 ("reads HDR PNG cICP metadata when ffprobe color fields are absent") is satisfied entirely by the ffprobe branch on modern builds. The PNG fallback could have been deleted and it would still have passed.
hasAlpha's anchor bound to one alternative
/(^|[^a-z])yuva|rgba|argb|bgra|gbrap|gray[a-z0-9]*a/i|binds looser than concatenation, so(^|[^a-z])guardsyuvaand is decorative for everything after it. Against the ffmpeg pix_fmt list,abgr,ya8,ya16be,ya16leandayuv64leall return false, andgray[a-z0-9]*amatches onlygray8a/gray16a— names FFmpeg renamed toya8/ya16in 2013, so dead against every modern build.A
ya8grayscale-plus-alpha PNG reportshasAlpha: false;codecMayHaveAlphaonly rescues vp9/vp8/prores, soresolveFrameFormatpicks jpg, alpha is flattened, and the overlay renders as an opaque rectangle. Replaced with the start-anchored formstudio-server/src/helpers/mediaMetadata.ts:137already uses.Extracted as exported
pixelFormatHasAlphaso the test asserts the shipped predicate rather than a copy of the pattern.The PNG parse ran eagerly and was thrown away
extractStillImageMetadatasat before the firstawait, soreadFileSyncplus the CRC walk ran for every file before a single ffprobe was spawned. A caller fanning out overcomposition.imageswithPromise.allserialised completely:2.5 s of event-loop stall that also blocks Puppeteer IPC and progress reporting — for a value discarded on every happy path. Now lazily memoized behind the paths that consult it.
Verification
18 pix_fmt cases against the real predicate. Reverting the regex fails 4. Engine suite: 1280 pass.
🤖 Generated with Claude Code