Skip to content

fix(engine): merge colour per field, detect alpha correctly, defer PNG parse - #2913

Merged
vanceingalls merged 1 commit into
mainfrom
ffprobe-2-metadata
Aug 1, 2026
Merged

fix(engine): merge colour per field, detect alpha correctly, defer PNG parse#2913
vanceingalls merged 1 commit into
mainfrom
ffprobe-2-metadata

Conversation

@vanceingalls

Copy link
Copy Markdown
Collaborator

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

const colorSpace = ffprobeColorSpace ?? stillImageMeta?.colorSpace ?? null;

ffprobeColorSpace is non-null if any of color_transfer / color_primaries / color_space is non-empty — and ffprobe 8.1.1 emits color_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 gbr but does not decode cICP — an HDR PQ PNG resolves colorTransfer: "", 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]) guards yuva and is decorative for everything after it. Against the ffmpeg pix_fmt list, abgr, ya8, ya16be, ya16le and ayuv64le all return false, and gray[a-z0-9]*a matches only gray8a/gray16a — names FFmpeg renamed to ya8/ya16 in 2013, so dead against every modern build.

A ya8 grayscale-plus-alpha PNG reports hasAlpha: false; codecMayHaveAlpha only rescues vp9/vp8/prores, so resolveFrameFormat picks jpg, alpha is flattened, and the overlay renders as an opaque rectangle. Replaced with the start-anchored form studio-server/src/helpers/mediaMetadata.ts:137 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 thrown away

extractStillImageMetadata sat before the first await, so readFileSync plus the CRC walk ran for every file before a single ffprobe was spawned. A caller fanning out over composition.images with Promise.all serialised completely:

12 4K PNGs, probe only 170 ms
12 4K PNGs, with eager parse 2649 ms (15.6×)

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

@miga-heygen miga-heygen 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.

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 when videoStream is 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
miguel-heygen previously approved these changes Jul 31, 2026

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 pixelFormatHasAlpha divergence Miga flagged is a latent bug in studio-server, not an intentional split. Studio-server's own docstring at packages/studio-server/src/helpers/mediaMetadata.ts:133-136 explicitly 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)/ misses ayuv* (packed 4:4:4:4 YUV+alpha, e.g. ayuv64le) that engine now includes. Studio-server's tests at mediaMetadata.test.ts:141+ cover abgr and ya8 but not any ayuv* variant, so the coverage gap isn't behavioural — it's untested + wrong. pixelFormatHasAlpha is called by mediaCodecMap.ts:121 to derive codecFactsFor(codecName, hasAlpha) on uploaded media metadata, so a ProRes 4444 upload that ffmpeg decodes to ayuv64le currently reports hasAlpha: false in 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. But merged.colorSpace still 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 the colorSpace field vs the transfer/primaries fields. Just calling it out.

Otherwise clean. LGTM from my side.

Review by Rames D Jusso

Base automatically changed from ffprobe-1-png to main August 1, 2026 02:49
@vanceingalls
vanceingalls dismissed miguel-heygen’s stale review August 1, 2026 02:49

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>
@vanceingalls
vanceingalls merged commit e794227 into main Aug 1, 2026
51 checks passed
@vanceingalls
vanceingalls deleted the ffprobe-2-metadata branch August 1, 2026 02:57
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.

4 participants