Skip to content

fix(gpu): preserve external content with MSAA - #457

Merged
kolkov merged 1 commit into
gogpu:mainfrom
besmpl:besmpl/preserve-external-content-with-msaa
Jul 30, 2026
Merged

fix(gpu): preserve external content with MSAA#457
kolkov merged 1 commit into
gogpu:mainfrom
besmpl:besmpl/preserve-external-content-with-msaa

Conversation

@besmpl

@besmpl besmpl commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements Strategy C from #455: when an MSAA vector/mixed pass must preserve existing surface content, gg now renders the overlay into the existing multisample attachment with a transparent clear, resolves it into a lazy single-sample texture, and alpha-composites that resolve over the surface.

This preserves external g3d content and earlier same-view gg flushes without duplicating the seven vector/text pipeline variants or reducing stencil-cover antialiasing quality.

What changed

  • keep ordinary frames on the existing direct-to-surface resolve path
  • keep preserved single-sample and blit-only frames on LoadOpLoad
  • route only preserved MSAA vector/mixed frames through the transparent resolve + composite path
  • reuse the existing premultiplied 1x textured-quad pipeline for the final composite
  • support both session-owned submission and the borrowed/shared command encoder used by gogpu
  • lazily allocate the extra 4 B/pixel resolve texture and recreate its binding safely on resize
  • discard transient multisample color storage after resolve
  • clear/discard transient depth-stencil state per pass
  • add regression coverage for external content, earlier gg flushes, direct vs composed routing, the non-grouped path, resize/rebind lifecycle, and MSAA StoreOpDiscard
  • update public comments so preservation behavior is accurate for both 1x and MSAA rendering

The GPU-direct Vello follow-up remains out of scope, but this establishes the render-to-texture -> GPU composite seam discussed in #455.

Why Strategy C

  • full MSAA and stencil-cover quality
  • no public API or session configuration change
  • no dual variants for seven pipelines
  • one coherent path for owned and borrowed encoders
  • the extra texture/pass is paid only when existing surface content actually needs preservation

Validation

  • CGO_ENABLED=0 go build ./...
  • CGO_ENABLED=0 go build ./examples/...
  • CGO_ENABLED=0 go build ./cmd/ggdemo
  • CGO_ENABLED=0 go test -tags nogpu -count=1 ./...
  • go test -race -tags nogpu -count=1 -coverprofile=coverage.txt -covermode=atomic ./... (79.2% total)
  • CGO_ENABLED=0 go vet ./...
  • GOOS=linux GOARCH=amd64 CGO_ENABLED=0 golangci-lint run --timeout=5m (0 issues)
  • focused GPU routing/lifecycle tests on the noop validation backend
  • CGO_ENABLED=0 go test -count=1 -skip '^TestMetalStencil(CoverMasksToShape|RoundedRectMasking|EvenOddMasking)$' ./internal/gpu

The unfiltered local macOS GPU run still reaches the repository's existing Metal-test limitation: the requested Metal adapter resolves to the software backend, which rejects SampleCount=4 in the three skipped stencil tests. The new MSAA route itself passes command validation; CI remains authoritative for the cross-platform builds.

Closes #455

cc @kolkov

@besmpl
besmpl requested a review from kolkov as a code owner July 29, 2026 16:59
@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Render preserved MSAA overlays into a transparent resolve texture and alpha-composite them onto the surface. Reuse the existing premultiplied blit pipeline, support shared encoders, discard resolved multisample storage, and cover routing plus resize lifecycle with regression tests.

@kolkov kolkov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work. Strategy C implemented cleanly — full MSAA quality preserved, zero overhead on ordinary frames, no pipeline duplication.

Architecture highlights

  • prepareSurfacePass() three-way routing (direct/LoadOpLoad/composite) is the right abstraction — single decision point, no if-chains scattered across paths
  • Render pass deduplication via encodeGroupedSurfacePass() eliminates ~167 LOC of copy-paste across three callers
  • Lazy ensureCompositeTexture() — users who never set PreserveContent pay nothing
  • MSAA StoreOpDiscard + Stencil StoreOpDiscard — correct per Skia Graphite pattern, bandwidth saving on tile-based GPUs
  • Old bind group deferred to pendingBindGroupRelease — submit-safe lifecycle

Validated

  • All preserveContent propagation paths covered (RenderFrame/RenderFrameGrouped set it, Flush passes target through)
  • Borrowed encoder path (encodeToEncoder) delegates to the same encodeGroupedSurfacePass — composition works for gogpu shared encoder
  • Multiple mid-frame flushes under preserveContent: each flush does transparent MSAA → resolve → alpha composite; composite pass uses LoadOpLoad — correct accumulation
  • Composite pass uses BlendStatePremultiplied (line 240 image_pipeline.go) — correct for premultiplied RGBA overlay
  • No depth/stencil on composite pass — correct (just textured quad)
  • Error path memory: lazy textures stay in textureSet for reuse/cleanup, no leaks

One question

rp.End() error handling: this PR changes 3 call sites from slogger().Warn to return fmt.Errorf (hard error), but 3 other rp.End() sites in the same file remain warn-and-continue. Is this intentional (only new/changed paths get hard errors) or should all 6 be consistent? Not blocking — just want to understand the intent.

LGTM — approve.

@kolkov

kolkov commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Visual validation: g3d fullscreen-overlay example

Tested locally with g3d/examples/fullscreen-overlay on all backends (Windows, Intel Iris Xe):

Vulkan ✅

3D cube visible, rotating, HUD overlay (title, FPS, crosshair, status bar) renders correctly on top.

image

DX12 ✅

Same as Vulkan — cube visible, HUD overlay correct, 60 FPS.

image

GLES ❌ (pre-existing)

HUD overlay visible (title, FPS, crosshair, status bar) but 3D cube invisible. This is a pre-existing issue — on main without this PR, GLES overlay didn't work at all. PR #457 actually improved the GLES path (HUD now renders), but the g3d GLES renderer may have a separate compositing issue.

image

Summary

Backend 3D Cube HUD Overlay Status
Vulkan Working
DX12 Working
GLES Pre-existing g3d GLES issue
Software Not tested

Strategy C confirmed working on Vulkan and DX12 — the two primary backends.

@kolkov

kolkov commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

GLES root cause found: gogpu/wgpu#284 — depth/stencil not attached to swapchain FBO in setupSurfaceTarget(). Pre-existing bug, not related to this PR. cc @besmpl

@kolkov

kolkov commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Thanks for this clean implementation, @besmpl. Strategy C works exactly as designed — Vulkan and DX12 validated visually.

Regarding the GLES overlay issue — we tracked it down to a pre-existing depth/stencil bug in wgpu's swapchain FBO setup. Since you're on macOS and wouldn't have a way to test the GLES path there, and you've got plenty on your plate with the race detector work, we went ahead and fixed it ourselves: gogpu/wgpu#284gogpu/wgpu#285 (shipped in wgpu v0.30.27).

On the rp.End() error handling consistency question from the review — no need to address it here. If you'd like to unify the remaining 3 call sites to hard errors, a follow-up PR is welcome. Not blocking.

Merging as-is. Thanks again — solid work on both this and the race detector.

@kolkov
kolkov merged commit 3f22e39 into gogpu:main Jul 30, 2026
11 checks passed
@kolkov kolkov mentioned this pull request Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: MSAA resolve overwrites external content — strategy discussion

2 participants