feat(egfx): add client-side EGFX with surface management and AVC420 decode by glamberson · Pull Request #1103 · Devolutions/IronRDP

Greg Lamberson (glamberson) · 2026-02-16T23:03:38Z

Replaces the 74-line trace-only EGFX client stub with a working client implementation that handles surface management, H.264 AVC420 decode, and frame acknowledgment.

New files:

decode.rs: H264Decoder trait, DecodedFrame, DecoderError. Core tier (no I/O, Send only). Consumers bring their own decoder implementation.
client.rs: Full rewrite. GraphicsPipelineClient with DvcProcessor integration.

Protocol compliance (MS-RDPEGFX):

Surface lifecycle: CreateSurface, DeleteSurface, MapSurfaceToOutput, ResetGraphics [3.3.1.6]
FrameAcknowledge sent after every EndFrame with queue depth tracking [3.3.5.12]
WireToSurface1 dispatch by codec_id [3.3.5.2]
AVC420 decode via RFX_AVC420_BITMAP_STREAM [2.2.4.4]
Capability advertisement on DVC channel start [3.3.5.1]

The H264Decoder trait accepts AVC-format NAL units (4-byte BE length prefix) from the bitmap stream and returns RGBA pixel data. Frame cropping handles H.264 macroblock alignment (e.g., 1920x1088 decoded to fit 1920x1080 destination).

Unhandled PDUs (SolidFill, SurfaceToSurface, cache operations, AVC444) are forwarded to GraphicsPipelineHandler::on_unhandled_pdu() for application-level handling.

No new dependencies. No breaking changes to existing public API beyond the expanded GraphicsPipelineClient::new() signature (now takes an optional H264Decoder).

Adds an optional OpenH264-backed implementation of the H264Decoder trait introduced in Devolutions#1103. - AVC-to-Annex B NAL unit conversion (OpenH264 requires start code format) - YUV420p to RGBA color space conversion - Checked arithmetic on NAL lengths to reject malicious input - Feature-gated: `openh264 = ["dep:openh264"]`, off by default 4 tests using the encoder to generate valid AVC bitstreams for round-trip validation.

Adds an optional `openh264` feature that provides a concrete H264Decoder implementation using Cisco's OpenH264 library, loaded dynamically at runtime via libloading for patent compliance. Depends on Devolutions#1103 which introduces the H264Decoder trait and client core. The decoder handles the format conversion required by the RDP protocol: AVC-format NAL units (4-byte BE length prefix per [2.2.4.4]) are converted to Annex B start codes before passing to OpenH264, then YUV420p output is converted to RGBA for the client pipeline. Patent compliance: - Default `openh264` feature uses `libloading` (Cisco binary, patent-covered) - Separate `openh264-source` feature for dev/testing only (no patent coverage) - Runtime path configuration via `OpenH264DecoderConfig` - System path search for well-known library locations - `OPENH264_ATTRIBUTION` constant for license condition 3 - `OPENH264-LICENSING.md` documents compliance requirements New optional dependency: - `openh264 = { version = "0.6", default-features = false, features = ["libloading"] }` - Feature: `openh264 = ["dep:openh264"]` - Feature: `openh264-source = ["dep:openh264", "openh264/source"]` (dev only) 4 tests generate valid H.264 bitstreams via the OpenH264 encoder, convert to AVC format, and decode through the full pipeline to verify round-trip correctness. Tests require `--features openh264-source`. Without the feature enabled, all existing tests continue to pass unchanged.

Benoît Cortier (CBenoit) · 2026-02-18T07:24:12Z

Heads-up: this is taking me more time than I initially thought, but I made progress.

Greg Lamberson (glamberson) · 2026-02-18T08:40:51Z

No problem at all. This is a big one — client-side EGFX with surface management and AVC420 decode has a lot of moving parts. Take the time you need to study it properly. I'd rather have thorough feedback than a rushed review.

Adds an optional OpenH264-backed implementation of the H264Decoder trait introduced in Devolutions#1103. - AVC-to-Annex B NAL unit conversion (OpenH264 requires start code format) - YUV420p to RGBA color space conversion - Checked arithmetic on NAL lengths to reject malicious input - Feature-gated: `openh264 = ["dep:openh264"]`, off by default 4 tests using the encoder to generate valid AVC bitstreams for round-trip validation.

Greg Lamberson (glamberson) · 2026-03-10T22:55:09Z

Updated: rebased + capability gating + complete PDU dispatch

Rebased onto current master (resolved conflict from edition 2024 migration).

Capability gating fix: DvcProcessor::start() now filters AVC-capable capability sets when no H264Decoder is provided. Previously, the client would advertise V10.7/V8.1 AVC support regardless, causing the server to send H.264 frames that would be silently dropped. With no decoder, only non-AVC sets (like V8) are advertised.

Complete server→client PDU dispatch: Added 11 dedicated handler trait methods for all remaining server→client PDUs that were falling through to on_unhandled_pdu():

Surface operations: on_solid_fill, on_surface_to_surface
Cache operations: on_surface_to_cache, on_cache_to_surface, on_evict_cache_entry, on_cache_import_reply
Surface mapping: on_map_surface_to_window, on_map_surface_to_scaled_output, on_map_surface_to_scaled_window
Progressive codec: on_wire_to_surface2, on_delete_encoding_context

All methods have default no-op implementations — no breaking changes for existing handler implementations. Each includes MS-RDPEGFX spec section references.

…ecode Add comprehensive client-side EGFX implementation with: - Surface tracking per MS-RDPEGFX 3.3.1.6 - Capability negotiation with AVC gating (V8/V8.1/V10.7) - H.264 AVC420 decode via pluggable H264Decoder trait - Frame acknowledgment per MS-RDPEGFX 3.3.5.12 - Macroblock-aligned frame cropping - Complete server→client PDU dispatch (21 handler methods) - Codec capability extraction from all 11 capability versions Capability gating: when no H264Decoder is provided, AVC-capable capability sets are automatically filtered from the advertisement. This prevents the server from sending H.264 frames that would be silently dropped.

Adds an optional OpenH264-backed implementation of the H264Decoder trait introduced in Devolutions#1103. - AVC-to-Annex B NAL unit conversion (OpenH264 requires start code format) - YUV420p to RGBA color space conversion - Checked arithmetic on NAL lengths to reject malicious input - Warn on malformed AVC NAL units instead of silent discard - Checked arithmetic for RGBA buffer allocation - Zero-dimension surface validation in CreateSurface handler - Truncation warning in crop_decoded_frame - Feature-gated: openh264 = ["dep:openh264"], off by default 4 tests using the encoder to generate valid AVC bitstreams for round-trip validation.

Greg Lamberson (glamberson) · 2026-03-11T00:37:11Z

I have to admit that before I really wasn't focusing on all the use cases for this implementaton. I was jsut trying to plug a hole. But as I've started doing betas of my multicodec development and also looking at crates I hadn't been using before (ironrdp-web), i started to see the mess here that needs cleaning up. I hope these changes start to get this shaped into what you expect as we move forward with egfx. Thanks.

Greg Lamberson (glamberson) · 2026-03-11T22:12:01Z

Hey Benoît Cortier (@CBenoit), this is rebased and all CI green. Could you review it? This is really critical for everything I'm doing now. I know you're busy, but this really blocks a ton of progress. Thanks!

Benoît Cortier (CBenoit) · 2026-03-12T15:30:43Z

Oh no… I remember leaving a feedback comment here, but I don’t see it; I probably messed something up. I’ll re-evaluate the PR.

EDIT: Also launching a Copilot run for early feedback.

Copilot

Pull request overview

This PR replaces the previous trace-only EGFX client stub with a functional client-side implementation that negotiates capabilities, tracks surfaces, decodes AVC420 via a pluggable H.264 decoder trait, and sends required frame acknowledgements.

Changes:

Added a new decode module defining H264Decoder, DecodedFrame, and DecoderError for pluggable AVC decode.
Rewrote the EGFX client to handle core MS-RDPEGFX PDUs (cap negotiation, ResetGraphics, surface lifecycle, WireToSurface1 dispatch, EndFrame → FrameAcknowledge).
Added client-side unit tests and a frame-cropping helper for macroblock-aligned decode output.

Reviewed changes

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

File	Description
crates/ironrdp-egfx/src/lib.rs	Exposes the new `decode` module from the crate root.
crates/ironrdp-egfx/src/decode.rs	Introduces a decoder abstraction layer (trait + frame/error types) for client-side AVC decode.
crates/ironrdp-egfx/src/client.rs	Implements the EGFX client state machine, surface tracking, AVC420 decode dispatch, and FrameAcknowledge generation.

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

You can also share your feedback on Copilot code review. Take the survey.

- Validate destination rectangle ordering before computing dimensions - Reset frame tracking state on ResetGraphics (preserving cap state) - Reject undersized decoded frames in AVC420 path - Assert cap set contents in capability advertisement tests - Clarify std requirement in decode module docs - Convert BGRX wire format to RGBA in uncompressed bitmap path

Adds an optional OpenH264-backed implementation of the H264Decoder trait introduced in Devolutions#1103. - AVC-to-Annex B NAL unit conversion (OpenH264 requires start code format) - YUV420p to RGBA color space conversion - Checked arithmetic on NAL lengths to reject malicious input - Warn on malformed AVC NAL units instead of silent discard - Checked arithmetic for RGBA buffer allocation - Zero-dimension surface validation in CreateSurface handler - Truncation warning in crop_decoded_frame - Feature-gated: openh264 = ["dep:openh264"], off by default 4 tests using the encoder to generate valid AVC bitstreams for round-trip validation.

Greg Lamberson (glamberson) · 2026-03-12T20:49:17Z

OK I think this is ready for review now. Thank you.

Greg Lamberson (glamberson) · 2026-03-16T18:18:56Z

Well, there's no reason to sit on moving these tests which can be moved...

I moved the public-API tests to the testsuite (egfx/client.rs). They go through process() with ZGFX-wrapped payloads now instead of calling handle_pdu() directly.

I kept a few unit tests inline for things that need private access: State transitions, frame tracking after reset, crop_decoded_frame, convert_uncompressed_to_rgba. Same approach as fast_path.rs and display.rs.

I also added some new coverage while I was at it as I'm focused on close, restart, and error conditions anyway:

Surface lifecycle through process() (create, delete, reset)
Error paths (unknown surface, bad rectangle ordering, out-of-bounds)
Multi-frame tracking, close transition

Now it's 5 inline + 16 testsuite = 21 total tests, up from 17. All xtask checks green.

Move public-API tests to ironrdp-testsuite-core/tests/egfx/client.rs, exercising the DVC process() path with ZGFX-wrapped payloads. Keep private-access unit tests inline (state transitions, frame tracking, crop/convert helpers). New coverage: surface lifecycle, error paths (unknown surface, invalid rectangle, out-of-bounds tolerance), multi-frame tracking, close. 5 inline + 16 testsuite = 21 total (was 17 inline-only).

Adds an optional OpenH264-backed implementation of the H264Decoder trait introduced in Devolutions#1103. - AVC-to-Annex B NAL unit conversion (OpenH264 requires start code format) - YUV420p to RGBA color space conversion - Checked arithmetic on NAL lengths to reject malicious input - Warn on malformed AVC NAL units instead of silent discard - Checked arithmetic for RGBA buffer allocation - Zero-dimension surface validation in CreateSurface handler - Truncation warning in crop_decoded_frame - Feature-gated: openh264 = ["dep:openh264"], off by default 4 tests using the encoder to generate valid AVC bitstreams for round-trip validation.

Benoît Cortier (CBenoit)

Thank you! All my main concerns are gone now.

I still left a few extra comments below, but I think this can be follow up PRs. I’ll merge this PR now because I made you wait for too long already, and I think it’s already very solid.

LGTM!

Benoît Cortier (CBenoit) · 2026-03-17T04:23:47Z

+    fn handle_create_surface(&mut self, surface_id: u16, width: u16, height: u16, pixel_format: PixelFormat) {
+        let surface = Surface {
+            id: surface_id,
+            width,
+            height,
+            pixel_format,
+            is_mapped: false,
+            output_origin_x: 0,
+            output_origin_y: 0,
+        };
+
+        debug!(surface_id, width, height, ?pixel_format, "Surface created");
+        self.handler.on_surface_created(&surface);
+        self.surfaces.insert(surface_id, surface);
+    }


question: FreeRDP defensively handles CreateSurface on an already-used surface ID by attempting a delete first before recreating the surface. Here we overwrite the map entry directly.

create surface PDU sometimes happens for surface ID that are already in use and have not
been removed yet. Ensure that there is no surface with the new ID by trying to remove it
manually.

https://github.com/Devolutions/FreeRDP/blob/1a40f8fdb778a5b8d92c3c6d59290cc94e44d081/channels/rdpgfx/client/rdpgfx_main.c#L1021

I’m sure you tested and the current code interoperates correctly, but what do you think? Should we match FreeRDP?

Benoît Cortier (CBenoit) · 2026-03-17T04:34:20Z

+        // Per spec, ResetGraphics implicitly destroys all surfaces
+        self.surfaces.clear();


Can you quote the part of the spec about this? Is it MS-RDPEGFX §3.3.5.14 (Processing an RDPGFX_RESET_GRAPHICS_PDU message)?

The structure and fields of the RDPGFX_RESET_GRAPHICS_PDU message are specified in section 2.2.2.14. The header field MUST be processed as specified in section 3.1.5.1. Once the RDPGFX_RESET_GRAPHICS_PDU message has been successfully decoded, the client MUST resize the Graphics Output Buffer (section 3.3.1.7) ADM element.

I don’t understand where it says all surfaces should be destroyed. Can you double check that?

Incidentally, FreeRDP in the rdpgfx_recv_reset_graphics_pdu function fires the ResetGraphics callback and does nothing to the surface table: https://github.com/Devolutions/FreeRDP/blob/1a40f8fdb778a5b8d92c3c6d59290cc94e44d081/channels/rdpgfx/client/rdpgfx_main.c#L510-L604

Benoît Cortier (CBenoit) · 2026-03-17T04:38:06Z

+//! H.264 data arrives inside [RFX_AVC420_BITMAP_STREAM][1] payloads
+//! within `RDPGFX_WIRE_TO_SURFACE_PDU_1` messages. The NAL units
+//! are in AVC format (4-byte big-endian length prefix per NAL unit),
+//! not Annex B (start code prefix).


This documentation seems to be backwards?

As per MS-RDPEGFX §2.2.4.4 (RFX_AVC420_BITMAP_STREAM):

The RFX_AVC420_BITMAP_STREAM structure encapsulates regions of a graphics frame compressed using the MPEG-4 AVC/H.264 codec in YUV420p mode (as specified in [ITU-H.264-201201]) and conforming to the byte stream format specified in [ITU-H.264-201201] Annex B.

The H.264 data uses Annex B start codes (0x00 0x00 0x00 0x01), not AVCC length prefixes. Any downstream decoder implementation built on this comment will be wrong. The test data in client_dispatches_avc420_via_process uses Annex B bytes ([0x00, 0x00, 0x00, 0x01, 0x67]), which seems actually correct.

Can you double check?

Benoît Cortier (CBenoit) · 2026-03-17T04:45:00Z

+    fn decode_avc420(&mut self, surface_id: u16, dest_rect: &InclusiveRectangle, bitmap_data: &[u8]) -> PduResult<()> {
+        let mut cursor = ReadCursor::new(bitmap_data);
+        let stream = Avc420BitmapStream::decode(&mut cursor).map_err(|e| decode_err!(e))?;
+
+        let Some(ref mut decoder) = self.h264_decoder else {
+            debug!("No H.264 decoder configured, skipping AVC420 frame");
+            return Ok(());
+        };
+
+        let frame = decoder
+            .decode(stream.data)
+            .map_err(|e| pdu_other_err!("H.264 decode", source: e))?;


I don’t see rectangles array handling (carried by Avc420BitmapStream).

For the common single-region case this is probably harmless, but for a multi-region frame (the server sends multiple region rectangles and quant values covering non-contiguous areas of the decoded frame), the current code applies the wrong single destination rect and ignores the actual region metadata.

Cross checking with FreeRDP: FreeRDP passes the full H264_METABLOCK (with all region rects and quant quality values) to the codec layer.

This won't be observable until a server sends multi-region AVC420 frames, but it's worth at least a // TODO: multi-region AVC420 support comment.

Can be a follow up PR if you prefer.

Adds an optional OpenH264-backed implementation of the H264Decoder trait introduced in Devolutions#1103. - AVC-to-Annex B NAL unit conversion (OpenH264 requires start code format) - YUV420p to RGBA color space conversion - Checked arithmetic on NAL lengths to reject malicious input - Warn on malformed AVC NAL units instead of silent discard - Checked arithmetic for RGBA buffer allocation - Zero-dimension surface validation in CreateSurface handler - Truncation warning in crop_decoded_frame - Feature-gated: openh264 = ["dep:openh264"], off by default 4 tests using the encoder to generate valid AVC bitstreams for round-trip validation.

Greg Lamberson (glamberson) mentioned this pull request Feb 16, 2026

feat(egfx): add openh264 feature-gated H.264 decoder #1104

Merged

Greg Lamberson (glamberson) force-pushed the feat/egfx-client-core branch from 4213016 to de1cb0a Compare February 16, 2026 23:20

Lamco Development Office Staff (lamco-office) force-pushed the feat/egfx-client-core branch from de1cb0a to 77804a5 Compare February 22, 2026 01:48

Greg Lamberson (glamberson) mentioned this pull request Feb 26, 2026

Add GFX ([MS-RDPEGFX]) support #427

Closed

Marco Köpcke (theCapypara) mentioned this pull request Mar 5, 2026

GNOME Remote Desktop not supported theCapypara/field-monitor#53

Open

Greg Lamberson (glamberson) force-pushed the feat/egfx-client-core branch from 77804a5 to 63afce1 Compare March 10, 2026 22:54

Greg Lamberson (glamberson) force-pushed the feat/egfx-client-core branch from 63afce1 to 28af4bf Compare March 10, 2026 22:59

Greg Lamberson (glamberson) force-pushed the feat/egfx-client-core branch from 28af4bf to 8f62974 Compare March 10, 2026 23:06

Benoît Cortier (CBenoit) requested a review from Copilot March 12, 2026 15:34

Copilot started reviewing on behalf of Benoît Cortier (CBenoit) March 12, 2026 15:35 View session

Copilot AI reviewed Mar 12, 2026

View reviewed changes

Greg Lamberson (glamberson) mentioned this pull request Mar 13, 2026

ironrdp-web becomes unresponsive when remote framebuffer updated rapidly #790

Open

Benoît Cortier (CBenoit) approved these changes Mar 17, 2026

View reviewed changes

Benoît Cortier (CBenoit) merged commit b6200c7 into Devolutions:master Mar 17, 2026
10 checks passed

Greg Lamberson (glamberson) mentioned this pull request Mar 17, 2026

fix(acceptor): skip credential check when server credentials are None #1150

Merged

Greg Lamberson (glamberson) deleted the feat/egfx-client-core branch March 17, 2026 12:12

This was referenced Apr 26, 2026

ironrdp-egfx: planned timeline for crates.io publish? #1229

Open

RDP error GlassHaven/Haven#109

Open

		// Per spec, ResetGraphics implicitly destroys all surfaces
		self.surfaces.clear();

Conversation

Greg Lamberson (glamberson) commented Feb 16, 2026

Uh oh!

Benoît Cortier (CBenoit) commented Feb 18, 2026

Uh oh!

Greg Lamberson (glamberson) commented Feb 18, 2026

Uh oh!

Greg Lamberson (glamberson) commented Mar 10, 2026

Uh oh!

Greg Lamberson (glamberson) commented Mar 11, 2026

Uh oh!

Greg Lamberson (glamberson) commented Mar 11, 2026

Uh oh!

Benoît Cortier (CBenoit) commented Mar 12, 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!

Uh oh!

Uh oh!

Uh oh!

Greg Lamberson (glamberson) commented Mar 12, 2026

Uh oh!

Greg Lamberson (glamberson) commented Mar 16, 2026

Uh oh!

Benoît Cortier (CBenoit) left a comment

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Benoît Cortier (CBenoit) commented Mar 12, 2026 •

edited

Loading