Skip to content

fix(egfx): advertise only capability sets the client can decode - #1564

Merged
Benoît Cortier (CBenoit) merged 1 commit into
Devolutions:masterfrom
totoshko88:fix/egfx-advertise-only-decodable-caps
Aug 7, 2026
Merged

fix(egfx): advertise only capability sets the client can decode#1564
Benoît Cortier (CBenoit) merged 1 commit into
Devolutions:masterfrom
totoshko88:fix/egfx-advertise-only-decodable-caps

Conversation

@totoshko88

Copy link
Copy Markdown
Contributor

Summary

GraphicsPipelineHandler::capabilities() defaulted to advertising CapabilitySet::V10_7, which
tells the server AVC444 is available unless AVC_DISABLED is set. GraphicsPipelineClient has no
AVC444 decoder — handle_wire_to_surface1 routes Codec1Type::Avc444 and Avc444v2 to
on_unhandled_pdu. The server picks one of the advertised sets and prefers the most capable, so on
a host that supports AVC444 the client asked for a codec it then discarded: every frame of the
desktop was lost, the session looked frozen, and nothing reached on_bitmap_updated at all.

This drops V10.7 from the default. V8.1 keeps AVC420_ENABLED, which does decode, so no H.264
capability is lost, and V8 remains the no-AVC fallback. V10.7 can come back the moment AVC444
decodes — the doc comment and the new test both say so.

Closes #1563. This is item 5 of the #1464 tracking list ("advertise only what we can decode", which
already names AVC444 as the example), addressed for the AVC444 case only.

Why the smaller set is not a downgrade

set AVC420 AVC444 decodable today
V10.7 (removed) yes yes no — AVC444 has no decoder
V8.1 AVC420_ENABLED yes no yes, via decode_avc420
V8 no no yes (Uncompressed, ClearCodec, Planar)

An alternative that keeps V10.x is to advertise it with AVC_DISABLED set, but per
CodecCapabilities::from_capability_set that flag clears avc420 as well, so it gives up H.264
entirely. V8.1 is the better default while AVC444 is missing.

Changes

  • crates/ironrdp-egfx/src/client.rs — remove V10_7 from the default capabilities(); document
    why V10.x is absent and what has to happen before it returns.
  • Two unit tests: the advertised sets imply no AVC444, and AVC420 stays advertised. They assert
    against CodecCapabilities::from_capability_set rather than naming versions, so they keep holding
    if the set changes shape.
  • crates/ironrdp-testsuite-core/tests/egfx/client.rsclient_keeps_avc_caps_with_decoder now
    expects two sets instead of three.

Validation

Reproduced and measured against Windows 11 Pro 25H2 (build 26200.8875), no GPU (WARP software
rendering), same LAN, base tcp rtt 5 ms. Server-side counters sampled with
Get-Counter "\RemoteFX Graphics(*)\*" -SampleInterval 2 -MaxSamples 12 while interacting:

client session res output fps avg/max avg encoding time
default caps, AVC444 selected 3360x1930 0.00 / 0.00 4.50 ms
FreeRDP sdl-freerdp, same host, minutes apart 3360x1930 9.31 / 24.00 5.25 ms

frames skipped/second — insufficient server resources and — insufficient client resources were
both 0.00 and guest CPU stayed at 1–3%: the server was encoding and not throttling. FreeRDP's
near-identical encoding time on the same pipeline confirms the server side was healthy, which is
what places the fault in the client's advertisement.

Local checks, all clean:

  • cargo xtask check fmt -v
  • cargo xtask check lints -v
  • cargo xtask check tests -v — 22 suites, 0 failed
  • cargo clippy -p ironrdp-egfx --all-targets --all-features — 0 warnings
  • cargo doc -p ironrdp-egfx --no-deps — the two remaining warnings (THIRD_PARTY_NOTICES,
    compositor → private Compositor) predate this branch

cargo xtask check typos -v was skipped: typos-cli is not installed in this environment.

Deliberately not in this PR

The default `GraphicsPipelineHandler::capabilities()` advertised V10.7,
which signals AVC444 support unless `AVC_DISABLED` is set. The client has
no AVC444 decoder: `handle_wire_to_surface1` routes `Avc444` and
`Avc444v2` to `on_unhandled_pdu`. Since the server picks one of the
advertised sets and prefers the most capable, a host that supports AVC444
sent every frame in a codec the client discards, leaving a blank screen
with no error and nothing reaching `on_bitmap_updated`.

Drop V10.7 from the default. V8.1 keeps AVC420, which does decode, so no
H.264 capability is lost, and V8 remains the no-AVC fallback. V10.7 can
return once AVC444 decodes.

Reproduced against Windows 11 Pro 25H2 (build 26200.8875): server-side
`RemoteFX Graphics` counters read 0.00 output frames/second while FreeRDP
on the same host and link sustained 9.31 with comparable encoding time.

Adds two unit tests asserting the advertised sets imply no AVC444 and
still enable AVC420, and updates the workspace testsuite expectation.

closes Devolutions#1563
@github-actions github-actions Bot added maintainer-required Maintainer review or intervention is required risk/unknown scope/core Touches the core architectural tier size/S Size: 30-149 lines of code labels Aug 7, 2026
Benoît Cortier (CBenoit) added a commit that referenced this pull request Aug 7, 2026
A valid classifier result for PR #1564 was discarded because structured
output represented an empty string as the literal text `""`. That left
the PR at `risk/unknown` and hid the concrete validation failure from
maintainers.

This change canonicalizes that exact empty-string artifact in the shared
text normalizer used by classifier, protocol-analysis, and reviewer
outputs. Required prose still fails closed, while optional empty fields
normalize consistently. Classification failures now publish a neutral
SHA-bound `AI classification` check containing the precise validator or
prerequisite reason, without opening the automated review gate.

Regression coverage includes the PR #1564 payload, reviewer and protocol
text behavior, and neutral diagnostic check publication.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@github-actions github-actions Bot added breaking-change Includes a breaking change, and requires special scrutiny at the boundaries kind/protocol Changes how we encode/decode or interpret RDP wire packets risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny and removed risk/unknown labels Aug 7, 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.

LGTM!

@CBenoit
Benoît Cortier (CBenoit) merged commit b7657bc into Devolutions:master Aug 7, 2026
59 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change Includes a breaking change, and requires special scrutiny at the boundaries kind/protocol Changes how we encode/decode or interpret RDP wire packets maintainer-required Maintainer review or intervention is required risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny scope/core Touches the core architectural tier size/S Size: 30-149 lines of code

Development

Successfully merging this pull request may close these issues.

egfx client advertises AVC444 it cannot decode, so frames are dropped

2 participants