feat(egfx): add MS-RDPEGFX Graphics Pipeline Extension #1057
Conversation
Add complete MS-RDPEGFX implementation with PDU types and server logic. PDU layer (based on work by @elmarco in Devolutions#648): - All 23 RDPGFX PDUs (WireToSurface, CreateSurface, ResetGraphics, etc.) - Capability sets V8 through V10.7 - AVC420/AVC444 bitmap stream codecs - Timestamp, QuantQuality, and supporting types Server implementation: - Multi-surface management (Offscreen Surfaces ADM element) - Frame tracking with flow control (Unacknowledged Frames ADM element) - V8/V8.1/V10/V10.1-V10.7 capability negotiation - AVC420 and AVC444 frame sending - QoE metrics processing - Cache import handling - Resize coordination - Backpressure via client queue depth Client-side DVC processor (from Devolutions#648): - Basic message processing scaffolding Credits: @elmarco for PDU definitions and protocol research in PR Devolutions#648.
Remove erroneous reserved field from FIXED_PART_SIZE. Per MS-RDPEGFX 2.2.2.23, this PDU has no reserved field (unlike MapSurfaceToScaledOutputPdu).
glamberson
force-pushed
the
egfx-server-complete
branch
from
9b7cf29 to
b9fd104
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a complete MS-RDPEGFX (Graphics Pipeline Extension) implementation to IronRDP, enabling H.264 AVC420/AVC444 video streaming over RDP. The implementation includes:
- PDU layer: All 23 RDPGFX PDU types with encode/decode support, capability sets V8-V10.7, and AVC420/AVC444 codecs
- Server implementation: Multi-surface management, frame tracking with flow control, capability negotiation, AVC frame transmission, QoE metrics, cache import, resize handling, and backpressure management
- Client scaffolding: Basic DVC processor with capability advertisement and PDU handling hooks
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ironrdp-egfx/src/server.rs | Complete server-side implementation with surface manager, frame tracker, capability negotiation, and AVC420/AVC444 frame sending |
| crates/ironrdp-egfx/src/pdu/cmd.rs | All 23 RDPGFX PDU definitions with encode/decode implementations |
| crates/ironrdp-egfx/src/pdu/avc.rs | AVC-specific utilities including Annex B to AVC conversion, region handling, and bitmap stream encoding |
| crates/ironrdp-egfx/src/pdu/common.rs | Common PDU types (Point, Color, PixelFormat) |
| crates/ironrdp-egfx/src/pdu/mod.rs | PDU module organization and exports |
| crates/ironrdp-egfx/src/client.rs | Client DVC processor with decompression and capability advertisement |
| crates/ironrdp-egfx/src/lib.rs | Crate root with module declarations |
| crates/ironrdp-egfx/Cargo.toml | Package configuration with dependencies |
| crates/ironrdp-egfx/CHANGELOG.md | Initial changelog for v0.1.0 |
| crates/ironrdp-egfx/README.md | Crate documentation |
| Cargo.lock | Workspace lock file update |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Remove redundant code in annex_b_to_avc() function. The previous logic: - Set i = start + (end - start), which simplifies to i = end - Then conditionally set i = end again (always redundant) Now simply: i = end with a clarifying comment. Addresses Copilot review feedback on PR Devolutions#1057. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove redundant code in annex_b_to_avc() function. The previous logic: - Set i = start + (end - start), which simplifies to i = end - Then conditionally set i = end again (always redundant) Now simply: i = end with a clarifying comment. Addresses Copilot review feedback on PR Devolutions#1057.
glamberson
force-pushed
the
egfx-server-complete
branch
from
e8db418 to
cc6eb7e
Compare
CBenoit
left a comment
CBenoit
left a comment
There was a problem hiding this comment.
This is looking good overall, although I didn’t review everything yet. Do you think you could feed Claude with the STYLE.md file and see whether it updates the code in a sensible way?
|
OK sure I'll go ahead and work on it. Thanks. OK actually this isn't as bad as I worried it was. Working on these different code divisions has me regularly confused, but thsi is done mostly correctly. So I'll push my updates right away as they're small. I've got this work that (potentially like this PR) I want to submit here, and then I've got my own open source crates, then I've got 2 products, one of which is non-commercial use only glue as an RDP server, then I've got my compositor/VDI/headless product , so I have different API surfaces and rules whichI thought I messed up here on that font, but it looks ok. Thanks again. And regarding style, there are really no significant discrepancies in regard to what's published. I'm glad to hear any comments you have regardless. |
- Use shrink_to pattern for decompressed buffer memory management - Change CHANNEL_NAME to pub(crate) visibility - Move tests to ironrdp-testsuite-core using only public API
I actually have a new thing that I'm working on. I literally threw this all together to get it to you, but let me make it a little more sensible and understandable. I'll push another commit after finishing. Stay tuned. |
server.rs: - Rename SurfaceManager to Surfaces for directness - Remove redundant comments that restate code - Add strategic comments explaining protocol requirements - Reduce excessive debug logging to critical points only - Rename variables for domain clarity (encoded_stream, target_rect) - Use idiomatic ? operator instead of verbose let...else pdu/cmd.rs, pdu/common.rs: - Fix 27 rustdoc warnings for bare URLs - Convert to proper markdown link references All tests pass, zero clippy warnings, zero rustdoc warnings.
3 participants
Complete MS-RDPEGFX implementation with PDU types and server logic. Supercedes #648.
Summary
Credits
PDU definitions and protocol research from @elmarco in #648.
Test plan