Skip to content

Commit b4ee713

Browse files
obiotclaude
andauthored
fix(video): six GL-core audit findings, each reproduced before fixing (#1542)
* fix(video): six GL-core audit findings, each reproduced before fixing Beat 1 of the resumed subsystem audit (video/GL core, single-finder + inline verification). Per protocol every bug was demonstrated red with a reproduction test BEFORE being touched; one finding was refined during reproduction (fillRect gradients never used the stencil path — scoped the fix to the shape fills that do). 1. getSupportedCompressedTextureFormats: six WEBKIT fallbacks read the never-assigned `this._gl` → TypeError on any device missing one compression family (everything except ANGLE/Metal Macs — which is why it survived: the dev machines expose every family). → `gl.` 2. Nested multi-effect post-effects: RenderTargetPool's two scalars (_activeBase/_previousBase) became a proper base STACK — each nested sprite pass gets its own lazily-allocated capture/ping-pong pair (2/3, 4/5, …) and begin/end pairs unwind like save/restore; the saved effect projection is a per-pass stack too. Pre-fix, a 2-effect child inside a 2-effect container cleared the parent's capture, popped its slot, and crashed endPostEffect every frame. 3. #gradientMask: write/clip/restore phases now use a high-bit stencil marker (collision-free with mask levels) gated on the VISIBLE value of the active mask (0 when inverted, maskLevel otherwise), and restore setMask's exact stencil test — the old code painted gradients inside inverted cutouts and left a NOTEQUAL parity test that inverted/leaked all subsequent masked draws. setMask/clearMask now track _maskInvert. 4. disableScissor() flushes pending quads before disabling the test (mirrors enableScissor/clipRect/restore) — batched sprites could escape the scissor entirely. 5. WebGL clearRect() erases to actual transparent black per its JSDoc ("#000000" parses with alpha 1 → it painted opaque black; Canvas parity). 6. Canvas clipRect() skip-optimizations now reason in DEVICE space (rotation/skew always clips + cache poisoned): the local-space early-outs dropped canvas-sized clips under a translate and skipped nested same-size clips via the stale cache. Reproductions: tests/glcore-audit.spec.js (5) + tests/canvas-cliprect-transform.spec.js (2) — all 7 red on the unfixed code (nested-FBO red with the exact predicted undefined.unbind). renderTargetPool.spec.js updated to the stack internals + 2 new nesting tests. Full suite 4736 passed / 0 failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A * fix(review): inert NaN cache poison + integer cache + reset cleanup + single-source mask ref The pre-merge review agent caught a real defect in fix 6: currentScissor is an Int32Array, so the rotated-clip cache poison (NaN) coerced to 0 — leaving a frankenstein box a later axis-aligned clip could false-match, reviving the exact stale-cache bug class the fix targets. Reproduced red (rotated → axis-aligned sequence) before fixing. - poison the WIDTH slot with -1 instead: device widths are normalized to >= 0, so -1 can never match (same trick the WebGL clipRect spec uses) - floor/ceil the device box before compare/store so the Int32Array cache is exact under fractional transforms (devicePixelRatio, sub-pixel translates) — the WebGL clipRect's integer discipline - WebGLRenderer.reset() clears _effectProjectionStack: a draw throwing mid-pass orphans stack entries; without the cleanup every later pass leaked a matrix and the pool's nesting depth inflated forever (reproduced red: begin-without-end + reset) - single-source the "which stencil value is visible" convention: setMask now stores _maskVisibleRef (0 inverted / maskLevel otherwise) and #gradientMask consumes it — the _maskInvert flag and the duplicated ternary are gone (the review's one duplicated-logic finding) Review verdict otherwise: public API untouched, no collateral behavior change outside the six bugs, stencil math and pool arithmetic verified correct. 2 new red-first tests; full suite 4738 passed / 0 failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A * fix(review): clamp mask nesting below the marker bit + zero-alloc projection slots Both Copilot comments (independently matching the review agent's follow-up findings): - setMask now clamps maskLevel at 127 with a one-shot warning: mask levels live in the stencil's low 7 bits — the high bit is reserved as #gradientMask's temporary marker, and an 8-bit stencil couldn't represent deeper nesting anyway. Reproduced red first (130 nested setMask calls → maskLevel 130 pre-fix). - the per-pass saved projection no longer clones a Matrix3d per beginPostEffect: preallocated slots + a depth counter (zero-alloc steady state, slots kept across reset() for reuse). Full suite 4739 passed / 0 failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 2d11694 commit b4ee713

7 files changed

Lines changed: 604 additions & 73 deletions

File tree

packages/melonjs/CHANGELOG.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,12 @@
2626
- **`moveTowards()` never reached its target — and returned the target instead of `this`** (all four vector classes) — the arrival test compared the remaining distance against `step * step` (wrong units: snapped way too early for steps above 1, dithered around the target forever for fractional steps), and on "arrival" it returned the **caller's target vector** without moving `this` at all — so the vector never landed, and chaining onto the result silently mutated the target. It now lands exactly on the target and always returns `this`, per its own documentation; negative-step flee semantics are unchanged.
2727
- **Every preloaded video was fetched with forced anonymous CORS** — the video parser unconditionally stamped the `crossorigin` attribute with the loader's `crossOrigin` setting, which defaults to `undefined` — and per the HTML spec an *invalid* value (the string `"undefined"`) maps to `anonymous`, unlike a *missing* attribute (no CORS). Cross-origin videos served without CORS headers failed to load outright when they would have played fine untainted. The attribute is now only set when `crossOrigin` is actually configured (the empty string remains a valid, honored value).
2828
- **Video preloading hung forever on autoplay-restricted browsers (e.g. iOS Safari) unless `autoplay: false` was spelled out** — the parser waited for `canplay`, which those browsers never fire for a video that isn't allowed to play, and only an *explicit* `autoplay: false` opted into the reliable `loadedmetadata` path — so the common manifest shape (no `autoplay` key) stalled the whole preloader and the game never started. Preloading now completes on `loadedmetadata` unless `autoplay: true` explicitly asks to wait for `canplay`.
29+
- **Compressed-texture loading crashed on any device missing one compression family** — six `WEBKIT_`-prefixed extension fallbacks in `getSupportedCompressedTextureFormats()` referenced `this._gl`, a field that is never assigned (the context lives in `this.gl`). The fallback only evaluates when the primary extension is absent, so the whole feature *worked on ANGLE/Metal Macs* (which expose every family) *and threw a TypeError everywhere else* — Windows (no ASTC), iOS (no S3TC), software renderers — killing every `.dds`/`.pvr`/`.pkm`/`.ktx`/`.ktx2` load.
30+
- **Nested multi-effect post-effects corrupted the render-target pool and crashed every frame** — a renderable with 2+ `postEffects` drawn inside a container with 2+ `postEffects` hit a half-guarded nesting path: the inner pass cleared the outer pass's capture target, its `end` popped the outer pass's pool slot, and the outer `endPostEffect` then dereferenced `undefined` (per-frame TypeError; with camera effects active it silently blitted the wrong texture instead). The pool now tracks a proper stack of passes — each nesting level gets its own lazily-allocated capture/ping-pong pair — and the saved effect projection is a per-pass stack too, so nested post-effect passes compose correctly.
31+
- **Gradient shape fills (`fillEllipse`/`fillPolygon`/`fillArc`/`fillRoundRect`) broke inverted and nested masks** — the internal stencil pass gated its write phase on the *hidden* region of an inverted `setMask` (painting the gradient inside the cutout instead of the visible area) and restored a `NOTEQUAL` parity test instead of the mask's actual test — inverting the mask for every subsequent draw, and leaking outside-all-masks pixels for nested masks. The pass now tags the shape's visible pixels with a high stencil bit (collision-free with mask levels), clips the gradient to the tag, and re-installs the exact stencil test `setMask` had established. (`fillRect` gradients were unaffected — they render through a textured quad.)
32+
- **`disableScissor()` didn't flush, so sprites batched inside the scissor escaped it** — GL scissor applies at draw time, not at batch time; every other scissor mutation (`enableScissor`, `clipRect`, `restore`) flushes pending vertices first, but `disableScissor` didn't — so the common `enableScissor → drawImage → disableScissor` pattern clipped nothing: the quads were still queued when the test was turned off.
33+
- **WebGL `clearRect()` painted opaque black instead of erasing** — its JSDoc (and the Canvas renderer) promise transparent black, but the internal `clearColor()` default (`"#000000"`) parses with alpha 1. Visible on `transparent: true` canvases and inside post-effect render targets, where the "erased" region composited as a solid black box.
34+
- **Canvas `clipRect()` skip-optimizations ignored the active transform** — the "full-viewport no-op" and "same-as-last-clip" early-outs compared the rect's raw *local* values, so a canvas-sized clipped container positioned off-origin got no clip at all, and a clipped container nested inside a same-sized clipped container skipped its own clip via the stale cache — both Canvas-only divergences from the transform-aware WebGL scissor path (the #1349 fix). The skip logic now reasons in device space (and always clips under rotation/skew, where an axis-aligned cache can't).
2935
- **`Path2D.arc()`/`arcTo()`/`ellipse()` appended to an open path started a new sub-path instead of connecting** — the native Path2D spec joins an arc to the current point with a straight line (`arcTo`'s own documentation even promises it), but all three called `moveTo()` unconditionally. Under this release's new multi-sub-path fill semantics that stray sub-path is treated as a **hole**, so an arc appended to a filled outline punched a hole in the shape. They now connect per the spec; the canonical circle-hole idiom (explicit `moveTo` before `arc()`) still registers a hole, and a virgin-path `arc()` still starts cleanly.
3036
- **`Rect.right`/`Rect.bottom` returned the width/height when the edge sat exactly at coordinate 0** — the getters computed `this.left + w || w`, so an edge landing precisely on 0 (falsy) fell back to the size. Bounds and collision math around the origin came out wrong for such rectangles.
3137
- **`Rect.toPolygon()` handed out the rectangle's live vertices**`Polygon.setVertices` stores a `Vector2d[]` by reference, so the "new" polygon shared the rect's actual points array: transforming or mutating the polygon silently corrupted the rectangle. The vertices are now cloned.

packages/melonjs/src/video/canvas/canvas_renderer.js

Lines changed: 53 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1255,31 +1255,67 @@ export default class CanvasRenderer extends Renderer {
12551255
* @param {number} height
12561256
*/
12571257
clipRect(x, y, width, height) {
1258+
const context = this.getContext();
1259+
const t = context.getTransform();
1260+
1261+
// The two skip-optimizations below (full-viewport no-op and
1262+
// same-as-last-clip) must reason in DEVICE space — the rect arrives
1263+
// in LOCAL coordinates (e.g. Container clipping translates first,
1264+
// then clips at (0, 0)), so comparing raw values against the canvas
1265+
// box or the cache is wrong under any transform. The WebGL renderer
1266+
// transforms the rect the same way (see the #1349 fix).
1267+
if (t.b !== 0 || t.c !== 0) {
1268+
// rotated/skewed: the axis-aligned skip logic can't reason about
1269+
// the clipped region — always clip, and poison the cache so a
1270+
// following axis-aligned call can't false-match. The cache is an
1271+
// Int32Array (NaN would coerce to 0!) and widths are normalized
1272+
// to >= 0 below, so -1 in the width slot can never match.
1273+
context.beginPath();
1274+
context.rect(x, y, width, height);
1275+
context.clip();
1276+
this.currentScissor[2] = -1;
1277+
return;
1278+
}
1279+
1280+
// axis-aligned device-space box (scale can be negative — normalize),
1281+
// floored/ceiled to integers so the Int32Array cache is exact under
1282+
// fractional transforms (same discipline as the WebGL clipRect)
1283+
let dx = x * t.a + t.e;
1284+
let dy = y * t.d + t.f;
1285+
let dw = width * t.a;
1286+
let dh = height * t.d;
1287+
if (dw < 0) {
1288+
dx += dw;
1289+
dw = -dw;
1290+
}
1291+
if (dh < 0) {
1292+
dy += dh;
1293+
dh = -dh;
1294+
}
1295+
const ix = Math.floor(dx);
1296+
const iy = Math.floor(dy);
1297+
const iw = Math.ceil(dx + dw) - ix;
1298+
const ih = Math.ceil(dy + dh) - iy;
1299+
12581300
const canvas = this.getCanvas();
1259-
// if requested box is different from the current canvas size;
1260-
if (
1261-
x !== 0 ||
1262-
y !== 0 ||
1263-
width !== canvas.width ||
1264-
height !== canvas.height
1265-
) {
1301+
// if the requested box is different from the full canvas;
1302+
if (ix !== 0 || iy !== 0 || iw !== canvas.width || ih !== canvas.height) {
12661303
const currentScissor = this.currentScissor;
12671304
// if different from the current scissor box
12681305
if (
1269-
currentScissor[0] !== x ||
1270-
currentScissor[1] !== y ||
1271-
currentScissor[2] !== width ||
1272-
currentScissor[3] !== height
1306+
currentScissor[0] !== ix ||
1307+
currentScissor[1] !== iy ||
1308+
currentScissor[2] !== iw ||
1309+
currentScissor[3] !== ih
12731310
) {
1274-
const context = this.getContext();
12751311
context.beginPath();
12761312
context.rect(x, y, width, height);
12771313
context.clip();
1278-
// save the new currentScissor box
1279-
currentScissor[0] = x;
1280-
currentScissor[1] = y;
1281-
currentScissor[2] = width;
1282-
currentScissor[3] = height;
1314+
// save the new scissor box in device space
1315+
currentScissor[0] = ix;
1316+
currentScissor[1] = iy;
1317+
currentScissor[2] = iw;
1318+
currentScissor[3] = ih;
12831319
}
12841320
}
12851321
}

packages/melonjs/src/video/rendertarget/render_target_pool.js

Lines changed: 49 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,11 @@
77
* Renderer-agnostic — the actual RenderTarget creation is delegated to a
88
* factory function provided by the renderer (WebGL, WebGPU, etc.).
99
*
10-
* Camera effects use pool indices 0 and 1 (capture + ping-pong),
11-
* sprite effects use indices 2 and 3.
10+
* Camera effects use pool indices 0 and 1 (capture + ping-pong); sprite
11+
* effects use indices 2 and 3, and each NESTED sprite pass (a multi-effect
12+
* renderable drawn inside another multi-effect renderable) gets its own pair
13+
* above that (4/5, 6/7, …), tracked by a stack of active bases so begin/end
14+
* pairs compose like save/restore.
1215
* Render targets are lazily created and resized to match the required dimensions.
1316
* @ignore
1417
*/
@@ -21,10 +24,23 @@ export default class RenderTargetPool {
2124
this._factory = factory;
2225
/** @type {RenderTarget[]} */
2326
this._pool = [];
24-
/** @type {number} */
25-
this._activeBase = -1;
26-
/** @type {number} */
27-
this._previousBase = -1;
27+
/**
28+
* active pass bases, innermost last — a STACK, so nested begin/end
29+
* pairs unwind correctly (two scalars silently corrupted the pool on
30+
* nested passes: the inner end() popped the outer pass's slot)
31+
* @type {number[]}
32+
*/
33+
this._baseStack = [];
34+
}
35+
36+
/**
37+
* The base index of the innermost active pass, or -1 when none.
38+
* @returns {number}
39+
*/
40+
get activeBase() {
41+
return this._baseStack.length > 0
42+
? this._baseStack[this._baseStack.length - 1]
43+
: -1;
2844
}
2945

3046
/**
@@ -53,16 +69,24 @@ export default class RenderTargetPool {
5369
* @returns {RenderTarget} the capture target (ready to bind)
5470
*/
5571
begin(isCamera, effectCount, width, height) {
56-
const newBase = isCamera ? 0 : 2;
57-
// guard against nested sprite post-effect passes (not yet supported)
58-
if (!isCamera && this._activeBase === newBase) {
59-
return this.get(this._activeBase, width, height);
72+
let newBase;
73+
if (isCamera) {
74+
newBase = 0;
75+
} else {
76+
// each nested sprite pass gets its own capture/ping-pong pair
77+
// (2/3, then 4/5, 6/7, … — lazily allocated)
78+
let depth = 0;
79+
for (const base of this._baseStack) {
80+
if (base >= 2) {
81+
depth++;
82+
}
83+
}
84+
newBase = 2 + depth * 2;
6085
}
61-
this._previousBase = this._activeBase;
62-
this._activeBase = newBase;
63-
const rt = this.get(this._activeBase, width, height);
86+
this._baseStack.push(newBase);
87+
const rt = this.get(newBase, width, height);
6488
if (effectCount > 1) {
65-
this.get(this._activeBase + 1, width, height);
89+
this.get(newBase + 1, width, height);
6690
}
6791
return rt;
6892
}
@@ -72,21 +96,23 @@ export default class RenderTargetPool {
7296
* @returns {RenderTarget|undefined} the capture target, or undefined if no active pass
7397
*/
7498
getCaptureTarget() {
75-
if (this._activeBase < 0) {
99+
const base = this.activeBase;
100+
if (base < 0) {
76101
return undefined;
77102
}
78-
return this._pool[this._activeBase];
103+
return this._pool[base];
79104
}
80105

81106
/**
82107
* Get the ping-pong render target for the current active pass.
83108
* @returns {RenderTarget|undefined} the ping-pong target, or undefined if no active pass
84109
*/
85110
getPingPongTarget() {
86-
if (this._activeBase < 0) {
111+
const base = this.activeBase;
112+
if (base < 0) {
87113
return undefined;
88114
}
89-
return this._pool[this._activeBase + 1];
115+
return this._pool[base + 1];
90116
}
91117

92118
/**
@@ -95,10 +121,10 @@ export default class RenderTargetPool {
95121
* @returns {RenderTarget|null} the parent target, or null if returning to screen
96122
*/
97123
end() {
98-
this._activeBase = this._previousBase;
99-
this._previousBase = -1;
100-
if (this._activeBase >= 0 && this._pool[this._activeBase]) {
101-
return this._pool[this._activeBase];
124+
this._baseStack.pop();
125+
const base = this.activeBase;
126+
if (base >= 0 && this._pool[base]) {
127+
return this._pool[base];
102128
}
103129
return null;
104130
}
@@ -126,7 +152,6 @@ export default class RenderTargetPool {
126152
}
127153
}
128154
this._pool.length = 0;
129-
this._activeBase = -1;
130-
this._previousBase = -1;
155+
this._baseStack.length = 0;
131156
}
132157
}

0 commit comments

Comments
 (0)