Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions packages/engine/src/services/frameCapture.captureOptions.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// @vitest-environment node
import { describe, expect, it } from "vitest";
import { resolveCaptureSessionOptions } from "./frameCapture.js";

describe("createCaptureSession captureBeyondViewport defaults", () => {
it("plumbs the macOS regular-Chrome default into returned session options", () => {
const options = resolveCaptureSessionOptions(
{
width: 1920,
height: 1080,
fps: { num: 30, den: 1 },
format: "jpeg",
},
"Chrome/149.0.7827.155",
"darwin",
);

expect(options.captureBeyondViewport).toBe(true);
});

it("preserves explicit caller overrides", () => {
const options = resolveCaptureSessionOptions(
{
width: 1920,
height: 1080,
fps: { num: 30, den: 1 },
format: "jpeg",
captureBeyondViewport: false,
},
"Chrome/149.0.7827.155",
"darwin",
);

expect(options.captureBeyondViewport).toBe(false);
});
});
22 changes: 18 additions & 4 deletions packages/engine/src/services/frameCapture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
getCdpSession,
pageScreenshotCapture,
initTransparentBackground,
shouldDefaultCaptureBeyondViewport,
} from "./screenshotService.js";
import { DEFAULT_CONFIG, type EngineConfig } from "../config.js";
import type {
Expand Down Expand Up @@ -310,6 +311,18 @@ export async function driveWarmupTicks(
}
}

export function resolveCaptureSessionOptions(
options: CaptureOptions,
browserVersion: string,
platform: NodeJS.Platform = process.platform,
): CaptureOptions {
return {
...options,
captureBeyondViewport:
options.captureBeyondViewport ?? shouldDefaultCaptureBeyondViewport(browserVersion, platform),
};
}

async function waitForCloseWithTimeout(promise: Promise<unknown>): Promise<boolean> {
let timedOut = false;
let timer: ReturnType<typeof setTimeout> | undefined;
Expand Down Expand Up @@ -417,6 +430,7 @@ export async function createCaptureSession(
}, variablesJson);
}
const browserVersion = await browser.version();
const sessionOptions = resolveCaptureSessionOptions(options, browserVersion);
const expectedMajor = config?.expectedChromiumMajor;
if (Number.isFinite(expectedMajor)) {
const actualChromiumMajor = Number.parseInt(
Expand All @@ -430,9 +444,9 @@ export async function createCaptureSession(
}
}
const viewport: Viewport = {
width: options.width,
height: options.height,
deviceScaleFactor: options.deviceScaleFactor || 1,
width: sessionOptions.width,
height: sessionOptions.height,
deviceScaleFactor: sessionOptions.deviceScaleFactor || 1,
};
await page.setViewport(viewport);

Expand All @@ -446,7 +460,7 @@ export async function createCaptureSession(
return {
browser,
page,
options,
options: sessionOptions,
serverUrl,
outputDir,
onBeforeCapture,
Expand Down
17 changes: 17 additions & 0 deletions packages/engine/src/services/screenshotService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
cdpSessionCache,
injectVideoFramesBatch,
syncVideoFrameVisibility,
shouldDefaultCaptureBeyondViewport,
} from "./screenshotService.js";

// Stub a Page + CDPSession just enough that pageScreenshotCapture can call
Expand Down Expand Up @@ -121,6 +122,22 @@ describe("pageScreenshotCapture supersample plumbing", () => {
});
});

describe("shouldDefaultCaptureBeyondViewport", () => {
it("guards regular Chrome on macOS", () => {
expect(shouldDefaultCaptureBeyondViewport("Chrome/149.0.7827.155", "darwin")).toBe(true);
});

it("keeps chrome-headless-shell on the faster viewport-bound path", () => {
expect(shouldDefaultCaptureBeyondViewport("HeadlessChrome/148.0.7778.97", "darwin")).toBe(
false,
);
});

it("does not change regular Chrome defaults on non-macOS platforms", () => {
expect(shouldDefaultCaptureBeyondViewport("Chrome/149.0.7827.155", "linux")).toBe(false);
});
});

describe("injectVideoFramesBatch replacement layout", () => {
it("does not copy opposing inset constraints onto the injected frame image", async () => {
const { window, document } = parseHTML(
Expand Down
11 changes: 11 additions & 0 deletions packages/engine/src/services/screenshotService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,17 @@ export async function getCdpSession(page: Page): Promise<import("puppeteer-core"
return client;
}

export function shouldDefaultCaptureBeyondViewport(
browserVersion: string,
platform: NodeJS.Platform = process.platform,
): boolean {
// Regular Chrome's viewport-bound screenshot path can expose a compositor
// surface shorter than the page viewport on affected macOS builds. In that
// case Chrome fills the clipped area with the page background. Headless shell
// reports as HeadlessChrome and keeps the faster viewport-bound path.
return platform === "darwin" && browserVersion.startsWith("Chrome/");
}

/**
* BeginFrame result with screenshot data and damage detection.
*/
Expand Down
9 changes: 5 additions & 4 deletions packages/engine/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,11 @@ export interface CaptureOptions {
quality?: number;
deviceScaleFactor?: number;
/**
* Opt into Chrome's capture-beyond-viewport screenshot path. Keep this off
* for ordinary viewport-sized captures because it is substantially slower in
* Chrome's screenshot compositor path. Enable for known compositor edge cases
* such as native video surfaces in tall portrait renders.
* Opt into Chrome's capture-beyond-viewport screenshot path. Leave undefined
* to let the engine pick the safe browser-specific default. Pass false only
* when the caller explicitly wants Chrome's faster viewport-bound path.
* Enable for known compositor edge cases such as native video surfaces in
* tall portrait renders.
*/
captureBeyondViewport?: boolean;
/**
Expand Down
2 changes: 1 addition & 1 deletion packages/producer/src/services/distributed/renderChunk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,7 @@ export async function renderChunk(
// declare `data-composition-variables` leave this undefined and the
// engine skips the `evaluateOnNewDocument` injection.
variables: encoder.variables,
captureBeyondViewport: (planVideos?.videos.length ?? 0) > 0,
...((planVideos?.videos.length ?? 0) > 0 ? { captureBeyondViewport: true } : {}),
// lock the BeginFrame warmup loop to a fixed iteration count so
// `beginFrameTimeTicks` is host-independent. Only chunks ever set this.
lockWarmupTicks: true,
Expand Down
7 changes: 6 additions & 1 deletion packages/producer/src/services/render/stages/captureStage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ export interface CaptureStageResult {
probeSession: CaptureSession | null;
/** Browser console buffer from whichever session was active last. */
lastBrowserConsole: string[];
/** Engine-resolved screenshot flag from the consumed sequential/probe session, when observed. */
captureBeyondViewport?: boolean;
}

export async function runCaptureStage(input: CaptureStageInput): Promise<CaptureStageResult> {
Expand All @@ -150,6 +152,7 @@ export async function runCaptureStage(input: CaptureStageInput): Promise<Capture
} = input;
let { workerCount, probeSession } = input;
let lastBrowserConsole: string[] = [];
let captureBeyondViewport: boolean | undefined = probeSession?.options.captureBeyondViewport;

// Derive a local cfg view rather than reading `forceScreenshot` from the
// caller-owned `cfg`. The sequencer threads the resolved value via the
Expand Down Expand Up @@ -228,6 +231,7 @@ export async function runCaptureStage(input: CaptureStageInput): Promise<Capture
workerCount = lastAttempt.workers;
}
if (probeSession) {
captureBeyondViewport = probeSession.options.captureBeyondViewport;
lastBrowserConsole = probeSession.browserConsoleBuffer;
await closeCaptureSession(probeSession);
probeSession = null;
Expand All @@ -245,6 +249,7 @@ export async function runCaptureStage(input: CaptureStageInput): Promise<Capture
videoInjector,
captureCfg,
));
captureBeyondViewport = session.options.captureBeyondViewport;
if (probeSession) {
prepareCaptureSessionForReuse(session, framesDir, videoInjector);
probeSession = null;
Expand Down Expand Up @@ -304,5 +309,5 @@ export async function runCaptureStage(input: CaptureStageInput): Promise<Capture
}
}

return { workerCount, probeSession, lastBrowserConsole };
return { workerCount, probeSession, lastBrowserConsole, captureBeyondViewport };
}
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ export type CaptureStreamingStageResult =
probeSession: CaptureSession | null;
lastBrowserConsole: string[];
workerCount: number;
/** Engine-resolved screenshot flag from the consumed sequential/probe session, when observed. */
captureBeyondViewport?: boolean;
}
| {
/** Spawn failed (non-abort) — sequencer should fall back to the disk path. */
Expand Down Expand Up @@ -156,6 +158,7 @@ export async function runCaptureStreamingStage(
} = input;
let { workerCount, probeSession } = input;
let lastBrowserConsole: string[] = [];
let captureBeyondViewport: boolean | undefined = probeSession?.options.captureBeyondViewport;

// Derive a local cfg view rather than reading `forceScreenshot` from the
// caller-owned `cfg`. The sequencer threads the resolved value via the
Expand Down Expand Up @@ -241,6 +244,7 @@ export async function runCaptureStreamingStage(
pushWorkerDedupPerfs(workerResults, dedupPerfs);

if (probeSession) {
captureBeyondViewport = probeSession.options.captureBeyondViewport;
lastBrowserConsole = probeSession.browserConsoleBuffer;
await closeCaptureSession(probeSession);
probeSession = null;
Expand All @@ -258,6 +262,7 @@ export async function runCaptureStreamingStage(
videoInjector,
captureCfg,
));
captureBeyondViewport = session.options.captureBeyondViewport;
if (probeSession) {
prepareCaptureSessionForReuse(session, framesDir, videoInjector);
probeSession = null;
Expand Down Expand Up @@ -325,6 +330,7 @@ export async function runCaptureStreamingStage(
probeSession,
lastBrowserConsole,
workerCount,
captureBeyondViewport,
};
} finally {
// Defensive cleanup: if the streaming branch threw before
Expand Down
26 changes: 21 additions & 5 deletions packages/producer/src/services/renderOrchestrator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1074,6 +1074,10 @@ export async function executeRenderJob(
fileServer = probeResult.fileServer;
probeSession = probeResult.probeSession;
lastBrowserConsole = probeResult.lastBrowserConsole;
let resolvedCaptureBeyondViewport = probeSession?.options.captureBeyondViewport;
if (resolvedCaptureBeyondViewport !== undefined) {
updateCaptureObservability({ captureBeyondViewport: resolvedCaptureBeyondViewport });
}
// The probe stage produces `duration` / `totalFrames` values; the
// sequencer owns the `RenderJob` and writes them onto it.
job.duration = probeResult.duration;
Expand Down Expand Up @@ -1213,11 +1217,13 @@ export async function executeRenderJob(
quality: needsAlpha ? undefined : job.config.quality === "draft" ? 80 : 95,
variables: job.config.variables,
deviceScaleFactor,
captureBeyondViewport: composition.videos.length > 0,
...(composition.videos.length > 0 ? { captureBeyondViewport: true } : {}),
};
updateCaptureObservability({
captureBeyondViewport: captureOptions.captureBeyondViewport ?? false,
});
resolvedCaptureBeyondViewport =
captureOptions.captureBeyondViewport ?? resolvedCaptureBeyondViewport;
if (resolvedCaptureBeyondViewport !== undefined) {
updateCaptureObservability({ captureBeyondViewport: resolvedCaptureBeyondViewport });
}

// Capture sessions do not need native browser metadata for videos whose
// pixels come from out-of-band FFmpeg frame extraction. Waiting on those
Expand Down Expand Up @@ -1442,7 +1448,7 @@ export async function executeRenderJob(
observability.checkpoint("capture_strategy", "resolved", {
workerCount,
forceScreenshot: captureForceScreenshot,
captureBeyondViewport: captureOptions.captureBeyondViewport ?? false,
captureBeyondViewport: resolvedCaptureBeyondViewport ?? null,
useStreamingEncode,
useLayeredComposite,
usePageSideCompositing: usePageSideCompositingForTransitions,
Expand Down Expand Up @@ -1592,6 +1598,11 @@ export async function executeRenderJob(
streamingHandled = true;
workerCount = streamingRes.workerCount;
updateCaptureObservability({ workerCount });
if (streamingRes.captureBeyondViewport !== undefined) {
updateCaptureObservability({
captureBeyondViewport: streamingRes.captureBeyondViewport,
});
}
probeSession = streamingRes.probeSession;
lastBrowserConsole = streamingRes.lastBrowserConsole;
perfStages.captureMs = Date.now() - stage4Start;
Expand Down Expand Up @@ -1637,6 +1648,11 @@ export async function executeRenderJob(
const captureFrameMs = Date.now() - captureFrameStart;
workerCount = captureRes.workerCount;
updateCaptureObservability({ workerCount });
if (captureRes.captureBeyondViewport !== undefined) {
updateCaptureObservability({
captureBeyondViewport: captureRes.captureBeyondViewport,
});
}
probeSession = captureRes.probeSession;
lastBrowserConsole = captureRes.lastBrowserConsole;

Expand Down
13 changes: 13 additions & 0 deletions packages/producer/tests/chrome-screenshot-bottom-edge/meta.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"name": "chrome-screenshot-bottom-edge",
"description": "Regression guard for screenshot capture leaking page background into the bottom of a viewport-sized MP4. The composition paints a red page background below full-frame content so any bottom-edge capture gap collapses PSNR.",
"tags": ["regression", "screenshot", "edge"],
"minPsnr": 30,
"maxFrameFailures": 0,
"minAudioCorrelation": 0,
"maxAudioLagWindows": 1,
"renderConfig": {
"fps": 12,
"workers": 1
}
}

Large diffs are not rendered by default.

Git LFS file not shown
Loading
Loading