Conversation
Ports the three GLES2 blur shaders (dual-Kawase down/up plus the brightness/contrast/saturation/noise effects pass) to Vulkan GLSL, and adds the pipeline plumbing they need: - blur1/blur2/blur_effects fragment shaders, compiled to SPIR-V via the existing glslang custom_target - a shared combined-image-sampler descriptor set layout, structurally identical to the one wlroots uses for texture descriptor sets so that sets from wlr_vk_render_pass_get_texture_ds() are layout compatible - pipeline creation/destruction wired into vk_pipelines and the renderer Blending is disabled for these pipelines since the blur passes fully replace the contents of their offscreen target. This is the shader half only. The drawing half (add_blur, add_optimized_blur and the offscreen ping-pong) still needs render targets: struct vk_buffer currently has no VkImage/VkImageView/VkFramebuffer, and wlroots-vkfx exports no way to run a render pass against one.
The previous commit created a descriptor set layout with no immutable sampler and claimed it was layout-compatible with wlroots' texture descriptor sets. That was wrong: wlroots declares .pImmutableSamplers on its binding, and Vulkan only treats two descriptor set layouts as compatible when they are identically defined, immutable samplers included. Binding a wlroots texture descriptor set to these pipeline layouts would have been invalid usage. Own the whole sampling path instead, which is needed regardless since the blur passes sample scenefx's offscreen images rather than wlr_textures: - a LINEAR / CLAMP_TO_EDGE sampler, declared immutable on the binding. Dual-Kawase depends on bilinear taps, so the filter mode is not incidental. - a descriptor pool plus vk_tex_ds_create/destroy helpers - teardown ordered so the pipeline layouts referencing the set layout are destroyed before it
Fills in vk_render_pass_add_blur(), which was a stub. The chain mirrors
the GLES2 implementation: N downsample passes with blur1, N upsample
passes with blur2, then a composite through blur_effects.
Blur has to sample the frame drawn so far, which is not possible from
inside the scene render pass. It therefore relies on wlroots'
wlr_vk_render_pass_suspend()/resume() to end the scene pass, run the
chain, and resume. Because of that this only engages on the two-pass
(blending buffer) pathway, which is also the pathway colour-managed and
HDR output always use; elsewhere it is skipped rather than corrupting
the frame.
New render targets live in render/vulkan/effects.c as plain VkImages
owned by the renderer rather than wlr_buffers, so the chain records into
the same command buffer as the scene and needs no DMA-BUF import or
cross-command-buffer synchronisation. Their framebuffers are built
against the scene render pass on purpose: Vulkan only treats render
passes as compatible when they are identical apart from a short
exemption list that does not include subpass dependencies, so a bespoke
pass would make the existing blur pipelines unusable with them.
Two subtleties worth recording:
* the composite must use blur_effects, not blur2 -- blur2 is the
upsample shader and begins with `suv = uv / 2.0`, so using it as a
plain blit magnifies the top-left quarter of the effect image over
the whole region, which shows as ghosted, offset copies of whatever
is behind the window;
* the chain runs over the whole buffer rather than the damaged region.
The effect images use loadOp DONT_CARE, so any pixel the chain skips
holds undefined memory while the composite samples the full window
area -- reading that as black produced black borders whenever damage
was small.
Verified with VK_LAYER_KHRONOS_validation: no validation errors.
save_blur_region()/apply_saved_blur_region() were stubs, so the blur had no way to keep its source clean under damage tracking: the blend image retains the previous frame's composited output wherever nothing was redrawn, and blurring that feeds back on itself. Implement both by copying the padding ring scenefx computes between the blend image and a renderer-owned snapshot image, suspending the scene pass around the transfer since vkCmdCopyImage cannot be recorded inside a render pass. With the source kept clean the chain follows damage again rather than covering the whole buffer. read_to_buffer() stays unimplemented on this path: it is oriented around wlr_buffers, while the snapshot is a plain VkImage owned by the renderer. The copies clamp to the image extents. scenefx bounds blur_padding_region by the output size, which is not always the render buffer size, and a copy running past the edge is invalid usage that loses the device -- caught by VUID-vkCmdCopyImage-srcOffset-00144. Verified with damage tracking enabled and VK_LAYER_KHRONOS_validation: no validation errors, and zero pixel drift between frames six seconds apart, confirming the feedback is gone.
There was a problem hiding this comment.
Pull request overview
Implements the missing Vulkan blur path by adding blur shaders, Vulkan-side offscreen effect images, and render-pass logic to run the Dual-Kawase down/up chain plus a final “effects” composite.
Changes:
- Add Vulkan blur fragment shaders (blur1/blur2/blur_effects) and wire them into the shader build/link process.
- Create and manage renderer-owned Vulkan images/framebuffers/descriptor sets for blur ping-pong targets and blend-image sampling.
- Implement blur rendering and blur-padding save/apply logic in the Vulkan render pass (using suspend/resume + copies + offscreen passes).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| render/vulkan/shaders/meson.build | Adds blur fragment shaders to the Vulkan shader build list. |
| render/vulkan/shaders/blur1.frag | Adds Dual-Kawase downsample shader (Vulkan GLSL). |
| render/vulkan/shaders/blur2.frag | Adds Dual-Kawase upsample shader (Vulkan GLSL). |
| render/vulkan/shaders/blur_effects.frag | Adds post-blur brightness/contrast/saturation/noise shader. |
| render/vulkan/shaders.c | Adds blur shader modules/layouts, immutable-sampler descriptor layout, and blur pipelines. |
| render/vulkan/renderer.c | Links/frees blur shaders and tears down blur resources on renderer destroy. |
| render/vulkan/pipeline.c | Initializes and destroys blur pipelines alongside existing pipelines. |
| render/vulkan/pass.c | Implements blur padding copy + full blur chain recording and compositing. |
| render/vulkan/meson.build | Adds new Vulkan effects implementation file to the build. |
| render/vulkan/effects.c | Implements renderer-owned blur effect images, cached blend descriptor set, and saved-pixels image allocation. |
| include/render/vulkan/vulkan.h | Extends vk_renderer with effect images, saved pixels, and related helper APIs. |
| include/render/vulkan/shaders.h | Adds blur push-constant structs and blur pipeline/shader helper declarations. |
| include/render/vulkan/pipeline.h | Extends vk_pipelines to include blur pipelines. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| .stageFlags = VK_SHADER_STAGE_VERTEX_BIT, | ||
| }, | ||
| { | ||
| .offset = pc_ranges[0].size, |
| .radius = radius, | ||
| }; | ||
|
|
||
| VkPipelineLayout layout = renderer->shader_info.blur1.pipeline_layout; |
| renderer->saved_pixels.initialised = true; | ||
| } | ||
|
|
||
| wlr_vk_render_pass_resume(fx_pass->render_pass); |
Comment on lines
+187
to
+190
| // Blur targets must go before the descriptor pool and layout in | ||
| // free_shaders(), since their descriptor sets are allocated from it. | ||
| vk_effect_images_finish(vk_renderer); | ||
|
|
Four issues raised in review of the blur implementation: - The fragment push constant range read pc_ranges[0].size while pc_ranges was still being initialised, which is unspecified and could have given a garbage offset. Use sizeof(struct vk_vert_pcr_data) directly. - render_blur_step() bound descriptor sets and push constants through blur1's pipeline layout no matter which blur pipeline was bound. That only worked because the blur layouts are identical. Record the layout on struct vk_pipeline_blur at creation and bind through the matching one. - The wlr_vk_render_pass_resume() result was ignored after the blur padding copy, so a failure would have left the following commands recorded outside the scene pass with no indication why. Log it. - vk_saved_pixels_ensure() allocates a renderer-owned VkImage and its memory, and nothing freed them: vk_effect_images_finish() does not cover the snapshot. Call vk_saved_pixels_finish() on renderer destroy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
|
Hey! Just wanted to let you know I'm aware that this exists, thank you for the work and sorry I haven't commented yet! We're trying to focus on the couple of prereq PRs to get in first, namely the renderer refactor and the base vulkan impl. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #7. Implements the
Blur → Regular bluritem from #192, on top of thatbranch.
Implements blur for the Vulkan renderer, filling in
vk_render_pass_add_blur(),save_blur_region()andapply_saved_blur_region(), which were stubs.This continues the existing
vulkan-blurwork (shaders, pipelines and thescenefx-owned sampling path were already in place) and makes blur render.
Important
This depends on wlroots changes that are not upstream. It calls
wlr_vk_render_pass_suspend(),_resume(),_get_blend_image_view()and_get_blend_image(), which do not exist in wlroots today. It will not buildwithout them. The matching branch is here:
https://github.com/spoloxs/wlroots-vkfx/tree/vulkan-effects
(based on
ammen99/wlrootsvulkan-effects, the fork this Vulkan work alreadytargets). The code here is complete and reviewable on its own, but it cannot
build or merge until those wlroots functions have a home — happy to split,
rebase, or hold this for as long as that takes.
Why wlroots had to change
Blur has to sample the frame drawn so far. That is impossible from inside the
scene render pass on the two-pass (blending buffer) pathway: geometry and the
blend → output colour transform were two subpasses of a single render pass, an
input attachment only permits same-pixel reads (blur needs neighbourhood taps),
and Vulkan forbids ending a render pass while the current subpass is not the
last.
The wlroots branch splits that into two render passes — scene → blend image, then
blend → output carrying the
output_pipe_*transforms — and exposessuspend/resume so a consumer can end the scene pass mid-frame, sample the blend
image as a regular texture, and resume with
LOAD_OP_LOAD.How the blur works
Mirrors the GLES2 implementation: N downsample passes (
blur1), N upsamplepasses (
blur2), then a composite throughblur_effects.Render targets live in a new
render/vulkan/effects.cas plainVkImages ownedby the renderer rather than
wlr_buffers, so the chain records into the samecommand buffer as the scene and needs no DMA-BUF import or cross-command-buffer
synchronisation.
Their framebuffers are deliberately built against the scene render pass:
Vulkan only treats render passes as compatible when they are identical apart
from a short exemption list that does not include subpass dependencies, so a
bespoke render pass would make the existing blur pipelines unusable with them.
Because blur only works on the two-pass pathway, it is skipped elsewhere rather
than corrupting the frame. That pathway is also the one colour-managed and HDR
output always take, so blur and HDR compose.
Notes that cost me time, recorded for the next person
The composite must use
blur_effects, notblur2.blur2is the upsampleshader and opens with
suv = uv / 2.0, so using it as a plain blit magnifies thetop-left quarter of the effect image over the whole region — it renders as
ghosted, offset copies of whatever is behind the window.
The padding repair is what makes damage tracking safe. Without
save_blur_region()/apply_saved_blur_region()the blend image still holds theprevious frame's composited output wherever nothing was redrawn, so the blur
re-blurs its own result and stair-step banding accumulates. Scoping the chain to
the damage instead just moves the problem: the effect images then have regions
the chain never wrote, which read back as black borders whenever damage is small.
Copies must be clamped to the image.
blur_padding_regionis bounded by theoutput size, which is not always the render buffer size; a copy running past the
edge is invalid usage and loses the device (
VUID-vkCmdCopyImage-srcOffset-00144).read_to_buffer()is intentionally left unimplemented here — it is orientedaround
wlr_buffers, while the snapshot is a renderer-ownedVkImage.Testing
NVIDIA RTX 5050 (proprietary 610.43.02), Arch, mango 0.15.6, nested Wayland
backend, with
VK_LAYER_KHRONOS_validationenabled:WLR_SCENE_DEBUG_DAMAGE=rerender)otherwise static scene, confirming no blur feedback accumulation
window: smooth blur, correct registration, no black edges or banding
Not yet tested: multi-output, output hotplug/resize under load, non-NVIDIA
drivers, and the
blur_optimizedpath (add_optimized_blur()is still a stubreturning false).