Skip to content

feat(egfx): add AVC444 tiles to mixed-codec frames - #2002

Merged
Benoît Cortier (CBenoit) merged 2 commits into
Devolutions:masterfrom
lamco-admin:feat/egfx-mixed-frame-avc444
Oct 8, 2026
Merged

Benoît Cortier (CBenoit) merged 2 commits into
Devolutions:masterfrom
lamco-admin:feat/egfx-mixed-frame-avc444

Conversation

@glamberson

@glamberson Greg Lamberson (glamberson) commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Depends on #2000 (tile checks in send_mixed_frame), which should merge first; until then this diff also shows its changes as the first commit, and the second commit alone is this change.

MixedTilePayload covers ClearCodec, Progressive and AVC420, so a server that negotiated AVC444 has to drop to AVC420 for any frame that also carries a lossless tile, or split the update into two frames and show a half-updated desktop in between. This adds Avc444 and Avc444v2 variants that share one Avc444Tile struct, with the same content as send_avc444v2_frame takes: The LC encoding, the regions and data of the first sub-stream, and an optional second sub-stream. The second sub-stream is one Avc444SubStream value, so its regions and its data can't be present without each other. Both codecs are in the same change because RFX_AVC444V2_BITMAP_STREAM is identical on the wire to RFX_AVC444_BITMAP_STREAM except for how the client combines the two views (2.2.4.6), and the existing sender already shares one code path for them.

The regions stay separate per sub-stream, because each sub-stream is a full RFX_AVC420_BITMAP_STREAM with its own metablock (2.2.4.5). The tile is checked with the others before StartFrame is queued: AVC444 support in the negotiated capabilities, a second sub-stream exactly when LC is 0, no LC 3, and the region rules from #2000 applied to both sub-streams, including at least one region in each. One helper, avc_regions_refusal, now holds those rules for AVC420 tiles and for both AVC444 sub-streams, so the AVC420 arm from #2000 calls it as well. A frame that mixes an AVC tile, AVC420 or AVC444, with a tile of another codec needs capability version 10.4 or later, the floor #2000 sets for AVC420. I moved the stream-shape check and the construction of the WireToSurface1 PDU out of send_avc444_frame_with_encoding into private helpers, so the single-codec senders and the mixed path build the same bytes. destRect stays the bounding box of both sub-streams, since 2.2.2.1 makes it a bounding rectangle for the AVC codecs.

The doc comments say what the spec does and does not promise. 2.2.3.7 guarantees same-frame mixing only for AVC in YUV420 mode, so no capability version promises that a client decodes AVC444 next to other codecs, and callers should test their target clients. The Avc444Tile docs name CodecCapabilities::avc420_in_mixed_frames as the flag the floor reads, which callers can check beforehand with codec_capabilities(). The mixed path still holds AVC444 beside another codec to the same 10.4 floor as AVC420, since no version promises it at all. The single-codec AVC444 senders keep their contract and don't check regions, which their doc comments now say. An LC 2 update is combined with previously decoded luma (2.2.4.5), so it must not cover an area that another codec has repainted since that area's last luma update.

Tests cover a ClearCodec plus AVC444 frame for LC 0, 1 and 2 and for both codec IDs, checking PDU order, codec IDs, LC, per-sub-stream regions and destRect, and that the ClearCodec tile still decodes. They also cover rejection with nothing queued when AVC444 was not negotiated, when the sub-stream shape does not match LC, and when a region in either sub-stream breaks the region rules, including an empty region list. They also cover the version floor, where an AVC444 tile beside ClearCodec or AVC420 is refused on V10 to V10.3 and queued at 10.4, and a frame of AVC444 tiles alone is queued on an earlier version. The existing AVC444 and AVC444v2 sender tests pass, and the invalid stream shape test for the single-codec sender gained two cases for a lone sub-stream regions list or lone data. The xtask fmt, lints, tests, typos and locks checks pass.

Part of #1158.

@github-actions github-actions Bot added kind/protocol Affects RDP or related protocol behavior risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny scope/core Touches the core architectural tier size/L Size: up to 899 counted lines and 20 files; exceeds M in either measure triage/overlap Possible overlap with another pull request; advisory only labels Sep 25, 2026
@github-actions

github-actions Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

This pull request may overlap with #2000.

Both validate mixed-frame tiles before queuing: capability-gated AVC420 support, the 10.4 floor for AVC beside other codecs, per-tile region/rectangle checks returning None with nothing queued and trace-level logging. This PR additionally adds AVC444/444v2 tiles and QP/quality limits on top of that shared scope.

This notice is advisory only. Automated review continues as usual, and how these pull requests relate is for maintainers and authors to decide.

Note

LLM-assisted content (no human feedback).

@github-actions github-actions Bot added the needs-review A human reviewer is the current next actor label Sep 25, 2026
@CBenoit Benoît Cortier (CBenoit) added automation-failed Exact-head automated classification or review failed or was unavailable and removed needs-review A human reviewer is the current next actor labels Sep 30, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #2002 adds Avc444/Avc444v2 variants to MixedTilePayload, extracts shared avc444_stream_shape_is_valid/avc444_wire_pdu helpers so the single-codec senders and send_mixed_frame build identical bytes, and (bundling #2000) adds pre-queue tile validation with all-or-nothing semantics. I independently verified the LC stream-shape table (second sub-stream exactly for LUMA_AND_CHROMA, LC 3 rejected), QP<=51/quality<=100 checks, destRect as the bounding rectangle of both sub-streams, and the AVC420/AVC444 capability gating against the code; all are correct, and tests cover ordering, wire shape, and rejection with nothing queued. Five issues survive: Progressive tiles bypass the THINCLIENT constraint, #2000's changes are bundled into this diff, the refactored single-codec AVC444 sender still skips the region rules the mixed path enforces, the capability gates read as a mixing guarantee the docs explicitly disclaim, and two minor duplication nits in the send arms and test setup.

  1. [skeptical] Capability gates suggest a same-frame mixing guarantee the docs disclaim — low 🟡 ❓ — crates/ironrdp-egfx/src/server.rs
    The added docs state MS-RDPEGFX 2.2.3.7 makes no same-frame AVC mixing promise before capability version 10.4 and none at all for AVC444, yet send_mixed_frame accepts an AVC420 tile whenever supports_avc420() is true (including V8_1 clients with AVC420_ENABLED) and AVC444 tiles for any V10+ client without AVC_DISABLED. The supports_* checks read as a library-enforced safety gate while the docs put verification on the caller. Whether to gate mixing on the capability versions the docs invoke or only clarify the docs is a design decision needing maintainer intent; at minimum the supports_* call sites should state that passing them does not make same-frame mixing spec-guaranteed.

Comment thread crates/ironrdp-egfx/src/server.rs Outdated
Comment thread crates/ironrdp-egfx/src/server.rs Outdated
Comment thread crates/ironrdp-egfx/src/server.rs Outdated
Comment thread crates/ironrdp-egfx/src/server.rs Outdated
Comment thread crates/ironrdp-testsuite-core/tests/egfx/server.rs
@github-actions github-actions Bot added ai-reviewed/1 One automated review completed needs-author-action The pull request author is the current next actor and removed automation-failed Exact-head automated classification or review failed or was unavailable labels Sep 30, 2026
@github-actions github-actions Bot removed the needs-author-action The pull request author is the current next actor label Oct 7, 2026
@github-actions github-actions Bot added breaking-change Includes a breaking change, and requires special scrutiny at the boundaries risk/medium Behavioral change that does not substantially alter a core public API and removed risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny labels Oct 7, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR adds AVC444/AVC444v2 tiles to mixed-codec frames with shared Avc444Tile/avc444_wire_pdu helpers, a 10.4-floor capability gate for AVC-in-mixed-frames, per-tile region/QP/quality validation, and a 6-bit QP width check in QuantQuality::encode. The extraction from send_avc444_frame_with_encoding is behavior-preserving, the 10.4 gate matches MS-RDPEGFX 2.2.3.7-2.2.3.10 for AVC420, and tests cover the new paths well. Residual issues: the encode layer still admits qp/quality values 2.2.4.4.2 forbids; AVC444-beside-other-codec mixing rides an AVC420-scoped capability flag that no spec section backs for YUV444; invalid-tile refusals are indistinguishable from backpressure (trace-only); Avc444v2 tiles pass on any AVC444-capable version including a test on V10; the AVC420 arm duplicates avc444_regions_refusal; and Avc444Tile splits one optional sub-stream into two Options that must agree.

  1. [protocol] QuantQuality encode permits qp and qualityVal values MS-RDPEGFX 2.2.4.4.2 forbids — low 🟡 — crates/ironrdp-egfx/src/pdu/avc.rs
    The new check rejects only quantization_parameter above 63 (the 6-bit field width) and quality is written unvalidated, so Encode accepts qualityVal 101-255 and qp 52-63, both forbidden by MS-RDPEGFX 2.2.4.4.2 (qp 0-51 for 8-bit, qualityVal 0-100). The new test asserting quality 255 encodes successfully codifies a MUST-violating PDU. Low because every send path added or touched in this PR guards the values before encoding.
  2. [code-compressor] Avc444Tile splits one optional sub-stream into two Options that must agree — low 🟡 — crates/ironrdp-egfx/src/server.rs
    stream2_regions and stream2_data are only ever both-Some or both-None, and every consumer re-encodes that invariant: avc444_stream_shape_is_valid takes two bools and callers pass .is_some() twice, the validator rejects a mismatch, and avc444_tile_pdu must zip the Options, silently dropping a lone Some at encode time if validation ever missed it. Since Avc444Tile is newly introduced, folding them into a single Option of a sub-stream struct (regions plus data) removes one public field, the two-bool helper signature, the mismatch case, and the zip, while the untouched send_avc444v2_frame keeps its signature and converts at the tile boundary.

Comment thread crates/ironrdp-egfx/src/server.rs
Comment thread crates/ironrdp-egfx/src/server.rs
Comment thread crates/ironrdp-egfx/src/server.rs
Comment thread crates/ironrdp-egfx/src/server.rs Outdated
@github-actions github-actions Bot added ai-reviewed/2 Two automated reviews completed needs-author-action The pull request author is the current next actor and removed ai-reviewed/1 One automated review completed labels Oct 7, 2026
send_mixed_frame emitted every tile unchecked: AVC420 tiles went out
even with AVC disabled, and empty or out-of-surface rectangles reached
the client. FreeRDP 3.32.0 rejects those before decoding the frame,
which leaves the H.264 reference chain broken. Validate every tile
first, queue nothing if one fails, and log the reason at trace level.

A mix of AVC420 with another codec now needs the capability versions
that promise it (10.4 or later, MS-RDPEGFX 2.2.3.7), reported by the
new CodecCapabilities::avc420_in_mixed_frames. QuantQuality::encode
returns an error for a QP above 63 instead of panicking. Also record
backpressure in the mixed and Progressive senders like the others, and
replace an unsourced doc claim with what the spec guarantees.
@glamberson

Copy link
Copy Markdown
Contributor Author

Done. Avc444Tile now holds the second sub-stream as one Option<Avc444SubStream>, with its regions and its data together, so a tile can't have one without the other. The shape check takes a single flag and the zip is gone. The single-codec senders keep their signatures and reject a lone regions list or lone data before they build anything, and the test for invalid stream shapes now has a case for each.

@glamberson

Copy link
Copy Markdown
Contributor Author

QuantQuality::encode serializes the wire field, so it rejects only what the field can't hold, a QP above 63. The ranges in 2.2.4.4.2 are enforced on the mixed-frame path, which checks both before it encodes anything. QuantQuality::decode accepts any value, and the fuzz inputs are meant to round-trip through Encode, so rejecting a quality above 100 there would break that. The test with quality 255 checks the bit layout, because an all-ones quality byte shows the encoder writes the field unchanged. It doesn't claim 255 is a valid quality. The single-codec senders stay as they are, as I said on #2000.

A server that negotiated AVC444 could not put an AVC444 update in the
same frame as a lossless tile. Add Avc444 and Avc444v2 variants to
MixedTilePayload with the fields of send_avc444v2_frame, check them
with the other tiles before the frame is queued, and share the stream
shape check and PDU construction with the single-codec AVC444 senders.
@glamberson

Copy link
Copy Markdown
Contributor Author

Pushed. The second commit is restacked onto the current head of #2000, so the first commit is again the same copy as #2000's. The rest answers the second automated review. Avc444Tile now holds the second sub-stream as one Avc444SubStream, one avc_regions_refusal holds the region rules for AVC420 tiles and both AVC444 sub-streams, and the Avc444Tile docs name the flag callers can check for the 10.4 floor. The description is updated to match.

@github-actions github-actions Bot added needs-review A human reviewer is the current next actor and removed needs-author-action The pull request author is the current next actor labels Oct 8, 2026

@CBenoit Benoît Cortier (CBenoit) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@CBenoit
Benoît Cortier (CBenoit) merged commit 44704e8 into Devolutions:master Oct 8, 2026
43 checks passed
@github-actions github-actions Bot removed the needs-review A human reviewer is the current next actor label Oct 8, 2026

This branch was successfully deployed

1 active deployment
llm-providers — ceede21b Deployed Oct 8, 2026 by glamberson via Classify pull request #1861
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed/2 Two automated reviews completed breaking-change Includes a breaking change, and requires special scrutiny at the boundaries kind/protocol Affects RDP or related protocol behavior risk/medium Behavioral change that does not substantially alter a core public API scope/core Touches the core architectural tier size/L Size: up to 899 counted lines and 20 files; exceeds M in either measure triage/overlap Possible overlap with another pull request; advisory only

Development

Successfully merging this pull request may close these issues.

2 participants