Repository navigation
Conversation
FillPath, StrokePath, FillShape and StrokeShape queued a draw command without establishing that a device could be had. Context.doFill and doStroke read the nil return as "the GPU has this" and skip the software rasterizer, so the failure only surfaced at Flush, where ensureGPU runs — by which time the pending draws had nowhere to go and were dropped. On a machine with no adapter the result was silent geometry loss: every filled and stroked path vanished while text still rendered, because DrawText and DrawGlyphMaskText make this check before they queue. What came out was a valid image with nothing in it but the labels, and no error anywhere: Stroke had already returned nil. The four queueing entry points now make the same check. It asks for deviceReady rather than gpuReady, because under strategyRasterAtlas the shape pipelines are deliberately absent and Flush dispatches the queue on the CPU — a queued draw still reaches the buffer there. It sits after the compute-mode delegation, which routes to VelloAccelerator and answers for its own readiness. The test is the property the gpu package documents — with the accelerator registered, a chart still renders — asserted on a stroked path. It fails against the old code on any machine without an adapter, and passes on hardware, where the draw goes to the GPU and comes back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qry8YtUUPEPY5fiaAK5QWu
kolkov
left a comment
There was a problem hiding this comment.
@timzifer, sorry these sat for two months with no word from us. Our time went into wgpu and naga, which gg runs on.
All eight have the same CI problem. The runs from September never got approval in time, and GitHub refuses to re-run a workflow older than a month. Every branch is already on current main and merges cleanly, so no rebase is needed, just a fresh push to each, even git commit --amend --no-edit && git push -f. I'll approve the runs as they appear. If you'd rather I push something myself to start them, say so.
Where they stand:
- #534, #539, #540, #541: I've read them, and I have no concerns beyond CI.
- #538: the same. One question there about which draws can split a group.
- #537: needs one more check against an older decision. Details in that PR.
- #542: perf; I'll measure it on CI hardware once it runs.
- #543: overlaps kivutar's earlier #517. Details in that PR.
On this one: the device check matches what DrawText and DrawGlyphMaskText already do before queueing. Dropping geometry silently while text still renders is exactly the kind of failure that's hard to report, so thanks for the minimal repro.
kolkov
left a comment
There was a problem hiding this comment.
The diagnosis holds, and I checked it against the code rather than taking it on trust:
tryGPUFillWithMode(context.go) treats a nil return as "the GPU has it" and only falls through todoFill's CPU path when the call returns an error. So a draw accepted byFillPath/StrokePaththat nothing later dispatches is lost silently. Exactly the failure you describe.DrawTextalready guarded before queueing; the four path/shape calls did not. The asymmetry is real.ErrFallbackToCPUis the right signal:tryGPUFillWithModeandtryGPUOpRCboth turn it into the CPU path, anddoStrokebehaves the same way, so the geometry lands somewhere instead of nowhere.
On the locking: ensureGPU is documented "must be called with s.mu held" and does not take it itself, so deviceUsable taking shared.mu and then calling it is correct — no nested lock. The draw path already takes shared.mu for velloAccel, so one more uncontended lock per draw is not a new hot-path cost.
Two things I'd change before merge:
- Move the guard below the empty-input checks. In
StrokePathit sits aboveif path.NumVerbs() == 0, so an empty path with no device returnsErrFallbackToCPUwhere today it returns nil. Nothing breaks, but the caller ends up running the CPU rasterizer for a path that draws nothing. The same ordering question is worth a glance in the other three. - Say why
deviceReadyand notgpuReadyin the comment, not only in the diff. That distinction is the subtle part for the next reader — rasterAtlas has a device and deliberately no pipelines, and its queue is dispatched on the CPU — and it belongs next to the code.
The test is the right shape: it asserts the package's documented promise (a chart renders with the accelerator registered) rather than an internal detail, and it covers both worlds because CI has no adapter and a developer machine does. I'd add one line to the comment noting that the machine-dependent half is covered by CI.
Approving on the code. The CI runs from September need a fresh push before merge.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Summary
With the GPU accelerator registered and no usable adapter present, every filled and stroked path is silently dropped. Text still renders, so the output is a valid image with the labels in it and none of the geometry — and no error is returned anywhere.
FillPath,StrokePath,FillShapeandStrokeShapequeue a draw command without establishing that a device can be had.Context.doFillanddoStrokeread thenilreturn as "the GPU has this" and skip the software rasterizer. The failure only surfaces atFlush, whereensureGPU()runs, fails withrequest adapter: no adapters available, logs a warning and returnsErrFallbackToCPU— by which point the pending draws have nowhere to go.DrawTextandDrawGlyphMaskTextalready make this check before they queue (gpu_render_context.go:546,:583), which is why text is unaffected and why the symptom looks like a rasterizer artefact rather than a missing device.Minimal reproduction, no external dependencies:
Without the
gpuimport, or aftergg.CloseAccelerator(), the same code draws the line.Changes
internal/gpu:FillPath,StrokePath,FillShapeandStrokeShapecheck for a device before queueing, returningErrFallbackToCPUwhen there is none, so the caller's CPU path runs.FillPathandStrokePathcalls return nil before device initialization, compute delegation or dash fallback, without queueing work.internal/gpu: newdeviceUsable()helper — the checkDrawTextmakes inline, hoisted.deviceReady, notgpuReady. UnderstrategyRasterAtlasthe shape pipelines are deliberately absent while a device exists andFlushdispatches the queue on the CPU, so a queued draw does reach the buffer there. Guarding ongpuReadybreaksTestFlushCPU_TempPixmapDimensionsandTestFlushCPUToView_WithNoop.VelloAcceleratorand answers for its own readiness throughCanCompute(). Guarding before it breaksTestStrokeRouting_ComputeModeUsesVello.gpu/fallback_test.go: regression test.No public API change. Non-empty draws with a ready device keep their rendering path; the readiness check is a short mutex-protected field read.
Testing
The test asserts the property the
gpupackage documents — "If GPU initialization fails, the registration is silently skipped and rendering falls back to CPU" — on a stroked path: with the accelerator registered, the stroke has to be in the buffer. It holds on hardware too, where the draw goes to the GPU and comes back.Against the current code on a machine with no adapter:
TestEmptyPathsDoNotInitializeDevicecovers fill, stroke and dashed stroke in Auto, RenderPass and Compute modes. All nine cases fail before the early returns are added and pass afterwards, checking both the return value and the absence of GPU initialization or queued work.TestDrawOperationsRequireReadyDeviceuses the noop backend to test all four path/shape operations with an allocated device that is not ready, and with a ready rasterAtlas device without shape pipelines. It checks that incomplete setup requests CPU fallback without queueing, while rasterAtlas accepts the draw. Both states run independently of the machine's GPU availability.The current upstream CI runs the GPU packages without
nogpu, both in the pure-Go test step and in the race-detector step. The public fallback test therefore runs in CI; its selected backend depends on the runner.With the fix, across the repository:
Found while rendering charts through a plotting library that offers this package as an opt-in tier; every chart came out with its axis labels and no axes.
Validated on Windows amd64 with Go 1.25.3, branched off
mainat bd563f4.Checklist
go test -race ./...)golangci-lint run --timeout=5m)gofmt.gpu/gpu.go's package comment already promises