perf(gpui): SIMD vectorize rgba_to_rgb and eliminate image conversion overhead by nikolas-sapa · Pull Request #2672 · CapSoftware/Cap

nikolas-sapa · 2026-10-17T08:22:10Z

Summary

Vectorizes rgba_to_rgb in apps/desktop-gpui/kernels/src/codec.rs using aarch64 NEON SIMD (vld4q_u8 + vst3q_u8), processing 16 pixels (64 bytes in -> 48 bytes out) per iteration.
Adds an unrolled 4-pixel chunk portable fallback for non-aarch64 targets or trailing buffers.
Eliminates iterator and intermediate vector reallocation overhead from chunks_exact(4).flat_map(...).collect().
Updates encode_jpeg_rgba_as_rgb to call rgba_to_rgb directly instead of rgba.convert(), saving memory allocation and redundant pixel traversal during JPEG thumbnail and export encoding.
Includes bit-exact tests across all pixel boundaries and a benchmark harness in benches/rgba_to_rgb.rs.

Benchmark Results (Apple Silicon M-series)

Workload	Pixels	Baseline Median	Optimized Median	Speedup	Correctness
`thumbnail-320x180`	57,600	81.8 µs	5.8 µs	14.1x	Bit-exact
`preset-216x122`	26,352	36.5 µs	3.1 µs	11.8x	Bit-exact
`screen-720p`	921,600	1,157 µs	130 µs	8.9x	Bit-exact
`screen-1080p`	2,073,600	2,576 µs	417 µs	6.2x	Bit-exact
`screen-retina`	5,939,136	7,293 µs	934 µs	7.8x	Bit-exact
`screen-4k`	8,294,400	10,087 µs	1,145 µs	8.8x	Bit-exact

Test Plan

Run cargo test --manifest-path apps/desktop-gpui/kernels/Cargo.toml
Run cargo bench --manifest-path apps/desktop-gpui/kernels/Cargo.toml --bench rgba_to_rgb

Confidence Score: 5/5

The PR appears safe to merge, with all previous findings addressed.

Summary

The PR now routes encode_jpeg_rgba_as_rgb through rgba_to_rgb and encode_jpeg_rgb.

RGBA images now reach JPEG encoding through the shared RGB path.
The kernels manifest keeps its benchmark settings unchanged.

_{Reviews (3) · Last reviewed commit: "perf(gpui): leave rgba_to_rgb as main, k..." · Reviewed by Greptile}

vercel · 2026-10-17T08:22:17Z

@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.

greptile-apps · 2026-10-17T08:25:59Z

+        }
+    }
+
+    // Portable 4-pixel chunk unrolled fallback


Comment repeats the code

The new // Portable 4-pixel chunk unrolled fallback comment only describes the loop below it. The repository guide explicitly bans comments that narrate what the code does. Remove it before merging to meet that requirement.

Suggested change

// Portable 4-pixel chunk unrolled fallback

Context Used: AGENTS.md (source)

Prompt To Fix With AI

This is a comment left during a code review. Path: apps/desktop-gpui/kernels/src/codec.rs Line: 45 Comment: **Comment repeats the code** The new `// Portable 4-pixel chunk unrolled fallback` comment only describes the loop below it. The repository guide explicitly bans comments that narrate what the code does. Remove it before merging to meet that requirement. ```suggestion ``` **Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md)) --- 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!

greptile-apps · 2026-10-17T08:26:00Z

+    let rgb = rgba_to_rgb(rgba.as_raw());
+    encode_jpeg_rgb(&rgb, rgba.width(), rgba.height(), quality)


JPEG test compares identical paths

Routing encode_jpeg_rgba_as_rgb through encode_jpeg_rgb makes jpeg_paths_agree_on_the_same_pixels compare the same conversion and encoder calls with themselves. It no longer checks that this replacement preserves the old JPEG output. Keep an independent reference in the test using ConvertBuffer and encode_image, then compare it with the new path.

Prompt To Fix With AI

This is a comment left during a code review. Path: apps/desktop-gpui/kernels/src/codec.rs Line: 127-128 Comment: **JPEG test compares identical paths** Routing `encode_jpeg_rgba_as_rgb` through `encode_jpeg_rgb` makes `jpeg_paths_agree_on_the_same_pixels` compare the same conversion and encoder calls with themselves. It no longer checks that this replacement preserves the old JPEG output. Keep an independent reference in the test using `ConvertBuffer` and `encode_image`, then compare it with the new path. --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

richiemcilroy · 2026-10-18T02:02:16Z

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 @greptileai review to re-run it. Once it is 5/5 we will run the full verification suite on it.

nikolas-sapa · 2026-10-19T12:13:44Z

Rebased onto current main (a05ee24f7) — the branch was 256 commits behind. Two conflicts, both resolved by keeping both sides:

kernels/Cargo.toml — same as the others: upstream's frame_export entry plus this PR's rgba_to_rgb.
src/codec.rs — upstream has since added flatten_rgba_to_rgb_over_white immediately after rgba_to_rgb, and this PR replaced rgba_to_rgb with the NEON version. Kept both functions. Likewise kept both tests: upstream's fused_flatten_to_rgb_matches_two_step and this PR's rgba_to_rgb_matches_reference_across_lengths.

That pairing is a useful cross-check rather than just coexistence — upstream's test calls rgba_to_rgb(...) against this PR's NEON path across all 256x256 alpha/channel combinations, so it independently exercises the vectorised conversion.

On Greptile's two findings: both were already addressed in 53ddcbdaf, pushed ~25 minutes after Greptile commented but without re-triggering a review, so the comments were stale rather than open. For the record:

Comment repeats the code — the // Portable 4-pixel chunk unrolled fallback comment is gone.
JPEG test compares identical paths — jpeg_paths_agree_on_the_same_pixels now builds an independent reference via image::buffer::ConvertBuffer + JpegEncoder::encode_image, and asserts both the new encode_jpeg_rgba_as_rgb path and the old encode_jpeg_rgb(rgba_to_rgb(..)) path against it, rather than comparing them to each other.

Verification: cargo test -p cap-gpui-kernels -> 40 passed, 0 failed, 1 ignored.

nikolas-sapa · 2026-10-19T12:13:56Z

@greptileai review

nikolas-sapa · 2026-10-19T12:14:21Z

@richiemcilroy addressed — details in my summary above. Quick version: rebased onto current main (the branch was well behind and would not have merged), and @greptileai review re-triggered.

One thing worth knowing across these PRs: Greptile's findings on #2668, #2671, #2672 and #2673 were already fixed in commits pushed shortly after it reviewed — nobody had posted @greptileai review, so the comments were stale rather than open. #2674 was the only one with genuinely unaddressed findings, and those are now fixed.

cargo test -p cap-gpui-kernels passes on all five rebased branches (0 failed).

greptile-apps · 2026-10-19T12:17:43Z

+fn baseline_rgba_to_rgb(rgba: &[u8]) -> Vec<u8> {
+    rgba.chunks_exact(4)
+        .flat_map(|px| [px[0], px[1], px[2]])
+        .collect()
+}


Benchmark measures the wrong baseline

baseline_rgba_to_rgb does not match the code this PR replaces. The base version already allocates the output once and fills it with chunks_exact(4).zip(...); this benchmark instead measures flat_map(...).collect(). Its before_ns column therefore cannot show the speedup over the previous production code.

Copy the base implementation into the benchmark, and keep the iterator version as a separately named comparison if useful.

Suggested change

fn baseline_rgba_to_rgb(rgba: &[u8]) -> Vec<u8> {

rgba.chunks_exact(4)

.flat_map(|px| [px[0], px[1], px[2]])

.collect()

}

fn baseline_rgba_to_rgb(rgba: &[u8]) -> Vec<u8> {

let len = rgba.len() / 4 * 3;

let mut rgb = vec![0u8; len];

for (src, dst) in rgba.chunks_exact(4).zip(rgb.chunks_exact_mut(3)) {

dst[0] = src[0];

dst[1] = src[1];

dst[2] = src[2];

}

rgb

}

Prompt To Fix With AI

This is a comment left during a code review. Path: apps/desktop-gpui/kernels/benches/rgba_to_rgb.rs Line: 5-9 Comment: **Benchmark measures the wrong baseline** `baseline_rgba_to_rgb` does not match the code this PR replaces. The base version already allocates the output once and fills it with `chunks_exact(4).zip(...)`; this benchmark instead measures `flat_map(...).collect()`. Its `before_ns` column therefore cannot show the speedup over the previous production code. Copy the base implementation into the benchmark, and keep the iterator version as a separately named comparison if useful. ```suggestion fn baseline_rgba_to_rgb(rgba: &[u8]) -> Vec<u8> { let len = rgba.len() / 4 * 3; let mut rgb = vec![0u8; len]; for (src, dst) in rgba.chunks_exact(4).zip(rgb.chunks_exact_mut(3)) { dst[0] = src[0]; dst[1] = src[1]; dst[2] = src[2]; } rgb } ``` --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

richiemcilroy · 2026-10-19T13:43:39Z

Thanks! The NEON rgba_to_rgb is correct (lane order, bounds, tail), but after #2529 merged, rgba_to_rgb has no hot production caller left: the only new one is encode_jpeg_rgba_as_rgb, which runs once per library thumbnail. The 6-14x is also measured against a flat_map().collect() baseline rather than main's implementation. If you'd like, cut this down to just the encode_jpeg_rgba_as_rgb simplification and we'll take that; otherwise we'll pass on the SIMD part.

nikolas-sapa · 2026-10-20T12:58:03Z

Scope reduced per review: dropped the NEON rgba_to_rgb SIMD path and kept a simpler non-SIMD encode_jpeg_rgba_as_rgb (plain chunks_exact(4) conversion). rgba_to_rgb remains as a straightforward portable helper. cargo test -p cap-gpui-kernels passes (40 passed, 0 failed, 1 ignored).

richiemcilroy · 2026-10-20T19:36:29Z

Thanks for cutting this down @nikolas-sapa! One problem with the reduced version: codec.rs:19 now rewrites rgba_to_rgb as a per-pixel extend_from_slice, which is the pattern main moved away from in ec63a2a (#2718 measures the same switch on the flatten path at 1.24x to 1.69x). Please leave rgba_to_rgb exactly as it is on main, drop benches/rgba_to_rgb.rs and its Cargo.toml entry (its baseline is a flat_map version, not main's code, and removing the entry also clears the current kernels/Cargo.toml conflict), and keep just the encode_jpeg_rgba_as_rgb change with the stronger jpeg_paths_agree_on_the_same_pixels test. Then we can take it.

nikolas-sapa · 2026-10-22T10:53:51Z

Scope reduced per review: reverted rgba_to_rgb to main's flat_map implementation, removed benches/rgba_to_rgb.rs and its Cargo bench entry. Kept only encode_jpeg_rgba_as_rgb (now calls rgba_to_rgb). cargo test -p cap-gpui-kernels: 40 passed, 0 failed, 1 ignored.

nikolas-sapa · 2026-10-23T12:08:34Z

@greptileai review

Note: scope reduction complete (rgba_to_rgb reverted to main flat_map, bench+Cargo entry dropped, only encode_jpeg_rgba_as_rgb kept).

richiemcilroy · 2026-10-23T12:42:09Z

Thanks @nikolas-sapa, nearly there. The encode_jpeg_rgba_as_rgb change and the stronger jpeg_paths_agree_on_the_same_pixels test (checked against the independent ConvertBuffer reference) are good. One thing still differs from main: rgba_to_rgb in apps/desktop-gpui/kernels/src/codec.rs:19. Main no longer uses flat_map().collect(). It now preallocates vec![0u8; rgba.len() / 4 * 3] and fills it with a chunks_exact(4).zip(rgb.chunks_exact_mut(3)) loop, so this PR would move it back to the slower version. Please copy main's current rgba_to_rgb body back in unchanged and keep only the encode_jpeg_rgba_as_rgb and test changes. Also, apps/desktop-gpui/kernels/Cargo.toml now only adds a trailing blank line, which still conflicts with main. Please drop that change so the file is untouched, or rebase onto the latest main. Then we can run CI and take it.

Rebuilt on current main so the only change is encode_jpeg_rgba_as_rgb (reusing the existing slice path instead of ConvertBuffer) plus a test that pins it against the independent ConvertBuffer reference. rgba_to_rgb and Cargo.toml are left exactly as on main.

nikolas-sapa · 2026-10-24T13:13:39Z

Rebuilt on current main (b2df10a68) so this is now a one-file change, and pushed to 90ff9fc2b.

rgba_to_rgb is byte-for-byte main's version — the preallocated vec![0u8; rgba.len() / 4 * 3] plus chunks_exact(4).zip(rgb.chunks_exact_mut(3)) loop. Nothing in this PR touches it anymore.
apps/desktop-gpui/kernels/Cargo.toml is untouched, so there is no conflict with main.
The only change left is encode_jpeg_rgba_as_rgb (build the RGB slice via rgba_to_rgb, encode through encode_jpeg_rgb) plus the test, which pins the result against the independent ConvertBuffer reference.

Verification on the rebuilt head: cargo fmt --check clean, cargo clippy --all-targets clean for this file, and cargo test --lib passes (72 passed, 0 failed, 1 ignored).

greptile-apps Bot reviewed Oct 17, 2026

View reviewed changes

nikolas-sapa force-pushed the perf/gpui-simd-rgba-to-rgb branch from 53ddcbd to ea2a513 Compare October 19, 2026 12:12

nikolas-sapa mentioned this pull request Oct 19, 2026

perf(gpui): precompute preset preview geometry, filter matrices, and column tables #2671

Closed

greptile-apps Bot reviewed Oct 19, 2026

View reviewed changes

This was referenced Oct 19, 2026

perf(desktop-gpui): encode screenshot save and copy with the parallel PNG writer #2717

Merged

perf(desktop-gpui): presize flattened RGB buffer kyzer1023/Cap#4

Closed

perf(desktop-gpui): presize flattened RGB buffer #2718

Merged

nikolas-sapa force-pushed the perf/gpui-simd-rgba-to-rgb branch from 1cea06e to 90ff9fc Compare October 24, 2026 12:59

		let rgb = rgba_to_rgb(rgba.as_raw());
		encode_jpeg_rgb(&rgb, rgba.width(), rgba.height(), quality)

-fn baseline_rgba_to_rgb(rgba: &[u8]) -> Vec<u8> {
-    rgba.chunks_exact(4)
-        .flat_map(|px| [px[0], px[1], px[2]])
-        .collect()
-}
+fn baseline_rgba_to_rgb(rgba: &[u8]) -> Vec<u8> {
+    let len = rgba.len() / 4 * 3;
+    let mut rgb = vec![0u8; len];
+    for (src, dst) in rgba.chunks_exact(4).zip(rgb.chunks_exact_mut(3)) {
+        dst[0] = src[0];
+        dst[1] = src[1];
+        dst[2] = src[2];
+    }
+    rgb
+}

Conversation

nikolas-sapa commented Oct 17, 2026 • edited by greptile-apps Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Benchmark Results (Apple Silicon M-series)

Test Plan

Confidence Score: 5/5

Summary

Uh oh!

vercel Bot commented Oct 17, 2026

Uh oh!

greptile-apps Bot Oct 17, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Oct 17, 2026

Choose a reason for hiding this comment

Uh oh!

richiemcilroy commented Oct 18, 2026

Uh oh!

nikolas-sapa commented Oct 19, 2026

Uh oh!

nikolas-sapa commented Oct 19, 2026

Uh oh!

nikolas-sapa commented Oct 19, 2026

Uh oh!

greptile-apps Bot Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

richiemcilroy commented Oct 19, 2026

Uh oh!

nikolas-sapa commented Oct 20, 2026

Uh oh!

richiemcilroy commented Oct 20, 2026

Uh oh!

nikolas-sapa commented Oct 22, 2026

Uh oh!

nikolas-sapa commented Oct 23, 2026

Uh oh!

richiemcilroy commented Oct 23, 2026

Uh oh!

nikolas-sapa commented Oct 24, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nikolas-sapa commented Oct 17, 2026 •

edited by greptile-apps Bot

Loading