feat(activex): present EGFX dirty regions by mamoreau-devolutions · Pull Request #1874 · Devolutions/IronRDP

Marc-André Moreau (mamoreau-devolutions) · 2026-09-01T18:36:50Z

Propagate validated ResetGraphics extents into the session framebuffer and deliver exact packed desktop damage to ActiveX without rebuilding full image snapshots.

Coalesce only fully covered regions, preserve sparse updates with a 64-event and 256 MiB pixel-data budget, and backpressure without evicting accepted frame or static-channel payloads. Update retained GDI and RPC surfaces in place. Reuse framebuffer storage with fallible growth, and retain software cursor shape, position, visibility, and clipping across graphics resets. Keep V8 non-AVC negotiation and full-frame output fallback unchanged. Rejected oversized resets still destroy prior surfaces and are reported as unsupported without allocating a session framebuffer.

Copilot

Pull request overview

Adds dirty-region delivery for ActiveX while retaining full-frame fallback.

Changes:

Propagates validated EGFX reset extents into session framebuffers.
Adds packed desktop updates and retained partial-frame presentation.
Introduces bounded damage queuing and related tests/documentation.

Reviewed changes

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

Show a summary per file

File	Description
`crates/ironrdp-testsuite-extra/tests/client/output_channel.rs`	Tests desktop-update validation.
`crates/ironrdp-session/src/active_stage.rs`	Resizes framebuffers after EGFX resets.
`crates/ironrdp-egfx/src/compositor.rs`	Validates output dimensions and memory limits.
`crates/ironrdp-egfx/src/client.rs`	Reports validated reset extents.
`crates/ironrdp-client/src/rdp.rs`	Adds packed dirty-region delivery.
`crates/ironrdp-activex/src/rpc.rs`	Applies partial updates to retained RPC frames.
`crates/ironrdp-activex/src/control.rs`	Queues, merges, presents, and invalidates dirty regions.
`crates/ironrdp-activex/README.md`	Documents the dirty-region pipeline.

Marc-André Moreau (mamoreau-devolutions) · 2026-09-01T20:27:43Z

The size/XXL warning is acknowledged. The diff has two conceptual review boundaries—EGFX reset/session propagation and ActiveX packed-damage presentation—but they form the end-to-end graphics path validated by this PR. This recovery was explicitly constrained to the existing PR branch and to preserving valid pushed work, so I did not create stacked branches, discard either boundary, or rewrite the existing commits. Both boundaries received separate protocol, implementation, documentation, and skeptical review passes; the retained findings are fixed at 82d445e88.

Note

Human-tuned, LLM-assisted content.

Propagate validated ResetGraphics extents into the session framebuffer and deliver packed desktop damage to ActiveX without rebuilding full image snapshots. Coalesce only fully covered regions, preserve disjoint updates with bounded backpressure, and update retained GDI and RPC surfaces in place. Keep V8 non-AVC negotiation and full-frame output fallback unchanged.

Destroy prior surfaces when a ResetGraphics output exceeds local allocation limits, matching protocol reset semantics. Notify handlers about the rejected output so capture replay retains its unsupported-path classification and fuzz models stay synchronized.

Preserve exact compositor regions so sparse updates do not expand into full bounding-box copies, and cap queued pixel payloads at 256 MiB with backpressure. Retain software cursor shape, position, and visibility when ResetGraphics replaces the session framebuffer.

Invalidate retained RPC frames when a resized desktop exceeds the screenshot budget, and consume the EGFX client's accepted reset state directly during capture replay.

Keep exact frame and static-channel updates ordered when the UI queue is saturated, and make dispatch recovery fail instead of waiting without an outstanding message. Reuse framebuffer storage across graphics resets and clip retained software cursors without source-rectangle underflow.

Integrate partial frame presentation with the modern screen-update suspension contract by snapshotting one bounded full-frame base and applying every suspended dirty region before resume.

Copilot

🟡 Changes recommended

Direct frame dispatch can violate lifecycle ordering, and differing framebuffer extents still evict accepted updates.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 11/11 changed files
Comments generated: 2
Review effort level: Balanced

+        let client = RdpClient::new(config, output_sender).with_desktop_update_handler(move |update| {
+            let _ = queue_worker_event(
+                &desktop_update_events,
+                &desktop_update_posted,
+                HWND(desktop_update_hwnd as *mut c_void),
+                WorkerEvent::Image {


+                if !same_extent {
+                    queue[index] = event;
+                    true


github-actions

The dirty-region pipeline is sound: packed updates are validated, covered-union coalescing preserves ordering, backpressure replaces payload eviction, and pointer-preserving reset is well tested. Two availability questions remain open: queue producers now block the RDP worker indefinitely on a UI-drained queue, and an oversized but protocol-valid ResetGraphics becomes a hard PDU error consumed only by capture-replay. The RPC retained frame can go stale after an extent change, and legacy oversized-frame discards need a reachability decision. One independent question covers damage regions cleared per ActiveStage call while packed delivery consumes them once per iteration. Remaining candidates are valid deduplication cleanups; the PR-scope concern is rejected as non-actionable and already answered.

[skeptical] Producers block the RDP worker thread indefinitely when the STA stops draining the event queue — medium 🟠 ❓ — crates/ironrdp-activex/src/control.rs
queue_worker_event now parks Image, StaticChannelData, and AutoReconnecting producers on space_available with no timeout; the only exits are a UI drain, queue close, or a failed PostMessageW. Producers execute on the RDP worker's current-thread runtime, so a stalled or non-pumping STA wedges transport reads, input encoding, keepalives, and reconnect decisions where payloads were previously dropped or failed fast. The diff does not show that every teardown path reaches WorkerEventQueue::close, so a teardown deadlock cannot be ruled out.
[skeptical] Oversized but protocol-valid ResetGraphics now returns a PDU error that plausibly terminates live sessions — medium 🟠 ❓ — crates/ironrdp-egfx/src/client.rs
handle_reset_graphics returns Err once materializable_output_size rejects the extent (a dimension above 32766 or output above the 256 MiB budget), after clearing surfaces and pending deltas. The new on_reset_graphics_rejected callback is implemented only by capture-replay; no ironrdp-client or ActiveX handler consumes it, so live clients observe a session-failing PDU error where oversized output previously degraded to dropped deltas. Confirm the intended live-client outcome and that no supported deployment negotiates such extents.
[skeptical] Discarded partial update leaves a stale previous-extent frame under new desktop-size properties — low 🟡 — crates/ironrdp-activex/src/rpc.rs
retain_frame_region_with_limit inserts desktopwidth/desktopheight before validation, but the partial-update-without-matching-base path returns without touching live.frame, unlike the oversized path which clears it. After an EGFX reset changes the extent, a partial update arriving before the guaranteed full frame leaves a screenshot-visible frame of the old dimensions advertised with the new desktop size. Clearing live.frame on this discard path keeps the snapshot coherent.
[skeptical] Legacy full-frame updates above the 256 MiB queue budget are discarded with only a warning — low 🟡 ❓ — crates/ironrdp-activex/src/control.rs
The Image arm drops any single update with more than MAX_PENDING_FRAME_PIXELS (~67M) pixels, logging only a warn. EGFX outputs are pre-validated to the same 256 MiB budget, and conformant legacy desktops cap at 8192x8192, which exactly equals the budget and passes the strictly-greater check, so the path may be unreachable defensive code. If larger extents are negotiable through the control, every frame is silently discarded and the display goes stale or black with no session-level signal; confirm reachability and prefer failing or signaling over a per-frame silent discard.

github-actions · 2026-09-10T07:42:16Z

+                if !post_worker_event_dispatch(event_posted, hwnd) {
+                    return false;
+                }
+                queue = match events.space_available.wait(queue) {
+                    Ok(queue) => queue,
+                    Err(poisoned) => poisoned.into_inner(),
+                };
+                if events.closed.load(Ordering::Acquire) {
+                    return false;
+                }


[code-compressor] Backpressure wait block copy-pasted five times in queue_worker_event — medium 🟠 — The sequence post_worker_event_dispatch, space_available.wait with poisoned fallback, and closed check appears verbatim at control.rs 16985-16994, 17032-17041, 17048-17057, 17074-17083, and 17097-17106. A helper returning None on failed dispatch or queue close collapses ~40 lines and keeps wake, poison, and close semantics in one place; five independent copies are where a future partial edit becomes a real concurrency bug.

github-actions · 2026-09-10T07:42:16Z

+fn pack_desktop_update(
+    image: &DecodedImage,
+    width: NonZeroU16,
+    height: NonZeroU16,
+    region: InclusiveRectangle,
+) -> SessionResult<DesktopUpdate> {
+    let region_width = region
+        .right
+        .checked_sub(region.left)
+        .and_then(|width| width.checked_add(1))
+        .ok_or_else(|| ironrdp_session::general_err!("invalid desktop update horizontal bounds"))?;
+    let region_height = region
+        .bottom
+        .checked_sub(region.top)
+        .and_then(|height| height.checked_add(1))
+        .ok_or_else(|| ironrdp_session::general_err!("invalid desktop update vertical bounds"))?;
+    if region.right >= width.get() || region.bottom >= height.get() {
+        return Err(ironrdp_session::general_err!(
+            "desktop update exceeds framebuffer bounds"
+        ));
+    }
+
+    let pixel_count = usize::from(region_width)
+        .checked_mul(usize::from(region_height))
+        .ok_or_else(|| ironrdp_session::general_err!("desktop update pixel count overflow"))?;
+    let mut buffer = Vec::new();
+    buffer
+        .try_reserve_exact(pixel_count)
+        .map_err(|_| ironrdp_session::general_err!("allocate desktop update buffer"))?;
+
+    let source_width = NonZeroUsize::from(width).get();
+    let left = usize::from(region.left);
+    let row_pixel_count = usize::from(region_width);
+    for y in region.top..=region.bottom {
+        let pixel_offset = usize::from(y)
+            .checked_mul(source_width)
+            .and_then(|offset| offset.checked_add(left))
+            .ok_or_else(|| ironrdp_session::general_err!("desktop update source offset overflow"))?;
+        let byte_offset = pixel_offset
+            .checked_mul(4)
+            .ok_or_else(|| ironrdp_session::general_err!("desktop update byte offset overflow"))?;
+        let byte_len = row_pixel_count
+            .checked_mul(4)
+            .ok_or_else(|| ironrdp_session::general_err!("desktop update row length overflow"))?;
+        let byte_end = byte_offset
+            .checked_add(byte_len)
+            .ok_or_else(|| ironrdp_session::general_err!("desktop update row end overflow"))?;
+        let row = image
+            .data()
+            .get(byte_offset..byte_end)
+            .ok_or_else(|| ironrdp_session::general_err!("desktop update source row is out of bounds"))?;
+        buffer.extend(row.chunks_exact(4).map(|pixel| {
+            let r = pixel[0];
+            let g = pixel[1];
+            let b = pixel[2];
+            u32::from_be_bytes([0, r, g, b])
+        }));
+    }
+
+    DesktopUpdate::new(buffer, width, height, region)
+        .ok_or_else(|| ironrdp_session::general_err!("packed desktop update is inconsistent"))
+}


[code-compressor] pack_desktop_update re-validates invariants DesktopUpdate::new re-checks, leaving a dead error branch — low 🟡 — pack_desktop_update validates region dimensions, framebuffer bounds, and pixel count, then hands the same values to DesktopUpdate::new, which repeats every check; the final 'packed desktop update is inconsistent' error is unreachable because the row loop appends exactly pixel_count pixels. A crate-internal constructor used by pack_desktop_update, with DesktopUpdate::new remaining the public validator, removes the duplicated checks and the misleading dead path.

github-actions · 2026-09-10T07:42:16Z

+    fn is_full_frame(&self) -> bool {
+        self.region.left == 0
+            && self.region.top == 0
+            && self.region.right.checked_add(1) == Some(self.width)
+            && self.region.bottom.checked_add(1) == Some(self.height)
+    }


[code-compressor] Full-frame predicate and covering-rectangle construction duplicated across three crates — low 🟡 — The covers-whole-framebuffer test is written in FrameUpdate::is_full_frame, inline in ActiveXRpc::retain_frame_region_with_limit, and the InclusiveRectangle{0,0,w-1,h-1} literal is rebuilt in FrameUpdate::full, PresentationSurface::copy_from, and rdp.rs. A covering(width, height) helper plus one shared predicate keeps the inclusive-coordinate convention in sync in a single place instead of five expressions that must stay in sync.

github-actions · 2026-09-10T07:42:17Z

+        let Some(region_width) = region
+            .right
+            .checked_sub(region.left)
+            .and_then(|value| value.checked_add(1))
+        else {
+            tracing::warn!(?region, "Discarding RPC frame update with invalid horizontal bounds");
+            return;
+        };
+        let Some(region_height) = region
+            .bottom
+            .checked_sub(region.top)
+            .and_then(|value| value.checked_add(1))
+        else {
+            tracing::warn!(?region, "Discarding RPC frame update with invalid vertical bounds");
+            return;
+        };
+        let Some(pixel_count) = usize::from(width).checked_mul(usize::from(height)) else {
+            tracing::warn!(width, height, "Discarding RPC frame update with an overflowing extent");
+            return;
+        };
+        let Some(update_pixel_count) = usize::from(region_width).checked_mul(usize::from(region_height)) else {
+            tracing::warn!(?region, "Discarding RPC frame update with an overflowing region");
+            return;
+        };
+        if width == 0
+            || height == 0
+            || region.right >= width
+            || region.bottom >= height
+            || pixels.len() != update_pixel_count
+        {
+            tracing::warn!(width, height, ?region, "Discarding inconsistent RPC frame update");
+            return;
+        }


[code-compressor] retain_frame_region_with_limit re-derives validation FrameUpdate::new already enforced — low 🟡 — The only production caller passes values from a FrameUpdate that already passed region-dimension, bounds, and buffer-length checks, yet rpc.rs 289-321 recomputes them in five warn-and-return blocks before the row copy. Accepting the validated update, or sharing one validation helper, removes ~30 redundant per-frame lines while keeping the production pixel-limit check and the test-only full-frame wrapper.

github-actions · 2026-09-10T07:42:17Z

+        self.data.resize(len, 0);
+        self.data.fill(0);


[code-compressor] reset_preserving_pointer zeroes the framebuffer twice — low 🟡 — data.resize(len, 0) zero-fills only the grown tail, then data.fill(0) rewrites the entire buffer, so pre-existing bytes are written twice on every ResetGraphics, including framebuffers near the 256 MiB budget. clear() followed by resize(len, 0) yields the identical all-zero buffer in one pass and preserves capacity, so the regression test's unchanged data pointer assertion still holds.

github-actions · 2026-09-10T07:42:17Z

+        let mut desktop_damage_regions = active_stage.take_damage_regions();
+        let mut desktop_damage_delivered = false;


[general] Packed damage delivery can drop regions when several ActiveStage calls share one iteration — low 🟡 ❓ — ActiveStage clears damage_regions at the start of every process and process_fastpath_input call, while take_damage_regions runs once per iteration and the packed handler delivers only on the first GraphicsUpdate via desktop_damage_delivered. If input processing and frame processing both contribute GraphicsUpdate outputs to one iteration, the earlier call's exact regions were cleared and are never delivered, leaving those pixels stale in the packed consumer until an overlapping update. The fallback union path is unaffected. Whether the client loop batches multiple ActiveStage calls per iteration could not be verified from the provided evidence.

Copilot AI balanced review requested due to automatic review settings September 1, 2026 18:36

Marc-André Moreau (mamoreau-devolutions) temporarily deployed to llm-providers September 1, 2026 18:37 — with GitHub Actions Inactive

Copilot started reviewing on behalf of Marc-André Moreau (mamoreau-devolutions) September 1, 2026 18:37 View session

Copilot AI reviewed Sep 1, 2026

View reviewed changes

Comment thread crates/ironrdp-activex/src/control.rs

Comment thread crates/ironrdp-client/src/rdp.rs Outdated

Comment thread crates/ironrdp-session/src/active_stage.rs Outdated

Marc-André Moreau (mamoreau-devolutions) temporarily deployed to llm-providers September 1, 2026 18:54 — with GitHub Actions Inactive

Marc-André Moreau (mamoreau-devolutions) force-pushed the copilot/activex-accelerated-graphics branch from 18e915a to 36a08cb Compare September 2, 2026 15:15

Marc-André Moreau (mamoreau-devolutions) added 6 commits September 2, 2026 15:30

fix(activex): clear stale graphics state

be89bda

Invalidate retained RPC frames when a resized desktop exceeds the screenshot budget, and consume the EGFX client's accepted reset state directly during capture replay.

fix(activex): retain suspended damage regions

4091362

Integrate partial frame presentation with the modern screen-update suspension contract by snapshotting one bounded full-frame base and applying every suspended dirty region before resume.

Marc-André Moreau (mamoreau-devolutions) force-pushed the copilot/activex-accelerated-graphics branch from 36a08cb to 4091362 Compare September 2, 2026 19:35

Marc-André Moreau (mamoreau-devolutions) requested a balanced review from Copilot September 2, 2026 20:50

Copilot started reviewing on behalf of Marc-André Moreau (mamoreau-devolutions) September 2, 2026 20:50 View session

Benoît Cortier (CBenoit) added the ai-review/allow-oversized Allows normal automated review of an oversized pull request label Sep 2, 2026

Benoît Cortier (CBenoit) temporarily deployed to llm-providers September 2, 2026 20:52 — with GitHub Actions Inactive

Copilot AI reviewed Sep 2, 2026

View reviewed changes

Benoît Cortier (CBenoit) added ai-review/allow-oversized Allows normal automated review of an oversized pull request and removed ai-review/allow-oversized Allows normal automated review of an oversized pull request labels Sep 3, 2026

Benoît Cortier (CBenoit) temporarily deployed to llm-providers September 3, 2026 17:48 — with GitHub Actions Inactive

Benoît Cortier (CBenoit) added ai-reviewed/1 One automated review completed ai-review/allow-oversized Allows normal automated review of an oversized pull request and removed ai-review/allow-oversized Allows normal automated review of an oversized pull request labels Sep 3, 2026

Benoît Cortier (CBenoit) temporarily deployed to llm-providers September 3, 2026 19:59 — with GitHub Actions Inactive

Benoît Cortier (CBenoit) added ai-review/allow-oversized Allows normal automated review of an oversized pull request and removed ai-review/allow-oversized Allows normal automated review of an oversized pull request labels Sep 3, 2026

Benoît Cortier (CBenoit) temporarily deployed to llm-providers September 4, 2026 01:28 — with GitHub Actions Inactive

github-actions Bot removed the breaking-change Includes a breaking change, and requires special scrutiny at the boundaries label Sep 4, 2026

Benoît Cortier (CBenoit) added ai-review/allow-oversized Allows normal automated review of an oversized pull request and removed ai-review/allow-oversized Allows normal automated review of an oversized pull request labels Sep 4, 2026

Benoît Cortier (CBenoit) temporarily deployed to llm-providers September 4, 2026 15:58 — with GitHub Actions Inactive

github-actions Bot reviewed Sep 10, 2026

View reviewed changes

github-actions Bot added ai-reviewed/2 Final automated review completed and removed ai-reviewed/1 One automated review completed labels Sep 10, 2026

Navigation Menu

Uh oh!

feat(activex): present EGFX dirty regions - #1874

feat(activex): present EGFX dirty regions#1874
Marc-André Moreau (mamoreau-devolutions) wants to merge 6 commits into
masterfrom
copilot/activex-accelerated-graphics

Marc-André Moreau (mamoreau-devolutions) commented Sep 1, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Marc-André Moreau (mamoreau-devolutions) commented Sep 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

github-actions Bot left a comment

Uh oh!

github-actions Bot Sep 10, 2026

Uh oh!

github-actions Bot Sep 10, 2026

Uh oh!

github-actions Bot Sep 10, 2026

Uh oh!

github-actions Bot Sep 10, 2026

Uh oh!

github-actions Bot Sep 10, 2026

Uh oh!

github-actions Bot Sep 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

		let mut desktop_damage_regions = active_stage.take_damage_regions();
		let mut desktop_damage_delivered = false;

Uh oh!

Conversation

Marc-André Moreau (mamoreau-devolutions) commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Marc-André Moreau (mamoreau-devolutions) commented Sep 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Marc-André Moreau (mamoreau-devolutions) commented Sep 1, 2026 •

edited

Loading