Repository navigation
wasm: harden the GPU surface and renderer - #17
Merged
Merged
Conversation
Review of the WebGL2 integration found real bugs in the texture path and pixel-size rounding, plus gaps in what the smoke test could prove. Texture path: - updateTexture built its source view as stride * h, but CUI hands the pointer to the first pixel of the damaged rect (cui_text.c:880), so the span ends at the last pixel of the last row: (h - 1) * stride + w * fmt. The old math read up to x * fmt bytes past the end of the atlas when the rect was not flush left, and threw outright when the source ended near the top of the wasm heap. Same formula CUI's runtime.js uses. - createTexture passed wasm address 0 to texImage2D when CUI creates an empty texture with a null pixel pointer (a fresh emoji atlas does), which uploaded the heap's first bytes as pixels instead of leaving it empty. Pixel size: - PixelWidth/PixelHeight truncated while CUI rounds (vw * scale + 0.5f), so a fractional display scale (1.5) disagreed by a pixel and the renderer refused the frame. Both sides now round, and GLRenderer follows the draw data for the canvas backing store instead of throwing when they drift. - resize() rounds the same way. Surface contract: - GetDrawData now throws ObjectDisposedException: cui_get_draw_data returns &ui->draw_data, so a destroyed context yields a plausible pointer into a freed struct rather than null. BeginFrame's summary, clobbered when CreateGpu was inserted, is restored; Incremental now rejects a GPU surface even when the value is unchanged. Host and test: - The scratch-canvas webgl2 probe is gone: it spent the one attempt the retry loop existed to repeat and under --disable-gpu could silently turn the whole fallback decision. The retry now asks the real canvas. - test-browser-smoke.py asserts the renderer the page reports (--renderer webgl2|2d), so a silent software fallback can no longer pass as the GPU path, and the Pages workflow smoke-tests both renderers and gains a pull_request trigger with the deploy jobs gated to main. - New GpuSurfaceTests pin the contract on desktop CI: atlas upload on first text, in-place update for new glyphs, draw-data lifetime after dispose, the software-only guards, callback validation, and the 1.5-scale pixel rounding. Verified: 697 + 28 tests and format green; both smoke paths pass with the renderer assertion active; a live headless run typed glyphs missing from the atlas and observed two texSubImage2D calls with no exception.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
wasm: harden the GPU surface and renderer
Follow-up to #16, applying its review findings. Stacked on
investigate/wasm-gpuso the diff here is only the fixes.Texture-path bugs (browser-only, would corrupt or crash on real pages)
updateTextureover-read the atlas (main.js): the view was built asstride * h, but CUI hands the callback a pointer to the first pixel of the damaged rect (cui_text.c:880passesatlas + y0 * atlas_w + x0), so the span ends at the last pixel of the last row:(h - 1) * stride + w * fmt. The old math read up tox * fmtbytes past the end of the atlas for any rect not flush left — and threwRangeErroroutright when the source ended near the top of the wasm heap. Same formula CUI's ownruntime.jsuses.createTextureuploaded wasm address 0 when CUI creates an empty texture with a null pixel pointer (a fresh emoji atlas does exactly that,cui_emoji.c:714), which would have uploaded the heap's first bytes as pixels. Null now stays null, as inruntime.js:83.Pixel-size agreement at fractional display scales
CuiSurface.PixelWidth/PixelHeighttruncated while CUI roundswidth_px = vw * scale + 0.5f(cui_core.c:849); the JSresize()truncated too. AtdevicePixelRatio1.5 with an odd CSS width the canvas and the frame disagreed by a pixel, andGLRenderer.drawthrew every frame. C# and JS now round the way CUI does, andGLRenderer.drawfollows the draw data for the backing store instead of refusing the frame (same behavior as CUI'spreview.js:192).Surface contract
GetDrawDatanow throwsObjectDisposedException:cui_get_draw_datareturns&ui->draw_data, so a destroyed context yields a plausible pointer into freed memory, not null.BeginFrame's<summary>, whichCreateGpuhad clobbered when inserted before it.Incrementalnow rejects a GPU surface even when assigning the value it already has.CreateGpunames the offending parameter in itsArgumentException.Test/CI gaps closed
GpuSurfaceTests(7 tests, desktop CI): font-atlas upload on first text, in-placetexture_updatewhen new glyphs appear, draw-data disposal guard, software-only member guards, missing-callback rejection, software surfaceKind, and the 1.5-scale pixel rounding.main.jsis gone — it spent the one attempt the retry loop existed to repeat, so under--disable-gpua slow GPU-process start silently chose software forever. The retry now asks the real canvas.test-browser-smoke.pytakes--renderer webgl2|2dand asserts the renderer the page reports, so a silent software fallback can no longer pass as the GPU path. The Pages workflow smoke-tests both renderers, gains apull_requesttrigger (browser regressions caught before merge), and keeps the deploy jobs gated tomain.GLRendererdeletes its shader objects after linking (ascui_render_gl.cdoes), takeskeepDrawingBufferexplicitly, and the smoke-state machine drops a dead assignment.Verification
dotnet format --verify-no-changes, andregen -NoNative -NoDiffall green.test-browser-smoke.py --renderer webgl2and--renderer 2dboth pass with the new renderer assertion active.∑∫€§Ω≈√∀∃) produced twotexSubImage2Dcalls through the corrected span with no exception, and the rendered field updated.