Skip to content

fix(graphics): honour RFX_TILE_DIFFERENCE in Progressive decode - #1701

Open
Yevhen Salitrynskyi (ysalitrynskyi) wants to merge 4 commits into
Devolutions:masterfrom
ysalitrynskyi:fix/progressive-rfx-tile-difference
Open

fix(graphics): honour RFX_TILE_DIFFERENCE in Progressive decode#1701
Yevhen Salitrynskyi (ysalitrynskyi) wants to merge 4 commits into
Devolutions:masterfrom
ysalitrynskyi:fix/progressive-rfx-tile-difference

Conversation

@ysalitrynskyi

@ysalitrynskyi Yevhen Salitrynskyi (ysalitrynskyi) commented Aug 18, 2026

Copy link
Copy Markdown

Problem

The Progressive RemoteFX decoder parses the tile flags byte on TILE_SIMPLE and TILE_FIRST blocks, but nothing ever consumes it. TileState::decode_first unconditionally set self.is_difference = false, and no coefficient path read that field, so a tile flagged RFX_TILE_DIFFERENCE (0x01) was decoded as if it carried absolute coefficients.

The delta is then rendered as if it were the image, and every later progressive upgrade pass refines the wrong coefficients, so the error compounds across the frame.

Spec

MS-RDPEGFX sections 2.2.4.2.1.5.3 (RFX_PROGRESSIVE_TILE_SIMPLE) and 2.2.4.2.1.5.4 (RFX_PROGRESSIVE_TILE_FIRST) define the flag:

RFX_TILE_DIFFERENCE 0x01 — Indicates that the tile contains the compressed difference of the DWT coefficients for the same tile between the current frame and the previous frame.

Section 3.3.8.2.1.1 gives the decode rule:

If the tile is a difference tile (section 3.1.8.1.2), then the progressively quantized coefficients are simply added to the DecDwtQ elements:
DecDwtQ = DecDwtQ + DecProgQ * PQF

and section 3.3.8.2.1 states the complementary rule for original tiles — the tile is zeroed in the current frame first, and the entropy decode result is added to it.

The same section requires the LL3 deltas to be summed up "even if the tile is not an original tile", so the LL3 differential decode stays unconditional. FreeRDP's progressive_decompress behaves the same way (progressive_rfx_dwt_2d_decode adds the freshly decoded buffer into the persisted current buffer, saturating at the INT16 bounds, when coeffDiff is set).

Changes

ironrdp-pducrates/ironrdp-pdu/src/codecs/rfx/progressive.rs

  • Add FLAG_TILE_DIFFERENCE and is_difference() accessors on TileSimple and TileFirst, following the existing FLAG_DWT_REDUCE_EXTRAPOLATE / uses_reduce_extrapolate() pattern. The seven high bits of the flags field remain reserved and ignored, as the spec requires.

ironrdp-graphicscrates/ironrdp-graphics/src/progressive.rs

  • Add TileState::decode_first_difference, which decodes the three components into a scratch buffer and accumulates them into the retained coefficients, saturating at the i16 bounds. The accumulation happens in the DecDwtQ domain — before base dequantization and the inverse DWT — which is where both the retained coefficients and the incoming differences live.
  • decode_tile_block routes TILE_SIMPLE and TILE_FIRST to that path when the flag is set, and to the existing path otherwise.
  • The shared first-pass bookkeeping moves into a private begin_first_pass, and is_difference now records how the most recent first-pass tile was encoded instead of being hardcoded to false.

Notes on the semantics:

  • DAS sign state keeps describing the incoming differences rather than the accumulated result, because that is what the upgrade passes refining this transmission are encoded against. This matches both the spec (section 3.3.8.2.1.1 derives the tri-state from DecProgQ) and FreeRDP, which fills its sign buffer from the freshly RLGR-decoded data regardless of the flag.
  • Unusable reference: the retained coefficients stay base-quantized until reconstruction, so they only mean anything while the base quantization tables and the DWT variant hold. If a stream changes either, the reference is dropped and the tile decodes as an original one. Without that, a difference tile carrying an all-zero delta corrupted most of the tile. The spec does not define the case; degrading one tile beats erroring out the whole frame.
  • First tile of a surface: a tile that has not been decoded yet holds zeroed coefficients, so a difference tile arriving first reconstructs to the same values as an absolute one. No special case is needed, and there is a test for it.
  • Upgrade passes: TILE_UPGRADE has no flags field on the wire, and it already accumulates into the retained coefficients, so it is unaffected. FreeRDP reads tile->flags in its upgrade path but the value is unused there, since it copies the persisted buffer into the DWT buffer regardless.

No public API break. decode_first keeps its signature and its behaviour for original tiles; the difference path is a new method.

No new dependencies.

Tests

ironrdp-graphics:

  • difference_tile_accumulates_into_retained_coefficients — an absolute tile followed by a flagged one; asserts the full 4096-coefficient buffer of all three components equals retained + newly decoded, and that the DAS state describes the incoming differences.
  • non_difference_tile_replaces_retained_coefficients — two absolute tiles in a row still replace, and the result is identical to decoding the second one on a fresh tile.
  • difference_tile_on_untouched_tile_matches_absolute_tile — first tile of a surface.
  • difference_tile_accumulation_saturates — accumulation clamps at the i16 bounds.
  • upgrade_pass_refines_accumulated_difference_coefficients — an upgrade pass after a difference tile refines the accumulated coefficients.
  • decoder_accumulates_difference_tiles_signalled_on_the_wire — end-to-end through ProgressiveDecoder::decode_bitmap, driven by the real flags byte: a flagged tile renders pixel-identical to a single absolute tile carrying the summed coefficients, an unflagged one renders identically to decoding it standalone, and the two differ.

ironrdp-pdu: tile_difference_flag_survives_round_trip and tile_first_difference_flag_survives_round_trip cover the flags byte and the reserved high bits.

The fixtures are built with the crate's existing rlgr::encode helper, the same approach as the surrounding progressive tests; there were no captured progressive fixtures in the repo to reuse.

Each new test was checked against a deliberately broken build (accumulation reverted to replacement, and the wire dispatch forced to the absolute path) to confirm it fails without the fix.

Verification

cargo xtask check fmt -v      # All good!
cargo xtask check lints -v    # All good!
cargo xtask check tests -v    # All good!
cargo xtask check typos -v    # All good!
cargo xtask check locks -v    # All good!

cargo test -p ironrdp-graphics -p ironrdp-pdu: 217 passed / 0 failed, 412 passed / 0 failed.

Additional tests cover TILE_FIRST end to end, the reduce-extrapolate band layout, and a difference tile arriving after an upgrade pass. Each new test was checked against a deliberately broken build to confirm it fails without the fix.

Overlap and known gaps

  • fix(graphics): retain Progressive difference tiles #1698 covers the same bug and models the retained state better: it keys the sub-band diffing reference by surface and keeps it across ResetGraphics, per MS-RDPEGFX 3.3.1.3. This PR reuses TileState::coefficients, which is keyed by (surface_id, codec_context_id), so RDPGFX_DELETE_ENCODING_CONTEXT drops the reference that 3.3.1.3 says must survive. Maintainers should take whichever base they prefer; the tests here apply to both.
  • Not validated against a captured difference-encoded frame; the correctness argument rests on the spec text and on matching FreeRDP.
  • Separately, use_reduce_extrapolate is read from the CONTEXT block, where bit 0 is RFX_SUBBAND_DIFFING (2.2.4.2.1.4). RFX_DWT_REDUCE_EXTRAPOLATE is a REGION flag (2.2.4.2.1.5), and ProgressiveRegion::uses_reduce_extrapolate() is currently dead code. Pre-existing and out of scope here.

Refs #1240

The Progressive RemoteFX decoder parsed the tile flags byte on TILE_SIMPLE
and TILE_FIRST blocks but never acted on it. `TileState::decode_first`
unconditionally set `is_difference = false`, and no coefficient path ever
read that field, so a tile flagged RFX_TILE_DIFFERENCE (0x01) was decoded
as if it carried absolute coefficients.

Per MS-RDPEGFX sections 2.2.4.2.1.5.3 and 2.2.4.2.1.5.4, that flag means
the tile carries the compressed difference of the DWT coefficients for the
same tile between the current frame and the previous frame. Section
3.3.8.2.1.1 gives the decode rule: `DecDwtQ = DecDwtQ + DecProgQ * PQF`,
that is the newly decoded coefficients are added to the ones retained for
that tile instead of replacing them. Treating a difference tile as an
absolute one renders the delta as if it were the image, and every later
progressive upgrade pass then refines the wrong coefficients.

Changes:

- ironrdp-pdu: add `FLAG_TILE_DIFFERENCE` and `is_difference()` accessors on
  `TileSimple` and `TileFirst`, following the existing
  `FLAG_DWT_REDUCE_EXTRAPOLATE` / `uses_reduce_extrapolate()` pattern. The
  seven high bits of the flags field stay reserved and ignored.
- ironrdp-graphics: add `TileState::decode_first_difference`, which decodes
  the components into a scratch buffer and accumulates them into the
  retained coefficients, saturating at the `i16` bounds. The accumulation
  happens in the `DecDwtQ` domain, before base dequantization and the
  inverse DWT, which is where both the retained coefficients and the
  incoming differences live. `decode_tile_block` routes TILE_SIMPLE and
  TILE_FIRST to it when the flag is set.

The DAS sign state keeps describing the incoming differences rather than
the accumulated result, because that is what the upgrade passes refining
this transmission are encoded against, matching FreeRDP's
`progressive_decompress`. TILE_UPGRADE has no flags field on the wire and
already accumulates into the retained coefficients, so it is unaffected. A
tile that has not been decoded yet holds zeroed coefficients, so a
difference tile arriving first on a surface still reconstructs correctly.

The public API is only extended: `decode_first` keeps its signature and its
behaviour for original tiles.

Tests: unit tests covering accumulation, unchanged behaviour for
non-difference tiles, a difference tile on an untouched tile, `i16`
saturation, an upgrade pass refining accumulated coefficients, and an
end-to-end test through `ProgressiveDecoder::decode_bitmap` asserting that
a flagged tile renders identically to one absolute tile carrying the summed
coefficients. Round-trip tests in ironrdp-pdu cover the flags byte.

Refs Devolutions#1240
@mamoreau-devolutions

Copy link
Copy Markdown
Contributor

Yevhen Salitrynskyi (@ysalitrynskyi) I think you found the same thing I found while replaying packet captures taken with mstsc in IronRDP, I've got this pull request here: #1701

Correct the spec reference on the accumulation test: RFX_TILE_DIFFERENCE
semantics are defined by MS-RDPEGFX 3.3.8.2.1.1, not MS-RDPRFX.

Trim the doc comments added by the previous commit, and drop the sentences
that restate the arithmetic.

Extend the tests:

- Cover TILE_FIRST end to end. Only TILE_SIMPLE went through
  `decode_bitmap` before, leaving one of the two dispatch sites untested.
- Cover the reduce-extrapolate band layout on the difference path.
- Cover a difference tile arriving after an upgrade pass, which resets the
  progressive state while keeping the upgraded coefficients.
- Pin that an original tile ignores whatever coefficients were retained.
- Split the end-to-end test in two, and factor the stream builders so
  TILE_SIMPLE and TILE_FIRST share the block scaffolding.

Refs Devolutions#1240
@ysalitrynskyi
Yevhen Salitrynskyi (ysalitrynskyi) marked this pull request as ready for review August 18, 2026 20:48
Copilot AI balanced review requested due to automatic review settings August 18, 2026 20:48
@ysalitrynskyi

Yevhen Salitrynskyi (ysalitrynskyi) commented Aug 18, 2026

Copy link
Copy Markdown
Author

Yes, same bug. You got there first with #1698, and its model is the right one: the sub-band diffing reference belongs to the surface and survives ResetGraphics, while mine reuses the per-codec-context tile state. Left the review on #1698, and the tests here apply to either base.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds decoding support for Progressive RemoteFX difference tiles.

Changes:

  • Exposes and parses RFX_TILE_DIFFERENCE.
  • Accumulates decoded coefficient deltas with saturation.
  • Adds unit and end-to-end coverage.

Reviewed changes

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

File Description
crates/ironrdp-pdu/src/codecs/rfx/progressive.rs Adds difference-flag accessors and tests.
crates/ironrdp-graphics/src/progressive.rs Implements difference accumulation and decoding tests.

Comment thread crates/ironrdp-graphics/src/progressive.rs Outdated
Comment thread crates/ironrdp-graphics/src/progressive.rs
@github-actions github-actions Bot added kind/protocol Affects RDP or related protocol behavior maintainer-required Maintainer review or intervention is required risk/medium Behavioral change that does not substantially alter a core public API scope/core Touches the core architectural tier size/L Size: up to 899 counted lines and 20 files; exceeds M in either measure labels Aug 18, 2026
The retained coefficients stay base-quantized until reconstruction, so they
are only a meaningful reference for a difference tile while the base
quantization tables and the DWT variant hold. `decode_first_difference`
replaced both unconditionally, so a stream that changed the quantization
index, or flipped reduce-extrapolate, made the decoder reinterpret the
retained coefficients under a scale and a band layout they were never
quantized with. A difference tile carrying an all-zero delta then corrupted
most of the tile instead of leaving it unchanged.

MS-RDPEGFX does not define that case; an encoder that changes either has no
reference to send a difference against. Drop the reference and decode the
tile as an original one, which degrades a single tile rather than the frame.

Also assert that TILE_FIRST ignores the seven reserved flag bits. Only the
TILE_SIMPLE accessor was covered, so a regression there went unnoticed.

Refs Devolutions#1240
@github-actions github-actions Bot added size/XL Size: up to 1299 counted lines and 49 files; exceeds L in either measure and removed size/L Size: up to 899 counted lines and 20 files; exceeds M in either measure labels Aug 18, 2026
…odes

`decode_first_difference` accumulated straight into the tile and replaced
its metadata up front, so a stream whose second or third component failed
to decode left the first ones accumulated and the metadata already
replaced, even though the call returned an error. An empty component
stream is enough to reach that, and the damage lands on the reference the
next difference tile is decoded against.

Decode into local state and commit it once all three components succeed,
matching `decode_upgrade`.

Reported by Copilot on Devolutions#1701.

Refs Devolutions#1240
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/protocol Affects RDP or related protocol behavior maintainer-required Maintainer review or intervention is required risk/medium Behavioral change that does not substantially alter a core public API scope/core Touches the core architectural tier size/XL Size: up to 1299 counted lines and 49 files; exceeds L in either measure

Development

Successfully merging this pull request may close these issues.

3 participants