mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-03 04:38:33 +00:00
fix(cli): validate stops misreporting slow-loading media as unreadable (#1849)
Two independent post-release feedback reports of validate warning about audio duration despite an explicit, correct data-duration slot, one of them naming a timeout explicitly. Root cause: auditClipDurations reads each <video>/<audio> element's intrinsic .duration via a single page.evaluate() snapshot taken after a flat, unconditional page-settle sleep (opts.timeout ?? 3000ms, shared with other audits). Per the HTML spec, HTMLMediaElement.duration is NaN until metadata loads. A slow-loading audio file (large narration WAV, remote source) can still be mid-fetch when that sleep elapses — el.duration is NaN at that exact instant, which the audit permanently records as "could not read the duration" even though the render pipeline (which properly awaits media readiness) handles the same file fine. Fix: race each not-yet-ready element's loadedmetadata/error event against a deadline instead of taking one fixed-time snapshot. Elements already ready resolve immediately (no added latency in the common case); only genuinely slow elements get a real second chance before the warning fires. The race/cleanup wiring lives twice by necessity — once inline inside the page.evaluate() closure (Puppeteer serializes and re-runs that closure in an isolated browser realm with no access to this module), and once as the exported, duck-typed raceMediaReady for a real, deterministic unit test via Node's built-in EventTarget (no browser or DOM library needed). The comment on raceMediaReady flags that both copies must move together.
This commit is contained in:
@@ -1,7 +1,60 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { extractCompositionErrorsFromLint, shouldIgnoreRequestFailure } from "./validate.js";
|
||||
import {
|
||||
extractCompositionErrorsFromLint,
|
||||
raceMediaReady,
|
||||
shouldIgnoreRequestFailure,
|
||||
} from "./validate.js";
|
||||
import type { ProjectLintResult } from "../utils/lintProject.js";
|
||||
|
||||
// Regression for the validate audio-duration-probe timeout: a slow-loading
|
||||
// media element's duration was snapshotted once, at a fixed point in time,
|
||||
// and any element still mid-load was permanently misreported as unreadable.
|
||||
// raceMediaReady is the extracted wiring auditClipDurations now uses to wait
|
||||
// for `loadedmetadata` up to a deadline instead. Node's built-in EventTarget
|
||||
// satisfies the same duck-typed shape as a real HTMLMediaElement here, so
|
||||
// this is a real test of the race/cleanup logic, not a browser mock.
|
||||
describe("raceMediaReady", () => {
|
||||
class FakeMediaElement extends EventTarget {
|
||||
duration = NaN;
|
||||
}
|
||||
|
||||
it("resolves immediately when duration is already available", async () => {
|
||||
const el = new FakeMediaElement();
|
||||
el.duration = 12.5;
|
||||
const start = Date.now();
|
||||
await raceMediaReady(el, Date.now() + 5000);
|
||||
expect(Date.now() - start).toBeLessThan(50);
|
||||
});
|
||||
|
||||
it("resolves as soon as loadedmetadata fires, before the deadline", async () => {
|
||||
const el = new FakeMediaElement();
|
||||
const promise = raceMediaReady(el, Date.now() + 5000);
|
||||
setTimeout(() => {
|
||||
el.duration = 8;
|
||||
el.dispatchEvent(new Event("loadedmetadata"));
|
||||
}, 20);
|
||||
const start = Date.now();
|
||||
await promise;
|
||||
expect(Date.now() - start).toBeLessThan(200);
|
||||
});
|
||||
|
||||
it("resolves on error without hanging until the deadline", async () => {
|
||||
const el = new FakeMediaElement();
|
||||
const promise = raceMediaReady(el, Date.now() + 5000);
|
||||
setTimeout(() => el.dispatchEvent(new Event("error")), 20);
|
||||
const start = Date.now();
|
||||
await promise;
|
||||
expect(Date.now() - start).toBeLessThan(200);
|
||||
});
|
||||
|
||||
it("falls back to the deadline when no event ever fires", async () => {
|
||||
const el = new FakeMediaElement();
|
||||
const start = Date.now();
|
||||
await raceMediaReady(el, Date.now() + 50);
|
||||
expect(Date.now() - start).toBeGreaterThanOrEqual(40);
|
||||
});
|
||||
});
|
||||
|
||||
describe("shouldIgnoreRequestFailure", () => {
|
||||
it("ignores aborted media preload requests", () => {
|
||||
expect(
|
||||
|
||||
@@ -74,6 +74,37 @@ async function seekTo(page: import("puppeteer-core").Page, time: number): Promis
|
||||
await new Promise((r) => setTimeout(r, SEEK_SETTLE_MS));
|
||||
}
|
||||
|
||||
/**
|
||||
* Race a media element's `loadedmetadata`/`error` event against a deadline,
|
||||
* whichever comes first. Already-ready elements resolve immediately.
|
||||
*
|
||||
* This is the same wiring as the inline copy inside `auditClipDurations`'s
|
||||
* `page.evaluate()` below — duplicated, not imported, because Puppeteer
|
||||
* serializes that closure's source and re-runs it in an isolated browser
|
||||
* realm with no access to this module's scope. Kept here (duck-typed on
|
||||
* `EventTarget`-shaped objects, not `HTMLMediaElement`) so the actual
|
||||
* race/cleanup logic has a real, deterministic unit test — Node's built-in
|
||||
* `EventTarget` satisfies the same shape without a browser or DOM library.
|
||||
* If you change one copy, change both.
|
||||
*/
|
||||
export function raceMediaReady(
|
||||
el: EventTarget & { duration: number },
|
||||
deadlineMs: number,
|
||||
): Promise<void> {
|
||||
if (Number.isFinite(el.duration) && el.duration > 0) return Promise.resolve();
|
||||
return new Promise<void>((resolve) => {
|
||||
const onReady = () => {
|
||||
el.removeEventListener("loadedmetadata", onReady);
|
||||
el.removeEventListener("error", onReady);
|
||||
clearTimeout(timer);
|
||||
resolve();
|
||||
};
|
||||
el.addEventListener("loadedmetadata", onReady, { once: true });
|
||||
el.addEventListener("error", onReady, { once: true });
|
||||
const timer = setTimeout(onReady, Math.max(0, deadlineMs - Date.now()));
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Flag `<video>`/`<audio>` clips whose source is meaningfully shorter than their
|
||||
* `data-duration` slot (the slot gets silently shortened in renders). Runs in
|
||||
@@ -83,8 +114,46 @@ async function seekTo(page: import("puppeteer-core").Page, time: number): Promis
|
||||
async function auditClipDurations(
|
||||
page: import("puppeteer-core").Page,
|
||||
analyzeClipMediaFit: typeof import("@hyperframes/engine").analyzeClipMediaFit,
|
||||
extraWaitMs: number,
|
||||
): Promise<ConsoleEntry[]> {
|
||||
const clips = await page.evaluate(() => {
|
||||
// fallow-ignore-next-line complexity
|
||||
const clips = await page.evaluate(async (maxWaitMs: number) => {
|
||||
const nodes = Array.from(
|
||||
document.querySelectorAll("video[data-duration], audio[data-duration]"),
|
||||
) as HTMLMediaElement[];
|
||||
|
||||
// The caller's page-settle sleep is a flat, unconditional wait shared with
|
||||
// other audits — it isn't aware of how long any given media element takes
|
||||
// to load metadata. A slow-loading audio file (large narration WAV, remote
|
||||
// source) can still be mid-fetch when that sleep elapses, which read as
|
||||
// el.duration === NaN and was misreported as "could not read the duration"
|
||||
// even though the render pipeline (which properly awaits media readiness)
|
||||
// handles the same file fine. Give still-loading elements one more real
|
||||
// chance via loadedmetadata before giving up, instead of a single fixed-time
|
||||
// snapshot. Elements that already have a duration resolve immediately, so
|
||||
// this adds no latency in the common case.
|
||||
const deadline = Date.now() + maxWaitMs;
|
||||
await Promise.all(
|
||||
nodes.map((el) => {
|
||||
if (Number.isFinite(el.duration) && el.duration > 0) return Promise.resolve();
|
||||
return new Promise<void>((resolve) => {
|
||||
// fallow-ignore-next-line code-duplication
|
||||
const cleanup = () => {
|
||||
el.removeEventListener("loadedmetadata", onReady);
|
||||
el.removeEventListener("error", onReady);
|
||||
clearTimeout(timer);
|
||||
};
|
||||
const onReady = () => {
|
||||
cleanup();
|
||||
resolve();
|
||||
};
|
||||
el.addEventListener("loadedmetadata", onReady, { once: true });
|
||||
el.addEventListener("error", onReady, { once: true });
|
||||
const timer = setTimeout(onReady, Math.max(0, deadline - Date.now()));
|
||||
});
|
||||
}),
|
||||
);
|
||||
|
||||
const rows: Array<{
|
||||
id: string;
|
||||
kind: string;
|
||||
@@ -93,10 +162,9 @@ async function auditClipDurations(
|
||||
duration: number;
|
||||
loop: boolean;
|
||||
}> = [];
|
||||
document.querySelectorAll("video[data-duration], audio[data-duration]").forEach((node) => {
|
||||
const el = node as HTMLMediaElement;
|
||||
for (const el of nodes) {
|
||||
const slot = parseFloat(el.getAttribute("data-duration") ?? "");
|
||||
if (!(slot > 0)) return;
|
||||
if (!(slot > 0)) continue;
|
||||
rows.push({
|
||||
id: el.id || el.getAttribute("src") || `(${el.tagName.toLowerCase()})`,
|
||||
kind: el.tagName === "AUDIO" ? "Audio" : "Video",
|
||||
@@ -105,9 +173,9 @@ async function auditClipDurations(
|
||||
duration: el.duration,
|
||||
loop: el.loop || el.getAttribute("data-loop") === "true",
|
||||
});
|
||||
});
|
||||
}
|
||||
return rows;
|
||||
});
|
||||
}, extraWaitMs);
|
||||
|
||||
const warnings: ConsoleEntry[] = [];
|
||||
const unreadable: string[] = [];
|
||||
@@ -288,7 +356,7 @@ async function validateInBrowser(
|
||||
await page.goto(server.url, { waitUntil: "domcontentloaded", timeout: 10000 });
|
||||
await new Promise((r) => setTimeout(r, opts.timeout ?? 3000));
|
||||
|
||||
for (const w of await auditClipDurations(page, analyzeClipMediaFit)) {
|
||||
for (const w of await auditClipDurations(page, analyzeClipMediaFit, opts.timeout ?? 3000)) {
|
||||
warnings.push(w);
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user