Skip to content

fix(graphics): apply YCbCr->RGB fixed-point descale in progressive decoder/encoder#1400

Open
Rocco De Angelis (rdeangel) wants to merge 2 commits into
Devolutions:masterfrom
rdeangel:fix/progressive-color-descale
Open

fix(graphics): apply YCbCr->RGB fixed-point descale in progressive decoder/encoder#1400
Rocco De Angelis (rdeangel) wants to merge 2 commits into
Devolutions:masterfrom
rdeangel:fix/progressive-color-descale

Conversation

@rdeangel

Copy link
Copy Markdown
Contributor

Problem

Against some real-world EGFX servers (xrdp, GNOME Remote Desktop), the progressive (RemoteFX) decoder produces posterised output. Smooth colour gradients collapse to a handful of saturated primaries and luma loses its mid-tones. Spatial detail (text, icons) stays crisp, which points away from a scaling issue and toward colour conversion.

Root cause

reconstruct_to_rgba() passes the dequantized Y/Cb/Cr values into the BT.601 YCbCr->RGB conversion, but the fixed-point headroom that dequantize_component_ccq intentionally retains isn't removed before conversion.

dequantize_component_ccq applies the spec-defined << (quant_value - 1) shift. For the wire quant table this works out to +5 bits of headroom, which matches FreeRDP's reference decoder (see the /* -6 + 5 = -1 */ comment in libfreerdp/primitives/prim_colors.c). The colour-conversion stage is the natural place to remove that headroom (R = ((CrR + Y) >> divisor) >> 5). This PR adds the missing >> 5 and the corresponding 128 << 5 = 4096 luma offset. Without them, each channel exceeds the legal ±128 range and clamp_u8 pins it to 0 or 255.

Worth noting: this isn't the integer inverse DWT. The DWT is unity-gain, verified separately below.

Verification

  1. DWT unity-gain check. A flat 64×64 tile of value 100 forward-transforms with no coefficient exceeding ~100 and round-trips back to exactly 100, so the ×2^5 scale doesn't originate there. Added tests/dwt_gain.rs to lock this in.
  2. Live server quant trace. Against an xrdp stream, the per-band << (q-1) factors land at +5 bits of headroom (max coefficient scaled ~32×). The +5 bits come from the dequant table.
  3. FreeRDP cross-check. libfreerdp/codec/progressive.c and libfreerdp/primitives/prim_colors.c apply the same << (q-1) shift and the matching >> 5 removal at the colour stage, so this patch's math lines up with the reference.
  4. Visual confirmation. With the >> 5 descale applied, xrdp and GNOME RD streams render with correct colour gradients; without it, the same streams are posterised.

Scope of this PR

Kept symmetric so the crate's internal encode<->decode round-trip test stays identity:

  • Decoder: >> 5 plus +128 luma offset in reconstruct_to_rgba (matching FreeRDP).
  • Encoder: matching << 5 plus offset on the RGB->YCbCr path in rgba_to_ycbcr, so the round-trip doesn't regress.

Files

  • crates/ironrdp-graphics/src/progressive.rs — decoder descale and encoder scale-up
  • crates/ironrdp-graphics/tests/dwt_gain.rs — regression test confirming the DWT is unity-gain

rdeangel and others added 2 commits July 1, 2026 20:27
…coder/encoder

The progressive (RemoteFX) decoder produced posterised output against
real EGFX servers (xrdp, GNOME Remote Desktop): smooth gradients
collapsed to saturated primaries and luma lost its mid-tones.

reconstruct_to_rgba() fed the dequantized Y/Cb/Cr into the BT.601
YCbCr->RGB conversion without removing the +5 bits of fixed-point
headroom that dequantize_component_ccq intentionally retains (the
DWT itself is unity-gain; see tests/dwt_gain.rs). Every channel then
exceeded the legal +/-128 range and clamp_u8 pinned it to 0/255.

Add the `>> 5` descale plus the 128<<5 luma offset at the colour
stage, matching FreeRDP's yCbCrToRGB_16s8u_P3AC4R. Apply the matching
`<< 5` on the encoder's RGB->YCbCr path so the crate's encode/decode
round-trip stays an identity.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The upstream CI runs clippy with -D warnings, which denies both
clippy::print_stderr and clippy::print_stdout. The diagnostic prints
were only useful with --nocapture and are redundant with the assert!
messages, so remove them entirely.
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.

1 participant