feat(lint): HEVC asset support docs + info-level preview-codec finding#2453
feat(lint): HEVC asset support docs + info-level preview-codec finding#2453miguel-heygen wants to merge 4 commits into
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
The cli, engine, and lint packages each carried their own copy of the ffmpeg/ffprobe lookup, annotated fallow-ignore code-duplication, and the copies had drifted: the engine copy handled Windows PATHEXT and executed which/where without a shell but lacked the Homebrew-dirs fallback for GUI-spawned processes; the cli copy had the opposite. One resolver in @hyperframes/parsers (the dependency-graph bottom) now carries the union of both hardenings, and all three packages delegate to it. Every consumer gets strictly more robust resolution; env-override semantics per call site are preserved via configuredMustExist.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 811b7554c6327224d0d23da556fd308448de36a3.
What this does
Consolidation + feature PR — three related but independent pieces:
- Shared FFmpeg/FFprobe binary resolver. New
packages/parsers/src/ffBinaries.ts(147 lines) withfindFfBinary({name, configuredMustExist?})— env override →which/where→ PATH scan (with Windows PATHEXT) → common install-dir fallback (macOS Homebrew paths for GUI/Dock-spawned processes). Behind a new./ff-binariessubpath export. Replaces two near-duplicate implementations:packages/cli/src/browser/ffmpeg.ts(−69) andpackages/engine/src/utils/ffmpegBinaries.ts(−90) now thin re-exports. - Shared asset-resolution helpers. New
packages/lint/src/assetResolution.ts(75 lines) withisRemoteOrInlineUrl/cleanAssetUrl/isWithinProjectRoot/resolveLocalAssetCandidates/resolveExistingLocalAsset/maskNonScannableRanges. Extracted fromproject.tsto break a circular import with the newhevcPreviewLint.ts. - Info-level HEVC preview-codec lint. New
packages/lint/src/hevcPreviewLint.ts(156 lines) — dedup local<video src>refs by resolved path, cap concurrentffprobeat 8, per-probe 4000 ms timeout, emit oneinfo-severity finding naming all HEVC-encoded files. Never escalates beyondinfo, silently no-ops if ffprobe isn't installed, and never probes a file thatmissing_local_assetwould already cover.
Plus: docs updates (rendering + troubleshooting mdx, skills references, SKILL.md), CI-level tests for both new modules, bun.lock bump, and skills-manifest hash refresh.
Net: +797 / −323 — the deletions come from collapsing the two duplicate binary resolvers into one; the additions are mostly the parsers module + the HEVC lint + comprehensive tests (+140 for ffBinaries.test.ts, +168 for project.test.ts HEVC section).
Verification — high-leverage adversarial angles
- Node/browser import boundary is correctly enforced.
ffBinaries.tsimportsnode:child_process/node:fs/node:path.packages/parsers/src/index.ts(the browser-safe root export) does NOT re-export it — same pattern the pre-PR./asset-pathssubpath already uses for Node-only helpers, called out explicitly in the index-file comment ("browser-safe … Node-only asset path helpers live behind the ./asset-paths subpath"). All three real callers (packages/cli/src/browser/ffmpeg.ts,packages/engine/src/utils/ffmpegBinaries.ts,packages/lint/src/hevcPreviewLint.ts) import via the explicit@hyperframes/parsers/ff-binariessubpath. No browser bundle can accidentally pull inexecFileSyncthrough the parsers root. ✓ (grep-verified acrosspackages/.) - Shared-resolver union is correct across CLI/engine. Both packages previously had near-identical resolvers; the new central version composes the union of both: env override (present in both),
which/whereprimary lookup (both), PATH scan with PATHEXT/x-bit checks (both),chooseBestPathCandidatepreferring.exeover.cmd/.baton Windows (originally engine-side, now applied everywhere), andfindInCommonDirsfor/opt/homebrew/binetc. (originally CLI-side, now applied everywhere). The lookup is cached per binary per process; env override is re-read every call so tests / dynamic reconfiguration work.clearFfBinaryLookupCache()is exposed for test hooks (used byffBinaries.test.ts). ✓ - Probe concurrency + timeout cleanup is safe.
execFile(file, args, { timeout: 4000 })— Node's built-in child-process timeout SIGKILLs the child and reports via the callback; no manual.kill()needed.probeIsHevccatches all errors and resolves tofalse, so a timed-out probe just misclassifies as "not HEVC" rather than surfacing as a lint failure. Concurrency is bounded via a sharednextIndexcounter acrossPROBE_CONCURRENCYworker Promises — safe under single-threaded JS becauseconst index = nextIndex++executes synchronously before the nextawait, so each worker claims a distinct index atomically. Per-run de-dup happens ONE level up incollectLocalVideoCandidates: same resolved path across multiple<video>refs collapses into one Map entry, so each file is probed exactly once. ✓ - HEVC-only detection is the correct invariant. The narrow-scoping to HEVC (and not VP9/AV1/DivX etc.) matches the actual browser-support landscape: VP9 has been mainline across Chrome/Firefox/Safari for years, AV1 support is broad enough by 2026, but HEVC is the codec whose browser support was historically absent (Chrome only added it recently, Firefox partial, Safari native). Info-level severity + fixHint pointing at the media-use skill's H.264 proxy path is the right ergonomics — this is a preview compatibility warning, not a render blocker (the render pipeline pre-decodes with FFmpeg either way, never the browser decoder). ✓
- Detection uses
codec_name === "hevc", notcodec_tag_string. ffprobe normalizes codec names to lowercase strings regardless of the container fourcc; the test at project.test.ts:344 pins thathev1andhvc1container tags both surface via the samecodec_namevalue, so both flag correctly. ✓ - Missing-file avoidance.
collectLocalVideoCandidatescallsresolveExistingLocalAssetbefore ever adding to the candidate map (line 93 —if (!resolvedAsset) continue;), so a missing video file never triggers a probe —missing_local_assetalready covers those, and the HEVC finding never doubles up. Test pins this at project.test.ts:376. - CI signal. Required checks are green at this SHA (per statusCheckRollup: skills tests, manifest sync, preview-regression, regression, player-perf, format, etc.); the remaining
IN_PROGRESSitems are the CodeQL/Preflight matrix that always runs post-push. No blocking failures visible.
Concerns — most-severe first
-
🟡
<source>HEVC gap.packages/lint/src/hevcPreviewLint.ts:81—videoSrcRe = /<video\b[^>]*\bsrc\s*=\s*["']([^"']+)["'][^>]*>/gi— matches ONLY<video src="...">. Butpackages/lint/src/project.ts:311— the siblinglocalAssetSrcRe = /<(video|img|source)\b.../gi— treats<source>as first-class for the missing-asset check. So a user writing<video><source src="clip.hevc.mp4"></video>(multi-codec fallback pattern) gets a "missing file" error if the file's missing but NO HEVC preview warning if it exists and is HEVC. Asymmetric — either both regexes should cover<source>or neither should. Suggest widening the HEVC regex to<(video|source)\b[^>]*\bsrc\s*=...and adding a<source>-pattern test alongside the existing<video src>cases. Non-blocking (HF authoring convention leans direct-src, so the class is small), but worth closing for consistency. -
🟡
PROBE_CONCURRENCY = 8andPROBE_TIMEOUT_MS = 4000are non-overridable. Constants atpackages/lint/src/hevcPreviewLint.ts:20-22. Worst-case wall time on a project with N videos isN/8 × 4s; a 200-video composition sits through 100 seconds offfprobeon every lint run. For interactive lint (studio save-hook, CLIcheck), that's a felt latency. Options: expose an env override (HYPERFRAMES_HEVC_PROBE_CONCURRENCY,HYPERFRAMES_SKIP_HEVC_PROBE), or short-circuit the probe when candidate count exceeds a ceiling (e.g.> 50→ skip with a diagnostic). Not blocking; info-only lint is already forgiving on this axis. -
🟢
packages/parsers/src/ffBinaries.ts:23-25candidateFileNameusestoLowerCase()on the full candidate for the fallback comparison. Fine on Windows (paths are case-insensitive) but on Linux/macOS,/opt/HOMEBREW/bin/FFMPEG(odd, but possible) would compare asffmpegon the fallback branch.existsSyncinfindInCommonDirswould fail for real anyway, so this is theoretical — but ifwhichreturned a strange-cased hit, it'd still be selected. Nit / harmless. -
🟢 Test file
packages/parsers/src/ffBinaries.test.ts:53-54mock returns\r\n-delimited output for the Windowswherecase, but not\n-delimited for the Unixwhichcase in an explicit test. Theoutput.split(/\r?\n/)regex handles both, but only one branch is exercised directly. Adjacent tests cover Unix flow viaexecFileSyncthrowing (falls to PATH scan), so the missing case is functionally covered by scan-fallback rather than by directwhichoutput parsing on Unix. Nit / future test-coverage tightening. -
isRemoteOrInlineUrldoesn't coverfile:URIs.packages/lint/src/assetResolution.ts:13— regex is^(https?:|data:|blob:|\/\/|#). A<video src="file:///abs/path.mp4">would fall through toresolveExistingLocalAsset(projectDir, "file:///abs/path.mp4")— the path resolution wouldn't find anything underprojectDir, so the finding is silently dropped (resolvedAssetis null →continue). No crash. Realistic exposure is near zero (nobody usesfile:in HF authoring). Note-only.
Verdict framing
Clean consolidation + a well-designed info-level lint. Node/browser boundary is respected, the shared resolver correctly unions the two previous implementations without regressing platform coverage, and probe concurrency/timeout semantics are safe under the built-in execFile cleanup. The <source>-tag asymmetry is the one place the class-level fix isn't quite class-complete — worth closing in this PR or immediately after. Everything else is nit / documentation. LGTM from my side.
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed at 811b7554c6327224d0d23da556fd308448de36a3.
Position: RIGHT to ship, with two important follow-ups worth landing in-scope if quick. Clean extraction of a resolver that lived duplicated across cli and engine; new lint rule is bounded, info-only, and defends against ffprobe absence. Additive to @james-russo-rames-d-jusso — I concur on the load-bearing verifications (Node/browser boundary, resolver union, probe cleanup, HEVC-only correctness) and add angles he didn't cover, including two code-level items surfaced by an independent adversarial pass.
Strengths
packages/lint/src/hevcPreviewLint.ts:46-63—probeIsHevccatches every failure mode (ffprobe missing, timeout, non-video file, unparsable output) and resolves to "not HEVC" rather than throwing. Combined with theconfiguredMustExist: truegate at line 119, the rule is genuinely unable to fail lint even under adversarial input.packages/parsers/src/ffBinaries.ts:99-104— extraction switches CLI's oldexecSync("which ffmpeg")(shell-interpolated string) toexecFileSync("which", ["ffmpeg"])(no shell). Not exploitable in practice (binary name is a literal"ffmpeg" | "ffprobe"), but the tightening is worth landing regardless.packages/lint/src/project.test.ts:246-403— covershvc1positive,hev1positive (canonical fourcc-mismatch case Miguel called out in the PR body), H.264 negative, ffprobe-missing skip, ffprobe-timeout skip, dedup-across-refs, missing-file-never-probed. Six clean branches, no aspirational-test smell.
Additive — code-level gaps
- important.
-select_streams v:0false-negative on containers with cover-art streams —packages/lint/src/hevcPreviewLint.ts:52. ffmpeg'sv:0selects the first stream of type video in container order, not the "real" video track. A container with anattached_pic/cover-art stream (mjpeg thumbnail, common in MOV/MP4 authored by NLE/consumer cameras) ordered before the main track will silently miss the HEVC stream — no info finding, user still hits the black-frame preview bug the rule exists to catch. No test exercises multi-video-stream containers. Fix: probe without-select_streams(or addcodec_type+dispositionto-show_entries) and checkhasHevcStreamacross all non-attached_picvideo streams. - important. Silent behavior change in engine — undocumented in PR description. Diffing
packages/engine/src/utils/ffmpegBinaries.tsagainst pre-PR: the old engine resolver had NOCOMMON_BIN_DIRSfallback (only CLI had it). The extracted resolver gives the engine the same/opt/homebrew/bin//usr/local/binfallback. Likely desirable — GUI-spawned engine processes now find Homebrew ffmpeg on macOS — but it's a real behavior change on a different package not called out in the PR description. Worth a line in the PR body under "Not covered" or "Also does". No engine-side test exercises this path (relies on shared package coverage), which is acceptable but worth noting. - nit → important.
findInCommonDirsdoesn't check executability —packages/parsers/src/ffBinaries.ts:87-91. UsesexistsSync, notisExecutablePathCandidate(which is defined two functions above in the same file and checksX_OK). Could return a directory namedffmpegor a non-executable file with that name;execFilewould then fail with a genericEACCES/EISDIR, not the friendly "install ffmpeg" hint the CLI wrapper promises. Trivial fix: reuseisExecutablePathCandidatein the loop.
Additive — coverage gaps
- important-follow-up. Multi-file positive test missing. No test for 2 distinct HEVC files → one finding listing both via
unique.join(", ")(hevcPreviewLint.ts:146). ThefixHintsingular-vs-plural branch (hevcPreviewLint.ts:151-153) is entirely untested on the plural path — a small addition toproject.test.tsHEVC block would close this. - nit. Malformed / partial-JSON probe untested.
probeIsHevc(hevcPreviewLint.ts:59) callsJSON.parse(stdout)inside try/catch; behavior is correct (swallow → false), but a truncated-stdout ffprobe crash (SIGKILL mid-write) is a realistic real-world case worth a one-line regression test.
Additive — docs / UX
docs/examples.mdx:227— stale "incompatible codec" language. The line reads "If the video uses an incompatible codec, it will be automatically transcoded to H.264 MP4 if FFmpeg is available." This is init-time transcoding, distinct from the render-path claim being added — but after this PR, HEVC is not an incompatible codec, and a user reading both pages back-to-back gets a contradiction. nit → important. Suggested: narrow the sentence or drop it.- Docs list VP9-in-MP4 and ProRes 4444 as supported inputs without any preview caveat.
docs/guides/rendering.mdxnew "Input Video Codecs" section explicitly lists VP8/VP9 and ProRes 4444 alongside HEVC as supported render inputs, but only HEVC gets the preview caveat. ProRes essentially never plays in a browser<video>tag; VP9-in-MP4 is a known Safari preview failure mode — same "renders fine, preview breaks" symptom. Miguel already scoped the LINT to HEVC-only in the PR body ("no ProRes/AV1 detection"), which is defensible in practice (HEVC-in-MP4 is the common storage case). But the docs surface reads asymmetric. nit → follow-up. Two lines noting "same preview caveat applies to ProRes-in-MOV and VP9-in-MP4 on Safari" would close it. - Info-level findings hidden by default —
packages/cli/src/commands/lint.ts:56filtersseverity !== "info"unless--verbose. Correct default for CI. But it means the discovery path for a user hitting "preview shows black" is: (1) find the troubleshooting doc, (2) learnhevc_preview_codecexists, (3) re-runlint --verbose. Miguel's PR body already flags the runtime-hint gap inpreview/play— reinforcing here that surfacingDEMUXER_ERROR_NO_SUPPORTED_STREAMSin preview would collapse discovery from three steps to one. follow-up. - Resolver contract split — three callers, three semantics.
packages/cli/src/browser/ffmpeg.ts:9,13usesconfiguredMustExist: true(surface install hint).packages/engine/src/utils/ffmpegBinaries.ts:9,13uses defaultfalse(spawn error names the configured path).packages/lint/src/hevcPreviewLint.ts:119usestrue(silent skip). Lint takes CLI's semantic; a user whoseHYPERFRAMES_FFPROBE_PATHpoints at a stale path silently loses the HEVC lint even when a real ffprobe is on PATH. Design-consistent with lint's "never fail" contract; consider renaming the option (strict/treatMissingConfiguredAsUnfound) since "must exist" reads like "assert existence" but semantically means "downgrade to not-found." nit.
+1 with @james-russo-rames-d-jusso on
<source>-tag asymmetry —hevcPreviewLint.ts:81regex covers<video src>only; siblingmissing_local_assetatproject.ts:311also covers<source>. Widening the HEVC regex is a one-line fix.PROBE_CONCURRENCY = 8/PROBE_TIMEOUT_MS = 4000non-overridable — his framing about interactive-lint latency stands; worst case (1000 videos on flaky storage) is ~500s silent lint hang for an info-only finding. Env override + wall-clock budget both defensible.file:URI gap inisRemoteOrInlineUrl— near-zero realistic exposure, note-only.
CI + freshness
Head SHA 811b7554c6327224d0d23da556fd308448de36a3, mergeable_state: blocked — the two failed entries in gh pr checks are conclusion: cancelled from a superseded push; current run's required checks (CLI smoke, Build, Render on windows-latest, Test, Test: runtime contract, Tests on windows-latest, Typecheck, Semantic PR title, regression) are all green. Blocked is reviewer-gate only.
Verdict: APPROVE.
Reasoning: The extraction is byte-consistent with the two pre-existing implementations plus a strict superset of platform coverage; new lint rule is guarded, tested, and info-only. Rames covered the code-level adversarial angles at the concurrency/import-boundary/regex layer; the additions here (-select_streams v:0 false-negative on cover-art containers, silent engine behavior change, findInCommonDirs executability, multi-file test gap) are worth landing in-scope if quick, but none block a v1 that Miguel's own PR body already scopes tightly.
— Via
What
hvc1/hev1) are supported render inputs: new "Input Video Codecs" section in the rendering guide, a troubleshooting entry for "plays black in preview but renders fine", and one-line notes in thehyperframes-coreandmedia-useskills.hevc_preview_codeclint finding when a composition references an HEVC video, pointing at the preview-only caveat and the H.264 proxy workaround. Info severity only; it can never faillintorcheck.packages/lint/src/project.tsintoassetResolution.ts(byte-identical moves) so the new rule can reuse them.Why
Users with HEVC-reencoded asset libraries assume HyperFrames cannot use them (a community request even asked for a patched Chromium build with HEVC support). In reality the render pipeline pre-decodes every input video with FFmpeg and injects frames at capture time, so the capture browser's codec support is irrelevant to rendered pixels; HEVC inputs already render correctly. The only real limitation is live preview/player playback in browsers without HEVC decode. Nothing in the docs or tooling said any of this, so the capability was invisible.
A browser fork is explicitly a non-goal: it would be redundant for rendering and a large standing build/maintenance cost.
How
packages/lint/src/hevcPreviewLint.ts: best-effort ffprobe probe, resolved through a new shared@hyperframes/parsers/ff-binariesmodule. The cli and engine packages each carried a drifted copy of this lookup (annotated fallow-ignore); both now delegate to the shared resolver, which is the union of their hardenings (Windows PATHEXT scan + shell-free which/where from engine, Homebrew-dirs fallback for GUI-spawned processes from cli). parsers is the dependency-graph bottom, so no cycle. ffprobe missing, erroring, or timing out silently skips: no finding, no failure. One probe per unique resolved file per run, bounded to 8 concurrent ffprobe processes. Detection is by ffprobecodec_name === "hevc", so bothhvc1andhev1fourccs fire. Node-only; the browser build's import graph is untouched.packages/lint/src/project.tswires the rule intolintProjectand now sits under the 600-line cap after the extraction.Test plan
packages/lint/src/project.test.ts(HEVC fires once with info severity,hev1fires, H.264 does not fire, ffprobe unavailable/erroring skips silently, missing files are never probed, duplicate references probe once). Fullpackages/lintsuite: 370/370 green;packages/clilint-consumer tests: 75/75 green; tsc, oxlint, oxfmt, fallow gate clean.chrome-headless-shell152.0.7928.2 has no HEVC support (DEMUXER_ERROR_NO_SUPPORTED_STREAMS), which is why the docs route preview fallout to H.264 proxies rather than a browser change.Not covered: no runtime hint in
preview/playwhen a<video>element fails on an undecodable codec (docs + lint only for now); the lint rule is HEVC-only (no ProRes/AV1 detection); Lambda's bundled ffmpeg build was not separately exercised.Post-Deploy Monitoring & Validation
No additional operational monitoring required: the change is docs plus a local, info-level lint finding with no render-path or service impact.