Skip to content

feat(egfx): composite client surface commands into pixel buffers - #1460

Merged
Benoît Cortier (CBenoit) merged 1 commit into
Devolutions:masterfrom
lamco-admin:feat/egfx-client-compositor
Jul 31, 2026
Merged

feat(egfx): composite client surface commands into pixel buffers#1460
Benoît Cortier (CBenoit) merged 1 commit into
Devolutions:masterfrom
lamco-admin:feat/egfx-client-compositor

Conversation

@glamberson

@glamberson Greg Lamberson (glamberson) commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • GraphicsPipelineClient decoded WireToSurface1 bitmaps but forwarded SolidFill, SurfaceToSurface, the bitmap cache, and MapSurfaceToOutput to no-op handler callbacks, so it never held the pixel state those commands operate on. Any server that uses them (grd, Windows) renders wrong.
  • Adds a surface compositor to ironrdp-egfx: persistent RGBA8888 per-surface buffers plus a bitmap cache, applies every surface command, maps surfaces to the output, and accumulates the changed output regions.
  • Regions are committed per EndFrame and drained via a new GraphicsPipelineClient::drain_output() -> Vec<OutputUpdate>.
  • Additive: existing GraphicsPipelineHandler callbacks are retained as notifications, so current consumers are unaffected.

Resource bounds

Clipping bounds a single access, not the total, and surfaces, cache slots and per-PDU rectangle counts are all keyed or counted by u16. Three amplification paths followed from that, all now closed:

  • Total pixel bytes are capped at 256 MiB across surfaces, cache and pending output. An allocation past the cap is refused, so the surface or slot simply does not exist and commands against it become no-ops.
  • Output deltas record regions during a frame and copy pixels once at EndFrame. A SolidFill naming 65,535 rectangles previously retained a full pixel copy for each; it now costs one copy per distinct region, and repeated paints of one region collapse before reaching the copy. Those copies are charged against the same cap.
  • CacheToSurface no longer clones the cached tile to satisfy borrowing, removing a full-tile copy and an uncharged peak from every invocation.

ResetGraphics now discards committed-but-undrained deltas along with the surfaces that produced them. A payload can carry EndFrame and ResetGraphics together, and those deltas were clipped against the previous output, so painting them into the new one repaints stale pixels and, after a shrink, addresses a region the new output no longer contains.

Reading pixels at commit also means a delta carries the frame's final state, so a region painted twice within a frame yields one update rather than two the consumer would paint over each other.

Validation

  • cargo xtask check fmt/lints/tests/typos/locks all pass.
  • 23 compositor unit tests: solid fill, frame atomicity, cache round-trip, oversized-destination clipping, source-rect clamping, reset, the surface/cache/queue budgets with their release and slot-replacement paths, region deferral, and the repeat filter.

Notes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds persistent EGFX surface composition and frame-scoped output updates.

Changes:

  • Composites bitmap, fill, copy, cache, and mapping commands.
  • Exposes completed regions through drain_output().
  • Adds compositor unit tests.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File Description
crates/ironrdp-egfx/src/lib.rs Exposes the compositor module.
crates/ironrdp-egfx/src/compositor.rs Implements surface composition, caching, output updates, and tests.
crates/ironrdp-egfx/src/client.rs Integrates composition into EGFX PDU processing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/ironrdp-egfx/src/compositor.rs Outdated
Comment thread crates/ironrdp-egfx/src/compositor.rs
Comment thread crates/ironrdp-egfx/src/compositor.rs
Comment thread crates/ironrdp-egfx/src/compositor.rs
Comment thread crates/ironrdp-egfx/src/compositor.rs Outdated
The client GraphicsPipelineClient decoded WireToSurface1 bitmaps and
forwarded every other surface command (SolidFill, SurfaceToSurface, the
bitmap cache, MapSurfaceToOutput) to no-op handler callbacks, so it never
maintained the pixel state those commands operate on. Against any server
that uses them (GNOME Remote Desktop, Windows) the rendered result was
wrong.

Add a surface compositor that keeps persistent RGBA8888 surfaces and a
bitmap cache, applies each command into them, maps surfaces onto the
graphics output, and accumulates the changed output regions per frame.
The regions are exposed through GraphicsPipelineClient::drain_output(),
committed on EndFrame so a partial frame is never observed. Reads and
writes are clipped to their buffers, so a malformed server rectangle
shrinks the update to empty rather than reading or writing out of range.

Clipping bounds a single access, not the total. Surfaces and cache slots
are both keyed by u16, so the compositor also caps the pixel bytes it
holds at once and refuses allocations that would exceed it. Output deltas
record regions during a frame and copy pixels once at EndFrame, which
keeps a PDU carrying 65,535 rectangles to one copy per distinct region,
and those copies are charged against the same cap. A ResetGraphics drops
pending deltas along with the surfaces that produced them, since their
regions describe the previous output.

The existing handler callbacks are kept as secondary notifications, so
current consumers are unaffected.
@github-actions github-actions Bot added size/XL Size: Above 800 lines of code and removed size/L Size: 400-800 lines of code labels Jul 31, 2026

#[cfg(test)]
mod tests {
use super::*;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (non-blocking): My usual nitpick about tests living outside of the testsuite crates; but I don’t want to block this PR on that now. Just informational.

@CBenoit Benoît Cortier (CBenoit) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-virtual-channel Area: Static or dynamic virtual channel rust Pull requests that update Rust code size/XL Size: Above 800 lines of code

Development

Successfully merging this pull request may close these issues.

3 participants