From 0b3dfb3f84dd2bd711a4a2ebfe93822c3969ea1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Tue, 14 Jul 2026 17:12:36 -0400 Subject: [PATCH] fix(render): consolidate duration and timing correctness (#2405) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(producer): pass variables to duration probe * fix(producer): tolerate rounded frame-boundary durations * fix(cli): resolve relative data-start references in composition duration `compositions --json` computed each timed child's start with a bare parseFloat(data-start ?? "0") in parseCompositions (host duration) and parseSubComposition (sub-comp duration). A relative reference like data-start="s1" ("start when clip s1 ends") is not numeric, so parseFloat returned NaN and that clip's contribution to the max-end was silently dropped — a host with two 3s clips (2nd data-start="s1") reported duration 3 instead of 6, breaking compositions/inspect/snapshot for composition-clip relative timing. Resolve relative references the same way the extractor does (parseStartExpression from @hyperframes/core + a findReferenceTargetEl/resolveReferencedStart port, since the engine's referenceResolver isn't a public export across the package boundary). Verified: host duration now 6; 3 tests pass. (Implemented via Codex; verified independently.) --- .../cli/src/commands/compositions.test.ts | 42 ++++++++++- packages/cli/src/commands/compositions.ts | 75 ++++++++++++++++++- .../services/render/stages/probeStage.test.ts | 43 ++++++++++- .../src/services/render/stages/probeStage.ts | 13 +++- 4 files changed, 167 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/commands/compositions.test.ts b/packages/cli/src/commands/compositions.test.ts index 50c4456ff..32c30caa1 100644 --- a/packages/cli/src/commands/compositions.test.ts +++ b/packages/cli/src/commands/compositions.test.ts @@ -1,6 +1,46 @@ +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { describe, expect, it, beforeEach } from "vitest"; import { ensureDOMParser } from "../utils/dom.js"; -import { parseSubComposition } from "./compositions.js"; +import { parseCompositions, parseSubComposition } from "./compositions.js"; + +describe("parseCompositions", () => { + beforeEach(() => { + ensureDOMParser(); + }); + + it("resolves relative sub-composition starts when computing host duration", () => { + const baseDir = mkdtempSync(join(tmpdir(), "hyperframes-compositions-")); + + try { + const compositionsDir = join(baseDir, "compositions"); + mkdirSync(compositionsDir); + + const subCompositionHtml = ` +`; + writeFileSync(join(compositionsDir, "scene.html"), subCompositionHtml); + + const html = ` +
+
+
+
`; + + const host = parseCompositions(html, baseDir).find( + (composition) => composition.id === "host", + ); + + expect(host?.duration).toBe(6); + } finally { + rmSync(baseDir, { recursive: true, force: true }); + } + }); +}); describe("parseSubComposition", () => { beforeEach(() => { diff --git a/packages/cli/src/commands/compositions.ts b/packages/cli/src/commands/compositions.ts index b31645ee5..f962640dc 100644 --- a/packages/cli/src/commands/compositions.ts +++ b/packages/cli/src/commands/compositions.ts @@ -1,4 +1,5 @@ import { defineCommand } from "citty"; +import { parseNumeric, parseStartExpression } from "@hyperframes/core"; import type { Example } from "./_examples.js"; import { existsSync, readFileSync } from "node:fs"; import { resolve, dirname } from "node:path"; @@ -45,12 +46,78 @@ function estimateDurationFromScripts(root: ParentNode): number { return duration; } -function parseCompositions(html: string, baseDir: string): CompositionInfo[] { +function findReferenceTargetEl(doc: Document, refId: string): Element | null { + return doc.getElementById(refId) ?? doc.querySelector(`[data-composition-id="${refId}"]`); +} + +function resolveStart( + doc: Document, + el: Element, + startCache: Map, + visiting: Set, +): number { + const cached = startCache.get(el); + if (cached !== undefined) return cached; + if (visiting.has(el)) return 0; + visiting.add(el); + + try { + const expression = parseStartExpression(el.getAttribute("data-start")); + if (!expression) { + startCache.set(el, 0); + return 0; + } + + if (expression.kind === "absolute") { + const value = Math.max(0, expression.value); + startCache.set(el, value); + return value; + } + + const target = findReferenceTargetEl(doc, expression.refId); + if (!target) { + startCache.set(el, 0); + return 0; + } + + const targetStart = resolveStart(doc, target, startCache, visiting); + const targetDuration = resolveReferencedDuration(doc, target, startCache, visiting); + const resolved = + targetDuration != null && targetDuration > 0 + ? Math.max(0, targetStart + targetDuration + expression.offset) + : Math.max(0, targetStart + expression.offset); + startCache.set(el, resolved); + return resolved; + } finally { + visiting.delete(el); + } +} + +function resolveReferencedDuration( + doc: Document, + el: Element, + startCache: Map, + visiting: Set, +): number | null { + const durationAttr = parseNumeric(el.getAttribute("data-duration")); + if (durationAttr != null && durationAttr > 0) return durationAttr; + const endAttr = parseNumeric(el.getAttribute("data-end")); + if (endAttr != null) { + const start = resolveStart(doc, el, startCache, visiting); + const delta = endAttr - start; + if (Number.isFinite(delta) && delta > 0) return delta; + } + return null; +} + +export function parseCompositions(html: string, baseDir: string): CompositionInfo[] { const parser = new DOMParser(); const doc = parser.parseFromString(html, "text/html"); const compositionDivs = doc.querySelectorAll("[data-composition-id]"); const compositions: CompositionInfo[] = []; + const startCache = new Map(); + const visiting = new Set(); compositionDivs.forEach((div) => { const id = div.getAttribute("data-composition-id") ?? "unknown"; @@ -75,7 +142,7 @@ function parseCompositions(html: string, baseDir: string): CompositionInfo[] { timedChildren.forEach((el) => { elementCount++; - const start = parseFloat(el.getAttribute("data-start") ?? "0"); + const start = resolveStart(doc, el, startCache, visiting); const endAttr = el.getAttribute("data-end"); const durationAttr = el.getAttribute("data-duration"); @@ -141,9 +208,11 @@ export function parseSubComposition( // Also check timed children for max end time if (compDiv) { const timedEls = compDiv.querySelectorAll("[data-start]"); + const startCache = new Map(); + const visiting = new Set(); timedEls.forEach((el) => { elementCount = Math.max(elementCount, timedEls.length); - const start = parseFloat(el.getAttribute("data-start") ?? "0"); + const start = resolveStart(doc, el, startCache, visiting); const endAttr = el.getAttribute("data-end"); const durAttr = el.getAttribute("data-duration"); diff --git a/packages/producer/src/services/render/stages/probeStage.test.ts b/packages/producer/src/services/render/stages/probeStage.test.ts index e84888cf3..10de8c806 100644 --- a/packages/producer/src/services/render/stages/probeStage.test.ts +++ b/packages/producer/src/services/render/stages/probeStage.test.ts @@ -10,6 +10,7 @@ import { // the correct forceScreenshot value (regression for #1236 — probe was launched // in beginframe mode even when lowMemoryMode demanded screenshot capture). const capturedCfgs: unknown[] = []; +const capturedOptions: unknown[] = []; const mockPage = { evaluate: async () => ({ @@ -43,12 +44,13 @@ mock.module("@hyperframes/engine", () => ({ createCaptureSession: async ( _url: string, _dir: string, - _opts: unknown, + opts: unknown, _nullArg: unknown, cfg: unknown, ) => { createSessionCallCount++; capturedCfgs.push(cfg); + capturedOptions.push(opts); if (createSessionError && createSessionCallCount <= createSessionFailUntilAttempt) { throw createSessionError; } @@ -312,6 +314,45 @@ describe("runProbeStage — forceScreenshot threading", () => { }); }); +describe("runProbeStage — render variable threading", () => { + it("passes render variables to the duration-discovery capture session", async () => { + capturedOptions.length = 0; + const { runProbeStage } = await import("./probeStage.js"); + const input = makeProbeInput({ stageForceScreenshot: false }); + input.job.config.variables = { short: true, sceneCount: 2 }; + + await runProbeStage(input); + + expect(capturedOptions[0]).toMatchObject({ + variables: { short: true, sceneCount: 2 }, + }); + }); +}); + +describe("runProbeStage — decimal duration frame count", () => { + it("does not add a frame for a six-decimal duration rounded from an exact frame boundary", async () => { + const { runProbeStage } = await import("./probeStage.js"); + const input = makeProbeInput({}); + input.composition.duration = 32.866667; + input.compiled.staticDuration = 32.866667; + + const result = await runProbeStage(input); + + expect(result.totalFrames).toBe(986); + }); + + it("still ceilings a duration that genuinely extends into the next frame", async () => { + const { runProbeStage } = await import("./probeStage.js"); + const input = makeProbeInput({}); + input.composition.duration = 32.867; + input.compiled.staticDuration = 32.867; + + const result = await runProbeStage(input); + + expect(result.totalFrames).toBe(987); + }); +}); + describe("runProbeStage — transient browser error retry (#1687)", () => { it("retries once on a transient 'Navigating frame was detached' error and succeeds", async () => { resetRetryMocks(); diff --git a/packages/producer/src/services/render/stages/probeStage.ts b/packages/producer/src/services/render/stages/probeStage.ts index b42a6cd9a..b5d31571a 100644 --- a/packages/producer/src/services/render/stages/probeStage.ts +++ b/packages/producer/src/services/render/stages/probeStage.ts @@ -83,6 +83,16 @@ export interface ProbeStageInput { deviceScaleFactor: number; } +const FRAME_BOUNDARY_EPSILON = 1e-3; + +function durationToFrameCount(duration: number, fps: number): number { + const rawFrameCount = duration * fps; + const nearestFrame = Math.round(rawFrameCount); + return Math.abs(rawFrameCount - nearestFrame) <= FRAME_BOUNDARY_EPSILON + ? nearestFrame + : Math.ceil(rawFrameCount); +} + export interface ProbeStageResult { /** May be reassigned from `recompileWithResolutions`. */ compiled: CompiledComposition; @@ -221,6 +231,7 @@ export async function runProbeStage(input: ProbeStageInput): Promise