feat(graphics,egfx): add progressive RFX decode and EGFX integration#1197
Conversation
Greg Lamberson (glamberson)
force-pushed
the
feat/progressive-rfx-decode
branch
5 times, most recently
from
ebcdf5d to
8c18a5d
Compare
|
The
One minor observation: WBT_TILE_FIRST tiles with Tested against a captured Server 2025 session: the three Progressive PDUs we have on disk decoded clean. Happy to share the captures + a smoke-test bin if it'd help. |
|
Thanks for the careful review and especially for confirming the On the BTreeMap / On RFX_TILE_DIFFERENCE: confirmed gap. The PDU layer parses the flags byte on TileSimple and TileFirst, but the graphics decoder unconditionally sets |
Greg Lamberson (glamberson)
force-pushed
the
feat/progressive-rfx-decode
branch
from
8c18a5d to
a108923
Compare
Add progressive RemoteFX decode pipeline and wire it into the EGFX client and server: ironrdp-graphics: Progressive decode algorithms (first-pass and upgrade-pass coefficient reconstruction via SRL + inverse DWT), tile state tracking for multi-pass refinement, surface-level tile management with automatic quality progression. ironrdp-egfx: WireToSurface2 dispatch for progressive codec in GraphicsPipelineClient. Progressive surface state stored on the server for multi-pass frame scheduling.
The merged progressive primitives PR (Devolutions#1196) added validation that a ProgressiveRegion must contain at least one rectangle. The test used an empty rects vec which now fails decoding.
Greg Lamberson (glamberson)
force-pushed
the
feat/progressive-rfx-decode
branch
from
a108923 to
1f22378
Compare
|
Full pre-review pass on the Progressive RFX stack. Three force-pushes, ordering tables added to all three bodies, fuzz infrastructure built on a separate branch for follow-up. Rebased onto current master (20 days behind → fresh)
Each step independently reviewable; intended merge order is PR-number order per the "Stack ordering" table now consistent across all three bodies. Author identity normalized to `Greg Lamberson greg@lamco.io` on every commit (matches the rest of my upstream contributions). Rebase fix surfaced on #1198Master moved during the 20-day window and exposed a missing `InclusiveRectangle` import in #1198's `MixedTilePayload::ClearCodec` variant. The variant declared `destination: InclusiveRectangle` but pushed into `WireToSurface1Pdu.destination_rectangle` which is `ExclusiveRectangle` per MS-RDPEGFX 2.2.1.4.1. Corrected the enum variant to use `ExclusiveRectangle` directly; `#1198` body has the post-rebase note. RFX_TILE_DIFFERENCE deferred per original scoping (#1240)Restated explicitly in #1197's body. The decoder parses the `flags` byte but does not consume `flags & 0x01` (delta-mode tile per MS-RDPRFX 3.1.8.1.7.1). Per #1240's "Scope" section, this is intentionally tracked as a follow-up pending Server 2025 captures from GlassOnTin that exercise the difference path. Synthetic test fixtures do not cover it. Fuzz infrastructure (separate branch)Two fuzz oracles built on `lamco-admin/IronRDP:precheck-fuzz-progressive` (rooted on the rebased #1199 head). To be filed as a small follow-up PR after the stack lands:
A round-trip oracle (encode → decode pixel match) is filed as a skeleton; full implementation needs a stable in-process helper that wraps `encode_first_pass` output as TILE_SIMPLE PDU bytes for re-feeding through `decode_bitmap`. Tracked as a follow-up. A differential vs FreeRDP 3.15.0 `progressive_decompress` was scoped but deferred for this PR pass: `progressive_decompress` requires multi-surface `PROGRESSIVE_CONTEXT` lifecycle + `REGION16` interaction, which is a larger harness extension than the ClearCodec differential. Will be filed under #1120 once the harness is ready. Verification post-rebasePer-PR, on each new head:
|
Address precheck findings on the Progressive RFX decode pipeline.
Cap per-axis surface dimensions at MAX_SURFACE_DIM = 32768 (rounded up
from the MS-RDPEGFX 2.2.2.14 normative ceiling of 32766 px) and return
ProgressiveDecodeError::SurfaceTooLarge for inputs above the cap. The
cap rejects nothing the spec considers conformant; it bounds the
backing Vec<Option<Box<TileState>>> at 512 * 512 = 262144 slots
= 2 MiB of pointer storage per surface before any tile is populated.
Reject streams missing the SYNC + CONTEXT prefix instead of silently
defaulting use_reduce_extrapolate = false, which would decode malformed
input under the wrong band layout. Per MS-RDPEGFX 2.2.4.2 every
Progressive stream MUST begin with SYNC + CONTEXT.
Move the redundant `extern crate alloc` declaration to its conventional
top-of-file location and import BTreeMap and Entry directly.
Document the RawBitReader past-end-of-stream zero-extension behavior at
the type level (matches the FreeRDP reference implementation's
tolerance for short truncation in this upgrade path).
Use saturating_add on TileState::pass to prevent theoretical wraparound
across upgrade-pass overflow.
Revert four incidental import-reorder changes (ironrdp-dvc/{client,lib}
and ironrdp-server/{lib,server}.rs) that drifted in from a local
nightly rustfmt run; both states pass cargo xtask check fmt because the
workspace's imports_granularity and group_imports settings are
nightly-only and the pinned stable rustfmt skips them with warnings.
These four files are outside this PR's stated graphics + egfx scope.
Adds one test (surface_tiles_rejects_over_cap_dimensions) covering the
new at-cap and over-cap behavior on each axis.
|
Per improved internal prechecks I'm running on my PRs, took the following tightening actions on this stack (#1197 / #1198 / #1199):
Stack heads after the precheck-driven amend + clean rebase chain: #1197 |
…encode invariants Address precheck findings on the Progressive RFX encode pipeline. Mark `MixedTilePayload` as `#[non_exhaustive]` so future EGFX codec additions (Avc444, hardware-accelerated paths, future Microsoft codecs) land without a SemVer break for downstream consumers that pattern-match on the enum. Matches the workspace convention used for analogous codec enums in `ironrdp-egfx/src/pdu/cmd.rs`. Document the monotonic-refinement contract on `encode_upgrade_pass` per MS-RDPRFX 3.1.8.1.7.2: the `saturating_sub` on `raw_mag` is deliberate because upgrade passes only add magnitude bits; the decoder accumulates with DAS-determined sign, so signed deltas have no place in the wire format. Future maintainers reading the code should not "fix" this. Document the i16 truncation on the zero-DAS SRL delta: SRL stream values are i16 by wire-format definition, so the `clamp_i16` is the wire boundary rather than a precision compromise. Document the `count <= 32` invariant on `RawBitWriter::write_bits` at the type level and add a `debug_assert` to catch misuse. Mirrors the counterpart documentation added on `RawBitReader` in the Devolutions#1197 amend.
There was a problem hiding this comment.
I’ll be honest, I don’t have the bandwidth to review this properly now, but I think we left the PR sit for long enough now, and you performed a self review yourself, so we may as well merge now and fix issues later if necessary! It will be easier for you to move forward that way.
Benoît Cortier (CBenoit)
merged commit a142799
into
Devolutions:master
…encode invariants Address precheck findings on the Progressive RFX encode pipeline. Mark `MixedTilePayload` as `#[non_exhaustive]` so future EGFX codec additions (Avc444, hardware-accelerated paths, future Microsoft codecs) land without a SemVer break for downstream consumers that pattern-match on the enum. Matches the workspace convention used for analogous codec enums in `ironrdp-egfx/src/pdu/cmd.rs`. Document the monotonic-refinement contract on `encode_upgrade_pass` per MS-RDPRFX 3.1.8.1.7.2: the `saturating_sub` on `raw_mag` is deliberate because upgrade passes only add magnitude bits; the decoder accumulates with DAS-determined sign, so signed deltas have no place in the wire format. Future maintainers reading the code should not "fix" this. Document the i16 truncation on the zero-DAS SRL delta: SRL stream values are i16 by wire-format definition, so the `clamp_i16` is the wire boundary rather than a precision compromise. Document the `count <= 32` invariant on `RawBitWriter::write_bits` at the type level and add a `debug_assert` to catch misuse. Mirrors the counterpart documentation added on `RawBitReader` in the Devolutions#1197 amend.
3 participants
Depends on #1196 (merged).
Depended on by #1198 (server-side encode) and #1199 (round-trip tests). Part of the multi-codec EGFX implementation described in #1158 (Section 4).
Summary
Add the Progressive RemoteFX decode pipeline (algorithm, tile state, surface management, EGFX-WireToSurface2 dispatch) and a server-side queue helper that wraps pre-encoded progressive bytes into a
WireToSurface2Pdufor transmission. Actual progressive encoding lives in #1198.ironrdp-graphics: Progressive decode algorithms (first-pass and upgrade-pass coefficient reconstruction via SRL + inverse DWT), tile state tracking for multi-pass refinement, surface-level tile management with automatic quality progression, and a per-axis surface-dimension cap matching the MS-RDPEGFX 2.2.2.14 normative ceiling.
ironrdp-egfx:
WireToSurface2dispatch for progressive codec in GraphicsPipelineClient. Server-side queue helper (send_remotefx_progressive_frame) for pre-encoded progressive payloads, used by #1198's encode side.Stack ordering
This is step 1 of 3. The full chain (must merge in PR-number order):
Branch is rebased onto current master. Each subsequent PR is stacked on its predecessor.
Surface-dimension cap
SurfaceTiles::newrejects per-axis dimensions aboveMAX_SURFACE_DIM = 32768and returnsProgressiveDecodeError::SurfaceTooLarge. The cap is the next power of two above the MS-RDPEGFX 2.2.2.14 normative ceiling (32766 px), so every spec-conformant surface is accepted. At the cap, the backingVec<Option<Box<TileState>>>is bounded to 512 × 512 = 262144 slots = 2 MiB of pointer storage per surface before any tile is populated.Concurrent-surface bounding (total per-connection budget across N attacker-controllable surfaces) is deferred to a follow-up. Per-surface bounding is necessary but not sufficient against a misbehaving server that creates many surfaces; the spec does not cap concurrent surface count.
Known limitation: RFX_TILE_DIFFERENCE deferred
@GlassOnTinsurfaced during review that this PR's decoder parses theflagsbyte onTileSimple/TileFirstbut does not consume theRFX_TILE_DIFFERENCEflag (flags & 0x01). Difference-encoded tiles (per MS-RDPRFX 3.1.8.1.7.1) carry coefficient deltas relative to the previously-decoded tile at the same(quantIdx, x, y)slot. This PR lands the non-difference path only; the difference path is intentionally tracked in #1240 pending real-world captures (Server 2025 emits difference-encoded tiles under common progressive refinement conditions; synthetic test fixtures do not exercise this path).Changes
4 files, ~1483 lines (
progressive.rsis the bulk).Test plan
cargo xtask check fmt/lints/tests/typos/locksall pass on the current head. Coverage includes the at-cap and over-cap rejection paths.