Repository navigation
perf(desktop-gpui): optimize 32-bit row conversions, NV12 planes, and screenshot blitting - #2668
nikolas-sapa wants to merge 2 commits into
Conversation
… screenshot blitting - Vectorize convert_32bit_rows across RGBA, BGRA, ARGB, and ABGR - Precompute scales and share UV chroma math across pixel pairs in convert_nv12_planes - Precalculate X coordinate offsets in downsample_nearest - Short-circuit alpha == 0 and fast-path alpha == 255 in screenshot blit_over_offset - Add frame_convert benchmark suite to lock in speedups Benchmarks: - 1080p BGRA conversion: 1311 µs -> 293 µs (4.48x speedup) - 1080p RGBA conversion: 832 µs -> 219 µs (3.80x speedup) - 1080p ARGB conversion: 1299 µs -> 242 µs (5.36x speedup) - 1080p ABGR conversion: 1333 µs -> 231 µs (5.77x speedup) - 1080p BGRA padded: 1324 µs -> 235 µs (5.64x speedup) - 1080p -> 720p downsample: 510 µs -> 413 µs (1.24x speedup) - NV12 plane conversions: ~1.15x speedup - Screenshot blitting: up to 11x faster on opaque overlays, 2.5x-3.2x on edge/sparse alpha
|
@nikolas-sapa is attempting to deploy a commit to the Cap Software Inc Team on Vercel. A member of the Team first needs to authorize it. |
| if x < width { | ||
| let cb = uv_row[x]; | ||
| let cr = uv_row[x + 1]; |
There was a problem hiding this comment.
convert_nv12_planes now panics for positive odd widths instead of returning None. With width = 3, height = 1, and sufficient plane data, the pair loop leaves x = 2. The remainder reads uv_row[3], but that slice has length 3—even when the original UV plane has padding. Keep the previous rejection of incomplete chroma pairs and add an odd-width test.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/kernels/src/frame.rs
Line: 354-356
Comment:
**Odd widths now panic**
`convert_nv12_planes` now panics for positive odd widths instead of returning `None`. With `width = 3`, `height = 1`, and sufficient plane data, the pair loop leaves `x = 2`. The remainder reads `uv_row[3]`, but that slice has length 3—even when the original UV plane has padding. Keep the previous rejection of incomplete chroma pairs and add an odd-width test.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| let g_chroma = -0.391762_f32 * cb_f - 0.812968_f32 * cr_f; | ||
| let b_chroma = 2.017232_f32 * cb_f; | ||
|
|
||
| let y1 = ((f32::from(y_row[x]) - y_sub).max(0.0)) * scale; | ||
| rgba_row[dst_idx] = clamp_channel(y1 + r_chroma); | ||
| rgba_row[dst_idx + 1] = clamp_channel(y1 + g_chroma); |
There was a problem hiding this comment.
Combining g_chroma changes the green byte for some valid NV12 pixels. In video range, (Y, Cb, Cr) = (36, 80, 58) previously produces green 99. The new expression produces approximately 98.999992 and truncates to 98. The benchmark's repeating inputs miss this case. Precompute the two chroma products separately, preserve the original subtraction order, and add this comparison case.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/kernels/src/frame.rs
Line: 335-340
Comment:
**Green bytes change**
Combining `g_chroma` changes the green byte for some valid NV12 pixels. In video range, `(Y, Cb, Cr) = (36, 80, 58)` previously produces green 99. The new expression produces approximately 98.999992 and truncates to 98. The benchmark's repeating inputs miss this case. Precompute the two chroma products separately, preserve the original subtraction order, and add this comparison case.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| for (name, width, height, stride, order) in [ | ||
| ("1080p-bgra", 1920, 1080, 1920 * 4, ChannelOrder::Bgra), | ||
| ("1080p-rgba", 1920, 1080, 1920 * 4, ChannelOrder::Rgba), | ||
| ("1080p-argb", 1920, 1080, 1920 * 4, ChannelOrder::Argb), | ||
| ("1080p-abgr", 1920, 1080, 1920 * 4, ChannelOrder::Abgr), | ||
| ("1080p-bgra-padded", 1920, 1080, 7936, ChannelOrder::Bgra), |
There was a problem hiding this comment.
The new convert_32bit_rows benchmark only uses widths divisible by four. Existing tests use two pixels, so neither checks an Argb or Abgr row that runs both the new 16-byte loop and its remainder. Add padded, multi-row cases with widths 5, 6, and 7 against the baseline. Otherwise, errors where those paths meet can pass every current comparison.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/kernels/benches/frame_convert.rs
Line: 60-65
Comment:
**Mixed row paths lack tests**
The new `convert_32bit_rows` benchmark only uses widths divisible by four. Existing tests use two pixels, so neither checks an `Argb` or `Abgr` row that runs both the new 16-byte loop and its remainder. Add padded, multi-row cases with widths 5, 6, and 7 against the baseline. Otherwise, errors where those paths meet can pass every current comparison.
---
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!
…test unaligned rows
|
Thanks @nikolas-sapa! Greptile is at 4/5 here. We only merge once Greptile reaches 5/5, so please address its findings in the summary above (or reply to them if you think one is wrong), then comment |
Description
Optimizes pixel kernel operations used in GPUI desktop app rendering, recording frame conversions, thumbnail downsampling, and screenshot blitting.
Changes
convert_32bit_rows(RGBA, BGRA, ARGB, ABGR):convert_nv12_planes:Nv12Range.downsample_nearest:(x * width / target_width) * 4for every pixel on every row.blit_over_offset(screenshot blitting):alpha == 0) without callingblend_pixel_over.alpha == 255) using direct slice copies without float conversions and roundtrip clamping.frame_convertbenchmark suite to lock in and track speedups.Benchmarks
Ran on Apple Silicon M-series (macOS):
convert_32bit_rowsdownsample_nearestconvert_nv12_planesscreenshot_blitVerification
cargo test -p cap-gpui-kernels: 30 passed, 0 failed.cargo bench --bench frame_convert: all assertions verifyexpected == actual.cargo bench --bench screenshot_blit: all assertions verifybefore == afterbit-for-bit.The PR appears safe to merge with non-blocking fixes for NV12 edge cases and test coverage.
Findings
Fix with agent prompt
Summary
Optimizes row copies, channel swaps, NV12 conversion, downsampling, and screenshot blending. Adds a reference-based frame benchmark.
Reviews (1) · Last reviewed commit: "perf(desktop-gpui): optimize 32-bit row ..." · Reviewed by Greptile