Copy an async texture read at the start of the next frame on request

[mvaligursky]

The copy which ends an async texture read on WebGL is synchronous, and the driver services it by submitting and waiting for whatever commands are outstanding when it runs. The read's own fence has signalled by then, so that wait buys nothing — it is spent on unrelated rendering queued behind the read. A read issued every frame pays it every frame.

Changes:

• Texture#read accepts a frequent option, for a read which is one of many issued every frame or every few frames. On WebGL such a read takes its blocking copy at the start of the next frame, where none of that frame's rendering is queued in front of it, trading a frame of latency for the stall — the trade a one-off read would not want. Ignored on WebGPU, whose readback does not block.
• SceneDepthReader sets it, reading every frame and easily able to absorb a frame of latency.
• A read whose device is destroyed, or whose context is lost, while its fence is still being waited on now answers for itself. A destroyed device settles the read without paying for a copy nobody receives; a lost context fails it, rather than handing back a destination the copy never wrote.
• A copy waiting for a frame which never arrives — rendering stops altogether for a hidden tab — goes ahead on its own after 100ms, so a read always settles.

API Changes:

• Added options.frequent to Texture#read, defaulting to false. Timing is unchanged for every existing caller: Picker, the glTF and USDZ exporters and SOG loading do not set it.
• Behaviour change on context loss, on all reads: a read now rejects instead of resolving with a destination which was never written. Previously getBufferSubData silently did nothing and the caller could not tell the result from a real one — Picker passes no destination of its own, so it received an all-zero pick, which reads as nothing picked rather than as a read which failed. Picker already handles a rejected read; CoreExporter does not, so an export now fails rather than quietly writing a blank texture.

Examples:

• gaussian-splatting/depth-effects gives each in-flight read its own destination array, taken from a free list, instead of sharing one between reads.

Performance:

• Measured on that example, WebGL2: getBufferSubData blocked the main thread for ~40ms on one call per frame, 2.7 seconds of every 3. With the option set the stall is gone.
• WebGPU was never affected, as mapAsync does not block.

[github-actions]

Public API report

This PR changes the public API surface (+1 / −1), per the docs' rules (@ignore / @Private / undocumented are excluded).

Show API diff

-Texture.read(x: number, y: number, width: number, height: number, options?: { data: Uint8Array<ArrayBufferLike> | Float32Array<ArrayBufferLike> | Uint32Array<ArrayBufferLike> | Uint16Array<ArrayBufferLike>; face: number; immediate: boolean; mipLevel: number; renderTarget: RenderTarget }): Promise<Uint8Array<ArrayBufferLike> | Float32Array<ArrayBufferLike> | Uint32Array<ArrayBufferLike> | Uint16Array<ArrayBufferLike>>
+Texture.read(x: number, y: number, width: number, height: number, options?: { data: Uint8Array<ArrayBufferLike> | Float32Array<ArrayBufferLike> | Uint32Array<ArrayBufferLike> | Uint16Array<ArrayBufferLike>; face: number; frequent: boolean; immediate: boolean; mipLevel: number; renderTarget: RenderTarget }): Promise<Uint8Array<ArrayBufferLike> | Float32Array<ArrayBufferLike> | Uint32Array<ArrayBufferLike> | Uint16Array<ArrayBufferLike>>

Informational only — this never fails the build.

[github-actions]

Build size report

This PR changes the size of the minified bundles.

Bundle	Minified	Gzip	Brotli
playcanvas.min.js	2408.1 KB (+0.8 KB, +0.03%)	620.2 KB (+0.2 KB, +0.04%)	481.1 KB (−0.2 KB, −0.03%)
playcanvas.min.mjs	2405.5 KB (+0.8 KB, +0.03%)	618.9 KB (+0.3 KB, +0.04%)	481.0 KB (+0.9 KB, +0.19%)

[mvaligursky]

Automated PR review — Codex (GPT-5)

Reviewed the complete change, public API and readback lifecycle at the current head. I found one actionable WebGL context-loss issue in the new deferred path. The ordinary frame-start, timeout and device-destruction paths otherwise look sound, and the change remains isolated from WebGPU.

Verification: npm run lint, npm run build:types, npm run test:types, git diff --check, and the full unit suite (2626 passing, 2 pending) all pass. GitHub CI and deployment checks are also green.

[mvaligursky]

[P2] Reject queued copies when the WebGL context is lost

After the fence signals, this PBO can remain in _readbackCopies for up to 100 ms. If the WebGL context is lost during that window, AppBase skips frameStart, so the timeout eventually calls copyOut against the lost context; if the context restores first, buf still belongs to the old context. In both cases WebGL can leave pixels unchanged while this code resolves successfully, returning zero/stale data (notably, SceneDepthReader reuses pooled buffers). The non-deferred path did not introduce this extra post-fence window. Please drain these entries as failures from loseContext (using a reject-capable completion rather than copying), so restoration cannot make an old PBO appear valid, and cover the loss-before-copy lifecycle in a test.

[mvaligursky]

🤖 Automated PR review — posted on my behalf by Claude Code (Opus 4.8). Not a human review. The points below are suggestions to weigh as possible improvements, not changes that necessarily need to be addressed.

---

The mechanism is sound: the fence has already signalled by the time the copy is scheduled, so deferring it only moves the getBufferSubData wait, never reorders it ahead of the data; frameStart is the earliest device-level hook the frame has, and super.frameStart() issues no GL commands ahead of the copy loop; and SceneDepthReader's pool is keyed by size and grows on demand, with each buffer owned by its in-flight read until .then — so the extra frame of latency can't recycle a buffer a pending copy is still writing into. deferCopy is threaded only through the WebGL device, as documented.

One real issue on the destroy path (inline, with the fix), and one doc caveat (inline).

[mvaligursky]

This pays the blocking getBufferSubData for data that is guaranteed to be discarded.

super.destroy() on the line above is what sets this._destroyed = true (it's the last statement of GraphicsDevice.destroy). So by the time copy.run() → copyOut() → resolve() lets readPixelsAsync resume and readTextureAsync's .then runs, it takes the if (this._destroyed) { reject(...) } branch unconditionally. The bytes getBufferSubData just waited for are never handed to anyone — the only thing the copy accomplishes here is gl.deleteBuffer(buf), which is moot on a context being torn down.

And the wait is the same one this PR exists to avoid: at destroy time whatever GL work is outstanding (typically the previous frame, if app.destroy() is called from an update-phase script) is what getBufferSubData blocks on — so teardown eats up to a frame's stall per pending deferred read, for nothing.

The read still needs to settle (the comment's goal is right); it just doesn't need to copy to do so. Split the two:

const copy = {
    timer: 0,
    run: () => copy.settle(true),
    // settles the read without touching the GPU - for when the device is going
    // away and the result is going to be rejected anyway
    settle: (doCopy) => {
        if (copied) return;
        copied = true;
        clearTimeout(copy.timer);
        this._readbackCopies.delete(copy);
        if (doCopy) copyOut();
        resolve();
    }
};

and call copy.settle(false) from destroy(). Same outcome for the awaiting caller (rejects with the device-destroyed error), minus the synchronous stall. Note that reordering the loop before super.destroy() wouldn't help — the continuation is a microtask that runs after destroy() returns, so _destroyed is true either way.

[mvaligursky]

Worth qualifying "the start of the next frame" slightly, because it's the start of the render, not the tick. AppBase.render() calls graphicsDevice.frameStart() after the whole update() phase — script update/postUpdate, animationUpdate, input — has already run, and anything those issue to the GPU (texture uploads, mesh/buffer writes, gsplat streaming) is queued in front of this copy and gets waited on.

That doesn't dent the measured result, which is dominated by the frame's drawing, but it does mean the gain is scene-dependent: a project doing heavy update-phase uploads (video textures, streaming) will see the copy block on those. A clause like "ahead of the frame's rendering, though after any GPU uploads made during update" would set the expectation correctly — and if a tick-start hook ever exists, it would be the strictly better home for the copy loop.

[mvaligursky]

🤖 Automated re-review (commit be1bab6) — posted on my behalf by Claude Code (Opus 4.8). Not a human review. The points below are suggestions to weigh as possible improvements, not changes that necessarily need to be addressed.

---

The destroy-path finding is resolved — abandon() settles without copyOut(), and the caller still gets the device-destroyed rejection via readTextureAsync's _destroyed check, as intended. The end(settle) split with three entry points reads cleanly and every path settles exactly once.

The new loseContext() → fail() handling is a good extension, and its reasoning (a caller reusing a buffer between reads can't tell a stale fill from a fresh result) is exactly right — but it leaves one window open, detailed inline on run. One small doc leftover, also inline.

[mvaligursky]

fail() in loseContext() only reaches copies that are already in the Set when the context goes. A read still inside clientWaitAsync at that moment isn't in the Set yet, and it gets there afterwards via a path that then hands back stale bytes as a success:

1. Context is lost → loseContext() fails the (empty-of-this-read) Set.
2. clientWaitAsync's next poll calls gl.clientWaitSync on the dead context. Lost-context calls return 0, which is neither TIMEOUT_EXPIRED nor WAIT_FAILED, so it takes the resolve() branch.
3. readPixelsAsync continues into this deferred block, creates the copy, adds it to the Set and arms the 100ms timer. No frame is coming.
4. The timer fires run() → copyOut() → getBufferSubData is a no-op on the lost context → resolve().
5. readTextureAsync's .then checks only _destroyed, which is false — so it resolves with the untouched destination buffer, i.e. whatever the caller's pixels last held.

That's the precise outcome the fail comment sets out to rule out. Gating run on the flag loseContext() already sets closes it, and covers the timer path for free:

run: () => copy.end(() => {
    if (this.contextLost) {
        reject(new Error('Texture read did not complete, as the WebGL context was lost.'));
        return;
    }
    copyOut();
    resolve();
}),

(or have fail be the thing run delegates to when this.contextLost, to keep the message in one place). For what it's worth, the non-deferred branch above has the same pre-existing hole — copyOut() no-ops and return pixels hands back the stale buffer — so the same contextLost check just before the early copyOut() would fix both paths at once. Out of scope for this PR, but it's a one-liner if you're already here.

[mvaligursky]

The update-phase caveat landed in the frameStart comment ("though after whatever the update phase issued, this being the start of the render") but not here, and this is the user-facing doc where the expectation actually gets set. This block still says just "at the start of the next frame". Worth carrying the same clause over — something like "ahead of that frame's rendering, though after any GPU work issued during the update phase" — so a project doing heavy uploads in update knows why its copy still blocks.

[mvaligursky]

Automated PR re-review — Codex (GPT-5)

Reviewed the complete latest head and the changes since 76bab6e. The follow-up correctly fixes the reported case once a deferred copy is present in _readbackCopies: context loss now rejects it without returning stale bytes, and destruction settles it without paying for getBufferSubData. One related pre-registration teardown race remains, reported inline.

The existing documentation caveat from the Claude Code review also remains applicable: the internal frameStart comment now acknowledges update-phase GPU work, but the public Texture#read JSDoc still describes the copy as being kept out of the frame without that qualification.

Verification: npm run lint, npm run build:types, npm run test:types, git diff --check, and the full unit suite (2626 passing, 2 pending) pass. I also reproduced the remaining race directly against readPixelsAsync; after destruction while its fence promise is pending, the operation subsequently appears in _readbackCopies. GitHub CI and deployment checks are green.

[mvaligursky]

[P2] Do not enqueue a deferred copy after teardown

_readbackCopies only starts tracking this operation after clientWaitAsync has resolved. If destroy() runs while that await is pending, its drain sees an empty set and returns; when the fence later resolves, this line adds the copy to an already-destroyed device. No later frame or destroy pass will abandon it, so the timeout waits another 100 ms and then performs the blocking getBufferSubData whose result readTextureAsync discards because _destroyed is true. I reproduced this transition with a controlled fence promise: the set becomes non-empty after destruction. Check _destroyed immediately after the fence wait and settle/delete the PBO without copying (or register the whole operation before awaiting); the same state/generation guard should prevent a pre-registration context-loss operation from joining a restored context. Please cover teardown both before and after queue registration.

[AlexAPPi]

I recently optimized my data readback strategy in WebGL2 and found the most effective approach. Reading occurs after copying data into a buffer that was populated via transformFeedback, and only after issuing a fenceSync.

The sequence is as follows:

1. Launch transformFeedback to write data into a buffer.
2. Clear a temporary destination buffer.
3. Copy data from the transformFeedback buffer into the cleared buffer (e.g., using copyBufferSubData).
4. Call fenceSync to synchronize GPU command completion.
5. Read the data (via getBufferSubData) after checking sync status, waiting at least 2 frames to avoid pipeline stalls.[registry.khronos](https://registry.khronos.org/webgl/specs/latest/2.0/)

This approach prevents GPU idle time and minimizes latency when reading data back to the CPU. [developer.mozilla](https://developer.mozilla.org/en-US/docs/Web/API/WebGL_API/WebGL_best_practices)

AlexAPPi/playcanvas-opti-pixel@767adbc

[AlexAPPi]

The current readback implementation using setTimeout is inefficient: it causes frequent clientWaitSync calls, adding unnecessary overhead without guaranteeing proper rendering synchronization.

Instead, tie readback to the frame loop—use requestAnimationFrame or a custom frameUpdate. For each buffer, wait for at least 2 full frames before reading. This will genuinely reduce latency and eliminate redundant clientWaitSync calls, since synchronization will follow the display refresh rhythm rather than an arbitrary timer.

[mvaligursky]

Thanks for the ingo @AlexAPPi , looking into this as well.
One issue with your solution not being timer based but being frame based is that it might never resolve in cases where frames do not render constantly - lets say manually driving frames, or a tab being hidden and similar. I'm not sure that is generic enough. I had that solution initially as well, but agent noticed those not being resolved at all when it was driving the page refresh manually. Or even picker running on application that has manual refresh and is not refreshing (editor can reasonably use that)

[mvaligursky]

🤖 Automated re-review (commit b20f862) — posted on my behalf by Claude Code (Opus 4.8). Not a human review. The points below are suggestions to weigh as possible improvements, not changes that necessarily need to be addressed.

---

The lost-context window on the frequent path is closed, and the comment's reasoning holds up: I checked that the new Promise executor body runs synchronously, so _readbackCopies.add(copy) really does land in the same turn as the two guards, with nothing able to slip between them. Handling it with an early guard rather than inside run is the better shape — it also covers the _destroyed case, which previously would have armed a 100ms timer to do a blocking copy on a dead device.

frequent reads better than deferCopy at the call sites, and the example's sample-array pool is sound (bounded by reads in flight, finally returns the array on both the resolve and throw paths, and the ?? new Float32Array fallback makes the empty-pool case allocate rather than hand out undefined).

One thing the fix stops just short of, inline: those guards sit below the early return, so the default path still has the hazard they were written for.

[mvaligursky]

This early return is above the two new guards, so the contextLost case the commit just fixed is still live on the default path — the one every existing caller takes.

Same race, without frequent: context is lost while the fence is being waited on → clientWaitSync on the dead context returns 0, which is neither TIMEOUT_EXPIRED nor WAIT_FAILED, so clientWaitAsync resolves → this branch runs copyOut(), whose getBufferSubData is a no-op on a lost context and leaves pixels untouched → return pixels. readTextureAsync's .then (line 2614) then checks only this._destroyed, never contextLost, so it resolves with a buffer that was never written. Exactly what the guard below describes as "passing off whatever it last held as a result".

Concretely, Picker._readTexture doesn't pass options.data, so readTextureAsync allocates the destination itself and the caller gets an all-zero pick buffer — indistinguishable from a legitimate "nothing was picked". A pick that happens to race a GPU reset comes back as a miss rather than an error.

Hoisting both guards above this block covers both paths with the code already written:

// (guards move up to here, before the `!frequent` early return)
if (this._destroyed) { gl.deleteBuffer(buf); return pixels; }
if (this.contextLost) { gl.deleteBuffer(buf); throw new Error('Texture read did not complete, as the WebGL context was lost.'); }

if (!frequent) {
    copyOut();
    return pixels;
}

The contextLost one is the correctness fix. Moving _destroyed up too is just tidiness — downstream already rejects for that case — but it saves a pointless blocking getBufferSubData on a device that is going away, which is the same wait this PR exists to avoid. The comment above the guards would want a small reword, since "a read is not being tracked below until that wait is over" then applies to both branches rather than only the deferred one.

[mvaligursky]

Automated PR re-review — Codex (GPT-5)

Reviewed the complete latest head b20f862f06eecd5952c021c30a14a3a7fea3fb4e, its discussion, and the changes since the previous review.

The prior frequent-path findings are fixed: reads which finish their fence wait after destruction or context loss now settle without entering the deferred queue, the intent-based frequent API is cleaner than exposing the copy tactic, and the depth-effects example now gives overlapping reads exclusive destination arrays.

One actionable P2 remains, already reported inline by the concurrent Claude Code review at discussion_r3932396109, so I have not duplicated the inline comment. The !frequent early return still runs before the new _destroyed / contextLost guards. I reproduced both cases directly: after simulated context loss the default path resolved the untouched [7, 7, 7, 7] destination as a successful read, and after destruction it still executed the otherwise discarded copy. Hoisting the guards above that early return fixes both paths.

Verification performed:

• Full unit suite: 2626 passing, 2 pending
• npm run lint: passing
• npm run build:types and npm run test:types: passing
• git diff --check: clean
• Completed GitHub build, declaration, unit, build-size, and API-doc deployment checks: passing; several remaining checks were still in progress at review time

The suggestion to replace fence polling entirely with a fixed two-frame delay is not required for correctness: a fixed frame count cannot guarantee completion and does not settle reads when rendering is paused or manually driven. Retaining the fence plus frame-start scheduling and timeout fallback is the safer general mechanism.

[mvaligursky]

🤖 Automated re-review (commit b34f45c) — posted on my behalf by Claude Code (Opus 4.8). Not a human review. The points below are suggestions to weigh as possible improvements, not changes that necessarily need to be addressed.

---

Resolved — the guards now sit above the !frequent early return, so both paths are covered, and the reworded comment is accurate for both (I re-checked that there is no await between the guards and either the copyOut() branch or the synchronous _readbackCopies.add(copy) inside the promise executor).

No further findings. The earlier doc caveat about update-phase uploads is moot now that the frequent wording describes the trade rather than promising "the start of the next frame" — "keeps it from stalling the frame it is issued in" is accurate as written. frequent appears nowhere in the WebGPU or null backends, matching the documented "ignored" behaviour.

[mvaligursky]

Automated PR re-review — Codex (GPT-5)

Reviewed the complete latest head b34f45c8827cd21e0674de0791262ca677778358 and the discussion. The remaining P2 is resolved: the post-fence _destroyed and contextLost guards now run before both the default and frequent branches.

I reproduced all pre-queue combinations for default and frequent reads. Context loss now rejects without copying, destruction settles without copying, normal reads copy once, each path deletes its PBO once, and none leaves queued work. The already-queued run / abandon / fail lifecycle, SceneDepthReader integration, intent-based public API, WebGPU isolation, and depth-effects destination pool also remain sound. I found no further code issues.

Verification performed:

• Full unit suite: 2626 passing, 2 pending
• npm run lint: passing
• npm run build:types and npm run test:types: passing
• git diff --check: clean
• Completed GitHub checks had no failures at review time; the newest CI run was still finishing

Non-blocking PR cleanup: the description still documents options.deferCopy in its Changes and API Changes sections, but the final public option is options.frequent. Please update those sections before merge so the PR record and eventual release-note source match the shipped API.