From 453bd6e313c39b8b8cae9f47b4776ffd5b55615b Mon Sep 17 00:00:00 2001 From: James Date: Thu, 7 May 2026 05:47:27 +0000 Subject: [PATCH] fix(engine,producer): reject alpha+supersample, unit-test clip plumbing --- .../src/services/screenshotService.test.ts | 92 +++++++++++++++++++ .../src/services/renderOrchestrator.test.ts | 23 +++-- .../src/services/renderOrchestrator.ts | 10 ++ 3 files changed, 119 insertions(+), 6 deletions(-) create mode 100644 packages/engine/src/services/screenshotService.test.ts diff --git a/packages/engine/src/services/screenshotService.test.ts b/packages/engine/src/services/screenshotService.test.ts new file mode 100644 index 000000000..b0f9f36c1 --- /dev/null +++ b/packages/engine/src/services/screenshotService.test.ts @@ -0,0 +1,92 @@ +// @vitest-environment node +import { describe, it, expect, vi } from "vitest"; +import { type Page } from "puppeteer-core"; +import { pageScreenshotCapture, cdpSessionCache } from "./screenshotService.js"; + +// Stub a Page + CDPSession just enough that pageScreenshotCapture can call +// `client.send("Page.captureScreenshot", ...)` and we can inspect the args. +function makeFakePageWithCdp(send: (method: string, params: object) => Promise<{ data: string }>) { + const fakeSession = { send } as unknown as import("puppeteer-core").CDPSession; + // Stub a Page object — the WeakMap cache is the only Page-thing used in the + // path under test, so we can pre-seed it and skip page.createCDPSession(). + const fakePage = {} as Page; + cdpSessionCache.set(fakePage, fakeSession); + return fakePage; +} + +describe("pageScreenshotCapture supersample plumbing", () => { + // Minimal 1×1 transparent PNG, base64. The function returns Buffer.from(data, "base64") + // and we never inspect the bytes — only the params we pass to client.send. + const ONE_PIXEL_PNG_B64 = + "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNkAAIAAAoAAv/lxKUAAAAASUVORK5CYII="; + + it("omits `clip` when deviceScaleFactor is undefined (default 1)", async () => { + const send = vi.fn().mockResolvedValue({ data: ONE_PIXEL_PNG_B64 }); + const page = makeFakePageWithCdp(send); + + await pageScreenshotCapture(page, { + width: 1920, + height: 1080, + fps: 30, + format: "jpeg", + quality: 80, + }); + + expect(send).toHaveBeenCalledWith( + "Page.captureScreenshot", + expect.not.objectContaining({ clip: expect.anything() }), + ); + }); + + it("omits `clip` when deviceScaleFactor is exactly 1", async () => { + const send = vi.fn().mockResolvedValue({ data: ONE_PIXEL_PNG_B64 }); + const page = makeFakePageWithCdp(send); + + await pageScreenshotCapture(page, { + width: 1920, + height: 1080, + fps: 30, + format: "jpeg", + deviceScaleFactor: 1, + }); + + const params = send.mock.calls[0]?.[1] as { clip?: unknown }; + expect(params.clip).toBeUndefined(); + }); + + it("passes `clip` with `scale = dpr` when deviceScaleFactor > 1 (the supersample contract)", async () => { + const send = vi.fn().mockResolvedValue({ data: ONE_PIXEL_PNG_B64 }); + const page = makeFakePageWithCdp(send); + + await pageScreenshotCapture(page, { + width: 1920, + height: 1080, + fps: 30, + format: "jpeg", + deviceScaleFactor: 2, + }); + + expect(send).toHaveBeenCalledWith( + "Page.captureScreenshot", + expect.objectContaining({ + clip: { x: 0, y: 0, width: 1920, height: 1080, scale: 2 }, + }), + ); + }); + + it("propagates a non-2 supersample factor (e.g. 720p → 4K = 3×)", async () => { + const send = vi.fn().mockResolvedValue({ data: ONE_PIXEL_PNG_B64 }); + const page = makeFakePageWithCdp(send); + + await pageScreenshotCapture(page, { + width: 1280, + height: 720, + fps: 30, + format: "jpeg", + deviceScaleFactor: 3, + }); + + const params = send.mock.calls[0]?.[1] as { clip?: { scale: number } }; + expect(params.clip?.scale).toBe(3); + }); +}); diff --git a/packages/producer/src/services/renderOrchestrator.test.ts b/packages/producer/src/services/renderOrchestrator.test.ts index 587004c04..3dc0b6d59 100644 --- a/packages/producer/src/services/renderOrchestrator.test.ts +++ b/packages/producer/src/services/renderOrchestrator.test.ts @@ -756,6 +756,7 @@ describe("resolveDeviceScaleFactor", () => { compositionWidth: 1920, compositionHeight: 1080, hdrRequested: false, + alphaRequested: false, } as const; it("returns 1 when no outputResolution is set (default behavior)", () => { @@ -780,10 +781,10 @@ describe("resolveDeviceScaleFactor", () => { it("returns 1 when the composition already matches the requested resolution", () => { expect( resolveDeviceScaleFactor({ + ...defaults, compositionWidth: 3840, compositionHeight: 2160, outputResolution: "landscape-4k", - hdrRequested: false, }), ).toBe(1); }); @@ -798,6 +799,16 @@ describe("resolveDeviceScaleFactor", () => { ).toThrow(/hdrMode='force-hdr'/); }); + it("rejects alpha + outputResolution (the alpha capture path doesn't apply DPR yet)", () => { + expect(() => + resolveDeviceScaleFactor({ + ...defaults, + outputResolution: "landscape-4k", + alphaRequested: true, + }), + ).toThrow(/alpha output/); + }); + it("rejects orientation mismatch (landscape comp → portrait-4k)", () => { expect(() => resolveDeviceScaleFactor({ ...defaults, outputResolution: "portrait-4k" }), @@ -807,24 +818,24 @@ describe("resolveDeviceScaleFactor", () => { it("rejects downsampling (4K composition → 1080p output)", () => { expect(() => resolveDeviceScaleFactor({ + ...defaults, compositionWidth: 3840, compositionHeight: 2160, outputResolution: "landscape", - hdrRequested: false, }), ).toThrow(/Downsampling/); }); it("rejects non-integer scale factors", () => { - // 1280×720 → 3840×2160 would be 3×, but width 1280 → 3840 is also 3× — that's actually integer. - // Use 1280×720 → 2160×3840 (mismatched orientation triggers aspect first), so use a real - // non-integer: 1500×844 → 3840×2160 = 2.56×. + // 1500×844 → 3840×2160 has slightly different ratios in width vs height. + // The aspect-ratio guard fires first; pinning the rejection message + // covers both error paths since either is an acceptable failure here. expect(() => resolveDeviceScaleFactor({ + ...defaults, compositionWidth: 1500, compositionHeight: 844, outputResolution: "landscape-4k", - hdrRequested: false, }), ).toThrow(/aspect ratio|non-integer/); }); diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts index 2bff02c9d..c7026c55d 100644 --- a/packages/producer/src/services/renderOrchestrator.ts +++ b/packages/producer/src/services/renderOrchestrator.ts @@ -594,6 +594,7 @@ export function resolveDeviceScaleFactor(input: { compositionHeight: number; outputResolution: CanvasResolution | undefined; hdrRequested: boolean; + alphaRequested: boolean; }): number { if (!input.outputResolution) return 1; if (input.hdrRequested) { @@ -603,6 +604,14 @@ export function resolveDeviceScaleFactor(input: { "support supersampling. Pick one or render in two passes.", ); } + if (input.alphaRequested) { + throw new Error( + "outputResolution cannot be combined with alpha output (--format webm|mov|png-sequence). " + + "The alpha screenshot path does not yet apply deviceScaleFactor and would silently " + + "produce composition-resolution frames. Render alpha at composition resolution and " + + "upscale separately, or use --format mp4.", + ); + } const target = CANVAS_DIMENSIONS[input.outputResolution]; // Aspect-ratio compare via cross-multiplication so the equality is integer- // safe. Float division (`target.width / compositionWidth`) loses precision @@ -2131,6 +2140,7 @@ export async function executeRenderJob( compositionHeight: height, outputResolution: job.config.outputResolution, hdrRequested: job.config.hdrMode === "force-hdr", + alphaRequested: needsAlpha, }); const outputWidth = width * deviceScaleFactor; const outputHeight = height * deviceScaleFactor;