Repository navigation
feat(egfx): add AVC444 tiles to mixed-codec frames - #2002
Benoît Cortier (CBenoit) merged 2 commits into
Conversation
|
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). |
There was a problem hiding this comment.
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.
- [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.
7ea7a9c to
290ac8b
Compare
290ac8b to
0067483
Compare
There was a problem hiding this comment.
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.
- [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. - [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.
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.
|
Done. |
|
|
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.
0067483 to
ceede21
Compare
|
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. |
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.