Skip to content

feat(egfx): add ClearCodec client-side decode dispatch#1175

Open
Greg Lamberson (glamberson) wants to merge 1 commit into
Devolutions:masterfrom
lamco-admin:feat/clearcodec-client-decode
Open

feat(egfx): add ClearCodec client-side decode dispatch#1175
Greg Lamberson (glamberson) wants to merge 1 commit into
Devolutions:masterfrom
lamco-admin:feat/clearcodec-client-decode

Conversation

@glamberson
Copy link
Copy Markdown
Contributor

@glamberson Greg Lamberson (glamberson) commented Mar 18, 2026

Follow-up to #1174. Supersedes #1195 (the standalone server-helper PR; its 46-line send_clearcodec_frame() is included here).

Wires ClearCodec into the EGFX client's WireToSurface1 codec dispatch,
matching the existing AVC420 and Uncompressed decode patterns.

Changes:

  • ClearCodecDecoder field on GraphicsPipelineClient (always enabled,
    pure Rust, no external codec dependency). Constructed eagerly in
    GraphicsPipelineClient::new(), so V-bar (32K full + 16K short
    entries) and glyph (4000 entries) ring buffers are allocated up
    front regardless of whether ClearCodec frames are subsequently
    received.
  • Codec1Type::ClearCodec arm decodes bitmap data and converts BGRA
    output to RGBA for the uniform BitmapUpdate pixel format
  • Decoder caches (V-bar, glyph) reset on ResetGraphics
  • GraphicsPipelineServer::send_clearcodec_frame() helper for
    symmetric server-side use (mirrors send_avc420_frame /
    send_avc444_frame pattern, including QoE backpressure recording)
  • 3 integration tests (basic decode, RGBA output verification,
    reset/re-decode cycle)
  • 1 unit test for BGRA-to-RGBA channel reordering

Unlike H.264 which requires an external decoder via the H264Decoder
trait, ClearCodec is pure Rust (ironrdp-graphics).

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces ClearCodec support across the stack (PDU parsing, graphics encode/decode, and EGFX client/server integration) and adds a comprehensive test suite to validate correctness and resilience.

Changes:

  • Added ClearCodec PDU parsing modules (residual, bands, subcodec, RLEX) under ironrdp-pdu.
  • Implemented ClearCodecDecoder/ClearCodecEncoder (with glyph/V-bar caching) under ironrdp-graphics.
  • Integrated ClearCodec handling into EGFX client decode path and EGFX server frame queuing; added new tests in testsuite-core.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
crates/ironrdp-testsuite-core/tests/graphics/mod.rs Registers the new ClearCodec graphics test module.
crates/ironrdp-testsuite-core/tests/graphics/clearcodec.rs Adds extensive round-trip and adversarial ClearCodec tests.
crates/ironrdp-testsuite-core/tests/egfx/client.rs Adds EGFX pipeline tests for ClearCodec processing/reset behavior.
crates/ironrdp-pdu/src/codecs/mod.rs Exposes the new ClearCodec codec module in the PDU crate.
crates/ironrdp-pdu/src/codecs/clearcodec/mod.rs Implements ClearCodec bitmap stream + composite payload decoding.
crates/ironrdp-pdu/src/codecs/clearcodec/residual.rs Implements residual layer (BGR RLE) encode/decode helpers.
crates/ironrdp-pdu/src/codecs/clearcodec/bands.rs Implements bands layer decoding (V-bar structures).
crates/ironrdp-pdu/src/codecs/clearcodec/subcodec.rs Implements subcodec layer decoding for regions.
crates/ironrdp-pdu/src/codecs/clearcodec/rlex.rs Implements RLEX subcodec decoding.
crates/ironrdp-graphics/src/lib.rs Exposes ironrdp_graphics::clearcodec.
crates/ironrdp-graphics/src/clearcodec/mod.rs Adds ClearCodec encoder/decoder and decode compositing pipeline.
crates/ironrdp-graphics/src/clearcodec/glyph_cache.rs Implements glyph cache storage for ClearCodec.
crates/ironrdp-graphics/src/clearcodec/vbar_cache.rs Implements V-bar cache storage and reconstruction for bands.
crates/ironrdp-egfx/src/server.rs Adds a send_clearcodec_frame helper to enqueue ClearCodec frames.
crates/ironrdp-egfx/src/client.rs Adds ClearCodec decode path and BGRA→RGBA conversion for bitmap updates.

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

Comment thread crates/ironrdp-egfx/src/server.rs
Comment thread crates/ironrdp-graphics/src/clearcodec/mod.rs
Comment thread crates/ironrdp-graphics/src/clearcodec/mod.rs
Comment thread crates/ironrdp-graphics/src/clearcodec/mod.rs
Comment thread crates/ironrdp-graphics/src/clearcodec/mod.rs Outdated
Comment thread crates/ironrdp-egfx/src/client.rs
Comment thread crates/ironrdp-testsuite-core/tests/egfx/client.rs
Comment thread crates/ironrdp-graphics/src/clearcodec/mod.rs
Comment thread crates/ironrdp-graphics/src/clearcodec/mod.rs
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds client-side ClearCodec decode support to EGFX by wiring a persistent ClearCodec decoder into WireToSurface1 codec dispatch and normalizing output pixels to the existing RGBA BitmapUpdate format.

Changes:

  • Add ClearCodec decode path in GraphicsPipelineClient (including BGRA→RGBA conversion and reset behavior on ResetGraphics).
  • Introduce/extend ClearCodec wire-format parsing in ironrdp-pdu and codec implementation (decoder/encoder + caches) in ironrdp-graphics.
  • Add integration/unit tests covering decode dispatch, pixel format conversion, cache/reset behavior, and adversarial inputs.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
crates/ironrdp-egfx/src/client.rs Adds ClearCodec decoder to client dispatch, resets state on ResetGraphics, and converts BGRA output to RGBA BitmapUpdate.
crates/ironrdp-egfx/src/server.rs Adds send_clearcodec_frame() helper for emitting ClearCodec WireToSurface1 frames.
crates/ironrdp-graphics/src/lib.rs Exposes the new clearcodec module.
crates/ironrdp-graphics/src/clearcodec/mod.rs Implements ClearCodec decoder/encoder and composite layer application (residual/bands/subcodecs).
crates/ironrdp-graphics/src/clearcodec/glyph_cache.rs Introduces glyph cache storage for ClearCodec glyph hits.
crates/ironrdp-graphics/src/clearcodec/vbar_cache.rs Adds V-bar and short V-bar ring-buffer cache implementation used by bands layer.
crates/ironrdp-pdu/src/codecs/mod.rs Exports the ClearCodec wire-format module.
crates/ironrdp-pdu/src/codecs/clearcodec/mod.rs Defines ClearCodec stream/container structures, flags, and composite payload parsing.
crates/ironrdp-pdu/src/codecs/clearcodec/residual.rs Adds residual layer (BGR RLE) decode/encode helpers.
crates/ironrdp-pdu/src/codecs/clearcodec/bands.rs Adds bands layer parsing (V-bar references and inline short V-bar misses).
crates/ironrdp-pdu/src/codecs/clearcodec/subcodec.rs Adds subcodec layer region parsing (raw/NSCodec/RLEX).
crates/ironrdp-pdu/src/codecs/clearcodec/rlex.rs Adds RLEX subcodec decoding (palette + packed run/suite segments).
crates/ironrdp-testsuite-core/tests/graphics/mod.rs Registers the new ClearCodec graphics tests module.
crates/ironrdp-testsuite-core/tests/graphics/clearcodec.rs Adds ClearCodec codec-level integration tests (round-trip, adversarial inputs, cache/session behavior).
crates/ironrdp-testsuite-core/tests/egfx/client.rs Adds EGFX client integration tests for ClearCodec dispatch, RGBA output verification, and reset/re-decode cycle.

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

Comment thread crates/ironrdp-pdu/src/codecs/clearcodec/residual.rs
Comment thread crates/ironrdp-pdu/src/codecs/clearcodec/bands.rs
Comment thread crates/ironrdp-pdu/src/codecs/clearcodec/subcodec.rs
Comment thread crates/ironrdp-pdu/src/codecs/clearcodec/rlex.rs
@glamberson Greg Lamberson (glamberson) force-pushed the feat/clearcodec-client-decode branch 3 times, most recently from a1c0315 to 9611e12 Compare April 1, 2026 15:04
@GlassOnTin
Copy link
Copy Markdown
Contributor

Reviewed against Haven's downstream EGFX decoder (we ship a hand-rolled equivalent) — overall structure looks great, and the loop-decode + per-EndFrame FrameAcknowledge solve the same backpressure issue we hit in our Phase 4 (without those, modern Windows servers throttle after max_unacknowledged_frame_count frames).

One concern in the dimension extraction:

client.rs:732 (and the parallel sites at 770 and 795) computes dest_rect.width() / dest_rect.height() against an InclusiveRectangle. ironrdp_pdu::geometry::InclusiveRectangle::width() returns right - left + 1. But per MS-RDPEGFX 2.2.1.4.1 RDPGFX_RECT16, the right / bottom fields are exclusive — one-past-end, matching FreeRDP's width = right - left. So a 64×64 tile arrives with left=0, right=64 and the current code decodes it as 65×65, which then fails the dimension check on glyph-cache hits and shifts the actual decoded pixels by one column. We hit this exact off-by-one when porting from FreeRDP — the parser type's name and the wire semantics disagree.

Suggestion: either compute right - left directly in these three call sites, or (cleaner) decode the WireToSurface1 destination rectangle into an ExclusiveRectangle since the wire semantics match. Happy to send a fixup commit on top of the branch if useful.

Also: minor — queue_depth: QueueDepth::from_u32(self.frames_queued) is spec-valid (sends an actual depth) but FreeRDP's reference client always sends 0xFFFFFFFF (Unavailable) per MS-RDPEGFX §2.2.3.1, interpreted as "no backlog, please send the next frame". Both work; just flagging in case there's a reason behind the choice.

Tested the decoder against a captured Windows Server 2025 EGFX session — once the right - left + 1 issue is sorted, ClearCodec tiles decode correctly. Haven would adopt the upstream API on merge and drop our hand-rolled egfx/clear.rs.

@glamberson
Copy link
Copy Markdown
Contributor Author

Confirmed and fixed. The audit found exactly the off-by-one you described, plus the test fixtures encoded the same inclusive interpretation, which is why CI was green on the broken behavior. The wire-format-is-exclusive comment now lives at the first call site (decode_avc420) and is referenced from decode_clearcodec and handle_uncompressed.

Changes in this push:

  • client.rs:732,770,795: replaced dest_rect.width()/height() with right - left and bottom - top, with a one-line MS-RDPEGFX 2.2.1.4.1 reference at the first site.
  • client.rs:62: dropped the now-unused Rectangle trait import.
  • client.rs:826: added a one-line note explaining the queue_depth choice (we send actual depth rather than Unavailable/0xFFFFFFFF so the server has real backpressure information; the QoE accumulation in feat(egfx): add QoE statistics accumulation on GraphicsPipelineServer #1167 cares about that signal).
  • tests/egfx/client.rs: updated all eleven test fixtures from inclusive to exclusive coordinates (4x4: right=3,bottom=3 → right=4,bottom=4; 16x16: right=15,bottom=15 → right=16,bottom=16; 2x1: right=1,bottom=0 → right=2,bottom=1). The "invalid ordering" and "out-of-bounds" tests stayed unchanged since both tests pass under either interpretation.

All three xtask gates (fmt, lints, tests) pass on the squashed commit.

Separate from this PR: I'll open a follow-up changing WireToSurface1Pdu.destination_rectangle to ExclusiveRectangle, which is the type-level fix you suggested. It's a breaking change to the ironrdp-egfx public API, so it needs its own audit of all encode/use sites and its own review thread. Pre-1.0 plus CBenoit's "ok with breaking API" stance from #1209 makes it tractable, just out of scope for this PR.

Thanks again for the careful review against Server 2025 captures, this would have shipped wrong without it.

@glamberson
Copy link
Copy Markdown
Contributor Author

Force-pushed 2f8081de -> 0eba4611. Three changes, none affecting the prior 9-thread Copilot review on the codec (all responses remain valid):

  1. Rebased onto amended feat(pdu,graphics,egfx): add ClearCodec bitmap compression codec #1174 head 6e142c37 (was stacked on 43d1f14e, the pre-amend head). This inherits the three hardening fixes I just landed on feat(pdu,graphics,egfx): add ClearCodec bitmap compression codec #1174: bands integer-overflow panic guard, per-dimension cap replacing the per-pixel-count cap, and the explicit BGR-lossless alpha contract documentation. Non-overlapping line ranges in clearcodec/mod.rs so the rebase auto-merged cleanly.

  2. Body updated to call out two items previously not mentioned: the new GraphicsPipelineServer::send_clearcodec_frame() server-side helper (mirrors the send_avc420_frame/send_avc444_frame shape including QoE backpressure recording), and that ClearCodecDecoder is constructed eagerly in GraphicsPipelineClient::new() so V-bar (32K + 16K) and glyph (4000) ring buffers are pre-allocated regardless of whether ClearCodec frames are subsequently received.

  3. Doc comment added at crates/ironrdp-graphics/src/clearcodec/mod.rs on the residual-layer run trimming, making explicit that the trim is the intentional CVE-class CPU-spin guard (per GHSA-32q9-m5qr-9j2v) rather than a missing parse-error path.

Author identity amended to Greg Lamberson <greg@lamco.io> during the rebase. cargo xtask check fmt/lints/tests/typos all pass post-amend (35/35 testsuite-core ClearCodec tests, 33/33 testsuite-core EGFX tests, plus 156K+ fuzz iterations clean on decode + round-trip against the rebased state).

Wire ClearCodec decoder into the EGFX client WireToSurface1 codec
dispatch. Persistent decoder with V-bar and glyph caches is stored
on GraphicsPipelineClient and reset on ResetGraphics. BGRA output
is converted to RGBA for the uniform BitmapUpdate format.

Includes RLEX stop_index bounds validation against palette count
and client-side decode tests in ironrdp-testsuite-core.
@glamberson Greg Lamberson (glamberson) force-pushed the feat/clearcodec-client-decode branch from ced71f2 to dc5bea5 Compare May 28, 2026 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants