feat(egfx): render RFX Progressive updates - #1676
feat(egfx): render RFX Progressive updates#1676Marc-André Moreau (mamoreau-devolutions) wants to merge 1 commit into
Conversation
592ea70 to
dd68471
Compare
c3aac01 to
a684f22
Compare
a684f22 to
e5d3a33
Compare
There was a problem hiding this comment.
Wiring of RFX Progressive decode into WireToSurface2 is sound: unknown-surface is rejected before decode, tile placement/cropping reuses the existing crop_decoded_frame helper and existing BitmapUpdate/compositor conventions (mirrors handle_uncompressed's Uncompressed codec_id + apply_bitmap pattern), and context lifecycle (delete on DeleteEncodingContext/DeleteSurface/ResetGraphics) is exercised by new tests. No correctness, safety, or scope defects found. Two minor, pre-existing-adjacent issues remain: a spec-citation error on the touched on_wire_to_surface2 doc comment, and no test for edge tiles on non-64-aligned surfaces.
Protocol analysis: partially_accepted — The doc-citation conflict (3.3.5.3 vs correct 3.3.5.2 for on_wire_to_surface2) is confirmed: surrounding lines were edited by this diff while the wrong citation itself was left untouched, a real but non-blocking nit. The 'conforms' assessments (surface validation, delete_context, tile-grid math) check out. The ResetGraphics/progressive_decoder.reset() concern is downgraded to a question: the handoff's own corpus lookup was inconclusive on whether context teardown is mandated, and clearing progressive state alongside the pre-existing surfaces.clear() is the defensible, conservative choice, consistent with ResetGraphics already tearing down surface-scoped state.
- non_blocking / low — crates/ironrdp-egfx/src/client.rs
The on_wire_to_surface2 doc comment was edited by this PR (new summary/implementation-hint lines added) but the adjacent 'Per [MS-RDPEGFX 3.3.5.3]' citation was left untouched and is wrong: 3.3.5.3 is 'Processing an RDPGFX_DELETE_ENCODING_CONTEXT_PDU message', while the correct section for RDPGFX_WIRE_TO_SURFACE_PDU_2 processing is 3.3.5.2. Since this PR is the one wiring real production behavior into this exact handler, it is a reasonable place to correct the citation while touching the comment block. - non_blocking / low — crates/ironrdp-egfx/src/client.rs
The only new test exercising handle_wire_to_surface2's rendering path uses a 64x64 surface, so every tile is exactly 64x64 and the crop_decoded_frame branch (taken when a surface's dimensions aren't multiples of 64 or a tile sits at the trailing edge) is never exercised by a test, only by the pre-existing generic crop_decoded_frame unit tests. A surface sized e.g. 100x100 would be a good addition to confirm edge tiles are cropped correctly.
| if let Some(ref mut decoder) = self.h264_decoder { | ||
| decoder.reset(); | ||
| } | ||
| self.progressive_decoder.reset(); |
There was a problem hiding this comment.
question / low: handle_reset_graphics now clears progressive_decoder contexts in addition to the pre-existing surfaces.clear(). The local MS-RDPEGFX 3.3.5.14 text located only mandates resizing the Graphics Output Buffer, not destroying codec/tile state, so this cannot be independently confirmed as spec-required versus merely a safe, conservative choice consistent with surfaces already being torn down. Worth a maintainer confirmation that no server relies on progressive tile state surviving a ResetGraphics.
6f440ce to
fe50d96
Compare
e5d3a33 to
f23db31
Compare
There was a problem hiding this comment.
Small, well-scoped wiring change: routes WireToSurface2's RFX Progressive bitmap data through the pre-existing ProgressiveDecoder into the compositor, plus lifecycle cleanup on DeleteEncodingContext/DeleteSurface/ResetGraphics. Tile-placement math, codec dispatch (only one Codec2Type variant exists), and surface-bounds handling were independently verified as correct and consistent with existing WireToSurface1 patterns; new tests cover the happy path and all three lifecycle-clearing transitions. The main substantive concern, independently confirmed against progressive.rs, is that this PR is the first to make an existing, incomplete RFX_TILE_DIFFERENCE code path reachable from live traffic — a real gap, but in an unchanged file and of uncertain real-world impact, so it is raised as a question rather than a blocker. No public API or architectural-boundary issues found.
Protocol analysis: partially_accepted — Re-derived from progressive.rs: RFX_TILE_DIFFERENCE is indeed never consulted (is_difference always false, coefficients always overwritten not accumulated), confirming the handoff's core discrepancy. The DeleteEncodingContext/Sub-Band-Diffing-context lifetime conflation is also confirmed, but downgraded in severity: since difference-tile reconstruction isn't implemented at all, no path currently reuses the discarded coefficient history, so it's folded in as a low-severity, contingent note. The 'conforms' assessments (tile placement, codec dispatch, cleanup on Reset/DeleteSurface) were re-verified against cited spec sections and held up without qualification.
- non_blocking / low — crates/ironrdp-egfx/src/client.rs
DeleteEncodingContext discards ProgressiveDecoder's entire per-(surface_id, codec_context_id) tile grid, including accumulated DecDwtQ coefficients. MS-RDPEGFX 3.3.1.1 scopes Codec Contexts to per-context deletion (matches this call), but 3.3.1.3 scopes Sub-Band Diffing Tile Contexts to the surface, requiring they persist across codec-context deletion until the surface is removed. Currently inert because RFX_TILE_DIFFERENCE reconstruction isn't implemented at all (see sibling finding), so no live path would reuse the discarded history yet. Worth tracking alongside that gap rather than fixing in isolation now. - non_blocking / low — crates/ironrdp-egfx/src/client.rs
The new tests cover the happy-path tile render and all three lifecycle-clearing transitions (ResetGraphics, DeleteEncodingContext, DeleteSurface) well, all on a 64x64 surface matching exactly one tile. Missing: a surface whose dimensions aren't a multiple of 64 (exercising the edge-tile cropping saturating_sub/min logic), and a malformed x_idx/y_idx or quant index that should surface TileOutOfBounds/InvalidQuantIndex as a decode error. Both paths look correct by inspection, but a regression there would only be caught by manual review until covered.
| .map_err(|error| { | ||
| warn!(?error, "rfx progressive decode failed"); | ||
| pdu_other_err!("rfx progressive decode failed") | ||
| })?; |
There was a problem hiding this comment.
question / medium: This method is the first call site that makes ProgressiveDecoder::decode_bitmap reachable from live server traffic. Verified against ironrdp-graphics/src/progressive.rs (unchanged, outside this diff): decode_first/decode_tile_block never inspect the tile's RFX_TILE_DIFFERENCE flag (TileState.is_difference is always false; decode_first_pass_to_dwtq always overwrites coefficients rather than adding a delta onto prior DecDwtQ). A server sending a difference-flagged tile will have that delta rendered as if it were the full tile, producing corrupted pixels once delivered via this new wiring to on_bitmap_updated. This is a pre-existing gap in an unmodified file, not introduced by this diff, but this PR is what makes it observable in production for the first time, and the empty PR description gives no indication whether that's a known/accepted limitation or an oversight worth a guard or follow-up.
No description provided.