From cdd4ab971ff6d8e1e49f137c9811dfc2e15fec4c Mon Sep 17 00:00:00 2001 From: Chris Lorenzo Date: Thu, 4 Jun 2026 19:14:05 -0400 Subject: [PATCH] fix(canvas): harden Canvas2D texture, memory, and clipping paths - CanvasTexture: capture textureData before the first await and abort the load if the texture is freed in-flight, preventing a state/image desync where state=loaded but image=undefined - CanvasTexture.getImage: return null instead of asserting on a missing image; CanvasRenderer skips the draw for that frame - Texture: exclude textures in the 'loading' state from cleanup and null out ctxTexture after free() so a reload re-creates it instead of reusing a stale reference - ImageTexture/CanvasTexture: fail fast for compressed textures in canvas mode (before any fetch/decode) with a descriptive failed state - TextureMemoryManager/Stage: treat criticalThreshold=0 as disabled and apply it automatically for the canvas renderer, since JS-heap textures are GC-managed and threshold-based upload blocking does not apply - CanvasRenderer: gate clipping on clippingRect.valid and skip rendering a node clipped to zero area, which previously rendered with no clip at all Also adds a --renderMode (webgl|canvas|all) flag to the visual-regression runner, storing canvas snapshots in a separate -canvas subdirectory. Co-Authored-By: Claude Opus 4.8 --- src/core/Stage.ts | 11 ++++++ src/core/TextureMemoryManager.ts | 7 ++-- src/core/renderers/canvas/CanvasRenderer.ts | 22 ++++++++++-- src/core/renderers/canvas/CanvasTexture.ts | 38 +++++++++++++++++---- src/core/textures/ImageTexture.ts | 18 ++++++++++ src/core/textures/Texture.ts | 8 +++++ visual-regression/src/index.ts | 36 ++++++++++++++++--- 7 files changed, 124 insertions(+), 16 deletions(-) diff --git a/src/core/Stage.ts b/src/core/Stage.ts index 405ad00..a81d666 100644 --- a/src/core/Stage.ts +++ b/src/core/Stage.ts @@ -247,6 +247,17 @@ export class Stage { const renderMode = this.renderer.mode || 'webgl'; + // Canvas2D textures are plain JS heap objects managed by the browser GC. + // Threshold-based upload blocking makes no sense for JS heap — disable it + // by setting criticalThreshold to 0 while keeping the eviction machinery. + if (renderMode === 'canvas') { + this.txMemManager.updateSettings({ + ...textureMemory, + criticalThreshold: 0, + doNotExceedCriticalThreshold: false, + }); + } + this.createDefaultTexture(); setPremultiplyMode(renderMode); diff --git a/src/core/TextureMemoryManager.ts b/src/core/TextureMemoryManager.ts index 4487e6a..10444fc 100644 --- a/src/core/TextureMemoryManager.ts +++ b/src/core/TextureMemoryManager.ts @@ -148,7 +148,7 @@ export class TextureMemoryManager { this.loadedTextures.add(texture); } - if (this.memUsed > this.criticalThreshold) { + if (this.criticalThreshold > 0 && this.memUsed > this.criticalThreshold) { this.criticalCleanupRequested = true; } } @@ -156,13 +156,14 @@ export class TextureMemoryManager { checkCleanup() { return ( this.criticalCleanupRequested || - (this.memUsed > this.targetThreshold && + (this.criticalThreshold > 0 && + this.memUsed > this.targetThreshold && this.frameTime - this.lastCleanupTime >= this.cleanupInterval) ); } checkCriticalCleanup() { - return this.memUsed > this.criticalThreshold; + return this.criticalThreshold > 0 && this.memUsed > this.criticalThreshold; } /** diff --git a/src/core/renderers/canvas/CanvasRenderer.ts b/src/core/renderers/canvas/CanvasRenderer.ts index 8ef5613..0a9cd74 100644 --- a/src/core/renderers/canvas/CanvasRenderer.ts +++ b/src/core/renderers/canvas/CanvasRenderer.ts @@ -63,7 +63,19 @@ export class CanvasRenderer extends CoreRenderer { } const hasTransform = ta !== 1; - const hasClipping = clippingRect.w !== 0 && clippingRect.h !== 0; + const clippingValid = clippingRect.valid === true; + + // If the clipping rect is valid but zero-area, the node is fully clipped — skip rendering + if ( + clippingValid === true && + clippingRect.w === 0 && + clippingRect.h === 0 + ) { + return; + } + + const hasClipping = + clippingValid === true && clippingRect.w !== 0 && clippingRect.h !== 0; const shader = node.props.shader; const hasShader = shader !== null; @@ -124,7 +136,7 @@ export class CanvasRenderer extends CoreRenderer { if (textureType !== TextureType.color) { const tintColor = parseColor(color); - let image: ImageBitmap | HTMLCanvasElement | HTMLImageElement; + let image: ImageBitmap | HTMLCanvasElement | HTMLImageElement | null; if (textureType === TextureType.subTexture) { image = ( @@ -134,6 +146,12 @@ export class CanvasRenderer extends CoreRenderer { image = (texture.ctxTexture as CanvasTexture).getImage(tintColor); } + // getImage returns null when the underlying image was freed mid-load; + // skip drawing this frame rather than crashing. + if (image === null) { + return; + } + this.context.globalAlpha = tintColor.a ?? node.worldAlpha; const txCoords = node.textureCoords; diff --git a/src/core/renderers/canvas/CanvasTexture.ts b/src/core/renderers/canvas/CanvasTexture.ts index 04d2836..72a5c0f 100644 --- a/src/core/renderers/canvas/CanvasTexture.ts +++ b/src/core/renderers/canvas/CanvasTexture.ts @@ -2,6 +2,7 @@ import type { Dimensions } from '../../../common/CommonTypes.js'; import { assertTruthy } from '../../../utils.js'; import { formatRgba, type IParsedColor } from '../../lib/colorParser.js'; import { CoreContextTexture } from '../CoreContextTexture.js'; +import type { Texture } from '../../textures/Texture.js'; export class CanvasTexture extends CoreContextTexture { protected image: @@ -17,10 +18,23 @@ export class CanvasTexture extends CoreContextTexture { | undefined; async load(): Promise { + // Capture textureData synchronously before any await - a pending + // freeTextureDataTask microtask could null textureSource.textureData + // during the first async suspension, causing onLoadRequest to fail. + const textureData = this.textureSource.textureData; + assertTruthy(textureData?.data, 'Texture data is null before load'); + this.textureSource.setState('loading'); try { - const size = await this.onLoadRequest(); + const size = await this.onLoadRequest(textureData.data); + + // Guard against the texture being freed while the load was in flight + if (this.textureSource.state === 'freed') { + this.image = undefined; + return; + } + this.textureSource.setState('loaded', size); this.textureSource.freeTextureData(); this.updateMemSize(); @@ -63,9 +77,11 @@ export class CanvasTexture extends CoreContextTexture { getImage( color: IParsedColor, - ): ImageBitmap | HTMLCanvasElement | HTMLImageElement { + ): ImageBitmap | HTMLCanvasElement | HTMLImageElement | null { const image = this.image; - assertTruthy(image, 'Attempt to get unloaded image texture'); + if (image === undefined) { + return null; + } if (color.isWhite) { if (this.tintCache) { @@ -114,9 +130,17 @@ export class CanvasTexture extends CoreContextTexture { return canvas; } - private async onLoadRequest(): Promise { - assertTruthy(this.textureSource?.textureData?.data, 'Texture data is null'); - const { data } = this.textureSource.textureData; + private async onLoadRequest( + data: NonNullable['data']>, + ): Promise { + // CompressedData objects (KTX, PVR, ASTC) carry GPU-format mipmap buffers + // that cannot be decoded by Canvas2D. Reject explicitly rather than falling + // through silently and leaving this.image unassigned. + if (typeof data === 'object' && 'mipmaps' in data) { + throw new Error( + 'CanvasTexture: Compressed texture data is not supported in Canvas2D render mode', + ); + } // TODO: canvas from text renderer should be able to provide the canvas directly // instead of having to re-draw it into a new canvas... @@ -125,7 +149,7 @@ export class CanvasTexture extends CoreContextTexture { canvas.width = data.width; canvas.height = data.height; const ctx = canvas.getContext('2d'); - if (ctx) ctx.putImageData(data, 0, 0); + if (ctx !== null) ctx.putImageData(data, 0, 0); this.image = canvas; return { w: data.width, h: data.height }; } else if ( diff --git a/src/core/textures/ImageTexture.ts b/src/core/textures/ImageTexture.ts index 8890a03..a9651cc 100644 --- a/src/core/textures/ImageTexture.ts +++ b/src/core/textures/ImageTexture.ts @@ -269,6 +269,24 @@ export class ImageTexture extends Texture { } override async getTextureSource(): Promise { + // Compressed textures are not supported by the Canvas2D renderer. + // Fail fast here before incurring a network fetch or binary decode. + if (this.txManager.renderer?.mode === 'canvas') { + const { src, type } = this.props; + if ( + type === 'compressed' || + (typeof src === 'string' && isCompressedTextureContainer(src) === true) + ) { + const err = new Error( + `ImageTexture: Compressed textures are not supported in Canvas2D render mode (src: ${String( + src, + )})`, + ); + this.setState('failed', err); + return { data: null }; + } + } + let resp: TextureData; try { resp = await this.determineImageTypeAndLoadImage(); diff --git a/src/core/textures/Texture.ts b/src/core/textures/Texture.ts index 84a33ca..dcfd5f7 100644 --- a/src/core/textures/Texture.ts +++ b/src/core/textures/Texture.ts @@ -240,6 +240,11 @@ export abstract class Texture extends EventEmitter { return false; } + // Don't cleanup a texture that is in the process of loading + if (this.state === 'loading') { + return false; + } + // Don't cleanup if not renderable if (this.renderable === true) { return false; @@ -341,6 +346,9 @@ export abstract class Texture extends EventEmitter { */ free(): void { this.ctxTexture?.free(); + // Null out the freed ctxTexture so a subsequent reload re-creates it via + // getCtxTexture() instead of reusing a stale, already-freed reference. + this.ctxTexture = undefined; } /** diff --git a/visual-regression/src/index.ts b/visual-regression/src/index.ts index 263a645..fb104fa 100644 --- a/visual-regression/src/index.ts +++ b/visual-regression/src/index.ts @@ -100,6 +100,17 @@ const argv = yargs(hideBin(process.argv)) description: 'Number of parallel browser pages used to run tests (sharded round-robin)', }, + renderMode: { + type: 'string', + alias: 'r', + // Defaults to webgl-only. Switch to 'all' (here or via --renderMode) + // once canvas baselines have been captured and committed, otherwise + // canvas compare runs fail for lack of reference snapshots. + default: 'webgl', + choices: ['webgl', 'canvas', 'all'], + description: + 'Renderer mode to test ("webgl", "canvas", or "all" for both)', + }, }) .parseSync(); @@ -137,6 +148,7 @@ async function dockerCiMode(): Promise { argv.port ? `--port ${argv.port}` : '', argv.filter ? `--filter "${argv.filter}"` : '', argv.workers > 1 ? `--workers ${argv.workers}` : '', + argv.renderMode ? `--renderMode ${argv.renderMode}` : '', ].join(' '); // Get the directory of the current file @@ -209,7 +221,17 @@ async function compareCaptureMode(): Promise { } // Run the tests - exitCode = await runTest('chromium'); + const renderModes: ('webgl' | 'canvas')[] = + argv.renderMode === 'all' + ? ['webgl', 'canvas'] + : [argv.renderMode as 'webgl' | 'canvas']; + exitCode = 0; + for (const mode of renderModes) { + const result = await runTest('chromium', mode); + if (result !== 0) { + exitCode = result; + } + } } finally { // Kill the serve-examples process serveExamplesChildProc.kill(); @@ -221,9 +243,13 @@ async function compareCaptureMode(): Promise { * Run the tests in capture or compare mode depending on the `argv.capture` flag * for a specific browser type. */ -async function runTest(browserType: 'chromium') { +async function runTest( + browserType: 'chromium', + renderMode: 'webgl' | 'canvas', +) { const paramString = Object.entries({ browser: browserType, + renderMode, overwrite: argv.overwrite, filter: argv.filter, RUNTIME_ENV: runtimeEnv, @@ -238,7 +264,9 @@ async function runTest(browserType: 'chromium') { ), ); - const snapshotSubDirName = `${browserType}-${runtimeEnv}`; + const snapshotSubDirName = `${browserType}-${runtimeEnv}${ + renderMode === 'canvas' ? '-canvas' : '' + }`; const snapshotSubDir = path.join(certifiedSnapshotDir, snapshotSubDirName); @@ -370,7 +398,7 @@ async function runTest(browserType: 'chromium') { const shardParam = workerCount > 1 ? `&shard=${shardIndex}/${workerCount}` : ''; await page.goto( - `http://localhost:${argv.port}/?automation=true&test=${argv.filter}${shardParam}`, + `http://localhost:${argv.port}/?automation=true&test=${argv.filter}&renderMode=${renderMode}${shardParam}`, ); await donePromise;