Would you accept an NSCodec encoder in ironrdp-server?

[clintcan]

Hi! I maintain macrdp — an xrdp-equivalent for macOS built on IronRDP — and I'm trying to upstream as much of my vendor/ironrdp-server divergence as possible. Before doing the port-and-cleanup work, I wanted to ask if you'd be open in principle to an NSCodec encoder in ironrdp-server.

Context

crates/ironrdp-pdu/src/rdp/capability_sets/bitmap_codecs/mod.rs defines the NSCodec capability advertisement + codec property, and ironrdp-server/src/server.rs:1109 has a CodecProperty::NsCodec(_) => () match arm so the server can see the client's NSCodec capability. But there's no actual NSCodec encoder — searching the full git history confirms one never existed.

That means a client that advertises only NSCodec for legacy bitmap codecs (no RemoteFx, no QOI) has to fall back to raw/RLE BitmapUpdate. The notable real-world case: the Microsoft Remote Desktop / Windows App on macOS advertises NSCodec only in its legacy codec list. Without an encoder, those clients pay heavy bandwidth for the legacy display path. (With --enable-h264 they decode AVC420 over EGFX fine, so this is specifically about the legacy fallback path.)

What I have

A ~200-line encoder following MS-RDPNSC §3.1.5: RGB→YCoCg color-space conversion, custom per-plane RLE, 20-byte frame header. Currently:

• 4:2:0 chroma subsampling is the obvious next step (it'd be "Phase 2") — for now ChromaSubsamplingLevel = 0 (no subsampling).
• Color Loss Level is configurable (passed by caller; default 3 in my use).
• Tested against the macOS MS Remote Desktop client in production for the last few months.
• Wires into the existing encoder/mod.rs set_surface() path the same way the other encoders do.

What I'm asking

Before I do the port-and-cleanup work for a PR:

1. Are you open in principle to an NSCodec encoder in ironrdp-server?
2. If yes — would you prefer it gated behind a Cargo feature (matches the qoi/qoiz precedent)?
3. Any concerns I should pre-empt? (Wire conformance, test coverage, integration with UpdateEncoderCodecs, etc.)

If the answer is "no, we don't want a second legacy codec encoder" — totally fair, I'll just keep it vendored. Happy either way; just want to avoid writing the cleanup PR if it'd be rejected on policy grounds.

Thanks!

[Marc-Andre Lureau (elmarco)]

nice! imho, nscodec could be added, gated would be good. Since you mention QOI, by any chance, did you try it? I haven't heard of people using it for real yet.

[clintcan]

Yeah, tried it briefly, and long story short, it doesn't work end to end.

Some detail below:

I built ironrdp-viewer with --features qoi,qoiz and pointed it at the macrdp server (which advertises QOI via the default features).
Negotiation picks QOI, the server starts encoding frames, and the client throws this once per frame:

WARN ironrdp_session::fast_path: Unsupported RGBA QOI data

…with the viewer drawing a blank window. ~412 warnings in ~12s on a loopback session.

You might want to confirm this on your end, but this is mainly on the stock ironrdp-server, I believe - I haven't looked into this issue as I don't have much use for it yet.

On NSCodec — thanks for the green light, will gate it behind a feature and put up the PR when time permits.

[Benoît Cortier (CBenoit)]

I’m also in favor of including NSCodec to IronRDP. Happy to get a battle tested implementation merged! I would like this to take the form of:

• A ironrdp-nscodec crate dedicated to it. Place the encoder behind an encoder feature flag, and we may implement the decoder side behind a decoder flag in the future. I would prefer no default feature enabled by default, so it’s opt-in either way.
• As suggested by Marc-Andre Lureau (@elmarco), integrate NSCodec to ironrdp-server, but behind a feature flag.

[Greg Lamberson (glamberson)]

I support the codec going in, and I often wonder about adding support for things like this myself but do not usually have time to do it, especially since they don't really touch my use case.

The placement is a separate question, and I want to surface it before the dedicated-crate shape gets locked in by #1332. The current approach looks like an ad hoc way to significantly change the architecture of the codebase.

The dedicated ironrdp-nscodec crate is fragmentation of the existing workspace pattern. Every other bitmap codec in the tree (ClearCodec, RDP 6.x, ZGFX, the RFX components) lives as a submodule of ironrdp-graphics, whose description reads "RDP image processing primitives." ironrdp-bulk is the only standalone codec crate and it covers compression rather than a bitmap codec. A 200-LOC NSCodec encoder belongs at ironrdp-graphics/src/nscodec/ alongside clearcodec/ and rdp6/.

The NSCodec part of this matters specifically. If the workspace were going to shift toward per-codec crates, NSCodec is the wrong starting point. It is a minor backward-compat codec with narrow real-world relevance (the macOS Microsoft Remote Desktop legacy fallback path being the primary case), and small enough that a dedicated crate carries a high overhead-to-value ratio. A peer crate ships LICENSE-APACHE, LICENSE-MIT, README, full Cargo metadata, and a feature-gated lib.rs as crate-infrastructure tax on what amounts to one encoder file. That overhead is acceptable for a foundational subsystem like ironrdp-egfx and not justified for a niche legacy encoder.

The arguments for peer-crate promotion do not hold up on inspection.

Opt-in by default is a solved problem inside ironrdp-graphics. A feature gate on the existing crate with #[cfg(feature = "nscodec")] pub mod nscodec; is functionally identical to a no-default-features empty-shell pattern, at zero crate-infrastructure cost.

Independent versioning is low-value for a stable 15-year-old codec API. The release-plz cost exceeds the decoupling benefit.

A possible future extraction roadmap for ClearCodec, RFX, or ZGFX does not require pre-emptive NSCodec placement. The extraction can happen later when there is an actual driver, and pre-empting creates inconsistency now for a hypothetical cleanup later.

The fragmentation cost is also real and compounding. A reader looking for codec implementations today looks in one place. With NSCodec out, the search space splits across ironrdp-graphics, ironrdp-nscodec, ironrdp-bulk, ironrdp-egfx, plus external qoicoubeh and openh264. Each extraction makes the next one cheaper to justify and the workspace harder to navigate.

My read is that NSCodec should fold into ironrdp-graphics/src/nscodec/ with an nscodec Cargo feature on the existing crate. I am also skeptical of pattern-shift toward per-codec crates more generally, and if one is on the roadmap, it should start with a codec whose weight justifies the overhead rather than a minor legacy fallback encoder.

[Benoît Cortier (CBenoit)]

Thanks Greg, that’s a fair point, and I understand why ironrdp-graphics/src/nscodec/ looks more consistent with the current tree.

That said, I don’t think the current layout fully reflects where I would like the architecture to go. Some of the existing codecs live in ironrdp-graphics mostly because I prioritized velocity and didn’t have time to split them into cleaner boundaries yet. I also realize I haven’t really expressed that intent widely, so thanks for challenging it and asking.

My preference is still to keep NSCodec as a separate crate. Module boundaries and crate boundaries are not the same thing: a crate gives us a clearer dependency boundary, compilation unit, public API surface, and opt-in story. Consumers can depend on ironrdp-nscodec or not, instead of growing ironrdp-graphics with more codec-specific feature flags.

I agree the crate overhead is not zero, especially for a small codec. But because NSCodec is small and self-contained, I think it’s actually a good place to start moving toward the cleaner codec boundaries I would eventually like to have for the other codecs too.

So unless there’s a strong reason not to, I would prefer to keep ironrdp-nscodec as its own crate.

Longer term, I would actually like Cargo to make this pattern cheaper rather than discourage it. There is already prior discussion around low-overhead package boundaries: [lib] support for single-file packages. That is very close to the kind of boundary I want here: a small self-contained codec with its own dependency/API/compilation boundary, without needing to pretend that a module boundary is equivalent. Current Cargo makes this a bit more verbose than ideal, but I don’t think that means the boundary itself is wrong.

I’m also open to having a slightly more nested hierarchy (such as placing all the codec crates into a codecs/ folder) if it gets hard to navigate, but I don’t feel like it’s necessary right now. What do you think?

[Benoît Cortier (CBenoit)]

I think that ultimately ironrdp-graphics should either provide common helper utilities or abstractions for all codecs, or be deprecated if there is nothing to salvage. But I remember we do have other items that are not codec implementations which could still live there.

While I’m at it, my other major refactoring plan is to split ironrdp-pdu into more meaningful crates. A trivial split based on specifications would give us something like:

• ironrdp-rdpbcgr (for the core structures)
• ironrdp-rdpegdi
• ironrdp-rdprfx
• …

Maybe we need to have pdu somewhere in there too, and maybe we can remove the rdp piece?

• ironrdp-pdu-bcgr
• ironrdp-pdu-egdi
• ironrdp-pdu-rfx
• …

That’s still something I am thinking about carefully though.

[Greg Lamberson (glamberson)]

Benoît Cortier (@CBenoit) OK well I'm a little surprised by this. As somebody who cares a lot about architecture, I wish this discussion had a better home than nested in a minor issue I wasn't really paying attention to. But really I'm mostly just rather unhappy generally with the state of communications in general, having had things fester on things I do care about (see #1172 ) with no result.

I hope that you'll consider this Marc-André Moreau (@awakecoding) and help improve things. Thanks.

[Benoît Cortier (CBenoit)]

Greg Lamberson (@glamberson) a few points from my side:

I can’t realistically write down everything I have in mind about IronRDP’s architecture upfront. Most of the time, the feedback I give is trigger-driven: a PR, a question, a concrete proposal. I think that’s appropriate given the bandwidth I have. That said, if you would like a dedicated discussion or issue for the codec-placement question (or any other architectural topic), feel free to open one and I’ll engage there.

On #1172 and the broader communications point, I replied separately with the specifics. As I mentioned there, we’ve merged a very large number of PRs from you and we want that to continue; I would like that to be the signal on our posture, not the gaps on individual PRs.

I also need to stay responsive to newer work and to other contributors, so I can’t allocate all of my review time to one queue; that’s the constraint I’m operating under, not a position on your contributions.

I’ll try over time to write down some of the architectural positions I’m carrying into issues, so they’re visible ahead of time rather than surfacing only at PR-review. It won’t be exhaustive, and I can’t put a date on it, but it should help when I get to it.

Thanks for raising it directly.

[Benoît Cortier (CBenoit)]

I created two issues to track the architectural directions mentioned in this discussion:

• Split bitmap codecs out of ironrdp-graphics into dedicated per-codec crates #1349
• Split ironrdp-pdu along Microsoft specification lines #1352

I also reviewed #1172

[clintcan]

Thanks Greg Lamberson (@glamberson) — that's a well-argued case, and the consistency point (every other bitmap codec living under ironrdp-graphics alongside clearcodec/, rdp6/, ZGFX, and RFX, with ironrdp-bulk being compression rather than a bitmap codec) is a fair one. The overhead-to-value observation for a ~200-LOC legacy encoder lands too.

I don't have a strong opinion on placement myself — my goal is just to get a working NSCodec encoder upstreamed in whatever shape the maintainers want to live with long-term. I built #1332 as a dedicated ironrdp-nscodec crate because that's the shape Benoît Cortier (@CBenoit) asked for above; refactoring it into ironrdp-graphics/src/nscodec/ behind an nscodec feature on the existing crate is a mechanical change I'm happy to make if that's the preferred direction.

Since this is really a workspace-architecture decision rather than anything specific to the encoder itself, I'll defer to Benoît Cortier (@CBenoit) on the call. Benoît Cortier (@CBenoit) — do you still want the dedicated-crate shape, or would you rather I fold it into ironrdp-graphics per the above? I'll move #1332 to match whichever you decide.

[Benoît Cortier (CBenoit)]

Yes, I’m still inclined towards a dedicated-crate shape: #1322 (reply in thread)

[clintcan]

Quick housekeeping note: both PRs that came up in this thread are now rebased onto current master and build clean locally —

• feat(nscodec): introduce ironrdp-nscodec crate + opt-in server integration #1332 (NSCodec encoder) — resolved the ironrdp-server/Cargo.toml conflict from the recent internal-crate version bumps and updated the new crate's ironrdp-graphics dep to match; --features nscodec and the default build both pass.
• fix(session): decode RGBA QOI bitmaps instead of dropping the frame #1341 (RGBA QOI decode, the companion to fix(server): emit RGB-channel QOI for opaque captures so ironrdp-session can decode #1335 for the Unsupported RGBA QOI data issue elmarco asked about) — clean rebase, and I gated the new apply_rgba32 helpers behind the qoi feature so the default build stays warning-free.

The placement question for #1332 (dedicated ironrdp-nscodec crate vs. folding into ironrdp-graphics/src/nscodec/) is still open and unchanged by the rebase — Benoît Cortier (@CBenoit), happy to reshape it whichever way you land on.

[Benoît Cortier (CBenoit)]

Thank you very much!! I’m currently finalizing the release of the crates, and I’ll stop merging PRs until the process is fully completed, but I’ll be reviewing 🙂