Skip to content

fix(vulkan): begin depth-only render passes instead of faulting on End - #353

Merged
lkmavi merged 1 commit into
gogpu:mainfrom
dvoyni:fix/vulkan-depth-only-render-pass
Sep 7, 2026
Merged

lkmavi merged 1 commit into
gogpu:mainfrom
dvoyni:fix/vulkan-depth-only-render-pass

Conversation

@dvoyni

@dvoyni dvoyni commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

What happens

A render pass with a depth attachment and no color attachment kills the
process. Not a validation error — a fault: an access violation inside the
driver, several frames of stack away from anything that names a pass.

github.com/gogpu/wgpu/hal/vulkan/vk.(*Commands).CmdEndRenderPass
github.com/gogpu/wgpu/hal/vulkan.vkCmdEndRenderPass       hal/vulkan/command.go
github.com/gogpu/wgpu/hal/vulkan.(*RenderPassEncoder).End hal/vulkan/command.go
github.com/gogpu/wgpu/core.(*CoreRenderPassEncoder).End   core/command.go

A depth-only pass is legal in WebGPU and is how a shadow map is drawn. A
browser's WebGPU encodes the same descriptor correctly, so this is specific to
the Vulkan HAL.

Reproduced on an NVIDIA GeForce RTX 4070 SUPER, so despite the Intel-driver
workarounds nearby this is not driver-specific — vkCmdEndRenderPass without a
matching begin is undefined behavior on any implementation.

Why

Two defects, and the second is the more serious because nothing asked for it.

1. The pass is never begun. BeginRenderPass derived the render area and
the sample count from the color attachments alone, so it bailed out before
vkCmdBeginRenderPass:

if e.active == 0 || len(desc.ColorAttachments) == 0 {
    return rpe
}

RenderPassCache.createRenderPass and createFramebuffer already handle
ColorCount == 0 correctly — a depth-only subpass with PColorAttachments nil
is exactly what they build. Only the encoder refused.

2. End ends a pass that was never begun. It called vkCmdEndRenderPass
whenever the command buffer was active, regardless of whether a pass had been
begun. That also fires on the two other early returns in BeginRenderPass
a descriptor whose attachments do not resolve, and a render pass or framebuffer
that could not be created — so those were already unsafe, independently of
depth-only passes.

What this changes

  • renderAreaView picks the attachment that defines the pass: the first color
    attachment carrying a view, falling back to the depth/stencil attachment when
    the pass declares no usable color attachment. Returns nil when nothing
    resolves, and BeginRenderPass then declines rather than encoding a pass with
    a zero extent.
  • attachmentSampleCount reads the sample count off that same view. This also
    consolidates two near-identical loops that walked the color attachments
    separately for extent and for samples.
  • RenderPassEncoder.begun records whether vkCmdBeginRenderPass actually ran,
    and End is gated on it.

Rust wgpu-hal has no equivalent of the first two: its RenderPassDescriptor
carries extent and sample_count as caller-supplied fields, so
begin_render_pass reads them straight off the descriptor. This HAL's
descriptor carries neither, so a pass has to derive both from its attachments —
and a depth-only pass has only the depth attachment to derive them from.

Tests

hal/vulkan/renderpass_depthonly_test.go — no GPU, runs in the default suite:
attachment selection (color precedence, sparse color arrays, the depth
fallback, typed-nil and foreign views), sample-count derivation, and both
directions
of the End guard — that it does not end an unbegun pass, and
that it still ends a begun one.

depth_only_pass_repro_test.go — behind the integration tag, following
arraylength_repro_test.go: clears depth in a color-less pass and reads the
value back, so it distinguishes "did not crash" from "actually ran". It skips
when no Vulkan adapter is available, and skips if the selected adapter is not
Vulkan, since another backend would make it pass without proving anything.

Every test was mutation-checked in both directions. Two findings came out of
that and are folded in: an earlier begun-flag test could only be falsified in
one direction until the positive case was added, and a redundant || v == nil
guard in the view-narrowing helper turned out to be unfalsifiable dead code —
a failed type assertion already yields a nil *TextureView — so it is gone and
the remaining comma-ok is covered by the foreign-view case.

Verification:

go build ./...                                  ok
go test ./...                                   22 packages ok
go vet ./hal/vulkan/...                          clean on linux, darwin, windows
go test -tags integration -run TestDepthOnlyRenderPassExecutes .   ok (RTX 4070 SUPER)

CHANGELOG.md is deliberately untouched — happy to add an entry if you would
rather it land with the PR.

What is still not fixed

Nothing here changes the GLES or software backends. Their len(ColorAttachments) == 0 checks are unrelated — they resolve a target texture and correctly report
"none" — not a begin/end mismatch.

🤖 Generated with Claude Code

A render pass with a depth attachment and no color attachment was never
begun, and then faulted inside the driver when it was ended.

BeginRenderPass derived the render area and sample count from the color
attachments alone, so it returned an encoder without ever calling
vkCmdBeginRenderPass whenever ColorAttachments was empty. End called
vkCmdEndRenderPass regardless. That is undefined behavior: it surfaces as
an access violation inside the driver rather than a validation error,
several frames of stack away from anything that names a pass.

Derive both from the depth/stencil attachment when a pass declares no
usable color attachment, so a depth-only pass -- how a shadow map is
drawn -- is encoded and executes. RenderPassCache and the framebuffer
cache already handled zero color attachments; only the encoder did not.

Gate End on whether vkCmdBeginRenderPass actually ran. That also covers
the two other early returns in BeginRenderPass, neither of which was
safe before: a descriptor whose attachments do not resolve, and a render
pass or framebuffer that could not be created.

Unit tests cover the attachment-selection helpers and both directions of
the End guard with no GPU. depth_only_pass_repro_test.go is the
end-to-end check behind the integration tag: it clears depth in a
color-less pass and reads the value back, so it distinguishes "did not
crash" from "actually ran". Reproduced and verified on Vulkan with an
NVIDIA GeForce RTX 4070 SUPER.
@codecov

codecov Bot commented Sep 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

dvoyni added a commit to dvoyni/cog-examples that referenced this pull request Sep 7, 2026
A replace only applies from the main module, so this one needs its own
alongside cog's. Same reason: gogpu/wgpu#353 carries the depth-only render
pass fix behind #115, and the tree is pinned to it until that merges.

Drops the v0.31.6 pin and follows cog to gogpu v0.54.0, gputypes v0.8.0,
naga v0.19.0 and gpucontext v0.31.3. The seven scene demos and their
acceptance suite pass unchanged against it.
dvoyni added a commit to dvoyni/cog that referenced this pull request Sep 7, 2026
… its v0.34 api

gogpu/wgpu#353 fixes the hal gap behind #115: a depth-only render pass was
never begun and faulted on End. Point the module at the local tree so the
fix is live here, and drop the v0.31.6 pin it was held at.

The bump is not free. gogpu/gogpu v0.53.0 does not compile against wgpu
v0.34, so it moves to v0.54.0, which carries gputypes to v0.8.0, naga to
v0.19.0 and gpucontext to v0.31.3. Three call sites follow that api:
Draw and DrawIndexed take gputypes.DrawArgs and DrawIndexedArgs instead of
positional uint32s, and VertexBufferLayout moved from wgpu to gputypes.

The replace is machine-specific and must not outlive the PR. When #353
merges and a release carries it, pin that version and drop the replace
here and in cog-examples and feuds-26.

depthOnlyPassSupported stays false on the native path: flipping it while
the behaviour comes from a local replace would be true only on this
machine.

@lkmavi lkmavi 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.

Verdict

Approve. The fix matches the crash: depth-only passes never called vkCmdBeginRenderPass, then End still called vkCmdEndRenderPass (UB). Caching already supported ColorCount == 0; only the encoder path was wrong. begun correctly covers the other early returns too.

What looks solid

  • renderAreaView / attachmentSampleCount: color first, depth fallback, sparse slots, typed-nil / foreign views.
  • End gated on begun && active; pool resets begun.
  • Unit tests cover both directions of the End guard + pool cleanup.
  • Integration test clears depth to 0.25 and readbacks — separates “no crash” from “pass ran”; Vulkan-only skip is right.
  • Relevant CI (build/test/lint) green. The earlier Dependencies failure was an unrelated proxy.golang.org flake on go-mod-outdated install (retried green; hardened in #354).

Non-blocking notes

  1. Depth fallback vs depth key pathrenderAreaView accepts any *TextureView; the depth block requires texture != nil. A HAL-only malformed depth view (nil texture) could begin an empty RP while still appending a depth clear. Public API views always have a texture; harden with view != nil && view.texture != nil on the depth fallback if you want belt-and-suspenders.
  2. ResidualDraw / Set* still don’t check begun (pre-existing for other failed begins). Crash was on End; optional follow-up.
  3. GLES/software ColorAttachments == 0 left alone — agreed, different problem.

No code blockers.

@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 first contribution, @dvoyni. This is a real bug — depth-only render passes (shadow maps) crash the process with an access violation inside the Vulkan driver. The root cause analysis, fix, and tests are all solid.

Verified against Rust wgpu-hal: the PR's claim is correct — Rust HAL carries extent and sample_count as caller-supplied fields on RenderPassDescriptor (wgpu-hal/src/lib.rs:2934). The core layer derives them from attachments (wgpu-core/src/command/render.rs:1256-1309, add_view() closure). Our HAL carries neither, so the attachment-to-extent derivation must happen inside BeginRenderPass — exactly what this PR does. Rust has no begun flag equivalent because Rust's ownership semantics prevent end-without-begin at compile time. In Go, the runtime boolean is the correct approach.

No blocking issues. LGTM.

Verified Claims

  1. RenderPassCache.createRenderPass handles ColorCount == 0 — confirmed. renderpass.go:166: for i := 0; i < key.ColorCount; i++ loops zero times, depth-only subpass is built correctly.
  2. Rust HAL descriptor carries extent/sample_count — confirmed. wgpu-hal/src/lib.rs:2934. Core derives from attachments in render.rs:1256-1309.
  3. Rust has no begun flag — confirmed. Ownership semantics make it compile-time impossible. Our Go runtime check is the correct translation.
  4. End() was unsafe on ALL early returns — confirmed. Not just depth-only: descriptor that doesn't resolve, failed render pass creation, failed framebuffer creation all left begun=false but End() called vkCmdEndRenderPass unconditionally.

Code Quality Assessment

  • renderAreaView — clean fallback chain: color → depth → nil. Handles sparse color arrays, typed-nil, foreign views. Well-documented.
  • asTextureView — comma-ok assertion correctly handles typed-nil and foreign HAL views.
  • attachmentSampleCount — swapchain view (no texture) defaults to 1. Correct.
  • begun flag — set only after vkCmdBeginRenderPass succeeds, checked in End(), cleared on pool return. No race (single-threaded command recording).
  • Tests — mutation-checked in both directions. TestEndEndsAPassThatWasBegun is clever: nil dispatch table → panic proves End() tried to reach the driver.

Non-blocking Suggestions

1. Integration test file location

depth_only_pass_repro_test.go is in the root wgpu package (matches arraylength_repro_test.go). Consistent with existing convention — good.

2. CHANGELOG entry

PR deliberately omits it. We'll add it at release — contributor doesn't need to guess our formatting.

3. Minor: rpe.begun = false on pool Get is redundant

rpe.begun = false  // line 838 in the PR

End() already sets rpe.begun = false before renderPassPool.Put(e). So Get always returns begun=false. The explicit init is defense-in-depth — harmless, slightly better for readability. Keep it.

4. Consider ci: approve fork workflow for CI

First-time fork PR needs manual workflow approval:

gh run list --status action_required --limit 5
gh api repos/gogpu/wgpu/actions/runs/{RUN_ID}/approve --method POST

Clean fix for a real crash. Tested on RTX 4070 SUPER, mutation-checked, Rust-reference verified. Good to merge.

@lkmavi
lkmavi merged commit 0e62880 into gogpu:main Sep 7, 2026
28 of 29 checks passed
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.

3 participants