Skip to content

Multi-texture batching: device-sized pool, shared slot table and texture store - #1588

Merged
obiot merged 16 commits into
masterfrom
maxtextures-1585
Aug 17, 2026
Merged

Multi-texture batching: device-sized pool, shared slot table and texture store#1588
obiot merged 16 commits into
masterfrom
maxtextures-1585

Conversation

@obiot

@obiot obiot commented Aug 16, 2026

Copy link
Copy Markdown
Member

Closes #1585.

Two problems, one subsystem. The batch limit was hardcoded, and — found while benchmarking that — an overflow past the limit re-created and re-uploaded every texture, once per draw, every frame.

The cliff is gone, not moved

512 quads/frame, round-robin over N distinct textures, headless Chromium:

N distinct textures before after
32 (fits the pool) 0.115 ms 0.205 ms
33 (overflows) 2.475 ms 0.230 ms
40 3.313 ms 0.210 ms
64 6.895 ms 0.215 ms

Overflowing now costs about what fitting costs. Reset/draw/GL-call counts are exact; milliseconds are noisy single runs.

What was wrong

The overflow re-built textures. uploadTexture asked one question — is boundTextures[unit] set? — and used it to decide both whether to bind and whether to upload. Since that array held the only reference to the GL handle, a cache reset destroyed the texture. Measured on master, an overflowing frame added 542 createTexture + texStorage2D + texSubImage2D + generateMipmap calls, with the displaced handles left to GC rather than freed.

Attribution ruled out the obvious suspects: forcing 32 flushes with one texture measured faster than a single flush (0.058 vs 0.105 ms), 32 distinct textures that fit cost the same as one, and a bindTexture is 0.015 µs so ~1084 of them is under 1% of the delta.

Lit sprites paid twice. LitQuadBatcher halved the pool and then permanently reserved the upper half for normal maps, because its shader declared two sampler sets and 2n had to fit the device. One lit sprite cost a scene half its units for the session — and the reserved range collided with the top units ShaderEffect and toFrameTexture claim, so an effect calling setTexture before lighting activated aliased onto a normal-map slot and corrupted lit sampling silently.

The pool was capped at the spec floor. 16 is the WebGL 2 minimum for MAX_TEXTURE_IMAGE_UNITS, not a hardware limit.

What changed

Two concerns were conflated; they are now two named, shared, device-free abstractions in src/video/gpu/, beside the existing neutral Batcher:

question keyed by
TextureSlotTable which slot, for this draw? (source, variant)
TextureStore does it exist on the GPU, is it current? source

Both backends run both, and each has one realization of the store — the same shape as Batcher / WebGLBatcher / WebGPUBatcher:

TextureStore                              policy: source -> record, reuse-or-upload, lifetime
  WebGLTextureStore extends TextureStore  GL create + destroy
  WebGPUTextureStore extends TextureStore device create + retire

The WebGL cache delegates slot assignment and keeps residency; the WebGPU batcher's segment keys move onto the table; the WebGPU store is re-keyed from unit to source — safe there before only because that backend builds its cache with no capacity, so units are never recycled. Same coupling, merely unreachable. The lit batcher's normal maps become a third consumer rather than a third implementation.

What is deliberately not unified is the reuse-vs-upload decision itself: WebGPU queue writes execute before recorded draws, so a same-frame content change needs a fresh texture there and does not on WebGL. A real semantic difference, not drift.

Sampler objects. GL bakes wrap and filter into the texture object, which is why one image at two repeat modes needed two uploads (#1448 fixed the correctness half and left the cost). WebGL 2 sampler objects — core in GLES 3.0, unconditionally available since 20.0 dropped WebGL 1, and unused in the tree — move that onto the unit, which is the separation WebGPU already has between GPUTexture and GPUSampler. That is what lets residency be keyed by source at all.

The lit shader addresses one sampler set with two per-quad ids, so the halving and the reservation are gone and the split is dynamic: sprites sharing a normal map cost one slot between them.

maxTextures: "auto" | number application setting, clamped once at renderer construction so the batchers' sampler counts and the cache's capacity cannot disagree. Init-only. Floor of 2, because a lit quad holds a colour slot and a normal slot at once.

Tests

242 files / 5951 passing. New: textureslots (16), texturestore (15), samplercache (6), maxtextures (21), maxtextures-cliff (5), texture-reupload (7), webgl_texture_store (5).

Assertions are on exact counts, not timings. The load-bearing ones were verified to fail against the broken code: restoring the conflated condition makes the re-upload test report 231 uploads instead of 0; driving the lit normal ladder from the colour id drops the readback from green 141 to 81; making the maxTextures setting inert fails 3 of 5 cliff tests; and removing the reservation-skip, the flush-before-evict ordering, or the setCapacity occupancy sweep each break the shared tables.

Later commits fixed two more defects the work itself created, both silent: deleteTexture2D freed handles the store still tracked, so the next resolve handed a dead texture to a draw (GL does not error, it samples black); and the WebGPU records lacked the generation field the base walks, so releaseAll skipped every one of them. Both are pinned.

Three specs that encoded the old mechanism were rewritten rather than deleted — per-texture wrap became per-sampler wrap, "a reset re-uploads" became "a reset does not re-upload", and unit-recycled texture sharing became source independence.

Verified in the browser

All 46 examples load and render on both backends in Chrome (WebGL 2 and WebGPU, Apple M4 Max), confirmed by reading the engine's own boot header rather than assuming which backend ran. Cross-backend comparison of a 4x4 block signature: 34 examples pixel-equivalent, 12 differing by 1-4 per channel (animation phase; compressed-textures legitimately differs since the two backends consume different formats). Zero cases where one backend rendered and the other did not. The only console error, identical on both, is the video example's autoplay-gesture policy.

Also in this PR

A per-quad hot path. Sampler resolution ran once per quad — 512x a frame in the benchmark — building a template-literal key and running two regexes to return the same sampler every time. Memoized behind a three-value guard, plus an allocation-free fast path in uploadTexture (it was constructing an options object and an upload closure per quad even when nothing uploaded). Below the batching limit: 0.100 -> 0.055 ms. Attribution was measured, not guessed — the wider 32-sampler shader ladder turned out to cost nothing.

An API regression, caught by diffing the generated types against master. WebGLRenderer.maxTextures had lost its readonly modifier, because the new fields were inserted between that property's JSDoc block and its assignment. index.d.ts is otherwise byte-identical to master: Renderer and QuadBatcher unchanged, and TextureCache / MaterialBatcher / LitQuadBatcher are not exported from the entry point, so reshaping them is internal.

Test-harness hygiene. 56 spec files now release the WebGL context they build, and three wrong claims in the harness documentation are corrected — the helper and the vitest config both asserted that spec files share one browser page so contexts accumulate across the run, which a probe disproves (vitest isolates each file). src/system/device.ts also shipped a published JSDoc example calling me.video.init(), an API that no longer exists. One spec that has failed intermittently in CI checked handler identity before checking a handler existed, so a context-acquisition failure reported itself as "reused a previous handler"; that now says what actually happened.

To be clear about scope: this does not claim to fix the intermittent CI failure. The leak theory behind that diagnosis was disproved during the work, and the cause is still unknown.

Notes

  • Needs a device reporting more than 16 units to show the pool change. Apple Silicon reports 16 via ANGLE/Metal (MAX_COMBINED is 32 — 16 fragment + 16 vertex — which is the likely origin of widely-quoted "32 texture units" figures). On those machines the gain is the lit path, which ran at an effective pool of 8, plus the overflow fix, which applies everywhere.
  • Eviction policy was measured and deliberately left alone — see Eviction policy: measured, not worth changing (see #1587) #1586. The cost was never the flushes or the binds.

🤖 Generated with Claude Code

https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi

obiot and others added 7 commits August 16, 2026 08:19
Both backends solve the same problem — many textures, one draw call — by
giving each texture a small integer slot the fragment shader selects on per
vertex. Only the resource differs: a WebGL texture unit, or an entry in a
WebGPU material bind group. The assignment policy was written twice, in
`TextureCache.allocateTextureUnit` and `WebGPUQuadBatcher.segmentSlotFor`,
which is how the two drifted.

`TextureSlotTable` now owns that policy and nothing else: capacity, key to
slot admission, the free-slot search that skips reservations, and the
flush-then-evict behaviour on exhaustion. It never touches a GL unit or a
bind group — each backend supplies the binding through callbacks, so the
same suite proves both behave alike.

The WebGL cache keeps residency (the source-keyed index, tinted variants,
the atlas cache) and its reservations, and delegates slot assignment.
`usedUnits` and `max_size` become views onto the table so the existing
surface is unchanged. WebGPU's segment keys move onto it wholesale.

Two entry points rather than one: `slotFor(key)` for WebGPU, which keys
directly, and a keyless `claim()` for the WebGL cache, which resolves
textures through its own source-keyed index and needs the policy without
the key map. Both overflow on the same rule.

Behaviour is unchanged on both sides; the overflow policy is now a single
pluggable path, so changing it changes both backends at once.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi
`LitQuadBatcher` halved its texture budget and then permanently reserved
the upper half for normal maps, on first bind, for the rest of the session.
Any scene drawing one lit sprite gave up half its units — whatever the
device reported — and the reserved range collided with the top units
`ShaderEffect` and `toFrameTexture` claim, so an effect that called
`setTexture` before lighting first activated aliased onto a normal-map
slot and corrupted lit sampling with no error.

The cause was the fragment shader: it declared `uSampler0..n-1` AND
`uNormalSampler0..n-1`, so 2n samplers had to fit the device, and the
pairing was positional — the normal for colour slot `i` lived at unit
`n + i`. But `aNormalTextureId` is already a distinct per-quad attribute,
so the positional pairing was never necessary.

The shader now addresses ONE sampler set with two independent ids, and a
normal map takes a slot from the shared pool like any other texture. The
halving and the reservation are gone, and the split is dynamic: sprites
sharing a normal map cost one slot between them rather than every scene
paying half its budget upfront. On a 16-unit device a lit scene goes from
an effective pool of 8 to 16.

`addQuad` re-resolves the colour unit when claiming the normal's slot
exhausts the pool, and falls back to the unlit path rather than let the
two ids collide when reservations leave a single assignable slot — the
alternative is sampling the sprite's own albedo as a normal map, silently.

Verified by pixel readback: a flat normal under an overhead light reads
green 141, and driving the normal ladder from the colour id — the
collision — reads 81. The existing lit assertions are all `> 40` and
cannot tell those apart.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi
The multi-texture pool was capped at `Math.min(device, 16)`. 16 is the
WebGL 2 / GLES 3.0 spec FLOOR for MAX_TEXTURE_IMAGE_UNITS — chosen because
it works everywhere — and plenty of hardware reports more. The fragment
shader is already generated for the count, so the cap was policy, not
structure.

`resolveMaxTextures` clamps once, at renderer construction, before the
batchers and the TextureCache are built — so their sampler counts and the
cache's capacity cannot disagree. `maxTextures: "auto" | number` overrides
it: "auto" takes the device limit capped at a conservative 32, a number is
clamped to what the device actually has (declaring more sampler2D than
units exist fails to LINK, at startup). Init-only — the batchers compile
their shaders against it — so unlike textureFilter there is no runtime
setter.

The floor is 2, not 1: a lit quad holds a colour slot and a normal slot at
once, and a one-slot pool makes the two ids collide.

ShaderEffect's extra samplers and toFrameTexture's scratch unit now count
down from the RENDERER's top unit rather than the active batcher's, so
which batcher happens to be bound no longer decides where they live.

Measured against 19.9.1 on a 32-unit device, 512 quads/frame round-robin
over 32 distinct textures: 32 draw calls and 32 cache evictions per frame
become 1 and 0. Read it as a threshold, not a speedup — nothing got
faster, the cliff moved from 17 textures to 33.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi
Second half of the pair started by TextureSlotTable. There are two separate
questions about a texture:

  which slot is it in, for this draw?   transient, bounded by sampler count
  does it exist on the GPU, is it current?   persistent, bounded by disposal

The WebGL backend answers both with one field — `boundTextures[unit]` — so
dropping a slot assignment destroys the handle, and the next draw rebuilds
the texture from scratch. Past the batching limit that is a full re-upload
and mip regeneration per quad, every frame.

`TextureStore` owns the source-to-record index, the reuse-or-upload
decision, and lifetime bookkeeping. It touches no GL or WebGPU API: each
backend supplies onCreate/onUpload/onDestroy, so the decision logic is
identical on both and testable without a device.

Three guards are structural rather than conventional, each for a failure
mode that is silent when it happens:

- a fresh record's version starts unmatchable, so it always uploads once —
  a record that exists but was never filled samples as garbage
- `onUpload` may return a REPLACEMENT handle, because immutable storage
  cannot be respecified and a shape change forces a new GPU object
- `releaseAll()` clears in place and bumps a generation; callers holding a
  handle of their own check `isCurrent()`. Responding to a restore by
  constructing a replacement store orphans everything the old one tracked,
  so the SECOND loss leaks it all — and both WebGL batchers re-run `init()`
  on restore, which is exactly how that gets written by accident.

No consumer yet; adoption follows.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi
GL bakes wrap and filter into the texture OBJECT. That is why one image
drawn at two repeat modes needs two texture units — and, more expensively,
two uploads and two storage allocations (#1448 fixed the correctness half
and left the cost).

WebGL 2 sampler objects move that state onto the unit instead, which is the
separation the WebGPU backend already has between GPUTexture and GPUSampler.
They are core in GLES 3.0 and unconditionally available since 20.0 dropped
WebGL 1; the tree used none.

`GLSamplerCache` dedupes by `(filter, wrapS, wrapT, mip)` — deliberately the
same key as the WebGPU store's `getSampler`, so the two read alike. A scene
has at most two filters times four repeat modes however many textures it
loads, so the set stays tiny. Renderer-owned, shared by every batcher, and
cleared rather than replaced on context loss.

`uploadTexture` now binds the sampler for the variant it resolved. The
texture parameters are still set at upload, so any path that binds no
sampler behaves exactly as before — a bound sampler simply wins at sample
time.

This is groundwork: it makes it LEGAL for one texture to serve every variant
of a source, which is the precondition for keying residency by source alone.
The shared upload is not realized until TextureStore is adopted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi
Past the batching limit a scene did not re-BIND its textures, it re-BUILT
them: 542 createTexture + texStorage2D + texSubImage2D + generateMipmap
calls per frame on a 512-quad scene, with the displaced handles dropped
unreferenced rather than freed. Frame time ~0.10 ms -> ~2-3 ms.

The cause was one over-loaded condition. `uploadTexture` asked a single
question — is `boundTextures[unit]` set? — and used it to decide BOTH
whether to bind and whether to upload. Since that array was the only
reference to the GL handle, a cache reset destroyed the texture, and the
next draw rebuilt it from scratch.

Those are two independent decisions and they are now written as two:
whether this unit already holds the texture decides a bind; whether the
source's content is current decides an upload. Residency moves to the
renderer-owned `TextureStore`, keyed by SOURCE, so a texture that merely
moves units costs a bind.

Renderer-owned rather than batcher-owned deliberately: both batchers re-run
`init()` on context restore, so a batcher-owned store would be REPLACED
there, orphaning every handle it tracked and leaking them on the next loss.
It is cleared, never reconstructed.

Also re-keys the WebGPU store from unit to source. It was safe there only
because that backend builds its cache with no capacity, so units are never
recycled — the same coupling, merely unreachable. One consequence: two
sources that happened to share a unit no longer share a GPU texture, which
was the clobber-then-re-upload mechanism in miniature.

`reset()` releases through the store rather than by walking the per-unit
array, which would now miss unassigned textures and double-free assigned
ones; the lit batcher frees its own normal-map handles for the same reason.

Verified by GL-call diff rather than timing: an overflowing frame must add
zero create/upload/mipmap calls over one that fits, and no deleteTexture
either — trading an upload storm for a delete storm would be no better.
Restoring the old conflated condition makes that test report 231 uploads.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi
Copilot AI lite review requested due to automatic review settings August 16, 2026 00:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Auditing the new API for unwired surface turned up a real defect and two
untested paths.

`deleteTexture2D` deleted the GL texture directly, but the store owns that
handle now — so the record outlived the texture and the next resolve for
that source handed a DEAD handle to a draw. GL does not error on a deleted
texture, it samples black. It now frees through `destroyTexture`, and
invalidates the per-unit beliefs rather than leaving them pointing at
nothing.

`markTextureDirty` — how a re-baked canvas (Text, gradients, any dynamic
surface) announces new pixels behind an unchanged source object — is live
from `CanvasRenderTarget` and had no coverage at all. Removing its force
broke no test. It is pinned now, including that the re-upload lands in the
SAME handle: with immutable storage a same-shape re-upload is a pure
texSubImage2D, and a new texture object would mean the storage was thrown
away, which is the churn this all exists to stop.

Also drops `TextureStore.isCurrent` and `invalidate`. Both were added on
reasoning about hazards rather than a caller, and neither acquired one —
shipping unused API next to tests that exercise only itself is how the next
reader learns to distrust the suite.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi
Copilot AI review requested due to automatic review settings August 16, 2026 02:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Closes the sharing the earlier commits left half-done. Residency was written
three times: the WebGL colour path, the WebGPU store, and the lit batcher's
normal maps. The first two were unified in design only — WebGL used the
shared base directly with callbacks inlined in the renderer constructor,
while WebGPU kept its own records map, generation and release code. That
asymmetry was build order, not design: it left WebGL texture concerns with
no home, which is why the normal-map path had to repeat the callbacks.

Now `TextureStore` is a base whose three hooks are overridable METHODS
(constructor injection still works, which is how the tests drive the policy
with no GPU), and each backend has a realization — the same shape as
Batcher / WebGLBatcher / WebGPUBatcher:

  WebGLTextureStore extends TextureStore      GL create + destroy
  WebGPUTextureStore extends TextureStore     device create + retire

Both WebGL stores are that subclass: the renderer's colour store every
batcher shares, and the lit batcher's normal-map store, which drops its
hand-rolled source->{tex, version} map.

Upload stays a per-call closure on the WebGL side. A GL upload needs the
BATCHER's `createTexture2D` — target unit, immutable-storage shape, texture
swap on shape change — and none of that is residency.

What is deliberately NOT unified is the reuse-vs-upload decision itself.
WebGPU queue writes execute before recorded draws, so a same-frame content
change needs a fresh texture there and does not on WebGL. That is a real
semantic difference, not drift, and forcing one code path would have to
special-case it anyway.

Two bugs this surfaced: the WebGPU records lacked the `generation` field the
base walks, so `releaseAll` skipped every one of them silently; and the
cache-reset handler needed `releaseAll(true)`, since the base defaults to
NOT destroying — right for a lost context, wrong for a live device.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi
Copilot AI review requested due to automatic review settings August 16, 2026 10:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Measured against 19.9.1 on the same machine: past the multi-texture limit
the old path re-created and re-uploaded ~544 textures per frame and
regenerated their mip chains; the new one does zero at every N.

Includes the two caveats rather than only the win: below the limit the new
path is slower (0.045 -> 0.100 ms at 512 quads), and the millisecond figures
are medians that still vary by ~25% while the call counts are exact.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi
Copilot AI review requested due to automatic review settings August 16, 2026 23:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

The below-limit regression the previous benchmark flagged, measured and
mostly removed. Attribution first, because the guess in the changelog was
wrong: the 32-sampler shader ladder costs nothing (pool 32 and pool 16
measure the same, 0.102 vs 0.105 ms), and `bindSampler` is nearly free.

It was `GLSamplerCache.get`. Called once per QUAD — 512 times a frame in
the benchmark — running two regexes and building a template-literal key to
return the same sampler every time. String work in the hottest path.

Memoized behind a three-value guard, plus per-unit tracking so a redundant
bind is skipped. That alone recovered all of it: 0.100 -> 0.060, exactly
matching a variant with samplers removed entirely.

`uploadTexture` also allocated an options object and an upload closure per
quad even when nothing uploaded. It now peeks first and constructs those
only on an actual miss: 0.060 -> 0.055.

512 quads/frame, 16 distinct textures, median of 7:

  19.9.1                0.045 ms
  before this commit    0.100 ms   (+122%)
  after                 0.055 ms   (+22%)

The overflow case improves too, 0.220 -> 0.163 ms. The residual is the
per-source residency lookup — the cost of not re-uploading 544 textures a
frame, which is the trade the whole change exists to make.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi
Copilot AI review requested due to automatic review settings August 16, 2026 23:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Two things an API audit against master turned up.

`WebGLRenderer.maxTextures` lost its `readonly` modifier in the published
types. The new sampler-cache and texture-store fields were inserted BETWEEN
that property's JSDoc block and its assignment, so `@readonly` ended up
documenting `samplerCache` instead. Moved back.

The rest of the audit is clean: `index.d.ts` is byte-identical to master,
`Renderer` and `QuadBatcher` are unchanged, and `TextureCache`,
`MaterialBatcher` and `LitQuadBatcher` are not exported from the entry
point, so reshaping them is internal. The only remaining delta on
`WebGLRenderer` is two `@ignore` fields, which that class already carries
fifteen of.

Also covers the per-quad memoization added in the previous commit. Both
halves are caches over GL objects, which is where a stale entry renders
wrongly rather than erroring:

- the memo must not answer for different arguments (a last-value cache
  invites exactly that)
- releasing must drop it, or a DELETED sampler goes straight to the next
  draw
- the bind skip is per unit, not global, and a real transition — including
  to null — still issues

Verified by mutation: ignoring the repeat argument, or making the bind skip
global, each fail. The reset block is load-bearing as a unit rather than
line by line, since `lastFilter = -1` alone already invalidates the memo —
removing the whole block fails two tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi
Copilot AI review requested due to automatic review settings August 16, 2026 23:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

The magnification filter had no coverage at all. Two mutations proved it:
never setting `TEXTURE_MAG_FILTER`, and hardcoding it to NEAREST, both
passed the suite — a bug that renders every sprite nearest-neighbour,
shipping silently.

Four cases added:

- both parameters, for both filters, so MAG cannot drift from MIN
- asking for mips changes MINification only; a chain says nothing about
  drawing a texture larger than itself
- `get(filter)` defaults match the spelled-out form exactly, and resolve to
  the SAME sampler — otherwise the per-quad memo would treat two spellings
  of one state as different and mint a second object
- an unrecognized repeat string clamps rather than tiling

Re-verified by mutation: dropping the MAG call, hardcoding it, and flipping
the default repeat each now fail.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi
Copilot AI review requested due to automatic review settings August 16, 2026 23:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Both new specs construct their own Application, and neither released it.
Browsers cap live WebGL contexts, so a suite that leaks one per spec
eventually cannot create any — and that surfaces as UNRELATED specs failing,
including the `webgl_available` tripwire, rather than as the leaking spec.

Seen for real during this branch's mutation testing: three specs failed
together after a run of rapid context creation, and passed on a re-run with
no source change.

This does not fix the wider pattern — many existing specs build an
Application and never destroy it — only the two added here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi
Copilot AI review requested due to automatic review settings August 16, 2026 23:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

The intermittent CI failures — the ones that surface as a 90s beforeAll
timeout in getWebGLRenderer, or as the `webgl_available` tripwire, rather
than as an assertion — are context starvation. Browsers cap live WebGL
contexts, and 61 spec files built an Application and never released it, so
the count climbed for the whole run and whichever spec happened to be next
when the cap was hit failed for reasons unrelated to what it tests.

The dominant shape was `const app = new Application(...)` INSIDE a
beforeAll: local to the hook, so unreachable from any teardown. Hoisted the
binding out per describe block and added an afterAll (afterEach where the
app is per-test) that releases it. 56 files, 72 describe blocks.

A previous fix had already addressed the other half of this: the teardown
hooks that construct a fresh Canvas Application to hand later spec files a
clean default were converted from AUTO precisely so they stop taking a GL
context. Verified none of those regressed — every reset-only app is still
Canvas.

Five describe blocks are deliberately left leaking. Two need their app past
teardown (adding a destroy fails their context-loss cases); the rest build
apps inside individual `it()` blocks or have no describe-scope binding, and
`destroy()` is terminal, so a wrong release is worse than a leak. Specs that
legitimately call destroy() as the thing under test are untouched.

Verified with three consecutive full runs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi
Copilot AI review requested due to automatic review settings August 17, 2026 00:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

…ause

Three documentation claims in the test harness were wrong, and the wrong one
was load-bearing: it produced a diagnosis of the intermittent CI failure that
does not hold.

- `tests/helpers/webgl-context.js` said vitest runs every spec file in one
  page, so WebGL contexts accumulate across files until the browser's cap
  force-loses the oldest and a later `beforeAll` stalls. Measurably false: a
  probe shows a global set in one file is `undefined` in the next, and a
  context opened in one is already lost by the next. Vitest isolates each
  spec FILE. Contexts accumulate only WITHIN a file, across describe blocks.
- `vitest.config.ts` repeated the same claim.
- `src/system/device.ts` carried a JSDoc `@example` calling `me.video.init()`
  — an API that no longer exists — on an already-deprecated function. That
  one is user-facing, since it ships in the published docs.

All three now say what was measured, and the helper carries an explicit note
not to rebuild the starvation theory from it. The helper itself is still
worth using, for a different reason: acquisition through a software
rasterizer is genuinely slow, so creating fewer contexts saves real time.

`application_lifecycle` is the one spec that has failed intermittently here.
It built SIX WebGL applications in a loop — the most context churn in the
suite — and checked handler identity before checking a handler exists, so a
cycle that failed to obtain a renderer reported "reused a previous handler".
That message sent this investigation the wrong way. Three cycles prove the
property just as well, and the existence check now runs first, so a failure
says what actually happened.

The cause of the CI flake remains unknown. The `hookTimeout` note — that
acquisition can take tens of seconds under load — is the better lead.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi
Copilot AI review requested due to automatic review settings August 17, 2026 04:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@obiot
obiot merged commit 6304db3 into master Aug 17, 2026
6 checks passed
@obiot
obiot deleted the maxtextures-1585 branch August 17, 2026 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multi-texture batching: device-sized pool, shared slot table, and a shared TextureStore

2 participants