fix(egfx): check mixed-frame tiles before queuing the frame

[Greg Lamberson (glamberson)]

send_mixed_frame queues StartFrame and then emits every tile without looking at it. An Avc420 tile goes out even when the negotiated capability set has AVC disabled, although send_avc420_frame refuses the same payload, and region rectangles are serialized as given. That matters in practice: A server that hands over an empty or out-of-surface region gets a frame the client can't use. FreeRDP 3.32.0 checks every region before it decodes (areRectsValid at the top of avc420_decompress in libfreerdp/codec/h264.c), so the H.264 access unit never reaches its decoder and the P-frames after it are predicted from a picture the decoder never received.

This change checks every tile first and returns None with nothing queued and no frame tracked if any tile fails, logging the reason at trace level. Avc420 tiles need AVC420 support in the negotiated capabilities, and a frame that mixes an Avc420 tile with another codec needs a capability set that promises it, which is version 10.4 or later without AVC_DISABLED (2.2.3.7, carried to 10.5 to 10.7 by 2.2.3.8 to 2.2.3.10). That rule is the new field CodecCapabilities::avc420_in_mixed_frames, derived with the other capability facts in from_capability_set, so send_mixed_frame now returns None for such a mix on a client below 10.4. A frame of Avc420 tiles alone isn't held to it. Each Avc420 tile needs at least one region, since a whole-surface default would cover the earlier ClearCodec and Avc420 tiles it overlaps. Each region must be non-empty, lie inside the surface, and carry a QP within the H.264 range that 2.2.4.4.2 refers to (0 to 51 for 8-bit video) and a quality of at most 100. ClearCodec destinations must be non-empty and inside the surface.

QuantQuality::encode used to panic on a QP above 63 and now returns an error. The single-codec senders still unwrap that encode, so they still panic on such a value. Refusing it there, or validating their regions, changes what they accept, so I left both out. #2002 adds the AVC444 tile variant to send_mixed_frame and builds on this change, so this one should merge first.

It also records backpressure in the QoE collector from send_mixed_frame and send_remotefx_progressive_frame, which were the only two senders that skipped it. The doc comment now cites what MS-RDPEGFX guarantees and no longer attributes the design to another product.

These are the first tests for send_mixed_frame: The PDU order and destination rectangles for a ClearCodec, Progressive and AVC420 frame, a rejection with an empty output queue for each check, the new field for every capability version, the QoE count for both senders, and the QP limit of QuantQuality::encode. The xtask fmt, lints, tests, typos and locks checks pass.

[github-actions]

The PR correctly adds pre-queue validation to send_mixed_frame (AVC420 capability gate, non-empty in-surface rectangles, QP <= 51, quality <= 100), QoE backpressure accounting for two senders, a spec-accurate doc rewrite, and the first tests for the API. Independently verified in the head tree: the mixed path still sends AVC420 alongside other codecs in one StartFrame/EndFrame for capability versions (V8_1, V10/V10_1/V10_2/V10_3) that the PR's own doc comment says make no same-frame AVC420 guarantee, an AVC420 tile with an empty regions list passes validation vacuously and encodes nRect = 0 with a full-surface fallback destination, and the QP > 63 panic the PR cites remains reachable via the unmodified single-codec senders. Published as one merged capability-gating finding, two verified residual findings, and one low-severity deduplication suggestion.

1. [skeptical] QP > 63 panic the PR cites remains reachable via send_avc420_frame and send_avc444_frame — medium 🟠 — crates/ironrdp-egfx/src/server.rs
   Verified in head: QuantQuality::encode (pdu/avc.rs line 45) calls set_bits(0..6, quantization_parameter), which panics for values above 63, and send_avc420_frame calls encode_avc420_bitmap_stream with no QP check (server.rs line 1474); the AVC444 path is similarly unchecked. The PR body motivates the new QP bound partly by this panic, but MAX_AVC_QP/MAX_AVC_QUALITY and rect_fits_surface only guard the mixed path, so the identical Avc420Region input that now returns None through send_mixed_frame still panics the server via the single-codec senders. Reusing the check there is a few lines; otherwise an explicit follow-up should be noted.
2. [code-compressor] Two new copies of the backpressure query-and-record triplet complete a seven-site duplication — low 🟡 — crates/ironrdp-egfx/src/server.rs
   Verified in head: the identical triplet `if self.should_backpressure() { self.qoe.record_backpressure(); return None; }` now appears at seven sender sites (1464, 1596, 1695, 1749, 1803, 1849, 1910), two added by this PR. A private helper such as `fn backpressured(&mut self) -> bool` that queries and records only when true is behavior-preserving and makes it impossible for a future sender to query without recording. Deduplicating the five pre-existing sites would widen the diff beyond the PR's scope, so this is a reasonable follow-up.

[Greg Lamberson (glamberson)]

The item in the review of 2026-09-30 about the QP panic staying reachable through the single-codec senders was right, and my description made it read as fixed. With a QP of 64, each of send_avc420_frame, send_avc444_frame and send_avc444v2_frame panics inside the call, at the set_bits in QuantQuality::encode. Only send_mixed_frame was protected, because it checks the QP before it encodes anything. QuantQuality::encode now returns an invalid field error for a QP above 63 instead of panicking. That's the root cause, and no value that encoded before changes. Two tests pin it. The largest QP the field holds encodes to the expected bits, and 64 and 255 return the error. encode_avc420_bitmap_stream returns a Vec<u8>, so it still panics on that error, and its # Panics section now says when. I didn't reuse the QP and quality checks in the three single-codec senders. They send QPs of 52 to 63 and qualities of 101 to 255 today, and refusing those would return None, which a caller can't tell from backpressure. That changes what the existing senders accept, so it belongs in a separate change, and until then they still panic on a QP above 63.

[Greg Lamberson (glamberson)]

Pushed as one commit rebased on current master. Beyond the QP change described above, the 10.4 rule that was a private helper is now the field CodecCapabilities::avc420_in_mixed_frames, set per capability version in from_capability_set and tested for every version. A frame refused for its tiles or the capabilities logs its reason at trace level. The AVC420 support check in the tile validation had no covering test, because the version rule refused first, so I added one. The doc comment about which tile wins an overlap is narrowed to ClearCodec and AVC420 tiles, since 3.3.5.2 doesn't order Progressive tiles (my reply on the empty-region thread had the broader version). The description now names the FreeRDP source for the client behavior, and the commit adds a test for the QoE count, links the spec sections and moves the table tests to rstest.

[Benoît Cortier (CBenoit)]

PR automation is failing because of a picky-krb 0.12.5 incompatibility, fixed on master by #2074. Please rebase on master to fix it.

Update: no rebase needed anymore. picky-krb 0.12.5 was yanked from crates.io (re-released as 0.13.0), so the API check builds again without changes to this branch. PR automation has been re-run here and passes.

[github-actions]

This pull request may overlap with #2002.

Both touch mixed-codec frame sending in ironrdp-egfx server.rs: this PR adds tile validation and the MS-RDPEGFX capability-version gate (avc420_in_mixed_frames) to send_mixed_frame, while #2002 extends the same send_mixed_frame/MixedTilePayload path with AVC444 tiles and explicitly builds on a tile-checks change matching this PR's 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]

The PR adds AVC420 conformance gating to send_mixed_frame (codec-mix capability rule via the new CodecCapabilities::avc420_in_mixed_frames field, per-tile support/region/QP/quality checks), converts the QuantQuality::encode QP panic into an error, records QoE backpressure for the two remaining senders, and adds the first send_mixed_frame tests. Independent inspection of pr-head confirms the checks behave as described and the spec readings match the protocol specialist's verification. Six candidates: four accepted as-is; the ambiguous-None finding is refined because the doc comment already discloses the refusal-vs-backpressure ambiguity, leaving the API-shape concern as the residual issue; the AVC420-only exemption below 10.4 is accepted as a low-severity open question since it preserves pre-PR behavior and the protocol specialist confirmed 2.2.3.7 promises only the codec mix.

1. [skeptical] send_mixed_frame signals permanent validation refusal through the same None as transient backpressure — medium 🟠 — crates/ironrdp-egfx/src/server.rs
   Tile/capability refusals return None exactly like not-ready, backpressure, and missing-surface cases. A caller whose payload is genuinely invalid (Avc420Region fields are plain u8; QuantQuality::encode itself accepts QP up to 63) retries forever and silently drops frames, unable to distinguish a permanent refusal from a transient one; only a trace log hints at the cause. The doc comment does disclose the ambiguity and the Option-based signature is uniform across all senders, which bounds the impact, but the API shape gives no way to surface a payload error. A distinct error result (or a documented variant) would let callers correct the payload instead of spinning on backpressure retries.
2. [code-compressor] New decode_drained_pdus helper leaves four pre-existing inline copies of the same decode dance — low 🟡 — crates/ironrdp-testsuite-core/tests/egfx/server.rs
   The PR adds decode_drained_pdus (drain, ZGFX-decompress, GfxPdu::decode) to deduplicate exactly the pattern that still sits inline at the avc420-shape test (~lines 173-183), the two avc444v2-shape loops (~233-243 and ~291-297), and the QoE backpressure test (~625-633). Reusing the helper there removes roughly 20 lines of repeated plumbing; the only differences are panic-message strings with no test value. The affected lines are not part of this diff, so no line range is given.

[Greg Lamberson (glamberson)]

The ambiguity is real, and the doc comment already says so. I'm leaving the return type alone because None already means several things across the crate. All eight senders return Option<u32>, and they use None for not being ready, for backpressure, for a surface that doesn't exist, and, in send_avc420_frame, for an AVC420 payload the negotiated capabilities don't support. The refusals this change adds work the same way, and their reason is logged at trace level. A distinct error on one sender would make it differ from the other seven, and changing all eight would break the public API of each, which is a decision about the crate's error shape and not something this fix should settle.

[Greg Lamberson (glamberson)]

Pushed. The change is the one described in the threads on lines 1982 and 1962. The AVC420 support check now runs once per frame, ahead of the codec-mix check, and the test for an AVC420 tile is written once. Nothing else in the diff changed.

[Greg Lamberson (glamberson)]

Closing this one, because it was merged as the first commit of #2002 (df3c1f9, which went in as 44704e8). Nothing in it is left to review. The threads are all answered, and the one part #2002 reworked, the mix rule in send_mixed_frame, is the version now on master.