feat(nscodec): introduce ironrdp-nscodec crate + opt-in server integration by clintcan · Pull Request #1332 · Devolutions/IronRDP

clintcan · 2026-05-27T20:19:36Z

Implements the NSCodec encoder agreed on in discussion #1322 (link). Follows the architecture Benoît Cortier (@CBenoit) suggested there: a new ironrdp-nscodec peer crate (rather than folding the encoder into ironrdp-server), encoder behind a feature flag, no default features, server integration also behind its own opt-in feature.

Summary

Two commits, intentionally separable:

feat(nscodec): introduce ironrdp-nscodec crate with MS-RDPNSC encoder
- New crates/ironrdp-nscodec/ crate. Encoder behind encoder feature, off by default. default-features = false consumers get an empty shell.
- Implementation of MS-RDPNSC §3.1.5: RGB→YCoCg conversion, per-plane RLE, 20-byte frame header.
- Public API: encoder::encode(data, w, h, stride, format, color_loss_level) -> Vec<u8> decoupled from BitmapUpdate so the server-side glue is thin.
- Verified byte-identical against a production-tested reference encoder over 1008 fuzzed cases (8 pixel formats × 6 sizes × 7 CLL values × 3 random seeds). No behavioral drift from the implementation that has been in use against macOS Microsoft Remote Desktop for the last several months.
- Limitations called out in code: no 4:2:0 subsampling yet (ChromaSubsamplingLevel = 0 always); encoder-only (the IronRDP client decodes NSCodec via ironrdp-session today).
feat(server): wire NSCodec encoder via ironrdp-nscodec behind a feature gate
- New opt-in nscodec feature on ironrdp-server pulling in ironrdp-nscodec with its encoder feature.
- Replaces the CodecProperty::NsCodec(_) => () no-op with an active arm gated on the new feature; the no-op is preserved when the feature is off, so default builds have zero behavior change.
- NsCodecHandler slots into the existing BitmapUpdater selection at lower priority than RemoteFX (priority: qoiz > qoi > remotefx > nscodec). NSCodec exists mainly for clients whose legacy bitmap-codec list advertises only NSCodec — notably the macOS Microsoft Remote Desktop / Windows App, which otherwise falls through to raw/RLE BitmapUpdate at much higher bandwidth.
- CLL is taken from the client's confirmed NsCodec capability so the server encodes at the same shift the client decodes against.

Test plan

Notes for review

The last-4-bytes-raw plane convention isn't in the MS-RDPNSC text but is mandatory by the FreeRDP reference implementation that Microsoft NSCodec decoders are written against. Comment in the encoder explains this.
debug_assert!(color_loss_level in 1..=7) because the i8 intermediate storage of Co/Cg overflows at CLL=0. Defensive clamping in release builds keeps the output decodable-shaped rather than wrapping-garbage. Real servers should advertise CLL ≥ 1.
The decoder side (decoder feature) is intentionally not pre-declared — happy to add it in a future PR if and when there's a need.

Discussion

#1322 — NSCodec encoder, asked & answered by Marc-Andre Lureau (@elmarco) / Benoît Cortier (@CBenoit).

…p gets a real legacy bitmap codec The vendored `vendor/ironrdp-server/src/encoder/nscodec.rs` file has existed for months but was never plugged in — no `mod nscodec;`, no selection arm, no handler. Negotiation went through with NSCodec advertised, then the server's `CodecProperty::NsCodec(_) => ()` no-op match arm dropped the confirmation, and the encoder selection fell through to raw `BitmapUpdate`. The macOS Microsoft Remote Desktop / Windows App client — whose legacy codec list contains *only* NSCodec — was therefore paying full uncompressed bitmap bandwidth on every frame. Wire it up properly, mirroring the upstream PR #1332 design (which lives behind a feature gate; the vendor here is unconditional since macrdp always wants this path): - `vendor/ironrdp-server/src/encoder/mod.rs`: - `mod nscodec;` so the existing dead encoder is actually compiled. - `nscodec: Option<(u8, u8)>` slot on `UpdateEncoderCodecs` plus a `set_nscodec` setter. - `BitmapUpdater::NsCodec(NsCodecHandler)` variant + dispatch. - Selection arm in `UpdateEncoder::new` *below* RemoteFX, so clients that also support RemoteFX keep using it; NSCodec is the legacy fallback for clients that don't. - `NsCodecHandler::new` emits a one-shot `debug!("NSCodec encoder selected for this session", codec_id, color_loss_level)` line so codec selection is observable at `RUST_LOG=...ironrdp_server::encoder=debug`. - `vendor/ironrdp-server/src/server.rs`: - `has_nscodec()` helper, same shape as `has_qoi` / `has_qoiz` / `has_remote_fx`. - Active `CodecProperty::NsCodec(client_ns) if self.opts.has_nscodec()` match arm above the existing no-op fallback; the CLL the server encodes at is taken from the client's confirmed capability so encoder and decoder run at the same shift. End-to-end verified against the macOS Microsoft Remote Desktop / Windows App client on 127.0.0.1:3390: Received connection peer=... client confirmed bitmap codecs codecs=[Codec { id: 1, property: NsCodec(NsCodec { ..., color_loss_level: 3 }) }] NSCodec encoder selected for this session codec_id=1 color_loss_level=3 Modern FreeRDP loads the NSCodec decoder module at connect but doesn't advertise NSCodec in its `ClientConfirmActive`, so xfreerdp/sfreerdp don't exercise this path today — the test client is specifically Windows App / Microsoft Remote Desktop on macOS. CLAUDE.md updated with divergence (6) under the `vendor/ironrdp-server` note, and the "keep until upstreamed AND released" sentence now lists (2)/(3)/(4)/(5)/(6). Upstream port: Devolutions/IronRDP#1332 Refs: Devolutions/IronRDP#1322 (NSCodec discussion).

Greg Lamberson (glamberson) · 2026-05-28T18:06:46Z

clintcan, the encoder reads cleanly and the byte-equivalence harness against your production reference over 1008 fuzzed cases is exactly the kind of evidence that earns merge. The implementation work is solid and I am glad to see it offered.

I have posted a placement observation on #1322 about the dedicated-crate shape versus folding into ironrdp-graphics/src/nscodec/ alongside the other bitmap codecs. That is a question for the architecture call made on the discussion, not a comment on the implementation here. If the placement ends up shifting, the encoder lift is mechanical and your harness carries over unchanged.

Adds an `ironrdp-nscodec` crate at `crates/ironrdp-nscodec/` carrying an MS-RDPNSC §3.1.5-compliant encoder. The encoder is opt-in behind an `encoder` feature flag so a default consumer with `default-features = false` (the normal case) compiles to an empty shell; no behavior change for crates that don't ask for it. Architecture follows guidance on discussion Devolutions#1322: - New peer crate (rather than folding the encoder into `ironrdp-server`), consistent with `ironrdp-egfx` / `ironrdp-cliprdr` etc. - No default features. Both server- and downstream-consumer integrations are expected to keep NSCodec opt-in. - Reserved space for a future `decoder` feature is *not* pre-declared — whoever adds the decoder side can add the feature in that PR. Implementation notes: - Public API is `encoder::encode(data, width, height, stride, format, color_loss_level) -> Vec<u8>`. Format is the existing `ironrdp_graphics::image_processing::PixelFormat`; all eight 32-bpp variants are handled. Output is suitable to drop into the bitmapData of a `TS_BITMAP_DATA_EX` in a `SurfaceBitsPdu` — PDU plumbing is intentionally left to the consumer (typically `ironrdp-server`). - YCoCg conversion uses the FreeRDP formulation that Microsoft clients decode against; verified byte-identical against a production-tested reference implementation across 1008 fuzzed cases (8 formats × 6 sizes × 7 CLL values × 3 random seeds, all pass). - `debug_assert!(color_loss_level in 1..=7)` because at CLL=0 the intermediate Co/Cg values overflow the i8 storage. Clamping on the cast paths is defensive against that case in release builds. - The last 4 bytes of each plane are emitted raw rather than RLE-encoded. This convention is implementation-derived (matches FreeRDP's reference encoder, which Microsoft NSCodec decoders are written against) rather than spelled out in the MS-RDPNSC text. - Chroma subsampling is not implemented; `ChromaSubsamplingLevel` is always emitted as 0 (full-resolution chroma). A future change can add 4:2:0 support behind the same encoder API. Limitations called out in code/README: - No 4:2:0 subsampling yet. - Encoder-only; no decoder side (the IronRDP client decodes NSCodec via `ironrdp-session` today). Tested via: - `cargo test -p ironrdp-nscodec --features encoder` (7/7 pass). - Standalone equivalence harness comparing against a production-tested encoder over 1008 random inputs — byte-identical output. - `cargo clippy -p ironrdp-nscodec [--features encoder] --all-targets -- -D warnings` (clean in both feature configurations). - `cargo fmt --check` (clean). - `cargo build --workspace` (no impact on other crates). Discussion: Devolutions#1322

…re gate When a client confirms `CodecProperty::NsCodec` during capability negotiation, the server now drives an `NsCodecHandler` that encodes each `SurfaceBitsPdu` via `ironrdp_nscodec::encoder::encode()`. The handler is selected only when no higher-priority codec has been negotiated (qoiz > qoi > remotefx > nscodec); priority is intentionally last so clients that also offer RemoteFX continue to use it. NSCodec exists mainly for clients whose legacy bitmap-codec list advertises only NSCodec — notably the macOS Microsoft Remote Desktop / Windows App, which otherwise falls through to raw/RLE BitmapUpdate at much higher bandwidth. Wiring lives behind a new opt-in `nscodec` feature on `ironrdp-server` (default-off) which pulls in `ironrdp-nscodec` with its `encoder` feature enabled. With the feature off, the existing `CodecProperty::NsCodec(_) => ()` no-op match arm is preserved, so there is no behavior change for default builds. The CLL the server uses for each connection is taken from the client's confirmed `NsCodec` capability rather than from the server's own advertisement — that way the server encodes at the same shift the client decodes against, regardless of which CLL each side proposed. Co-design with Devolutions#1322 (NSCodec encoder discussion).

Benoît Cortier (CBenoit)

Thank you clintcan and Greg Lamberson (@glamberson). Implementation is also looking good to me.
I flagged a few places, and I’ll request a Copilot review for sanity, address if relevant.
My intention is to merge this PR once I am done with publishing latest versions for ironrdp-egfx and ironrdp-server.

Benoît Cortier (CBenoit) · 2026-06-01T10:59:28Z

+    #[cfg(feature = "nscodec")]
+    fn has_nscodec(&self) -> bool {
+        self.codecs
+            .0
+            .iter()
+            .any(|codec| matches!(codec.property, CodecProperty::NsCodec(_)))
+    }


question: Would it make sense to have this method defined regardless of the nscodec feature flag, but with a blanket implementation returning an unconditional false when the codec is not part of the build?

rationale: Typically less conditional compilation churn in consumer code, but I’m not sure how useful that is broadly.

Good question. I went with the gated method to follow the existing qoi / qoiz precedent: has_qoi and has_qoiz are themselves #[cfg(feature = "qoi")] / #[cfg(feature = "qoiz")] (server.rs:126/134), and the whole chain is gated to match — the nscodec field on UpdateEncoderCodecs, its set_nscodec setter, has_nscodec, and the codec-selection match arm.

A blanket has_nscodec() -> false would compile, but it wouldn't remove the #[cfg] at the one call site: the match arm's body calls update_codecs.set_nscodec(...), and that setter (plus the field it writes) only exists under the feature, so the arm has to stay gated either way. Making just has_nscodec unconditional would then leave it inconsistent with has_qoi/has_qoiz without saving any churn.

If you'd prefer the blanket-false style as a broader convention, the cleaner move is to apply it to qoi/qoiz too in a separate pass so they stay consistent — happy to do that here or as a follow-up, whichever you'd rather. For this PR I'd lean toward keeping it matching the current pattern.

Sounds good to me. You convinced me to not change anything unless we really see a benefit to it.

Benoît Cortier (CBenoit) · 2026-06-01T11:01:10Z

+NSCodec ([MS-RDPNSC]) implementation for IronRDP.
+
+NSCodec is a legacy bitmap codec used in the RDP "Surface Bits" command path. It
+predates RemoteFX and QOI/QOIZ but remains the only legacy codec advertised by


nit: I think QOI/QOIZ being an IronRDP-specific experimentation I wouldn’t mention it along something like RemoteFX

Benoît Cortier (CBenoit) · 2026-06-01T11:02:38Z

issue: Same comment for both LICENSE-APACHE and LICENSE-MIT files. IIRC we are using symlink everywhere else. Copy-paste the symlinks so it’s identical everywhere and we don’t have duplicate files.

Copilot

Pull request overview

This PR adds an opt-in implementation of the legacy RDP NSCodec encoder as a standalone crate, and wires it into ironrdp-server behind a feature flag so servers can serve NSCodec-only clients (notably macOS Microsoft Remote Desktop / Windows App) without default-build behavior changes.

Changes:

Introduce new ironrdp-nscodec crate with an encoder feature exposing an encode(...) -> Vec<u8> API implementing MS-RDPNSC plane encoding + RLE.
Add an opt-in nscodec feature to ironrdp-server and select NsCodecHandler as a low-priority bitmap updater when negotiated.
Update workspace lockfile to include the new crate dependency graph.

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
crates/ironrdp-server/src/server.rs	Records negotiated NSCodec (codec id + client CLL) into the session encoder selection when `nscodec` is enabled.
crates/ironrdp-server/src/encoder/mod.rs	Adds `NsCodecHandler` and integrates it into codec selection and bitmap update dispatch behind `cfg(feature = "nscodec")`.
crates/ironrdp-server/Cargo.toml	Adds `nscodec` feature and optional dependency on `ironrdp-nscodec` with `encoder` enabled.
crates/ironrdp-nscodec/src/lib.rs	Defines the crate surface as an “empty shell” unless the `encoder` feature is enabled.
crates/ironrdp-nscodec/src/encoder.rs	Implements MS-RDPNSC encoder (RGB→YCoCg, per-plane RLE, 20-byte header) plus unit tests.
crates/ironrdp-nscodec/README.md	Documents crate purpose, feature flags, and current limitations.
crates/ironrdp-nscodec/Cargo.toml	Declares new crate metadata and `encoder` feature dependency on `ironrdp-graphics`.
crates/ironrdp-nscodec/LICENSE-MIT	Adds MIT license text for the new crate.
crates/ironrdp-nscodec/LICENSE-APACHE	Adds Apache-2.0 license text for the new crate.
Cargo.lock	Adds `ironrdp-nscodec` entries and `ironrdp-server` dependency update.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+///   wrong shift and chroma will look wrong. We `debug_assert!` `>= 1`; at
+///   CLL=0 the intermediate Co/Cg values overflow `i8` storage and produce
+///   garbage chroma — callers should either clamp upstream or arrange for the
+///   capability advertisement to never send CLL=0.


+/// Debug-asserts `color_loss_level >= 1` and `color_loss_level <= 7`. In
+/// release builds the function does not panic on bad CLL but the output will
+/// be undecodable (CLL=0) or shifted past zero precision (CLL>7).


+    let w = usize::from(width);
+    let h = usize::from(height);
+    let pixels = w * h;
+    let cll = i32::from(color_loss_level);
+    let bpp = usize::from(format.bytes_per_pixel());


+///
+/// # Parameters
+///
+/// - `data` — pixel buffer in `format`, with `stride` bytes per row.


- LICENSE-APACHE/LICENSE-MIT: replace copied files with symlinks to the workspace root, matching every other crate (CBenoit). - README: drop the QOI/QOIZ mention next to RemoteFX (IronRDP-specific experimentation, not a peer of RemoteFX) (CBenoit). - encoder docs: correct the CLL=0 claim — Co/Cg are clamped into i8, not overflowed, so the frame decodes with severe chroma clipping rather than being undecodable; reword # Panics accordingly (Copilot). - encoder docs: document that input pixel alpha is ignored (always emits an opaque 0xFF alpha plane) (Copilot). - encode: add debug_assert!s for stride >= width*bpp and minimum data length so invalid inputs fail with a clear message instead of an opaque OOB slice panic; release behavior unchanged (Copilot).

Benoît Cortier (CBenoit)

LGTM! Thank you

Clint Christopher Canada added 2 commits May 29, 2026 07:38

clintcan force-pushed the feat-nscodec-crate branch from 520e604 to 5249a38 Compare May 28, 2026 23:41

Benoît Cortier (CBenoit) linked an issue Jun 1, 2026 that may be closed by this pull request

Pilot the per-codec-crate pattern by introducing ironrdp-nscodec #1350

Closed

Benoît Cortier (CBenoit) reviewed Jun 1, 2026

View reviewed changes

Benoît Cortier (CBenoit) requested a review from Copilot June 1, 2026 11:05

Copilot started reviewing on behalf of Benoît Cortier (CBenoit) June 1, 2026 11:05 View session

Copilot AI reviewed Jun 1, 2026

View reviewed changes

Benoît Cortier (CBenoit) approved these changes Jun 1, 2026

View reviewed changes

Benoît Cortier (CBenoit) merged commit 54af8f6 into Devolutions:master Jun 1, 2026
20 checks passed

devolutionsbot mentioned this pull request Jun 1, 2026

chore(release): prepare for publishing #1340

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(nscodec): introduce ironrdp-nscodec crate + opt-in server integration#1332

feat(nscodec): introduce ironrdp-nscodec crate + opt-in server integration#1332
Benoît Cortier (CBenoit) merged 3 commits into
Devolutions:masterfrom
clintcan:feat-nscodec-crate

clintcan commented May 27, 2026

Uh oh!

Greg Lamberson (glamberson) commented May 28, 2026

Uh oh!

Benoît Cortier (CBenoit) left a comment •

edited

Loading

Uh oh!

Benoît Cortier (CBenoit) Jun 1, 2026

Uh oh!

clintcan Jun 1, 2026

Uh oh!

Benoît Cortier (CBenoit) Jun 1, 2026

Uh oh!

Benoît Cortier (CBenoit) Jun 1, 2026

Uh oh!

Benoît Cortier (CBenoit) Jun 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Benoît Cortier (CBenoit) left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Conversation

clintcan commented May 27, 2026

Summary

Test plan

Notes for review

Discussion

Uh oh!

Greg Lamberson (glamberson) commented May 28, 2026

Uh oh!

Benoît Cortier (CBenoit) left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

clintcan Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Benoît Cortier (CBenoit) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Benoît Cortier (CBenoit) left a comment •

edited

Loading