mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-03 04:38:33 +00:00
fix(engine,producer): drive forceScreenshot from one authoritative local
Miguel R4 blocker on #2359: my R3 fix at renderOrchestrator only updated the observability copy, leaving the authoritative captureForceScreenshot local at compileResult.forceScreenshot (false for auto→software). The frameCapture side clamped its own local and correctly routed screenshot, but downstream orchestrator code overwrote observability back to beginframe from the still-false local at two sites: - Parallel-stream label at renderOrchestrator.ts:2293 mis-labelled the stream as 'beginframe' when actual capture was 'screenshot'. - capture_strategy telemetry at renderOrchestrator.ts:2440-2450 overwrote the earlier observability correction, so the final captureMode observation flipped back to 'beginframe' while the engine actually captured screenshot. Fix: extract the clamp into a caller-facing helper applyConcreteGpuScreenshotClamp(current, resolvedGpuMode, cfg) that returns the (possibly-promoted) new boolean. Callers assign it back to their authoritative local, so routing + telemetry + strategy code read one value. Changes: - packages/engine/src/config.ts: new exported applyConcreteGpuScreenshotClamp; delegates to shouldClampToScreenshotForConcreteGpu but computes the caller's final value, not just the clamp decision. Reads the programmatic opt-out from cfg.forceScreenshotExplicitlyOptedOut. Idempotent on already-true input. - packages/engine/src/index.ts: export the new helper. - packages/engine/src/services/frameCapture.ts: replace the inline OR expression with applyConcreteGpuScreenshotClamp. - packages/producer/src/services/renderOrchestrator.ts: assign result into the AUTHORITATIVE captureForceScreenshot local (was updating only observability). Downstream parallel-stream label at :2293 and capture_strategy telemetry at :2440-2450 now read the corrected value. Tests: 6 new caller-level cases for applyConcreteGpuScreenshotClamp covering the exact matrix Miguel called out: - resolved software + default false → promotes to true (screenshot) - resolved software + programmatic opt-out → stays false (BeginFrame) - resolved hardware + default false → stays false - resolved software + already-true → stays true (idempotent) - resolved software + env PRODUCER_FORCE_SCREENSHOT=false → stays false - resolved software + undefined cfg → promotes to true (frameCapture path) Local: 67/67 engine config tests pass (was 61). oxfmt clean.
This commit is contained in:
@@ -6,7 +6,9 @@ import {
|
||||
DEFAULT_CONFIG,
|
||||
scaleProtocolTimeoutForComposition,
|
||||
shouldClampToScreenshotForConcreteGpu,
|
||||
applyConcreteGpuScreenshotClamp,
|
||||
} from "./config.js";
|
||||
import type { EngineConfig } from "./config.js";
|
||||
import { isLowMemorySystem } from "./services/systemMemory.js";
|
||||
|
||||
describe("resolveConfig", () => {
|
||||
@@ -422,6 +424,77 @@ describe("resolveConfig", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("applyConcreteGpuScreenshotClamp (caller-level contract)", () => {
|
||||
// This is the helper both frameCapture.ts and renderOrchestrator.ts call
|
||||
// to compute the value the AUTHORITATIVE `forceScreenshot` local should
|
||||
// hold after the concrete GPU is resolved. Routing AND telemetry read
|
||||
// from that one value, so this contract must hold across default and
|
||||
// opt-out combinations.
|
||||
type OptOutCfg = Pick<EngineConfig, "forceScreenshotExplicitlyOptedOut">;
|
||||
const cleanEnv = {} as NodeJS.ProcessEnv;
|
||||
|
||||
it("resolved software + default false → promotes to true (screenshot route)", () => {
|
||||
// The core auto→software fix: routing AND downstream telemetry read
|
||||
// the promoted value, so `updateCaptureObservability({ forceScreenshot:
|
||||
// captureForceScreenshot })` at the capture_strategy site reports
|
||||
// screenshot instead of overwriting back to beginframe.
|
||||
expect(applyConcreteGpuScreenshotClamp(false, "software", {} as OptOutCfg, cleanEnv)).toBe(
|
||||
true,
|
||||
);
|
||||
});
|
||||
|
||||
it("resolved software + programmatic opt-out → stays false (BeginFrame preserved)", () => {
|
||||
// The programmatic escape hatch caller-level contract: setting
|
||||
// overrides.forceScreenshot=false must keep BeginFrame across BOTH
|
||||
// routing (frameCapture) and telemetry (renderOrchestrator) — since
|
||||
// resolveConfig lifts the flag onto the config, both callers converge.
|
||||
expect(
|
||||
applyConcreteGpuScreenshotClamp(
|
||||
false,
|
||||
"software",
|
||||
{ forceScreenshotExplicitlyOptedOut: true } as OptOutCfg,
|
||||
cleanEnv,
|
||||
),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it("resolved hardware + default false → stays false (no clamp needed)", () => {
|
||||
expect(applyConcreteGpuScreenshotClamp(false, "hardware", {} as OptOutCfg, cleanEnv)).toBe(
|
||||
false,
|
||||
);
|
||||
});
|
||||
|
||||
it("resolved software + already-true forceScreenshot → stays true (idempotent)", () => {
|
||||
// Config-time clamp already fired (literal browserGpuMode:"software"),
|
||||
// so re-applying at the concrete-resolved site is a no-op.
|
||||
expect(applyConcreteGpuScreenshotClamp(true, "software", {} as OptOutCfg, cleanEnv)).toBe(
|
||||
true,
|
||||
);
|
||||
});
|
||||
|
||||
it("resolved software + env PRODUCER_FORCE_SCREENSHOT=false → stays false", () => {
|
||||
// Env opt-out preserved even when programmatic flag is not set (some
|
||||
// callers, like debugging BeginFrame-on-software from CI, opt-out via
|
||||
// env only).
|
||||
expect(
|
||||
applyConcreteGpuScreenshotClamp(
|
||||
false,
|
||||
"software",
|
||||
{} as OptOutCfg,
|
||||
{
|
||||
PRODUCER_FORCE_SCREENSHOT: "false",
|
||||
} as NodeJS.ProcessEnv,
|
||||
),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it("resolved software + undefined cfg → default (no programmatic opt-out) → clamps to true", () => {
|
||||
// Sanity: frameCapture.ts calls with `config` possibly undefined.
|
||||
// Default case must still promote.
|
||||
expect(applyConcreteGpuScreenshotClamp(false, "software", undefined, cleanEnv)).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe("forceScreenshotExplicitlyOptedOut provenance", () => {
|
||||
it("is set to true when programmatic override forceScreenshot=false is passed", () => {
|
||||
setEnv("PRODUCER_BROWSER_GPU_MODE", "hardware");
|
||||
|
||||
@@ -661,3 +661,31 @@ export function shouldClampToScreenshotForConcreteGpu(
|
||||
if (opts.programmaticOptOut) return false;
|
||||
return env["PRODUCER_FORCE_SCREENSHOT"] !== "false";
|
||||
}
|
||||
|
||||
/**
|
||||
* Caller-facing pair to `shouldClampToScreenshotForConcreteGpu`: computes the
|
||||
* value the *authoritative* `forceScreenshot` local should hold after the
|
||||
* concrete-resolved-GPU decision fires. Returns the (possibly-promoted) new
|
||||
* boolean, so the caller can assign it back to its local — driving both
|
||||
* routing AND telemetry from one source of truth.
|
||||
*
|
||||
* Reads the programmatic opt-out from `cfg.forceScreenshotExplicitlyOptedOut`
|
||||
* (set by `resolveConfig` when EITHER env `PRODUCER_FORCE_SCREENSHOT=false`
|
||||
* OR programmatic `overrides.forceScreenshot === false` was present).
|
||||
*
|
||||
* Idempotent: `applyConcreteGpuScreenshotClamp(true, ...)` returns `true`
|
||||
* without consulting anything else.
|
||||
*/
|
||||
export function applyConcreteGpuScreenshotClamp(
|
||||
currentForceScreenshot: boolean,
|
||||
resolvedGpuMode: "software" | "hardware",
|
||||
cfg: Pick<EngineConfig, "forceScreenshotExplicitlyOptedOut"> | undefined,
|
||||
env: NodeJS.ProcessEnv = process.env,
|
||||
): boolean {
|
||||
return (
|
||||
currentForceScreenshot ||
|
||||
shouldClampToScreenshotForConcreteGpu(resolvedGpuMode, currentForceScreenshot, env, {
|
||||
programmaticOptOut: cfg?.forceScreenshotExplicitlyOptedOut ?? false,
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
@@ -49,6 +49,7 @@ export {
|
||||
DEFAULT_CONFIG,
|
||||
scaleProtocolTimeoutForComposition,
|
||||
shouldClampToScreenshotForConcreteGpu,
|
||||
applyConcreteGpuScreenshotClamp,
|
||||
type EngineConfig,
|
||||
} from "./config.js";
|
||||
export {
|
||||
|
||||
@@ -43,11 +43,7 @@ import {
|
||||
produceDrawElementFrameBatch,
|
||||
} from "./drawElementService.js";
|
||||
import { initThreeDProjection, detectCssEffectRisk } from "./threeDProjection.js";
|
||||
import {
|
||||
DEFAULT_CONFIG,
|
||||
shouldClampToScreenshotForConcreteGpu,
|
||||
type EngineConfig,
|
||||
} from "../config.js";
|
||||
import { DEFAULT_CONFIG, applyConcreteGpuScreenshotClamp, type EngineConfig } from "../config.js";
|
||||
import type {
|
||||
CaptureOptions,
|
||||
CaptureVideoMetadataHint,
|
||||
@@ -826,15 +822,15 @@ export async function createCaptureSession(
|
||||
// string, so `"auto"` that probes to software would otherwise slip through
|
||||
// and launch BeginFrame + SwiftShader (the exact combination the invariant
|
||||
// is meant to prevent). Both env and programmatic opt-outs preserved via
|
||||
// the shared helper (the programmatic one carried on the config as
|
||||
// `forceScreenshotExplicitlyOptedOut`, since at this point the boolean
|
||||
// `forceScreenshot === false` is otherwise ambiguous between default and
|
||||
// explicit opt-out).
|
||||
const effectiveForceScreenshot =
|
||||
forceScreenshot ||
|
||||
shouldClampToScreenshotForConcreteGpu(resolvedGpuMode, forceScreenshot, process.env, {
|
||||
programmaticOptOut: config?.forceScreenshotExplicitlyOptedOut ?? false,
|
||||
});
|
||||
// `applyConcreteGpuScreenshotClamp` (the programmatic one carried on the
|
||||
// config as `forceScreenshotExplicitlyOptedOut`, since at this point the
|
||||
// boolean `forceScreenshot === false` is otherwise ambiguous between
|
||||
// default and explicit opt-out).
|
||||
const effectiveForceScreenshot = applyConcreteGpuScreenshotClamp(
|
||||
forceScreenshot,
|
||||
resolvedGpuMode,
|
||||
config,
|
||||
);
|
||||
const preMode: CaptureMode =
|
||||
headlessShell &&
|
||||
isLinux &&
|
||||
|
||||
@@ -70,7 +70,7 @@ import {
|
||||
type SubTimelineWaitOutcome,
|
||||
resolveBrowserGpuMode,
|
||||
resolveHeadlessShellPath,
|
||||
shouldClampToScreenshotForConcreteGpu,
|
||||
applyConcreteGpuScreenshotClamp,
|
||||
scaleProtocolTimeoutForComposition,
|
||||
isMemoryExhaustionError,
|
||||
isTransientBrowserError,
|
||||
@@ -1909,25 +1909,27 @@ export async function executeRenderJob(
|
||||
chromePath: resolveHeadlessShellPath(cfg),
|
||||
browserTimeout: cfg.browserTimeout,
|
||||
});
|
||||
// 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. Both env and programmatic opt-outs
|
||||
// preserved via the shared helper (the programmatic one carried on the
|
||||
// Apply the software-GPU→screenshot clamp to the AUTHORITATIVE local
|
||||
// `captureForceScreenshot` (not just the observability copy) so all
|
||||
// downstream strategy + telemetry code reads the corrected value.
|
||||
// Otherwise: `frameCapture.ts` clamps its own local and routes
|
||||
// screenshot, but the still-`false` orchestrator local (a) mislabels the
|
||||
// parallel-stream logging as "beginframe" below and (b) overwrites the
|
||||
// earlier observability correction back to BeginFrame at the
|
||||
// capture_strategy telemetry site. `resolveConfig` couldn't see
|
||||
// `browserGpuMode:"auto"` resolving to software at config time, so
|
||||
// `captureForceScreenshot` was still `compileResult.forceScreenshot === false`
|
||||
// on that path. Both env and programmatic opt-outs preserved via
|
||||
// `applyConcreteGpuScreenshotClamp` (the programmatic one carried on the
|
||||
// config as `forceScreenshotExplicitlyOptedOut`).
|
||||
const observabilityForceScreenshot =
|
||||
captureObservability.forceScreenshot ||
|
||||
shouldClampToScreenshotForConcreteGpu(
|
||||
resolvedBrowserGpuMode,
|
||||
captureObservability.forceScreenshot,
|
||||
process.env,
|
||||
{ programmaticOptOut: cfg.forceScreenshotExplicitlyOptedOut ?? false },
|
||||
);
|
||||
captureForceScreenshot = applyConcreteGpuScreenshotClamp(
|
||||
captureForceScreenshot,
|
||||
resolvedBrowserGpuMode,
|
||||
cfg,
|
||||
);
|
||||
updateCaptureObservability({
|
||||
browserGpuMode: resolvedBrowserGpuMode,
|
||||
forceScreenshot: observabilityForceScreenshot,
|
||||
forceScreenshot: captureForceScreenshot,
|
||||
});
|
||||
const videoCaptureBeyondViewport = resolveVideoCaptureBeyondViewport(composition.videos.length);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user