feat(software): expose headless surface readback - #276
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
kolkov
left a comment
There was a problem hiding this comment.
Clean, well-designed contribution. A few notes from the review:
API design — The optional hal.PixelReader capability pattern is exactly right. It matches the existing PixelWriter/PixelPresenter family and avoids widening the mandatory hal.Surface interface. The zero-sized HeadlessSurfaceTarget{} integrates cleanly with the typed target system from #273.
Correctness — Verified: owned copy semantics (HAL make+copy), BGRA normalization, state machine enforcement (nil/released/unconfigured/acquired), and cross-backend consistency (native/browser/rust all expose same ReadPixels() ([]byte, error) with clear rejection messages).
Test coverage — 462 lines of tests cover format variants, ownership isolation, triangle rendering readback, state error paths, unsupported backend rejection, and GetFramebuffer backward compatibility. Solid.
One minor observation (not blocking): ReadPixels in surface_native.go has a s == nil guard that the sibling WritePixels method lacks. The nil check is strictly better — consider adding it to WritePixels in a follow-up for consistency.
Meets #256 requirements. LGTM.
Closes #256.
I’m very excited to get this foundation upstream: it makes deterministic, display-free software rendering available through the normal public surface lifecycle, while keeping backend ownership explicit and the API small.
What this adds
wgpu.HeadlessSurfaceTarget, a zero-sized safe target backed by the internalhal.SurfaceTargetHeadlessdelivered in feat(surface): match Rust v29 typed surface targets #273.Surface.ReadPixels(), returning an owned, tightly packed, top-left RGBA8 snapshot after present/discard.hal.PixelReadercapability rather than widening the mandatoryhal.Surfaceinterface.software.Surface.GetFramebuffer()remains as a compatibility alias.This is intentionally a Go software extension, not a claim that Rust
wgpuexposes a headlessSurfaceTarget. Rust-tag and browser builds retain a compatible Go method set but reject the target/readback explicitly. Other native backends remain unchanged and unsupported; ordinary WebGPU GPU readback continues to use texture-to-buffer copies.Public flow
docs/SURFACE-TARGETS.mdcontains the complete compilable acquire/render/submit/present/readback example.Verification
Passed locally with
CGO_ENABLED=0andGOWORK=off:go test -count=1 ./hal/software ./core ../hal/metalgolangci-lint run --timeout=5m --new-from-rev=upstream/main(0 issues)GOOS=js GOARCH=wasmAPI compile64output bytes)git diff --checkThe complete local Apple-silicon
go test ./...currently reaches only the existing threehal/metalprivate-staging fixture expectation failures. The standalone, clean, green portability fix is #275; this PR deliberately does not stack or duplicate that unrelated change. Full coverage reaches the same fixture failure. This branch is based directly on currentmain.This PR supplies the wgpu foundation only; it does not claim to implement the downstream golden-file harness from later phases of the parent roadmap.