Repository navigation
Conversation
Both GPU stroke paths — GPURenderContext.preTessellateStroke and VelloAccelerator.StrokePath — filled the expanded stroke outline with EvenOdd. EvenOdd cancels every region the outline covers twice, and an open stroke covers itself wherever it loops or turns back sharply: self-crossing lines, tight zigzags and overlapping joins rendered with holes on the GPU and solid on the CPU. SoftwareRenderer.Stroke fills the same expander output with the paint's rule, NonZero, and is the reference. Closed outlines are an outer and an inner contour of opposite winding (gogpu#372), so rings stay hollow under NonZero; and NonZero does not use the even-odd invert stencil that misrenders on some AMD drivers (gogpu#374). Measured through the public API with the accelerator registered, colour by colour against the CPU: a self-crossing loop 8.4% of its ink wrong before, 0.6% after; a tight zigzag 20.2% before, 0.0% after; a stroked rectangle and circle unchanged at 0.5% and 0.2% (anti-aliasing). New regression test: a self-overlapping stroke through the render context's queue, dispatched to software, compared pixel by pixel with SoftwareRenderer.Stroke. It fails without this change (10008 of 26400 stroked pixels differ) and passes with it. The tests that asserted EvenOdd for strokes now assert NonZero.
The EvenOdd rule that preTessellateStroke used to set was also what kept stroke outlines off the convex fast path: gogpu#347 gated that path on the fill rule because stroke outlines can pass IsConvex without being convex. Filled NonZero, they reach it again. They do pass. At an inner join the expander pivots through the centerline point, a small loop that turns the same way as the outer corners, so the outline of an open stroke with one corner has cross products of a single sign and few enough direction flips. The convex path draws it as one unstenciled triangle fan, and the fan covers the hull between the two arms: an L-shaped line, one corner of a step chart, rendered as a solid wedge on the GPU and as a line on the CPU. preTessellateStroke now tessellates the outline for stencil-then-cover directly. The stencil half of preTessellateFill moves into preTessellateStencil, which both use; fills still try the convex path first. Stroke outlines were never meant to take it, and an open stroke's outline is convex only when it is a single straight segment. New regression test: the outlines of an L, a step with a repeated point, and a collinear line with a repeated point all reach stencil-then-cover with NonZero, not the convex path. Before this change all three took the convex path.
kolkov
left a comment
There was a problem hiding this comment.
This changes a deliberate choice, so I want the history on the table before it goes in. Both GPU stroke paths were switched to EvenOdd in #369 and #374. Those were thin 1 px round-rect strokes that came out as solid boxes. The reasons given then were that the compute path filled the two-contour ring solid under NonZero, and that the stencil fan mishandled inner-join V-shapes under NonZero.
Your argument is the Skia/tiny-skia one: the expander emits the inner contour reversed, so NonZero keeps the ring hollow and fills self-overlaps solid. That's the reference behaviour, and TestStrokePath_OutlineNeverTakesConvexFastPath looks aimed at the #369 failure.
Needed before merge: a test that strokes a 1 px round rect at the sizes from #369, on both the stencil path and VelloAccelerator.StrokePath, and checks that the interior stays empty. With that I'm happy to take NonZero. It's the right rule if the regression stays fixed.
kolkov
left a comment
There was a problem hiding this comment.
This reverses a choice made across three PRs (#369, #374, #376/#378), so I went through the history before judging it. The new argument is stronger than the old one, and the code now says why — that alone is an improvement.
Where I agree, with the evidence:
- The expander output is NonZero geometry.
strokeResultToPathemits an outer and an inner contour of opposite winding, so a ring stays hollow under NonZero, and a region the outline covers twice stays filled instead of being cancelled. EvenOdd is what turned self-overlapping strokes into holes. The CPU stroker has always filled this output with the paint's rule (default NonZero), so the GPU now matches the reference rather than diverging from it. - The AMD history does not speak against this. I read
stencil_pipeline.goon main: the driver bug in #374 wasStencilOperationInvert, used by the EvenOdd parity path, and #378 replaced it withIncrementWrap+WriteMask=0x01. NonZero usesIncrementWrap/DecrementWrapwith0xFF— a different operation on a different mask, and the path that fills have used on the same GPUs all along. The old comment's worry about "NonZero on ring paths" was never reproduced; it should not outweigh a demonstrated correctness bug. - Keeping stroke outlines off the convex fast path is required, not optional. The inner join pivots through the centerline point, so the outline passes a convexity test without being convex, and the fan fills the hull. The L-shaped fixture in
stroke_convex_test.gois exactly the step-chart case that shows it.
Three things before merge:
- Get one AMD run. lkmavi reported #374 on a Radeon 890M, which is the GPU family the old comment worried about. A single stroke scene on
fix/*before release would settle it; I will ask him and point at this PR. If it comes back clean, the comment instencil_pipeline.goshould lose its speculative sentence so nobody re-litigates this. - The CPU side should force the same rule.
SoftwareRenderer.Strokepasses the caller's paint straight intoFill, so a caller who setsFillRuleEvenOddgets EvenOdd strokes on the CPU and NonZero on the GPU after this change. The GPU behaviour is the correct one; make the CPU stroker set NonZero for the expanded outline too (Skia's stroke geometry is winding-filled), and both paths and the paper trail agree. - Measure the cost of dropping the fast path. Simple strokes no longer take the fan. The existing stroke benchmarks before and after would show whether this is noise or a regression worth a follow-up for the convex case only.
The tests are the part that convinced me: TestStrokePath_SelfOverlapMatchesCPUStroker renders the queued path to software and compares it with SoftwareRenderer.Stroke pixel by pixel, with a guard that fails when the reference drew nothing. That is a reference comparison, not a self-referential golden.
Approving on the code, conditional on the AMD check and the CPU-side rule. CI needs a fresh push regardless.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Problem
On the GPU tier, a stroke gets holes wherever it overlaps itself: a line that crosses itself, a tight zigzag, or a thick polyline whose joins fold back. The CPU renders the same strokes solid.
Both GPU stroke paths expand the stroke into an outline and fill it with EvenOdd:
GPURenderContext.preTessellateStroke(render-pass path: convex / stencil-then-cover)VelloAccelerator.StrokePath(compute path)EvenOdd cancels every region the outline covers twice. An open stroke covers itself at every sharp turn, so those regions come out empty.
SoftwareRenderer.Strokepreviously inherited the caller's fill rule: the default NonZero worked, but callers selecting EvenOdd also got holes on the CPU, including in hairline strokes.Fix
Fill expanded outlines NonZero on both GPU paths and in the CPU stroker, including its hairline path. The CPU uses a paint copy so the caller's fill rule and other paint settings stay unchanged. Closed outlines still come out right: the expander emits an outer and an inner contour of opposite winding, so a ring stays hollow under NonZero too.
Stroke outlines stay on stencil-then-cover. Their inner joins can pass the convexity test despite containing small loops, so sending them to a simple triangle fan can fill the hull instead of the stroke. Before this PR the EvenOdd gate already excluded strokes from the convex fast path; the explicit stencil route preserves that behavior when switching to NonZero.
Minimal example
repro/main.go has no dependencies beyond gg and wgpu. It strokes a 22 px zigzag once with the GPU accelerator registered and once on the CPU, then compares the two images pixel by pixel (a pixel counts as wrong if any channel differs by more than 48):
In the wild: figure
We hit this in figure, a grammar-of-graphics plotting library for Go that renders through gg. On the GPU tier, its thick-line charts of dense signals were riddled with holes wherever the line turned back on itself.
The chart below plots 400 samples of a noisy signal with a 5 px line, rendered with figure's GPU tier (
github.com/timzifer/figure/backend/gg/gpu):figure builds against our fork (timzifer/gg, tag
v0.52.6-figure.1) until this lands upstream. Its GPU parity test compares every GPU render against the CPU and now passes.Tests
TestSoftwareStrokeIgnoresFillRule: normal, hairline and non-antialiased strokes produce the same pixels for both caller fill rules, without mutating the paint. All three cases fail before the CPU fix and pass afterwards.TestStrokeRoundRect_QueuedOutlinesStayHollow: checks the exact(40, 40, 300, 120, radius=12)1px stroke from [Bug]: thin (1px) round-rect strokes render as solid filled boxes on GPU (regression of #346/#347) #369 through both queued outlines, including Vello's CPU reference pipeline.TestGoldenSoftware_StrokeRoundRectStaysHollow: renders that stroke through the real GPU stencil andVelloAccelerator.StrokePath/compute pipelines. It checks every pixel in a safe interior rectangle and requires visible ink on all four edges. Both paths pass locally on NVIDIA GeForce RTX 3070 Laptop GPU (Vulkan); neither was skipped.gpuintegrationbuild tag. The existing Mesa/lavapipe golden-test job runs the new regression in a separate test process; its other golden tests retain their existing invocation. Unavailable GPU pipelines are reported as skips, never replaced with a CPU result. Reproduce the hardware check withgo test -tags gpuintegration ./internal/gpu -run '^TestGoldenSoftware_StrokeRoundRectStaysHollow$' -count=1 -v.internal/gpu/stroke_fill_rule_test.go:TestStrokePath_SelfOverlapMatchesCPUStrokersends a self-overlapping zigzag through the GPU render context's queue, then throughdispatchDrawsToSoftware, and compares the result withSoftwareRenderer.Strokepixel by pixel. Without the fix, 10008 of 26400 pixels differ.TestVelloAccelerator_StrokeUsesNonZero/StrokeShapeUsesNonZero(invello_accumulate_test.go) and the pipeline wiring expectation now assert NonZero.go test -race ./...passes (Windows amd64, Go 1.25.3).golangci-lint run --timeout=5mpasses.Performance
BenchmarkProfileStrokePath_EnqueueLine, compared with upstream basebd563f4, six 1-second samples per version, alternating before/after batches on the same machine (Windows amd64, Ryzen 7 5800H, CGO disabled):The median is about 3% higher with overlapping sample ranges; these measurements do not establish a performance regression. This benchmark measures stroke expansion/tessellation and queueing, not GPU execution. The existing
BenchmarkDraw_StrokePathwas not used as rendering evidence becauseStroke()clears its path after the first iteration.Remaining hardware validation
The Radeon 890M run requested in review remains outstanding and is being coordinated by the maintainer. The local NVIDIA result does not replace that check.
In CI run
42965177309, the Mesa job explicitly skipped both native subtests because this adapter was routed to rasterAtlas and exposed neither pipeline. The always-run queued-outline checks still execute in the regular tests; the real GPU pixel evidence is the local NVIDIA run above.The isolated native test passed twice. Running all golden tests together with native backends enabled timed out during cleanup in Vulkan
DestroyInstance, after another test had initialized and destroyed a GPU device; that combined invocation is therefore not used in CI. This PR does not change GPU device lifecycle management.