Repository navigation
Optional GPU presentation on Wayland (EGL/GLES2) - #434
Merged
Merged
Conversation
Upload the caller's buffer as a texture and let the GPU scale it, so per-frame cost tracks the buffer size rather than the window size. The software scaler is unchanged and still handles everything the GPU path cannot: it is what a failed EGL setup, a missing driver extension, an oversized buffer or a CPU rasteriser such as llvmpipe falls back to. Everything is loaded with dlopen, so GL never becomes a link-time dependency. The context is built on the first update_with_buffer, not at window creation, so a caller driving the wl_surface through raw-window-handle never gets a competing EGL surface on it. Geometry mirrors src/native/posix/scalar.c pixel for pixel, integer truncation and nearest-neighbour sampling offset included, so toggling WindowOptions::use_gpu cannot shift the image. Only Wayland is wired up; X11 still scales in software.
There was a problem hiding this comment.
🟡 Changes recommended
The GL sampler differs from the software scaler, and shader linking fails on GLES2 implementations without fragment high precision.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds optional EGL/GLES2 GPU presentation for Wayland with automatic software fallback.
Changes:
- Adds the
UseGPUwindow option and Wayland GL lifecycle. - Implements dynamic EGL/GLES2 loading, scaling, and tests.
- Updates versioning and release documentation.
File summaries
| File | Description |
|---|---|
src/os/posix/gl.rs |
Implements EGL/GLES2 rendering and tests. |
src/os/posix/wayland.rs |
Integrates GPU presentation and fallback. |
src/os/posix/mod.rs |
Registers the GL module. |
src/lib.rs |
Adds the public GPU option. |
Cargo.toml |
Bumps the crate version. |
README.md |
Updates the dependency version. |
CHANGELOG.md |
Documents the feature and API break. |
.gitignore |
Ignores aider state files. |
Review details
- Files reviewed: 7/8 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The resize loops stepped a 10.10 fixed-point ratio, quantising the step to 1/1024. On a non-integer scale factor that drifts: 320 -> 641 first picks the wrong source column at 513, and 320 -> 3840 misses on 2430 of 3840 columns by up to two pixels. Walk an integer remainder instead, which indexes floor(j * src / dst) exactly with no per-pixel divide. The GPU path maps the exact ratio, so this is also what makes the two paths pixel-identical rather than merely close; the new test pins the C indexing to that formula. Also declare the v_uv varying with matching explicit precision in every stage. GLSL ES 1.00 does not require varying precision to match, but a mediump uv cannot address texels of a texture wider than ~1024, which is what GL_NEAREST needs it to do.
There was a problem hiding this comment.
🟡 Changes recommended
The scaler has severe downscaling regressions, while the GPU path can misrender on limited-precision hardware and mishandle oversized dimensions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/native/posix/scalar.c:96
- This duplicate resize loop has the same unbounded carry cost: a large source scaled into a narrow pillarbox performs roughly
src_widthcarry iterations for every destination row, even though it writes onlydst_widthpixels. Use precomputed quotient/remainder steps here as well so this path remains O(destination pixels).
rem_x += src_width;
while (rem_x >= dst_width) {
rem_x -= dst_width;
sx++;
}
- Files reviewed: 9/10 changed files
- Comments generated: 3
- Review effort level: Balanced
Three fixes from the PR review: The exact-indexing rewrite carried `src` and subtracted `dst` until it fit, which is `src / dst` iterations per pixel -- 220M for the 2.2M -> 100 case the tests cover, against 10k before. Precompute the quotient and remainder so each pixel costs one add and at most one carry. Same output, measured 39ms -> 0.01ms for that blit. Require highp in the fragment language rather than falling back to mediump. mediump guarantees 2^-10 relative precision, which cannot resolve adjacent texels above ~1024 wide, so the fallback sampled the wrong texel on buffers GL_MAX_TEXTURE_SIZE accepts. #error makes the shader ill-formed, so such a driver takes the software path instead. Take the buffer dimensions as usize in present() and convert with try_from. Casting wrapped a width past i32::MAX to a negative one, which slipped under the size check, skipped the upload and reported success -- presenting a cleared window instead of falling back. A zero height lets any width through check_buffer_size, so it was reachable from safe code.
There was a problem hiding this comment.
🟡 Changes recommended
The EGL fallback can pass an incompatible native display pointer, and texture uploads corrupt colors on big-endian targets.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/os/posix/gl.rs:856
- This upload assumes little-endian
u32layout. On a big-endian Linux/BSD target, an API pixel0x00RRGGBBis stored as bytes00 RR GG BB, butGL_BGRA/GL_UNSIGNED_BYTEconsumes those bytes as B, G, R, A, so the default GPU path swaps/corrupts channels (and alpha) while the software path retains the documented bit layout. Either disable this path on big-endian targets or byte-swap/repack pixels before upload.
GL_BGRA,
GL_UNSIGNED_BYTE,
pixels as *const c_void,
- Files reviewed: 9/10 changed files
- Comments generated: 1
- Review effort level: Balanced
platform_display no longer falls back to eglGetDisplay when neither platform entry point resolves. That case is EGL 1.4 without EGL_EXT_platform_base, which has not shipped alongside a working libwayland-egl in a decade, so the fallback only traded a guaranteed software path for a driver that may read the wl_display* as an X11 Display* and fault -- on the path UseGPU::Auto puts everyone by default. Reject big-endian targets. GL_BGRA with GL_UNSIGNED_BYTE consumes the texture as bytes, which matches minifb's 0x00RRGGBB u32 only on little-endian; on big-endian GL reads the padding byte as blue and blue as alpha. GLES2 has no swizzle or packed 8888 format to say it another way, and byte-swapping every frame would spend the CPU pass this path exists to avoid, so those targets take the software scaler.
There was a problem hiding this comment.
🟡 Changes recommended
Large-dimension sampling precision and zero-dimension behavior can diverge from the software path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/10 changed files
- Comments generated: 2
- Review effort level: Balanced
The uv range carried a `0.5 / (src * dst)` nudge meant to land `GL_NEAREST` on the same texel the C scalers index. At real sizes that nudge is under one f32 ULP -- 1080p into a 4K window is already there -- and GLES2 guarantees `highp` only 2^-16 relative precision, a hundred times coarser than the gap it has to resolve. The phase it bought could not be relied on, so drop it and sample the whole texture at fragment centres, which is what the macOS Metal path has always done. The two paths now differ by up to half a destination pixel, and by nothing at all for a 1:1 blit or a whole-number scale. Say so in the `UseGPU` docs and the changelog instead of claiming they match. An empty frame also has to follow the scaler it replaces: `image_resize_linear` returns without writing, leaving the previous frame up, while the other three clear to the background first. Presenting a cleared frame for `Stretch` changed what a window showed depending on `use_gpu`, for a size `check_buffer_size` accepts. The first frame still swaps -- there is nothing to preserve yet, and an unattached Wayland surface is not mapped at all. The sampling test restated `floor(j * src / dst)` instead of calling the scaler, so it could not disagree with the code it checked; it now reads the indices back out of `image_resize_linear` itself, per axis, over sizes up to 4K and 2.2M. Against the pre-90fe901 fixed-point scaler it fails at column 1027 of 2560 -> 3840, which the old test could not see.
There was a problem hiding this comment.
🟡 Changes recommend 8BD0 ed
A rebuilt EGL context incorrectly clears zero-dimension Stretch frames instead of preserving existing Wayland surface content.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/10 changed files
- Comments generated: 1
- Review effort level: Balanced
A zero-dimension buffer under `Stretch` skips the swap whether or not anything has been presented, dropping the bookkeeping that made the first frame an exception.
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.
Uploads the caller's buffer as a texture and lets the GPU scale it, so per-frame cost tracks the buffer size instead of the window size.
WindowOptions::use_gpu:UseGPU::Auto(default) orUseGPU::Disabled.dlopened, so GL never becomes a link-time dependency.update_with_buffer, not at window creation, so a caller driving thewl_surfacethroughraw-window-handlenever gets a competing EGL surface on it.GL_EXT_texture_format_BGRA8888, a buffer pastGL_MAX_TEXTURE_SIZE, or a CPU rasteriser such as llvmpipe.src/native/posix/scalar.cpixel for pixel, integer truncation included, so togglinguse_gpucannot move it within the window. Sampling within that rect is nearest at destination pixel centres, the same as the macOS Metal path: identical to the software scaler for a 1:1 blit and whole-number scales, and up to half a destination pixel later than it otherwise. A test reads the chosen indices back out ofimage_resize_linearitself, per axis, over sizes up to 4K and 2.2M.Also fixes the C scaler itself, which affects X11 and the software path generally: it stepped a 10.10 fixed-point ratio that drifts on non-integer scale factors (320 -> 3840 picked the wrong source column for 2430 of 3840 columns). It now indexes
floor(j * src / dst)exactly.Only Wayland is wired up for GPU presentation; X11 still scales in software. Adding a field to
WindowOptionsis a source break for literals without..WindowOptions::default(), hence the 0.29 bump.Part of #82