perf: improve backdrop blur quality - #8
Conversation
WalkthroughBackdrop blur now uses a shared ChangesBackdrop blur pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Window
participant BackdropBlurPlan
participant Renderer
participant BlurShaders
participant TargetTexture
Window->>Renderer: submit positive finite blur radius
Renderer->>BackdropBlurPlan: derive passes, padding, sample_distance
Renderer->>TargetTexture: snapshot backdrop source
Renderer->>BlurShaders: execute downsample and upsample passes
BlurShaders-->>Renderer: return blurred view
Renderer->>TargetTexture: composite blurred output
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/gpui_macos/src/metal_renderer.rs (1)
637-666: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftBackdrop texture reuse rebuilds the whole pyramid whenever a cluster's
max_sizeshrinks. Both backends copied the samecan_reuse_backdrop_texturepredicate, which rejects an existing texture that is larger than the current cluster'smax_size; the fallback then reallocates the backdrop texture and every downsample/upsample level clamped down to that bound. Becausemax_backdrop_texture_sizeisviewport - cluster_origin, clusters at different origins in a single frame alternately shrink and regrow the pyramid every frame. The upper bound itself is needed (it keeps the region copy inside the surface), so the fix is to stop letting it force a downsize — reuse onrequired_sizealone and clamp the scratch bounds tomax_size, or only shrink when the excess is large.
crates/gpui_macos/src/metal_renderer.rs#L637-L666: drop thecurrent <= maxcondition fromcan_reuse_backdrop_textureand rely onfit_backdrop_scratch_boundsto keep the sampled region inside the viewport.crates/gpui_wgpu/src/wgpu_renderer.rs#L1404-L1425: apply the same change, and clampscratch_bounds.texture_sizetomax_sizebeforecopy_texture_to_textureso the region copy stays in bounds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gpui_macos/src/metal_renderer.rs` around lines 637 - 666, Stop backdrop texture pyramids from resizing when only max_size shrinks: in crates/gpui_macos/src/metal_renderer.rs lines 637-666, update can_reuse_backdrop_texture to require only current_size >= required_size and rely on fit_backdrop_scratch_bounds for viewport safety; in crates/gpui_wgpu/src/wgpu_renderer.rs lines 1404-1425, apply the same reuse predicate and clamp scratch_bounds.texture_size to max_size before copy_texture_to_texture.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gpui_macos/src/metal_renderer.rs`:
- Around line 1451-1465: The backdrop blur pass derives level_sizes from the
fitted scratch bounds, which can differ from the allocated pyramid dimensions.
Update the Metal backdrop blur flow around backdrop_blur_downsample_textures,
upsample_textures, and render_backdrop_blur_texture_for_plan to use
self.backdrop_blur_level_sizes for pass texture dimensions, or otherwise pass
allocated dimensions separately from logical extents; preserve scratch_bounds
for planning while ensuring sampling uses the actual allocated level sizes.
In `@crates/gpui_wgpu/src/wgpu_renderer.rs`:
- Around line 2896-2954: Move the shared pure backdrop-blur
helpers—backdrop_blur_padding, backdrop_blur_level_sizes_for,
backdrop_blur_plan_for_radius, backdrop_blur_plan_groups,
backdrop_source_bounds, backdrop_blur_clusters, and
can_reuse_backdrop_texture—into the common scene module alongside
BackdropBlurPlan, then update the WGPU, Metal, and DirectX backends to use those
shared symbols and remove their local copies. Consolidate the duplicated unit
tests with the shared implementation, leaving only backend-specific
resource-binding logic in each renderer.
- Around line 2493-2520: The first backdrop blur pass in the loop around
draw_backdrop_blur_pass double-decodes sRGB input when backdrop_view uses an
sRGB surface format. Ensure the level == 0 input uses a linear-format view of
backdrop_view, or otherwise disable the shader’s manual sRGB decode only for
that input, while preserving existing decoding for subsequent blur passes.
In `@crates/gpui_windows/src/directx_renderer.rs`:
- Around line 605-621: Skip zero-pass plans before invoking draw_backdrop_blurs.
In the blur grouping logic around backdrop_blur_plan_for_radius and
current_plan, detect plan.passes == 0 and bypass drawing for that group while
preserving normal processing for plans with passes greater than zero.
- Around line 3441-3466: Update DirectXRenderer::backdrop_blur_clusters and its
regression coverage to handle interleaved transitive overlaps without reordering
source epochs: add an A, B, C case where C overlaps A, B is disjoint, and assert
the resulting draw order remains contiguous and source-preserving. Ensure
merging does not remove A and append A/C after B; retain the original sequence
position of merged clusters rather than relying only on sorting by first index.
---
Outside diff comments:
In `@crates/gpui_macos/src/metal_renderer.rs`:
- Around line 637-666: Stop backdrop texture pyramids from resizing when only
max_size shrinks: in crates/gpui_macos/src/metal_renderer.rs lines 637-666,
update can_reuse_backdrop_texture to require only current_size >= required_size
and rely on fit_backdrop_scratch_bounds for viewport safety; in
crates/gpui_wgpu/src/wgpu_renderer.rs lines 1404-1425, apply the same reuse
predicate and clamp scratch_bounds.texture_size to max_size before
copy_texture_to_texture.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 70758928-865e-4184-ba68-48178023bd09
⛔ Files ignored due to path filters (1)
crates/gpui_windows/src/shaders.hlslis excluded by!**/*.hlsl
📒 Files selected for processing (8)
crates/gpui/src/scene.rscrates/gpui/src/styled.rscrates/gpui/src/window.rscrates/gpui_macos/src/metal_renderer.rscrates/gpui_macos/src/shaders.metalcrates/gpui_wgpu/src/shaders.wgslcrates/gpui_wgpu/src/wgpu_renderer.rscrates/gpui_windows/src/directx_renderer.rs
Centralize pure blur planning, preserve source epochs, prevent duplicate sRGB decoding, and retain safe texture allocations across backend clusters.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/gpui_wgpu/src/wgpu_renderer.rs (1)
2776-2874: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftDuplicated scratch-bounds/fit/prepare helpers.
Same byte-identical duplication as
crates/gpui_macos/src/metal_renderer.rs(lines 1700-1795) andcrates/gpui_windows/src/directx_renderer.rs(lines 1026-1116); see consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gpui_wgpu/src/wgpu_renderer.rs` around lines 2776 - 2874, Remove the duplicated backdrop scratch-bounds helpers from the renderer and reuse the consolidated shared implementation used by the macOS and Windows renderers. Update references to backdrop_scratch_bounds, max_backdrop_texture_size, fit_backdrop_scratch_bounds, and prepare_backdrop_blurs to resolve through that shared implementation while preserving their current behavior.crates/gpui_macos/src/metal_renderer.rs (1)
1700-1795: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
backdrop_scratch_bounds,max_backdrop_texture_size,fit_backdrop_scratch_bounds, andprepare_backdrop_blursremain duplicated across all three backends.These four functions are byte-identical in
crates/gpui_wgpu/src/wgpu_renderer.rs(lines 2776-2874) andcrates/gpui_windows/src/directx_renderer.rs(lines 1026-1116). The prior review already movedbackdrop_blur_padding,backdrop_blur_level_sizes_for,backdrop_blur_plan_groups,backdrop_source_bounds,backdrop_blur_clusters, andcan_reuse_backdrop_textureinto sharedgpuihelpers for exactly this reason; these remaining four pure functions overBackdropScratchBounds/Size<DevicePixels>should follow the same path to prevent future divergence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gpui_macos/src/metal_renderer.rs` around lines 1700 - 1795, The four backdrop scratch helpers remain duplicated across the Metal, WGPU, and DirectX backends. Move backdrop_scratch_bounds, max_backdrop_texture_size, fit_backdrop_scratch_bounds, and prepare_backdrop_blurs into the shared gpui helpers alongside the previously centralized backdrop functions, then remove the backend-local copies and update each backend to use the shared implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gpui/src/scene.rs`:
- Around line 914-960: Update backdrop_source_bounds to return None before
calculating bounds when blur.blur_radius.0 is zero, negative, or NaN, so no-op
radii are excluded from backdrop_blur_clusters. Preserve existing bounds
clipping for valid radii, and add coverage for zero, negative, and NaN inputs.
---
Outside diff comments:
In `@crates/gpui_macos/src/metal_renderer.rs`:
- Around line 1700-1795: The four backdrop scratch helpers remain duplicated
across the Metal, WGPU, and DirectX backends. Move backdrop_scratch_bounds,
max_backdrop_texture_size, fit_backdrop_scratch_bounds, and
prepare_backdrop_blurs into the shared gpui helpers alongside the previously
centralized backdrop functions, then remove the backend-local copies and update
each backend to use the shared implementations.
In `@crates/gpui_wgpu/src/wgpu_renderer.rs`:
- Around line 2776-2874: Remove the duplicated backdrop scratch-bounds helpers
from the renderer and reuse the consolidated shared implementation used by the
macOS and Windows renderers. Update references to backdrop_scratch_bounds,
max_backdrop_texture_size, fit_backdrop_scratch_bounds, and
prepare_backdrop_blurs to resolve through that shared implementation while
preserving their current behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 37c534e0-7b91-4374-8bbb-1a41edf9544d
📒 Files selected for processing (4)
crates/gpui/src/scene.rscrates/gpui_macos/src/metal_renderer.rscrates/gpui_wgpu/src/wgpu_renderer.rscrates/gpui_windows/src/directx_renderer.rs
Keep invalid radii out of source clustering and centralize scratch-bound preparation so every renderer preserves the same viewport and reuse rules.
|
Addressed the two outside-diff scratch-helper findings in |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gpui/src/scene.rs`:
- Around line 976-993: In backdrop_scratch_bounds, bind the result of
blurs.first()? once and reuse that BackdropBlur reference when initializing
bounds, instead of calling blurs.first()? separately for bounds and blur_radius.
Keep the subsequent union logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ce1767b8-0ac3-4129-a35d-e1ab982d0963
📒 Files selected for processing (4)
crates/gpui/src/scene.rscrates/gpui_macos/src/metal_renderer.rscrates/gpui_wgpu/src/wgpu_renderer.rscrates/gpui_windows/src/directx_renderer.rs
Bind the cluster head once so scratch-bound initialization cannot drift between duplicate lookups.
1 participant
Summary
Replace backend-specific backdrop blur approximations with one bounded, phase-stable blur pyramid across Metal, WGPU, and Direct3D.
Changes
Testing
cargo fmt --all -- --checkcargo metadata --locked --format-version 1cargo check --locked --workspace --all-targetscargo test --locked -p gpui --features test-support— 277 tests passedcargo test -p gpui_macos --lib— 10 tests passedcargo test -p gpui_wgpu --lib— 38 tests passed, including WGSL validationcargo test -p gpui_windows— host-side crate/doc gate passed; native D3D execution unavailable on macOScargo build -p gpui --example backdrop_blurcrates/util/src/util.rs:188(manual_clear)Review notes
Claude Fable and Oracle browser reviews challenged the original kernel/variance model; the final exact-moment 4+8 design incorporates that feedback. Independent Codex autoreview reports no actionable findings. RGBA16Float and a direct sub-minimum-radius path are intentionally deferred: either doubles pyramid memory or expands scope without evidence of a menu/dropdown blocker.
Texture reuse stays bounded by copy-safe cluster extents. Rare alternating edge-cluster sizes may reallocate rather than process an oversized retained texture; pooling is separate follow-up territory.
Note
Improve backdrop blur quality with multi-pass downsample/upsample pyramid
sample_distancerather than a fixed offset.BackdropBlurPlanin scene.rs to compute pass count and sample distance for a given radius using binary search over variance; helper functions for clustering, scratch bounds, texture reuse, and plan grouping are extracted togpuiand shared across all three renderer backends (Metal, WGPU, DirectX).backdrop_blur; the number and size of intermediate GPU textures allocated per frame increases proportionally with blur radius and cluster count.Macroscope summarized 3d37709.