From d5ab8ada67fb43574ee2a5956cb53f65c6872fa5 Mon Sep 17 00:00:00 2001 From: Tim <6703801+timzifer@users.noreply.github.com> Date: Mon, 7 Sep 2026 09:13:31 +0200 Subject: [PATCH 1/3] fix(gpu): check for a device before queueing a path draw MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01Qry8YtUUPEPY5fiaAK5QWu --- gpu/fallback_test.go | 58 ++++++++++++++++++++++++++++++ internal/gpu/gpu_render_context.go | 58 ++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+) create mode 100644 gpu/fallback_test.go diff --git a/gpu/fallback_test.go b/gpu/fallback_test.go new file mode 100644 index 00000000..0c079c2f --- /dev/null +++ b/gpu/fallback_test.go @@ -0,0 +1,58 @@ +//go:build !nogpu + +package gpu_test + +import ( + "image" + "testing" + + "github.com/gogpu/gg" + _ "github.com/gogpu/gg/gpu" +) + +// Registering the accelerator must not cost the caller its geometry. +// +// The queueing operations used to accept a draw without checking that a device +// could be had, which made Context.doStroke skip the software rasterizer; the +// failure surfaced at Flush, by which time the draw had nowhere left to go. On +// a machine with no adapter — a container, a CI runner, a VM — that turned +// every filled and stroked path into nothing at all, silently, while text +// (which checks before it queues) still came out. +// +// The property is the one the package documents: with the accelerator +// registered, a chart still renders. It holds on hardware too, where the draw +// goes to the GPU and comes back. +func TestAPathIsDrawnWithOrWithoutADevice(t *testing.T) { + t.Logf("accelerator registered: %v", gg.Accelerator() != nil) + + draw := func(t *testing.T) image.Image { + t.Helper() + c := gg.NewContext(60, 40) + t.Cleanup(func() { _ = c.Close() }) + c.ClearPath() + c.MoveTo(5, 35) + c.LineTo(30, 5) + c.LineTo(55, 35) + c.SetStroke(gg.Stroke{Width: 3}) + c.SetColor(gg.RGBA{R: 1, A: 1}) + if err := c.Stroke(); err != nil { + t.Fatalf("Stroke: %v", err) + } + if err := c.FlushGPU(); err != nil && err != gg.ErrFallbackToCPU { + t.Fatalf("FlushGPU: %v", err) + } + return c.Image() + } + + img := draw(t) + b := img.Bounds() + first := img.At(b.Min.X, b.Min.Y) + for y := b.Min.Y; y < b.Max.Y; y++ { + for x := b.Min.X; x < b.Max.X; x++ { + if img.At(x, y) != first { + return + } + } + } + t.Error("the stroked path is not in the buffer: every pixel is the same colour") +} diff --git a/internal/gpu/gpu_render_context.go b/internal/gpu/gpu_render_context.go index d55ea56a..69fa2ab9 100644 --- a/internal/gpu/gpu_render_context.go +++ b/internal/gpu/gpu_render_context.go @@ -535,6 +535,24 @@ func (rc *GPURenderContext) QueueGlyphMask(target gg.GPURenderTarget, batch Glyp rc.hasPendingTarget = true } +// deviceUsable reports whether a device can actually be had, initializing one +// if that has not been tried yet. +// +// It asks for deviceReady rather than gpuReady, which is what the queueing +// operations need: under strategyRasterAtlas the shape pipelines are +// deliberately absent and Flush dispatches the queue on the CPU, so a queued +// draw still reaches the buffer. What must not happen is queueing with no +// device at all, where nothing will dispatch it. +func (rc *GPURenderContext) deviceUsable() bool { + rc.shared.mu.Lock() + defer rc.shared.mu.Unlock() + if rc.shared.deviceReady { + return true + } + err := rc.shared.ensureGPU() + return err == nil && rc.shared.deviceReady +} + // DrawText shapes and queues text for MSDF rendering (Tier 4). func (rc *GPURenderContext) DrawText(target gg.GPURenderTarget, face any, s string, x, y float64, color gg.RGBA, matrix gg.Matrix, deviceScale float64) error { textFace, ok := face.(text.Face) @@ -710,6 +728,16 @@ func (rc *GPURenderContext) FillPath(target gg.GPURenderTarget, path *gg.Path, p } } + // Refuse the work while no device can be had, rather than queueing it. + // A draw accepted here is drawn by nothing else: doFill and doStroke treat + // a nil return as "the GPU has it" and skip the software rasterizer, so a + // failure discovered later — at Flush, where ensureGPU is called today — + // loses the geometry silently. DrawText already checks before it queues; + // these four did not. + if !rc.deviceUsable() { + return gg.ErrFallbackToCPU + } + if rc.hasPendingTarget && !sameTarget(&rc.pendingTarget, &target) { if err := rc.Flush(rc.pendingTarget); err != nil { return err @@ -759,6 +787,16 @@ func (rc *GPURenderContext) StrokePath(target gg.GPURenderTarget, path *gg.Path, } } + // Refuse the work while no device can be had, rather than queueing it. + // A draw accepted here is drawn by nothing else: doFill and doStroke treat + // a nil return as "the GPU has it" and skip the software rasterizer, so a + // failure discovered later — at Flush, where ensureGPU is called today — + // loses the geometry silently. DrawText already checks before it queues; + // these four did not. + if !rc.deviceUsable() { + return gg.ErrFallbackToCPU + } + if path.NumVerbs() == 0 { return nil } @@ -808,6 +846,16 @@ func (rc *GPURenderContext) FillShape(target gg.GPURenderTarget, shape gg.Detect } } + // Refuse the work while no device can be had, rather than queueing it. + // A draw accepted here is drawn by nothing else: doFill and doStroke treat + // a nil return as "the GPU has it" and skip the software rasterizer, so a + // failure discovered later — at Flush, where ensureGPU is called today — + // loses the geometry silently. DrawText already checks before it queues; + // these four did not. + if !rc.deviceUsable() { + return gg.ErrFallbackToCPU + } + if rc.hasPendingTarget && !sameTarget(&rc.pendingTarget, &target) { if err := rc.Flush(rc.pendingTarget); err != nil { return err @@ -852,6 +900,16 @@ func (rc *GPURenderContext) StrokeShape(target gg.GPURenderTarget, shape gg.Dete } } + // Refuse the work while no device can be had, rather than queueing it. + // A draw accepted here is drawn by nothing else: doFill and doStroke treat + // a nil return as "the GPU has it" and skip the software rasterizer, so a + // failure discovered later — at Flush, where ensureGPU is called today — + // loses the geometry silently. DrawText already checks before it queues; + // these four did not. + if !rc.deviceUsable() { + return gg.ErrFallbackToCPU + } + if rc.hasPendingTarget && !sameTarget(&rc.pendingTarget, &target) { if err := rc.Flush(rc.pendingTarget); err != nil { return err From b1c1047bbe4c841a0e0e2b678d40043cc45c1d22 Mon Sep 17 00:00:00 2001 From: Tim <6703801+timzifer@users.noreply.github.com> Date: Tue, 10 Nov 2026 11:24:41 +0100 Subject: [PATCH 2/3] fix(gpu): skip empty paths before checking device readiness --- gpu/fallback_test.go | 7 +++-- internal/gpu/empty_path_test.go | 49 ++++++++++++++++++++++++++++++ internal/gpu/gpu_render_context.go | 14 ++++++--- 3 files changed, 64 insertions(+), 6 deletions(-) create mode 100644 internal/gpu/empty_path_test.go diff --git a/gpu/fallback_test.go b/gpu/fallback_test.go index 0c079c2f..12106997 100644 --- a/gpu/fallback_test.go +++ b/gpu/fallback_test.go @@ -3,6 +3,7 @@ package gpu_test import ( + "errors" "image" "testing" @@ -22,6 +23,8 @@ import ( // The property is the one the package documents: with the accelerator // registered, a chart still renders. It holds on hardware too, where the draw // goes to the GPU and comes back. +// CI runs this package without the nogpu tag, including the fallback on runners +// without an adapter; the selected backend is allowed to vary by machine. func TestAPathIsDrawnWithOrWithoutADevice(t *testing.T) { t.Logf("accelerator registered: %v", gg.Accelerator() != nil) @@ -38,7 +41,7 @@ func TestAPathIsDrawnWithOrWithoutADevice(t *testing.T) { if err := c.Stroke(); err != nil { t.Fatalf("Stroke: %v", err) } - if err := c.FlushGPU(); err != nil && err != gg.ErrFallbackToCPU { + if err := c.FlushGPU(); err != nil && !errors.Is(err, gg.ErrFallbackToCPU) { t.Fatalf("FlushGPU: %v", err) } return c.Image() @@ -54,5 +57,5 @@ func TestAPathIsDrawnWithOrWithoutADevice(t *testing.T) { } } } - t.Error("the stroked path is not in the buffer: every pixel is the same colour") + t.Error("the stroked path is not in the buffer: every pixel is the same color") } diff --git a/internal/gpu/empty_path_test.go b/internal/gpu/empty_path_test.go new file mode 100644 index 00000000..a1b26481 --- /dev/null +++ b/internal/gpu/empty_path_test.go @@ -0,0 +1,49 @@ +// Copyright 2026 The gogpu Authors +// SPDX-License-Identifier: MIT + +//go:build !nogpu + +package gpu + +import ( + "testing" + + "github.com/gogpu/gg" +) + +// Empty paths must be no-ops even before a device has been initialized. +// In particular, they must not request a CPU fallback or queue an empty draw. +func TestEmptyPathsDoNotInitializeDevice(t *testing.T) { + for _, mode := range []gg.PipelineMode{gg.PipelineModeAuto, gg.PipelineModeRenderPass, gg.PipelineModeCompute} { + for _, operation := range []string{"fill", "stroke", "dashed_stroke"} { + t.Run(mode.String()+"/"+operation, func(t *testing.T) { + s := NewGPUShared() + t.Cleanup(s.Close) + rc := s.NewRenderContext() + t.Cleanup(rc.Close) + rc.pipelineMode = mode + paint := gg.NewPaint() + if operation == "dashed_stroke" { + paint.SetStroke(gg.DashedStroke(2, 2)) + } + path := gg.NewPath() + target := makeTestTarget(60, 40) + var err error + if operation == "fill" { + err = rc.FillPath(target, path, paint) + } else { + err = rc.StrokePath(target, path, paint) + } + if err != nil { + t.Errorf("empty path returned %v, want nil", err) + } + if s.instance != nil || s.device != nil || s.deviceReady { + t.Error("empty path initialized GPU resources") + } + if len(rc.pendingDraws) != 0 || rc.hasPendingTarget { + t.Error("empty path queued work") + } + }) + } + } +} diff --git a/internal/gpu/gpu_render_context.go b/internal/gpu/gpu_render_context.go index 69fa2ab9..21c4acb0 100644 --- a/internal/gpu/gpu_render_context.go +++ b/internal/gpu/gpu_render_context.go @@ -714,6 +714,11 @@ func (rc *GPURenderContext) drawShapedGlyphMaskText(target gg.GPURenderTarget, f // The path is cloned at queue time so the caller can reuse or mutate it. // At Flush time, the path is dispatched to either GPU (convex/stencil) or CPU. func (rc *GPURenderContext) FillPath(target gg.GPURenderTarget, path *gg.Path, paint *gg.Paint) error { + // An empty draw needs neither a device nor a CPU fallback. + if path.NumVerbs() == 0 { + return nil + } + rc.sceneStats.PathCount++ rc.sceneStats.ShapeCount++ @@ -769,6 +774,11 @@ func (rc *GPURenderContext) FillPath(target gg.GPURenderTarget, path *gg.Path, p // StrokePath queues a stroked path as a backend-agnostic draw command (ADR-051). // Dashed strokes fall back to CPU. The path is cloned at queue time. func (rc *GPURenderContext) StrokePath(target gg.GPURenderTarget, path *gg.Path, paint *gg.Paint) error { + // Check before dash fallback and compute routing as well as device setup. + if path.NumVerbs() == 0 { + return nil + } + if paint.IsDashed() { return gg.ErrFallbackToCPU } @@ -797,10 +807,6 @@ func (rc *GPURenderContext) StrokePath(target gg.GPURenderTarget, path *gg.Path, return gg.ErrFallbackToCPU } - if path.NumVerbs() == 0 { - return nil - } - if rc.hasPendingTarget && !sameTarget(&rc.pendingTarget, &target) { if err := rc.Flush(rc.pendingTarget); err != nil { return err From 2500506afdb12d08901f636b8dc022976a7980fd Mon Sep 17 00:00:00 2001 From: Tim <6703801+timzifer@users.noreply.github.com> Date: Tue, 10 Nov 2026 11:31:00 +0100 Subject: [PATCH 3/3] test(gpu): cover device readiness for all draw operations --- internal/gpu/device_readiness_test.go | 74 +++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 internal/gpu/device_readiness_test.go diff --git a/internal/gpu/device_readiness_test.go b/internal/gpu/device_readiness_test.go new file mode 100644 index 00000000..54247edd --- /dev/null +++ b/internal/gpu/device_readiness_test.go @@ -0,0 +1,74 @@ +// Copyright 2026 The gogpu Authors +// SPDX-License-Identifier: MIT + +//go:build !nogpu + +package gpu + +import ( + "errors" + "testing" + + "github.com/gogpu/gg" +) + +// Having a device object is not sufficient to accept a draw: device setup +// must be complete. Conversely, rasterAtlas needs no shape pipelines. +// Use the noop backend so both readiness states run on every machine. +func TestDrawOperationsRequireReadyDevice(t *testing.T) { + for _, ready := range []bool{false, true} { + state := "not_ready" + if ready { + state = "raster_atlas_ready" + } + for _, operation := range []string{"fill_path", "stroke_path", "fill_shape", "stroke_shape"} { + t.Run(state+"/"+operation, func(t *testing.T) { + device, queue, cleanup := createNoopDevice(t) + t.Cleanup(cleanup) + s := NewGPUShared() + s.device = device + s.queue = queue + s.deviceReady = ready + s.strategy = strategyRasterAtlas + rc := s.NewRenderContext() + t.Cleanup(rc.Close) + rc.pipelineMode = gg.PipelineModeRenderPass + paint := gg.NewPaint() + paint.SetStroke(gg.Stroke{Width: 3}) + path := gg.NewPath() + path.MoveTo(5, 35) + path.LineTo(30, 5) + path.LineTo(55, 35) + path.Close() + shape := gg.DetectedShape{Kind: gg.ShapeRect, CenterX: 30, CenterY: 20, Width: 40, Height: 20} + target := makeTestTarget(60, 40) + var err error + switch operation { + case "fill_path": + err = rc.FillPath(target, path, paint) + case "stroke_path": + err = rc.StrokePath(target, path, paint) + case "fill_shape": + err = rc.FillShape(target, shape, paint) + case "stroke_shape": + err = rc.StrokeShape(target, shape, paint) + } + if !ready { + if !errors.Is(err, gg.ErrFallbackToCPU) { + t.Errorf("draw returned %v, want ErrFallbackToCPU", err) + } + if len(rc.pendingDraws) != 0 || rc.hasPendingTarget { + t.Error("draw queued before device setup completed") + } + return + } + if err != nil { + t.Errorf("draw returned %v with a ready rasterAtlas device", err) + } + if len(rc.pendingDraws) != 1 || !rc.hasPendingTarget { + t.Error("draw was not queued with a ready rasterAtlas device") + } + }) + } + } +}