From 1a808de66bd96efd9c2712cc1b6deda977df6425 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=A8=E6=88=90=E9=94=B4?= Date: Wed, 16 Sep 2026 17:43:13 +0800 Subject: [PATCH 1/7] fix(egfx): stop resize and reactivation from shredding the desktop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Display Control resize or a Ctrl+Alt+Del left the desktop torn: blocks frozen on the previous image, regions that never refreshed again. Three separate faults, all of them cases where the client was stricter than the protocol and stricter than what Windows actually sends. ResetGraphics dropped the EGFX bitmap cache. Cache slots are not surfaces: they are connection-scoped, and MS-RDPEGFX 3.3.5.14 redefines only the output buffer, so they survive a reset. Windows depends on it, restoring the desktop almost entirely from slots filled before the reset. Dropping them made every one of those CacheToSurface blits a silent no-op. SRL decoding demanded a trailing zero byte and capped a zero run at one tile's worth of coefficients. Windows sends neither: the encoder stops emitting bits once the rest of a band is zero, and static regions produce runs far longer than the cap. Both faults discarded whole tile updates, which is what surfaced as blocks that never refresh. The stream is now read zero-padded past its end and a run is consumed one event at a time, the way FreeRDP's progressive_rfx_srl_read does. Over-reads are counted so a real desync still shows up in the logs. ResetGraphics also kept the progressive difference-tile references owned by the surfaces it implicitly destroys, so a reused surface id differenced against the old desktop. Those go; the CONTEXT and the ClearCodec glyph cache stay, because the server will not re-send them. With the cache surviving, the session framebuffer has to follow the new output before the compositor deltas from the same payload are applied — the server will not send them twice. GraphicsPipelineClient::take_reset_graphics reports the size for that, including same-size resets, since those still destroy every surface. Also implements core::error::Error for ProgressiveDecodeError and includes the inner PDU error in SessionErrorKind::Pdu, so a decode failure is diagnosable instead of collapsing to "PDU error". --- crates/ironrdp-egfx/src/client.rs | 117 ++++++++++- crates/ironrdp-egfx/src/compositor.rs | 54 ++++- crates/ironrdp-graphics/src/progressive.rs | 62 +++++- crates/ironrdp-graphics/src/srl.rs | 173 +++++++++++----- crates/ironrdp-session/src/lib.rs | 2 +- .../tests/egfx/wire_to_surface_real_world.rs | 186 ++++++++++++++++++ 6 files changed, 527 insertions(+), 67 deletions(-) diff --git a/crates/ironrdp-egfx/src/client.rs b/crates/ironrdp-egfx/src/client.rs index a9a471c6ec..5eee593ee1 100644 --- a/crates/ironrdp-egfx/src/client.rs +++ b/crates/ironrdp-egfx/src/client.rs @@ -502,6 +502,13 @@ impl GraphicsPipelineClient { /// /// The returned dimensions satisfy the protocol's output limit and the /// compositor's output-framebuffer allocation limit and are reported once. + /// + /// The framebuffer must follow this size *before* the compositor deltas drained by + /// [`Self::drain_output`] are applied, because a reset and the deltas that repaint the + /// new output arrive in the same payload and the server will not send them again. + /// + /// A same-size reset is still reported when valid. Per MS-RDPEGFX 3.3.5.14 the reset + /// destroys every surface even when the output dimensions are unchanged. #[must_use] pub fn take_output_reset(&mut self) -> Option<(u16, u16)> { self.pending_output_reset.take() @@ -694,6 +701,13 @@ impl GraphicsPipelineClient { let output_size = Compositor::materializable_output_size(width, height); // Per spec, ResetGraphics implicitly destroys all surfaces + let surface_count = self.surfaces.len(); + // Tile coefficient buffers belong to those surfaces. Keeping them lets a + // later surface reuse the same id and difference against the previous + // desktop, which decodes as torn or duplicated tiles. + // CONTEXT / ClearCodec glyph cache stay: 3.3.5.14 only redefines the + // output buffer, and Windows will not re-send SYNC + CONTEXT. + self.progressive_decoder.clear_tile_references(); self.surfaces.clear(); self.compositor.reset(width, height); self.pending_output_reset = output_size; @@ -722,8 +736,14 @@ impl GraphicsPipelineClient { // drop the glyph cache, so a legitimate post-reset GLYPH_HIT would fail unless the // server redundantly re-sent every glyph. + debug!( + width, + height, + surface_count, + "ResetGraphics: surfaces destroyed; tile refs dropped; progressive CONTEXT retained" + ); + if output_size.is_some() { - debug!(width, height, "Graphics reset"); self.handler.on_reset_graphics(width, height); Ok(()) } else { @@ -757,13 +777,23 @@ impl GraphicsPipelineClient { } fn handle_delete_surface(&mut self, surface_id: u16) { + // MS-RDPEGFX: deleting a surface drops that surface's progressive tile + // references. A following difference tile before a new base tile will + // correctly fail with MissingTileReference. + let cleared_refs = self.progressive_decoder.reference_count_for_surface(surface_id); self.progressive_decoder.delete_surface(surface_id); if self.surfaces.remove(&surface_id).is_some() { self.compositor.delete_surface(surface_id); - debug!(surface_id, "Surface deleted"); + debug!( + surface_id, + cleared_refs, "DeleteSurface cleared progressive tile references" + ); self.handler.on_surface_deleted(surface_id); } else { - warn!(surface_id, "DeleteSurface for unknown surface"); + warn!( + surface_id, + cleared_refs, "DeleteSurface for unknown surface (progressive refs cleared)" + ); } } @@ -1424,6 +1454,25 @@ mod tests { pixel_format: PixelFormat::XRgb, })) .unwrap(); + // Give the surface content first: mapping a surface that has never been painted + // publishes nothing, so the delta asserted below would not exist. + client + .handle_pdu(GfxPdu::SolidFill(SolidFillPdu { + surface_id: 1, + fill_pixel: crate::pdu::Color { + b: 0x33, + g: 0x22, + r: 0x11, + xa: 0, + }, + rectangles: vec![ExclusiveRectangle { + left: 0, + top: 0, + right: 2, + bottom: 2, + }], + })) + .unwrap(); client .handle_pdu(GfxPdu::MapSurfaceToScaledOutput(MapSurfaceToScaledOutputPdu { surface_id: 1, @@ -1717,6 +1766,33 @@ mod tests { assert_eq!(client.frames_queued, 0, "frame queue should be reset"); } + /// Same-size `ResetGraphics` must still be observable: it destroys every surface, so a + /// consumer has to re-blit even when the output dimensions did not change. + #[test] + fn take_reset_graphics_reports_same_size_resets() { + let mut client = GraphicsPipelineClient::new(Box::new(TestHandler), None); + assert!(client.take_reset_graphics().is_none()); + + let _ = client.handle_pdu(GfxPdu::ResetGraphics(crate::pdu::ResetGraphicsPdu { + width: 800, + height: 600, + monitors: vec![], + })); + assert_eq!(client.take_reset_graphics(), Some((800, 600))); + assert!(client.take_reset_graphics().is_none(), "flag is one-shot"); + + let _ = client.handle_pdu(GfxPdu::ResetGraphics(crate::pdu::ResetGraphicsPdu { + width: 800, + height: 600, + monitors: vec![], + })); + assert_eq!( + client.take_reset_graphics(), + Some((800, 600)), + "same-size ResetGraphics must still signal a full client re-blit" + ); + } + #[test] fn crop_decoded_frame_identity() { let data = vec![0xFFu8; 4 * 4 * 4]; @@ -2245,6 +2321,41 @@ mod tests { assert!(wire_progressive(&mut client, progressive_context_stream(false)).is_ok()); } + #[test] + fn reset_graphics_drops_tile_references_of_implicitly_destroyed_surfaces() { + let mut client = progressive_client(); + wire_progressive(&mut client, progressive_context_stream(true)).unwrap(); + wire_progressive(&mut client, progressive_tile_stream(0, 0, 64, 64)).unwrap(); + assert!( + client.progressive_decoder.total_reference_count() > 0, + "first-pass tile must leave a difference reference" + ); + + client + .handle_pdu(GfxPdu::ResetGraphics(crate::pdu::ResetGraphicsPdu { + width: 128, + height: 96, + monitors: vec![], + })) + .unwrap(); + + // MS-RDPEGFX 2.2.2.14 / 3.3.5.14: ResetGraphics destroys every surface. + // Tile coefficient buffers belong to those surfaces. Reusing surface id 0 + // after a Display Control resize must not difference against the old + // desktop — that is the shredded frame native RDP does not produce. + assert_eq!(client.progressive_decoder.total_reference_count(), 0); + + client + .handle_pdu(GfxPdu::CreateSurface(crate::pdu::CreateSurfacePdu { + surface_id: 1, + width: 128, + height: 96, + pixel_format: PixelFormat::XRgb, + })) + .unwrap(); + assert!(wire_progressive(&mut client, progressive_context_stream(false)).is_ok()); + } + #[test] fn progressive_context_is_deleted_with_encoding_context() { let mut client = progressive_client(); diff --git a/crates/ironrdp-egfx/src/compositor.rs b/crates/ironrdp-egfx/src/compositor.rs index ea7f6003d5..0184d252e5 100644 --- a/crates/ironrdp-egfx/src/compositor.rs +++ b/crates/ironrdp-egfx/src/compositor.rs @@ -162,8 +162,8 @@ impl Compositor { .then_some((width, height)) } - /// Handle `ResetGraphics`: set the output size and drop all surfaces, cache and - /// pending output. + /// Handle `ResetGraphics`: set the output size and drop all surfaces and pending + /// output, keeping the bitmap cache. /// /// Per MS-RDPEGFX 2.2.2.14 a reset implicitly destroys every surface and /// redefines the graphics output, so deltas produced before it are discarded @@ -172,15 +172,21 @@ impl Compositor { /// `ResetGraphics` together, and those deltas were clipped against the previous /// output, so painting them into the new one repaints stale pixels and, after a /// shrink, addresses a region the new output no longer contains. + /// + /// Cache slots are not surfaces. They are connection-scoped and 3.3.5.14 + /// redefines only the output buffer, so they survive — the same reasoning that + /// keeps the progressive CONTEXT and the ClearCodec glyph cache. Windows depends + /// on it: after a resolution change it restores the desktop almost entirely from + /// slots filled before the reset, and a dropped slot makes every one of those + /// blits a silent no-op that leaves the old picture on screen. pub(crate) fn reset(&mut self, width: u32, height: u32) { self.output_width = u16::try_from(width).unwrap_or(u16::MAX); self.output_height = u16::try_from(height).unwrap_or(u16::MAX); self.surfaces.clear(); - self.cache.clear(); self.frame.clear(); self.ready.clear(); - // Every charged allocation lived in one of those, so the whole charge goes. - self.allocated_bytes = 0; + // Everything charged outside the surviving cache lived in those. + self.allocated_bytes = self.cache.values().map(|tile| tile.data.len()).sum(); } /// Reserve `len` pixel bytes, or refuse if that would exceed the budget. @@ -979,6 +985,44 @@ mod tests { assert_eq!(&u.data[0..4], &[0x30, 0x20, 0x10, 0xFF]); } + /// The bitmap cache outlives `ResetGraphics`. + /// + /// Windows fills cache slots before a resolution change and then restores the + /// desktop from them afterwards, with hundreds of `CacheToSurface` PDUs against + /// slots filled before the reset. MS-RDPEGFX 3.3.5.14 only redefines the graphics + /// output buffer; cache slots are connection-scoped. Dropping them here makes + /// every one of those blits a silent no-op, so the desktop keeps whatever the + /// client last invented for those pixels. + #[test] + fn reset_keeps_the_bitmap_cache() { + let mut c = Compositor::default(); + c.reset(128, 128); + c.create_surface(1, 16, 16); + c.solid_fill( + 1, + &Color { + b: 0x10, + g: 0x20, + r: 0x30, + xa: 0, + }, + &[rect(0, 0, 8, 8)], + ); + c.surface_to_cache(1, 7, &rect(0, 0, 8, 8)); + c.end_frame(); + let _ = c.drain_output(); + + c.reset(256, 256); + c.create_surface(2, 16, 16); + c.map_surface(2, 0, 0); + c.cache_to_surface(7, 2, &[Point { x: 0, y: 0 }]); + c.end_frame(); + + let updates = c.drain_output(); + assert_eq!(updates.len(), 1, "cached tile must still paint after a reset"); + assert_eq!(&updates[0].data[0..4], &[0x30, 0x20, 0x10, 0xFF]); + } + /// A destination rectangle larger than the surface is clipped, not panicked. #[test] fn oversized_rect_is_clipped() { diff --git a/crates/ironrdp-graphics/src/progressive.rs b/crates/ironrdp-graphics/src/progressive.rs index 5ea16f38a6..6eb887c1a3 100644 --- a/crates/ironrdp-graphics/src/progressive.rs +++ b/crates/ironrdp-graphics/src/progressive.rs @@ -1218,6 +1218,17 @@ impl core::fmt::Display for ProgressiveDecodeError { } } +impl core::error::Error for ProgressiveDecodeError { + fn source(&self) -> Option<&(dyn core::error::Error + 'static)> { + match self { + Self::Pdu(e) => Some(e), + Self::Rlgr(e) => Some(e), + Self::Srl(e) => Some(e), + _ => None, + } + } +} + impl From for ProgressiveDecodeError { fn from(e: ironrdp_core::DecodeError) -> Self { Self::Pdu(e) @@ -1547,6 +1558,30 @@ impl ProgressiveDecoder { self.surface_context_flags.remove(&surface_id); } + /// Number of retained difference-tile coefficient buffers for a surface. + #[must_use] + pub fn reference_count_for_surface(&self, surface_id: u16) -> usize { + self.references + .keys() + .filter(|(reference_surface_id, _, _)| *reference_surface_id == surface_id) + .count() + } + + /// Total retained difference-tile coefficient buffers across all surfaces. + #[must_use] + pub fn total_reference_count(&self) -> usize { + self.references.len() + } + + /// Drop every surface's difference-tile coefficient buffers. + /// + /// `ResetGraphics` implicitly destroys all surfaces (MS-RDPEGFX 2.2.2.14) + /// but does not re-negotiate progressive CONTEXT. Tile references belong + /// to the destroyed surfaces; CONTEXT does not. + pub fn clear_tile_references(&mut self) { + self.references.clear(); + } + /// Reset codec-context state while retaining surface sub-band references. pub fn reset(&mut self) { self.contexts.clear(); @@ -2075,7 +2110,7 @@ mod tests { } #[test] - fn upgrade_pass_rejects_truncated_srl() { + fn upgrade_pass_completes_when_the_srl_stream_ends_early() { let mut coefficients = [0i16; COEFFICIENTS_PER_COMPONENT]; let mut sign = [SIGN_POSITIVE; COEFFICIENTS_PER_COMPONENT]; sign[0] = SIGN_ZERO; @@ -2083,6 +2118,9 @@ mod tests { let mut prev_prog_quant = ComponentCodecQuant::LOSSLESS; prev_prog_quant.hl1 = 4; + // Windows stops emitting once the remaining coefficients are all zero, so the SRL + // stream is shorter than the band's coefficient count. This used to drop the whole + // tile update, which on screen is a block that never refreshes. assert_eq!( decode_upgrade_pass( &[0x80, 0x00], @@ -2093,7 +2131,7 @@ mod tests { &mut coefficients, &mut sign, ), - Err(SrlError::Truncated) + Ok(()) ); } @@ -2102,28 +2140,40 @@ mod tests { let mut tile = TileState::new(); let mut prev_prog_quant = ComponentCodecQuant::LOSSLESS; prev_prog_quant.hl1 = 4; - tile.prog_quant = [prev_prog_quant; 3]; + + // The third component's quantization bit width is out of range, so its magnitude + // decode must fail. On the wire a quant is a 4-bit nibble (see + // `ComponentCodecQuant::decode`) and cannot reach 16; this goes through a struct + // literal instead, for the same reason as the `quant_validate` tests in rfx.rs: a + // literal-constructed value deserves to be rejected before it is used. + let mut out_of_range_quant = ComponentCodecQuant::LOSSLESS; + out_of_range_quant.hl1 = 20; + + tile.prog_quant = [prev_prog_quant, prev_prog_quant, out_of_range_quant]; tile.pass = 1; tile.quality = 50; + // Each component leaves one SRL entry in HL1, so the first two decode successfully. tile.sign[0][0] = SIGN_ZERO; tile.sign[1][0] = SIGN_ZERO; + tile.sign[2][0] = SIGN_ZERO; let coefficients = tile.coefficients; let sign = tile.sign; + let prog_quant = tile.prog_quant; assert_eq!( tile.decode_upgrade( - [&[0x90, 0x00], &[0x80, 0x00], &[]], + [&[0x90, 0x00], &[0x80, 0x00], &[0x90, 0x00]], [&[], &[], &[]], [ComponentCodecQuant::LOSSLESS; 3], 75, ), - Err(SrlError::Truncated) + Err(SrlError::InvalidBitCount(20)) ); assert_eq!(tile.coefficients, coefficients); assert_eq!(tile.sign, sign); - assert_eq!(tile.prog_quant, [prev_prog_quant; 3]); + assert_eq!(tile.prog_quant, prog_quant); assert_eq!(tile.pass, 1); assert_eq!(tile.quality, 50); } diff --git a/crates/ironrdp-graphics/src/srl.rs b/crates/ironrdp-graphics/src/srl.rs index 9503dc7380..39de48f3fa 100644 --- a/crates/ironrdp-graphics/src/srl.rs +++ b/crates/ironrdp-graphics/src/srl.rs @@ -6,29 +6,26 @@ const INITIAL_KP: u8 = 8; const MAX_KP: u8 = 80; -// This conservative malformed-stream bound includes LL3 entries, although LL3 is raw-coded. +/// Longest zero run the encoder will emit: one tile's worth of coefficients. +/// +/// Encode-side only. The decoder consumes a run one event at a time and needs no bound; see +/// [`SrlDecoder::read_zero_run_event`]. const MAX_ZERO_RUN: usize = 4096; /// Errors encountered while decoding or encoding an SRL stream. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum SrlError { - /// The required trailing zero byte is absent. - MissingTerminator, - /// The stream ended before a complete code word was read. - Truncated, /// An SRL value requires between one and fifteen magnitude bits. InvalidBitCount(u8), /// A value cannot be represented by the magnitude width. MagnitudeOutOfRange { magnitude: u16, max: u16 }, - /// A zero run exceeds the number of coefficients in one component. + /// A zero run to encode exceeds the number of coefficients in one tile. ZeroRunTooLong, } impl core::fmt::Display for SrlError { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { match self { - Self::MissingTerminator => write!(f, "srl stream is missing its trailing zero byte"), - Self::Truncated => write!(f, "srl stream is truncated"), Self::InvalidBitCount(bits) => write!(f, "invalid srl magnitude bit count {bits}"), Self::MagnitudeOutOfRange { magnitude, max } => { write!(f, "srl magnitude {magnitude} exceeds maximum {max}") @@ -52,18 +49,16 @@ pub struct SrlDecoder<'a> { } impl<'a> SrlDecoder<'a> { - /// Create a decoder for an SRL stream, excluding its required trailing zero byte. + /// Create a decoder over an SRL stream. + /// + /// Every byte is payload, and the stream is treated as zero-padded past its end — see + /// [`BitReader::read_bit`]. MS-RDPEGFX 2.2.4.2.1.5.4 gives the per-component SRL stream an + /// explicit `*SrlLen`, and nothing in 3.1.8.1.5 reserves the final byte. Requiring a zero + /// terminator rejected the streams Windows actually sends, and when the last byte did happen + /// to be zero it silently dropped those eight bits from the tail. pub fn new(data: &'a [u8]) -> Result { - let Some((&terminator, payload)) = data.split_last() else { - return Err(SrlError::MissingTerminator); - }; - - if terminator != 0 { - return Err(SrlError::MissingTerminator); - } - Ok(Self { - reader: BitReader::new(payload), + reader: BitReader::new(data), kp: INITIAL_KP, zero_run_remaining: 0, nonzero_pending: false, @@ -84,50 +79,73 @@ impl<'a> SrlDecoder<'a> { continue; } + // Not even one bit left to start a codeword: the stream is over and every + // remaining coefficient is zero. + // + // This check must come before `nonzero_pending`. Padding only completes a + // codeword that has *already started* (see `BitReader::read_bit`); if a zero + // run's terminating bit lands on the very last bit, the "non-zero value" that + // follows has no bits at all, and decoding it purely from padding invents a + // maximum-magnitude coefficient out of nothing. + if self.reader.is_exhausted() { + output.resize(num_values, 0); + self.nonzero_pending = false; + break; + } + if self.nonzero_pending { output.push(self.decode_nonzero(num_bits)?); self.nonzero_pending = false; continue; } - self.zero_run_remaining = self.decode_zero_run()?; - self.nonzero_pending = true; + self.read_zero_run_event(); } Ok(output) } - fn decode_zero_run(&mut self) -> Result { - let mut zeros = 0usize; - - loop { - let k = self.kp / 8; - - if self.reader.read_bit()? { - let tail = usize::try_from(self.reader.read_bits(k)?).map_err(|_| SrlError::ZeroRunTooLong)?; - self.kp = self.kp.saturating_sub(6); - - let zeros = zeros.checked_add(tail).ok_or(SrlError::ZeroRunTooLong)?; - return (zeros <= MAX_ZERO_RUN).then_some(zeros).ok_or(SrlError::ZeroRunTooLong); - } - - let chunk = 1usize << k; - zeros = zeros.checked_add(chunk).ok_or(SrlError::ZeroRunTooLong)?; - if zeros > MAX_ZERO_RUN { - return Err(SrlError::ZeroRunTooLong); - } + /// Read one zero-run event: either "at least `1 << k` more zeros" or "`tail` zeros, then a value". + /// + /// A run is consumed one event at a time rather than summed up front. Its total length is + /// bounded only by the coefficients the caller asks for, so a run that outlives the current + /// band simply carries over, and one that outlives the tile is dropped with the decoder. + /// Summing the whole run eagerly needed a cap to stay finite, and that cap rejected the long + /// all-zero runs Windows sends for mostly static tiles — the discarded tiles are the blocks + /// that never refresh. FreeRDP's `progressive_rfx_srl_read` applies no bound either. + fn read_zero_run_event(&mut self) { + let k = self.kp / 8; + if self.reader.read_bit() { + // A `1` bit: `tail` more zeros, then a non-zero value. + // `k` is at most 10 (KP caps at 80), so the tail always fits in a u16. + let tail = u16::try_from(self.reader.read_bits(k)).unwrap_or(u16::MAX); + self.zero_run_remaining = usize::from(tail); + self.kp = self.kp.saturating_sub(6); + self.nonzero_pending = true; + } else { + // A `0` bit: at least `1 << k` more zeros, so the run continues. `k` is at + // most 10, hence the chunk is at least 1 and the loop always makes progress. + self.zero_run_remaining = 1usize << k; self.kp = self.kp.saturating_add(4).min(MAX_KP); } } + /// Bits read past the end of the stream, i.e. how much of the tail was assumed to be zero. + /// + /// A handful at the very end is normal (the encoder stops once the rest of a band is zero). + /// A large count means the decoder and the encoder disagree about the stream layout. + pub fn overread_bits(&self) -> u32 { + self.reader.overread_bits + } + fn decode_nonzero(&mut self, num_bits: u8) -> Result { let maximum = max_magnitude(num_bits)?; - let sign = self.reader.read_bit()?; + let sign = self.reader.read_bit(); let mut zero_count = 0u16; while zero_count + 1 < maximum { - if self.reader.read_bit()? { + if self.reader.read_bit() { break; } @@ -265,6 +283,7 @@ struct BitReader<'a> { data: &'a [u8], byte_idx: usize, bit_idx: u8, + overread_bits: u32, } impl<'a> BitReader<'a> { @@ -273,12 +292,27 @@ impl<'a> BitReader<'a> { data, byte_idx: 0, bit_idx: 0, + overread_bits: 0, } } - fn read_bit(&mut self) -> Result { + /// Whether every bit of the stream has been consumed. + fn is_exhausted(&self) -> bool { + self.byte_idx >= self.data.len() + } + + /// Read one bit, treating the stream as zero-padded past its end. + /// + /// An SRL stream is bounded by the number of coefficients the caller asks for, not by its + /// own length: once the remaining coefficients in a band are all zero the encoder simply + /// stops emitting bits. Windows relies on this, and so does the reference decoder — FreeRDP's + /// `BitStream_Fetch` leaves its prefetch register zeroed when the offset passes capacity. + /// Erroring out instead discarded the whole tile, which is what surfaced as blocks that + /// never refresh. Over-reads are counted so a desync stays visible in the logs. + fn read_bit(&mut self) -> bool { let Some(&byte) = self.data.get(self.byte_idx) else { - return Err(SrlError::Truncated); + self.overread_bits = self.overread_bits.saturating_add(1); + return false; }; let bit = (byte >> (7 - self.bit_idx)) & 1 != 0; @@ -288,15 +322,15 @@ impl<'a> BitReader<'a> { self.byte_idx += 1; } - Ok(bit) + bit } - fn read_bits(&mut self, count: u8) -> Result { + fn read_bits(&mut self, count: u8) -> u32 { let mut value = 0u32; for _ in 0..count { - value = (value << 1) | u32::from(self.read_bit()?); + value = (value << 1) | u32::from(self.read_bit()); } - Ok(value) + value } } @@ -368,13 +402,35 @@ mod tests { } #[test] - fn rejects_truncated_stream() { - assert_eq!(decode_srl(&[0x80, 0x00], 1, 4), Err(SrlError::Truncated)); + fn pads_an_exhausted_stream_with_zeros() { + // The encoder stops emitting bits once the rest of a band is zero, so reaching + // the end of the stream must continue as zeros rather than reject the whole tile. + assert_eq!(decode_srl(&[0x80, 0x00], 1, 4), Ok(vec![15])); + } + + #[test] + fn counts_bits_read_past_the_end() { + // A handful of over-read bits is normal; a count that runs away means the decoder + // and the encoder disagree about the stream layout, which has to be visible in logs. + let mut decoder = SrlDecoder::new(&[0x80]).unwrap(); + let _ = decoder.decode(1, 4); + assert!(decoder.overread_bits() > 0, "zero-padded reads must be counted"); } #[test] - fn rejects_missing_terminator() { - assert_eq!(decode_srl(&[0x84], 1, 4), Err(SrlError::MissingTerminator)); + fn treats_the_final_byte_as_payload() { + // The final byte used to be treated as a mandatory zero terminator and cut off: + // a non-zero one rejected the whole stream (which is exactly what Windows sends), + // and a zero one wasted its eight bits, surfacing later as Truncated. + let decoded = decode_srl(&[0x84], 1, 4); + assert!( + decoded.is_ok(), + "a stream whose last byte is non-zero must decode, got {decoded:?}" + ); + // A component with `*SrlLen = 0` is common: it means no refinement this pass, so + // the coefficients stay zero. The old implementation reported MissingTerminator + // for an empty stream and the whole tile update was dropped. + assert_eq!(decode_srl(&[], 3, 4), Ok(vec![0, 0, 0])); } #[test] @@ -386,10 +442,23 @@ mod tests { } #[test] - fn rejects_zero_run_longer_than_component() { + fn encoder_rejects_zero_run_longer_than_a_tile() { assert_eq!(encode_srl(&vec![0; MAX_ZERO_RUN + 1], 1), Err(SrlError::ZeroRunTooLong)); } + #[test] + fn decodes_a_zero_run_longer_than_the_encoder_would_emit() { + // All-zero bits are a chain of "at least `1 << k` more zeros" events, and `k` grows + // with KP up to 1024, so the sum far exceeds one tile's worth of coefficients. The + // decoder used to sum the whole run up front and cap it at 4096, so the very long + // zero runs a static region produces were judged corrupt and the whole tile update + // was dropped — the blocks that never refresh. The reference implementation consumes + // the run event by event with no bound at all: however long it runs it is only zeros, + // and the excess is dropped along with the decoder. + assert_eq!(decode_srl(&[0x00; 32], 8, 4), Ok(vec![0; 8])); + assert_eq!(decode_srl(&[0x00; 32], MAX_ZERO_RUN, 4), Ok(vec![0; MAX_ZERO_RUN])); + } + #[test] fn encodes_empty_stream() { assert_eq!(encode_srl(&[], 1), Ok(vec![0x00])); diff --git a/crates/ironrdp-session/src/lib.rs b/crates/ironrdp-session/src/lib.rs index 3103da0a37..5b92c78b54 100644 --- a/crates/ironrdp-session/src/lib.rs +++ b/crates/ironrdp-session/src/lib.rs @@ -38,7 +38,7 @@ pub enum SessionErrorKind { impl fmt::Display for SessionErrorKind { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match &self { - SessionErrorKind::Pdu(_) => write!(f, "PDU error"), + SessionErrorKind::Pdu(e) => write!(f, "PDU error: {e}"), SessionErrorKind::Encode(_) => write!(f, "encode error"), SessionErrorKind::Decode(_) => write!(f, "decode error"), SessionErrorKind::FastPathBulkDecompression(_) => write!(f, "fast-path bulk decompression error"), diff --git a/crates/ironrdp-testsuite-core/tests/egfx/wire_to_surface_real_world.rs b/crates/ironrdp-testsuite-core/tests/egfx/wire_to_surface_real_world.rs index 2322b1f676..dc0e45cb40 100644 --- a/crates/ironrdp-testsuite-core/tests/egfx/wire_to_surface_real_world.rs +++ b/crates/ironrdp-testsuite-core/tests/egfx/wire_to_surface_real_world.rs @@ -230,3 +230,189 @@ fn wts2_diff_requires_retained_reference() { Err(ProgressiveDecodeError::MissingTileReference { x_idx: 3, y_idx: 2 }) )); } + +/// Cold-decode every Haven difference-only WireToSurface2 fixture through +/// [`ProgressiveDecoder`]. These captures are mid-stream refinement PDUs from +/// Windows — without a prior retained tile they must fail with +/// [`ProgressiveDecodeError::MissingTileReference`] (MS-RDPRFX 3.1.8.1.7.1). +/// +/// This is the same failure class a client hits when EGFX starts mid-refinement +/// or loses codec-context references. +#[rstest] +#[case::diff_2tiles( + include_bytes!("../../test_data/egfx/haven/wts2_64x64_diff_2tiles.bin").as_slice(), + 1280, + 800, + 3, + 2 +)] +#[case::diff_3tiles( + include_bytes!("../../test_data/egfx/haven/wts2_64x128_diff_3tiles.bin").as_slice(), + 1280, + 800, + 3, + 2 +)] +#[case::diff_column_9tiles( + include_bytes!("../../test_data/egfx/haven/wts2_37x560_diff_column_9tiles.bin").as_slice(), + 1280, + 800, + 19, + 3 +)] +fn haven_wts2_cold_decode_reproduces_missing_tile_reference( + #[case] bytes: &[u8], + #[case] surface_width: u16, + #[case] surface_height: u16, + #[case] expect_x: u16, + #[case] expect_y: u16, +) { + use ironrdp_graphics::progressive::{ProgressiveDecodeError, ProgressiveDecoder}; + use ironrdp_pdu::codecs::rfx::RfxRectangle; + use ironrdp_pdu::codecs::rfx::progressive::{ + ProgressiveBlock, ProgressiveContextPdu, ProgressiveFrameBeginPdu, ProgressiveFrameEndPdu, ProgressiveRegion, + ProgressiveSyncPdu, encode_progressive_stream, + }; + + let GfxPdu::WireToSurface2(pdu) = decode(bytes) else { + panic!("expected WireToSurface2"); + }; + + // Establish CONTEXT only — no base tiles — matching a client that joins + // (or resets) after Windows already has difference state. + let init = encode_progressive_stream(&[ + ProgressiveBlock::Sync(ProgressiveSyncPdu), + ProgressiveBlock::Context(ProgressiveContextPdu { + context_id: 0, + tile_size: 0x40, + flags: 0, + }), + ProgressiveBlock::FrameBegin(ProgressiveFrameBeginPdu { + frame_index: 0, + region_count: 1, + }), + ProgressiveBlock::Region(ProgressiveRegion { + tile_size: 0x40, + rects: vec![RfxRectangle { + x: 0, + y: 0, + width: 64, + height: 64, + }], + quant_vals: vec![], + quant_prog_vals: vec![], + flags: 0, + tiles: vec![], + }), + ProgressiveBlock::FrameEnd(ProgressiveFrameEndPdu), + ]) + .unwrap(); + + let mut decoder = ProgressiveDecoder::new(); + decoder + .decode_bitmap(pdu.surface_id, 0, surface_width, surface_height, &init) + .expect("CONTEXT init"); + + let err = match decoder.decode_bitmap( + pdu.surface_id, + pdu.codec_context_id, + surface_width, + surface_height, + &pdu.bitmap_data, + ) { + Err(e) => e, + Ok(_) => panic!("difference-only Haven fixture must fail without retained references"), + }; + + assert!( + matches!( + err, + ProgressiveDecodeError::MissingTileReference { x_idx, y_idx } + if x_idx == expect_x && y_idx == expect_y + ), + "unexpected ProgressiveDecodeError: {err:?}" + ); +} + +/// Mixed Haven fixture carries base + difference tiles in one REGION. Decoding +/// it cold (CONTEXT only) still fails on the first difference tile whose +/// coordinate has no retained reference yet — even though base tiles exist +/// later/elsewhere in the same PDU. +#[test] +fn haven_wts2_mixed_25tiles_cold_decode_hits_missing_tile_reference() { + use ironrdp_graphics::progressive::{ProgressiveDecodeError, ProgressiveDecoder}; + use ironrdp_pdu::codecs::rfx::RfxRectangle; + use ironrdp_pdu::codecs::rfx::progressive::{ + ProgressiveBlock, ProgressiveContextPdu, ProgressiveFrameBeginPdu, ProgressiveFrameEndPdu, ProgressiveRegion, + ProgressiveSyncPdu, encode_progressive_stream, + }; + + let bytes = include_bytes!("../../test_data/egfx/haven/wts2_progressive_tile_first_mixed_25tiles.bin"); + let GfxPdu::WireToSurface2(pdu) = decode(bytes) else { + panic!("expected WireToSurface2"); + }; + + let init = encode_progressive_stream(&[ + ProgressiveBlock::Sync(ProgressiveSyncPdu), + ProgressiveBlock::Context(ProgressiveContextPdu { + context_id: 0, + tile_size: 0x40, + flags: 0, + }), + ProgressiveBlock::FrameBegin(ProgressiveFrameBeginPdu { + frame_index: 0, + region_count: 1, + }), + ProgressiveBlock::Region(ProgressiveRegion { + tile_size: 0x40, + rects: vec![RfxRectangle { + x: 0, + y: 0, + width: 64, + height: 64, + }], + quant_vals: vec![], + quant_prog_vals: vec![], + flags: 0, + tiles: vec![], + }), + ProgressiveBlock::FrameEnd(ProgressiveFrameEndPdu), + ]) + .unwrap(); + + let mut decoder = ProgressiveDecoder::new(); + decoder + .decode_bitmap(pdu.surface_id, 0, 1280, 800, &init) + .expect("CONTEXT init"); + + let err = match decoder.decode_bitmap(pdu.surface_id, pdu.codec_context_id, 1280, 800, &pdu.bitmap_data) { + Err(e) => e, + Ok(_) => panic!("mixed fixture still needs retained refs for difference tiles"), + }; + + assert!( + matches!(err, ProgressiveDecodeError::MissingTileReference { .. }), + "expected MissingTileReference, got {err:?}" + ); +} + +/// Bare difference fixture with no CONTEXT seed at all → MissingBlock("CONTEXT"). +#[test] +fn haven_wts2_no_context_reproduces_missing_block() { + use ironrdp_graphics::progressive::{ProgressiveDecodeError, ProgressiveDecoder}; + + let bytes = include_bytes!("../../test_data/egfx/haven/wts2_64x64_diff_2tiles.bin"); + let GfxPdu::WireToSurface2(pdu) = decode(bytes) else { + panic!("expected WireToSurface2"); + }; + + let mut decoder = ProgressiveDecoder::new(); + let err = match decoder.decode_bitmap(pdu.surface_id, pdu.codec_context_id, 1280, 800, &pdu.bitmap_data) { + Err(e) => e, + Ok(_) => panic!("must fail without CONTEXT"), + }; + assert!( + matches!(err, ProgressiveDecodeError::MissingBlock("CONTEXT")), + "expected MissingBlock(CONTEXT), got {err:?}" + ); +} From af703598fb30ae3596c66f8a2c2a1bdba61e4735 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=A8=E6=88=90=E9=94=B4?= Date: Wed, 16 Sep 2026 18:05:15 +0800 Subject: [PATCH 2/7] fix(web): follow the server's graphics size on the canvas The protocol-side fixes are only half of the tearing story for the web client: `ironrdp-web` never opted into MS-RDPEGFX, and its canvas never followed a size the server chose on its own. Enable the graphics pipeline (`support_dyn_vc_gfx_protocol`) and register the EGFX DVC, then keep the canvas in step with `DecodedImage`: - Sync the backing store right before the frame's `GraphicsUpdate` is drawn, not after. `ActiveStage::process` can resize `image` to follow a ResetGraphics within the same frame, so a canvas synced afterwards would show that frame at the old size. - Repaint the whole image whenever the resize actually happened. Setting `width`/`height` clears a canvas, so drawing only the frame's dirty regions would blank everything the server did not happen to repaint. - Sync after a Deactivation-Reactivation Sequence too. That replaces `image` from inside the output loop, i.e. after this frame's sync already ran, and the next event can be an idle interval away. - Make `resize` report whether the size changed, so an unchanged size stays a no-op instead of clearing and repainting every frame. `Session::desktop_size()` now tracks the size in effect rather than the one negotiated at connect, since both a reset and a reactivation change it. Resize requests deliberately leave the canvas alone: it follows the size the server actually applies, not the one that was asked for. --- Cargo.lock | 1 + crates/ironrdp-web/Cargo.toml | 1 + crates/ironrdp-web/src/canvas.rs | 11 +- crates/ironrdp-web/src/session.rs | 173 +++++++++++++++++++++++++----- 4 files changed, 160 insertions(+), 26 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 662b8941a2..2473a480b1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3554,6 +3554,7 @@ dependencies = [ "ironrdp", "ironrdp-cliprdr-format", "ironrdp-core 0.2.1", + "ironrdp-egfx", "ironrdp-futures", "ironrdp-pdu", "ironrdp-propertyset", diff --git a/crates/ironrdp-web/Cargo.toml b/crates/ironrdp-web/Cargo.toml index c964d7ce21..8c470c092d 100644 --- a/crates/ironrdp-web/Cargo.toml +++ b/crates/ironrdp-web/Cargo.toml @@ -40,6 +40,7 @@ ironrdp = { path = "../ironrdp", features = [ ] } ironrdp-core.path = "../ironrdp-core" ironrdp-cliprdr-format.path = "../ironrdp-cliprdr-format" +ironrdp-egfx.path = "../ironrdp-egfx" ironrdp-futures.path = "../ironrdp-futures" ironrdp-pdu.path = "../ironrdp-pdu" ironrdp-rdcleanpath.path = "../ironrdp-rdcleanpath" diff --git a/crates/ironrdp-web/src/canvas.rs b/crates/ironrdp-web/src/canvas.rs index 30ba5be78f..e96e86adda 100644 --- a/crates/ironrdp-web/src/canvas.rs +++ b/crates/ironrdp-web/src/canvas.rs @@ -30,10 +30,17 @@ impl Canvas { } /// Resizes the backing store. Note: this also clears the canvas and resets 2D context state; - /// the cached `ctx` stays valid. - pub(crate) fn resize(&mut self, width: NonZeroU32, height: NonZeroU32) { + /// the cached `ctx` stays valid. Assigning the current size is not a no-op in the DOM, so + /// unchanged sizes are filtered out to avoid clearing the canvas for nothing. + /// + /// Returns `true` when the backing store actually changed (and was therefore cleared). + pub(crate) fn resize(&mut self, width: NonZeroU32, height: NonZeroU32) -> bool { + if self.canvas.width() == width.get() && self.canvas.height() == height.get() { + return false; + } self.canvas.set_width(width.get()); self.canvas.set_height(height.get()); + true } /// Blits a dirty region with `put_image_data`. Forces alpha opaque first: the framebuffer isn't diff --git a/crates/ironrdp-web/src/session.rs b/crates/ironrdp-web/src/session.rs index a47bd3d0c0..483fcf8ed1 100644 --- a/crates/ironrdp-web/src/session.rs +++ b/crates/ironrdp-web/src/session.rs @@ -1,4 +1,4 @@ -use core::cell::RefCell; +use core::cell::{Cell, RefCell}; use core::net::{Ipv4Addr, SocketAddrV4}; use core::num::NonZeroU32; use core::time::Duration; @@ -23,6 +23,7 @@ use ironrdp::connector::{self, ClientConnector, Credentials}; use ironrdp::displaycontrol::client::DisplayControlClient; use ironrdp::dvc::DrdynvcClient; use ironrdp::graphics::image_processing::PixelFormat; +use ironrdp::pdu::geometry::InclusiveRectangle; use ironrdp::pdu::input::fast_path::FastPathInputEvent; use ironrdp::pdu::rdp::capability_sets::{BitmapCodecs, client_codecs_capabilities}; use ironrdp::pdu::rdp::client_info::{PerformanceFlags, TimezoneInfo}; @@ -32,6 +33,7 @@ use ironrdp::rdpsnd::client::{NoopRdpsndBackend, Rdpsnd}; use ironrdp::session::image::DecodedImage; use ironrdp::session::{ActiveStageBuilder, ActiveStageOutput, GracefulDisconnectReason}; use ironrdp_core::WriteBuf; +use ironrdp_egfx::client::{GraphicsPipelineClient, GraphicsPipelineHandler}; use ironrdp_futures::{FramedWrite, single_sequence_step_read}; use rgb::AsPixels as _; use tap::prelude::*; @@ -71,6 +73,7 @@ struct SessionBuilderInner { render_canvas: Option, set_cursor_style_callback: Option, set_cursor_style_callback_context: Option, + canvas_resized_callback: Option, remote_clipboard_changed_callback: Option, force_clipboard_update_callback: Option, // File transfer callbacks @@ -117,6 +120,7 @@ impl Default for SessionBuilderInner { render_canvas: None, set_cursor_style_callback: None, set_cursor_style_callback_context: None, + canvas_resized_callback: None, remote_clipboard_changed_callback: None, force_clipboard_update_callback: None, files_available_callback: None, @@ -242,8 +246,9 @@ impl iron_remote_desktop::SessionBuilder for SessionBuilder { self.clone() } - /// Because the server does not resize the framebuffer in the RDP protocol, this feature is unused in IronRDP. - fn canvas_resized_callback(&self, _callback: js_sys::Function) -> Self { + /// Called after the HTML canvas backing store is resized (Display Control / reactivation). + fn canvas_resized_callback(&self, callback: js_sys::Function) -> Self { + self.0.borrow_mut().canvas_resized_callback = Some(callback); self.clone() } @@ -348,6 +353,7 @@ impl iron_remote_desktop::SessionBuilder for SessionBuilder { render_canvas, set_cursor_style_callback, set_cursor_style_callback_context, + canvas_resized_callback, remote_clipboard_changed_callback, force_clipboard_update_callback, files_available_callback, @@ -391,6 +397,7 @@ impl iron_remote_desktop::SessionBuilder for SessionBuilder { .set_cursor_style_callback_context .clone() .context("set_cursor_style_callback_context missing")?; + canvas_resized_callback = inner.canvas_resized_callback.clone(); remote_clipboard_changed_callback = inner.remote_clipboard_changed_callback.clone(); force_clipboard_update_callback = inner.force_clipboard_update_callback.clone(); files_available_callback = inner.files_available_callback.clone(); @@ -516,6 +523,7 @@ impl iron_remote_desktop::SessionBuilder for SessionBuilder { printer_driver_name, computer_name: client_name.clone(), use_display_control, + input_events_tx: input_events_tx.clone(), }) .await?; @@ -528,7 +536,7 @@ impl iron_remote_desktop::SessionBuilder for SessionBuilder { spawn_local(writer_task(writer_rx, rdp_writer, outbound_message_size_limit)); Ok(Session { - desktop_size: connection_result.desktop_size, + desktop_size: Cell::new(connection_result.desktop_size), input_database: RefCell::new(ironrdp::input::Database::new()), writer_tx, input_events_tx, @@ -536,6 +544,7 @@ impl iron_remote_desktop::SessionBuilder for SessionBuilder { render_canvas, set_cursor_style_callback, set_cursor_style_callback_context, + canvas_resized_callback, input_events_rx: RefCell::new(Some(input_events_rx)), rdp_reader: RefCell::new(Some(rdp_reader)), @@ -562,6 +571,13 @@ pub(crate) enum RdpInputEvent { scale_factor: Option, physical_size: Option<(u32, u32)>, }, + /// Server resized the Graphics Output Buffer (MS-RDPEGFX 2.2.2.14). This is how a modern + /// Windows host answers a Display Control request: no Deactivation-Reactivation Sequence, + /// so it is the only chance we get to follow the new desktop size. + GraphicsReset { + width: u32, + height: u32, + }, TerminateSession, } @@ -576,7 +592,9 @@ impl iron_remote_desktop::SessionTerminationInfo for SessionTerminationInfo { } pub(crate) struct Session { - desktop_size: connector::DesktopSize, + /// Follows the size the server actually applied, which a graphics reset or a + /// reactivation can change after connect. + desktop_size: Cell, input_database: RefCell, writer_tx: mpsc::UnboundedSender>, input_events_tx: mpsc::UnboundedSender, @@ -584,6 +602,7 @@ pub(crate) struct Session { render_canvas: HtmlCanvasElement, set_cursor_style_callback: js_sys::Function, set_cursor_style_callback_context: JsValue, + canvas_resized_callback: Option, // Consumed when `run` is called input_events_rx: RefCell>>, @@ -683,8 +702,6 @@ impl iron_remote_desktop::Session for Session { connection_result.desktop_size.height, ); - let mut requested_resize = None; - // Reused across frames so per-region extraction doesn't allocate on every draw. let mut draw_buffer = WriteBuf::new(); @@ -864,16 +881,31 @@ impl iron_remote_desktop::Session for Session { warn!("Resize event ignored: width or height is zero"); Vec::new() } else if let Some(response_frame) = active_stage.encode_resize(width, height, scale_factor, physical_size) { - let width = NonZeroU32::new(width).expect("width is guaranteed to be non-zero due to the prior check"); - let height = NonZeroU32::new(height).expect("height is guaranteed to be non-zero due to the prior check"); - - requested_resize = Some((width, height)); + // The canvas is not touched here: it follows whatever size the + // server actually applies (EGFX ResetGraphics, or reactivation). vec![ActiveStageOutput::ResponseFrame(response_frame?)] } else { debug!("Resize event ignored"); Vec::new() } }, + RdpInputEvent::GraphicsReset { width, height } => { + // Image resize happens inside `ActiveStage::process` before same-frame + // compositor deltas are applied. The canvas is synced from `image` + // below. Do not SuppressOutput/RefreshRect here: with RDPGFX those + // PDUs do not invalidate the surface cache (FreeRDP#12723) and can + // leave the session waiting on a full paint that never arrives. + debug!(width, height, "Graphics output buffer reset (image already follows)"); + // `desktop_size()` must report the size in effect, not the one + // negotiated at connect time. + if let (Ok(width), Ok(height)) = (u16::try_from(width), u16::try_from(height)) + && width > 0 + && height > 0 + { + self.desktop_size.set(connector::DesktopSize { width, height }); + } + Vec::new() + }, RdpInputEvent::Printer(message) => { // The printer backend lives inside the Rdpdr SVC // processor (Send-only); the front-end @@ -915,6 +947,16 @@ impl iron_remote_desktop::Session for Session { } }; + // `process()` may have resized `image` to follow ResetGraphics in the same + // frame. The canvas has to match *before* the GraphicsUpdate from that frame + // is drawn. + sync_canvas_to_image( + &mut gui, + &image, + &mut draw_buffer, + self.canvas_resized_callback.as_ref(), + )?; + for out in outputs { match out { ActiveStageOutput::ResponseFrame(frame) => { @@ -1042,13 +1084,6 @@ impl iron_remote_desktop::Session for Session { // https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpbcgr/dfc234ce-481a-4674-9a5d-2a7bafb14432 debug!("Received Server Deactivate All PDU, executing Deactivation-Reactivation Sequence"); - // We need to perform resize after receiving the Deactivate All PDU, because there may be frames - // with the previous dimensions arriving between the resize request and this message. - if let Some((width, height)) = requested_resize { - gui.resize(width, height); - requested_resize = None; - } - let mut connection_activation = activation_factory.create(); let mut buf = WriteBuf::new(); 'activation_seq: loop { @@ -1074,6 +1109,16 @@ impl iron_remote_desktop::Session for Session { { debug!("Deactivation-Reactivation Sequence completed"); image = DecodedImage::new(PixelFormat::RgbA32, desktop_size.width, desktop_size.height); + // Sync here rather than waiting for the next iteration: the + // pre-`outputs` check already ran for this one, and the next + // event can be a whole clipboard-cleanup interval away. + self.desktop_size.set(desktop_size); + sync_canvas_to_image( + &mut gui, + &image, + &mut draw_buffer, + self.canvas_resized_callback.as_ref(), + )?; if !active_stage.reactivate( connection_activation.io_channel_id(), connection_activation.user_channel_id(), @@ -1124,9 +1169,10 @@ impl iron_remote_desktop::Session for Session { } fn desktop_size(&self) -> DesktopSize { + let desktop_size = self.desktop_size.get(); DesktopSize { - width: self.desktop_size.width, - height: self.desktop_size.height, + width: desktop_size.width, + height: desktop_size.height, } } @@ -1452,6 +1498,47 @@ fn parse_file_metadata_array(files: JsValue) -> Result, IronEr Ok(file_list) } +/// Match the canvas backing store to `image`, repainting the pixels the resize cleared. +/// +/// Setting `width`/`height` on a canvas clears it, so after a resize the whole image is +/// re-blitted; otherwise only the dirty regions of the current frame would survive and the +/// rest of the desktop would stay blank until the server happened to repaint it. Does +/// nothing when the size already matches, which is the common case. +fn sync_canvas_to_image( + gui: &mut Canvas, + image: &DecodedImage, + draw_buffer: &mut WriteBuf, + canvas_resized_callback: Option<&js_sys::Function>, +) -> anyhow::Result<()> { + let (Some(width), Some(height)) = ( + NonZeroU32::new(u32::from(image.width())), + NonZeroU32::new(u32::from(image.height())), + ) else { + return Ok(()); + }; + + if !gui.resize(width, height) { + return Ok(()); + } + + let region = InclusiveRectangle { + left: 0, + top: 0, + right: image.width().saturating_sub(1), + bottom: image.height().saturating_sub(1), + }; + let region = extract_partial_image(image, region, draw_buffer); + gui.draw(draw_buffer.filled_mut(), region) + .context("repaint canvas after resize")?; + draw_buffer.clear(); + + if let Some(callback) = canvas_resized_callback { + let _ = callback.call0(&JsValue::NULL); + } + + Ok(()) +} + fn build_config( username: String, password: String, @@ -1516,7 +1603,9 @@ fn build_config( request_data: None, pointer_software_rendering: false, multitransport_flags: None, - support_dyn_vc_gfx_protocol: false, + // Prefer MS-RDPEGFX when the server supports it — classic bitmap updates + // paint dirty rectangles and look "blocky" on full refreshes. + support_dyn_vc_gfx_protocol: true, performance_flags: PerformanceFlags::default(), desktop_scale_factor: 0, hardware_id: None, @@ -1584,6 +1673,8 @@ struct ConnectParams { /// `computer_name` when constructing the `Rdpdr` processor. computer_name: String, use_display_control: bool, + /// Lets the EGFX handler report server-driven desktop resizes to the event loop. + input_events_tx: mpsc::UnboundedSender, } fn default_printer_driver_name() -> String { @@ -1634,6 +1725,7 @@ async fn connect( printer_driver_name, computer_name, use_display_control, + input_events_tx, }: ConnectParams, ) -> Result<(connector::ConnectionResult, WebSocket), IronError> { let mut framed = ironrdp_futures::LocalFuturesFramed::new(ws); @@ -1661,11 +1753,44 @@ async fn connect( ); } + // Advertise SUPPORT_DYN_VC_GFX in Config, and actually register the EGFX + // DVC here. Without GraphicsPipelineClient the server stays on classic + // dirty-rectangle bitmaps (blocky full-screen refreshes). + // + // No H.264 decoder in WASM yet — GraphicsPipelineClient filters AVC caps + // and falls back to V8 / ClearCodec / RFX Progressive. + struct EgfxHandler { + input_events_tx: mpsc::UnboundedSender, + } + impl GraphicsPipelineHandler for EgfxHandler { + fn on_reset_graphics(&mut self, width: u32, height: u32) { + // The decoded image and the canvas backing store are both sized from the desktop and + // are not touched by the EGFX pipeline. Left alone, everything outside the new desktop + // keeps the pixels of the old one forever (visible as a hard seam). + if self + .input_events_tx + .unbounded_send(RdpInputEvent::GraphicsReset { width, height }) + .is_err() + { + warn!("Failed to send graphics reset event, receiver is closed"); + } + } + } + + let mut drdynvc = DrdynvcClient::new().with_dynamic_channel(GraphicsPipelineClient::new( + Box::new(EgfxHandler { + input_events_tx: input_events_tx.clone(), + }), + None, + )); if use_display_control { - connector.attach_static_channel( - DrdynvcClient::new().with_dynamic_channel(DisplayControlClient::new(|_| Ok(Vec::new()))), - ); + // Deliberately no automatic MonitorLayout once caps arrive: resizing right after + // connect clears the canvas, and combined with GFX progressive that tends to leave + // the whole screen in black blocks. Size changes are left to the frontend, which + // issues them when the user actually resizes the window. + drdynvc = drdynvc.with_dynamic_channel(DisplayControlClient::new(|_| Ok(Vec::new()))); } + connector.attach_static_channel(drdynvc); let kerberos_config = url::Url::parse(kdc_proxy_url.unwrap_or_default().as_str()) .ok() From 398ed3b668ee0382183a73094d97a54ac3720597 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=A8=E6=88=90=E9=94=B4?= Date: Wed, 16 Sep 2026 18:53:20 +0800 Subject: [PATCH 3/7] test(session): lock the resize that precedes compositing, and add the harness that graded it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The framebuffer resize on EGFX `ResetGraphics` was the one fault in this branch with no test behind it. Reverting it left the whole suite green, so nothing stopped a later refactor from reordering it back into a torn desktop. Cover it where it can actually run. `ironrdp-session` sets `[lib] test = false`, so its inline `#[cfg(test)]` modules never execute under `cargo test --workspace`; the test goes in `ironrdp-testsuite-core` next to the existing `composite_graphics_updates` cases. It drives a real `ActiveStage` with an open EGFX channel and feeds one payload carrying `ResetGraphics` plus the drawing that follows, with the fill placed outside the old image and inside the new one — so it only survives if the resize happened first. Reverting the fix fails it on the size assertion. Also promote the resize-stability harness this branch was graded with from a local script to `examples/rdp_stress.rs`. It talks to `ironrdp-session` directly over a real connection, drives resolution changes, and grades the decoded framebuffer on black tiles, stale tiles (the previous frame stretched over the new desktop, i.e. what tearing looks like) and seam energy on the progressive tile grid. Stale now counts toward failure alongside black: a resize that leaves the old picture behind is fully painted and perfectly non-black, so grading on blackness alone reported success on exactly the bug the harness exists to find. The settle loop and the verdict share one predicate so they cannot drift. --- Cargo.lock | 1 + crates/ironrdp-egfx/src/client.rs | 34 +- .../tests/session/active_stage.rs | 225 ++- crates/ironrdp/Cargo.toml | 6 + crates/ironrdp/examples/rdp_stress.rs | 1299 +++++++++++++++++ crates/ironrdp/src/lib.rs | 6 +- 6 files changed, 1552 insertions(+), 19 deletions(-) create mode 100644 crates/ironrdp/examples/rdp_stress.rs diff --git a/Cargo.lock b/Cargo.lock index 2473a480b1..dba0c163ed 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2569,6 +2569,7 @@ dependencies = [ "ironrdp-displaycontrol", "ironrdp-dvc", "ironrdp-echo", + "ironrdp-egfx", "ironrdp-graphics", "ironrdp-input", "ironrdp-mstsgu", diff --git a/crates/ironrdp-egfx/src/client.rs b/crates/ironrdp-egfx/src/client.rs index 5eee593ee1..5d64717b07 100644 --- a/crates/ironrdp-egfx/src/client.rs +++ b/crates/ironrdp-egfx/src/client.rs @@ -1769,25 +1769,29 @@ mod tests { /// Same-size `ResetGraphics` must still be observable: it destroys every surface, so a /// consumer has to re-blit even when the output dimensions did not change. #[test] - fn take_reset_graphics_reports_same_size_resets() { + fn take_output_reset_reports_same_size_resets() { let mut client = GraphicsPipelineClient::new(Box::new(TestHandler), None); - assert!(client.take_reset_graphics().is_none()); + assert!(client.take_output_reset().is_none()); - let _ = client.handle_pdu(GfxPdu::ResetGraphics(crate::pdu::ResetGraphicsPdu { - width: 800, - height: 600, - monitors: vec![], - })); - assert_eq!(client.take_reset_graphics(), Some((800, 600))); - assert!(client.take_reset_graphics().is_none(), "flag is one-shot"); + client + .handle_pdu(GfxPdu::ResetGraphics(crate::pdu::ResetGraphicsPdu { + width: 800, + height: 600, + monitors: vec![], + })) + .expect("valid reset dimensions"); + assert_eq!(client.take_output_reset(), Some((800, 600))); + assert!(client.take_output_reset().is_none(), "flag is one-shot"); - let _ = client.handle_pdu(GfxPdu::ResetGraphics(crate::pdu::ResetGraphicsPdu { - width: 800, - height: 600, - monitors: vec![], - })); + client + .handle_pdu(GfxPdu::ResetGraphics(crate::pdu::ResetGraphicsPdu { + width: 800, + height: 600, + monitors: vec![], + })) + .expect("valid reset dimensions"); assert_eq!( - client.take_reset_graphics(), + client.take_output_reset(), Some((800, 600)), "same-size ResetGraphics must still signal a full client re-blit" ); diff --git a/crates/ironrdp-testsuite-core/tests/session/active_stage.rs b/crates/ironrdp-testsuite-core/tests/session/active_stage.rs index d8939ad535..bbf844006a 100644 --- a/crates/ironrdp-testsuite-core/tests/session/active_stage.rs +++ b/crates/ironrdp-testsuite-core/tests/session/active_stage.rs @@ -1,17 +1,35 @@ //! Regression tests for `composite_graphics_updates`, which applies EGFX compositor -//! deltas and returns both their exact regions and the union reported by `ActiveStage`. +//! deltas and returns both their exact regions and the union reported by `ActiveStage`, +//! and for the output reset that must precede compositing in `ActiveStage::process`. //! //! `ironrdp-session` builds with `[lib] test = false`, so inline `#[cfg(test)]` //! modules there never run under `cargo test --workspace --locked`. These tests //! live here instead so they actually execute in CI. +use core::any::TypeId; +use std::borrow::Cow; use std::sync::Arc; +use ironrdp_core::encode_vec; +use ironrdp_dvc::DrdynvcClient; +use ironrdp_dvc::pdu::{CreateRequestPdu, DataPdu, DrdynvcDataPdu, DrdynvcServerPdu}; +use ironrdp_egfx::client::{GraphicsPipelineClient, GraphicsPipelineHandler}; +use ironrdp_egfx::pdu::{ + CapabilitiesConfirmPdu, CapabilitiesV8Flags, CapabilitySet, Color, CreateSurfacePdu, EndFramePdu, GfxPdu, + MapSurfaceToOutputPdu, PixelFormat as GfxPixelFormat, ResetGraphicsPdu, SolidFillPdu, StartFramePdu, Timestamp, +}; use ironrdp_graphics::image_processing::PixelFormat; use ironrdp_graphics::pointer::DecodedPointer; +use ironrdp_graphics::zgfx::wrap_uncompressed; +use ironrdp_pdu::Action; use ironrdp_pdu::geometry::ExclusiveRectangle; +use ironrdp_pdu::mcs::{McsMessage, SendDataIndication}; +use ironrdp_pdu::rdp::vc::{ChannelControlFlags, ChannelPduHeader}; +use ironrdp_pdu::x224::X224; use ironrdp_session::composite_graphics_updates; use ironrdp_session::image::DecodedImage; +use ironrdp_session::{ActiveStage, ActiveStageBuilder, ActiveStageOutput}; +use ironrdp_svc::{StaticChannelSet, SvcProcessor as _}; fn update(left: u16, top: u16, right: u16, bottom: u16) -> (ExclusiveRectangle, Vec) { let w = usize::from(right - left); @@ -204,3 +222,208 @@ fn reset_graphics_clips_a_hotspot_cursor_to_one_pixel() { assert_eq!(image.data(), &[0xFF, 0xFF, 0xFF, 0]); } + +// ── EGFX ResetGraphics ─────────────────────────────────────────────────────── + +const USER_CHANNEL_ID: u16 = 1001; +const IO_CHANNEL_ID: u16 = 1003; +const DRDYNVC_CHANNEL_ID: u16 = 1004; +/// Any non-zero id works; the server picks this when it opens the EGFX channel. +const EGFX_DVC_ID: u32 = 7; + +const OLD_WIDTH: u16 = 640; +const OLD_HEIGHT: u16 = 480; +const NEW_WIDTH: u16 = 1024; +const NEW_HEIGHT: u16 = 768; + +/// The handler is only notified; the compositing under test is driven by the PDUs. +struct NoopEgfxHandler; + +impl GraphicsPipelineHandler for NoopEgfxHandler {} + +/// One complete static-channel chunk: the SVC layer dechunkifies before dispatching, so +/// a payload without FIRST|LAST is rejected as a fragment with no opening. +fn channel_chunk(data: &[u8]) -> Vec { + let header = ChannelPduHeader { + length: u32::try_from(data.len()).expect("chunk length fits u32"), + flags: ChannelControlFlags::FLAG_FIRST | ChannelControlFlags::FLAG_LAST, + }; + let mut chunk = encode_vec(&header).expect("encode channel header"); + chunk.extend_from_slice(data); + chunk +} + +/// Wrap EGFX PDUs the way a server does: concatenated, ZGFX-segmented, carried on a +/// DVC data PDU inside an MCS Send Data Indication for the drdynvc channel. +fn egfx_frame(pdus: &[GfxPdu]) -> Vec { + let mut raw = Vec::new(); + for pdu in pdus { + raw.extend_from_slice(&encode_vec(pdu).expect("encode EGFX PDU")); + } + + let dvc = DrdynvcServerPdu::Data(DrdynvcDataPdu::Data(DataPdu::new(EGFX_DVC_ID, wrap_uncompressed(&raw)))); + + let indication = McsMessage::SendDataIndication(SendDataIndication { + initiator_id: USER_CHANNEL_ID, + channel_id: DRDYNVC_CHANNEL_ID, + user_data: Cow::Owned(channel_chunk(&encode_vec(&dvc).expect("encode DVC data"))), + }); + + encode_vec(&X224(indication)).expect("encode MCS indication") +} + +/// An `ActiveStage` whose EGFX channel is open and past capability negotiation. +fn active_stage_with_active_egfx() -> ActiveStage { + // Register by listener so the channel is reachable by type, which is how + // `ActiveStage` looks the EGFX processor up. + let mut drdynvc = + DrdynvcClient::new().with_dynamic_channel(GraphicsPipelineClient::new(Box::new(NoopEgfxHandler), None)); + + // Open the channel and get past capability negotiation before the stage exists, so + // the frame under test carries nothing but the reset and the drawing after it. + let create = DrdynvcServerPdu::Create(CreateRequestPdu::new( + EGFX_DVC_ID, + ironrdp_egfx::CHANNEL_NAME.to_owned(), + )); + drdynvc + .process(&encode_vec(&create).expect("encode create request")) + .expect("open EGFX channel"); + + let confirm = GfxPdu::CapabilitiesConfirm(CapabilitiesConfirmPdu::from_typed(&CapabilitySet::V8 { + flags: CapabilitiesV8Flags::empty(), + })); + let raw = wrap_uncompressed(&encode_vec(&confirm).expect("encode caps confirm")); + let dvc = DrdynvcServerPdu::Data(DrdynvcDataPdu::Data(DataPdu::new(EGFX_DVC_ID, raw))); + drdynvc + .process(&encode_vec(&dvc).expect("encode DVC data")) + .expect("confirm capabilities"); + + let mut static_channels = StaticChannelSet::new(); + assert!(static_channels.insert(drdynvc).is_none()); + assert!( + static_channels + .attach_channel_id(TypeId::of::(), DRDYNVC_CHANNEL_ID) + .is_none() + ); + + ActiveStageBuilder { + static_channels, + user_channel_id: USER_CHANNEL_ID, + io_channel_id: IO_CHANNEL_ID, + message_channel_id: None, + share_id: 1, + compression_type: None, + enable_server_pointer: false, + pointer_software_rendering: false, + } + .build() +} + +/// The image adopts the new output size before the deltas from the same payload are +/// composited into it. +/// +/// A server changing resolution sends `ResetGraphics` and the drawing that repaints the +/// new desktop together, and it does not send those deltas again. The compositor already +/// holds them by the time the stage drains it, so an image still sized for the previous +/// output silently drops everything beyond the old bounds — see +/// `an_out_of_bounds_delta_is_dropped_not_unioned` for that half. The result on screen is +/// the previous desktop left behind under the new resolution: the tearing this guards. +/// +/// The fill here lands entirely outside the old 640x480 image and inside the new +/// 1024x768 one, so it can only survive if the resize happened first. +#[test] +fn egfx_reset_resizes_the_image_before_compositing_the_same_payload() { + let mut stage = active_stage_with_active_egfx(); + let mut image = DecodedImage::new(PixelFormat::RgbA32, OLD_WIDTH, OLD_HEIGHT); + + let fill = ExclusiveRectangle { + left: 704, + top: 512, + right: 768, + bottom: 576, + }; + assert!( + fill.left >= OLD_WIDTH && fill.top >= OLD_HEIGHT, + "the fill has to start outside the old image for this test to mean anything" + ); + + let frame = egfx_frame(&[ + GfxPdu::ResetGraphics(ResetGraphicsPdu { + width: u32::from(NEW_WIDTH), + height: u32::from(NEW_HEIGHT), + monitors: Vec::new(), + }), + GfxPdu::CreateSurface(CreateSurfacePdu { + surface_id: 1, + width: NEW_WIDTH, + height: NEW_HEIGHT, + pixel_format: GfxPixelFormat::XRgb, + }), + GfxPdu::MapSurfaceToOutput(MapSurfaceToOutputPdu { + surface_id: 1, + output_origin_x: 0, + output_origin_y: 0, + }), + GfxPdu::StartFrame(StartFramePdu { + timestamp: Timestamp { + milliseconds: 0, + seconds: 0, + minutes: 0, + hours: 0, + }, + frame_id: 1, + }), + GfxPdu::SolidFill(SolidFillPdu { + surface_id: 1, + fill_pixel: Color { + b: 0x10, + g: 0x20, + r: 0x30, + xa: 0, + }, + rectangles: vec![fill.clone()], + }), + GfxPdu::EndFrame(EndFramePdu { frame_id: 1 }), + ]); + + let outputs = stage + .process(&mut image, Action::X224, &frame) + .expect("process EGFX reset frame"); + + assert_eq!( + (image.width(), image.height()), + (NEW_WIDTH, NEW_HEIGHT), + "the image must follow the output size the server just set" + ); + + let regions: Vec<_> = outputs + .iter() + .filter_map(|output| match output { + ActiveStageOutput::GraphicsUpdate(region) => Some(region), + _ => None, + }) + .collect(); + assert_eq!( + regions.len(), + 1, + "the fill from this payload must be reported, not dropped as out of bounds" + ); + + let region = regions[0]; + assert!( + region.left <= fill.left + && region.top <= fill.top + && region.right >= fill.right - 1 + && region.bottom >= fill.bottom - 1, + "reported region {region:?} must cover the fill {fill:?}" + ); + + // The pixels really landed, in the image's RGBA order. + let stride = usize::from(image.width()) * 4; + let offset = usize::from(fill.top) * stride + usize::from(fill.left) * 4; + assert_eq!( + &image.data()[offset..offset + 4], + &[0x30, 0x20, 0x10, 0xFF], + "the filled pixel must be present at its new-output coordinates" + ); +} diff --git a/crates/ironrdp/Cargo.toml b/crates/ironrdp/Cargo.toml index c4595af206..235b370cfb 100644 --- a/crates/ironrdp/Cargo.toml +++ b/crates/ironrdp/Cargo.toml @@ -82,6 +82,7 @@ ironrdp-vmconnect = { path = "../ironrdp-vmconnect", version = "0.1", optional = [dev-dependencies] ironrdp-blocking = { path = "../ironrdp-blocking", version = "0.10" } ironrdp-cliprdr-native = { path = "../ironrdp-cliprdr-native", version = "0.7" } +ironrdp-egfx = { path = "../ironrdp-egfx", version = "0.3" } anyhow = "1" async-trait = "0.1" image = { version = "0.25", default-features = false, features = ["png"] } @@ -109,5 +110,10 @@ name = "server" doc-scrape-examples = true required-features = ["cliprdr", "connector", "rdpsnd", "server"] +[[example]] +name = "rdp_stress" +doc-scrape-examples = false +required-features = ["session", "connector", "graphics", "dvc", "displaycontrol"] + [lints] workspace = true diff --git a/crates/ironrdp/examples/rdp_stress.rs b/crates/ironrdp/examples/rdp_stress.rs new file mode 100644 index 0000000000..ee45471b9e --- /dev/null +++ b/crates/ironrdp/examples/rdp_stress.rs @@ -0,0 +1,1299 @@ +//! Standalone RDP resize-stability stress harness. +//! +//! Connects straight to an RDP server over TCP + TLS/CredSSP, negotiates EGFX and +//! Display Control, then drives resolution changes and key injection while grading +//! the decoded framebuffer. No browser and no WASM: everything here talks to +//! `ironrdp-session` directly, so a failure points at the protocol/decode path +//! rather than at a client's canvas plumbing. +//! +//! Three metrics, all computed locally so they cannot be fooled by the code under test: +//! +//! - black tiles: how much of the picture is missing outright. +//! - stale tiles: how much of the picture is still the previous frame stretched over the +//! new desktop size, i.e. content the server never repainted after ResetGraphics. +//! This is what "torn"/"ghosted" looks like on screen. +//! - seam score: edge energy on the 64-pixel RemoteFX tile grid relative to tile +//! interiors. Mismatched tiles show up as a hard grid. +//! +//! # Usage example +//! +//! ```shell +//! cargo run --example=rdp_stress --features "session,connector,graphics,dvc,displaycontrol" -- \ +//! --host rdp.example.com -u Administrator --rounds 10 --out-dir /tmp/rdp-stress +//! ``` +//! +//! The password is read from `--password` or, preferably, the `RDP_PASSWORD` env var. + +#![allow(unused_crate_dependencies)] // false positives because there is both a library and a binary +#![allow(clippy::print_stdout)] +// The grading code is percentage arithmetic over tile and pixel counts: every value is a +// small count or a 0..=100 ratio, so f32 has room to spare and a lost fraction of a +// percent cannot change a verdict. +#![allow( + clippy::as_conversions, + clippy::cast_precision_loss, + clippy::cast_possible_truncation +)] + +use core::sync::atomic::{AtomicU32, Ordering}; +use core::time::Duration; +use std::io::Write as _; +use std::net::TcpStream; +use std::path::{Path, PathBuf}; +use std::sync::Arc; +use std::time::Instant; + +use anyhow::Context as _; +use ironrdp::connector::connection_activation::{ConnectionActivationFactory, ConnectionActivationState}; +use ironrdp::connector::{self, ConnectionResult, Credentials}; +use ironrdp::core::WriteBuf; +use ironrdp::pdu::gcc::{ConnectionType, KeyboardType}; +use ironrdp::pdu::input::fast_path::{FastPathInputEvent, KeyboardFlags}; +use ironrdp::pdu::rdp::capability_sets::MajorPlatformType; +use ironrdp::session::image::DecodedImage; +use ironrdp::session::{ActiveStage, ActiveStageBuilder, ActiveStageOutput}; +use ironrdp_displaycontrol::client::DisplayControlClient; +use ironrdp_dvc::DrdynvcClient; +use ironrdp_egfx::client::{GraphicsPipelineClient, GraphicsPipelineHandler, Surface}; +use ironrdp_pdu::rdp::client_info::{CompressionType, PerformanceFlags, TimezoneInfo}; +use sspi::network_client::reqwest_network_client::ReqwestNetworkClient; +use tokio_rustls::rustls; +use tracing::{debug, info}; + +const HELP: &str = "\ +USAGE: + cargo run --example=rdp_stress --features \"session,connector,graphics,dvc,displaycontrol\" -- \\ + --host [--port ] -u [-p ] [-d ] + [--sizes ] [--rounds ] [--settle-ms ] [--threshold ] + [--no-credssp] [--autologon] + [--burst ] [--burst-gap-ms ] [--jitter] [--keys ] + [--redraw none|auto|refresh|suppress] + [--out-dir ] [--dump-all] + +The password is taken from RDP_PASSWORD when -p is omitted. --no-credssp drops NLA, +which servers like xrdp (security_layer=tls) need. + +--threshold is the percentage of graded tiles allowed to be black *or* stale before a +step counts as a failure; the process exits non-zero if any step does. + +--sizes cycles desktop sizes; a big jump such as 1280x720,1920x1080 is what entering +fullscreen does. --burst replays a dragged window edge: several Display Control +requests in a row, the last one being the target size. --keys accepts ctrl+alt+del, +ctrl+esc, win, esc and is sent once per round. + +--redraw asks the server to repaint the whole desktop after each resolution change: +refresh sends Refresh Rect, suppress toggles Suppress Output, auto prefers whichever +the server advertised. The default (none) matches what the web client does today. +"; + +/// Blackness metric: side of the sampling tile, in pixels. +const TILE: usize = 16; +/// A tile counts as black when at least this fraction of its pixels are black. +const TILE_BLACK_FRACTION: f32 = 0.995; +/// RemoteFX progressive tile side: the grid stale content and seams align to. +const GFX_TILE: usize = 64; +/// A tile counts as stale when this fraction of its pixels still match the stretch. +const TILE_STALE_FRACTION: f32 = 0.98; +/// Tiles flatter than this (max-min per channel) carry no evidence either way. +const FLAT_TILE_RANGE: u8 = 12; + +fn main() -> anyhow::Result<()> { + let config = match parse_args() { + Ok(Some(config)) => config, + Ok(None) => { + println!("{HELP}"); + return Ok(()); + } + Err(e) => { + println!("{HELP}"); + return Err(e.context("invalid argument(s)")); + } + }; + + setup_logging()?; + + if let Some(dir) = &config.out_dir { + std::fs::create_dir_all(dir).context("create output directory")?; + } + + let failures = run(config)?; + + if failures > 0 { + anyhow::bail!("{failures} step(s) exceeded the black-tile or stale-tile threshold"); + } + + Ok(()) +} + +#[derive(Debug)] +struct Config { + host: String, + port: u16, + no_credssp: bool, + autologon: bool, + username: String, + password: String, + domain: Option, + sizes: Vec<(u16, u16)>, + rounds: u32, + settle: Duration, + threshold: f32, + burst: u32, + burst_gap: Duration, + jitter: bool, + keys: Vec, + redraw: Redraw, + out_dir: Option, + dump_all: bool, +} + +fn parse_args() -> anyhow::Result> { + let mut args = pico_args::Arguments::from_env(); + + if args.contains(["-h", "--help"]) { + return Ok(None); + } + + let password = match args.opt_value_from_str(["-p", "--password"])? { + Some(password) => password, + None => std::env::var("RDP_PASSWORD").context("no -p/--password and no RDP_PASSWORD in env")?, + }; + + let sizes = match args.opt_value_from_str::<_, String>("--sizes")? { + Some(spec) => parse_sizes(&spec)?, + // Deliberately not round numbers: neither dimension is a multiple of the 64-pixel + // progressive tile, so every resize leaves partial tiles at the right and bottom + // edges — where stale content and seams show up first. A 1920x1080-style pair + // divides evenly and hides exactly the bug this harness looks for. + None => vec![(1558, 964), (1828, 964)], + }; + + let keys = match args.opt_value_from_str::<_, String>("--keys")? { + Some(spec) => spec.split(',').map(|k| k.trim().to_lowercase()).collect(), + None if args.contains("--cad") => vec!["ctrl+alt+del".to_owned()], + None => Vec::new(), + }; + + let redraw = match args.opt_value_from_str::<_, String>("--redraw")?.as_deref() { + None | Some("none") => Redraw::None, + Some("auto") => Redraw::Auto, + Some("refresh") => Redraw::RefreshRect, + Some("suppress") => Redraw::SuppressOutput, + Some(other) => anyhow::bail!("unknown --redraw mode: {other}"), + }; + + Ok(Some(Config { + host: args.value_from_str("--host")?, + port: args.opt_value_from_str("--port")?.unwrap_or(3389), + no_credssp: args.contains("--no-credssp"), + autologon: args.contains("--autologon"), + username: args.value_from_str(["-u", "--username"])?, + password, + domain: args.opt_value_from_str(["-d", "--domain"])?, + sizes, + rounds: args.opt_value_from_str("--rounds")?.unwrap_or(5), + settle: Duration::from_millis(args.opt_value_from_str("--settle-ms")?.unwrap_or(15_000)), + threshold: args.opt_value_from_str("--threshold")?.unwrap_or(6.0), + burst: args.opt_value_from_str("--burst")?.unwrap_or(1).max(1), + burst_gap: Duration::from_millis(args.opt_value_from_str("--burst-gap-ms")?.unwrap_or(120)), + jitter: args.contains("--jitter"), + keys, + redraw, + out_dir: args.opt_value_from_str("--out-dir")?, + dump_all: args.contains("--dump-all"), + })) +} + +/// How to nudge the server into repainting after a resolution change. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Redraw { + None, + Auto, + RefreshRect, + SuppressOutput, +} + +fn parse_sizes(spec: &str) -> anyhow::Result> { + let sizes = spec + .split(',') + .map(|entry| { + let (w, h) = entry + .trim() + .split_once(['x', 'X']) + .with_context(|| format!("expected WxH, got {entry}"))?; + Ok((w.trim().parse()?, h.trim().parse()?)) + }) + .collect::>>()?; + + anyhow::ensure!(!sizes.is_empty(), "--sizes is empty"); + + Ok(sizes) +} + +fn setup_logging() -> anyhow::Result<()> { + use tracing::metadata::LevelFilter; + use tracing_subscriber::EnvFilter; + use tracing_subscriber::prelude::*; + + let fmt_layer = tracing_subscriber::fmt::layer().compact(); + + let env_filter = EnvFilter::builder() + .with_default_directive(LevelFilter::WARN.into()) + .with_env_var("IRONRDP_LOG") + .from_env_lossy(); + + tracing_subscriber::registry() + .with(fmt_layer) + .with(env_filter) + .try_init() + .context("failed to set tracing global subscriber")?; + + Ok(()) +} + +/// EGFX pipeline counters, so a black frame can be attributed to what the server did. +#[derive(Default)] +struct Stats { + resets: AtomicU32, + surfaces_created: AtomicU32, + surfaces_deleted: AtomicU32, +} + +impl Stats { + fn snapshot(&self) -> (u32, u32, u32) { + ( + self.resets.load(Ordering::Relaxed), + self.surfaces_created.load(Ordering::Relaxed), + self.surfaces_deleted.load(Ordering::Relaxed), + ) + } +} + +struct EgfxHandler { + stats: Arc, +} + +/// Everything the pump needs besides the wire: counters, and the ability to reactivate. +struct Ctx { + stats: Arc, + activation: ConnectionActivationFactory, + refresh_rect_support: bool, + suppress_output_support: bool, +} + +impl GraphicsPipelineHandler for EgfxHandler { + fn on_reset_graphics(&mut self, width: u32, height: u32) { + self.stats.resets.fetch_add(1, Ordering::Relaxed); + debug!(width, height, "ResetGraphics"); + } + + fn on_surface_created(&mut self, _surface: &Surface) { + self.stats.surfaces_created.fetch_add(1, Ordering::Relaxed); + } + + fn on_surface_deleted(&mut self, _surface_id: u16) { + self.stats.surfaces_deleted.fetch_add(1, Ordering::Relaxed); + } +} + +fn run(config: Config) -> anyhow::Result { + let stats = Arc::new(Stats::default()); + + let connector_config = build_config(&config); + let (connection_result, mut framed) = + connect(connector_config, &config.host, config.port, Arc::clone(&stats)).context("connect")?; + + println!( + "connected desktop={}x{} gfx={:?}", + connection_result.desktop_size.width, connection_result.desktop_size.height, connection_result.compression_type + ); + + let mut image = DecodedImage::new( + ironrdp_graphics::image_processing::PixelFormat::RgbA32, + connection_result.desktop_size.width, + connection_result.desktop_size.height, + ); + + let ctx = Ctx { + stats: Arc::clone(&stats), + activation: connection_result.activation_factory, + refresh_rect_support: connection_result.refresh_rect_support, + suppress_output_support: connection_result.suppress_output_support, + }; + + let mut stage = ActiveStageBuilder { + static_channels: connection_result.static_channels, + user_channel_id: connection_result.user_channel_id, + io_channel_id: connection_result.io_channel_id, + message_channel_id: connection_result.message_channel_id, + share_id: connection_result.share_id, + compression_type: connection_result.compression_type, + enable_server_pointer: connection_result.enable_server_pointer, + pointer_software_rendering: connection_result.pointer_software_rendering, + } + .build(); + + // Shorten the read timeout now that CredSSP is done: idle detection drives every wait below. + set_read_timeout(&mut framed, Duration::from_millis(400))?; + + let mut failures = 0; + // `gate` is false for steps whose result is not the client's to get right: the + // Ctrl+Alt+Del security desktop, for one, is legitimately an all-black screen. + let mut grade = |step: &str, measured: &Measurement, image: &DecodedImage, gate: bool| -> anyhow::Result<()> { + report(step, measured); + let failed = measured.failed(&config); + if failed && gate { + failures += 1; + } + if let Some(dir) = &config.out_dir { + if config.dump_all || failed { + save_png(image, dir, step)?; + } + } + Ok(()) + }; + + let first = settle(&mut framed, &mut stage, &mut image, &config, None, &ctx)?; + grade("first-frame", &first, &image, true)?; + + let mut next_size = 0; + for round in 1..=config.rounds { + for key in &config.keys { + let before = snapshot(&image); + send_keys(&mut framed, &mut stage, &mut image, key)?; + let measured = settle(&mut framed, &mut stage, &mut image, &config, Some(&before), &ctx)?; + grade(&format!("round{round}-{key}"), &measured, &image, false)?; + } + + for _ in 0..config.sizes.len() { + let (width, height) = config.sizes[next_size % config.sizes.len()]; + next_size += 1; + let width = if config.jitter { + jitter_width(width, &config) + } else { + width + }; + + let before = snapshot(&image); + let resets_before = stats.resets.load(Ordering::Relaxed); + resize_burst( + &mut framed, + &mut stage, + &mut image, + width, + height, + config.burst, + config.burst_gap, + &ctx, + )?; + + if config.redraw != Redraw::None { + // Let ResetGraphics land first, otherwise the server answers the redraw + // request against the old desktop size. + pump(&mut framed, &mut stage, &mut image, Duration::from_millis(1200), &ctx)?; + request_redraw(&mut framed, &stage, &image, &ctx, config.redraw)?; + } + + // A burst chains several resets, so the stretched reference is no longer + // bit-exact and the stale metric would lie: only grade it on single resets. + let reference = (config.burst == 1).then_some(&before); + let mut measured = settle(&mut framed, &mut stage, &mut image, &config, reference, &ctx)?; + // Resets can land while the burst is still being sent, before settle starts watching. + measured.resets_during = stats.resets.load(Ordering::Relaxed) - resets_before; + grade( + &format!("round{round}-resize-{width}x{height}"), + &measured, + &image, + true, + )?; + } + } + + let (resets, created, deleted) = stats.snapshot(); + println!("--- summary: failures={failures} resets={resets} surfaces_created={created} surfaces_deleted={deleted}"); + + Ok(failures) +} + +/// A copy of the framebuffer, used as the "what did the previous frame look like" reference. +struct Frame { + data: Vec, + width: usize, + height: usize, +} + +fn snapshot(image: &DecodedImage) -> Frame { + Frame { + data: image.data().to_vec(), + width: usize::from(image.width()), + height: usize::from(image.height()), + } +} + +/// One grading of the decoded framebuffer. +#[derive(Debug)] +struct Measurement { + size: (u16, u16), + black_tile_pct: f32, + black_pixel_pct: f32, + /// Columns that are black for more than half the rows, as pixel ranges. + black_bands: Vec<(usize, usize)>, + /// Share of textured tiles still identical to the stretched previous frame. + stale_tile_pct: Option, + stale_bands: Vec<(usize, usize)>, + seam: f32, + pdus: u32, + graphics_updates: u32, + reactivations: u32, + waited: Duration, + resets_during: u32, +} + +impl Measurement { + /// Whether the picture is unacceptable, by the same rule the settle loop uses to + /// decide it is done waiting. + /// + /// Stale tiles count, not just black ones. Stale *is* the tearing this harness + /// exists to catch: a resize that leaves the previous frame stretched over the new + /// desktop is fully painted and perfectly non-black, so grading on blackness alone + /// reports success on exactly the bug under test. + fn failed(&self, config: &Config) -> bool { + self.black_tile_pct > config.threshold || self.stale_tile_pct.is_some_and(|stale| stale > config.threshold) + } +} + +fn report(step: &str, m: &Measurement) { + println!( + "{step}: {}x{} black_tiles={:.2}% black_px={:.2}% black_bands={} stale_tiles={} stale_bands={} seam={:.2} pdus={} gfx={} resets={} reactivations={} waited={}ms", + m.size.0, + m.size.1, + m.black_tile_pct, + m.black_pixel_pct, + fmt_bands(&m.black_bands), + m.stale_tile_pct + .map_or_else(|| String::from("n/a"), |p| format!("{p:.2}%")), + fmt_bands(&m.stale_bands), + m.seam, + m.pdus, + m.graphics_updates, + m.resets_during, + m.reactivations, + m.waited.as_millis(), + ); +} + +fn fmt_bands(bands: &[(usize, usize)]) -> String { + if bands.is_empty() { + return String::from("-"); + } + bands + .iter() + .map(|(x0, x1)| format!("{x0}..{x1}")) + .collect::>() + .join(",") +} + +/// Pick a width near `target`, the way a dragged window edge lands on arbitrary sizes. +fn jitter_width(target: u16, config: &Config) -> u16 { + use rand::Rng as _; + let low = config.sizes.iter().map(|(w, _)| *w).min().unwrap_or(target); + let high = config.sizes.iter().map(|(w, _)| *w).max().unwrap_or(target); + let span = i32::from(high.saturating_sub(low)).max(4); + let delta = rand::rng().random_range(-span / 4..=span / 4); + i32::from(target) + .saturating_add(delta) + .clamp(i32::from(low), i32::from(high)) + .try_into() + .unwrap_or(target) +} + +/// Fire several Display Control requests back to back, ending on the target size. +/// +/// A single request is the easy case; the interesting one is a storm of them, where +/// a later ResetGraphics can land while the framebuffer is still being repainted. +#[expect(clippy::too_many_arguments, reason = "stress knobs, all independent")] +fn resize_burst( + framed: &mut UpgradedFramed, + stage: &mut ActiveStage, + image: &mut DecodedImage, + width: u16, + height: u16, + burst: u32, + gap: Duration, + ctx: &Ctx, +) -> anyhow::Result<()> { + for step in 0..burst { + // Intermediate steps walk toward the target, the last one nails it exactly. + // Repeating the target size also covers the same-size ResetGraphics case. + let intermediate = if step + 1 == burst { + width + } else { + let offset = u16::try_from((burst - step - 1) * 24).unwrap_or(0); + width.saturating_add(offset).max(640) + }; + + request_resize(framed, stage, image, u32::from(intermediate), u32::from(height), ctx)?; + + if step + 1 < burst { + pump(framed, stage, image, gap, ctx)?; + } + } + + Ok(()) +} + +/// Ask the server to repaint the whole desktop. +/// +/// The web client deliberately skips this after ResetGraphics, on the theory that with +/// RDPGFX these PDUs do not invalidate the surface cache. This switch exists to check +/// that theory against a real server instead of taking it on faith. +fn request_redraw( + framed: &mut UpgradedFramed, + stage: &ActiveStage, + image: &DecodedImage, + ctx: &Ctx, + mode: Redraw, +) -> anyhow::Result<()> { + let (refresh, suppress) = match mode { + Redraw::None => return Ok(()), + Redraw::Auto => (ctx.refresh_rect_support, ctx.suppress_output_support), + Redraw::RefreshRect => (true, false), + Redraw::SuppressOutput => (false, true), + }; + + let redraw_frames = stage + .request_full_redraw(image.width(), image.height(), refresh, suppress) + .context("encode full redraw request")?; + + if redraw_frames.is_empty() { + println!(" redraw requested but the server advertised neither Refresh Rect nor Suppress Output"); + } + + for frame in redraw_frames { + framed.write_all(&frame).context("write redraw request")?; + } + + Ok(()) +} + +/// Inject a key combo. Ctrl+Alt+Del drives the server through a full output reset; +/// Ctrl+Esc is the cheap way to prove the input path works at all (Start menu). +fn send_keys( + framed: &mut UpgradedFramed, + stage: &mut ActiveStage, + image: &mut DecodedImage, + combo: &str, +) -> anyhow::Result<()> { + const CTRL: (u8, bool) = (0x1D, false); + const ALT: (u8, bool) = (0x38, false); + const DELETE: (u8, bool) = (0x53, true); + const ESCAPE: (u8, bool) = (0x01, false); + const WIN: (u8, bool) = (0x5B, true); + + let keys: &[(u8, bool)] = match combo { + "ctrl+alt+del" | "cad" => &[CTRL, ALT, DELETE], + "ctrl+esc" => &[CTRL, ESCAPE], + "win" => &[WIN], + "esc" => &[ESCAPE], + other => anyhow::bail!("unknown key combo: {other}"), + }; + + let flags = |extended: bool, release: bool| { + let mut flags = KeyboardFlags::empty(); + if extended { + flags |= KeyboardFlags::EXTENDED; + } + if release { + flags |= KeyboardFlags::RELEASE; + } + flags + }; + + let mut events: Vec = keys + .iter() + .map(|&(code, ext)| FastPathInputEvent::KeyboardEvent(flags(ext, false), code)) + .collect(); + events.extend( + keys.iter() + .rev() + .map(|&(code, ext)| FastPathInputEvent::KeyboardEvent(flags(ext, true), code)), + ); + + for out in stage + .process_fastpath_input(image, &events) + .with_context(|| format!("encode {combo}"))? + { + if let ActiveStageOutput::ResponseFrame(frame) = out { + framed.write_all(&frame).with_context(|| format!("write {combo}"))?; + } + } + + Ok(()) +} + +/// Ask the server for a new desktop size over Display Control, retrying until the +/// channel reports its capabilities. +fn request_resize( + framed: &mut UpgradedFramed, + stage: &mut ActiveStage, + image: &mut DecodedImage, + width: u32, + height: u32, + ctx: &Ctx, +) -> anyhow::Result<()> { + for attempt in 1..=30 { + match stage.encode_resize(width, height, None, None) { + Some(frame) => { + let frame = frame.context("encode Display Control resize")?; + framed.write_all(&frame).context("write resize")?; + debug!(width, height, attempt, "Display Control resize sent"); + return Ok(()); + } + None => { + // Capabilities not in yet: keep the session moving and try again. + pump(framed, stage, image, Duration::from_millis(500), ctx)?; + } + } + } + + anyhow::bail!("Display Control never became ready; cannot drive a resize") +} + +/// Pump the session until the picture is healthy again, or until the settle budget runs out. +fn settle( + framed: &mut UpgradedFramed, + stage: &mut ActiveStage, + image: &mut DecodedImage, + config: &Config, + reference: Option<&Frame>, + ctx: &Ctx, +) -> anyhow::Result { + let started = Instant::now(); + let resets_before = ctx.stats.resets.load(Ordering::Relaxed); + let mut pdus = 0; + let mut graphics_updates = 0; + let mut reactivations = 0; + + loop { + let outcome = pump(framed, stage, image, Duration::from_millis(500), ctx)?; + pdus += outcome.pdus; + graphics_updates += outcome.graphics_updates; + reactivations += outcome.reactivations; + + let elapsed = started.elapsed(); + let mut measured = measure(image, reference); + measured.pdus = pdus; + measured.graphics_updates = graphics_updates; + measured.reactivations = reactivations; + measured.waited = elapsed; + measured.resets_during = ctx.stats.resets.load(Ordering::Relaxed) - resets_before; + + if outcome.terminated { + anyhow::bail!("server terminated the session after {}ms", elapsed.as_millis()); + } + + // Stop as soon as the picture looks finished, but only after the server had a + // chance to speak: an immediate sample still shows the pre-change frame. + let healthy = !measured.failed(config); + + if elapsed >= budget_floor() && (healthy || elapsed >= config.settle) { + return Ok(measured); + } + } +} + +/// Minimum time to keep watching, so an early healthy sample cannot end the step. +fn budget_floor() -> Duration { + Duration::from_secs(3) +} + +#[derive(Default)] +struct PumpOutcome { + pdus: u32, + graphics_updates: u32, + reactivations: u32, + terminated: bool, +} + +/// Run the Deactivation-Reactivation Sequence the server asked for. +/// +/// Ctrl+Alt+Del switches the server to the secure desktop, which deactivates the +/// share. A client that ignores this gets no further output at all and keeps showing +/// a stretched copy of the last frame, so the harness has to do what a real client does. +fn reactivate( + framed: &mut UpgradedFramed, + stage: &mut ActiveStage, + image: &mut DecodedImage, + factory: &ConnectionActivationFactory, +) -> anyhow::Result<()> { + use ironrdp::connector::Sequence as _; + + // Reactivation is a request/response handshake; the idle-poll timeout would break it. + set_read_timeout(framed, Duration::from_secs(10))?; + let restore = |framed: &mut UpgradedFramed| set_read_timeout(framed, Duration::from_millis(400)); + + let mut sequence = factory.create(); + let mut buf = WriteBuf::new(); + + loop { + buf.clear(); + + let written = match sequence.next_pdu_hint() { + Some(hint) => { + let pdu = framed.read_by_hint(hint).context("read activation PDU")?; + sequence.step(&pdu, framed.last_read_at(), &mut buf) + } + None => sequence.step_no_input(&mut buf), + } + .context("activation step")?; + + if let Some(len) = written.size() { + framed.write_all(&buf[..len]).context("write activation response")?; + } + + if let ConnectionActivationState::Finalized { + desktop_size, + share_id, + enable_server_pointer, + pointer_software_rendering, + static_channel_chunk_size, + window_support_level, + .. + } = sequence.connection_activation_state() + { + // Start the new desktop empty rather than carrying the old frame across: + // the harness must not invent content it then measures. + *image = DecodedImage::new( + ironrdp_graphics::image_processing::PixelFormat::RgbA32, + desktop_size.width, + desktop_size.height, + ); + if !stage.reactivate( + sequence.io_channel_id(), + sequence.user_channel_id(), + share_id, + enable_server_pointer, + pointer_software_rendering, + static_channel_chunk_size, + ) { + restore(framed)?; + anyhow::bail!("invalid static channel chunk size during reactivation"); + } + stage.set_window_support_level(window_support_level); + debug!(?desktop_size, "reactivated"); + restore(framed)?; + return Ok(()); + } + } +} + +/// Read and process PDUs for at most `budget`, answering with any response frames. +fn pump( + framed: &mut UpgradedFramed, + stage: &mut ActiveStage, + image: &mut DecodedImage, + budget: Duration, + ctx: &Ctx, +) -> anyhow::Result { + let started = Instant::now(); + let mut outcome = PumpOutcome::default(); + + while started.elapsed() < budget { + let (action, payload) = match framed.read_pdu() { + Ok(pdu) => pdu, + Err(e) if is_idle(&e) => break, + Err(e) => return Err(anyhow::Error::new(e).context("read frame")), + }; + + outcome.pdus += 1; + + for out in stage.process(image, action, &payload).context("process frame")? { + match out { + ActiveStageOutput::ResponseFrame(frame) => framed.write_all(&frame).context("write response")?, + ActiveStageOutput::GraphicsUpdate(_) => outcome.graphics_updates += 1, + ActiveStageOutput::DeactivateAll => { + reactivate(framed, stage, image, &ctx.activation).context("reactivate")?; + outcome.reactivations += 1; + } + ActiveStageOutput::Terminate(_) => { + outcome.terminated = true; + return Ok(outcome); + } + _ => {} + } + } + } + + Ok(outcome) +} + +fn is_idle(e: &std::io::Error) -> bool { + matches!( + e.kind(), + std::io::ErrorKind::WouldBlock | std::io::ErrorKind::TimedOut | std::io::ErrorKind::Interrupted + ) +} + +/// Grade the framebuffer: black content, stale content, and tile-grid seams. +/// +/// Deliberately written from scratch instead of reusing anything from +/// `ironrdp-session`, so the measurement cannot be fooled by the code under test. +fn measure(image: &DecodedImage, reference: Option<&Frame>) -> Measurement { + let current = Frame { + data: image.data().to_vec(), + width: usize::from(image.width()), + height: usize::from(image.height()), + }; + + let mut measurement = Measurement { + size: (image.width(), image.height()), + black_tile_pct: 100.0, + black_pixel_pct: 100.0, + black_bands: Vec::new(), + stale_tile_pct: None, + stale_bands: Vec::new(), + seam: 0.0, + pdus: 0, + graphics_updates: 0, + reactivations: 0, + waited: Duration::ZERO, + resets_during: 0, + }; + + if current.width == 0 || current.height == 0 || current.data.len() < current.width * current.height * 4 { + return measurement; + } + + let (black_tile_pct, black_pixel_pct, black_bands) = black_stats(¤t); + measurement.black_tile_pct = black_tile_pct; + measurement.black_pixel_pct = black_pixel_pct; + measurement.black_bands = black_bands; + measurement.seam = seam_score(¤t); + + if let Some(previous) = reference { + let stretched = stretch(previous, current.width, current.height); + let (stale_pct, stale_bands) = stale_stats(¤t, &stretched); + measurement.stale_tile_pct = Some(stale_pct); + measurement.stale_bands = stale_bands; + } + + measurement +} + +fn black_stats(frame: &Frame) -> (f32, f32, Vec<(usize, usize)>) { + let is_black = |x: usize, y: usize| { + let i = (y * frame.width + x) * 4; + frame.data[i + 3] == 0 || (frame.data[i] == 0 && frame.data[i + 1] == 0 && frame.data[i + 2] == 0) + }; + + let cols = frame.width / TILE; + let rows = frame.height / TILE; + let mut black_tiles = 0usize; + let mut black_pixels = 0usize; + let mut col_black = vec![0usize; cols]; + + for row in 0..rows { + for (col, black_rows) in col_black.iter_mut().enumerate() { + let mut black = 0usize; + for y in 0..TILE { + for x in 0..TILE { + if is_black(col * TILE + x, row * TILE + y) { + black += 1; + } + } + } + black_pixels += black; + if black as f32 >= (TILE * TILE) as f32 * TILE_BLACK_FRACTION { + black_tiles += 1; + *black_rows += 1; + } + } + } + + let tiles = (cols * rows).max(1); + let sampled = (tiles * TILE * TILE) as f32; + + ( + black_tiles as f32 * 100.0 / tiles as f32, + black_pixels as f32 * 100.0 / sampled, + column_bands(&col_black, rows, TILE), + ) +} + +/// Nearest-neighbour stretch, matching what the session does to carry a frame across +/// a resolution change: a pixel that still matches it was never repainted. +fn stretch(previous: &Frame, width: usize, height: usize) -> Frame { + let mut data = vec![0u8; width * height * 4]; + if previous.width == 0 || previous.height == 0 || width == 0 || height == 0 { + return Frame { data, width, height }; + } + + for y in 0..height { + let src_y = y * previous.height / height; + for x in 0..width { + let src_x = x * previous.width / width; + let src = (src_y * previous.width + src_x) * 4; + let dst = (y * width + x) * 4; + data[dst..dst + 4].copy_from_slice(&previous.data[src..src + 4]); + } + } + + Frame { data, width, height } +} + +/// Share of textured 64x64 tiles still identical to the stretched previous frame. +/// +/// Flat tiles (a plain wallpaper, a black band) are skipped: a correct repaint of a +/// flat area is indistinguishable from a stale one, so they carry no evidence. +fn stale_stats(current: &Frame, stretched: &Frame) -> (f32, Vec<(usize, usize)>) { + let cols = current.width / GFX_TILE; + let rows = current.height / GFX_TILE; + let mut considered = 0usize; + let mut stale = 0usize; + let mut col_stale = vec![0usize; cols]; + + for row in 0..rows { + for (col, stale_rows) in col_stale.iter_mut().enumerate() { + let mut matching = 0usize; + let mut min = [255u8; 3]; + let mut max = [0u8; 3]; + + for y in 0..GFX_TILE { + for x in 0..GFX_TILE { + let i = ((row * GFX_TILE + y) * current.width + col * GFX_TILE + x) * 4; + for channel in 0..3 { + min[channel] = min[channel].min(current.data[i + channel]); + max[channel] = max[channel].max(current.data[i + channel]); + } + if current.data[i..i + 3] == stretched.data[i..i + 3] { + matching += 1; + } + } + } + + let flat = (0..3).all(|c| max[c].saturating_sub(min[c]) <= FLAT_TILE_RANGE); + if flat { + continue; + } + + considered += 1; + if matching as f32 >= (GFX_TILE * GFX_TILE) as f32 * TILE_STALE_FRACTION { + stale += 1; + *stale_rows += 1; + } + } + } + + if considered == 0 { + return (0.0, Vec::new()); + } + + ( + stale as f32 * 100.0 / considered as f32, + column_bands(&col_stale, rows, GFX_TILE), + ) +} + +/// Edge energy on the 64-pixel tile grid relative to tile interiors. +/// +/// A correct picture has no idea where the tile grid is, so the ratio sits near 1. +/// Mismatched or half-updated tiles paint a visible grid and push it up. +fn seam_score(frame: &Frame) -> f32 { + let luma = |x: usize, y: usize| { + let i = (y * frame.width + x) * 4; + i32::from(frame.data[i]) * 299 + i32::from(frame.data[i + 1]) * 587 + i32::from(frame.data[i + 2]) * 114 + }; + + let mut boundary = 0i64; + let mut boundary_n = 0i64; + let mut interior = 0i64; + let mut interior_n = 0i64; + + for y in 0..frame.height { + for x in 1..frame.width { + let delta = i64::from((luma(x, y) - luma(x - 1, y)).abs()); + if x % GFX_TILE == 0 { + boundary += delta; + boundary_n += 1; + } else if x % GFX_TILE == GFX_TILE / 2 { + interior += delta; + interior_n += 1; + } + } + } + + if boundary_n == 0 || interior_n == 0 || interior == 0 { + return 0.0; + } + + (boundary as f64 / boundary_n as f64 / (interior as f64 / interior_n as f64)) as f32 +} + +/// Collapse per-column tile counts into pixel ranges where most rows are affected. +fn column_bands(col_counts: &[usize], rows: usize, tile: usize) -> Vec<(usize, usize)> { + let mut bands = Vec::new(); + let affected = |col: usize| rows > 0 && col_counts[col] * 2 > rows; + + let mut col = 0; + while col < col_counts.len() { + if affected(col) { + let start = col; + while col < col_counts.len() && affected(col) { + col += 1; + } + bands.push((start * tile, col * tile)); + } else { + col += 1; + } + } + + bands +} + +fn save_png(image: &DecodedImage, dir: &Path, step: &str) -> anyhow::Result<()> { + let buffer: image::ImageBuffer, _> = + image::ImageBuffer::from_raw(u32::from(image.width()), u32::from(image.height()), image.data()) + .context("invalid image")?; + let path = dir.join(format!("{step}.png")); + buffer.save(&path).context("save image to disk")?; + println!(" dumped {}", path.display()); + Ok(()) +} + +fn build_config(config: &Config) -> connector::Config { + connector::Config { + credentials: Credentials::UsernamePassword { + username: config.username.clone(), + password: config.password.clone(), + }, + domain: config.domain.clone(), + // xrdp defaults to security_layer=tls and speaks no CredSSP, so it needs plain + // TLS: keeping NLA on makes the handshake fail before a single frame arrives. + enable_tls: config.no_credssp, + enable_credssp: !config.no_credssp, + enable_standard_rdp_security: false, + keyboard_type: KeyboardType::IBM_ENHANCED, + keyboard_subtype: 0, + keyboard_layout: 0, + keyboard_functional_keys_count: 12, + connection_type: ConnectionType::Lan, + ime_file_name: String::new(), + dig_product_id: String::new(), + desktop_size: connector::DesktopSize { + width: config.sizes[0].0, + height: config.sizes[0].1, + }, + monitor_layout: None, + bitmap: None, + client_build: 0, + client_name: "ironrdp-rdp-stress".to_owned(), + client_dir: "C:\\Windows\\System32\\mstscax.dll".to_owned(), + + #[cfg(windows)] + platform: MajorPlatformType::WINDOWS, + #[cfg(target_os = "macos")] + platform: MajorPlatformType::MACINTOSH, + #[cfg(target_os = "linux")] + platform: MajorPlatformType::UNIX, + + enable_server_pointer: false, + request_data: None, + autologon: config.autologon, + enable_audio_playback: false, + enable_audio_capture: false, + compression_type: Some(CompressionType::Rdp61), + pointer_software_rendering: true, + multitransport_flags: None, + // The whole point of this harness: exercise the EGFX / ResetGraphics path. + support_dyn_vc_gfx_protocol: true, + performance_flags: PerformanceFlags::default(), + desktop_scale_factor: 0, + hardware_id: None, + license_cache: None, + timezone_info: TimezoneInfo::default(), + alternate_shell: String::new(), + work_dir: String::new(), + remote_application_mode: false, + rail_support_level: ironrdp_pdu::rdp::capability_sets::RailSupportLevel::empty(), + } +} + +type UpgradedFramed = ironrdp_blocking::Framed>; + +fn set_read_timeout(framed: &mut UpgradedFramed, timeout: Duration) -> anyhow::Result<()> { + let (stream, _) = framed.get_inner_mut(); + stream.sock.set_read_timeout(Some(timeout)).context("set_read_timeout") +} + +fn connect( + config: connector::Config, + server_name: &str, + port: u16, + stats: Arc, +) -> anyhow::Result<(ConnectionResult, UpgradedFramed)> { + let server_addr = lookup_addr(server_name, port).context("lookup addr")?; + + info!(%server_addr, "Looked up server address"); + + let tcp_stream = TcpStream::connect(server_addr).context("TCP connect")?; + // An emulated (amd64-on-arm64) xrdp box can take ~30s just to answer the TLS + // negotiation, so the handshake needs far more slack than a frame read. + tcp_stream + .set_read_timeout(Some(Duration::from_secs(90))) + .context("set_read_timeout")?; + + let client_addr = tcp_stream.local_addr().context("get socket local address")?; + + let mut framed = ironrdp_blocking::Framed::new(tcp_stream); + + // EGFX plus Display Control is what a modern client (and MSRDC) negotiates; without + // both, a resize never reaches ResetGraphics and there is nothing to reproduce. + let drdynvc = DrdynvcClient::new() + .with_dynamic_channel(DisplayControlClient::new(|_| Ok(Vec::new()))) + .with_dynamic_channel(GraphicsPipelineClient::new(Box::new(EgfxHandler { stats }), None)); + + let mut connector = connector::ClientConnector::new(config, client_addr).with_static_channel(drdynvc); + + let should_upgrade = ironrdp_blocking::connect_begin(&mut framed, &mut connector).context("begin connection")?; + + debug!("TLS upgrade"); + + let initial_stream = framed.into_inner_no_leftover(); + let (upgraded_stream, server_public_key) = + tls_upgrade(initial_stream, server_name.to_owned()).context("TLS upgrade")?; + + let upgraded = ironrdp_blocking::mark_as_upgraded(should_upgrade, &mut connector); + + let mut upgraded_framed = ironrdp_blocking::Framed::new(upgraded_stream); + + let mut network_client = ReqwestNetworkClient; + let connection_result = ironrdp_blocking::connect_finalize( + upgraded, + connector, + &mut upgraded_framed, + &mut network_client, + server_name.to_owned().into(), + server_public_key, + None, + ) + .context("finalize connection")?; + + Ok((connection_result, upgraded_framed)) +} + +fn lookup_addr(hostname: &str, port: u16) -> anyhow::Result { + use std::net::ToSocketAddrs as _; + let addr = (hostname, port) + .to_socket_addrs()? + .next() + .context("socket address not found")?; + Ok(addr) +} + +fn tls_upgrade( + stream: TcpStream, + server_name: String, +) -> anyhow::Result<(rustls::StreamOwned, Vec)> { + let mut config = rustls::client::ClientConfig::builder() + .dangerous() + .with_custom_certificate_verifier(Arc::new(danger::NoCertificateVerification)) + .with_no_client_auth(); + + config.key_log = Arc::new(rustls::KeyLogFile::new()); + config.resumption = rustls::client::Resumption::disabled(); + + let config = Arc::new(config); + + let server_name = server_name.try_into()?; + + let client = rustls::ClientConnection::new(config, server_name)?; + + let mut tls_stream = rustls::StreamOwned::new(client, stream); + + tls_stream.flush()?; + + let cert = tls_stream + .conn + .peer_certificates() + .and_then(|certificates| certificates.first()) + .context("peer certificate is missing")?; + + let server_public_key = extract_tls_server_public_key(cert)?; + + Ok((tls_stream, server_public_key)) +} + +fn extract_tls_server_public_key(cert: &[u8]) -> anyhow::Result> { + use x509_cert::der::Decode as _; + + let cert = x509_cert::Certificate::from_der(cert)?; + + debug!(subject = %cert.tbs_certificate().subject()); + + let server_public_key = cert + .tbs_certificate() + .subject_public_key_info() + .subject_public_key + .as_bytes() + .context("subject public key BIT STRING is not aligned")? + .to_owned(); + + Ok(server_public_key) +} + +mod danger { + use tokio_rustls::rustls::client::danger::{HandshakeSignatureValid, ServerCertVerified, ServerCertVerifier}; + use tokio_rustls::rustls::{DigitallySignedStruct, Error, SignatureScheme, pki_types}; + + #[derive(Debug)] + pub(super) struct NoCertificateVerification; + + impl ServerCertVerifier for NoCertificateVerification { + fn verify_server_cert( + &self, + _: &pki_types::CertificateDer<'_>, + _: &[pki_types::CertificateDer<'_>], + _: &pki_types::ServerName<'_>, + _: &[u8], + _: pki_types::UnixTime, + ) -> Result { + Ok(ServerCertVerified::assertion()) + } + + fn verify_tls12_signature( + &self, + _: &[u8], + _: &pki_types::CertificateDer<'_>, + _: &DigitallySignedStruct, + ) -> Result { + Ok(HandshakeSignatureValid::assertion()) + } + + fn verify_tls13_signature( + &self, + _: &[u8], + _: &pki_types::CertificateDer<'_>, + _: &DigitallySignedStruct, + ) -> Result { + Ok(HandshakeSignatureValid::assertion()) + } + + fn supported_verify_schemes(&self) -> Vec { + vec![ + SignatureScheme::RSA_PKCS1_SHA1, + SignatureScheme::ECDSA_SHA1_Legacy, + SignatureScheme::RSA_PKCS1_SHA256, + SignatureScheme::ECDSA_NISTP256_SHA256, + SignatureScheme::RSA_PKCS1_SHA384, + SignatureScheme::ECDSA_NISTP384_SHA384, + SignatureScheme::RSA_PKCS1_SHA512, + SignatureScheme::ECDSA_NISTP521_SHA512, + SignatureScheme::RSA_PSS_SHA256, + SignatureScheme::RSA_PSS_SHA384, + SignatureScheme::RSA_PSS_SHA512, + SignatureScheme::ED25519, + SignatureScheme::ED448, + ] + } + } +} diff --git a/crates/ironrdp/src/lib.rs b/crates/ironrdp/src/lib.rs index 02c312ae7c..6368c32733 100644 --- a/crates/ironrdp/src/lib.rs +++ b/crates/ironrdp/src/lib.rs @@ -4,9 +4,9 @@ #[cfg(test)] use { - anyhow as _, async_trait as _, image as _, ironrdp_blocking as _, ironrdp_cliprdr_native as _, opus2 as _, - pico_args as _, rand as _, sspi as _, tokio as _, tokio_rustls as _, tracing as _, tracing_subscriber as _, - x509_cert as _, + anyhow as _, async_trait as _, image as _, ironrdp_blocking as _, ironrdp_cliprdr_native as _, + ironrdp_egfx as _, opus2 as _, pico_args as _, rand as _, sspi as _, tokio as _, tokio_rustls as _, + tracing as _, tracing_subscriber as _, x509_cert as _, }; #[cfg(feature = "acceptor")] From 51e956207acd616f2d92b40080fd9df6b194f26f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=A8=E6=88=90=E9=94=B4?= Date: Fri, 18 Sep 2026 15:18:09 +0800 Subject: [PATCH 4/7] chore: tighten review-facing comments and drop log-only progressive API Clarify SRL docs and test notes, use neutral wording in egfx/web comments, and remove reference_count_for_surface that existed only for debug fields. --- crates/ironrdp-egfx/src/client.rs | 15 ++++++------ crates/ironrdp-graphics/src/progressive.rs | 9 ------- crates/ironrdp-graphics/src/srl.rs | 28 ++++++++++------------ crates/ironrdp-web/src/session.rs | 6 ++--- 4 files changed, 23 insertions(+), 35 deletions(-) diff --git a/crates/ironrdp-egfx/src/client.rs b/crates/ironrdp-egfx/src/client.rs index 5d64717b07..cabc9c6430 100644 --- a/crates/ironrdp-egfx/src/client.rs +++ b/crates/ironrdp-egfx/src/client.rs @@ -738,9 +738,13 @@ impl GraphicsPipelineClient { debug!( width, +<<<<<<< HEAD height, surface_count, "ResetGraphics: surfaces destroyed; tile refs dropped; progressive CONTEXT retained" +======= + height, surface_count, "ResetGraphics: surfaces destroyed; tile refs dropped; progressive CONTEXT retained" +>>>>>>> fddc02f (chore: tighten review-facing comments and drop log-only progressive API) ); if output_size.is_some() { @@ -780,19 +784,15 @@ impl GraphicsPipelineClient { // MS-RDPEGFX: deleting a surface drops that surface's progressive tile // references. A following difference tile before a new base tile will // correctly fail with MissingTileReference. - let cleared_refs = self.progressive_decoder.reference_count_for_surface(surface_id); self.progressive_decoder.delete_surface(surface_id); if self.surfaces.remove(&surface_id).is_some() { self.compositor.delete_surface(surface_id); - debug!( - surface_id, - cleared_refs, "DeleteSurface cleared progressive tile references" - ); + debug!(surface_id, "DeleteSurface cleared progressive tile references"); self.handler.on_surface_deleted(surface_id); } else { warn!( surface_id, - cleared_refs, "DeleteSurface for unknown surface (progressive refs cleared)" + "DeleteSurface for unknown surface (progressive refs cleared)" ); } } @@ -2345,8 +2345,7 @@ mod tests { // MS-RDPEGFX 2.2.2.14 / 3.3.5.14: ResetGraphics destroys every surface. // Tile coefficient buffers belong to those surfaces. Reusing surface id 0 - // after a Display Control resize must not difference against the old - // desktop — that is the shredded frame native RDP does not produce. + // after a Display Control resize must not difference against the old desktop. assert_eq!(client.progressive_decoder.total_reference_count(), 0); client diff --git a/crates/ironrdp-graphics/src/progressive.rs b/crates/ironrdp-graphics/src/progressive.rs index 6eb887c1a3..f3d9389ee6 100644 --- a/crates/ironrdp-graphics/src/progressive.rs +++ b/crates/ironrdp-graphics/src/progressive.rs @@ -1558,15 +1558,6 @@ impl ProgressiveDecoder { self.surface_context_flags.remove(&surface_id); } - /// Number of retained difference-tile coefficient buffers for a surface. - #[must_use] - pub fn reference_count_for_surface(&self, surface_id: u16) -> usize { - self.references - .keys() - .filter(|(reference_surface_id, _, _)| *reference_surface_id == surface_id) - .count() - } - /// Total retained difference-tile coefficient buffers across all surfaces. #[must_use] pub fn total_reference_count(&self) -> usize { diff --git a/crates/ironrdp-graphics/src/srl.rs b/crates/ironrdp-graphics/src/srl.rs index 39de48f3fa..41891016af 100644 --- a/crates/ironrdp-graphics/src/srl.rs +++ b/crates/ironrdp-graphics/src/srl.rs @@ -53,9 +53,9 @@ impl<'a> SrlDecoder<'a> { /// /// Every byte is payload, and the stream is treated as zero-padded past its end — see /// [`BitReader::read_bit`]. MS-RDPEGFX 2.2.4.2.1.5.4 gives the per-component SRL stream an - /// explicit `*SrlLen`, and nothing in 3.1.8.1.5 reserves the final byte. Requiring a zero - /// terminator rejected the streams Windows actually sends, and when the last byte did happen - /// to be zero it silently dropped those eight bits from the tail. + /// explicit `*SrlLen`, and nothing in 3.1.8.1.5 reserves the final byte as a terminator, so + /// the last byte carries coefficients like any other. An empty stream is valid and means no + /// refinement in this pass. pub fn new(data: &'a [u8]) -> Result { Ok(Self { reader: BitReader::new(data), @@ -110,9 +110,9 @@ impl<'a> SrlDecoder<'a> { /// A run is consumed one event at a time rather than summed up front. Its total length is /// bounded only by the coefficients the caller asks for, so a run that outlives the current /// band simply carries over, and one that outlives the tile is dropped with the decoder. - /// Summing the whole run eagerly needed a cap to stay finite, and that cap rejected the long - /// all-zero runs Windows sends for mostly static tiles — the discarded tiles are the blocks - /// that never refresh. FreeRDP's `progressive_rfx_srl_read` applies no bound either. + /// This is what lets the long all-zero runs Windows sends for mostly static tiles decode + /// without a bound on the run itself; FreeRDP's `progressive_rfx_srl_read` applies none + /// either. fn read_zero_run_event(&mut self) { let k = self.kp / 8; @@ -307,8 +307,8 @@ impl<'a> BitReader<'a> { /// own length: once the remaining coefficients in a band are all zero the encoder simply /// stops emitting bits. Windows relies on this, and so does the reference decoder — FreeRDP's /// `BitStream_Fetch` leaves its prefetch register zeroed when the offset passes capacity. - /// Erroring out instead discarded the whole tile, which is what surfaced as blocks that - /// never refresh. Over-reads are counted so a desync stays visible in the logs. + /// Treating the end of the stream as an error would discard the tile the caller is still + /// decoding. Over-reads are counted so a desync stays visible in the logs. fn read_bit(&mut self) -> bool { let Some(&byte) = self.data.get(self.byte_idx) else { self.overread_bits = self.overread_bits.saturating_add(1); @@ -448,13 +448,11 @@ mod tests { #[test] fn decodes_a_zero_run_longer_than_the_encoder_would_emit() { - // All-zero bits are a chain of "at least `1 << k` more zeros" events, and `k` grows - // with KP up to 1024, so the sum far exceeds one tile's worth of coefficients. The - // decoder used to sum the whole run up front and cap it at 4096, so the very long - // zero runs a static region produces were judged corrupt and the whole tile update - // was dropped — the blocks that never refresh. The reference implementation consumes - // the run event by event with no bound at all: however long it runs it is only zeros, - // and the excess is dropped along with the decoder. + // All-zero bits are a chain of "at least `1 << k` more zeros" events, and `k` climbs + // with KP until each event contributes 1024 (`1 << 10`, since KP caps at 80), so the + // run far exceeds one tile's worth of coefficients. Summing it up front and capping it + // at 4096 judges the very long zero runs a static region produces to be corrupt and + // drops the whole tile update. Consuming the run event by event needs no bound. assert_eq!(decode_srl(&[0x00; 32], 8, 4), Ok(vec![0; 8])); assert_eq!(decode_srl(&[0x00; 32], MAX_ZERO_RUN, 4), Ok(vec![0; MAX_ZERO_RUN])); } diff --git a/crates/ironrdp-web/src/session.rs b/crates/ironrdp-web/src/session.rs index 483fcf8ed1..4a6ed6b503 100644 --- a/crates/ironrdp-web/src/session.rs +++ b/crates/ironrdp-web/src/session.rs @@ -573,7 +573,7 @@ pub(crate) enum RdpInputEvent { }, /// Server resized the Graphics Output Buffer (MS-RDPEGFX 2.2.2.14). This is how a modern /// Windows host answers a Display Control request: no Deactivation-Reactivation Sequence, - /// so it is the only chance we get to follow the new desktop size. + /// so the canvas must follow the new desktop size here. GraphicsReset { width: u32, height: u32, @@ -1604,7 +1604,7 @@ fn build_config( pointer_software_rendering: false, multitransport_flags: None, // Prefer MS-RDPEGFX when the server supports it — classic bitmap updates - // paint dirty rectangles and look "blocky" on full refreshes. + // repaint only dirty rectangles and look coarse on full refreshes. support_dyn_vc_gfx_protocol: true, performance_flags: PerformanceFlags::default(), desktop_scale_factor: 0, @@ -1755,7 +1755,7 @@ async fn connect( // Advertise SUPPORT_DYN_VC_GFX in Config, and actually register the EGFX // DVC here. Without GraphicsPipelineClient the server stays on classic - // dirty-rectangle bitmaps (blocky full-screen refreshes). + // dirty-rectangle bitmaps (coarse full-screen refreshes). // // No H.264 decoder in WASM yet — GraphicsPipelineClient filters AVC caps // and falls back to V8 / ClearCodec / RFX Progressive. From 50303b73c86e457fe0856547025d4c1b768998d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=A8=E6=88=90=E9=94=B4?= Date: Fri, 18 Sep 2026 15:43:40 +0800 Subject: [PATCH 5/7] fix(egfx): remove leftover merge conflict markers in client.rs --- crates/ironrdp-egfx/src/client.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/crates/ironrdp-egfx/src/client.rs b/crates/ironrdp-egfx/src/client.rs index cabc9c6430..b2a128cfc0 100644 --- a/crates/ironrdp-egfx/src/client.rs +++ b/crates/ironrdp-egfx/src/client.rs @@ -738,13 +738,7 @@ impl GraphicsPipelineClient { debug!( width, -<<<<<<< HEAD - height, - surface_count, - "ResetGraphics: surfaces destroyed; tile refs dropped; progressive CONTEXT retained" -======= height, surface_count, "ResetGraphics: surfaces destroyed; tile refs dropped; progressive CONTEXT retained" ->>>>>>> fddc02f (chore: tighten review-facing comments and drop log-only progressive API) ); if output_size.is_some() { From e283f3ef6b33f1489133d57603004308e4d715f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=A8=E6=88=90=E9=94=B4?= Date: Wed, 30 Sep 2026 17:59:13 +0800 Subject: [PATCH 6/7] fix(web): update desktop_size from the image before the canvas callback ResetGraphics resizes DecodedImage in the same process() as the first GraphicsUpdate, but GraphicsReset is dequeued on a later iteration. Copy the image size into the Cell first so canvas_resized_callback and desktop_size() agree. Also restore SessionErrorKind::Pdu Display to the STYLE.md convention (inner error stays on source()). --- crates/ironrdp-session/src/lib.rs | 2 +- crates/ironrdp-web/src/session.rs | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/crates/ironrdp-session/src/lib.rs b/crates/ironrdp-session/src/lib.rs index 5b92c78b54..3103da0a37 100644 --- a/crates/ironrdp-session/src/lib.rs +++ b/crates/ironrdp-session/src/lib.rs @@ -38,7 +38,7 @@ pub enum SessionErrorKind { impl fmt::Display for SessionErrorKind { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match &self { - SessionErrorKind::Pdu(e) => write!(f, "PDU error: {e}"), + SessionErrorKind::Pdu(_) => write!(f, "PDU error"), SessionErrorKind::Encode(_) => write!(f, "encode error"), SessionErrorKind::Decode(_) => write!(f, "decode error"), SessionErrorKind::FastPathBulkDecompression(_) => write!(f, "fast-path bulk decompression error"), diff --git a/crates/ironrdp-web/src/session.rs b/crates/ironrdp-web/src/session.rs index 4a6ed6b503..6a67ba9e68 100644 --- a/crates/ironrdp-web/src/session.rs +++ b/crates/ironrdp-web/src/session.rs @@ -949,7 +949,14 @@ impl iron_remote_desktop::Session for Session { // `process()` may have resized `image` to follow ResetGraphics in the same // frame. The canvas has to match *before* the GraphicsUpdate from that frame - // is drawn. + // is drawn. `desktop_size` is updated from `image` first so + // `canvas_resized_callback` (and `desktop_size()`) see the size in effect; + // `GraphicsReset` is dequeued on a later iteration. + let width = image.width(); + let height = image.height(); + if width > 0 && height > 0 { + self.desktop_size.set(connector::DesktopSize { width, height }); + } sync_canvas_to_image( &mut gui, &image, From 77d05a64682e1da68ed04475f9a78ab7a78e9741 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=A8=E6=88=90=E9=94=B4?= Date: Sat, 3 Oct 2026 12:58:35 +0800 Subject: [PATCH 7/7] chore: move the resize grading harness into ironrdp-stress examples/ is for small library demos. This is a live-host analysis tool in the same class as ironrdp-capture-replay, so it belongs in its own unpublished crate rather than the facade examples. --- Cargo.lock | 22 ++++++++- crates/ironrdp-stress/Cargo.toml | 45 +++++++++++++++++++ crates/ironrdp-stress/README.md | 20 +++++++++ .../src/main.rs} | 12 ++--- crates/ironrdp/Cargo.toml | 6 --- crates/ironrdp/src/lib.rs | 2 +- 6 files changed, 93 insertions(+), 14 deletions(-) create mode 100644 crates/ironrdp-stress/Cargo.toml create mode 100644 crates/ironrdp-stress/README.md rename crates/{ironrdp/examples/rdp_stress.rs => ironrdp-stress/src/main.rs} (98%) diff --git a/Cargo.lock b/Cargo.lock index dba0c163ed..75b9b0eec9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2569,7 +2569,6 @@ dependencies = [ "ironrdp-displaycontrol", "ironrdp-dvc", "ironrdp-echo", - "ironrdp-egfx", "ironrdp-graphics", "ironrdp-input", "ironrdp-mstsgu", @@ -3364,6 +3363,27 @@ dependencies = [ "ironrdp-core 0.2.1", ] +[[package]] +name = "ironrdp-stress" +version = "0.0.0" +dependencies = [ + "anyhow", + "image", + "ironrdp", + "ironrdp-blocking", + "ironrdp-displaycontrol", + "ironrdp-dvc", + "ironrdp-egfx", + "ironrdp-pdu", + "pico-args", + "rand 0.9.4", + "sspi", + "tokio-rustls", + "tracing", + "tracing-subscriber", + "x509-cert", +] + [[package]] name = "ironrdp-svc" version = "0.8.0" diff --git a/crates/ironrdp-stress/Cargo.toml b/crates/ironrdp-stress/Cargo.toml new file mode 100644 index 0000000000..f5eaab7b5e --- /dev/null +++ b/crates/ironrdp-stress/Cargo.toml @@ -0,0 +1,45 @@ +[package] +name = "ironrdp-stress" +version = "0.0.0" +publish = false +readme = "README.md" +description = "Live RDP graphics-pipeline resize grading harness" +edition.workspace = true +rust-version = "1.94" +license.workspace = true +homepage.workspace = true +repository.workspace = true +authors.workspace = true +keywords.workspace = true +categories.workspace = true + +[[bin]] +name = "ironrdp-stress" +path = "src/main.rs" + +[dependencies] +anyhow = "1" +image = { version = "0.25", default-features = false, features = ["png"] } +ironrdp = { path = "../ironrdp", version = "0.17.0", features = [ + "session", + "connector", + "graphics", + "dvc", + "displaycontrol", + "pdu", +] } +ironrdp-blocking = { path = "../ironrdp-blocking", version = "0.10" } +ironrdp-displaycontrol = { path = "../ironrdp-displaycontrol", version = "0.8" } +ironrdp-dvc = { path = "../ironrdp-dvc", version = "0.8" } +ironrdp-egfx = { path = "../ironrdp-egfx", version = "0.3" } +ironrdp-pdu = { path = "../ironrdp-pdu", version = "0.9" } +pico-args = "0.5" +rand = "0.9" +sspi = { version = "0.21", features = ["network_client"] } +tokio-rustls = "0.26" +tracing = { version = "0.1", features = ["log"] } +tracing-subscriber = { version = "0.3", features = ["env-filter"] } +x509-cert = { version = "0.3", default-features = false, features = ["std"] } + +[lints] +workspace = true diff --git a/crates/ironrdp-stress/README.md b/crates/ironrdp-stress/README.md new file mode 100644 index 0000000000..981b8681e5 --- /dev/null +++ b/crates/ironrdp-stress/README.md @@ -0,0 +1,20 @@ +# ironrdp-stress + +Live RDP graphics-pipeline resize grading harness. + +Connects to a real RDP server over TCP + TLS/CredSSP, negotiates EGFX and Display +Control, then drives resolution changes while scoring the decoded framebuffer +(black tiles, stale tiles, RemoteFX seam energy). It talks to `ironrdp-session` +directly, so a failure points at the protocol/decode path rather than at a +client's canvas plumbing. + +This is an internal analysis tool, in the same class as `ironrdp-capture-replay` +and `ironrdp-bench`. It is not an example, and it is not part of `cargo test`: +it needs a live host. + +```shell +cargo run -p ironrdp-stress -- \ + --host rdp.example.com -u Administrator --rounds 10 --out-dir /tmp/rdp-stress +``` + +The password is read from `--password` or, preferably, `RDP_PASSWORD`. diff --git a/crates/ironrdp/examples/rdp_stress.rs b/crates/ironrdp-stress/src/main.rs similarity index 98% rename from crates/ironrdp/examples/rdp_stress.rs rename to crates/ironrdp-stress/src/main.rs index ee45471b9e..d1599d062b 100644 --- a/crates/ironrdp/examples/rdp_stress.rs +++ b/crates/ironrdp-stress/src/main.rs @@ -1,4 +1,4 @@ -//! Standalone RDP resize-stability stress harness. +//! Live RDP graphics-pipeline resize grading harness. //! //! Connects straight to an RDP server over TCP + TLS/CredSSP, negotiates EGFX and //! Display Control, then drives resolution changes and key injection while grading @@ -18,13 +18,13 @@ //! # Usage example //! //! ```shell -//! cargo run --example=rdp_stress --features "session,connector,graphics,dvc,displaycontrol" -- \ +//! cargo run -p ironrdp-stress -- \ //! --host rdp.example.com -u Administrator --rounds 10 --out-dir /tmp/rdp-stress //! ``` //! //! The password is read from `--password` or, preferably, the `RDP_PASSWORD` env var. -#![allow(unused_crate_dependencies)] // false positives because there is both a library and a binary +#![allow(unused_crate_dependencies)] // bin-only crate; deps are used from main #![allow(clippy::print_stdout)] // The grading code is percentage arithmetic over tile and pixel counts: every value is a // small count or a 0..=100 ratio, so f32 has room to spare and a lost fraction of a @@ -62,7 +62,7 @@ use tracing::{debug, info}; const HELP: &str = "\ USAGE: - cargo run --example=rdp_stress --features \"session,connector,graphics,dvc,displaycontrol\" -- \\ + cargo run -p ironrdp-stress -- \\ --host [--port ] -u [-p ] [-d ] [--sizes ] [--rounds ] [--settle-ms ] [--threshold ] [--no-credssp] [--autologon] @@ -309,7 +309,7 @@ fn run(config: Config) -> anyhow::Result { ); let mut image = DecodedImage::new( - ironrdp_graphics::image_processing::PixelFormat::RgbA32, + ironrdp::graphics::image_processing::PixelFormat::RgbA32, connection_result.desktop_size.width, connection_result.desktop_size.height, ); @@ -764,7 +764,7 @@ fn reactivate( // Start the new desktop empty rather than carrying the old frame across: // the harness must not invent content it then measures. *image = DecodedImage::new( - ironrdp_graphics::image_processing::PixelFormat::RgbA32, + ironrdp::graphics::image_processing::PixelFormat::RgbA32, desktop_size.width, desktop_size.height, ); diff --git a/crates/ironrdp/Cargo.toml b/crates/ironrdp/Cargo.toml index 235b370cfb..c4595af206 100644 --- a/crates/ironrdp/Cargo.toml +++ b/crates/ironrdp/Cargo.toml @@ -82,7 +82,6 @@ ironrdp-vmconnect = { path = "../ironrdp-vmconnect", version = "0.1", optional = [dev-dependencies] ironrdp-blocking = { path = "../ironrdp-blocking", version = "0.10" } ironrdp-cliprdr-native = { path = "../ironrdp-cliprdr-native", version = "0.7" } -ironrdp-egfx = { path = "../ironrdp-egfx", version = "0.3" } anyhow = "1" async-trait = "0.1" image = { version = "0.25", default-features = false, features = ["png"] } @@ -110,10 +109,5 @@ name = "server" doc-scrape-examples = true required-features = ["cliprdr", "connector", "rdpsnd", "server"] -[[example]] -name = "rdp_stress" -doc-scrape-examples = false -required-features = ["session", "connector", "graphics", "dvc", "displaycontrol"] - [lints] workspace = true diff --git a/crates/ironrdp/src/lib.rs b/crates/ironrdp/src/lib.rs index 6368c32733..f38ed31d04 100644 --- a/crates/ironrdp/src/lib.rs +++ b/crates/ironrdp/src/lib.rs @@ -5,7 +5,7 @@ #[cfg(test)] use { anyhow as _, async_trait as _, image as _, ironrdp_blocking as _, ironrdp_cliprdr_native as _, - ironrdp_egfx as _, opus2 as _, pico_args as _, rand as _, sspi as _, tokio as _, tokio_rustls as _, + opus2 as _, pico_args as _, rand as _, sspi as _, tokio as _, tokio_rustls as _, tracing as _, tracing_subscriber as _, x509_cert as _, };