Repository navigation
Fix WebGPU draws using the destroyed gpu texture after InternalTexture.updateSize - #19123
Conversation
…e.updateSize updateTextureDimensions released the gpu texture right away and created the new one on the same InternalTexture, with the same uniqueId. The material contexts compared the id through the stored object, and the bind group cache is keyed by that id, so both kept returning bind groups over the destroyed texture. - Give the texture a new wrapper for the new gpu texture and release the previous one at the end of the frame, since the current frame's commands may still use it. - Refresh the uniqueId of the texture after the gpu texture is replaced. - Store in WebGPUMaterialContext the id that was bound and compare against it, so rebinding the same object with a new id marks the context dirty.
deltakosh
left a comment
deltakosh
left a comment
There was a problem hiding this comment.
Hello @serdarucel,
This fixes both halves of the issue:
- Deferred release:
updateTextureDimensionsnow detaches the previous wrapper and releases it at the end of the frame, using the existing deferred list.createGPUTextureForInternalTexturebuilds a freshWebGPUHardwareTexturebecause_hardwareTextureis null, and recomputes its format and usages from theInternalTexture, so nothing carried by the old wrapper is lost apart fromtextureAdditionalUsages, which is forwarded. The old comment's concern (releasing the new GPU texture at the end of the frame) no longer applies, since the two wrappers are now distinct. - Cache invalidation: refreshing
uniqueIdmisses the global bind group cache, and storing the bound id in the material context cache catches the same object being bound again with a new id. That also covers_swapAndDie.setTextureis the only writer of that cache entry, so the stored id can't go stale.
LGTM, thanks!
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
|
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/19123/merge/index.html#WGZLGJ#4600 Links to test your changes to core in the published versions of the Babylon tools (does not contain changes you made to the tools themselves): https://playground.babylonjs.com/?snapshot=refs/pull/19123/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/19123/merge#BCU1XR#0 If you made changes to the sandbox or playground in this PR, additional comments will be generated soon containing links to the dev versions of those tools. |
🟢 Memory Leak Test Results4 passed, 0 leaked out of 4 scenarios 🟢 All memory leak tests passed — no leaks detected. Passed Scenarios (4)
|
|
Visualization tests for WebGPU |
|
WebGL2 visualization test reporter: |
⚡ Performance Test Results🟢 All performance tests passed — no regressions detected. |
deltakosh
left a comment
deltakosh
left a comment
There was a problem hiding this comment.
Re-reviewed the delta e6605b9...f2933a7.
updateTextureDimensionsnow reads_originalFormatIsRGBfrom the previous wrapper before releasing it, and sets it on the replacement thatcreateGPUTextureForInternalTexturecreates. A resized RGB raw cube therefore still goes through the RGB->RGBA expansion inupdateRawCubeTexture.- I checked the rest of
WebGPUHardwareTexture. The other fields are either recomputed bycreateGPUTextureForInternalTexture(format,originalFormat,textureUsages), carried over explicitly (textureAdditionalUsages), or lazily rebuilt GPU resources (mipmap generation and invertY copy state). Nothing else is lost when the wrapper is replaced. - The new regression test runs the real
updateRawCubeTextureafter the resize and checks 65,536-byte faces with the expected RGBA layout.
Thanks @Popov72 for catching this one, I missed it in my previous pass.
|
Up @serdarucel |
…ateTextureDimensions
deltakosh
left a comment
deltakosh
left a comment
There was a problem hiding this comment.
Re-reviewed the delta f2933a7...243f829.
updateTextureDimensionsnow keeps theWebGPUHardwareTexturereturned bycreateGPUTextureForInternalTexture, which is declared with that return type and returns the wrapper it attached totexture._hardwareTexture, and sets_originalFormatIsRGBon that local. Because the cast on the null-narrowedtexture._hardwareTextureis gone, the TS2352 that @Popov72 reported no longer applies. My previous approval missed that error, so thanks for catching it.- The runtime behaviour matches f2933a7: the same wrapper receives the same flag.
- Both test stubs of
createGPUTextureForInternalTexturenow return the wrapper, as the real method does, so the tests exercise the new call shape.
LGTM.
Popov72
left a comment
Popov72
left a comment
There was a problem hiding this comment.
Verified the requested fixes and the complete follow-up delta. No new actionable findings.
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). 5 pipeline(s) were filtered out due to trigger conditions. |
🟢 Memory Leak Test Results4 passed, 0 leaked out of 4 scenarios 🟢 All memory leak tests passed — no leaks detected. Passed Scenarios (4)
|
⚡ Performance Test Results🟢 All performance tests passed — no regressions detected. |
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU |
Fixes #19087
After
InternalTexture.updateSize()on a WebGPU texture, every later frame loggedDestroyed texture [...] used in a submitand the mesh using the texture drew nothing.WebGPUEngine.updateTextureDimensionsreleased the GPU texture and created the new one on the sameInternalTexture, keeping itsuniqueId. Two caches then returned the old bind group:WebGPUMaterialContext.setTexturecompared the ids through the stored texture object (the same object, so it always saw "no change"), and the global bind group cache is keyed byuniqueId. The old GPU texture was also destroyed immediately, although the current frame's commands may still use it.Changes:
updateTextureDimensionsgives the texture a new hardware texture wrapper for the new GPU texture, and releases the previous wrapper at the end of the frame (newWebGPUTextureManager.releaseHardwareTexture, which uses the existing deferred release list). The previous code could not defer the release because the wrapper was shared with the new texture.uniqueId, as_swapAndDiealready does.WebGPUMaterialContext.setTexturestores the id that was bound and compares against it, so binding the same object again with a new id marks the context dirty. This also covers_swapAndDie, which refreshes the id on an object that can be bound again.Before: Chrome 156 (WebGPU, NVIDIA), UMD build of master (1fa12be). A 64x64 blue
RawTextureon aStandardMaterialemissive slot, drawn for 10 frames, theninternal.updateSize(128, 128)andtexture.update(red 128x128), then 60 frames, withdeviceuncapturederrorcounted and the center pixel read back:Destroyed texture [... InternalUniqueId0_64x64x1 ...] used in a submit.60 errors in 60 frames, center pixel0,0,0,0. The same when the resize runs insidescene.onAfterRenderObservable(61 errors).After: same page, UMD build of this branch: 0 errors in 60 frames, center pixel
255,0,0,255,uniqueId0 -> 1. The same with the resize insideonAfterRenderObservable(0 errors).Tests:
npx vitest run --project=unit packages/dev/core/test/unit/Engines-> 44 files, 404 passed (1 expected fail already in the suite).prettier --checkandeslinton the changed files report no errors.New tests (
webgpuUpdateTextureDimensions.test.ts, 4 tests): on master 3 fail on their own assertions (expected ... not to be ...for the new wrapper,expected 1 not to be 1for the id,expected false to be truefor the dirty flag), with the change all 4 pass. The unit tests run on stubs without a GPU device, so the real-GPU check above is the one that shows the GPU error is gone.Not covered: #19086 (shadow uniform uploads), which is a separate change.