From af6539052ffda2f3514d6cf57254c1568fcd5663 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Thu, 19 Mar 2026 23:33:21 +0400 Subject: [PATCH 1/3] fix(graphics): align RLGR encoder RL mode with MS-RDPRFX spec and FreeRDP MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Run-Length mode in the RLGR encoder had two issues compared to the reference implementation in FreeRDP (rfx_rlgr.c) and the MS-RDPRFX §3.1.8.1.7.3 pseudocode: 1. Zero-counting consumed all zeros via peek/advance, leaving no current value. Then `if let Some(val) = input.next()` would skip encoding the sign bit and GR code when input was exhausted after a zero run. The spec and FreeRDP always emit sign + GR(mag-1) after the run, even when the trailing value is zero. 2. When the trailing value was zero (input exhausted), the encoder would call `code_gr(mag - 1)` which underflows for mag=0. Fix: restructure RL mode to match FreeRDP's GetNextInput pattern: - Read the first value before the zero-counting loop - Count zeros by checking if current value is zero AND more input exists - Always emit sign bit + GR code after the run (using mag-1 for nonzero, 0 for zero magnitude) This ensures the decoder, which unconditionally reads sign + GR after a run, will always find the expected bits in the stream. Ref: MS-RDPRFX §3.1.8.1.7.3.2 (RLGR Encode pseudocode) Co-Authored-By: Claude Opus 4.6 --- crates/ironrdp-graphics/src/rlgr.rs | 33 ++++++++++++++++++----------- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/crates/ironrdp-graphics/src/rlgr.rs b/crates/ironrdp-graphics/src/rlgr.rs index 42a8a0914..4b4ea1552 100644 --- a/crates/ironrdp-graphics/src/rlgr.rs +++ b/crates/ironrdp-graphics/src/rlgr.rs @@ -67,6 +67,7 @@ pub fn encode(mode: EntropyAlgorithm, input: &[i16], tile: &mut [u8]) -> Result< clippy::as_conversions, reason = "u32-to-usize and usize-to-u32 conversions, mostly fine, and hot loop" )] + #![expect(clippy::missing_panics_doc, reason = "unreachable panics (prior checks)")] if input.is_empty() { return Err(RlgrError::EmptyTile); @@ -83,15 +84,23 @@ pub fn encode(mode: EntropyAlgorithm, input: &[i16], tile: &mut [u8]) -> Result< while input.peek().is_some() { match CompressionMode::from(k) { CompressionMode::RunLength => { - let mut nz = 0; - while let Some(&&x) = input.peek() { - if x == 0 { - nz += 1; - input.next(); - } else { + // Read the first value (guaranteed Some by the while condition). + // This mirrors FreeRDP's GetNextInput before the zero-counting loop. + let mut val = *input + .next() + .expect("value is guaranteed to be `Some` due to the prior check"); + let mut nz: u32 = 0; + + // Count zeros: while current value is zero and more data exists, + // count it as a run zero and read the next value. + while val == 0 { + if input.peek().is_none() { break; } + nz += 1; + val = *input.next().expect("peek confirmed Some"); } + let mut runmax: u32 = 1 << k; while nz >= runmax { bits.output_bit(1, false); @@ -103,16 +112,16 @@ pub fn encode(mode: EntropyAlgorithm, input: &[i16], tile: &mut [u8]) -> Result< bits.output_bit(1, true); bits.output_bits(k as usize, nz); - if let Some(val) = input.next() { - let mag = u32::from(val.unsigned_abs()); - bits.output_bit(1, *val < 0); - code_gr(&mut bits, &mut krp, mag - 1); - } + // Always encode the value after the run (even if it's 0 due + // to exhausted input). This matches the reference encoder. + let mag = u32::from(val.unsigned_abs()); + bits.output_bit(1, val < 0); + code_gr(&mut bits, &mut krp, if mag > 0 { mag - 1 } else { 0 }); + kp = kp.saturating_sub(DN_GR); k = kp >> LS_GR; } CompressionMode::GolombRice => { - #[expect(clippy::missing_panics_doc, reason = "unreachable panic (prior check)")] let input_first = *input .next() .expect("value is guaranteed to be `Some` due to the prior check"); From 7e0a56e72a09e5a3259f0ca967d38acfb115f6e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Thu, 19 Mar 2026 23:36:04 +0400 Subject: [PATCH 2/3] fix(graphics): use zero default for RLGR3 exhausted second input value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In RLGR3 Golomb-Rice mode, two input values are consumed per iteration. When the input is exhausted before the second value can be read, the encoder used `unwrap_or(1)` as the default twoMs value, but the correct default is 0. FreeRDP's GetNextInput macro returns 0 when the input stream is exhausted (data_size == 0), and Get2MagSign(0) = 2*|0| - sign(0) = 0. Our encoder must match this behavior for interoperability. This changes 2 bytes in the Y component test vector (indices 939-940: 0x89,0x23 → 0x88,0xe3). These bytes differ from the MS-RDPRFX §4.2.4.1 reference hex dump, which appears to have been generated with a default of 1 — a discrepancy between the spec's example data and both the spec's pseudocode semantics and the FreeRDP reference implementation. The encoder→decoder roundtrip remains correct. Ref: MS-RDPRFX §3.1.8.1.7.3.2 (RLGR3 encode pseudocode) Co-Authored-By: Claude Opus 4.6 --- crates/ironrdp-graphics/src/rlgr.rs | 2 +- crates/ironrdp-testsuite-core/tests/graphics/rlgr.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ironrdp-graphics/src/rlgr.rs b/crates/ironrdp-graphics/src/rlgr.rs index 4b4ea1552..42df6c839 100644 --- a/crates/ironrdp-graphics/src/rlgr.rs +++ b/crates/ironrdp-graphics/src/rlgr.rs @@ -139,7 +139,7 @@ pub fn encode(mode: EntropyAlgorithm, input: &[i16], tile: &mut [u8]) -> Result< } EntropyAlgorithm::Rlgr3 => { let two_ms1 = get_2magsign(input_first); - let two_ms2 = input.next().map(|&n| get_2magsign(n)).unwrap_or(1); + let two_ms2 = input.next().map(|&n| get_2magsign(n)).unwrap_or(0); let sum2ms = two_ms1 + two_ms2; code_gr(&mut bits, &mut krp, sum2ms); diff --git a/crates/ironrdp-testsuite-core/tests/graphics/rlgr.rs b/crates/ironrdp-testsuite-core/tests/graphics/rlgr.rs index 9297ec253..1e9680948 100644 --- a/crates/ironrdp-testsuite-core/tests/graphics/rlgr.rs +++ b/crates/ironrdp-testsuite-core/tests/graphics/rlgr.rs @@ -205,7 +205,7 @@ const Y_DATA_ENCODED: [u8; 942] = [ 0x42, 0x02, 0xd9, 0x37, 0x11, 0xde, 0x2d, 0xd4, 0x3f, 0xfe, 0x61, 0xe7, 0x33, 0xd7, 0x89, 0x4a, 0xdd, 0xb0, 0x34, 0x47, 0xf4, 0xdc, 0xad, 0xaa, 0xc9, 0x9d, 0x7e, 0x6d, 0x4b, 0xcc, 0xdc, 0x17, 0x89, 0x57, 0xfd, 0xbb, 0x37, 0x75, 0x47, 0x5a, 0xec, 0x2c, 0x6e, 0x3c, 0x15, 0x92, 0x54, 0x64, 0x2c, 0xab, 0x9e, 0xab, 0x2b, 0xdd, 0x3c, 0x66, 0xa0, - 0x8f, 0x47, 0x5e, 0x93, 0x1a, 0x37, 0x16, 0xf4, 0x89, 0x23, 0x00, + 0x8f, 0x47, 0x5e, 0x93, 0x1a, 0x37, 0x16, 0xf4, 0x88, 0xe3, 0x00, ]; const CB_DATA_ENCODED: [u8; 975] = [ From a00843727762662c1190901ae6abeeb4bc1bfe0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Thu, 19 Mar 2026 23:36:41 +0400 Subject: [PATCH 3/3] fix(graphics): use UQ_GR constant in RLGR1 GR-mode kp update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The RLGR1 Golomb-Rice mode kp parameter update used UP_GR (4) when the encoded value (twoMs) was zero, but the correct constant is UQ_GR (3). Per MS-RDPRFX §3.1.8.1.7.3 pseudocode constants: - UP_GR = 4: increase in kp after a zero run in Run-Length mode - UQ_GR = 3: increase in kp after a zero symbol in GR mode These are distinct constants for different modes. The encoder incorrectly used the RL-mode constant (UP_GR=4) in the GR-mode path, causing kp to grow faster than specified, which shifts the boundary between RL and GR modes and produces a different bitstream. Both FreeRDP (rfx_rlgr.c line 748) and the MS-RDPRFX §3.1.8.1.7.3.2 pseudocode use UQ_GR for this update. Note: the MS-RDPRFX §4.2.4.1 reference hex dump appears to have been generated with UP_GR=4 in this position (a spec errata), as the spec's example data is consistent with the old buggy behavior. Our fix follows the normative pseudocode, which is authoritative over example data. Ref: MS-RDPRFX §3.1.8.1.7.3 (RLGR constants: UQ_GR=3, UP_GR=4) Ref: MS-RDPRFX §3.1.8.1.7.3.2 (RLGR1 encode pseudocode) Co-Authored-By: Claude Opus 4.6 --- crates/ironrdp-graphics/src/rlgr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ironrdp-graphics/src/rlgr.rs b/crates/ironrdp-graphics/src/rlgr.rs index 42df6c839..624c423a9 100644 --- a/crates/ironrdp-graphics/src/rlgr.rs +++ b/crates/ironrdp-graphics/src/rlgr.rs @@ -131,7 +131,7 @@ pub fn encode(mode: EntropyAlgorithm, input: &[i16], tile: &mut [u8]) -> Result< let two_ms = get_2magsign(input_first); code_gr(&mut bits, &mut krp, two_ms); if two_ms == 0 { - kp = min(kp + UP_GR, KP_MAX); + kp = min(kp + UQ_GR, KP_MAX); } else { kp = kp.saturating_sub(DQ_GR); }