From b44826803ed0cf6e3a607a3539014e622b6b5c94 Mon Sep 17 00:00:00 2001 From: "mykola.gervasyuk" Date: Mon, 31 Aug 2026 14:13:39 +0300 Subject: [PATCH 1/2] perf: sign matching variations on the worker pool, not the event loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opening "Approve variations" on a build with many locales took ~20s and blocked every other request while it ran. Finding the siblings that carry the same change signed the reviewed run and each sibling one after another, and each signature synchronously decoded two full-size PNGs on the main thread — 66 decodes back to back for a screen with 33 variations. The signature now runs as a job on the existing diff worker pool, next to the pixelmatch diff it already carried: the pool's job type became a union discriminated on `kind`, and both the worker and its inline fallback go through one dispatcher so the two paths cannot answer differently. The undecoded bytes travel to the worker, so no screenshot is decoded on the event loop any more. On top of that: - The reviewed run and its siblings are signed in one fan-out, bounded to 8 in flight so a 100-locale screen cannot flood the queue that build ingestion shares. - The free checks run first. A sibling whose change is far larger, or that has no change at all, is answered from its diffPercent and never costs a decode. The skipped list is put back in the siblings' own order so the dialog does not reorder by which check rejected what. - Signatures are memoized per image pair, ignore areas and diff config, so reopening the dialog or stepping back to a screen is free. Bounded to 2000 entries; a failed read is not remembered. The histogram, downscale and cosine similarity moved to signature.core.ts unchanged, and are now covered directly — position independence, ignore areas and dimension mismatch included. --- src/compare/compare.service.ts | 15 +- src/compare/diff-worker-pool.spec.ts | 57 ++++ src/compare/diff-worker-pool.ts | 29 +- .../libs/pixelmatch/pixelmatch.core.ts | 1 + .../libs/pixelmatch/pixelmatch.service.ts | 1 + .../libs/pixelmatch/pixelmatch.worker.ts | 6 +- .../libs/pixelmatch/signature.core.spec.ts | 78 ++++++ src/compare/libs/pixelmatch/signature.core.ts | 148 +++++++++++ src/compare/libs/pixelmatch/worker-job.ts | 14 + src/test-runs/test-runs.service.spec.ts | 214 +++++++++++---- src/test-runs/test-runs.service.ts | 247 ++++++++++-------- 11 files changed, 634 insertions(+), 176 deletions(-) create mode 100644 src/compare/diff-worker-pool.spec.ts create mode 100644 src/compare/libs/pixelmatch/signature.core.spec.ts create mode 100644 src/compare/libs/pixelmatch/signature.core.ts create mode 100644 src/compare/libs/pixelmatch/worker-job.ts diff --git a/src/compare/compare.service.ts b/src/compare/compare.service.ts index f7895e9f..f3139b8b 100644 --- a/src/compare/compare.service.ts +++ b/src/compare/compare.service.ts @@ -9,6 +9,8 @@ import { LookSameService } from './libs/looks-same/looks-same.service'; import { OdiffService } from './libs/odiff/odiff.service'; import { VlmService } from './libs/vlm/vlm.service'; import { isHddStaticServiceConfigured } from '../static/utils'; +import { DiffWorkerPool } from './diff-worker-pool'; +import { SignatureJobInput, SignatureJobOutput } from './libs/pixelmatch/signature.core'; @Injectable() export class CompareService { @@ -19,7 +21,8 @@ export class CompareService { private readonly lookSameService: LookSameService, private readonly odiffService: OdiffService, private readonly vlmService: VlmService, - private readonly prismaService: PrismaService + private readonly prismaService: PrismaService, + private readonly diffWorkerPool: DiffWorkerPool ) {} async getDiff({ projectId, data }: { projectId: string; data: ImageCompareInput }): Promise { @@ -29,6 +32,16 @@ export class CompareService { return comparator.getDiff(data, config); } + /** + * Position-independent signature of what changed between a baseline and a + * screenshot, used to tell whether two screens carry the same change. Runs on + * the diff worker pool: decoding a pair of full-size screenshots takes long + * enough that doing it on the event loop stalls every other request. + */ + async getChangeSignature(input: Omit): Promise { + return this.diffWorkerPool.run({ kind: 'signature', ...input }); + } + getComparator(imageComparison: ImageComparison): ImageComparator { switch (imageComparison) { case ImageComparison.pixelmatch: { diff --git a/src/compare/diff-worker-pool.spec.ts b/src/compare/diff-worker-pool.spec.ts new file mode 100644 index 00000000..5ed908cb --- /dev/null +++ b/src/compare/diff-worker-pool.spec.ts @@ -0,0 +1,57 @@ +import { PNG } from 'pngjs'; +import { DiffWorkerPool } from './diff-worker-pool'; + +// Opaque throughout: pixelmatch composites transparent pixels onto white, so a +// fully transparent image would read as equal to a white one. +const png = (width: number, height: number, rgb: number): Buffer => { + const image = new PNG({ width, height }); + for (let i = 0; i < width * height; i++) { + image.data[i * 4] = rgb; + image.data[i * 4 + 1] = rgb; + image.data[i * 4 + 2] = rgb; + image.data[i * 4 + 3] = 255; + } + return PNG.sync.write(image); +}; + +describe('DiffWorkerPool', () => { + let pool: DiffWorkerPool; + + beforeEach(() => { + pool = new DiffWorkerPool(); + }); + + afterEach(async () => { + await pool.onModuleDestroy(); + }); + + it('answers a diff job with the pixel mismatch it found', async () => { + const output = await pool.run({ + kind: 'diff', + baseline: png(10, 10, 0), + image: png(10, 10, 255), + ignoreAreas: [], + threshold: 0.1, + includeAA: false, + allowDiffDimensions: false, + diffTolerancePercent: 0, + saveDiff: false, + }); + + expect(output).toMatchObject({ equal: false, pixelMisMatchCount: 100 }); + }); + + it('answers a signature job with a change signature', async () => { + const output = await pool.run({ + kind: 'signature', + baseline: png(10, 10, 0), + image: png(10, 10, 255), + ignoreAreas: [], + threshold: 0.1, + includeAA: false, + }); + + expect(output.signature).toHaveLength(64); + expect(output.signature.reduce((sum, value) => sum + value, 0)).toBeCloseTo(1); + }); +}); diff --git a/src/compare/diff-worker-pool.ts b/src/compare/diff-worker-pool.ts index 20324d45..cf92f21a 100644 --- a/src/compare/diff-worker-pool.ts +++ b/src/compare/diff-worker-pool.ts @@ -3,7 +3,9 @@ import { Worker } from 'worker_threads'; import { availableParallelism } from 'os'; import { existsSync } from 'fs'; import { join } from 'path'; -import { computePixelmatchDiff, PixelmatchJobInput, PixelmatchJobOutput } from './libs/pixelmatch/pixelmatch.core'; +import { PixelmatchJobInput, PixelmatchJobOutput } from './libs/pixelmatch/pixelmatch.core'; +import { SignatureJobInput, SignatureJobOutput } from './libs/pixelmatch/signature.core'; +import { runWorkerJob, WorkerJobInput, WorkerJobOutput } from './libs/pixelmatch/worker-job'; const WORKER_FILE = join(__dirname, 'libs', 'pixelmatch', 'pixelmatch.worker.js'); @@ -13,17 +15,18 @@ const MAX_SPAWN_FAILURES = 3; const MAX_JOB_ATTEMPTS = 2; interface Job { - input: PixelmatchJobInput; - resolve: (output: PixelmatchJobOutput) => void; + input: WorkerJobInput; + resolve: (output: WorkerJobOutput) => void; reject: (error: Error) => void; attempts: number; } /** - * Fixed pool of worker threads for CPU-bound image diffing. Keeps the event - * loop free during build ingestion so the API stays responsive while - * screenshots are compared. Pool size: DIFF_WORKERS_COUNT env var, defaulting - * to cores - 1 (capped) so the main thread always has a core left. + * Fixed pool of worker threads for CPU-bound image work — diffing a screenshot + * against its baseline, and the change signatures the variations dialog + * compares. Keeps the event loop free so the API stays responsive while + * screenshots are decoded and compared. Pool size: DIFF_WORKERS_COUNT env var, + * defaulting to cores - 1 (capped) so the main thread always has a core left. */ @Injectable() export class DiffWorkerPool implements OnModuleDestroy { @@ -46,9 +49,11 @@ export class DiffWorkerPool implements OnModuleDestroy { private destroyed = false; private spawnFailures = 0; - async run(input: PixelmatchJobInput): Promise { + async run(input: PixelmatchJobInput): Promise; + async run(input: SignatureJobInput): Promise; + async run(input: WorkerJobInput): Promise { if (this.inline) { - return computePixelmatchDiff(input); + return runWorkerJob(input); } if (this.destroyed) { throw new Error('Image diff worker pool is shut down'); @@ -57,7 +62,7 @@ export class DiffWorkerPool implements OnModuleDestroy { throw new Error(`Image diff queue is full (${this.queueLimit} jobs)`); } this.start(); - return new Promise((resolve, reject) => { + return new Promise((resolve, reject) => { this.queue.push({ input, resolve, reject, attempts: 0 }); this.dispatch(); }); @@ -74,7 +79,7 @@ export class DiffWorkerPool implements OnModuleDestroy { private spawn(): void { const worker = new Worker(WORKER_FILE); - worker.on('message', (output: PixelmatchJobOutput & { error?: string }) => { + worker.on('message', (output: WorkerJobOutput & { error?: string }) => { const job = this.inFlight.get(worker); this.inFlight.delete(worker); // a message can arrive after the worker was dropped or the pool shut @@ -144,7 +149,7 @@ export class DiffWorkerPool implements OnModuleDestroy { this.queue = []; for (const job of queued) { try { - job.resolve(computePixelmatchDiff(job.input)); + job.resolve(runWorkerJob(job.input)); } catch (error) { job.reject(error instanceof Error ? error : new Error(String(error))); } diff --git a/src/compare/libs/pixelmatch/pixelmatch.core.ts b/src/compare/libs/pixelmatch/pixelmatch.core.ts index ef765dcd..47bbc71e 100644 --- a/src/compare/libs/pixelmatch/pixelmatch.core.ts +++ b/src/compare/libs/pixelmatch/pixelmatch.core.ts @@ -11,6 +11,7 @@ import { applyIgnoreAreas, scaleImageToSize } from '../../utils'; * build ingestion. All input/output must stay structured-clone serializable. */ export interface PixelmatchJobInput { + kind: 'diff'; baseline: Buffer | Uint8Array; image: Buffer | Uint8Array; ignoreAreas: IgnoreAreaDto[]; diff --git a/src/compare/libs/pixelmatch/pixelmatch.service.ts b/src/compare/libs/pixelmatch/pixelmatch.service.ts index 8a47df89..0cb9c800 100644 --- a/src/compare/libs/pixelmatch/pixelmatch.service.ts +++ b/src/compare/libs/pixelmatch/pixelmatch.service.ts @@ -36,6 +36,7 @@ export class PixelmatchService implements ImageComparator { // decode + pixelmatch + diff encode run off the event loop const output = await this.diffWorkerPool.run({ + kind: 'diff', baseline: baselineBuffer, image: imageBuffer, ignoreAreas: data.ignoreAreas, diff --git a/src/compare/libs/pixelmatch/pixelmatch.worker.ts b/src/compare/libs/pixelmatch/pixelmatch.worker.ts index 2434d105..67340249 100644 --- a/src/compare/libs/pixelmatch/pixelmatch.worker.ts +++ b/src/compare/libs/pixelmatch/pixelmatch.worker.ts @@ -1,9 +1,9 @@ import { parentPort } from 'worker_threads'; -import { computePixelmatchDiff, PixelmatchJobInput } from './pixelmatch.core'; +import { runWorkerJob, WorkerJobInput } from './worker-job'; -parentPort.on('message', (input: PixelmatchJobInput) => { +parentPort.on('message', (input: WorkerJobInput) => { try { - parentPort.postMessage(computePixelmatchDiff(input)); + parentPort.postMessage(runWorkerJob(input)); } catch (error) { parentPort.postMessage({ error: error instanceof Error ? error.message : String(error) }); } diff --git a/src/compare/libs/pixelmatch/signature.core.spec.ts b/src/compare/libs/pixelmatch/signature.core.spec.ts new file mode 100644 index 00000000..42029b4e --- /dev/null +++ b/src/compare/libs/pixelmatch/signature.core.spec.ts @@ -0,0 +1,78 @@ +import { PNG } from 'pngjs'; +import { computeChangeSignature } from './signature.core'; + +const WIDTH = 40; +const HEIGHT = 40; + +const png = (paint: (set: (x: number, y: number, rgb: [number, number, number]) => void) => void): Buffer => { + const image = new PNG({ width: WIDTH, height: HEIGHT }); + image.data.fill(255); + paint((x, y, [r, g, b]) => { + const index = (y * WIDTH + x) * 4; + image.data[index] = r; + image.data[index + 1] = g; + image.data[index + 2] = b; + image.data[index + 3] = 255; + }); + return PNG.sync.write(image); +}; + +const block = (left: number, top: number, rgb: [number, number, number]): Buffer => + png((set) => { + for (let y = top; y < top + 8; y++) { + for (let x = left; x < left + 8; x++) { + set(x, y, rgb); + } + } + }); + +const blank = png(() => undefined); + +const RED: [number, number, number] = [255, 0, 0]; +const BLUE: [number, number, number] = [0, 0, 255]; + +const signatureOf = (baseline: Buffer, image: Buffer, ignoreAreas = []): number[] | null => + computeChangeSignature({ + kind: 'signature', + baseline, + image, + ignoreAreas, + threshold: 0.1, + includeAA: false, + }).signature; + +describe('computeChangeSignature', () => { + it('has no signature when the images have different dimensions', () => { + const taller = new PNG({ width: WIDTH, height: HEIGHT * 2 }); + taller.data.fill(255); + + expect(signatureOf(blank, PNG.sync.write(taller))).toBeNull(); + }); + + it('has no signature when nothing changed', () => { + expect(signatureOf(blank, blank)).toBeNull(); + }); + + it('concentrates on the color the changed pixels took in the new image', () => { + const signature = signatureOf(blank, block(0, 0, RED)); + + const brightest = signature.indexOf(Math.max(...signature)); + // 4 buckets per channel: pure red is the last red bucket, first green/blue + expect(brightest).toBe(3 * 16); + expect(signature.reduce((sum, value) => sum + value, 0)).toBeCloseTo(1); + }); + + it('is the same wherever the change sits, so per-locale reflow still matches', () => { + expect(signatureOf(blank, block(0, 0, RED))).toEqual(signatureOf(blank, block(24, 28, RED))); + }); + + it('tells a different color of change apart', () => { + expect(signatureOf(blank, block(0, 0, RED))).not.toEqual(signatureOf(blank, block(0, 0, BLUE))); + }); + + it('has no signature when the only change is inside an ignore area', () => { + const ignoreAreas = [{ x: 0, y: 0, width: 16, height: 16 }]; + + expect(signatureOf(blank, block(4, 4, RED), ignoreAreas)).toBeNull(); + }); +}); diff --git a/src/compare/libs/pixelmatch/signature.core.ts b/src/compare/libs/pixelmatch/signature.core.ts new file mode 100644 index 00000000..c843f2d8 --- /dev/null +++ b/src/compare/libs/pixelmatch/signature.core.ts @@ -0,0 +1,148 @@ +import { PNG } from 'pngjs'; +import Pixelmatch from 'pixelmatch'; +import { IgnoreAreaDto } from '../../../test-runs/dto/ignore-area.dto'; +import { applyIgnoreAreas } from '../../utils'; + +/** + * CPU-bound part of "do these two screens carry the same change?": PNG decode, + * downscale, pixelmatch and histogram. Extracted so it can run inside a worker + * thread (see pixelmatch.worker.ts / DiffWorkerPool) — the variations dialog + * asks for one of these per sibling, and decoding full-size screenshots on the + * main thread froze the whole API for as long as it took. All input/output must + * stay structured-clone serializable. + */ +export interface SignatureJobInput { + kind: 'signature'; + // PNG bytes, not decoded pixels: decoding is the expensive part and belongs + // in the worker. + baseline: Buffer | Uint8Array; + image: Buffer | Uint8Array; + ignoreAreas: IgnoreAreaDto[]; + threshold: number; + includeAA: boolean; +} + +export interface SignatureJobOutput { + signature: number[] | null; +} + +// Colors are quantized to this many levels per RGB channel, giving +// COLOR_BUCKETS_PER_CHANNEL^3 histogram buckets. +const COLOR_BUCKETS_PER_CHANNEL = 4; + +// Longest side (px) images are downscaled to before computing the color +// signature — keeps the histogram representative while cutting pixelmatch cost. +const SIGNATURE_MAX_DIMENSION = 500; + +// Two changes are considered the same pattern when their color signatures' +// cosine similarity is at least this value. +export const SIGNATURE_SIMILARITY_THRESHOLD = 0.9; + +interface RawImage { + data: Buffer; + width: number; + height: number; +} + +// postMessage turns Buffers into Uint8Array views over their own ArrayBuffer. +function toBuffer(data: Buffer | Uint8Array): Buffer { + return Buffer.isBuffer(data) ? data : Buffer.from(data.buffer, data.byteOffset, data.byteLength); +} + +// Nearest-neighbour downscale so the longest side is at most maxDimension. +// Returns the original when already small enough. +function downscale(source: RawImage, maxDimension: number): RawImage { + const scale = maxDimension / Math.max(source.width, source.height); + if (scale >= 1) { + return source; + } + const width = Math.max(1, Math.round(source.width * scale)); + const height = Math.max(1, Math.round(source.height * scale)); + const data: Buffer = Buffer.alloc(width * height * 4); + for (let y = 0; y < height; y++) { + const sourceY = Math.min(source.height - 1, Math.floor(y / scale)); + for (let x = 0; x < width; x++) { + const sourceX = Math.min(source.width - 1, Math.floor(x / scale)); + const sourceIndex = (sourceY * source.width + sourceX) * 4; + const targetIndex = (y * width + x) * 4; + data[targetIndex] = source.data[sourceIndex]; + data[targetIndex + 1] = source.data[sourceIndex + 1]; + data[targetIndex + 2] = source.data[sourceIndex + 2]; + data[targetIndex + 3] = source.data[sourceIndex + 3]; + } + } + return { data, width, height }; +} + +/** + * Position-independent signature of a change: a normalized histogram of the + * colors that the changed pixels take in the new image. Because it ignores + * *where* the change is, it is robust to per-locale layout shifts (a title + * wrapping to a different number of lines, options moving down, etc.) while + * still capturing *what* changed (a selection highlight, a recolored button). + * Null when there is nothing to compare: unreadable bytes, differing + * dimensions, or no change outside the ignore areas. + */ +export function computeChangeSignature(input: SignatureJobInput): SignatureJobOutput { + const baselineImage = PNG.sync.read(toBuffer(input.baseline)); + const checkpointImage = PNG.sync.read(toBuffer(input.image)); + if (baselineImage.width !== checkpointImage.width || baselineImage.height !== checkpointImage.height) { + return { signature: null }; + } + + // Masked regions must not count toward the signature, so they are blanked at + // full resolution — the areas are given in full-resolution coordinates — + // before the downscale makes both images cheap to compare. + applyIgnoreAreas(baselineImage, input.ignoreAreas); + applyIgnoreAreas(checkpointImage, input.ignoreAreas); + + const baseline = downscale(baselineImage, SIGNATURE_MAX_DIMENSION); + const image = downscale(checkpointImage, SIGNATURE_MAX_DIMENSION); + const { width, height } = baseline; + const mask = new PNG({ width, height }); + const changedPixels = Pixelmatch(baseline.data, image.data, mask.data, width, height, { + threshold: input.threshold, + includeAA: input.includeAA, + diffMask: true, + }); + if (changedPixels === 0) { + return { signature: null }; + } + + const bucketSize = 256 / COLOR_BUCKETS_PER_CHANNEL; + const histogram = new Array(COLOR_BUCKETS_PER_CHANNEL ** 3).fill(0); + for (let i = 0; i < width * height; i++) { + if (mask.data[i * 4 + 3] === 0) { + continue; + } + const r = Math.min(COLOR_BUCKETS_PER_CHANNEL - 1, Math.floor(image.data[i * 4] / bucketSize)); + const g = Math.min(COLOR_BUCKETS_PER_CHANNEL - 1, Math.floor(image.data[i * 4 + 1] / bucketSize)); + const b = Math.min(COLOR_BUCKETS_PER_CHANNEL - 1, Math.floor(image.data[i * 4 + 2] / bucketSize)); + histogram[r * COLOR_BUCKETS_PER_CHANNEL * COLOR_BUCKETS_PER_CHANNEL + g * COLOR_BUCKETS_PER_CHANNEL + b]++; + } + + const total = histogram.reduce((sum, value) => sum + value, 0); + if (total === 0) { + return { signature: null }; + } + return { signature: histogram.map((value) => value / total) }; +} + +function cosineSimilarity(a: number[], b: number[]): number { + let dot = 0; + let normA = 0; + let normB = 0; + for (let i = 0; i < a.length; i++) { + dot += a[i] * b[i]; + normA += a[i] * a[i]; + normB += b[i] * b[i]; + } + if (normA === 0 || normB === 0) { + return 0; + } + return dot / (Math.sqrt(normA) * Math.sqrt(normB)); +} + +export function signaturesMatch(a: number[], b: number[]): boolean { + return cosineSimilarity(a, b) >= SIGNATURE_SIMILARITY_THRESHOLD; +} diff --git a/src/compare/libs/pixelmatch/worker-job.ts b/src/compare/libs/pixelmatch/worker-job.ts new file mode 100644 index 00000000..73f23426 --- /dev/null +++ b/src/compare/libs/pixelmatch/worker-job.ts @@ -0,0 +1,14 @@ +import { computePixelmatchDiff, PixelmatchJobInput, PixelmatchJobOutput } from './pixelmatch.core'; +import { computeChangeSignature, SignatureJobInput, SignatureJobOutput } from './signature.core'; + +/** + * The jobs {@link DiffWorkerPool} runs off the event loop, discriminated by + * `kind`. Both the worker thread and the pool's inline fallback go through + * {@link runWorkerJob}, so the two paths cannot answer a job differently. + */ +export type WorkerJobInput = PixelmatchJobInput | SignatureJobInput; +export type WorkerJobOutput = PixelmatchJobOutput | SignatureJobOutput; + +export function runWorkerJob(input: WorkerJobInput): WorkerJobOutput { + return input.kind === 'signature' ? computeChangeSignature(input) : computePixelmatchDiff(input); +} diff --git a/src/test-runs/test-runs.service.spec.ts b/src/test-runs/test-runs.service.spec.ts index 009c8da4..57d2da60 100644 --- a/src/test-runs/test-runs.service.spec.ts +++ b/src/test-runs/test-runs.service.spec.ts @@ -1,6 +1,6 @@ import { mocked } from 'jest-mock'; import { Test, TestingModule } from '@nestjs/testing'; -import { TestRunsService } from './test-runs.service'; +import { SIGNATURE_CONCURRENCY, TestRunsService } from './test-runs.service'; import { PrismaService } from '../prisma/prisma.service'; import { StaticService } from '../static/static.service'; import { TestStatus, TestRun, TestVariation } from '@prisma/client'; @@ -44,6 +44,8 @@ const initService = async ({ testVariationFindUniqueMock = jest.fn(), projectFindUniqueMock = jest.fn(), compareGetDiffMock = jest.fn(), + compareGetChangeSignatureMock = jest.fn(), + getImageBufferMock = jest.fn(), }) => { const module: TestingModule = await Test.createTestingModule({ providers: [ @@ -76,6 +78,7 @@ const initService = async ({ provide: StaticService, useValue: { getImage: getImageMock, + getImageBuffer: getImageBufferMock, saveImage: saveImageMock, deleteImage: deleteImageMock, }, @@ -106,6 +109,7 @@ const initService = async ({ provide: CompareService, useValue: { getDiff: compareGetDiffMock, + getChangeSignature: compareGetChangeSignatureMock, }, }, ], @@ -706,61 +710,181 @@ describe('TestRunsService', () => { }); describe('getMatchingVariations', () => { - it('matches same-palette, same-size siblings and skips different pattern / far larger change', async () => { - const testRun = generateTestRun({ - id: 'target', + const SAME_PALETTE = [1, 0]; + const OTHER_PALETTE = [0, 1]; + + // A sibling of the reviewed screen. imageName doubles as the run's identity + // in the signature mocks below, which see buffers rather than runs. + const sibling = (id: string, overrides: Partial = {}): TestRun => + generateTestRun({ + id, status: TestStatus.unresolved, name: 'Screen A', + baselineName: `${id}.baseline.png`, + imageName: `${id}.screenshot.png`, + customTags: id, diffPercent: 12, + ...overrides, }); - const matching = generateTestRun({ - id: 'match', - status: TestStatus.unresolved, - name: 'Screen A', - customTags: 'locale-a', - diffPercent: 12, + + // The pool takes PNG bytes, so the fake static service hands back the image + // name as its own content and the signature mock reads the name back out. + const initMatchingService = async ({ + testRun, + siblings, + signatureOf, + }: { + testRun: TestRun; + siblings: TestRun[]; + signatureOf: (imageName: string) => Promise | number[] | null; + }) => { + const compareGetChangeSignatureMock = jest.fn().mockImplementation(async (input) => ({ + signature: await signatureOf(input.image.toString()), + })); + const built = await initService({ + testRunFindUniqueMock: jest.fn().mockResolvedValue({ ...testRun, testVariation: generateTestVariation() }), + testRunFindManyMock: jest.fn().mockResolvedValue(siblings), + projectFindUniqueMock: jest + .fn() + .mockResolvedValue({ bulkApproveVariations: true, bulkApproveGroupBy: 'customTags' }), + getImageBufferMock: jest.fn().mockImplementation(async (name: string) => Buffer.from(name)), + compareGetChangeSignatureMock, }); - const bigger = generateTestRun({ - id: 'bigger', - status: TestStatus.unresolved, - name: 'Screen A', - customTags: 'locale-b', - diffPercent: 30, + return { service: built, compareGetChangeSignatureMock }; + }; + + it('matches same-palette, same-size siblings and skips different pattern / far larger change', async () => { + const testRun = sibling('target', { customTags: '' }); + const matching = sibling('locale-a'); + const bigger = sibling('locale-b', { diffPercent: 30 }); + const different = sibling('locale-c'); + + const built = await initMatchingService({ + testRun, + siblings: [matching, bigger, different], + signatureOf: (imageName) => (imageName.startsWith('locale-c') ? OTHER_PALETTE : SAME_PALETTE), }); - const different = generateTestRun({ - id: 'diff', - status: TestStatus.unresolved, - name: 'Screen A', - customTags: 'locale-c', - diffPercent: 12, + + const result = await built.service.getMatchingVariations(testRun.id); + + expect(result.variations.map((variation) => variation.id)).toEqual([testRun.id, matching.id]); + expect(result.skipped.map((item) => ({ id: item.id, reason: item.reason }))).toEqual([ + { id: bigger.id, reason: 'different change size' }, + { id: different.id, reason: 'different change pattern' }, + ]); + }); + + it('skips a far larger change without paying for its signature', async () => { + const testRun = sibling('target', { customTags: '' }); + const bigger = sibling('locale-b', { diffPercent: 30 }); + + const built = await initMatchingService({ + testRun, + siblings: [bigger], + signatureOf: () => SAME_PALETTE, }); - const testRunFindUniqueMock = jest - .fn() - .mockResolvedValueOnce({ ...testRun, testVariation: generateTestVariation() }); - const testRunFindManyMock = jest.fn().mockResolvedValueOnce([matching, bigger, different]); - const projectFindUniqueMock = jest - .fn() - .mockResolvedValueOnce({ bulkApproveVariations: true, bulkApproveGroupBy: 'customTags' }); - service = await initService({ testRunFindUniqueMock, testRunFindManyMock, projectFindUniqueMock }); + await built.service.getMatchingVariations(testRun.id); - const referenceSignature = [1, 0]; - service['getChangeSignature'] = jest - .fn() - .mockResolvedValueOnce(referenceSignature) // target - .mockResolvedValueOnce(referenceSignature) // matching sibling - .mockResolvedValueOnce(referenceSignature) // bigger sibling (same palette, 2.5x larger change) - .mockResolvedValueOnce([0, 1]); // different palette sibling + const asked = built.compareGetChangeSignatureMock.mock.calls.map(([input]) => input.image.toString()); + expect(asked).toEqual([testRun.imageName]); + }); - const result = await service.getMatchingVariations(testRun.id); + it('says a sibling has no diff rather than blaming its change size', async () => { + const testRun = sibling('target', { customTags: '' }); + const unchanged = sibling('locale-b', { diffPercent: 0 }); - expect(result.variations.map((variation) => variation.id)).toEqual([testRun.id, matching.id]); - expect(result.skipped.map((item) => ({ id: item.id, customTags: item.customTags, reason: item.reason }))).toEqual( - [ - { id: bigger.id, customTags: 'locale-b', reason: 'different change size' }, - { id: different.id, customTags: 'locale-c', reason: 'different change pattern' }, - ] - ); + const built = await initMatchingService({ + testRun, + siblings: [unchanged], + signatureOf: () => SAME_PALETTE, + }); + + const result = await built.service.getMatchingVariations(testRun.id); + + expect(result.skipped.map((item) => item.reason)).toEqual(['no diff to match']); + }); + + it('asks for the siblings signatures at once rather than one after another', async () => { + const testRun = sibling('target', { customTags: '' }); + const siblings = ['locale-a', 'locale-b', 'locale-c'].map((id) => sibling(id)); + + let inFlight = 0; + let peakInFlight = 0; + const built = await initMatchingService({ + testRun, + siblings, + signatureOf: async () => { + inFlight += 1; + peakInFlight = Math.max(peakInFlight, inFlight); + await new Promise((resolve) => setTimeout(resolve, 0)); + inFlight -= 1; + return SAME_PALETTE; + }, + }); + + await built.service.getMatchingVariations(testRun.id); + + expect(peakInFlight).toBe(siblings.length + 1); + }); + + it('keeps the number of images being decoded at once bounded', async () => { + const testRun = sibling('target', { customTags: '' }); + const siblings = Array.from({ length: SIGNATURE_CONCURRENCY * 3 }, (_, index) => sibling(`locale-${index}`)); + + let inFlight = 0; + let peakInFlight = 0; + const built = await initMatchingService({ + testRun, + siblings, + signatureOf: async () => { + inFlight += 1; + peakInFlight = Math.max(peakInFlight, inFlight); + await new Promise((resolve) => setTimeout(resolve, 0)); + inFlight -= 1; + return SAME_PALETTE; + }, + }); + + await built.service.getMatchingVariations(testRun.id); + + expect(peakInFlight).toBe(SIGNATURE_CONCURRENCY); + }); + + it('reuses the signatures it already computed when the dialog is reopened', async () => { + const testRun = sibling('target', { customTags: '' }); + const matching = sibling('locale-a'); + + const built = await initMatchingService({ + testRun, + siblings: [matching], + signatureOf: () => SAME_PALETTE, + }); + + await built.service.getMatchingVariations(testRun.id); + const afterFirst = built.compareGetChangeSignatureMock.mock.calls.length; + const second = await built.service.getMatchingVariations(testRun.id); + + expect(afterFirst).toBe(2); + expect(built.compareGetChangeSignatureMock).toHaveBeenCalledTimes(2); + expect(second.variations.map((variation) => variation.id)).toEqual([testRun.id, matching.id]); + }); + + it('recomputes a signature once the run gained an ignore area', async () => { + const testRun = sibling('target', { customTags: '' }); + const matching = sibling('locale-a'); + + const built = await initMatchingService({ + testRun, + siblings: [matching], + signatureOf: () => SAME_PALETTE, + }); + + await built.service.getMatchingVariations(testRun.id); + matching.ignoreAreas = JSON.stringify([{ x: 0, y: 0, width: 10, height: 10 }]); + await built.service.getMatchingVariations(testRun.id); + + expect(built.compareGetChangeSignatureMock).toHaveBeenCalledTimes(3); }); }); }); diff --git a/src/test-runs/test-runs.service.ts b/src/test-runs/test-runs.service.ts index 44ae2926..2104e37f 100644 --- a/src/test-runs/test-runs.service.ts +++ b/src/test-runs/test-runs.service.ts @@ -1,6 +1,4 @@ import { forwardRef, Inject, Injectable, Logger } from '@nestjs/common'; -import { PNG } from 'pngjs'; -import Pixelmatch from 'pixelmatch'; import { CreateTestRequestDto } from './dto/create-test-request.dto'; import { IgnoreAreaDto } from './dto/ignore-area.dto'; import { StaticService } from '../static/static.service'; @@ -14,13 +12,15 @@ import { TestRunDto } from './dto/testRun.dto'; import { getTestVariationUniqueData } from '../utils'; import { CompareService } from '../compare/compare.service'; import { UpdateTestRunDto } from './dto/update-test.dto'; -import { applyIgnoreAreas, parseConfig } from '../compare/utils'; +import { parseConfig } from '../compare/utils'; import { DEFAULT_CONFIG } from '../compare/libs/pixelmatch/pixelmatch.service'; import { PixelmatchConfig } from '../compare/libs/pixelmatch/pixelmatch.types'; +import { signaturesMatch } from '../compare/libs/pixelmatch/signature.core'; @Injectable() export class TestRunsService { private readonly logger: Logger = new Logger(TestRunsService.name); + private readonly signatureCache = new BoundedCache>(SIGNATURE_CACHE_SIZE); constructor( @Inject(forwardRef(() => TestVariationsService)) @@ -202,7 +202,6 @@ export class TestRunsService { } } - const referenceSignature = await this.getChangeSignature(testRun, config); const siblings = await this.prismaService.testRun.findMany({ where: { id: { not: testRun.id }, @@ -217,25 +216,57 @@ export class TestRunsService { const matching: TestRun[] = []; const skipped: SkippedSibling[] = []; - if (!referenceSignature) { - for (const sibling of siblings) { - skipped.push({ run: sibling, reason: 'no reference diff' }); + // Comparing change sizes is free, computing a signature costs two decoded + // screenshots — so the cheap test goes first and takes those siblings out + // of the expensive pass entirely. + const candidates: TestRun[] = []; + for (const sibling of siblings) { + if (!sibling.diffPercent) { + // Nothing changed on this one, so there is no change to recognise — + // the same verdict the signature pass would have reached, for free. + skipped.push({ run: sibling, reason: 'no diff to match' }); + } else if (!magnitudesSimilar(testRun.diffPercent, sibling.diffPercent)) { + skipped.push({ run: sibling, reason: 'different change size' }); + } else { + candidates.push(sibling); } - return { testRun, matching, skipped }; } - for (const sibling of siblings) { - const signature = await this.getChangeSignature(sibling, config); + // The reviewed run and every remaining sibling are signed in one bounded + // fan-out across the worker pool. Signing them one after another is what + // made this dialog take tens of seconds on a build with many locales. + const [referenceSignature, ...candidateSignatures] = await mapWithConcurrency( + [testRun, ...candidates], + SIGNATURE_CONCURRENCY, + (run) => this.getChangeSignature(run, config) + ); + + // Nothing to match against: every sibling goes to manual review, whatever + // the cheap pass made of it. + if (!referenceSignature) { + return { + testRun, + matching, + skipped: siblings.map((run) => ({ run, reason: 'no reference diff' })), + }; + } + + candidates.forEach((sibling, index) => { + const signature = candidateSignatures[index]; if (!signature) { skipped.push({ run: sibling, reason: 'no diff to match' }); } else if (!signaturesMatch(referenceSignature, signature)) { skipped.push({ run: sibling, reason: 'different change pattern' }); - } else if (!magnitudesSimilar(testRun.diffPercent, sibling.diffPercent)) { - skipped.push({ run: sibling, reason: 'different change size' }); } else { matching.push(sibling); } - } + }); + + // The cheap pass ran first, so restore the order the siblings came in — + // the dialog lists them as one group and its order should not depend on + // which test rejected a sibling. + const orderOf = new Map(siblings.map((sibling, index) => [sibling.id, index] as const)); + skipped.sort((a, b) => orderOf.get(a.run.id) - orderOf.get(b.run.id)); return { testRun, matching, skipped }; } @@ -260,28 +291,59 @@ export class TestRunsService { /** * Position-independent color signature of the change between a test run's - * baseline and image: a normalized histogram of the colors the changed pixels - * took in the new image. Null when there is no baseline, dimensions differ, or - * nothing changed. + * baseline and image. Null when there is no baseline, dimensions differ, or + * nothing changed. The bytes go to the worker pool undecoded — decoding is + * the expensive part and must not happen on the event loop. + * + * Memoized: a reviewer who reopens the variations dialog, or steps back to a + * screen already looked at, would otherwise pay for the same decodes again. */ private async getChangeSignature(testRun: TestRun, config: PixelmatchConfig): Promise { if (!testRun.baselineName) { return null; } - const baseline = await this.staticService.getImage(testRun.baselineName); - const image = await this.staticService.getImage(testRun.imageName); - if (!baseline || !image || baseline.width !== image.width || baseline.height !== image.height) { + const ignoreAreas = this.getAllIgnoteAreas(testRun); + // Image names are unique per upload, so only the ignore areas and the + // project's diff config can change a signature under a stable pair. + const key = [ + testRun.baselineName, + testRun.imageName, + config.threshold, + config.ignoreAntialiasing, + JSON.stringify(ignoreAreas), + ].join('|'); + const cached = this.signatureCache.get(key); + if (cached) { + return cached; + } + + const pending = this.computeChangeSignature(testRun, ignoreAreas, config); + this.signatureCache.set(key, pending); + // A failed read must not be remembered as "no signature" forever. + pending.catch(() => this.signatureCache.delete(key)); + return pending; + } + + private async computeChangeSignature( + testRun: TestRun, + ignoreAreas: IgnoreAreaDto[], + config: PixelmatchConfig + ): Promise { + const [baseline, image] = await Promise.all([ + this.staticService.getImageBuffer(testRun.baselineName), + this.staticService.getImageBuffer(testRun.imageName), + ]); + if (!baseline || !image) { return null; } - // Apply ignore areas exactly as the diff does, so masked regions never count - // toward the change signature. - const ignoreAreas = this.getAllIgnoteAreas(testRun); - applyIgnoreAreas(baseline, ignoreAreas); - applyIgnoreAreas(image, ignoreAreas); - return changeColorSignature(baseline, image, { + const { signature } = await this.compareService.getChangeSignature({ + baseline, + image, + ignoreAreas, threshold: config.threshold, includeAA: config.ignoreAntialiasing, }); + return signature; } async setStatus(id: string, status: TestStatus): Promise { @@ -555,108 +617,63 @@ function resolveGroupByAxis(value: string | null | undefined): string { return value && (GROUP_BY_AXES as readonly string[]).includes(value) ? value : 'customTags'; } -// Colors are quantized to this many levels per RGB channel, giving -// COLOR_BUCKETS_PER_CHANNEL^3 histogram buckets. -const COLOR_BUCKETS_PER_CHANNEL = 4; -// Two changes are considered the same pattern when their color signatures' -// cosine similarity is at least this value. -const SIGNATURE_SIMILARITY_THRESHOLD = 0.9; - -// Longest side (px) images are downscaled to before computing the color -// signature — keeps the histogram representative while cutting pixelmatch cost. -const SIGNATURE_MAX_DIMENSION = 500; - -// Nearest-neighbour downscale so the longest side is at most maxDimension. -// Returns the original when already small enough. -function downscale( - source: { data: Buffer; width: number; height: number }, - maxDimension: number -): { data: Buffer; width: number; height: number } { - const scale = maxDimension / Math.max(source.width, source.height); - if (scale >= 1) { - return source; - } - const width = Math.max(1, Math.round(source.width * scale)); - const height = Math.max(1, Math.round(source.height * scale)); - const data: Buffer = Buffer.alloc(width * height * 4); - for (let y = 0; y < height; y++) { - const sourceY = Math.min(source.height - 1, Math.floor(y / scale)); - for (let x = 0; x < width; x++) { - const sourceX = Math.min(source.width - 1, Math.floor(x / scale)); - const sourceIndex = (sourceY * source.width + sourceX) * 4; - const targetIndex = (y * width + x) * 4; - data[targetIndex] = source.data[sourceIndex]; - data[targetIndex + 1] = source.data[sourceIndex + 1]; - data[targetIndex + 2] = source.data[sourceIndex + 2]; - data[targetIndex + 3] = source.data[sourceIndex + 3]; - } - } - return { data, width, height }; -} +// How many sibling screenshots may be in the worker pool at once for one +// variations dialog. The pool queue is shared with build ingestion, and every +// queued job holds two image buffers, so the fan-out stays bounded rather than +// handing the pool a job per locale. +export const SIGNATURE_CONCURRENCY = 8; + +// Signatures kept across requests, keyed by the image pair and diff settings. +// Roughly one build's worth of screens, at 64 floats each. +const SIGNATURE_CACHE_SIZE = 2000; /** - * Position-independent signature of a change: a normalized histogram of the - * colors that the changed pixels take in the new image. Because it ignores - * *where* the change is, it is robust to per-locale layout shifts (a title - * wrapping to a different number of lines, options moving down, etc.) while - * still capturing *what* changed (a selection highlight, a recolored button). + * Insertion-ordered cache with a hard size limit: reviewing build after build + * would otherwise grow the signature memo without bound. Reinserting a key + * refreshes its position, so what the reviewer keeps coming back to survives. */ -function changeColorSignature( - baselineImage: { data: Buffer; width: number; height: number }, - checkpointImage: { data: Buffer; width: number; height: number }, - options: { threshold: number; includeAA: boolean } -): number[] | null { - // The signature is a coarse color histogram, so full resolution is wasteful — - // downscale first to make pixelmatch (CPU-bound, run per variation) much faster. - const baseline = downscale(baselineImage, SIGNATURE_MAX_DIMENSION); - const image = downscale(checkpointImage, SIGNATURE_MAX_DIMENSION); - const { width, height } = baseline; - const mask = new PNG({ width, height }); - const changedPixels = Pixelmatch(baseline.data, image.data, mask.data, width, height, { - threshold: options.threshold, - includeAA: options.includeAA, - diffMask: true, - }); - if (changedPixels === 0) { - return null; - } +class BoundedCache { + private readonly entries = new Map(); + + constructor(private readonly limit: number) {} - const bucketSize = 256 / COLOR_BUCKETS_PER_CHANNEL; - const histogram = new Array(COLOR_BUCKETS_PER_CHANNEL ** 3).fill(0); - for (let i = 0; i < width * height; i++) { - if (mask.data[i * 4 + 3] === 0) { - continue; + get(key: string): T | undefined { + const value = this.entries.get(key); + if (value !== undefined) { + this.entries.delete(key); + this.entries.set(key, value); } - const r = Math.min(COLOR_BUCKETS_PER_CHANNEL - 1, Math.floor(image.data[i * 4] / bucketSize)); - const g = Math.min(COLOR_BUCKETS_PER_CHANNEL - 1, Math.floor(image.data[i * 4 + 1] / bucketSize)); - const b = Math.min(COLOR_BUCKETS_PER_CHANNEL - 1, Math.floor(image.data[i * 4 + 2] / bucketSize)); - histogram[r * COLOR_BUCKETS_PER_CHANNEL * COLOR_BUCKETS_PER_CHANNEL + g * COLOR_BUCKETS_PER_CHANNEL + b]++; + return value; } - const total = histogram.reduce((sum, value) => sum + value, 0); - if (total === 0) { - return null; + set(key: string, value: T): void { + this.entries.delete(key); + this.entries.set(key, value); + while (this.entries.size > this.limit) { + this.entries.delete(this.entries.keys().next().value); + } } - return histogram.map((value) => value / total); -} -function cosineSimilarity(a: number[], b: number[]): number { - let dot = 0; - let normA = 0; - let normB = 0; - for (let i = 0; i < a.length; i++) { - dot += a[i] * b[i]; - normA += a[i] * a[i]; - normB += b[i] * b[i]; + delete(key: string): void { + this.entries.delete(key); } - if (normA === 0 || normB === 0) { - return 0; - } - return dot / (Math.sqrt(normA) * Math.sqrt(normB)); } -function signaturesMatch(a: number[], b: number[]): boolean { - return cosineSimilarity(a, b) >= SIGNATURE_SIMILARITY_THRESHOLD; +/** + * Runs `task` over every item with at most `limit` in flight, keeping the + * results in the items' order. + */ +async function mapWithConcurrency(items: T[], limit: number, task: (item: T) => Promise): Promise { + const results = new Array(items.length); + let next = 0; + const worker = async (): Promise => { + while (next < items.length) { + const index = next++; + results[index] = await task(items[index]); + } + }; + await Promise.all(Array.from({ length: Math.min(limit, items.length) }, worker)); + return results; } // Same palette but a much larger/smaller change area signals a different or From 56a40866cc4840993b47fb4e4202e6bd7cac72ad Mon Sep 17 00:00:00 2001 From: "mykola.gervasyuk" Date: Mon, 31 Aug 2026 16:53:58 +0300 Subject: [PATCH 2/2] fix: skip an unreadable screenshot instead of failing the whole dialog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moving the signature onto the worker pool sent raw PNG bytes rather than going through staticService.getImage, which had been catching decode failures and answering undefined — a corrupt or truncated screenshot used to leave its sibling reported as "no diff to match". Undecodable bytes now throw out of the worker instead, and because the siblings are signed in a single fan-out, one bad candidate rejects the whole Approve variations request rather than being left out of it. Guard both decodes and answer with no signature, as the doc comment above already claimed. Found by CodeRabbit on #372. --- src/compare/libs/pixelmatch/signature.core.spec.ts | 11 +++++++++++ src/compare/libs/pixelmatch/signature.core.ts | 14 ++++++++++++-- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/compare/libs/pixelmatch/signature.core.spec.ts b/src/compare/libs/pixelmatch/signature.core.spec.ts index 42029b4e..5c6acbb1 100644 --- a/src/compare/libs/pixelmatch/signature.core.spec.ts +++ b/src/compare/libs/pixelmatch/signature.core.spec.ts @@ -53,6 +53,17 @@ describe('computeChangeSignature', () => { expect(signatureOf(blank, blank)).toBeNull(); }); + // One unreadable screenshot must not reject the whole matching request: the + // siblings are signed in a single fan-out, so a throw here would take the + // variations dialog down with it rather than skipping that one candidate. + it('has no signature when the checkpoint cannot be decoded', () => { + expect(signatureOf(blank, Buffer.from('not a png'))).toBeNull(); + }); + + it('has no signature when the baseline is truncated', () => { + expect(signatureOf(blank.subarray(0, 30), block(0, 0, RED))).toBeNull(); + }); + it('concentrates on the color the changed pixels took in the new image', () => { const signature = signatureOf(blank, block(0, 0, RED)); diff --git a/src/compare/libs/pixelmatch/signature.core.ts b/src/compare/libs/pixelmatch/signature.core.ts index c843f2d8..9909354c 100644 --- a/src/compare/libs/pixelmatch/signature.core.ts +++ b/src/compare/libs/pixelmatch/signature.core.ts @@ -84,8 +84,18 @@ function downscale(source: RawImage, maxDimension: number): RawImage { * dimensions, or no change outside the ignore areas. */ export function computeChangeSignature(input: SignatureJobInput): SignatureJobOutput { - const baselineImage = PNG.sync.read(toBuffer(input.baseline)); - const checkpointImage = PNG.sync.read(toBuffer(input.image)); + // A corrupt or truncated screenshot is one candidate the reviewer cannot be + // offered, not a failed request: the siblings are signed in a single + // fan-out, so throwing here would take the whole variations dialog down. + let baselineImage: PNG; + let checkpointImage: PNG; + try { + baselineImage = PNG.sync.read(toBuffer(input.baseline)); + checkpointImage = PNG.sync.read(toBuffer(input.image)); + } catch { + return { signature: null }; + } + if (baselineImage.width !== checkpointImage.width || baselineImage.height !== checkpointImage.height) { return { signature: null }; }