Repository navigation
Conversation
…rmat The disk-capture headroom gate estimated temporary frame storage as uncompressed RGBA (4 B/px) while disk frames are actually written compressed: measured 1080p JPEG captures land at ~150-400 KB/frame (~1/20-1/30 of the estimate). Ordinary renders were mis-rejected on machines with ample free disk - a 44s 1080p30 composition estimated at ~11GB against 11.4GB free failed, with an actual JPEG footprint of a few hundred MB. estimateDiskCaptureBytes now bills by capture format: JPEG 0.5 B/px (keeps a ~5x safety margin, shrinks the estimate 8x) and PNG 2 B/px (lossless, half the RGBA ceiling). The gate message now states the compressed-frame estimate basis.
…is insufficient When the disk-capture headroom gate rejected a render, the render failed outright even when single-worker streaming was eligible: auto-parallel renders always take the disk path (ordered streaming would stall later workers), and the gate ran as a hard error at capture time instead of a replan trigger. Long renders past the streaming duration cap (240s) had no escape at all - 5 minutes of 1080p30 estimated at ~60GB under the raw RGBA ceiling, rejecting effectively every machine. executeRenderJob now checks the gate right after the initial capture plan is built and, when headroom is insufficient and streaming is eligible at one worker (same gates as the normal streaming decision: format, duration cap, enableStreamingEncode, low-memory mode), degrades the plan to single-worker streaming via shouldDegradeDiskCaptureToStreaming - before capture and telemetry observe it, so the degraded plan is indistinguishable from one the streaming gate chose up front.
miga-heygen
left a comment
There was a problem hiding this comment.
Strengths
captureImageFormat.ts:20/captureStage.ts:166-169— the size estimate itself correctly keys offresolveCaptureImageFormat({needsAlpha, motionBlur})'s output ("png"vs everything else), not a re-derivation. Fixes the real over-estimate: old gate was4 B/pxraw RGBA, new is0.5/2B/px by format.renderOrchestrator.ts:1437-1461(shouldDegradeDiskCaptureToStreaming) — single decision point, calls the pre-existingshouldUseStreamingEncoderather than reimplementing eligibility;captureStage.tsonly ever consumes the frozenCapturePlan.kind. No duplicated eligibility logic, no double-run risk — the degrade check runs once, beforerunCaptureStageis ever reached.- Mutation-tested the estimate and the degrade gate myself: reverting
JPEG_FRAME_BYTES_PER_PIXEL_ESTIMATEto the old raw-RGBA value, inverting theformat === "png"check, and dropping theworkerCount > 1gate all correctly turn tests red. Real coverage on the parts this PR is actually about.
Blocker
captureStage.ts:296—frameExt: needsAlpha ? "png" : "jpg"replacesframeExt: frameFileExtension(captureOptions.format)(and drops theframeFileExtensionimport entirely). This re-derives the file extension from whether the output needs alpha, ignoringmotionBlur. ButresolveCaptureImageFormat(captureImageFormat.ts:20) forces PNG whenneedsAlpha || motionBlur— andmotionBlurRoute.ts:24explicitly listssdr_disk(the exact route this file handles) as a route that supports motion blur (UNSUPPORTED_REASON.sdr_disk = null). So for anymotionBlur-on,needsAlpha-off disk-capture render: actual frames are written as.png(engine writes viaframeFileExtension(session.options.format), andformatresolved to"png"), butframeExt: "jpg"is what gets passed intoexecuteDiskCaptureWithAdaptiveRetry's path-building helpers (renderOrchestrator.ts:865/1071, consumed at1147/1181/1234) — every captured frame's expected path is built with the wrong extension, sofindMissingFrameRangessees every frame as missing.
The engine's ownframeFileExtensiondoc comment (packages/engine/src/services/frameCapture.ts:4008-4011) literally names this exact mistake: "rather than re-deriving the answer from whether the OUTPUT needs alpha, which is a different question and stops being equivalent as soon as anything else forces PNG." This PR re-introduces the mistake the comment was written to prevent, on a route (sdr_disk) the diff otherwise treats as the primary path.
Confirmed with a mutation test: inverting this one line's branches (needsAlpha ? "jpg" : "png") still passes all 271 tests in both changed test files unchanged — zero coverage in either direction. Fix: restoreframeExt: frameFileExtension(captureOptions.format)and keep the import; add a motionBlur+no-alpha case to the disk-capture test file.
Important, non-blocking
captureStage.ts:153-154—JPEG_FRAME_BYTES_PER_PIXEL_ESTIMATE = 0.5/PNG = 2. The comment's own cited range for JPEG is "~150-400 KB/frame (~0.05-0.1 B/px)" — a 2.6x spread — so the claimed "5x safety margin" is against the low end only; against the high end of the author's own numbers it's closer to 2x. High-entropy content (film grain, noise, fine detail — this repo's own effects registry ships some) compresses worse than the assumption, and there's no runtime disk-usage check during capture, only the one pre-capture gate. Recommend a fast-follow: measure actual JPEG bytes/frame across a spread of real content (including worst-case grain/noise) before trusting this margin in production, since under-estimating fails destructively mid-render instead of refusing safely up front like the old gate did.
Evidence gap, not a defect
- Every number in #4060 (18GB free, the 44s/11GB estimate, "few hundred MB actual") is reporter/author-provided and unreproduced here — no Windows box, no real capture run. A bounded repro (offered by Genesis, accepted) would need actual JPEG bytes/frame at the real render quality (q=80/95) across representative + worst-case-entropy content, compared against 0.5 B/px, plus the PNG/alpha equivalent against 2 B/px.
Verdict: REQUEST CHANGES
Reasoning: The size-estimate and streaming-degrade mechanisms are sound and tested; the frameExt line is a real, silent, untested regression on a currently-supported route (motion blur + no alpha, sdr_disk), re-committing a mistake the codebase already has a comment warning against. Fix that one line plus add a covering test, and this is close to done.
— Miga
|
Apply-ready patch from local rebase ( @CasbaL, please apply and push this on your own PR head; that will re-run CI and lets you update the PR body/permalinks on your branch. diff --git a/packages/producer/src/services/render/stages/captureStage.test.ts b/packages/producer/src/services/render/stages/captureStage.test.ts
index 46d6ee16b..ec57dbb79 100644
--- a/packages/producer/src/services/render/stages/captureStage.test.ts
+++ b/packages/producer/src/services/render/stages/captureStage.test.ts
@@ -5,6 +5,7 @@ import { join } from "node:path";
import { DEFAULT_CONFIG, classifyCaptureFailure } from "@hyperframes/engine";
import { createRenderJob, isTransientCaptureRetryEligible } from "../../renderOrchestrator.js";
import { formatCaptureFrameName } from "../../../utils/paths.js";
+import { resolveCaptureImageFormat } from "../captureImageFormat.js";
// Mock only the engine session primitives; `classifyCaptureFailure` stays
// real so the retry decision is exercised.
@@ -318,6 +319,32 @@ describe("runCaptureStage measured disk projection (both branches)", () => {
/measured from the first 10 frames/,
);
});
+
+ it("uses the resolved png format for motion blur frames without alpha", async () => {
+ const { workDir, framesDir } = makeDirs();
+ const format = resolveCaptureImageFormat({ needsAlpha: false, motionBlur: {} });
+ expect(format).toBe("png");
+ freeBytes.mockReturnValue({ bavail: 100_000_000, bsize: 1 });
+ executeParallelCapture.mockImplementation(async () => {
+ for (let index = 0; index < 20; index++) {
+ writeFileSync(join(framesDir, formatCaptureFrameName(index, "png")), "captured-frame");
+ }
+ return [];
+ });
+
+ await expect(
+ runCaptureStage({
+ ...makeInput(2, workDir, framesDir),
+ plan: { workerCount: 2, forceScreenshot: false, needsAlpha: false },
+ buildCaptureOptions: () => ({
+ width: 64,
+ height: 64,
+ fps: { num: 30, den: 1 },
+ format,
+ }),
+ }),
+ ).resolves.toMatchObject({ workerCount: 2 });
+ });
});
describe("runCaptureStage — sequential path transient Target-closed single retry (integration)", () => {
diff --git a/packages/producer/src/services/renderOrchestrator.test.ts b/packages/producer/src/services/renderOrchestrator.test.ts
index 221f47b80..da0153a55 100644
--- a/packages/producer/src/services/renderOrchestrator.test.ts
+++ b/packages/producer/src/services/renderOrchestrator.test.ts
@@ -1,3 +1,4 @@
+// fallow-ignore-file code-duplication complexity
import { afterEach, describe, expect, it, vi } from "vitest";
import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs";
import { join, win32 } from "node:path";
@@ -55,6 +56,7 @@ import {
resolveAdaptersUsed,
resolveDeShortBand,
shouldClampDefaultDrawElement,
+ shouldDegradeDiskCaptureToStreaming,
shouldPreferParallelDrawElement,
shouldPreferSingleWorkerDrawElement,
isCaptureParallelStreamRouterEnabled,
@@ -723,6 +725,74 @@ describe("explainStreamingEncodeGate", () => {
});
});
+describe("shouldDegradeDiskCaptureToStreaming", () => {
+ it("degrades a multi-worker disk plan when disk lacks headroom and streaming is eligible", () => {
+ expect(
+ shouldDegradeDiskCaptureToStreaming({
+ planKind: "sdr_disk",
+ workerCount: 4,
+ diskHeadroomAvailable: false,
+ streamingEligible: true,
+ }),
+ ).toBe(true);
+ });
+
+ it("keeps the disk plan when headroom is sufficient", () => {
+ expect(
+ shouldDegradeDiskCaptureToStreaming({
+ planKind: "sdr_disk",
+ workerCount: 4,
+ diskHeadroomAvailable: true,
+ streamingEligible: true,
+ }),
+ ).toBe(false);
+ });
+
+ it("never degrades streaming or layered plans", () => {
+ expect(
+ shouldDegradeDiskCaptureToStreaming({
+ planKind: "sdr_streaming",
+ workerCount: 4,
+ diskHeadroomAvailable: false,
+ streamingEligible: true,
+ }),
+ ).toBe(false);
+ expect(
+ shouldDegradeDiskCaptureToStreaming({
+ planKind: "hdr_layered",
+ workerCount: 4,
+ diskHeadroomAvailable: false,
+ streamingEligible: true,
+ }),
+ ).toBe(false);
+ });
+
+ it("keeps the disk plan when streaming is ineligible for a single worker", () => {
+ // Covers png-sequence/gif outputs, renders past the streaming duration
+ // cap, and enableStreamingEncode=false — all surfaced via the eligibility
+ // flag computed by shouldUseStreamingEncode(cfg, format, 1, duration).
+ expect(
+ shouldDegradeDiskCaptureToStreaming({
+ planKind: "sdr_disk",
+ workerCount: 4,
+ diskHeadroomAvailable: false,
+ streamingEligible: false,
+ }),
+ ).toBe(false);
+ });
+
+ it("ignores single-worker disk plans", () => {
+ expect(
+ shouldDegradeDiskCaptureToStreaming({
+ planKind: "sdr_disk",
+ workerCount: 1,
+ diskHeadroomAvailable: false,
+ streamingEligible: true,
+ }),
+ ).toBe(false);
+ });
+});
+
describe("createCompiledFrameSrcResolver", () => {
it("maps extracted frame paths under compiledDir to encoded server URLs", () => {
const resolver = createCompiledFrameSrcResolver("/tmp/hf job/compiled");
diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts
index 388192d6a..4836aa50f 100644
--- a/packages/producer/src/services/renderOrchestrator.ts
+++ b/packages/producer/src/services/renderOrchestrator.ts
@@ -1547,6 +1547,33 @@ export function shouldUseStreamingEncode(
).enabled;
}
+/**
+ * Whether a disk capture plan that failed the disk-headroom gate should
+ * degrade to single-worker streaming instead of failing the render.
+ *
+ * The disk gate estimates worst-case temporary frame storage and refuses
+ * captures whose estimate crosses 90% of free disk, while auto-parallel
+ * renders always land on the disk path (ordered streaming would stall later
+ * workers). A machine with modest free disk therefore failed renders that
+ * single-worker streaming could finish. Degradation is only offered when
+ * streaming is genuinely eligible for this render at one worker (same gates
+ * as the normal streaming decision: format, duration cap,
+ * enableStreamingEncode, low-memory mode).
+ */
+export function shouldDegradeDiskCaptureToStreaming(args: {
+ planKind: CapturePlan["kind"];
+ workerCount: number;
+ diskHeadroomAvailable: boolean;
+ streamingEligible: boolean;
+}): boolean {
+ return (
+ args.planKind === "sdr_disk" &&
+ args.workerCount > 1 &&
+ !args.diskHeadroomAvailable &&
+ args.streamingEligible
+ );
+}
+
/**
* Integer tuning knob from the environment. Matches the convention the
* surrounding DE thresholds already use: unset OR set-but-empty falls back to
@@ -4287,6 +4314,34 @@ async function executeRenderPipeline(input: {
needsAlpha,
routing: captureRouting,
});
+ // Disk-headroom degradation (see shouldDegradeDiskCaptureToStreaming):
+ // rebuild the plan as single-worker streaming BEFORE capture and
+ // telemetry read it, so the degraded plan is indistinguishable from one
+ // the streaming gate chose up front.
+ if (
+ shouldDegradeDiskCaptureToStreaming({
+ planKind: capturePlan.kind,
+ workerCount: capturePlan.workerCount,
+ diskHeadroomAvailable: inspectDiskCaptureHeadroom(
+ framesDir,
+ totalFrames,
+ buildCaptureOptions(),
+ ).available,
+ streamingEligible: shouldUseStreamingEncode(cfg, outputFormat, 1, job.duration),
+ })
+ ) {
+ log.info(
+ `[Render] Disk capture lacks headroom for ~${totalFrames} frames at ${framesDir}; ` +
+ `degrading ${capturePlan.workerCount}-worker disk capture to single-worker streaming encode.`,
+ );
+ capturePlan = createCapturePlan({
+ ...capturePlan,
+ workerCount: 1,
+ useStreamingEncode: true,
+ useLayeredComposite: false,
+ forceParallelStream: false,
+ });
+ }
const syncCapturePlan = (): void => {
// Every route the render can end up on passes through here, including the ones a
// replan lands on, so this is where motion blur's one supported-route answer belongs. |
|
Non-blocking review follow-ups (not part of this patch):
|
|
Closing as stale after a few weeks without activity. Thanks for the contribution; feel free to reopen after rebasing on main. |
|
Finally rebased — GitHub won't reopen a PR whose head was force-pushed while closed, so the successor is #5334 (same branch name, reimplemented on v0.8.143's capturePlan/replanAfterFailure seams). The estimate half was absorbed upstream (JPEG at raw/8 + the measured projection); #5334 is the degrade, and the frameExt blocker from this PR is moot since the new diff never touches frameExt. Thanks @heygengenesis — the motionBlur test from your patch is covered upstream by the existing |
Summary
Fixes #4060
Two-part fix for the disk-capture headroom gate rejecting ordinary renders on machines with modest free disk:
1. Calibrate the estimate to the actual capture format (
captureStage.ts)estimateDiskCaptureBytesbilledtotalFrames × width × height × 4— a raw RGBA ceiling — while the capture stage writes compressedjpg(orpngfor alpha) frames. It now bills by format:The gate message now states the compressed-frame estimate basis. A 44 s 1080p30 composition that previously estimated ~11 GB now estimates ~1.4 GB (actual JPEG footprint: a few hundred MB).
2. Degrade to single-worker streaming when headroom is insufficient (
renderOrchestrator.ts)Auto-parallel renders always take the disk path (
shouldUseStreamingEncoderequiresworkerCount === 1), so the only reason a render landed on the disk path was parallelism — and the gate then hard-failed the render even though single-worker streaming could finish it.executeRenderJobnow checksinspectDiskCaptureHeadroomright after the initial capture plan is built; when headroom is insufficient andshouldUseStreamingEncode(cfg, format, 1, duration)is eligible, ashouldDegradeDiskCaptureToStreamingpredicate triggers a rebuild of the plan as single-worker streaming — before capture and telemetry observe it, so the degraded plan is indistinguishable from one the streaming gate chose up front. Permanent ineligibility (png-sequence/gif output, duration past the streaming cap,enableStreamingEncode=false) keeps the existing gate error, now with the calibrated estimate.Test plan
captureStage.test.ts: format-dependent estimate (JPEG vs PNG), calibrated gate rejection, updated message assertion.renderOrchestrator.test.ts: newshouldDegradeDiskCaptureToStreamingdecision table (5 cases — multi-worker disk + no headroom + streaming-eligible degrades; sufficient headroom / streaming plans / ineligible streaming / single-worker plans do not).renderOrchestrator.test.ts+captureStage.test.tssuites pass (the only failures are 6 pre-existing environment-dependent cases that fail identically on cleanmain).tsc --noEmitclean for@hyperframes/producer.