Draft: fix(graphics): correct three RLGR encoder bugs per MS-RDPRFX spec#1179
Open
Marc-Andre Lureau (elmarco) wants to merge 3 commits intoDevolutions:masterfrom
Open
Draft: fix(graphics): correct three RLGR encoder bugs per MS-RDPRFX spec#1179Marc-Andre Lureau (elmarco) wants to merge 3 commits intoDevolutions:masterfrom
Marc-Andre Lureau (elmarco) wants to merge 3 commits intoDevolutions:masterfrom
Conversation
…eRDP 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Marc-Andre Lureau (elmarco)
changed the title
Contributor
Author
|
I mark this as Draft, I was testing the openspec skills with Claude. I didn't test yet the result with mstsc. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes three bugs in the RLGR entropy encoder (
ironrdp-graphics::rlgr::encode) identified by cross-referencing the MS-RDPRFX §3.1.8.1.7.3 pseudocode and the FreeRDP reference implementation (rfx_rlgr.c).GetNextInputpattern.unwrap_or(1)as defaulttwoMswhen the second value in a GR-mode pair was unavailable. FreeRDP'sGetNextInputreturns 0 when exhausted, andGet2MagSign(0) = 0, so the correct default is 0.UP_GR(4, the RL-mode constant) instead ofUQ_GR(3, the GR-mode constant) when updatingkpfor zero symbols. Both the spec pseudocode and FreeRDP useUQ_GRhere.Each fix is in a separate commit with full rationale and spec/FreeRDP references.
Spec errata note
The MS-RDPRFX §4.2.4.1 reference hex dump appears to have been generated with
UQ_GR=4andtwoMs2=1defaults — inconsistent with the normative pseudocode in §3.1.8.1.7.3. Our encoder follows the pseudocode (which is authoritative over example data) and matches FreeRDP. The Y test vector is updated accordingly (2 bytes differ from the spec hex dump at indices 939–940). Encoder→decoder roundtrip is verified correct.Test plan
cargo clippy -p ironrdp-graphicscleancargo xtask check lints -vpasses🤖 Generated with Claude Code