fix(egfx): draw AVC420 partial updates from regionRects (#2042) - #2052
Benoît Cortier (CBenoit) merged 3 commits into
Conversation
…2042) decode_avc420 ignored Avc420BitmapStream.rectangles and crop_decoded_frame always copied a single bounding-box block from the decoded frame origin (0,0). A partial AVC420 update whose region did not sit at the origin was therefore blitted with pixels from the top-left corner, and the gaps between regions were overwritten -- the 'horizontal lines + unfilled rectangle outlines' artifact. Iterate regionRects (MS-RDPEGFX 2.2.4.4): clip each rect to surface-intersect- frame, copy pixels (x,y)->(x,y) via new copy_frame_region helper, and emit one surface update per rectangle. Streams with no regionRects keep the previous single bounding-box path. Adds a deterministic-decoder unit test that fails before the fix (1 origin-cropped update) and passes after (per-region updates drawn from matching coordinates). Co-fix based on the region-rects harness/recipe proposed by issue author se-wo. Refs: Devolutions#2042
GitHub Actions
Active
github-actions
Bot
added
the
needs-review
github-actions
Bot
added
kind/protocol
GitHub Actions
Active
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The AVC420 fix is correct: it honors MS-RDPEGFX 2.2.4.4 regionRects, emitting one surface update per region copied from its own (x,y) in the decoded frame, with a safe fallback for streams that omit regionRects. The surface-absolute coordinate interpretation is corroborated by MS-RDPEGFX 2.2.2.1 (destRect is the bounding rectangle of regionRects, so rects cannot be destRect-relative) and by this repo's own server encoder (server.rs compute_dest_rect + Avc420Region), which emits regions in frame coordinates with destRect as their bounding box. Clipping to surface and frame bounds is defensive and correct; malformed or inverted rects are skipped without out-of-bounds reads. Remaining candidates are minor: a test-coverage gap (no test with a non-zero dest_rect origin), silent skipping of clipped-away regions, and three small deduplication/simplification opportunities in the new helper, emit block, and test assertions. No correctness or safety defects were found in the added code.
- [code-compressor] Fallback branch and per-region loop duplicate the BitmapUpdate build + emit block — low 🟡 — crates/ironrdp-egfx/src/client.rs
The no-regionRects fallback (1016-1025) and the loop body (1054-1063) both construct the same BitmapUpdate from a rectangle and emit it via compositor.apply_bitmap followed by handler.on_bitmap_updated; only the data source (origin crop vs same-coordinate copy) differs. A small local (rect, data) helper or closure that derives width/height from the rect -- matching the existing emit_update closure pattern in the uncompressed path at line 910 -- removes the duplicated emit shape with identical behavior.
| for region in &stream.rectangles { | ||
| // Clip the region to the surface and the decoded frame so a | ||
| // malformed or over-large rect cannot read out of bounds or paint | ||
| // outside the surface. | ||
| let frame_w = u16::try_from(frame.width()).unwrap_or(u16::MAX); | ||
| let frame_h = u16::try_from(frame.height()).unwrap_or(u16::MAX); | ||
| let clip_right = region.right.min(surface_bounds.0).min(frame_w); | ||
| let clip_bottom = region.bottom.min(surface_bounds.1).min(frame_h); | ||
| if region.left >= clip_right || region.top >= clip_bottom { | ||
| // Empty after clipping -- skip. | ||
| continue; | ||
| } | ||
| let clipped = ExclusiveRectangle { | ||
| left: region.left, | ||
| top: region.top, | ||
| right: clip_right, | ||
| bottom: clip_bottom, | ||
| }; |
There was a problem hiding this comment.
[skeptical] No test exercises the surface-absolute regionRect interpretation with a non-zero dest_rect origin — low 🟡 — The loop treats each regionRect as both the surface destination and the decoded-frame source offset. That interpretation is well-founded: MS-RDPEGFX 2.2.2.1 defines the AVC420 destRect as the bounding rectangle of regionRects (implying shared, surface/frame-absolute coordinates), and IronRDP's own server encoder (server.rs compute_dest_rect with Avc420Region coordinates) emits exactly that shape. However, the new test uses dest_rect (0,0,48,48), where surface-absolute and destRect-relative interpretations coincide, so nothing in-tree pins the convention or would catch a regression to destRect-relative placement. Add a test whose dest_rect.left/top and regionRect origins are both non-zero to lock in the correct interpretation.
| if region.left >= clip_right || region.top >= clip_bottom { | ||
| // Empty after clipping -- skip. | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[skeptical] Regions dropped by clipping are skipped without any diagnostic — low 🟡 — A region that becomes empty after clipping to surface/frame bounds is silently skipped. If a server's coordinate convention or rect bounds mismatch expectations, the result is permanently stale screen areas with zero log output -- the same class of rendering defect this PR fixes, again undebuggable in the field. Emit at least a trace!/warn! with the surface id, region rect, and clip bounds when a region is skipped.
| fn copy_frame_region(frame_data: &[u8], frame_width: u32, region: &ExclusiveRectangle) -> Vec<u8> { | ||
| const BYTES_PER_PIXEL: usize = 4; | ||
|
|
||
| let src_stride = usize::try_from(frame_width) | ||
| .unwrap_or(usize::MAX) | ||
| .saturating_mul(BYTES_PER_PIXEL); | ||
| let region_width = usize::from(region.right - region.left); | ||
| let region_height = usize::from(region.bottom - region.top); | ||
| let row_bytes = region_width.saturating_mul(BYTES_PER_PIXEL); | ||
| let left_off = usize::from(region.left).saturating_mul(BYTES_PER_PIXEL); | ||
|
|
||
| let mut out = Vec::with_capacity(row_bytes.saturating_mul(region_height)); | ||
| for row in 0..region_height { | ||
| let src_row = usize::from(region.top).saturating_add(row); | ||
| let src_start = src_row.saturating_mul(src_stride).saturating_add(left_off); | ||
| let src_end = src_start.saturating_add(row_bytes); | ||
| if src_end <= frame_data.len() { | ||
| out.extend_from_slice(&frame_data[src_start..src_end]); | ||
| } else { | ||
| // Defensive: the caller clips to frame bounds, so this should not | ||
| // happen, but never read out of bounds if it does. | ||
| break; | ||
| } | ||
| } | ||
| out | ||
| } |
There was a problem hiding this comment.
[code-compressor] copy_frame_region duplicates the row-copy loop already in crop_decoded_frame — low 🟡 — copy_frame_region at an origin rect performs exactly the copy crop_decoded_frame's loop does: identical stride arithmetic and per-row bounds handling. Keep crop_decoded_frame's zero-dimension guard, fast path, and truncation warning, but delegate its body to copy_frame_region with an origin rect so there is one RGBA row-copy implementation to maintain (~20 lines of duplicated saturating-arithmetic code removed). This slightly touches a function the diff otherwise leaves alone (crop_decoded_frame is also used by the uncompressed path), but the consolidation is worth it.
| for (codec_id, width, height, data) in updates.iter() { | ||
| assert_eq!(*codec_id, Codec1Type::Avc420); | ||
| assert_eq!(*width, 16, "each region is 16 wide"); | ||
| assert_eq!(*height, 16, "each region is 16 tall"); | ||
| let (r, g) = top_left(data); | ||
| // The CoordinateDecoder stamps R=x, G=y at each pixel. The top-left | ||
| // pixel of a region drawn correctly must carry that region's own | ||
| // (left, top). Region B lives at (32, 32); if the fix copies from the | ||
| // frame origin, this would be (0, 0) instead. | ||
| assert!( | ||
| (r, g) == origin_rg(&rect_a) || (r, g) == origin_rg(&rect_b), | ||
| "region top-left pixel ({r}, {g}) does not match either region origin; \ | ||
| pixels were copied from the wrong source coordinate (issue #2042)" | ||
| ); | ||
| } | ||
|
|
||
| // Specifically prove region B is present with its correct origin pixel. | ||
| let has_region_b = updates | ||
| .iter() | ||
| .any(|(_, _, _, data)| top_left(data) == origin_rg(&rect_b)); | ||
| assert!( | ||
| has_region_b, | ||
| "region B at (32, 32) was not drawn from its own coordinates -- \ | ||
| AVC420 partial update took pixels from the frame origin (issue #2042)" | ||
| ); | ||
| } |
There was a problem hiding this comment.
[code-compressor] Test pairs a disjunctive per-update assert with has_region_b; two any() asserts are shorter and strictly stronger — low 🟡 — The per-update loop asserts each update's top-left pixel equals rect_a's or rect_b's origin, then a separate has_region_b block proves region B is present. Two independent any() assertions -- one per region origin -- replace both: they are ~10 lines shorter and strictly stronger because they directly prove both regions are present with their own origin pixels (the current pair cannot distinguish one-of-each from both-are-A without the second check). Pass/fail behavior on fixed and pre-fix code is preserved.
…onRects fix - dedup the BitmapUpdate build+emit into one emit_avc420 closure, shared by the no-regionRects fallback and the per-region loop (mirrors emit_update) - delegate crop_decoded_frame's row copy to copy_frame_region at an origin rect, so there is one RGBA row-copy implementation; keep the zero guard, equal-dims fast path and truncation warning - warn! when a regionRect clips away to nothing instead of skipping silently, logging surface id, region rect and clip bounds - add a test with a non-zero dest_rect origin and non-zero region origins to pin the surface-absolute coordinate convention (a destRect-relative regression would now fail)
GitHub Actions
Active
github-actions
Bot
added
size/L
|
Thank you for the fix, and to se-wo for the analysis behind it. The server I maintain, lamco-rdp-server, depends on the convention this PR implements: Its mixed frames carry one AVC420 tile with a whole-surface picture, the changed rectangles as regionRects in surface coordinates and destRect as their bounding box, followed by ClearCodec tiles. I ran that frame shape through IronRDP's send_mixed_frame into the client with the OpenH264 decoder, on master (cdea64d) and on 58da2a3. On master, a rectangle at (32,32)-(64,64) shows the picture's black top-left corner instead of its white bottom-right quadrant, and two corner rectangles overwrite the gap between them with black. On 58da2a3 both are correct, and ClearCodec tiles sent after the H.264 tile still win where they meet. The pictures came from IronRDP's OpenH264 encoder, not lamco-rdp-server's own encoder, so this covers the frame shape and not our encoder output. |
Benoît Cortier (CBenoit)
left a comment
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
Thank you for the fix, LGTM, I’ll merge as-is
9f07fd4
into
Devolutions:master
This branch was successfully deployed
3 participants
AVC420 partial frame updates paint the wrong pixels. On the client decode path,
decode_avc420decoded theAvc420BitmapStreambut only ever usedstream.data— theregionRectsinstream.rectangles(theRFX_AVC420_METABLOCKfrom [MS-RDPEGFX] 2.2.4.4) were never read.crop_decoded_framethen copied a single block from the decoded frame's origin(0,0)and blitted it across the whole destination rectangle.So whenever a frame's changed regions did not happen to sit at the top-left corner, each region was filled with pixels lifted from
(0,0), and the space between regions got overwritten. That is the "horizontal lines and unfilled rectangle outlines" people see in GFX / H.264 (AVC420) mode once the server starts sending partial updates instead of full frames.The fix follows the spec:
regionRectsare the sub-regions that actually changed, each one takes its pixels from the same(x, y)in the decoded frame, and the PDU's destination rectangle is just their bounding box — not a copy source.decode_avc420now walksstream.rectangles, clips each rect tosurface ∩ frame, copies(x, y) → (x, y)through a small newcopy_frame_regionhelper, and emits one surface update per rectangle. A stream that carries noregionRectskeeps the old single bounding-box path, so nothing changes for that case.The change is deliberately narrow — only the client decode path in
crates/ironrdp-egfx/src/client.rs. AVC444, the server encode path, and the NAL-format work in #1986 are all left alone.For the test, a deterministic stand-in decoder stamps every pixel with its own coordinate (
R = x,G = y), so a test can tell which source pixel actually landed where. It sends an AVC420 update with two disjoint regions, neither at the origin, whose bounding box is the destination rectangle, and checks that each region is drawn from its own coordinates. Before the fix it fails with a single origin-cropped update where two were expected; after the fix it gets one update per region, with region B at(32, 32)carrying its own pixels.cargo test -p ironrdp-egfxpasses (54 tests) andcargo clippy -p ironrdp-egfx --all-targetsis clean.This one is a co-fix. The root-cause analysis and the region-rects harness/recipe (
c06_avc420_region_rects.rs) came from the issue author, se-wo; this PR implements that recipe on the client decode path and turns the harness into an in-tree unit test. Glad to fold in se-wo's original harness verbatim or adjust the attribution however maintainers prefer.Refs: #2042