Repository navigation
perf(desktop-gpui): convert screenshot editor frames in one pass and skip stale ones - #2836
Merged
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This comment has been minimized.
This comment has been minimized.
Member
Author
On-device macOS check (real hardware)Machine: The three byte-identity tests ( Conversion alone (release
|
| frame | base | head | speedup |
|---|---|---|---|
| 1080p | 1.54 ms | 0.52 ms | 2.96x |
| 4K | 6.50 ms | 2.17 ms | 3.00x |
| 5K | 11.93 ms | 3.93 ms | 3.04x |
| 5K at padding 40 (6760x3802) | 22.03 ms | 7.08 ms | 3.11x |
Padding slider drag, end to end on Metal (dev-profile test build, benchmark_padding_slider_drag, 120 ticks at 60 Hz, median of 3 alternating rounds)
| still | converted of 120 base / head | CPU / tick base / head | convert median base / head | MB written / edit base / head | median edit-to-frame base / head |
|---|---|---|---|---|---|
| 1920x1080 | 120 / 120 | 5.45 / 4.08 ms | 3.27 / 0.92 ms | 33.6 / 11.2 | 6.4 / 5.4 ms |
| 3840x2160 | 120 / 120 | 18.14 / 8.83 ms | 12.12 / 2.93 ms | 134.5 / 44.8 | 21.0 / 11.5 ms |
| 5120x2880 | 78 / 119 | 24.07 / 15.56 ms | 24.57 / 5.20 ms | 151.1 / 78.9 | 113.6 / 18.6 ms |
Reading it:
- The conversion speedup is the same as on the CI VMs (about 3x), with lower absolute times than the M5 Max VM.
- The real GPU renders much faster than the VM, so conversion becomes the bottleneck on base. At 5K, base converts only 72 to 94 of the 120 edits per round and the median edit-to-frame latency is 114 ms. Head keeps up (119 of 120) at 18.6 ms. At 4K, latency drops from 21.0 to 11.5 ms and process CPU per tick halves.
- The drain still dropped nothing on head (rendered equals converted in every round), so the latency win comes from the cheaper conversion, not from skipping frames.
richiemcilroy
marked this pull request as ready for review
October 22, 2026 06:22
Member
Author
|
@greptileai review |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
What
Renderer-input edits in the screenshot editor (padding, rounding, shadow, border, background, crop, aspect) re-render the full-resolution still, and every frame that came back was converted three times on the CPU before it reached the atlas:
unpad_rowsinto a tight RGBA copy (kept for the mask overlays),rgba.clone()into a second full-size buffer,swap_red_blueover that clone for gpui's BGRA atlas.Frames were also converted strictly in order, so a frame already superseded by a newer one in
frame_rxwas still copied, swizzled and uploaded.This PR:
cap_gpui_kernels::frame::unpad_rgba_to_bgra, through the video editor's existingeditor_window::frame_image. One read of the padded GPU buffer, one write, one allocation.RenderImage::as_bytes(0)), and the per-overlayswap_red_bluegoes away because the output is already in atlas order.mask_filter(both the pixelate copy and the twoTriangleresizes inimage0.25.10) treats every channel identically and independently, so filtering BGRA and skipping the swap is byte-identical to filtering RGBA and swapping.frame_rxto the newest frame before converting.Not changed: preview resolution, texture allocation, annotation/mask coordinate spaces, export, copy to clipboard and save (those still go through
render_export_rgbaand RGBAmasked_region_image).Byte-identical output
New unit tests:
the_displayed_frame_matches_unpadding_then_swapping_a_copy:frame_imageequals the oldunpad_rows+ clone +swap_red_bluepath for odd widths (1, 3, 7, 33, 1001) and tight, +4 and 256-aligned strides.mask_overlays_from_bgra_match_filtering_rgba_then_swapping: blur (levels 4/9/16/33) and pixelate (2/7/20) overlays on a 37x23 frame with a 256-byte stride and varying alpha are byte-identical between the old path (RGBA filter, thenrgba_to_render_image) and the new one.only_the_newest_queued_frame_is_converted.Measured
Scratch branch
ci/screenshot-copies-ab(runs 39609307696, 39611730829), on Blacksmith runners.Conversion alone (release
cargo bench, 256-byte stride, 40 samples, A/B alternating in one binary)Base is
unpad_rows+ clone +swap_red_blue. Head isunpad_rgba_to_bgra. Median per frame:CPU bytes written per converted frame go from 3x to 1x the tight frame size (5K: 176.9 MB to 59.0 MB; 5K at padding 40: 308.4 MB to 102.8 MB). Allocations per frame go from two full frames to one, and the window keeps one full-size copy of the frame resident instead of two (59 MB less at 5K).
Padding slider drag, end to end (real
run_still_renderer+ frame pump, 120 ticks at 60 Hz, padding 0 to 40 and back, median of 3 alternating rounds)Dev-profile test build, same harness as
benchmark_annotation_slider_drag. The Windows runner has no GPU adapter, so this part is macOS (Metal) and Linux (lavapipe) only.On real hardware (Apple M4 Max, 128 GB, on-device comment) the conversion speedup is the same 3x (5K: 11.93 to 3.93 ms). The drag benefits more than on the VM because the real GPU renders faster and conversion becomes base's bottleneck: at 5K base converts only 78 of 120 edits with a 113.6 ms median edit-to-frame, while head converts 119 at 18.6 ms. At 4K edit-to-frame goes from 21.0 to 11.5 ms.
On Linux, process CPU is dominated by lavapipe rasterizing on the CPU, so the conversion saving is a small share of it there.
The drain did not drop any frames in these runs: the renderer was always slower than the conversion, so a second frame never queued. The drain is a guard for when conversion or main-thread handoff falls behind (busy machine, slow atlas upload), and it costs one uncontended channel lock per frame.
The main-thread atlas upload (
replace_regionof the full frame) is unchanged and not covered by these numbers.Interaction with #2703
#2703 (screenshot editor startup) touches the same files but none of the same lines:
view.frame_arrived(image, rgba, image_size, ...)for its instant preview. This PR removes thergbaargument, so whichever lands second gets a compile error rather than a silent channel mismatch. The fix on that side is to drop the argument (and thesource.rgba.clone()that feeds it): its preview image comes fromrgba_to_render_image, which is BGRA, so masks read correct pixels from it.revision >= view.revisioncheck in the frame loop composes with the drain here: the drain makes the frame it checks the newest one, so fewer frames are converted only to be discarded.

Confidence Score: 5/5
This PR appears safe to merge.Summary
The screenshot editor now converts each frame once and shares its BGRA bytes with mask overlays.
No actionable issues found. Tests and the native UI were not run.
Reviews (1) · Last reviewed commit: "perf(desktop-gpui): convert screenshot e..." · Reviewed by Greptile