Repository navigation
perf: separable triangle resample for screenshot blur masks - #2979
Conversation
|
@kyzer1023 is attempting to deploy a commit to the Cap Software Inc Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ) | ||
| }; | ||
| let elapsed = start.elapsed().as_nanos(); | ||
| black_box(image.unwrap().into_raw().len()); |
There was a problem hiding this comment.
time_call passes only the output length to black_box. That length does not depend on the computed pixels, so an optimized build may remove pixel calculations and distort the speedup comparison. Pass the whole image or its pixel buffer to black_box instead.
| black_box(image.unwrap().into_raw().len()); | |
| black_box(image.unwrap()); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/kernels/benches/screenshot_blur.rs
Line: 58
Comment:
**Timings may skip pixel work**
`time_call` passes only the output length to `black_box`. That length does not depend on the computed pixels, so an optimized build may remove pixel calculations and distort the speedup comparison. Pass the whole image or its pixel buffer to `black_box` instead.
```suggestion
black_box(image.unwrap());
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in f28779a. time_call now passes the finished image to black_box. Reran with the same profile: 1080p level 16 is 65.25 ms vs 10.18 ms (6.41x), 4K level 16 is 550.5 ms vs 51.2 ms (10.74x).
| let rgba: Vec<u8> = (0..width * height * 4) | ||
| .map(|i| (i.wrapping_mul(37).wrapping_add(19) % 251) as u8) |
There was a problem hiding this comment.
The new blur tests generate every channel modulo 251, so they never test 255, including the fully opaque alpha used by ordinary screenshots. They also do not directly test the midpoint correction in quantize_channel.
Add full-range image cases and a direct test comparing quantize_channel with value.clamp(0.0, 255.0).round() as u8 at each half-integer and the adjacent representable values. This would catch rounding changes that the current image cases may miss.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/kernels/src/screenshot.rs
Line: 744-745
Comment:
**Rounding boundaries lack direct tests**
The new blur tests generate every channel modulo 251, so they never test 255, including the fully opaque alpha used by ordinary screenshots. They also do not directly test the midpoint correction in `quantize_channel`.
Add full-range image cases and a direct test comparing `quantize_channel` with `value.clamp(0.0, 255.0).round() as u8` at each half-integer and the adjacent representable values. This would catch rounding changes that the current image cases may miss.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in f28779a. blur_matches_triangle_resize_for_full_byte_range covers frames with 0 and 255 in every channel (including opaque alpha), solid 0, solid 255 and a sub-rectangle, compared byte for byte with the two Triangle resizes. quantize_channel_matches_clamped_round checks every half-integer from 0.5 to 254.5 and its neighboring floats, 0 to 255, negatives and values above 255 (including infinities) against value.clamp(0.0, 255.0).round() as u8.
|
Thanks @kyzer1023, this looks great: we checked the blur output is byte-identical to the previous
|
Pass the whole blur image to black_box. Add a full-range byte match and a direct quantize_channel test.
|
Thanks @richiemcilroy. Both notes are addressed in f28779a: the bench passes the whole image to @greptileai review |
Summary
Screenshot blur masks scale a region down with a triangle filter and then scale it back up. This used
image::imageops::resizetwice. A separable resampler takes the same samples and produces byte-identical output, and it is 4x to 12x faster. Pixelate masks are unchanged.Change
mask_filterinapps/desktop-gpui/kernels/src/screenshot.rsstill usespixelate_regionwhen pixelate is set. The blur branch now resamples the region directly: a vertical pass intof32, then a horizontal pass that rounds tou8. The downsample factor is still(level / 4).round().max(2), and a second resample restores the original size. The weights follow the triangle kernel inimage0.25.10, including the scaled support and the division by the weight sum. Rounding truncatesx + 0.5and steps back by one just below a midpoint. That matchesf32::roundon [0, 255] without the libm call.Benchmarks
Command:
cargo bench --bench screenshot_blurRelease profile with
lto = "thin"andcodegen-units = 1. Each time is the median of alternating before and after runs, after 3 discarded warmup iterations. The bench asserts the full buffers are equal before it records times, and passes each finished image toblack_boxso no pixel work can be optimized away. Toolchain: rustc 1.95.0.Machine: Intel Xeon, family 6 model 207, 4 vCPU, x86_64, KVM.
Max absolute difference is 0, and so is the mean.
Tests
cargo test --releaseoncap-gpui-kernels: 65 passed, 0 failed, 1 ignored. The ignored test is the existing waveform benchmark.blur_matches_triangle_resize_for_full_byte_rangeuses frames whose channels include 0 and 255 (including opaque alpha), plus solid 0, solid 255 and a sub-rectangle, and compares byte for byte with the twoimageTriangle resizes.quantize_channel_matches_clamped_roundcomparesquantize_channelwithvalue.clamp(0.0, 255.0).round() as u8for every half-integer from 0.5 to 254.5 and its neighboring floats, the integers 0 to 255, negatives down to negative infinity, and values above 255 up to positive infinity.blur_matches_triangle_resize_across_sizes_levels_and_odd_dimensionscompares the new blur with twoimageTriangle resizes. It covers odd sizes, sub-rectangles, empty edges, levels from 0.5 to 100, and 400x300 at levels 8, 16 and 40.pixelate_matches_two_nearest_resizesstill passes.cargo fmt,cargo checkandcargo clippy --all-targets -- -D warningsare clean.Note: the cloud machine could not resolve the workspace
gpuipatch, so these ran from a standalone copy ofapps/desktop-gpui/kernelswith the same sources andimagepinned to 0.25.10. CI is the first full-workspace build.Test plan
cargo test -p cap-gpui-kernels --releasecargo bench -p cap-gpui-kernels --bench screenshot_blurRisk
The blur bytes match the triangle resize in
image0.25.10 for every size covered here. Blur no longer callsimageops::resize, so laterimagereleases will not change this path. The horizontal loops read through pointers that stay inside the precomputed tap window. Pixelate is untouched.
The PR appears safe to merge; both previous findings are addressed and no new blocking issue was found.Summary
Replaces screenshot blur's two Triangle resizes with a separable resampler. Pixelate remains unchanged.
Reviews (2) · Last reviewed commit: "fix: cover screenshot blur bench output ..." · Reviewed by Greptile