fix: render parity for transparent looped videos#478
Conversation
0d2ce8a to
8a130e4
Compare
8a130e4 to
3118408
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Verdict: approve — two independent WYSIWYG gaps, both reproduced, both cleanly fixed
Local verification
Looped-duration clamp bug reproduced on main (e8c43f08). Cherry-picked packages/core/src/compiler/htmlCompiler.test.ts onto main and ran it via vitest:
FAIL src/compiler/htmlCompiler.test.ts > compileHtml > preserves explicit looped media durations that exceed source duration
AssertionError: expected '<video ...>' to contain 'data-duration="4"'
Expected: "data-duration=\"4\""
Received: "<video id=\"hero\" src=\"hero.webm\" data-start=\"0\" data-duration=\"3.125\"
data-end=\"3.125\" loop data-has-audio=\"true\">"
Compiler clamped data-duration="4" → "3.125" because the source file is 3.125s and the non-looped clamp doesn't know about loop semantics. That's the exact failure shape driving the blank-polygon/glow at ~0:03 in the Comfy repro: after t=3.125s in the render, frame lookup treats the video as inactive, the injected-frame path hides the native <video>, and the rendered MP4 diverges from Studio preview (which loops natively).
Fix verified on 31184087 via vitest:
bun run test -- src/compiler/timingCompiler.test.ts src/compiler/htmlCompiler.test.ts→ 16/16 passbun run test -- videoFrameExtractor ffprobe→ 28/28 pass (including the 5 newparseVideoElements/FrameLookupTable/extractMediaMetadatacases)bun run --filter @hyperframes/core typecheck→ clean
CI on 31184087: 27/28 check runs green; 1 regression-shard (styles-e) still in-flight at review time. Includes Tests on windows-latest + Render on windows-latest green, full perf suite green, all other regression-shards green.
Staff review — two distinct gaps, correctly decomposed
Gap 1: alpha stripping on frame extraction. VP9-alpha videos (pix_fmt=yuva420p or tags.alpha_mode=1) were being decoded with ffmpeg's default VP9 decoder, which drops alpha. Injected PNG/JPG frames then had the wrong colors, producing the polygon/glow artifact. Fix in three places:
extractMediaMetadata(inffprobe.ts) now reportshasAlphabased on pixel format (yuva*/rgba/argb/bgra/gbrap/gray*a) OR thealpha_modestream tag. The regex(^|[^a-z])yuva|rgba|argb|bgra|gbrap|gray[a-z0-9]*acorrectly anchorsyuvaagainst word boundaries soyuv420pdoesn't false-positive.extractVideoFramesRangeauto-selects PNG output for alpha streams and prepends-c:v libvpx-vp9to ffmpeg args when the codec is VP9+alpha. TheresolveFrameFormat(metadata, options.format)helper defers to an explicit caller-specified format, preserving backward compat — only auto-switches when the caller omits format.- Snapshot path (
packages/cli/src/commands/snapshot.ts) has a matching path viashouldUseVp9AlphaDecoder+alphaDecoderCache. Cache is per-render which is the right scope (file path stable, metadata stable for render lifetime).
One subtle thing worth flagging: the hasAlpha regex uses gbrap (Green-Blue-Red-Alpha Planar). That covers gbrap10le, gbrap16le, etc. — ffmpeg's naming is consistent enough that the regex holds. ✓
Gap 2: looped-duration clamp. The non-looped clamp (data-duration clamped to source duration) is correct for one-shot clips but wrong for looped ones. data-duration="4" on a 3.125s loop means "play one full cycle + 0.875s of the next." Fix propagates a single loop: boolean flag through the pipeline:
timingCompiler.ts#extractResolvedMediareadsloopfrom the tag →ResolvedMediaElement.loophtmlCompiler.ts#compileHtmlskips the duration clamp whenel.loop === truehtmlCompiler.ts(producer) filters the clamp pre-resolution similarly (!!el.src && !el.loop)videoFrameExtractor.tsparsesloopfrom<video>tags →VideoElement.loopdiscoverMediaFromBrowserreadsloopviahtmlEl.hasAttribute("loop")→BrowserMediaElement.looprenderOrchestratormergesloop: truewhen browser discovers itFrameLookupTable.addVideocarriesloop, andgetFrameAtTime+getActiveFramePayloadswraplocalTime %= loopDurationwhenloop && localTime >= loopDuration
That's full pipeline coverage — static parse → compiler → browser discovery → render frame lookup. Preview side doesn't need changes because the browser's native <video loop> handles it; the bug was strictly that render was diverging from preview.
hasAttr regex change is subtle but correct. The old ${attr}=["'] required =" or =', so boolean attrs like loop (which have no = in HTML) were always "not present." New \\s${attr}(?:\\s|=|>|/) correctly matches <video ... loop>, <video loop/>, <video loop>, <video loop="...">, etc.
I traced false-positive scenarios:
class="loop": char beforeloopis", not\s→ no match ✓data-hf-loop-thing: char beforeloopis-, not\s→ no match ✓looper: char afterloopisr, not\s|=|>|/→ no match ✓<videoloop>: no space beforeloop→ no match ✓
Note on generality: this regex change affects ALL hasAttr callers, not just the new loop one. If there were existing call sites using hasAttr with boolean-style attr names that previously returned false (e.g., checking hasAttr(tag, "muted") on <video muted>), they now return true. I didn't find any in the visible changes, but it's a behavior delta worth knowing if you ever hit a weird edge case post-merge.
Loop math correctness. loopDuration = max(0, metadata.durationSeconds - mediaStart) accounts for data-media-start trim. localTime %= loopDuration wraps. The defensive if (loop && frameIndex >= totalFrames) → last-frame fallback handles the floating-point edge where (localTime %= loopDuration) lands just above the last frame index due to rounding. Correct on both paths. Tests cover wrap (0.5/1.5/4.5 → same frame 15) and non-wrap (past-end on non-looped → undefined).
CLI fail-fast on missing Studio bundle. Orthogonal DX fix bundled in — resolveStudioBundle() now returns {available, checkedPaths} and runEmbeddedMode exits early with a helpful error listing the paths it checked. Matches the "fail before opening a broken browser page" goal in the PR summary. Clean.
Pattern observation across the 4 recent Miguel PRs (hf#474, #476, #477, #478)
Home asked me to flag any common root. There is one, and it's worth naming:
All four PRs close semantic-awareness gaps between the authoring contract and the subsystems that implement it:
- #474 (lint) — lint engine didn't know about GSAP scene-boundary-exit determinism.
- #476 (compiler) — compiler's media scraper didn't know that sub-compositions use
<template>wrappers (LinkeDOM DocumentFragment semantics). - #477 (runtime) — runtime media sync didn't know about nested composition-inherited timing (no direct
data-start). - #478 (compiler + render) — compiler didn't know loop semantics preclude source-duration clamping; render didn't know alpha codecs need
-c:v libvpx-vp9+ PNG output.
Each fix adds one new bit of semantic awareness to a subsystem that was already close to correct. The failure mode is always "subsystem silently drops information" — zero-length media arrays (#476), orphan media elements (#477), clamped durations (#478), no-op lint on a real bug (#474).
The meta-question worth raising for the next review pass: is there a way to systematically ensure that subsystems maintain full awareness of the authoring contract? Options:
- A shared authoring-contract "schema" (the set of supported attributes and their semantics) that each subsystem declares conformance against. Each feature launch requires updating the schema and getting conformance from compiler/runtime/render/lint.
- A "WYSIWYG contract test" fixture suite that renders a representative set of compositions through both preview and render, diffs the output visually/programmatically, and catches divergence. Per the PR body:
hyperframes lint/validatedon't catch these because they're syntactic, not semantic-parity. Ahyperframes wysiwyg-check(or similar) that exercisestemplate-wrapped comps, nested comps, looped videos, alpha videos, GSAP exit patterns in one sweep would have caught #476/#477/#478 in one go.
Not a thing to block this PR on. Just flagging because the pattern is consistent enough across 4 PRs in 24 hours that it's a signal worth surfacing.
Non-blocking observations on #478 specifically
-
Format argument honored even when wrong for alpha.
resolveFrameFormat(metadata, options.format)prefers the caller-specified format. So if any caller explicitly passesoptions.format = "jpg"for an alpha video, alpha silently gets stripped. I grepped the visible diffs and didn't find such a caller, but this is a foot-gun worth a comment at the helper definition ("passundefinedto auto-detect; explicit format disables alpha auto-promotion"). -
Cache key churn. The cache key includes
format. Existing alpha-video cache entries keyed as JPG will become orphaned when the new logic promotes them to PNG on the same (path, mtime, size). No correctness issue (new entries are fresh), just cache-bloat. Ahfcache-v3-bump would force a fresh start; not necessary. -
extractVideoFrameToBuffersignature. Added a positional booleanuseVp9AlphaDecoder = falseas the third argument. Positional booleans are a minor smell when they compose — if a future fourth argument shows up,foo(a, b, true, maybe_something)gets hard to read. Options: wrap in an options object, or name it explicitly. Low priority. -
Loop-wrap test coverage — missing source-longer-than-display case. The existing
FrameLookupTabletest hastotalFrames=30, fps=30, end=5→loopDuration=1s, display window of 5s — so we loop 5× in the window. A mirror test for "source longer than display window" would verify that the wrap math stays within[0, loopDuration)even whenlocalTime / loopDuration < 1(no wrap needed). Not blocking. -
Snapshot loop-wrap duplication.
snapshot.tsimplements its ownrelTimewrap formula separate fromgetFrameAtTime's. Both should produce the same answer, but having two copies of the same math means bugs can diverge. Minor maintenance concern.
Ship once the last regression-shard settles.
Review by Rames Jusso
Summary
<video loop>semantics through static parsing, compiler duration resolution, browser media discovery, and render frame lookupWhy
The Studio preview and rendered MP4 could disagree for timed transparent looped videos. The Comfy funding composition exposed two separate parity bugs: render-injected frames needed alpha-preserving PNG extraction, and the compiler was clamping a looped
data-duration="4"video down to the 3.125s source duration. After the first source cycle, render lookup treated the video as inactive, hid the native video, and produced the blank polygon/glow the user saw around the rounded0:03mark.hyperframes lintandhyperframes validatedid not catch this because they check syntax/load/console/accessibility, not preview-vs-render visual parity. This PR adds regression coverage for the compiler loop-duration path and frame lookup path.Verification
bun run --filter @hyperframes/core test -- src/compiler/timingCompiler.test.ts src/compiler/htmlCompiler.test.tsbun test packages/producer/src/services/htmlCompiler.test.tsbun run --filter @hyperframes/engine test -- videoFrameExtractor ffprobebun run --filter @hyperframes/core typecheckbun run --filter @hyperframes/engine typecheckbun run --filter @hyperframes/producer typecheckbun run --filter @hyperframes/cli typecheckbun run lintbun run format:check ...on touched filesnode packages/cli/dist/cli.js validate-> no console errors, 44 text elements pass WCAG AA/tmp/comfy-render-compare/fixed6-comfy.mp4, 1920x1080, 30fps, 21.8s, 654 frames/tmp/comfy-render-compare/fixed6-window-contact.png/tmp/comfy-render-compare/probe-capture-fixed/captured/frame_000112.jpg/tmp/comfy-render-compare/agent-browser-studio-3_7-fixed.png/tmp/comfy-render-compare/agent-browser-wysiwyg-3s-fixed.webmNote:
bun run --filter @hyperframes/cli dev -- validateis blocked in source mode by the existingcontrast-audit.browser.jsdefault-export loader issue; packagednode packages/cli/dist/cli.js validatepasses for this project.