feat(egfx): add client-side EGFX with surface management and AVC420 decode

[Greg Lamberson (glamberson)]

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).

[Benoît Cortier (CBenoit)]

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

[Greg Lamberson (glamberson)]

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.

[Greg Lamberson (glamberson)]

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.

[Greg Lamberson (glamberson)]

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)]

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)]

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.

[Copilot]

InclusiveRectangle::width()/height() do unchecked u16 subtraction (right - left + 1), which will wrap if the server sends an invalid rectangle (e.g. left > right / top > bottom). This code only warns on out-of-bounds but still later calls width()/height(), which can lead to huge target_width/height values and excessive allocations (e.g. in crop_decoded_frame). Consider validating rectangle ordering and bounds up-front (return an error for invalid rectangles, and/or clamp/intersect with the surface rectangle) before using width()/height().

[Copilot]

ResetGraphics clears surfaces and resets the decoder, but it leaves frame/queue tracking state (current_frame_id, frames_queued, possibly negotiated_caps/codec_caps) untouched. If a reset occurs mid-stream, subsequent FrameAcknowledge PDUs can report an incorrect queue_depth and the client may appear to be in the middle of a frame. Consider resetting frame-related state in handle_reset_graphics (e.g. current_frame_id = None; frames_queued = 0;).

Suggested change

	// Reset frame/queue tracking and capability negotiation state
	self.current_frame_id = None;
	self.frames_queued = 0;
	self.negotiated_caps = None;
	self.codec_caps = CodecCapabilities::default();

[Copilot]

crop_decoded_frame can return fewer than target_width * target_height * 4 bytes when the decoded frame is smaller than the destination rectangle (it clamps rows/columns via min). However BitmapUpdate docs (and the fields set below) imply the buffer always matches width * height * 4. Consider rejecting frames where frame.width/height are smaller than the destination rectangle (or clamping the reported width/height to the cropped output) so handlers don’t get inconsistent dimensions/buffer lengths.

Suggested change

	let dest_width = dest_rect.width();
	let dest_height = dest_rect.height();
	let cropped_data = crop_decoded_frame(&frame.data, frame.width, frame.height, dest_width, dest_height);
	let update = BitmapUpdate {
	surface_id,
	destination_rectangle: dest_rect.clone(),
	codec_id: Codec1Type::Avc420,
	data: cropped_data,
	width: dest_width,
	height: dest_height,
	let requested_width = dest_rect.width();
	let requested_height = dest_rect.height();
	// Clamp the requested destination size to the decoded frame dimensions
	let effective_width = requested_width.min(frame.width);
	let effective_height = requested_height.min(frame.height);
	if effective_width != requested_width || effective_height != requested_height {
	warn!(
	frame_width = frame.width,
	frame_height = frame.height,
	requested_width,
	requested_height,
	effective_width,
	effective_height,
	"AVC420 destination rectangle larger than decoded frame; clamping update size to frame"
	);
	}
	let cropped_data =
	crop_decoded_frame(&frame.data, frame.width, frame.height, effective_width, effective_height);
	let update = BitmapUpdate {
	surface_id,
	destination_rectangle: dest_rect.clone(),
	codec_id: Codec1Type::Avc420,
	data: cropped_data,
	width: effective_width,
	height: effective_height,

[Copilot]

This test intends to verify AVC capability filtering, but it currently has no assertion that inspects the advertised CapabilitiesAdvertisePdu contents (it only checks messages.len() == 1). As written, it will pass even if filtering regresses. Consider downcasting/decoding the returned DvcMessage to confirm only V8 (or otherwise non-AVC) capability sets are advertised when h264_decoder is None.

Suggested change

	assert_eq!(messages.len(), 1);
	// The default capabilities() returns V10.7, V8.1, V8 — without
	// a decoder, only V8 should survive the filter.
	assert_eq!(messages.len(), 1);
	// Decode the advertised capabilities and ensure only V8 (non-AVC) caps are present.
	// The default capabilities() returns V10.7, V8.1, V8 — without
	// a decoder, only V8 should survive the filter.
	let msg = &messages[0];
	// Extract the raw payload from the DvcMessage and decode it as a CapabilitiesAdvertisePdu.
	let mut cursor = ReadCursor::new(&msg.payload);
	let caps_pdu = CapabilitiesAdvertisePdu::decode(&mut cursor).expect("failed to decode capabilities PDU");
	// We expect exactly one capability set, and it must be V8 (non-AVC).
	assert_eq!(caps_pdu.capability_sets.len(), 1, "expected exactly one capability set when no decoder is present");
	assert!(
	matches!(caps_pdu.capability_sets[0], CapabilitySet::V8(_)),
	"expected only V8 capability set without decoder"
	);

[Copilot]

The module-level docs state “no std dependency”, but the implementation uses Vec, String, and Box without alloc imports, and the crate itself is std-based. This is misleading for consumers expecting no_std compatibility. Consider either updating the docs to reflect the actual requirements, or switching to extern crate alloc + alloc::{vec::Vec, string::String, boxed::Box} and documenting that the module is no_std + alloc friendly.

Suggested change

	//! no `std` dependency, `Send` only.
	//! `Send` only. They are intended for use in `std` environments;
	//! `no_std` + `alloc` support is not currently guaranteed.

[Copilot]

In the Uncompressed path, BitmapUpdate.data is set to pdu.bitmap_data without any pixel-format conversion, but BitmapUpdate’s docs describe the buffer as RGBA. Also, the update does not carry the pixel_format from the PDU, so a handler cannot reliably interpret uncompressed pixels. Consider either (a) converting uncompressed XRgb/ARgb to the declared RGBA format before calling on_bitmap_updated, or (b) adding pixel_format (and documenting per-codec buffer formats) to BitmapUpdate.

Suggested change

	fn handle_uncompressed(&mut self, pdu: crate::pdu::WireToSurface1Pdu) {
	let dest_width = pdu.destination_rectangle.width();
	let dest_height = pdu.destination_rectangle.height();
	let update = BitmapUpdate {
	surface_id: pdu.surface_id,
	destination_rectangle: pdu.destination_rectangle,
	codec_id: Codec1Type::Uncompressed,
	data: pdu.bitmap_data,
	/// Convert uncompressed 32bpp little-endian pixels (XRGB/ARGB) to RGBA8888.
	///
	/// The wire format for uncompressed graphics is typically 0xAARRGGBB in a
	/// 32-bit little-endian word, which corresponds to bytes [B, G, R, A].
	/// This helper reorders bytes into [R, G, B, 0xFF], discarding the source
	/// alpha channel and treating all pixels as fully opaque, which matches
	/// the previous behavior that did not expose alpha to handlers.
	fn convert_uncompressed_to_rgba(src: &[u8]) -> Vec<u8> {
	let mut dst = Vec::with_capacity(src.len());
	for pixel in src.chunks_exact(4) {
	let b = pixel[0];
	let g = pixel[1];
	let r = pixel[2];
	// Ignore source alpha / padding and treat as fully opaque.
	dst.extend_from_slice(&[r, g, b, 0xFF]);
	}
	dst
	}
	fn handle_uncompressed(&mut self, pdu: crate::pdu::WireToSurface1Pdu) {
	let dest_width = pdu.destination_rectangle.width();
	let dest_height = pdu.destination_rectangle.height();
	let rgba_data = GraphicsPipelineClient::convert_uncompressed_to_rgba(&pdu.bitmap_data);
	let update = BitmapUpdate {
	surface_id: pdu.surface_id,
	destination_rectangle: pdu.destination_rectangle,
	codec_id: Codec1Type::Uncompressed,
	data: rgba_data,