Draft: fix(graphics): correct three RLGR encoder bugs per MS-RDPRFX spec#1179
Draft
Marc-Andre Lureau (elmarco) wants to merge 3 commits intoDevolutions:masterfrom
Draft
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. |
There was a problem hiding this comment.
Pull request overview
This PR fixes three correctness issues in the ironrdp-graphics RLGR entropy encoder by aligning edge-case handling and parameter updates with the MS-RDPRFX normative pseudocode and FreeRDP’s reference implementation, and updates the RLGR3 Y-component test vector accordingly.
Changes:
- Fix RL-mode end-of-input handling to always emit the trailing sign bit + GR code (using a “read-first, then count zeros” structure).
- Fix RLGR3 GR-mode default for a missing second value in a pair (
twoMs2defaults to0, not1). - Fix RLGR1 GR-mode
kpupdate constant for zero symbols (UQ_GRinstead ofUP_GR), and update the affected golden encoded bytes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
crates/ironrdp-graphics/src/rlgr.rs |
Adjusts RL/GR encoding control flow and parameter updates to match spec/reference behavior for end-of-input and GR-mode updates. |
crates/ironrdp-testsuite-core/tests/graphics/rlgr.rs |
Updates the RLGR3 Y-component encoded golden vector to reflect the corrected encoder output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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