Repository navigation
refactor(server): thread quantization values through the RemoteFX encoder - #1684
Merged
Marc-André Moreau (mamoreau-devolutions) merged 1 commit intoAug 18, 2026
Conversation
…oder RfxEncoder hardcoded Quant::default() at the point of use instead of accepting it as encoder state. Add a quant field to RfxEncoder and a remotefx_quant field to UpdateEncoderCodecs, with a setter mirroring the existing set_remotefx pattern. Server behavior is unchanged since the capability-negotiation call site still passes Quant::default(). This is prerequisite plumbing for making quantization configurable.
Greg Lamberson (glamberson)
temporarily deployed
to
llm-providers
August 16, 2026 22:28 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
github-actions
Bot
added
kind/protocol
This was referenced Aug 16, 2026
Closed
Marc-André Moreau (mamoreau-devolutions)
requested a balanced review from Copilot
August 18, 2026 00:10
Copilot started reviewing on behalf of
Marc-André Moreau (mamoreau-devolutions)
August 18, 2026 00:11
Contributor
There was a problem hiding this comment.
Pull request overview
Threads sender-selected RemoteFX quantization through the server encoder while preserving current defaults.
Changes:
- Stores quantization in
UpdateEncoderCodecsandRfxEncoder. - Passes the configured table through encoder construction.
- Retains
Quant::default()during capability processing.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
crates/ironrdp-server/src/server.rs |
Supplies default RemoteFX quantization. |
crates/ironrdp-server/src/encoder/mod.rs |
Threads quantization through codec selection. |
crates/ironrdp-server/src/encoder/rfx.rs |
Uses encoder-held quantization values. |
| /// Quantization values for the RemoteFX encoder. Independent of | ||
| /// `remotefx` (entropy coder + codec id, negotiated from the client's | ||
| /// capabilities) because this is server-chosen, not client-negotiated: | ||
| /// [MS-RDPRFX] 2.2.2.1.5 leaves quantization entirely up to the sender. |
Marc-André Moreau (mamoreau-devolutions)
merged commit Aug 18, 2026
1bb08bf
into
Devolutions:master
59 checks passed
Marc-André Moreau (mamoreau-devolutions)
pushed a commit
that referenced
this pull request
Aug 18, 2026
Add Quant::try_new(), a validating constructor that rejects any subband value outside the 6..=15 range, and RdpServerBuilder::with_remotefx_quant(quant: Quant), wiring it through RdpServerOptions to the same capability-negotiation call site #1684 touched. Per [MS-RDPRFX] 2.2.2.1.5, each of the 10 TS_RFX_CODEC_QUANT values is a 4-bit field, and the legal range is 6 to 15. #1557 reports the quant table is hardcoded to Quant::default(); this gives callers a validated way to set it instead. Builds on #1684, which added the storage this PR wires up. Default behavior is unchanged: with_remotefx_quant is opt-in, and a server that doesn't call it still gets Quant::default(), the same values Windows RDP servers send. Closes #1557.
Anton Mostovoy (antonmos)
added a commit
to antonmos/IronRDP
that referenced
this pull request
Aug 18, 2026
Two purely additive conflicts against master's Devolutions#1684/Devolutions#1685 (configurable RemoteFX quantization table, unrelated to preemption): RdpServerOptions and its builder-state counterpart each gained a new remotefx_quant: Quant field alongside this branch's own new preempt_existing_session: bool field -- both fields kept. Also added the new field to the one hand-built RdpServerOptions literal this branch has (the preempt_tests test-context helper), which the auto-merge couldn't do for me since master has no equivalent code to diff against there. 23/23 tests pass in both default and --features egfx, clippy --all-targets clean in both, fmt clean, full cargo build --workspace green.
Marc-André Moreau (mamoreau-devolutions)
pushed a commit
that referenced
this pull request
Aug 20, 2026
… last (#1686) The client's advertised TS_RFX_ICAP array was folded with a plain assignment in the capability exchange loop, so whichever entry happened to be parsed last silently decided the entropy coder. Add pick_remotefx_entropy_coder and RdpServerBuilder::with_remotefx_entropy_coder so the caller can state a preferred coder, with a documented fallback instead of an accidental one. Per MS-RDPRFX 2.2.1.1.1.1, the TS_RFX_ICAP array is the set of codecs the client supports, not a ranked preference, so the server is entitled to pick any entry. With no preference configured, the new fallback is whichever coder the client offers first, which is deterministic and matches the array's natural order, instead of whichever happened to be parsed last. mstsc sends RLGR1 then RLGR3, so today's last-wins behavior always lands on RLGR3, and RLGR1 never runs against a real client. Builds on #1684 and #1685, which give the quant table (#1557) the same server-side shape. Default behavior changes for clients that offer both coders: the server now uses the first-offered coder instead of the last-offered one, unless with_remotefx_entropy_coder states otherwise. Closes #1562.
Anton Mostovoy (antonmos)
added a commit
to antonmos/IronRDP
that referenced
this pull request
Aug 20, 2026
…, keyboard metadata, entropy coder) Three upstream commits since the last sync, all in crates/ironrdp-server: Devolutions#1691 (feat!, keyboard metadata via ConnectionHandler::on_connection_info) merged with no conflict -- purely additive (a new struct, a new default trait method, one call site in client_accepted, which this branch never touches). Devolutions#1721 (fix, release static channels when a connection ends) restructured run_connection_with itself, which this branch also restructured (into PendingConnection-based negotiation) -- git's merge conflicted on run()'s whole preemption-race loop and, separately, split my mod preempt_tests and Devolutions#1721's own new mod tests into two overlapping conflict hunks (same file-tail location, different module names). Resolved by taking this branch's side for the run() loop (a superset of upstream's simpler non-preemption tail -- same on_disconnected call, just wrapped in the preemption if/else) and keeping BOTH test modules side by side. One splice bug caught by the build: mod preempt_tests was left without its own closing brace; fixed. The real interaction worth documenting: Devolutions#1721's fix assumes run_connection_with is the only connection entry point, but this branch adds a second one, serve_negotiated (the preemption-winner path), which never calls run_connection_with. Removing run()'s own static_channels reset -- which upstream's fix intends, since run_connection_with now resets it universally -- would have silently reintroduced Devolutions#1721's exact leak, just scoped to every preemption takeover instead of every run_connection embedder. Kept run()'s reset, with a comment explaining why it is NOT redundant despite resetting the same field twice on the run_connection path. Devolutions#1685/Devolutions#1684 (RemoteFX quant, already merged in c831bcc) plus this sync's new Devolutions#1686 (entropy coder weighting) add a second new RdpServerOptions field, remotefx_entropy_coder: Option<EntropyBits> -- added to the one hand-built RdpServerOptions literal in preempt_tests, same as remotefx_quant before it. Verification, in order, catching problems at each step rather than trusting a clean `cargo build`: plain build was clean but does not compile #[cfg(test)] code, so `cargo test` is what actually caught the missing remotefx_entropy_coder field. 24/24 tests pass in both default and --features egfx (up from 23 -- upstream's own run_connection_releases_the_static_channels regression test now runs against this branch's PendingConnection-based negotiation and passes, which is strong evidence the merge is semantically correct, not just textually conflict-free). clippy --all-targets clean in both configs, fmt clean, full cargo build --workspace green.
This branch was previously deployed
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.
RfxEncoder hardcoded Quant::default() at the point of use instead of accepting it as encoder state. Add a quant field to RfxEncoder and a remotefx_quant field to UpdateEncoderCodecs, with a setter that mirrors the existing set_remotefx pattern.
Per [MS-RDPRFX] 2.2.2.1.5, quantization is chosen entirely by the sender and isn't part of capability negotiation, so making it configurable is purely a server-side plumbing problem. This is the first step: give the encoder somewhere to actually hold and use a caller-chosen Quant. #1557 asks for the ability to set it; this PR doesn't add that public entry point yet, it just makes the encoder capable of taking one.
Server behavior is unchanged. The capability negotiation call site still passes Quant::default(), so this is pure plumbing with no functional change.