feat: Add selective blur filter on screen #916
Conversation
WalkthroughAdds selective blur support end-to-end: new data types (Rect, BlurSegment), editor UI (timeline track, overlay, sidebar), project API updates, and a GPU-based selective blur rendering pass with WGSL shader and pipeline. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Timeline as BlurTrack
participant Context as EditorContext
participant Sidebar as ConfigSidebar
participant Overlay as BlurOverlay
User->>Timeline: click/add/drag/resize segment
Timeline->>Context: update project.timeline.blurSegments
Timeline->>Context: set selection {type:"blur", index}
User->>Sidebar: adjust blur amount / rect sliders
Sidebar->>Context: setProject(...blurSegments[index]...)
Context-->>Overlay: reactive project/editorState change
Overlay->>Overlay: compute active segments for current time
Overlay-->>User: render/update blur rectangles on preview
sequenceDiagram
participant App
participant Renderer as RendererLayers
participant Layer as SelectiveBlurLayer
participant GPU as WGPU
App->>Renderer: render(device, encoder, session, uniforms, current_time)
Renderer->>Renderer: draw main pass to current texture
Renderer->>Renderer: active_blur_segments = check(uniforms.timeline, current_time)
alt active_blur_segments exist
Renderer->>Layer: render(pass, device, current_texture_view, uniforms, current_time)
Layer->>GPU: bind texture, sampler, blur segments buffer
Layer->>GPU: set pipeline and draw fullscreen triangle
Renderer->>Renderer: swap textures
else no segments
Renderer-->>App: skip blur pass
end
Renderer-->>App: present
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/routes/editor/Timeline/index.tsx (1)
79-91: Don’t move playhead while a blur segment is being draggedCurrently only zoom drags are honored. Include
blurSegmentDragStatein the guard.async function handleUpdatePlayhead(e: MouseEvent) { const { left } = timelineBounds; - if (zoomSegmentDragState.type !== "moving") { + if ( + zoomSegmentDragState.type !== "moving" && + blurSegmentDragState.type !== "moving" + ) { setEditorState( "playbackTime", Math.min( secsPerPixel() * (e.clientX - left!) + transform().position, totalDuration(), ), ); } }
🧹 Nitpick comments (16)
crates/rendering/src/shaders/blur.wgsl (1)
91-108: Clarify overlap semantics and add a fast path for zero segments.
- Overlapping rectangles: fs_main returns on the first match. If overlap order matters, ensure the CPU side writes blur_segments in the intended priority order (or document this).
- Minor perf: skip the loop when blur_segments_count == 0.
Apply this small guard if you want the fast path:
@fragment fn fs_main(frag_in: VertexOutput) -> @location(0) vec4<f32> { + if (uniforms.blur_segments_count == 0u) { + return textureSample(t_input, s_input, frag_in.uv); + } // Check if current pixel is inside any blur rectangle for (var i: u32 = 0u; i < uniforms.blur_segments_count; i = i + 1u) { let segment = blur_segments[i]; let rect = segment.rect;Additionally, please confirm that the rect packing matches the UI and Rust side, i.e., rect = vec4(x, y, width, height) in normalized [0..1] space, with y increasing downward. If it’s different, adjust the bounds check accordingly.
apps/desktop/src/routes/editor/context.ts (1)
144-156: LGTM: deleteBlurSegment mirrors existing segment deletion patterns.
- Uses batch to avoid unnecessary re-renders.
- Mutates with produce and clears selection.
Two optional tweaks you may consider:
- Guard when timeline is missing: if (!project.timeline) return;
- No need to return s.splice(...); mutating in place is enough.
Here’s a minor clean-up keeping behavior unchanged:
- deleteBlurSegment: (segmentIndex: number) => { - batch(() => { - setProject( - "timeline", - "blurSegments", - produce((s) => { - if (!s) return; - return s.splice(segmentIndex, 1); - }), - ); - setEditorState("timeline", "selection", null); - }); - }, + deleteBlurSegment: (segmentIndex: number) => { + if (!project.timeline) return; + batch(() => { + setProject( + "timeline", + "blurSegments", + produce((s) => { + if (!s) return; + s.splice(segmentIndex, 1); + }), + ); + setEditorState("timeline", "selection", null); + }); + },apps/desktop/src-tauri/src/recording.rs (1)
28-29: Nit: minor formatting in imports
BlurSegment,ZoomSegmentlacks a space after the comma; while valid, it’s inconsistent with the surrounding style.Apply this tiny formatting tweak:
- BlurSegment,ZoomSegment, cursor::CursorEvents, + BlurSegment, ZoomSegment, cursor::CursorEvents,apps/desktop/src/routes/editor/Timeline/index.tsx (1)
55-74: Ensure blurSegments is initialized even when timeline already existsThis block only initializes
blurSegmentswhenproject.timelineis nullish, but not when it exists without ablurSegmentskey (e.g., opening older projects). InitializeblurSegmentsseparately to guarantee it’s always present.Apply this minimal guard just after the existing
setProject(produce(...))(still inside this if-block):setProject( produce((project) => { project.timeline ??= { segments: [ { start: 0, end: duration(), timescale: 1, }, ], zoomSegments: [], blurSegments: [], }; }), ); + + if (!project.timeline?.blurSegments) { + setProject("timeline", "blurSegments", []); + }crates/project/src/configuration.rs (2)
462-469: Consider using f64 for start/end to match timeline/zoom types
BlurSegment.start/endaref32whileTimelineSegmentandZoomSegmentusef64. Mixed precision introduces avoidable casts and potential rounding. Unless constrained by GPU uniform layout, preferf64for consistency with the rest of the timeline model.-pub struct BlurSegment { - pub start:f32, - pub end: f32, +pub struct BlurSegment { + pub start: f64, + pub end: f64, pub blur_amount: Option<f32>, pub rect: Rect, }Note: If you adopt this, update the renderer to cast from
f64tof32where required.
480-485: Optional: avoid Option for blur_segmentsUsing
Option<Vec<BlurSegment>>requires extra null checks across UI and renderer. Consider a plainVec<BlurSegment>with#[serde(default)]to default to an empty list and simplify consumers.- pub struct TimelineConfiguration { + pub struct TimelineConfiguration { pub segments: Vec<TimelineSegment>, pub zoom_segments: Vec<ZoomSegment>, - pub blur_segments: Option<Vec<BlurSegment>>, + #[serde(default)] + pub blur_segments: Vec<BlurSegment>, }This change will also simplify code such as
.and_then(|t| t.blur_segments.as_ref())in the renderer.crates/rendering/src/layers/selective_blur.rs (2)
41-56: Minor: redundant cast and extra allocation
segment.start as f32/end as f32are unnecessary ifBlurSegmentusesf32for time.- You create
active_segments: Vec<&BlurSegment>and then immediately re-iterate to buildgpu_blur_segments. You can fuse both into a single iterator over the original slice.- let active_segments: Vec<&cap_project::BlurSegment> = uniforms + let active_segments = uniforms .project .timeline .as_ref() .and_then(|t| t.blur_segments.as_ref()) - .map(|segments| { - segments - .iter() - .filter(|segment| { - current_time >= segment.start as f32 && - current_time <= segment.end as f32 - }) - .collect() - }) + .map(|segments| segments.iter().filter(|segment| { + current_time >= segment.start && current_time <= segment.end + })) .unwrap_or_default();And then:
- let gpu_blur_segments: Vec<GpuBlurSegment> = active_segments - .iter() + let gpu_blur_segments: Vec<GpuBlurSegment> = active_segments
97-100: Nit: unused imports
bytemuck::cast_sliceandwgpu::util::DeviceExtaren’t used in this file. Removing them cuts warnings.-use bytemuck::cast_slice; -use wgpu::{util::DeviceExt, RenderPass}; +use wgpu::RenderPass;apps/desktop/src/routes/editor/Timeline/BlurTrack.tsx (1)
135-139: Don’t treat 0 blur as “missing”; check nullish instead of falsy.0 is a valid intensity but renders “…”. Use nullish checks.
- const blurPercentage = () => { - const amount = segment.blur_amount; - // Handle potential null or undefined amount - return amount ? `${amount.toFixed(1)}x` : '...'; - }; + const blurPercentage = () => { + const amount = segment.blur_amount; + return amount != null ? `${amount.toFixed(1)}x` : "..."; + };apps/desktop/src/routes/editor/BlurOverlay.tsx (2)
61-70: Confirm blur intensity scale parity between CSS and shader.Backdrop-filter blur uses pixels; shader blur_amount likely has different units. Multiplying by 20 may drift from the GPU result, confusing users.
If shader expects a different scale, either:
- Normalize both sides to a single “intensity” (0..1) and map consistently; or
- Convert shader amount to px (based on output size) to match the overlay.
Would you like me to wire a single helper (shared constant or mapping function) to ensure parity?
79-137: Tighten types and post-min-size clamping for resize.
- corner should be a discriminated union for clarity.
- After enforcing min size, ensure the rect still fits in [0,1] by adjusting x/y if needed.
-function BlurRectangle(props: BlurRectangleProps) { - const handleMouseDown = (e: MouseEvent, action: 'move' | 'resize', corner?: string) => { +type Corner = 'nw' | 'ne' | 'sw' | 'se'; +function BlurRectangle(props: BlurRectangleProps) { + const handleMouseDown = (e: MouseEvent, action: 'move' | 'resize', corner?: Corner) => { @@ - // Ensure minimum size after any operation - if (newRect.width < 0.05) newRect.width = 0.05; - if (newRect.height < 0.05) newRect.height = 0.05; + // Ensure minimum size after any operation + if (newRect.width < 0.05) newRect.width = 0.05; + if (newRect.height < 0.05) newRect.height = 0.05; + // Keep within bounds after min-size adjustments + if (newRect.x + newRect.width > 1) newRect.x = 1 - newRect.width; + if (newRect.y + newRect.height > 1) newRect.y = 1 - newRect.height;crates/rendering/src/selective_blur_pipeline.rs (2)
2-5: Remove unused import.std::default isn’t used and will trigger a warning (or fail under deny-warnings).
-use std::{borrow::Cow, default}; +use std::borrow::Cow;
34-74: Bind group layout matches WGSL IO; consider min_binding_size for validation.The layout is correct. Optionally specify min_binding_size to get wgpu’s extra validation/debugging support.
If desired:
- For uniforms: min_binding_size: Some(std::num::NonZeroU64::new(std::mem::size_of::() as u64).unwrap())
- For storage: min_binding_size: Some(…size_of::() as u64)
crates/rendering/src/lib.rs (3)
7-7: Drop unused importtimefrom core.
core::{f64, time}importstimewhich is not used (you already use std::time::Instant). This may warn/fail depending on settings.-use core::{f64, time}; +use core::f64;
904-986: Two-pass render: choose Clear on the blur pass target for determinism.Writing to the OTHER texture then swapping is correct. Using LoadOp::Load assumes the pass fully overwrites; safer to Clear for future changes.
- let mut pass = render_pass!(session.other_texture_view(), wgpu::LoadOp::Load); + let mut pass = render_pass!(session.other_texture_view(), wgpu::LoadOp::Clear(wgpu::Color::TRANSPARENT));
989-1002: Avoid allocation in active_blur_segments; return bool via any().You only need emptiness. This currently allocates a Vec. Minor, but cheap to improve.
Proposed change:
- Replace active_blur_segments(...) -> has_active_blur_segments(...) -> bool using iter().any()
- Update the call site accordingly.
Example implementation:
-fn active_blur_segments(uniforms: &ProjectUniforms, current_time: f32) -> Vec<&cap_project::BlurSegment> { - uniforms - .project - .timeline - .as_ref() - .and_then(|t| t.blur_segments.as_ref()) - .map(|segments| { - segments - .iter() - .filter(|segment| current_time >= segment.start as f32 && current_time <= segment.end as f32) - .collect() - }) - .unwrap_or_default() -} +fn has_active_blur_segments(uniforms: &ProjectUniforms, current_time: f32) -> bool { + uniforms + .project + .timeline + .as_ref() + .and_then(|t| t.blur_segments.as_ref()) + .map(|segments| { + segments.iter().any(|segment| current_time >= segment.start as f32 && current_time <= segment.end as f32) + }) + .unwrap_or(false) +}And:
- if !active_blur_segments(uniforms, current_time).is_empty() { + if has_active_blur_segments(uniforms, current_time) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
apps/desktop/src-tauri/src/recording.rs(2 hunks)apps/desktop/src/routes/editor/BlurOverlay.tsx(1 hunks)apps/desktop/src/routes/editor/ConfigSidebar.tsx(3 hunks)apps/desktop/src/routes/editor/Player.tsx(3 hunks)apps/desktop/src/routes/editor/Timeline/BlurTrack.tsx(1 hunks)apps/desktop/src/routes/editor/Timeline/index.tsx(4 hunks)apps/desktop/src/routes/editor/context.ts(2 hunks)apps/desktop/src/utils/tauri.ts(2 hunks)crates/project/src/configuration.rs(2 hunks)crates/rendering/src/frame_pipeline.rs(1 hunks)crates/rendering/src/layers/mod.rs(1 hunks)crates/rendering/src/layers/selective_blur.rs(1 hunks)crates/rendering/src/lib.rs(7 hunks)crates/rendering/src/selective_blur_pipeline.rs(1 hunks)crates/rendering/src/shaders/blur.wgsl(1 hunks)packages/ui-solid/src/auto-imports.d.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (10)
apps/desktop/src/routes/editor/Player.tsx (1)
apps/desktop/src/routes/editor/BlurOverlay.tsx (1)
BlurOverlay(17-75)
crates/rendering/src/layers/selective_blur.rs (3)
apps/desktop/src/utils/tauri.ts (1)
BlurSegment(330-330)crates/rendering/src/lib.rs (8)
new(63-133)new(312-349)new(531-764)new(780-785)new(832-842)new(1011-1041)render(787-817)render(904-986)crates/rendering/src/selective_blur_pipeline.rs (1)
new(28-113)
apps/desktop/src/routes/editor/Timeline/BlurTrack.tsx (1)
apps/desktop/src/routes/editor/Timeline/Track.tsx (4)
TrackRoot(12-26)SegmentRoot(49-87)SegmentHandle(103-125)SegmentContent(89-101)
crates/project/src/configuration.rs (1)
apps/desktop/src/utils/tauri.ts (2)
Rect(410-410)BlurSegment(330-330)
apps/desktop/src-tauri/src/recording.rs (2)
apps/desktop/src/utils/tauri.ts (1)
BlurSegment(330-330)crates/rendering/src/lib.rs (6)
new(63-133)new(312-349)new(531-764)new(780-785)new(832-842)new(1011-1041)
crates/rendering/src/selective_blur_pipeline.rs (3)
apps/desktop/src/utils/tauri.ts (1)
BlurSegment(330-330)crates/rendering/src/layers/selective_blur.rs (1)
new(14-31)crates/rendering/src/lib.rs (6)
new(63-133)new(312-349)new(531-764)new(780-785)new(832-842)new(1011-1041)
apps/desktop/src/routes/editor/Timeline/index.tsx (2)
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (1)
ZoomSegmentDragState(25-28)apps/desktop/src/routes/editor/Timeline/BlurTrack.tsx (2)
BlurSegmentDragState(23-26)BlurTrack(28-410)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (2)
apps/desktop/src/utils/tauri.ts (1)
BlurSegment(330-330)apps/desktop/src/routes/editor/ui.tsx (3)
EditorButton(352-412)Field(25-47)Slider(65-147)
crates/rendering/src/lib.rs (2)
crates/rendering/src/frame_pipeline.rs (1)
finish_encoder(66-141)apps/desktop/src/utils/tauri.ts (1)
BlurSegment(330-330)
apps/desktop/src/utils/tauri.ts (3)
apps/desktop/src/utils/box.ts (1)
Bounds(6-6)apps/desktop/src-tauri/src/captions.rs (1)
captions(744-768)apps/desktop/src/store/captions.ts (1)
CaptionSettings(12-25)
🔇 Additional comments (21)
packages/ui-solid/src/auto-imports.d.ts (3)
12-12: New global IconCapBlur declaration looks correct and consistent.Matches existing naming and typing patterns for icons, and aligns with the PR’s blur UI additions.
7-7: export {} without semicolon is fine.This still marks the file as a module, enabling the declare global block. No issues.
9-93: Verify icon plugin wiring and TS types inclusionIt looks like the new
IconCapBlurdeclaration is in place and being used in your components, but we didn’t find evidence that the icons auto-import plugin or types file is actually wired up in your build or compiler configs. Please double-check:
- Vite plugin configuration
• apps/storybook/vite.config.ts currently shows no references tounplugin-iconsorunplugin-auto-import. Ensure your main Vite configs (for desktop, web, storybook, etc.) include:import Icons from 'unplugin-icons/vite' import AutoImport from 'unplugin-auto-import/vite' // ... plugins: [ AutoImport({ /* ... */ }), Icons({ /* custom collections: { cap: './src/icons/cap' } */ }), // … ]- Icon collection availability
• Confirm that yourcap/blur.jsxicon is present under the configured custom collection (e.g.src/icons/cap/blur.jsx) or in your Iconify/custom setup.- TS declarations pickup
•packages/ui-solid/tsconfig.jsononly definescompilerOptions—noincludeortypeRoots. Make sure it (or your root tsconfig.json) includessrc/auto-imports.d.ts, for example:{ "include": ["src", "src/auto-imports.d.ts"], // … }Once these are wired up, you should see the auto-imported icons resolve correctly and get full typings.
crates/rendering/src/shaders/blur.wgsl (1)
33-54: LGTM: full-screen triangle and UV mapping match top-left origin.The full-screen triangle with UVs mapping top-left (0,0) to bottom-right (1,1) is correct and consistent with most 2D UI coordinate systems. This should align with the frontend overlay as long as normalized rects are based on the displayed frame region.
Double-check on a letterboxed layout that the frontend overlay computes rects relative to the video content area (not the container), to avoid any misalignment between UI and shader sampling.
crates/rendering/src/layers/mod.rs (1)
7-7: LGTM: module exposure is correct.Adding the module and re-exporting its contents is consistent with the existing pattern in this file and unblocks downstream usage.
Also applies to: 16-16
crates/rendering/src/frame_pipeline.rs (1)
6-6: No functional change.Only whitespace has changed; nothing to review here.
apps/desktop/src/routes/editor/Player.tsx (1)
25-25: Import looks good.Importing BlurOverlay here is appropriate; after the wrapper refactor above, no additional props are needed.
apps/desktop/src/routes/editor/context.ts (1)
252-256: LGTM: selection type extended to include blur.This keeps the state machine consistent with existing selection types.
apps/desktop/src-tauri/src/recording.rs (1)
970-991: LGTM: initialize blur_segments in generated project configSeeding
timeline.blur_segmentswithSome(Vec::new())ensures stable serialization and avoids null checks downstream. This matches the renderer’s expectation and aligns with the new UI.apps/desktop/src/routes/editor/Timeline/index.tsx (2)
236-241: LGTM: BlurTrack integration and drag state wiringThe BlurTrack is correctly integrated and its drag state is captured for higher-level timeline interactions.
590-609: LGTM: Sidebar renders blur segment config when selectedThe conditional rendering for blur selection mirrors zoom/clip behavior and passes the right props.
crates/project/src/configuration.rs (1)
452-460: Rect type LGTMPublic Rect with camelCase serde aligns with TS bindings and UI.
crates/rendering/src/layers/selective_blur.rs (1)
57-76: LGTM: normalized rect and intensity mappingMapping rect to normalized f32 and scaling 0–1 intensity by 8.0 matches the shader expectations described in the PR and avoids heavy blur when users choose subtle values.
apps/desktop/src/routes/editor/Timeline/BlurTrack.tsx (1)
389-407: Nice UX touch with hover preview; ensure it doesn’t block input.Pointer-events-none and group styling are correctly applied for a non-interactive preview. LGTM.
apps/desktop/src/routes/editor/BlurOverlay.tsx (2)
26-31: Inclusive end check matches renderer; good.Using currentTime >= start && <= end aligns with backend active_blur_segments() logic. Consistent behavior.
140-149: Overlay interactivity and visuals look solid.Pointer-events toggling based on isEditing and using backdrop-filter for live preview is clean and accessible.
crates/rendering/src/selective_blur_pipeline.rs (1)
82-108: Pipeline config is sane for a full-screen pass.No vertex buffers, alpha blending on SRGB target, and default primitive/multisample are appropriate here.
crates/rendering/src/lib.rs (2)
828-841: Selective blur layer wiring looks correct.Layer creation is consistent with other layers and
RendererLayersencapsulation.
1119-1120: Call site updated correctly with current_time; good.Passing segment_frames.recording_time keeps renderer and shader in sync.
apps/desktop/src/utils/tauri.ts (2)
328-331: Type surface for BlurSegment/Rect aligns with UI usage.TimelineConfiguration.blurSegments and BlurSegment shape match the new UI/data flow. Generated file—no manual edits recommended.
428-429: Confirm nullability contract for blurSegments.Front-end code assumes
timeline.blurSegmentsmay be null/undefined and normalizes before use. Keep backend emittingnull(not[]) when absent to match this contract.Would you like me to scan for any places still assuming
timeline.blurSegmentsis always an array?
| function BlurSegmentConfig(props: { | ||
| segmentIndex: number; | ||
| segment: BlurSegment; | ||
| }) { | ||
| const { | ||
| project, | ||
| setProject, | ||
| editorInstance, | ||
| setEditorState, | ||
| projectHistory, | ||
| projectActions, | ||
| } = useEditorContext(); | ||
|
|
||
| return ( | ||
| <> | ||
| <div class="flex flex-row justify-between items-center"> | ||
| <div class="flex gap-2 items-center"> | ||
| <EditorButton | ||
| onClick={() => setEditorState("timeline", "selection", null)} | ||
| leftIcon={<IconLucideCheck />} | ||
| > | ||
| Done | ||
| </EditorButton> | ||
| </div> | ||
| <EditorButton | ||
| variant="danger" | ||
| onClick={() => { | ||
| projectActions.deleteBlurSegment(props.segmentIndex); | ||
| }} | ||
| leftIcon={<IconCapTrash />} | ||
| > | ||
| Delete | ||
| </EditorButton> | ||
| </div> | ||
|
|
||
| <Field name="Blur Intensity" icon={<IconCapBlur />}> | ||
| <Slider | ||
|
|
||
| value={[props.segment.blur_amount ?? 0]} | ||
| onChange={(v) => | ||
| setProject( | ||
| "timeline", | ||
| "blurSegments", | ||
| props.segmentIndex, | ||
| "blur_amount", | ||
| v[0], | ||
| ) | ||
| } | ||
|
|
||
| minValue={0} | ||
| maxValue={1} | ||
| step={0.01} | ||
| formatTooltip={(value) => `${Math.round(value * 100)}%`} | ||
| /> | ||
| </Field> | ||
|
|
||
| <Field name="Blur Area" icon={<IconCapBgBlur />}> | ||
| <div class="space-y-4"> | ||
| <div class="flex gap-2"> | ||
| <div class="flex-1"> | ||
| <label class="text-xs text-gray-11">X Position</label> | ||
| <Slider | ||
| value={[props.segment.rect.x * 100]} | ||
| onChange={(v) => | ||
| setProject( | ||
| "timeline", | ||
| "blurSegments", | ||
| props.segmentIndex, | ||
| "rect", | ||
| "x", | ||
| v[0] / 100, | ||
| ) | ||
| } | ||
| minValue={0} | ||
| maxValue={100} | ||
| step={0.1} | ||
| formatTooltip="%" | ||
| /> | ||
| </div> | ||
| <div class="flex-1"> | ||
| <label class="text-xs text-gray-11">Y Position</label> | ||
| <Slider | ||
| value={[props.segment.rect.y * 100]} | ||
| onChange={(v) => | ||
| setProject( | ||
| "timeline", | ||
| "blurSegments", | ||
| props.segmentIndex, | ||
| "rect", | ||
| "y", | ||
| v[0] / 100, | ||
| ) | ||
| } | ||
| minValue={0} | ||
| maxValue={100} | ||
| step={0.1} | ||
| formatTooltip="%" | ||
| /> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div class="flex gap-2"> | ||
| <div class="flex-1"> | ||
| <label class="text-xs text-gray-11">Width</label> | ||
| <Slider | ||
| value={[props.segment.rect.width * 100]} | ||
| onChange={(v) => | ||
| setProject( | ||
| "timeline", | ||
| "blurSegments", | ||
| props.segmentIndex, | ||
| "rect", | ||
| "width", | ||
| v[0] / 100, | ||
| ) | ||
| } | ||
| minValue={1} | ||
| maxValue={100} | ||
| step={0.1} | ||
| formatTooltip="%" | ||
| /> | ||
| </div> | ||
| <div class="flex-1"> | ||
| <label class="text-xs text-gray-11">Height</label> | ||
| <Slider | ||
| value={[props.segment.rect.height * 100]} | ||
| onChange={(v) => | ||
| setProject( | ||
| "timeline", | ||
| "blurSegments", | ||
| props.segmentIndex, | ||
| "rect", | ||
| "height", | ||
| v[0] / 100, | ||
| ) | ||
| } | ||
| minValue={1} | ||
| maxValue={100} | ||
| step={0.1} | ||
| formatTooltip="%" | ||
| /> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </Field> | ||
| </> | ||
| ); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Unify blur_amount semantics across UI and renderer; verify icon import
- The slider here treats
blur_amountas a normalized 0–1 value (percent). That’s good and matches the renderer which scales by 8.0. However, BlurTrack initializes new segments withblur_amount: 8, which will be interpreted as 800% here and 64× in the shader after the 8.0 scale. This will cause extreme blur. Standardizeblur_amountto 0–1 everywhere and fix the BlurTrack default. - Also verify that
IconCapBluris imported; it’s not visible in this file’s imports and will cause a compile error if missing.
Proposed changes outside this file:
- In apps/desktop/src/routes/editor/Timeline/BlurTrack.tsx, change the default:
- blur_amount: 8,
+ blur_amount: 0.4,- In the same file, adjust the label from “x” to percent for consistency:
- return amount ? `${amount.toFixed(1)}x` : '...';
+ return amount != null ? `${Math.round(amount * 100)}%` : '...';Within this file (optional UI consistency):
- <Field name="Blur Intensity" icon={<IconCapBlur />}>
+ <Field name="Blur Intensity" icon={<IconCapBlur />}>If IconCapBlur isn’t imported, add it to the icon imports at the top of the file.
🏁 Script executed:
#!/bin/bash
set -e
# Locate BlurTrack.tsx
blur_file=$(fd BlurTrack.tsx -t f | head -n1)
echo "=== BlurTrack file: $blur_file ==="
# Show default blur_amount initialization
echo "=== Default blur_amount occurrences ==="
rg -n "blur_amount" -C3 "$blur_file"
# Show label formatting for blur amount in BlurTrack
echo "=== Label formatting for blur amount ==="
rg -n "toFixed" -C3 "$blur_file" || echo "No toFixed usage found"
# Locate ConfigSidebar.tsx
cs_file=$(fd ConfigSidebar.tsx -t f | head -n1)
echo "=== ConfigSidebar file: $cs_file ==="
# Check for IconCapBlur import
echo "=== IconCapBlur import lines ==="
rg -n "import.*IconCapBlur" -C2 "$cs_file" || echo "IconCapBlur not imported"Length of output: 2275
Standardize blur_amount to 0–1 range and add missing IconCapBlur import
-
apps/desktop/src/routes/editor/Timeline/BlurTrack.tsx
• At the segment initializer (line 114), change:- blur_amount: 8, + blur_amount: 0.4,
• In the
blurPercentagehelper (around line 136), update the label from “x” to percent:- return amount ? `${amount.toFixed(1)}x` : '...'; + return amount != null ? `${Math.round(amount * 100)}%` : '...';
-
apps/desktop/src/routes/editor/ConfigSidebar.tsx
• Add the missing import forIconCapBluralongside the other icons at the top of the file:import { IconLucideCheck, IconCapTrash, IconCapBlur, IconCapBgBlur } from '…';
These changes ensure that:
- All
blur_amountvalues are consistently treated as a 0–1 slider range (avoiding 800%/64× blur bugs). - The “Blur Intensity” field’s icon reference compiles without errors.
🤖 Prompt for AI Agents
In apps/desktop/src/routes/editor/ConfigSidebar.tsx around lines 2001 to 2148,
the Blur Intensity field uses IconCapBlur but that icon is not imported and the
component assumes blur_amount is a 0–1 value; add IconCapBlur to the icon import
list at the top of the file (alongside IconLucideCheck, IconCapTrash,
IconCapBgBlur) and ensure the Slider value and onChange continue to treat
props.segment.blur_amount as a 0–1 normalized value (no conversion to
percentages) so the component compiles and the blur_amount remains standardized
to the 0–1 range.
| width={currentFrame().width} | ||
| height={currentFrame().data.height} | ||
| /> | ||
| <BlurOverlay/> |
There was a problem hiding this comment.
Overlay misalignment on letterboxed/cropped previews. Wrap canvas+overlay in a sized relative container.
BlurOverlay uses container bounds to convert normalized rects to pixels. Placing it as a sibling of the canvas inside a larger, centered flex container will misalign the overlay whenever the canvas is not filling the container (common for aspect-fit letterboxing). Wrap both canvas and BlurOverlay in a relative wrapper sized to the rendered canvas dimensions so the overlay’s absolute rects align 1:1 with the video content.
Apply this refactor to the preview block:
return (
- <div class="flex overflow-hidden absolute inset-0 justify-center items-center h-full">
- <canvas
- style={{
- width: `${size().width - padding * 2}px`,
- height: `${size().height}px`,
- }}
- class="bg-blue-50 rounded"
- ref={canvasRef}
- id="canvas"
- width={currentFrame().width}
- height={currentFrame().data.height}
- />
- <BlurOverlay/>
- </div>
+ <div class="flex overflow-hidden absolute inset-0 justify-center items-center h-full">
+ <div
+ class="relative"
+ style={{
+ width: `${size().width - padding * 2}px`,
+ height: `${size().height}px`,
+ }}
+ >
+ <canvas
+ style={{
+ width: "100%",
+ height: "100%",
+ }}
+ class="bg-blue-50 rounded"
+ ref={canvasRef}
+ id="canvas"
+ width={currentFrame().width}
+ height={currentFrame().data.height}
+ />
+ <BlurOverlay />
+ </div>
+ </div>
);This ensures BlurOverlay’s root fills the exact area of the canvas (its immediate parent), keeping rects aligned with pixels across all aspect ratios and padding.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <BlurOverlay/> | |
| return ( | |
| <div class="flex overflow-hidden absolute inset-0 justify-center items-center h-full"> | |
| - <canvas | |
| - style={{ | |
| - width: `${size().width - padding * 2}px`, | |
| - height: `${size().height}px`, | |
| - }} | |
| - class="bg-blue-50 rounded" | |
| - ref={canvasRef} | |
| - id="canvas" | |
| - width={currentFrame().width} | |
| - height={currentFrame().data.height} | |
| - /> | |
| <div | |
| class="relative" | |
| style={{ | |
| width: `${size().width - padding * 2}px`, | |
| height: `${size().height}px`, | |
| }} | |
| > | |
| <canvas | |
| style={{ | |
| width: "100%", | |
| height: "100%", | |
| }} | |
| class="bg-blue-50 rounded" | |
| ref={canvasRef} | |
| id="canvas" | |
| width={currentFrame().width} | |
| height={currentFrame().data.height} | |
| /> | |
| <BlurOverlay /> | |
| </div> | |
| </div> | |
| ); |
| const bounds = e.target.getBoundingClientRect()!; | ||
|
|
||
| let time = | ||
| (e.clientX - bounds.left) * secsPerPixel() + | ||
| editorState.timeline.transform.position; | ||
|
|
||
| const nextSegmentIndex = project.timeline?.blurSegments?.findIndex( | ||
| (s) => time < s.start, | ||
| ); | ||
|
|
||
| if (nextSegmentIndex !== undefined) { | ||
| const prevSegmentIndex = nextSegmentIndex - 1; | ||
|
|
||
| if (prevSegmentIndex === undefined) return; | ||
|
|
||
| const nextSegment = | ||
| project.timeline?.blurSegments?.[nextSegmentIndex]; | ||
|
|
||
| if (prevSegmentIndex !== undefined && nextSegment) { | ||
| const prevSegment = | ||
| project.timeline?.blurSegments?.[prevSegmentIndex]; | ||
|
|
||
| if (prevSegment) { | ||
| const availableTime = nextSegment?.start - prevSegment?.end; | ||
|
|
||
| if (availableTime < 1) return; | ||
| } | ||
| } | ||
|
|
||
| if (nextSegment && nextSegment.start - time < 1) { | ||
| time = nextSegment.start - 1; | ||
| } | ||
| } | ||
|
|
||
| setHoveredTime(Math.min(time, duration() - 1)); | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix hovered-time calc: use currentTarget bounds, handle -1 from findIndex, and clamp to [0, duration-1].
The current logic uses e.target (can be a child) and treats findIndex “not found” (-1) as defined, which causes negative indices and inconsistent hover preview. Also ensure non-negative clamping.
Apply this diff:
- const bounds = e.target.getBoundingClientRect()!;
-
- let time =
- (e.clientX - bounds.left) * secsPerPixel() +
- editorState.timeline.transform.position;
-
- const nextSegmentIndex = project.timeline?.blurSegments?.findIndex(
- (s) => time < s.start,
- );
-
- if (nextSegmentIndex !== undefined) {
- const prevSegmentIndex = nextSegmentIndex - 1;
-
- if (prevSegmentIndex === undefined) return;
-
- const nextSegment =
- project.timeline?.blurSegments?.[nextSegmentIndex];
-
- if (prevSegmentIndex !== undefined && nextSegment) {
- const prevSegment =
- project.timeline?.blurSegments?.[prevSegmentIndex];
-
- if (prevSegment) {
- const availableTime = nextSegment?.start - prevSegment?.end;
-
- if (availableTime < 1) return;
- }
- }
-
- if (nextSegment && nextSegment.start - time < 1) {
- time = nextSegment.start - 1;
- }
- }
-
- setHoveredTime(Math.min(time, duration() - 1));
+ const bounds = (e.currentTarget as HTMLDivElement).getBoundingClientRect();
+
+ let time =
+ (e.clientX - bounds.left) * secsPerPixel() +
+ editorState.timeline.transform.position;
+
+ const segments = project.timeline?.blurSegments ?? [];
+ if (segments.length > 0) {
+ const nextIndex = segments.findIndex((s) => time < s.start);
+ if (nextIndex >= 0) {
+ const prevIndex = nextIndex - 1;
+ if (prevIndex >= 0) {
+ const availableTime = segments[nextIndex].start - segments[prevIndex].end;
+ if (availableTime < 1) {
+ setHoveredTime(undefined);
+ return;
+ }
+ }
+ if (segments[nextIndex].start - time < 1) {
+ time = segments[nextIndex].start - 1;
+ }
+ }
+ }
+
+ setHoveredTime(Math.max(0, Math.min(time, duration() - 1)));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const bounds = e.target.getBoundingClientRect()!; | |
| let time = | |
| (e.clientX - bounds.left) * secsPerPixel() + | |
| editorState.timeline.transform.position; | |
| const nextSegmentIndex = project.timeline?.blurSegments?.findIndex( | |
| (s) => time < s.start, | |
| ); | |
| if (nextSegmentIndex !== undefined) { | |
| const prevSegmentIndex = nextSegmentIndex - 1; | |
| if (prevSegmentIndex === undefined) return; | |
| const nextSegment = | |
| project.timeline?.blurSegments?.[nextSegmentIndex]; | |
| if (prevSegmentIndex !== undefined && nextSegment) { | |
| const prevSegment = | |
| project.timeline?.blurSegments?.[prevSegmentIndex]; | |
| if (prevSegment) { | |
| const availableTime = nextSegment?.start - prevSegment?.end; | |
| if (availableTime < 1) return; | |
| } | |
| } | |
| if (nextSegment && nextSegment.start - time < 1) { | |
| time = nextSegment.start - 1; | |
| } | |
| } | |
| setHoveredTime(Math.min(time, duration() - 1)); | |
| }} | |
| const bounds = (e.currentTarget as HTMLDivElement).getBoundingClientRect(); | |
| let time = | |
| (e.clientX - bounds.left) * secsPerPixel() + | |
| editorState.timeline.transform.position; | |
| const segments = project.timeline?.blurSegments ?? []; | |
| if (segments.length > 0) { | |
| const nextIndex = segments.findIndex((s) => time < s.start); | |
| if (nextIndex >= 0) { | |
| const prevIndex = nextIndex - 1; | |
| if (prevIndex >= 0) { | |
| const availableTime = segments[nextIndex].start - segments[prevIndex].end; | |
| if (availableTime < 1) { | |
| setHoveredTime(undefined); | |
| return; | |
| } | |
| } | |
| if (segments[nextIndex].start - time < 1) { | |
| time = segments[nextIndex].start - 1; | |
| } | |
| } | |
| } | |
| setHoveredTime(Math.max(0, Math.min(time, duration() - 1))); |
🤖 Prompt for AI Agents
In apps/desktop/src/routes/editor/Timeline/BlurTrack.tsx around lines 48 to 83,
the hover-time calc uses e.target (which may be a child), treats
Array.findIndex’s -1 as a valid index, and doesn't clamp to a non-negative
range; replace e.target.getBoundingClientRect() with
e.currentTarget.getBoundingClientRect(), treat nextSegmentIndex === -1 as “not
found” (skip prev/next logic when -1), compute prevIndex = nextSegmentIndex - 1
only when nextSegmentIndex > 0, and finally clamp the computed time to be
between 0 and duration() - 1 before calling setHoveredTime to ensure no negative
or out-of-range hover times.
| createRoot((dispose) => { | ||
| createEventListener(e.currentTarget, "mouseup", (e) => { | ||
| dispose(); | ||
|
|
||
| const time = hoveredTime(); | ||
| if (time === undefined) return; | ||
|
|
There was a problem hiding this comment.
Track-click creation should listen on window mouseup to avoid missed events.
If the mouse is released outside the track, the currentTarget “mouseup” won’t fire. Use window to guarantee disposal and correct segment creation.
- createRoot((dispose) => {
- createEventListener(e.currentTarget, "mouseup", (e) => {
+ createRoot((dispose) => {
+ createEventListener(window, "mouseup", (e) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| createRoot((dispose) => { | |
| createEventListener(e.currentTarget, "mouseup", (e) => { | |
| dispose(); | |
| const time = hoveredTime(); | |
| if (time === undefined) return; | |
| createRoot((dispose) => { | |
| createEventListener(window, "mouseup", (e) => { | |
| dispose(); | |
| const time = hoveredTime(); | |
| if (time === undefined) return; |
🤖 Prompt for AI Agents
In apps/desktop/src/routes/editor/Timeline/BlurTrack.tsx around lines 86-92, the
mouseup listener is currently attached to e.currentTarget which misses releases
outside the track; change the listener to attach to window (e.g.,
window.addEventListener("mouseup", ...)) so mouseup is always caught, call
dispose/removeEventListener inside the handler to clean up, and ensure the rest
of the handler logic (getting hoveredTime, creating the segment) remains the
same after switching the target.
| let index = blurSegments.length; | ||
|
|
||
| for (let i = blurSegments.length - 1; i >= 0; i--) { | ||
| if (blurSegments[i].start > time) { | ||
| index = i; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| blurSegments.splice(index, 0, { | ||
| start: time, | ||
| end: time + 1, | ||
| blur_amount: 8, | ||
| rect: { x: 0.25, y: 0.25, width: 0.5, height: 0.5 }, | ||
| }); |
There was a problem hiding this comment.
Insertion index bug: reverse scan inserts at wrong position; use forward findIndex.
The reverse loop breaks on the first element from the end, which misplaces insertions. This can shuffle new segments after later ones.
- let index = blurSegments.length;
-
- for (let i = blurSegments.length - 1; i >= 0; i--) {
- if (blurSegments[i].start > time) {
- index = i;
- break;
- }
- }
-
- blurSegments.splice(index, 0, {
+ let index = blurSegments.findIndex((s) => s.start > time);
+ if (index === -1) index = blurSegments.length;
+ blurSegments.splice(index, 0, {
start: time,
end: time + 1,
blur_amount: 8,
rect: { x: 0.25, y: 0.25, width: 0.5, height: 0.5 },
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let index = blurSegments.length; | |
| for (let i = blurSegments.length - 1; i >= 0; i--) { | |
| if (blurSegments[i].start > time) { | |
| index = i; | |
| break; | |
| } | |
| } | |
| blurSegments.splice(index, 0, { | |
| start: time, | |
| end: time + 1, | |
| blur_amount: 8, | |
| rect: { x: 0.25, y: 0.25, width: 0.5, height: 0.5 }, | |
| }); | |
| let index = blurSegments.findIndex((s) => s.start > time); | |
| if (index === -1) index = blurSegments.length; | |
| blurSegments.splice(index, 0, { | |
| start: time, | |
| end: time + 1, | |
| blur_amount: 8, | |
| rect: { x: 0.25, y: 0.25, width: 0.5, height: 0.5 }, | |
| }); |
🤖 Prompt for AI Agents
In apps/desktop/src/routes/editor/Timeline/BlurTrack.tsx around lines 102-116,
the reverse loop that sets the insertion index incorrectly picks the last
element from the end with start > time, so new segments can be inserted in the
wrong order; replace the reverse scan with a forward scan (or use
Array.findIndex) to find the first segment whose start is greater than time and
set index to that position (default to blurSegments.length if none), then splice
at that index so the new segment is inserted before the first later segment,
preserving chronological order.
| setProject( | ||
| "timeline", | ||
| "blurSegments", | ||
| produce((s) => { | ||
| s?.sort((a, b) => a.start - b.start); | ||
| }), | ||
| ); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove sort-on-move to keep indices stable during drag.
Sorting here can make subsequent setProject calls update the wrong item.
- setProject(
- "timeline",
- "blurSegments",
- produce((s) => {
- s?.sort((a, b) => a.start - b.start);
- }),
- );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setProject( | |
| "timeline", | |
| "blurSegments", | |
| produce((s) => { | |
| s?.sort((a, b) => a.start - b.start); | |
| }), | |
| ); | |
| }, | |
| }, |
🤖 Prompt for AI Agents
In apps/desktop/src/routes/editor/Timeline/BlurTrack.tsx around lines 272-279,
the current code sorts blurSegments after a move which changes item indices and
causes subsequent setProject updates to target the wrong element; remove the
s?.sort((a, b) => a.start - b.start) call so the move preserves the existing
array order (or replace it with a proper stable reorder that only repositions
the moved item by index), and ensure setProject only updates the moved segment
by index rather than resorting the whole array.
| setProject( | ||
| "timeline", | ||
| "blurSegments", | ||
| produce((s) => { | ||
| s?.sort((a, b) => a.start - b.start); | ||
| }), | ||
| ); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove sort-on-resize to avoid index churn mid-drag.
Same rationale as start-handle.
- setProject(
- "timeline",
- "blurSegments",
- produce((s) => {
- s?.sort((a, b) => a.start - b.start);
- }),
- );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setProject( | |
| "timeline", | |
| "blurSegments", | |
| produce((s) => { | |
| s?.sort((a, b) => a.start - b.start); | |
| }), | |
| ); | |
| }, | |
| }, |
🤖 Prompt for AI Agents
In apps/desktop/src/routes/editor/Timeline/BlurTrack.tsx around lines 374 to
381, the resize handler currently calls s?.sort((a, b) => a.start - b.start)
which reorders blurSegments mid-drag and causes index churn; remove that sort so
the array order is preserved during resize (only perform sorting on
finalize/drop if needed), i.e., delete the sort call and leave the produce block
without reordering to match the same rationale as the start-handle change.
| let zoomSegmentDragState = { type: "idle" } as ZoomSegmentDragState; | ||
| let blurSegmentDragState = { type: "idle" } as BlurSegmentDragState; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also freeze playhead while dragging blur segments
handleUpdatePlayhead correctly ignores updates while zoom segments are moving; it should also ignore updates while blur segments are moving.
Apply this change:
- let blurSegmentDragState = { type: "idle" } as BlurSegmentDragState;
+ let blurSegmentDragState = { type: "idle" } as BlurSegmentDragState;And update the check in handleUpdatePlayhead below (see next comment).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/desktop/src/routes/editor/Timeline/index.tsx around lines 76 to 78,
ensure playhead updates are frozen while blur segments are being dragged by
treating blur drags like zoom drags: keep the blurSegmentDragState declaration
as a DragState (already present) and modify the handleUpdatePlayhead logic
(below) to also early-return when blurSegmentDragState.type !== "idle" (i.e.,
ignore playhead updates while a blur segment is moving).
| let sampler = device.create_sampler(&wgpu::SamplerDescriptor { | ||
| label: Some("Selective Blur Sampler"), | ||
| address_mode_u: wgpu::AddressMode::ClampToEdge, | ||
| address_mode_v: wgpu::AddressMode::ClampToEdge, | ||
| address_mode_w: wgpu::AddressMode::ClampToEdge, | ||
| mag_filter: wgpu::FilterMode::Linear, | ||
| min_filter: wgpu::FilterMode::Linear, | ||
| mipmap_filter: wgpu::FilterMode::Linear, | ||
| anisotropy_clamp: 1, | ||
| ..Default::default() | ||
| }); |
There was a problem hiding this comment.
Compilation issue: invalid SamplerDescriptor.anisotropy_clamp type
On current wgpu (as used elsewhere in this PR with entry_point: Some("...") and compilation_options), SamplerDescriptor::anisotropy_clamp is Option<NonZeroU8>. Assigning 1 will not compile.
Apply one of the following:
- If you don’t need anisotropic filtering:
- anisotropy_clamp: 1,
+ anisotropy_clamp: None,- If you do:
+ anisotropy_clamp: std::num::NonZeroU8::new(4),Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/rendering/src/layers/selective_blur.rs around lines 18 to 28,
SamplerDescriptor.anisotropy_clamp is currently set to the integer 1 which fails
to compile because the field type is Option<NonZeroU8>; change this to either
None if you don't need anisotropy, or Some(NonZeroU8::new(1).unwrap()) (and add
use std::num::NonZeroU8) if you do need 1 level of anisotropic filtering so the
type matches Option<NonZeroU8>.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
crates/rendering/src/shaders/blur.wgsl (1)
56-83: Prevent NaNs when blur_amount is zero; ensure stable kernel math.Zero blur leads to radius/sigma = 0 and
exp(-0/(2*0))-> NaN, propagating to the output. Add an early-out, clamp radius ≥ 1 for non-zero blur, and use a precomputed factor. This mirrors prior feedback.Apply this diff:
fn apply_blur(uv: vec2<f32>, blur_amount: f32) -> vec4<f32> { - let pixel_size = 1.0 / uniforms.output_size; - var color = vec4<f32>(0.0); - var total_weight = 0.0; + // Clamp to non-negative and early-out to avoid NaNs when amount == 0 + let amount = max(blur_amount, 0.0); + if (amount <= 0.0) { + return textureSample(t_input, s_input, clamp(uv, vec2<f32>(0.0), vec2<f32>(1.0))); + } + + let pixel_size = 1.0 / uniforms.output_size; + var color = vec4<f32>(0.0); + var total_weight = 0.0; // Reduced kernel size for better performance - let radius = i32(blur_amount * 8.0); // Dynamic radius based on blur amount - let max_radius = min(radius, 25); // Cap at 25 to prevent excessive samples - let sigma = f32(max_radius) / 2.5; + // Ensure radius >= 1 for non-zero blur and cap to prevent excessive samples + let radius = min(max(i32(amount * 8.0), 1), 25); // Dynamic radius based on blur amount + let sigma = f32(radius) / 2.5; + let inv_2_sigma2 = 1.0 / (2.0 * sigma * sigma); - for (var y = -max_radius; y <= max_radius; y = y + 1) { - for (var x = -max_radius; x <= max_radius; x = x + 1) { + for (var y = -radius; y <= radius; y = y + 1) { + for (var x = -radius; x <= radius; x = x + 1) { let offset = vec2<f32>(f32(x), f32(y)) * pixel_size; let sample_pos = uv + offset; let sample_uv = clamp(sample_pos, vec2<f32>(0.0), vec2<f32>(1.0)); let dist_sq = f32(x * x + y * y); - let weight = exp(-dist_sq / (2.0 * sigma * sigma)); + let weight = exp(-dist_sq * inv_2_sigma2); color += textureSample(t_input, s_input, sample_uv) * weight; total_weight += weight; } } - return color / max(total_weight, 0.001); + // total_weight is guaranteed > 0 here + return color / total_weight; }
🧹 Nitpick comments (3)
crates/rendering/src/shaders/blur.wgsl (3)
85-102: Clamp UVs in fragment to avoid edge sampling issues.For safety against precision drift at the viewport edges, clamp
frag_in.uvbefore comparisons and sampling. Also ensures fallback path never samples out-of-range.Apply this diff:
@fragment fn fs_main(frag_in: VertexOutput) -> @location(0) vec4<f32> { - - for (var i: u32 = 0u; i < uniforms.blur_segments_count; i = i + 1u) { - let segment = blur_segments[i]; + // Clamp UVs for safety against precision drift at screen edges + let uv = clamp(frag_in.uv, vec2<f32>(0.0), vec2<f32>(1.0)); + + for (var i: u32 = 0u; i < uniforms.blur_segments_count; i = i + 1u) { + let segment = blur_segments[i]; let rect = segment.rect; // Check if pixel is inside the blur rectangle - if (frag_in.uv.x >= rect.x && frag_in.uv.x <= rect.x + rect.z && - frag_in.uv.y >= rect.y && frag_in.uv.y <= rect.y + rect.w) { + if (uv.x >= rect.x && uv.x <= rect.x + rect.z && + uv.y >= rect.y && uv.y <= rect.y + rect.w) { // Apply blur - sample from entire texture, not just rectangle - return apply_blur(frag_in.uv, segment.blur_amount); + return apply_blur(uv, segment.blur_amount); } } - return textureSample(t_input, s_input, frag_in.uv); + return textureSample(t_input, s_input, uv); }
85-102: Optional: predictable behavior for overlapping blur rectangles (combine via max intensity).Current logic returns on the first matching segment; overlapping segments may produce inconsistent results depending on buffer order. Consider combining overlaps via max blur for determinism.
Apply this diff (assumes the storage-buffer struct fix and loop-bound guard):
@fragment fn fs_main(frag_in: VertexOutput) -> @location(0) vec4<f32> { - // Clamp UVs for safety against precision drift at screen edges - let uv = clamp(frag_in.uv, vec2<f32>(0.0), vec2<f32>(1.0)); - - for (var i: u32 = 0u; i < uniforms.blur_segments_count; i = i + 1u) { - let segment = blur_segments.segments[i]; - let rect = segment.rect; - if (uv.x >= rect.x && uv.x <= rect.x + rect.z && - uv.y >= rect.y && uv.y <= rect.y + rect.w) { - return apply_blur(uv, segment.blur_amount); - } - } - return textureSample(t_input, s_input, uv); + // Clamp UVs for safety against precision drift at screen edges + let uv = clamp(frag_in.uv, vec2<f32>(0.0), vec2<f32>(1.0)); + + // Combine overlapping segments by taking the max blur amount + let count = min(uniforms.blur_segments_count, arrayLength(&blur_segments.segments)); + var max_blur: f32 = 0.0; + for (var i: u32 = 0u; i < count; i = i + 1u) { + let segment = blur_segments.segments[i]; + let rect = segment.rect; + if (uv.x >= rect.x && uv.x <= rect.x + rect.z && + uv.y >= rect.y && uv.y <= rect.y + rect.w) { + max_blur = max(max_blur, segment.blur_amount); + } + } + if (max_blur > 0.0) { + return apply_blur(uv, max_blur); + } + return textureSample(t_input, s_input, uv); }
56-76: Performance note: consider separable Gaussian blur (two-pass) later.A 51x51 kernel at the cap is 2601 samples per pixel, multiplied by segments and affected area. A separable blur (horizontal then vertical) reduces complexity from O(r^2) to O(2r) per pixel, often with better cache behavior.
If you want, I can sketch the pipeline split (two fragment passes with intermediate texture and reusing the same segment mask).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
crates/rendering/src/shaders/blur.wgsl(1 hunks)
🔇 Additional comments (1)
crates/rendering/src/shaders/blur.wgsl (1)
33-53: Fullscreen triangle setup and UV mapping look correct.Using a fullscreen triangle with precomputed UVs is a solid choice to avoid vertex buffers and edge artifacts.
| @group(0) @binding(0) var<uniform> uniforms: Uniforms; | ||
| @group(0) @binding(1) var t_input: texture_2d<f32>; | ||
| @group(0) @binding(2) var s_input: sampler; | ||
| @group(0) @binding(3) var<storage, read> blur_segments: array<BlurSegment>; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix invalid storage buffer declaration (runtime array must be inside a struct).
WGSL requires storage bindings to be struct types; top-level array<...> is not allowed. This will fail shader validation.
Apply this diff:
+// Wrap runtime array in a struct per WGSL rules
+struct BlurSegments {
+ segments: array<BlurSegment>,
+};
@group(0) @binding(0) var<uniform> uniforms: Uniforms;
@group(0) @binding(1) var t_input: texture_2d<f32>;
@group(0) @binding(2) var s_input: sampler;
-@group(0) @binding(3) var<storage, read> blur_segments: array<BlurSegment>;
+@group(0) @binding(3) var<storage, read> blur_segments: BlurSegments;🤖 Prompt for AI Agents
In crates/rendering/src/shaders/blur.wgsl around lines 23 to 27, the storage
binding declares a top-level array which WGSL disallows; wrap the runtime array
inside a struct and update the binding type accordingly. Define a struct (e.g.,
struct BlurSegments { data: array<BlurSegment>; };) and change the binding to
var<storage, read> blur_segments: BlurSegments; so the storage buffer is a
struct type containing the runtime array while keeping the same @group/@binding.
| for (var i: u32 = 0u; i < uniforms.blur_segments_count; i = i + 1u) { | ||
| let segment = blur_segments[i]; | ||
| let rect = segment.rect; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against OOB reads: clamp loop bound to actual storage array length.
If the CPU sets blur_segments_count higher than the buffer length, the loop will read out-of-bounds. Use arrayLength.
Apply this diff:
- for (var i: u32 = 0u; i < uniforms.blur_segments_count; i = i + 1u) {
+ let count = min(uniforms.blur_segments_count, arrayLength(&blur_segments.segments));
+ for (var i: u32 = 0u; i < count; i = i + 1u) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (var i: u32 = 0u; i < uniforms.blur_segments_count; i = i + 1u) { | |
| let segment = blur_segments[i]; | |
| let rect = segment.rect; | |
| // Clamp the segment count to the actual length of the runtime array to avoid OOB reads | |
| let count = min(uniforms.blur_segments_count, arrayLength(&blur_segments.segments)); | |
| for (var i: u32 = 0u; i < count; i = i + 1u) { | |
| let segment = blur_segments[i]; | |
| let rect = segment.rect; |
| let segment = blur_segments[i]; | ||
| let rect = segment.rect; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update indexing to reflect storage struct wrapper.
After wrapping the storage array in a struct, access the segments field.
Apply this diff:
- let segment = blur_segments[i];
+ let segment = blur_segments.segments[i];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let segment = blur_segments[i]; | |
| let rect = segment.rect; | |
| let segment = blur_segments.segments[i]; | |
| let rect = segment.rect; |
🤖 Prompt for AI Agents
In crates/rendering/src/shaders/blur.wgsl around lines 89 to 90, the code still
indexes the storage array directly (blur_segments[i]); update the indexing to
access the wrapped field (e.g., blur_segments.segments[i]) so you read the
element from the struct's segments array, and then use that element for rect
(let segment = blur_segments.segments[i]; let rect = segment.rect;).
Description
Implements selective blur functionality for video input with proper coordinate system alignment between frontend UI and GPU shader.
Changes Made
Frontend (TypeScript/SolidJS):
BlurOverlay.tsxfor interactive blur rectangle manipulationBlurTrack.tsxfor timeline-based blur segment managementBackend (Rust/WGPU):
SelectiveBlurPipelinewith proper GPU buffer managementSelectiveBlurLayerfor render pass integrationShader (WGSL):
Resolves #143
Summary by CodeRabbit