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..5c6acbb1 --- /dev/null +++ b/src/compare/libs/pixelmatch/signature.core.spec.ts @@ -0,0 +1,89 @@ +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(); + }); + + // 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)); + + 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..9909354c --- /dev/null +++ b/src/compare/libs/pixelmatch/signature.core.ts @@ -0,0 +1,158 @@ +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 { + // 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 }; + } + + // 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