mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-01 19:42:03 +00:00
fix(engine,producer): apply software-GPU screenshot invariant at concrete-resolved point
Addresses Miguel's R1 blockers:
1. `browserGpuMode: "auto"` that runtime-probes to software slipped past the
`resolveConfig` clamp — that clamp only sees the pre-resolve string. Add
`shouldClampToScreenshotForConcreteGpu(resolvedGpuMode, currentForceScreenshot, env)`
in `packages/engine/src/config.ts` and apply it at BOTH concrete-resolution
sites:
- `packages/engine/src/services/frameCapture.ts`: downgrades `preMode`
from "beginframe" to "screenshot" when resolved GPU is software (respects
`PRODUCER_FORCE_SCREENSHOT=false` env opt-out), fixing the routing.
- `packages/producer/src/services/renderOrchestrator.ts`: updates
`captureObservability.forceScreenshot` (and thus `captureMode`) at the
same call site, fixing the observability truth on the auto → software
case.
2. New unit tests in `config.test.ts`:
- Documents the auto-branch gap (resolveConfig leaves auto as
forceScreenshot=false — the runtime companion closes it).
- 5 branch tests on `shouldClampToScreenshotForConcreteGpu` covering
software / hardware / already-forced / env-opt-out / non-"false" env
values.
Full suite: 56/56 pass.
Scope narrowing on Blocker 2: the distributed rendering path at
`packages/producer/src/services/distributed/plan.ts:753-754` and
`renderChunk.ts:462-466` explicitly hardcodes `browserGpuMode:"software",
forceScreenshot:false` post-resolveConfig and stays outside this PR's
invariant boundary. `compileStage` may still flip it to true for alpha
formats, but generic MP4 distributed renders on SwiftShader hosts remain
BeginFrame. That's a separate architectural cleanup (needs its own
behavior-change trace); the PR body now scopes the invariant to the
in-process CLI/orchestrator path.
This commit is contained in:
@@ -1,7 +1,12 @@
|
||||
import { describe, it, expect, beforeEach, afterEach } from "vitest";
|
||||
import { join } from "node:path";
|
||||
import { tmpdir } from "node:os";
|
||||
import { resolveConfig, DEFAULT_CONFIG, scaleProtocolTimeoutForComposition } from "./config.js";
|
||||
import {
|
||||
resolveConfig,
|
||||
DEFAULT_CONFIG,
|
||||
scaleProtocolTimeoutForComposition,
|
||||
shouldClampToScreenshotForConcreteGpu,
|
||||
} from "./config.js";
|
||||
import { isLowMemorySystem } from "./services/systemMemory.js";
|
||||
|
||||
describe("resolveConfig", () => {
|
||||
@@ -337,6 +342,59 @@ describe("resolveConfig", () => {
|
||||
const config = resolveConfig({ forceScreenshot: true });
|
||||
expect(config.forceScreenshot).toBe(true);
|
||||
});
|
||||
|
||||
it("documents the auto-branch gap: resolveConfig leaves auto→software as forceScreenshot=false", () => {
|
||||
// resolveConfig's clamp keys on the string `browserGpuMode`; `"auto"`
|
||||
// that runtime-probes to software is invisible to this layer. The
|
||||
// runtime companion `shouldClampToScreenshotForConcreteGpu` (below)
|
||||
// closes the gap at the frameCapture + renderOrchestrator sites.
|
||||
setEnv("PRODUCER_BROWSER_GPU_MODE", "auto");
|
||||
unsetEnv("PRODUCER_FORCE_SCREENSHOT");
|
||||
const config = resolveConfig();
|
||||
expect(config.browserGpuMode).toBe("auto");
|
||||
expect(config.forceScreenshot).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("shouldClampToScreenshotForConcreteGpu (runtime companion for auto→software)", () => {
|
||||
it("returns true when resolved GPU is software AND forceScreenshot is currently false", () => {
|
||||
// Env explicitly cleared so PRODUCER_FORCE_SCREENSHOT="false" opt-out
|
||||
// doesn't fire.
|
||||
expect(
|
||||
shouldClampToScreenshotForConcreteGpu("software", false, {} as NodeJS.ProcessEnv),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("returns false when resolved GPU is hardware (no clamp needed)", () => {
|
||||
expect(
|
||||
shouldClampToScreenshotForConcreteGpu("hardware", false, {} as NodeJS.ProcessEnv),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it("returns false when forceScreenshot is already true (invariant already satisfied)", () => {
|
||||
expect(shouldClampToScreenshotForConcreteGpu("software", true, {} as NodeJS.ProcessEnv)).toBe(
|
||||
false,
|
||||
);
|
||||
});
|
||||
|
||||
it("honors PRODUCER_FORCE_SCREENSHOT=false env opt-out on software", () => {
|
||||
// BeginFrame-on-software debugging escape hatch.
|
||||
expect(
|
||||
shouldClampToScreenshotForConcreteGpu("software", false, {
|
||||
PRODUCER_FORCE_SCREENSHOT: "false",
|
||||
} as NodeJS.ProcessEnv),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it("does NOT treat other PRODUCER_FORCE_SCREENSHOT values as opt-out", () => {
|
||||
// Only literal "false" opts out; "true", "0", missing, anything else clamps.
|
||||
for (const value of [undefined, "true", "1", "0", "no", ""]) {
|
||||
const env = (
|
||||
value === undefined ? {} : { PRODUCER_FORCE_SCREENSHOT: value }
|
||||
) as NodeJS.ProcessEnv;
|
||||
expect(shouldClampToScreenshotForConcreteGpu("software", false, env)).toBe(true);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("lowMemoryMode", () => {
|
||||
|
||||
@@ -610,3 +610,28 @@ export function resolveConfig(overrides?: Partial<EngineConfig>): EngineConfig {
|
||||
vp9CpuUsed: normalizeVp9CpuUsed(merged.vp9CpuUsed),
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Runtime-resolved companion to the software-GPU screenshot clamp in
|
||||
* `resolveConfig`. Returns `true` iff callers should treat this render as
|
||||
* `forceScreenshot=true` even though the config's stored `forceScreenshot`
|
||||
* is `false`. Fires when the concrete resolved GPU is software AND the
|
||||
* env-level opt-out (`PRODUCER_FORCE_SCREENSHOT=false`) is NOT set.
|
||||
*
|
||||
* `resolveConfig`'s clamp only sees `browserGpuMode` as a string, so
|
||||
* `"auto"` that runtime-probes to software slips through. This helper
|
||||
* closes that gap at the concrete-resolution points (`frameCapture` and
|
||||
* `renderOrchestrator`). Same invariant, same env opt-out, one predicate.
|
||||
*
|
||||
* Callers should skip when the invariant is already satisfied
|
||||
* (`currentForceScreenshot === true`) to avoid redundant work.
|
||||
*/
|
||||
export function shouldClampToScreenshotForConcreteGpu(
|
||||
resolvedGpuMode: "software" | "hardware",
|
||||
currentForceScreenshot: boolean,
|
||||
env: NodeJS.ProcessEnv = process.env,
|
||||
): boolean {
|
||||
if (currentForceScreenshot) return false;
|
||||
if (resolvedGpuMode !== "software") return false;
|
||||
return env["PRODUCER_FORCE_SCREENSHOT"] !== "false";
|
||||
}
|
||||
|
||||
@@ -48,6 +48,7 @@ export {
|
||||
resolveConfig,
|
||||
DEFAULT_CONFIG,
|
||||
scaleProtocolTimeoutForComposition,
|
||||
shouldClampToScreenshotForConcreteGpu,
|
||||
type EngineConfig,
|
||||
} from "./config.js";
|
||||
export {
|
||||
|
||||
@@ -43,7 +43,11 @@ import {
|
||||
produceDrawElementFrameBatch,
|
||||
} from "./drawElementService.js";
|
||||
import { initThreeDProjection, detectCssEffectRisk } from "./threeDProjection.js";
|
||||
import { DEFAULT_CONFIG, type EngineConfig } from "../config.js";
|
||||
import {
|
||||
DEFAULT_CONFIG,
|
||||
shouldClampToScreenshotForConcreteGpu,
|
||||
type EngineConfig,
|
||||
} from "../config.js";
|
||||
import type {
|
||||
CaptureOptions,
|
||||
CaptureVideoMetadataHint,
|
||||
@@ -812,15 +816,26 @@ export async function createCaptureSession(
|
||||
// need explicit clip+scale on `Page.captureScreenshot`, so fall back to
|
||||
// the screenshot path for any DPR > 1.
|
||||
const supersampling = (options.deviceScaleFactor ?? 1) > 1;
|
||||
const preMode: CaptureMode =
|
||||
headlessShell && isLinux && !forceScreenshot && !supersampling && !drawElementTransparent
|
||||
? "beginframe"
|
||||
: "screenshot";
|
||||
const requestedGpuMode = config?.browserGpuMode ?? DEFAULT_CONFIG.browserGpuMode;
|
||||
const resolvedGpuMode = await resolveBrowserGpuMode(requestedGpuMode, {
|
||||
chromePath: headlessShell ?? undefined,
|
||||
browserTimeout: config?.browserTimeout,
|
||||
});
|
||||
// Apply the software-GPU→screenshot invariant at the concrete-resolved
|
||||
// point too — `resolveConfig` can only see the pre-resolve `browserGpuMode`
|
||||
// string, so `"auto"` that probes to software would otherwise slip through
|
||||
// and launch BeginFrame + SwiftShader (the exact combination the invariant
|
||||
// is meant to prevent). Env-level opt-out preserved via the shared helper.
|
||||
const effectiveForceScreenshot =
|
||||
forceScreenshot || shouldClampToScreenshotForConcreteGpu(resolvedGpuMode, forceScreenshot);
|
||||
const preMode: CaptureMode =
|
||||
headlessShell &&
|
||||
isLinux &&
|
||||
!effectiveForceScreenshot &&
|
||||
!supersampling &&
|
||||
!drawElementTransparent
|
||||
? "beginframe"
|
||||
: "screenshot";
|
||||
const chromeArgs = buildChromeArgs(
|
||||
{ width: options.width, height: options.height, captureMode: preMode },
|
||||
{ ...config, browserGpuMode: resolvedGpuMode },
|
||||
|
||||
@@ -70,6 +70,7 @@ import {
|
||||
type SubTimelineWaitOutcome,
|
||||
resolveBrowserGpuMode,
|
||||
resolveHeadlessShellPath,
|
||||
shouldClampToScreenshotForConcreteGpu,
|
||||
scaleProtocolTimeoutForComposition,
|
||||
isMemoryExhaustionError,
|
||||
isTransientBrowserError,
|
||||
@@ -1908,7 +1909,23 @@ export async function executeRenderJob(
|
||||
chromePath: resolveHeadlessShellPath(cfg),
|
||||
browserTimeout: cfg.browserTimeout,
|
||||
});
|
||||
updateCaptureObservability({ browserGpuMode: resolvedBrowserGpuMode });
|
||||
// Mirror the frameCapture.ts routing invariant here so observability
|
||||
// reports the actual capture mode on `browserGpuMode: "auto"` renders
|
||||
// that probe to software: `resolveConfig` couldn't see this at config
|
||||
// time, so `captureObservability.forceScreenshot` was still false,
|
||||
// misreporting `captureMode: "beginframe"` for a session that will
|
||||
// actually take the screenshot path. Env-level opt-out preserved via
|
||||
// the shared helper.
|
||||
const observabilityForceScreenshot =
|
||||
captureObservability.forceScreenshot ||
|
||||
shouldClampToScreenshotForConcreteGpu(
|
||||
resolvedBrowserGpuMode,
|
||||
captureObservability.forceScreenshot,
|
||||
);
|
||||
updateCaptureObservability({
|
||||
browserGpuMode: resolvedBrowserGpuMode,
|
||||
forceScreenshot: observabilityForceScreenshot,
|
||||
});
|
||||
const videoCaptureBeyondViewport = resolveVideoCaptureBeyondViewport(composition.videos.length);
|
||||
|
||||
const captureOptions: CaptureOptions = {
|
||||
|
||||
Reference in New Issue
Block a user