From f894c67259c3fcf53a1bf446472cd0be7b4fa187 Mon Sep 17 00:00:00 2001 From: truebest Date: Sun, 12 Jul 2026 20:37:46 -0700 Subject: [PATCH 1/5] feat(egfx): wire RemoteFX Progressive decode into WireToSurface2 dispatch GraphicsPipelineClient::handle_pdu currently only forwards WireToSurface2 (the progressive codec) to handler.on_wire_to_surface2() and returns, without decoding it -- unlike AVC420 (decode_avc420) and ClearCode's WireToSurface1 dispatch (#1175), which both decode into BitmapUpdate callbacks. Add a progressive_decoder field to GraphicsPipelineClient, decode WireToSurface2 streams through it, and emit each updated 64x64 tile as a BitmapUpdate through the existing on_bitmap_updated path. A decode failure now propagates as a terminal error instead of being logged and dropped, matching decode_avc420's behavior. ResetGraphics and DeleteEncodingContext now clear the decoder's per-context tile state, since both destroy the codec_context_id(s) scoped to them and a later reused id must not decode against stale tiles. --- crates/ironrdp-egfx/src/client.rs | 210 ++++++++++++++++++++++++++++++ 1 file changed, 210 insertions(+) diff --git a/crates/ironrdp-egfx/src/client.rs b/crates/ironrdp-egfx/src/client.rs index d4757dad6..24ccb991d 100644 --- a/crates/ironrdp-egfx/src/client.rs +++ b/crates/ironrdp-egfx/src/client.rs @@ -58,6 +58,7 @@ use std::collections::BTreeMap; use ironrdp_core::{Decode as _, ReadCursor, impl_as_any}; use ironrdp_dvc::{DvcClientProcessor, DvcMessage, DvcProcessor}; use ironrdp_graphics::clearcodec::ClearCodecDecoder; +use ironrdp_graphics::progressive::ProgressiveDecoder; use ironrdp_graphics::rdp6::BitmapStreamDecoder; use ironrdp_graphics::zgfx; use ironrdp_pdu::geometry::ExclusiveRectangle; @@ -398,6 +399,7 @@ pub struct GraphicsPipelineClient { decompressor: zgfx::Decompressor, decompressed_buffer: Vec, + progressive_decoder: ProgressiveDecoder, state: ClientState, negotiated_caps: Option, @@ -423,6 +425,7 @@ impl GraphicsPipelineClient { planar_decoder: BitmapStreamDecoder::default(), decompressor: zgfx::Decompressor::new(), decompressed_buffer: Vec::new(), + progressive_decoder: ProgressiveDecoder::new(), state: ClientState::WaitingForConfirm, negotiated_caps: None, codec_caps: CodecCapabilities::default(), @@ -519,6 +522,7 @@ impl GraphicsPipelineClient { GfxPdu::WireToSurface2(pdu) => { trace!("WireToSurface2 (progressive codec)"); self.handler.on_wire_to_surface2(&pdu); + self.handle_wire_to_surface2(pdu)?; Ok(vec![]) } GfxPdu::EndFrame(end) => self.handle_end_frame(end.frame_id), @@ -610,6 +614,7 @@ impl GraphicsPipelineClient { codec_context_id = pdu.codec_context_id, "DeleteEncodingContext" ); + self.progressive_decoder.delete_context(pdu.codec_context_id); self.handler.on_delete_encoding_context(&pdu); Ok(vec![]) } @@ -675,6 +680,7 @@ impl GraphicsPipelineClient { if let Some(ref mut decoder) = self.h264_decoder { decoder.reset(); } + self.progressive_decoder.reset(); // The ClearCodec decoder is deliberately NOT reset here. MS-RDPEGFX 3.3.5.14 only // resizes the Graphics Output Buffer; cache lifetime is driven by the stream instead, // through CLEARCODEC_FLAG_CACHE_RESET (2.2.4.1), which ClearCodecDecoder::decode @@ -790,6 +796,49 @@ impl GraphicsPipelineClient { Ok(()) } + /// Decode a RemoteFX Progressive (`WireToSurface2`) bitmap stream and emit each + /// updated 64x64 tile through `on_bitmap_updated`. + fn handle_wire_to_surface2(&mut self, pdu: WireToSurface2Pdu) -> PduResult<()> { + let Some(surface) = self.surfaces.get(&pdu.surface_id) else { + warn!(surface_id = pdu.surface_id, "WireToSurface2 for unknown surface"); + return Ok(()); + }; + let (surface_width, surface_height) = (surface.width, surface.height); + + let tiles = match self.progressive_decoder.decode_bitmap( + pdu.codec_context_id, + surface_width, + surface_height, + &pdu.bitmap_data, + ) { + Ok(tiles) => tiles, + Err(e) => { + warn!(error = ?e, "RFX progressive decode failed"); + return Err(pdu_other_err!("RFX progressive decode failed")); + } + }; + + for tile in tiles { + let left = tile.x_idx.saturating_mul(64); + let top = tile.y_idx.saturating_mul(64); + let update = BitmapUpdate { + surface_id: pdu.surface_id, + destination_rectangle: ExclusiveRectangle { + left, + top, + right: left.saturating_add(64), + bottom: top.saturating_add(64), + }, + codec_id: Codec1Type::Uncompressed, + data: tile.pixels, + width: 64, + height: 64, + }; + self.handler.on_bitmap_updated(&update); + } + Ok(()) + } + fn decode_avc420(&mut self, surface_id: u16, dest_rect: &ExclusiveRectangle, bitmap_data: &[u8]) -> PduResult<()> { let mut cursor = ReadCursor::new(bitmap_data); let stream = Avc420BitmapStream::decode(&mut cursor).map_err(|e| decode_err!(e))?; @@ -1408,4 +1457,165 @@ mod tests { "no advertised set enables AVC420" ); } + + /// Builds a minimal single-tile RFX Progressive stream, optionally opening a codec + /// context (SYNC+CONTEXT) first. + fn build_progressive_stream(with_context: bool) -> Vec { + use ironrdp_pdu::codecs::rfx::RfxRectangle; + use ironrdp_pdu::codecs::rfx::progressive::{ + ProgressiveBlock, ProgressiveContextPdu, ProgressiveFrameBeginPdu, ProgressiveFrameEndPdu, + ProgressiveRegion, ProgressiveSyncPdu, encode_progressive_stream, + }; + + let 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![], + }; + + let mut blocks = Vec::new(); + if with_context { + blocks.push(ProgressiveBlock::Sync(ProgressiveSyncPdu)); + blocks.push(ProgressiveBlock::Context(ProgressiveContextPdu { + context_id: 0, + tile_size: 0x0040, + flags: 0, + })); + } + blocks.push(ProgressiveBlock::FrameBegin(ProgressiveFrameBeginPdu { + frame_index: 0, + region_count: 1, + })); + blocks.push(ProgressiveBlock::Region(region)); + blocks.push(ProgressiveBlock::FrameEnd(ProgressiveFrameEndPdu)); + + encode_progressive_stream(&blocks).unwrap() + } + + #[test] + fn wire_to_surface2_decode_failure_propagates_error() { + use crate::pdu::Codec2Type; + + let mut client = GraphicsPipelineClient::new(Box::new(TestHandler), None); + let _ = client.handle_pdu(GfxPdu::CreateSurface(crate::pdu::CreateSurfacePdu { + surface_id: 1, + width: 640, + height: 480, + pixel_format: PixelFormat::XRgb, + })); + + let result = client.handle_pdu(GfxPdu::WireToSurface2(WireToSurface2Pdu { + surface_id: 1, + codec_id: Codec2Type::RemoteFxProgressive, + codec_context_id: 7, + pixel_format: PixelFormat::XRgb, + bitmap_data: build_progressive_stream(false), + })); + + assert!( + result.is_err(), + "a progressive decode failure must propagate as a terminal error, not be silently dropped" + ); + } + + #[test] + fn reset_graphics_clears_progressive_decoder_context() { + use crate::pdu::Codec2Type; + + let mut client = GraphicsPipelineClient::new(Box::new(TestHandler), None); + let _ = client.handle_pdu(GfxPdu::CreateSurface(crate::pdu::CreateSurfacePdu { + surface_id: 1, + width: 640, + height: 480, + pixel_format: PixelFormat::XRgb, + })); + + let result = client.handle_pdu(GfxPdu::WireToSurface2(WireToSurface2Pdu { + surface_id: 1, + codec_id: Codec2Type::RemoteFxProgressive, + codec_context_id: 7, + pixel_format: PixelFormat::XRgb, + bitmap_data: build_progressive_stream(true), + })); + assert!( + result.is_ok(), + "establishing the context should succeed: {:?}", + result.as_ref().err() + ); + + let _ = client.handle_pdu(GfxPdu::ResetGraphics(crate::pdu::ResetGraphicsPdu { + width: 1920, + height: 1080, + monitors: vec![], + })); + let _ = client.handle_pdu(GfxPdu::CreateSurface(crate::pdu::CreateSurfacePdu { + surface_id: 1, + width: 640, + height: 480, + pixel_format: PixelFormat::XRgb, + })); + + let result = client.handle_pdu(GfxPdu::WireToSurface2(WireToSurface2Pdu { + surface_id: 1, + codec_id: Codec2Type::RemoteFxProgressive, + codec_context_id: 7, + pixel_format: PixelFormat::XRgb, + bitmap_data: build_progressive_stream(false), + })); + assert!( + result.is_err(), + "progressive decoder context must not survive ResetGraphics" + ); + } + + #[test] + fn delete_encoding_context_clears_progressive_decoder_context() { + use crate::pdu::Codec2Type; + + let mut client = GraphicsPipelineClient::new(Box::new(TestHandler), None); + let _ = client.handle_pdu(GfxPdu::CreateSurface(crate::pdu::CreateSurfacePdu { + surface_id: 1, + width: 640, + height: 480, + pixel_format: PixelFormat::XRgb, + })); + + let result = client.handle_pdu(GfxPdu::WireToSurface2(WireToSurface2Pdu { + surface_id: 1, + codec_id: Codec2Type::RemoteFxProgressive, + codec_context_id: 7, + pixel_format: PixelFormat::XRgb, + bitmap_data: build_progressive_stream(true), + })); + assert!( + result.is_ok(), + "establishing the context should succeed: {:?}", + result.as_ref().err() + ); + + let _ = client.handle_pdu(GfxPdu::DeleteEncodingContext(DeleteEncodingContextPdu { + surface_id: 1, + codec_context_id: 7, + })); + + let result = client.handle_pdu(GfxPdu::WireToSurface2(WireToSurface2Pdu { + surface_id: 1, + codec_id: Codec2Type::RemoteFxProgressive, + codec_context_id: 7, + pixel_format: PixelFormat::XRgb, + bitmap_data: build_progressive_stream(false), + })); + assert!( + result.is_err(), + "progressive decoder context must not survive DeleteEncodingContext" + ); + } } From 9537c38f7e1332b5cc2e8c9f1cafe51e4e0b73ff Mon Sep 17 00:00:00 2001 From: truebest Date: Sun, 12 Jul 2026 20:48:03 -0700 Subject: [PATCH 2/5] address review: unknown-surface error, lowercase message, clip edge tiles - handle_wire_to_surface2 now returns a terminal error for an unknown surface instead of logging and silently returning Ok(()), matching handle_wire_to_surface1's existing behavior. - Lowercase the progressive-decode-failure error message to match this file's convention. - Clip each tile's destination_rectangle/width/height to the surface bounds and crop the RGBA buffer with the existing crop_decoded_frame helper, so a surface whose dimensions aren't a multiple of 64 no longer gets a BitmapUpdate claiming pixels outside the surface. --- crates/ironrdp-egfx/src/client.rs | 32 ++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/crates/ironrdp-egfx/src/client.rs b/crates/ironrdp-egfx/src/client.rs index 24ccb991d..43cd8ce1d 100644 --- a/crates/ironrdp-egfx/src/client.rs +++ b/crates/ironrdp-egfx/src/client.rs @@ -799,10 +799,10 @@ impl GraphicsPipelineClient { /// Decode a RemoteFX Progressive (`WireToSurface2`) bitmap stream and emit each /// updated 64x64 tile through `on_bitmap_updated`. fn handle_wire_to_surface2(&mut self, pdu: WireToSurface2Pdu) -> PduResult<()> { - let Some(surface) = self.surfaces.get(&pdu.surface_id) else { - warn!(surface_id = pdu.surface_id, "WireToSurface2 for unknown surface"); - return Ok(()); - }; + let surface = self + .surfaces + .get(&pdu.surface_id) + .ok_or_else(|| pdu_other_err!("unknown surface in WireToSurface2"))?; let (surface_width, surface_height) = (surface.width, surface.height); let tiles = match self.progressive_decoder.decode_bitmap( @@ -813,26 +813,36 @@ impl GraphicsPipelineClient { ) { Ok(tiles) => tiles, Err(e) => { - warn!(error = ?e, "RFX progressive decode failed"); - return Err(pdu_other_err!("RFX progressive decode failed")); + warn!(error = ?e, "rfx progressive decode failed"); + return Err(pdu_other_err!("rfx progressive decode failed")); } }; for tile in tiles { let left = tile.x_idx.saturating_mul(64); let top = tile.y_idx.saturating_mul(64); + let width = surface_width.saturating_sub(left).min(64); + let height = surface_height.saturating_sub(top).min(64); + if width == 0 || height == 0 { + continue; + } + let data = if width == 64 && height == 64 { + tile.pixels + } else { + crop_decoded_frame(&tile.pixels, 64, 64, width, height) + }; let update = BitmapUpdate { surface_id: pdu.surface_id, destination_rectangle: ExclusiveRectangle { left, top, - right: left.saturating_add(64), - bottom: top.saturating_add(64), + right: left + width, + bottom: top + height, }, codec_id: Codec1Type::Uncompressed, - data: tile.pixels, - width: 64, - height: 64, + data, + width, + height, }; self.handler.on_bitmap_updated(&update); } From 376147fc71f6f9d82155f52b03c72d2644114a86 Mon Sep 17 00:00:00 2001 From: truebest Date: Sun, 12 Jul 2026 21:00:52 -0700 Subject: [PATCH 3/5] address review: scope progressive contexts by surface ProgressiveDecoder keyed its per-context tile state by codec_context_id alone. Per MS-RDPEGFX, codec_context_id is scoped to the surface that owns it (RDPGFX_DELETE_ENCODING_CONTEXT_PDU carries both surface_id and codec_context_id together), so two surfaces reusing the same codec_context_id value would collide: decoding the second surface could overwrite the first's tile state, and deleting one surface's context would remove the other's. decode_bitmap() and delete_context() now also take surface_id and key contexts by (surface_id, codec_context_id). Added decoder_contexts_scoped_by_surface to cover it. --- crates/ironrdp-egfx/src/client.rs | 4 +- crates/ironrdp-graphics/src/progressive.rs | 88 ++++++++++++++++++---- 2 files changed, 78 insertions(+), 14 deletions(-) diff --git a/crates/ironrdp-egfx/src/client.rs b/crates/ironrdp-egfx/src/client.rs index 43cd8ce1d..29bd4b159 100644 --- a/crates/ironrdp-egfx/src/client.rs +++ b/crates/ironrdp-egfx/src/client.rs @@ -614,7 +614,8 @@ impl GraphicsPipelineClient { codec_context_id = pdu.codec_context_id, "DeleteEncodingContext" ); - self.progressive_decoder.delete_context(pdu.codec_context_id); + self.progressive_decoder + .delete_context(pdu.surface_id, pdu.codec_context_id); self.handler.on_delete_encoding_context(&pdu); Ok(vec![]) } @@ -806,6 +807,7 @@ impl GraphicsPipelineClient { let (surface_width, surface_height) = (surface.width, surface.height); let tiles = match self.progressive_decoder.decode_bitmap( + pdu.surface_id, pdu.codec_context_id, surface_width, surface_height, diff --git a/crates/ironrdp-graphics/src/progressive.rs b/crates/ironrdp-graphics/src/progressive.rs index 96fd1f76f..c2330961c 100644 --- a/crates/ironrdp-graphics/src/progressive.rs +++ b/crates/ironrdp-graphics/src/progressive.rs @@ -1102,16 +1102,19 @@ impl From for ProgressiveDecodeError { } } -/// Per-context progressive state, identified by codec_context_id. +/// Per-context progressive state, identified by (surface_id, codec_context_id). struct ProgressiveContext { surface: SurfaceTiles, } /// High-level progressive bitmap decoder for EGFX WireToSurface2 processing. /// -/// Maintains per-context tile state across frames, keyed by `codec_context_id`. -/// Feed it progressive bitmap data from `WireToSurface2Pdu.bitmap_data` and -/// get back decoded RGBA tiles for compositing. +/// Maintains per-context tile state across frames, keyed by +/// `(surface_id, codec_context_id)`. Per MS-RDPEGFX, `codec_context_id` is scoped to +/// the surface that owns it (`RDPGFX_DELETE_ENCODING_CONTEXT_PDU` carries both), so two +/// surfaces may reuse the same `codec_context_id` value independently. Feed it +/// progressive bitmap data from `WireToSurface2Pdu.bitmap_data` and get back decoded +/// RGBA tiles for compositing. /// /// # Usage /// @@ -1120,6 +1123,7 @@ struct ProgressiveContext { /// /// // On receiving WireToSurface2Pdu: /// let tiles = decoder.decode_bitmap( +/// pdu.surface_id, /// pdu.codec_context_id, /// surface_width, surface_height, /// &pdu.bitmap_data, @@ -1130,7 +1134,7 @@ struct ProgressiveContext { /// } /// ``` pub struct ProgressiveDecoder { - contexts: BTreeMap, + contexts: BTreeMap<(u16, u32), ProgressiveContext>, } impl ProgressiveDecoder { @@ -1147,12 +1151,15 @@ impl ProgressiveDecoder { /// returns RGBA pixel data for each tile that was updated. /// /// # Arguments + /// - `surface_id`: surface ID from the WireToSurface2Pdu; `codec_context_id` is scoped + /// to this surface /// - `codec_context_id`: context ID from the WireToSurface2Pdu /// - `surface_width`: surface width in pixels (for tile grid sizing) /// - `surface_height`: surface height in pixels /// - `bitmap_data`: raw progressive block stream from the PDU pub fn decode_bitmap( &mut self, + surface_id: u16, codec_context_id: u32, surface_width: u16, surface_height: u16, @@ -1182,13 +1189,13 @@ impl ProgressiveDecoder { Some(v) => v, None => self .contexts - .get(&codec_context_id) + .get(&(surface_id, codec_context_id)) .map(|c| c.surface.use_reduce_extrapolate) .ok_or(ProgressiveDecodeError::MissingBlock("CONTEXT"))?, }; - // Get or create the context for this codec_context_id - let context = match self.contexts.entry(codec_context_id) { + // Get or create the context for this (surface_id, codec_context_id) + let context = match self.contexts.entry((surface_id, codec_context_id)) { Entry::Occupied(e) => e.into_mut(), Entry::Vacant(e) => { let surface = SurfaceTiles::new(surface_width, surface_height, use_reduce_extrapolate)?; @@ -1233,9 +1240,10 @@ impl ProgressiveDecoder { /// Delete a codec context, freeing its tile state. /// - /// Called when the server sends RDPGFX_DELETE_ENCODING_CONTEXT. - pub fn delete_context(&mut self, codec_context_id: u32) { - self.contexts.remove(&codec_context_id); + /// Called when the server sends RDPGFX_DELETE_ENCODING_CONTEXT, which carries both + /// `surface_id` and `codec_context_id`. + pub fn delete_context(&mut self, surface_id: u16, codec_context_id: u32) { + self.contexts.remove(&(surface_id, codec_context_id)); } /// Reset all contexts (e.g., on EGFX channel reset). @@ -1685,7 +1693,7 @@ mod tests { fn decoder_delete_nonexistent_context() { let mut decoder = ProgressiveDecoder::new(); // Should not panic on non-existent context - decoder.delete_context(42); + decoder.delete_context(1, 42); } #[test] @@ -1729,7 +1737,7 @@ mod tests { ]; let encoded = encode_progressive_stream(&blocks).unwrap(); - let result = decoder.decode_bitmap(1, 640, 480, &encoded); + let result = decoder.decode_bitmap(1, 1, 640, 480, &encoded); assert!(result.is_ok()); assert_eq!(decoder.contexts.len(), 1); @@ -1737,6 +1745,60 @@ mod tests { assert!(decoder.contexts.is_empty()); } + #[test] + fn decoder_contexts_scoped_by_surface() { + use ironrdp_pdu::codecs::rfx::RfxRectangle; + use ironrdp_pdu::codecs::rfx::progressive::{ + ProgressiveBlock, ProgressiveContextPdu, ProgressiveFrameBeginPdu, ProgressiveFrameEndPdu, + ProgressiveRegion, ProgressiveSyncPdu, encode_progressive_stream, + }; + + fn minimal_stream() -> Vec { + let 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![], + }; + let blocks = vec![ + ProgressiveBlock::Sync(ProgressiveSyncPdu), + ProgressiveBlock::Context(ProgressiveContextPdu { + context_id: 0, + tile_size: 0x0040, + flags: 0, + }), + ProgressiveBlock::FrameBegin(ProgressiveFrameBeginPdu { + frame_index: 0, + region_count: 1, + }), + ProgressiveBlock::Region(region), + ProgressiveBlock::FrameEnd(ProgressiveFrameEndPdu), + ]; + encode_progressive_stream(&blocks).unwrap() + } + + let mut decoder = ProgressiveDecoder::new(); + + // Two different surfaces reusing the same codec_context_id must not collide. + let result_a = decoder.decode_bitmap(1, 0, 640, 480, &minimal_stream()); + let result_b = decoder.decode_bitmap(2, 0, 800, 600, &minimal_stream()); + assert!(result_a.is_ok()); + assert!(result_b.is_ok()); + assert_eq!(decoder.contexts.len(), 2); + + // Deleting surface 1's context must leave surface 2's context intact. + decoder.delete_context(1, 0); + assert_eq!(decoder.contexts.len(), 1); + assert!(decoder.contexts.contains_key(&(2, 0))); + } + #[test] fn decoder_error_display() { let e = ProgressiveDecodeError::MissingBlock("SYNC"); From b8d305d12c5c17ddaf29457082926272a5c8bab3 Mon Sep 17 00:00:00 2001 From: truebest Date: Wed, 5 Aug 2026 20:01:37 -0700 Subject: [PATCH 4/5] address review: composite progressive region updates --- crates/ironrdp-egfx/src/client.rs | 355 +++++++++++++++++++-- crates/ironrdp-graphics/src/progressive.rs | 284 ++++++++++++++++- 2 files changed, 601 insertions(+), 38 deletions(-) diff --git a/crates/ironrdp-egfx/src/client.rs b/crates/ironrdp-egfx/src/client.rs index 29bd4b159..63652e1dc 100644 --- a/crates/ironrdp-egfx/src/client.rs +++ b/crates/ironrdp-egfx/src/client.rs @@ -510,6 +510,7 @@ impl GraphicsPipelineClient { Ok(vec![]) } GfxPdu::StartFrame(start) => { + self.progressive_decoder.begin_frame(); self.current_frame_id = Some(start.frame_id); self.frames_queued = self.frames_queued.saturating_add(1); trace!(frame_id = start.frame_id, "StartFrame"); @@ -716,6 +717,7 @@ impl GraphicsPipelineClient { } fn handle_delete_surface(&mut self, surface_id: u16) { + 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"); @@ -797,8 +799,9 @@ impl GraphicsPipelineClient { Ok(()) } - /// Decode a RemoteFX Progressive (`WireToSurface2`) bitmap stream and emit each - /// updated 64x64 tile through `on_bitmap_updated`. + /// Decode a RemoteFX Progressive (`WireToSurface2`) bitmap stream, apply each + /// REGION-clipped tile update to the compositor, and emit it through + /// `on_bitmap_updated`. fn handle_wire_to_surface2(&mut self, pdu: WireToSurface2Pdu) -> PduResult<()> { let surface = self .surfaces @@ -821,32 +824,60 @@ impl GraphicsPipelineClient { }; for tile in tiles { - let left = tile.x_idx.saturating_mul(64); - let top = tile.y_idx.saturating_mul(64); - let width = surface_width.saturating_sub(left).min(64); - let height = surface_height.saturating_sub(top).min(64); - if width == 0 || height == 0 { + let tile_left = tile.x_idx.saturating_mul(64); + let tile_top = tile.y_idx.saturating_mul(64); + let tile_width = surface_width.saturating_sub(tile_left).min(64); + let tile_height = surface_height.saturating_sub(tile_top).min(64); + if tile_width == 0 || tile_height == 0 { continue; } - let data = if width == 64 && height == 64 { - tile.pixels - } else { - crop_decoded_frame(&tile.pixels, 64, 64, width, height) + + let tile_rectangle = ExclusiveRectangle { + left: tile_left, + top: tile_top, + right: tile_left + tile_width, + bottom: tile_top + tile_height, }; - let update = BitmapUpdate { - surface_id: pdu.surface_id, - destination_rectangle: ExclusiveRectangle { - left, - top, - right: left + width, - bottom: top + height, - }, - codec_id: Codec1Type::Uncompressed, - data, - width, - height, + let mut emit_update = |destination_rectangle: ExclusiveRectangle, data: Vec| { + let width = destination_rectangle.right - destination_rectangle.left; + let height = destination_rectangle.bottom - destination_rectangle.top; + let update = BitmapUpdate { + surface_id: pdu.surface_id, + destination_rectangle, + codec_id: Codec1Type::Uncompressed, + data, + width, + height, + }; + self.compositor + .apply_bitmap(update.surface_id, &update.destination_rectangle, &update.data); + self.handler.on_bitmap_updated(&update); }; - self.handler.on_bitmap_updated(&update); + + if tile.update_rectangles.len() == 1 && tile.update_rectangles[0] == tile_rectangle { + let data = if tile_width == 64 && tile_height == 64 { + tile.pixels + } else { + crop_decoded_frame(&tile.pixels, 64, 64, tile_width, tile_height) + }; + emit_update(tile_rectangle, data); + continue; + } + + for destination_rectangle in tile.update_rectangles { + let width = destination_rectangle.right - destination_rectangle.left; + let height = destination_rectangle.bottom - destination_rectangle.top; + let source_x = usize::from(destination_rectangle.left - tile_left); + let source_y = usize::from(destination_rectangle.top - tile_top); + let source_stride = 64 * 4; + let row_bytes = usize::from(width) * 4; + let mut data = Vec::with_capacity(row_bytes * usize::from(height)); + for row in 0..usize::from(height) { + let source_start = (source_y + row) * source_stride + source_x * 4; + data.extend_from_slice(&tile.pixels[source_start..source_start + row_bytes]); + } + emit_update(destination_rectangle, data); + } } Ok(()) } @@ -1005,6 +1036,7 @@ impl GraphicsPipelineClient { self.total_frames_decoded = self.total_frames_decoded.wrapping_add(1); self.current_frame_id = None; self.frames_queued = self.frames_queued.saturating_sub(1); + self.progressive_decoder.end_frame(); // Commit the frame's compositor deltas so `drain_output` can surface them. self.compositor.end_frame(); @@ -1512,6 +1544,238 @@ mod tests { encode_progressive_stream(&blocks).unwrap() } + #[test] + fn wire_to_surface2_clips_compositor_output_to_region() { + use crate::pdu::Codec2Type; + + use ironrdp_graphics::progressive::{COEFFICIENTS_PER_COMPONENT, encode_first_pass}; + use ironrdp_pdu::codecs::rfx::RfxRectangle; + use ironrdp_pdu::codecs::rfx::progressive::{ + ComponentCodecQuant, ProgressiveBlock, ProgressiveContextPdu, ProgressiveFrameBeginPdu, + ProgressiveFrameEndPdu, ProgressiveRegion, ProgressiveSyncPdu, ProgressiveTile, TileSimple, + encode_progressive_stream, + }; + + // Send one real, neutral-gray TILE_SIMPLE, then reference it from a + // second Progressive payload in the same RDPGFX frame. + let (first_bitmap_data, shared_bitmap_data) = { + let base_quant = ComponentCodecQuant { + ll3: 6, + hl3: 6, + lh3: 6, + hh3: 6, + hl2: 6, + lh2: 6, + hh2: 6, + hl1: 6, + lh1: 6, + hh1: 6, + }; + let mut component = [0i16; COEFFICIENTS_PER_COMPONENT]; + let mut component_data = [0u8; 8192]; + let component_len = encode_first_pass( + &mut component, + &mut component_data, + &base_quant, + &ComponentCodecQuant::LOSSLESS, + false, + ) + .unwrap(); + let component_data = &component_data[..component_len]; + + let region = ProgressiveRegion { + tile_size: 0x40, + rects: vec![RfxRectangle { + x: 8, + y: 12, + width: 16, + height: 20, + }], + quant_vals: vec![base_quant], + quant_prog_vals: vec![], + flags: 0, + tiles: vec![ProgressiveTile::Simple(TileSimple { + quant_idx_y: 0, + quant_idx_cb: 0, + quant_idx_cr: 0, + x_idx: 0, + y_idx: 0, + flags: 0, + y_data: component_data, + cb_data: component_data, + cr_data: component_data, + tail_data: &[], + })], + }; + let shared_tile_region = ProgressiveRegion { + tile_size: 0x40, + rects: vec![RfxRectangle { + x: 32, + y: 40, + width: 8, + height: 10, + }], + quant_vals: vec![], + quant_prog_vals: vec![], + flags: 0, + tiles: vec![], + }; + + let first_bitmap_data = encode_progressive_stream(&[ + ProgressiveBlock::Sync(ProgressiveSyncPdu), + ProgressiveBlock::Context(ProgressiveContextPdu { + context_id: 0, + tile_size: 0x0040, + flags: 0, + }), + ProgressiveBlock::FrameBegin(ProgressiveFrameBeginPdu { + frame_index: 0, + region_count: 1, + }), + ProgressiveBlock::Region(region), + ProgressiveBlock::FrameEnd(ProgressiveFrameEndPdu), + ]) + .unwrap(); + let shared_bitmap_data = encode_progressive_stream(&[ + ProgressiveBlock::FrameBegin(ProgressiveFrameBeginPdu { + frame_index: 1, + region_count: 1, + }), + ProgressiveBlock::Region(shared_tile_region), + ProgressiveBlock::FrameEnd(ProgressiveFrameEndPdu), + ]) + .unwrap(); + + (first_bitmap_data, shared_bitmap_data) + }; + + let mut client = GraphicsPipelineClient::new(Box::new(TestHandler), None); + client + .handle_pdu(GfxPdu::ResetGraphics(crate::pdu::ResetGraphicsPdu { + width: 64, + height: 64, + monitors: vec![], + })) + .unwrap(); + client + .handle_pdu(GfxPdu::CreateSurface(crate::pdu::CreateSurfacePdu { + surface_id: 1, + width: 64, + height: 64, + pixel_format: PixelFormat::XRgb, + })) + .unwrap(); + + // Commit and drain the initial map separately so the next output can + // only have been produced by WireToSurface2. + client + .handle_pdu(GfxPdu::StartFrame(crate::pdu::StartFramePdu { + timestamp: crate::pdu::Timestamp { + milliseconds: 0, + seconds: 0, + minutes: 0, + hours: 0, + }, + frame_id: 1, + })) + .unwrap(); + client + .handle_pdu(GfxPdu::MapSurfaceToOutput(crate::pdu::MapSurfaceToOutputPdu { + surface_id: 1, + output_origin_x: 0, + output_origin_y: 0, + })) + .unwrap(); + client + .handle_pdu(GfxPdu::EndFrame(crate::pdu::EndFramePdu { frame_id: 1 })) + .unwrap(); + let _ = client.drain_output(); + + client + .handle_pdu(GfxPdu::StartFrame(crate::pdu::StartFramePdu { + timestamp: crate::pdu::Timestamp { + milliseconds: 0, + seconds: 0, + minutes: 0, + hours: 0, + }, + frame_id: 2, + })) + .unwrap(); + client + .handle_pdu(GfxPdu::WireToSurface2(WireToSurface2Pdu { + surface_id: 1, + codec_id: Codec2Type::RemoteFxProgressive, + codec_context_id: 7, + pixel_format: PixelFormat::XRgb, + bitmap_data: first_bitmap_data, + })) + .unwrap(); + client + .handle_pdu(GfxPdu::WireToSurface2(WireToSurface2Pdu { + surface_id: 1, + codec_id: Codec2Type::RemoteFxProgressive, + codec_context_id: 7, + pixel_format: PixelFormat::XRgb, + bitmap_data: shared_bitmap_data.clone(), + })) + .unwrap(); + client + .handle_pdu(GfxPdu::EndFrame(crate::pdu::EndFramePdu { frame_id: 2 })) + .unwrap(); + + let output = client.drain_output(); + assert_eq!(output.len(), 2); + assert_eq!( + output[0].region, + ExclusiveRectangle { + left: 8, + top: 12, + right: 24, + bottom: 32, + } + ); + assert_eq!(output[0].data.len(), 16 * 20 * 4); + assert!(output[0].data.iter().any(|&value| value != 0)); + assert_eq!( + output[1].region, + ExclusiveRectangle { + left: 32, + top: 40, + right: 40, + bottom: 50, + } + ); + assert_eq!(output[1].data.len(), 8 * 10 * 4); + assert!(output[1].data.iter().any(|&value| value != 0)); + + // A later RDPGFX frame cannot reference tiles from this completed frame. + client + .handle_pdu(GfxPdu::StartFrame(crate::pdu::StartFramePdu { + timestamp: crate::pdu::Timestamp { + milliseconds: 0, + seconds: 0, + minutes: 0, + hours: 0, + }, + frame_id: 3, + })) + .unwrap(); + client + .handle_pdu(GfxPdu::WireToSurface2(WireToSurface2Pdu { + surface_id: 1, + codec_id: Codec2Type::RemoteFxProgressive, + codec_context_id: 7, + pixel_format: PixelFormat::XRgb, + bitmap_data: shared_bitmap_data, + })) + .unwrap(); + client + .handle_pdu(GfxPdu::EndFrame(crate::pdu::EndFramePdu { frame_id: 3 })) + .unwrap(); + assert!(client.drain_output().is_empty()); + } + #[test] fn wire_to_surface2_decode_failure_propagates_error() { use crate::pdu::Codec2Type; @@ -1630,4 +1894,47 @@ mod tests { "progressive decoder context must not survive DeleteEncodingContext" ); } + + #[test] + fn delete_surface_clears_progressive_decoder_context() { + use crate::pdu::Codec2Type; + + let mut client = GraphicsPipelineClient::new(Box::new(TestHandler), None); + let create_surface = || { + GfxPdu::CreateSurface(crate::pdu::CreateSurfacePdu { + surface_id: 1, + width: 640, + height: 480, + pixel_format: PixelFormat::XRgb, + }) + }; + client.handle_pdu(create_surface()).unwrap(); + + client + .handle_pdu(GfxPdu::WireToSurface2(WireToSurface2Pdu { + surface_id: 1, + codec_id: Codec2Type::RemoteFxProgressive, + codec_context_id: 7, + pixel_format: PixelFormat::XRgb, + bitmap_data: build_progressive_stream(true), + })) + .unwrap(); + + client + .handle_pdu(GfxPdu::DeleteSurface(crate::pdu::DeleteSurfacePdu { surface_id: 1 })) + .unwrap(); + client.handle_pdu(create_surface()).unwrap(); + + let result = client.handle_pdu(GfxPdu::WireToSurface2(WireToSurface2Pdu { + surface_id: 1, + codec_id: Codec2Type::RemoteFxProgressive, + codec_context_id: 7, + pixel_format: PixelFormat::XRgb, + bitmap_data: build_progressive_stream(false), + })); + assert!( + result.is_err(), + "progressive decoder context must not survive DeleteSurface" + ); + } } diff --git a/crates/ironrdp-graphics/src/progressive.rs b/crates/ironrdp-graphics/src/progressive.rs index c2330961c..e9c8c3c1f 100644 --- a/crates/ironrdp-graphics/src/progressive.rs +++ b/crates/ironrdp-graphics/src/progressive.rs @@ -35,12 +35,15 @@ extern crate alloc; use alloc::collections::BTreeMap; +use alloc::collections::BTreeSet; use alloc::collections::btree_map::Entry; use ironrdp_pdu::codecs::rfx::EntropyAlgorithm; use ironrdp_pdu::codecs::rfx::progressive::ComponentCodecQuant; +use ironrdp_pdu::geometry::{ExclusiveRectangle, InclusiveRectangle}; use crate::dwt_extrapolate::BandInfo; +use crate::rectangle_processing::Region; use crate::rlgr::RlgrError; use crate::srl; @@ -1038,6 +1041,9 @@ pub struct DecodedTile { pub y_idx: u16, /// RGBA pixel data (64x64 = 16384 bytes). pub pixels: Vec, + /// Surface-relative rectangles where this tile is visible, clipped to the + /// Progressive REGION and surface bounds. + pub update_rectangles: Vec, } /// Per-axis cap on surface dimensions, in pixels. @@ -1114,7 +1120,8 @@ struct ProgressiveContext { /// the surface that owns it (`RDPGFX_DELETE_ENCODING_CONTEXT_PDU` carries both), so two /// surfaces may reuse the same `codec_context_id` value independently. Feed it /// progressive bitmap data from `WireToSurface2Pdu.bitmap_data` and get back decoded -/// RGBA tiles for compositing. +/// RGBA tiles for compositing. Call [`Self::begin_frame`] and [`Self::end_frame`] +/// around an RDPGFX frame so REGION blocks in separate bitmap payloads can share tiles. /// /// # Usage /// @@ -1130,11 +1137,15 @@ struct ProgressiveContext { /// )?; /// /// for tile in &tiles { -/// blit_tile(surface, tile.x_idx, tile.y_idx, &tile.pixels); +/// for rectangle in &tile.update_rectangles { +/// blit_tile_region(surface, tile.x_idx, tile.y_idx, rectangle, &tile.pixels); +/// } /// } /// ``` pub struct ProgressiveDecoder { contexts: BTreeMap<(u16, u32), ProgressiveContext>, + frame_tiles: BTreeMap<(u16, u32), BTreeSet<(u16, u16)>>, + frame_active: bool, } impl ProgressiveDecoder { @@ -1142,13 +1153,29 @@ impl ProgressiveDecoder { pub fn new() -> Self { Self { contexts: BTreeMap::new(), + frame_tiles: BTreeMap::new(), + frame_active: false, } } + /// Start an RDPGFX frame, resetting the set of tiles available to REGION blocks. + pub fn begin_frame(&mut self) { + self.frame_tiles.clear(); + self.frame_active = true; + } + + /// Finish an RDPGFX frame and discard its transient tile references. + pub fn end_frame(&mut self) { + self.frame_tiles.clear(); + self.frame_active = false; + } + /// Decode a progressive bitmap stream from WireToSurface2Pdu. /// /// Parses the progressive block stream, updates per-tile state, and /// returns RGBA pixel data for each tile that was updated. + /// Without an active frame started by [`Self::begin_frame`], the payload is + /// treated as a self-contained frame. /// /// # Arguments /// - `surface_id`: surface ID from the WireToSurface2Pdu; `codec_context_id` is scoped @@ -1206,35 +1233,134 @@ impl ProgressiveDecoder { // If surface dimensions changed, reallocate let expected_wide = surface_width.div_ceil(64); let expected_high = surface_height.div_ceil(64); - if context.surface.tiles_wide != expected_wide || context.surface.tiles_high != expected_high { + let surface_resized = + context.surface.tiles_wide != expected_wide || context.surface.tiles_high != expected_high; + if surface_resized { context.surface = SurfaceTiles::new(surface_width, surface_height, use_reduce_extrapolate)?; } context.surface.use_reduce_extrapolate = use_reduce_extrapolate; + // Direct users of the decoder get one self-contained frame per call. + // The EGFX client brackets multiple payloads with begin_frame/end_frame. + if !self.frame_active { + self.frame_tiles.clear(); + } + let frame_key = (surface_id, codec_context_id); + let frame_tiles = self.frame_tiles.entry(frame_key).or_default(); + if surface_resized { + frame_tiles.clear(); + } + let mut decoded_tiles = Vec::new(); - // Process REGION blocks (the main content) + // Process REGION blocks only inside the first FRAME_BEGIN/FRAME_END + // pair in this bitmap stream. Codec state persists across RDPGFX + // frames, while frame_tiles persists only across payloads in one frame. + let mut in_frame = false; + let mut frame_ended = false; for block in &blocks { let region = match block { - ProgressiveBlock::Region(r) => r, + ProgressiveBlock::FrameBegin(_) if !frame_ended => { + in_frame = true; + continue; + } + ProgressiveBlock::FrameEnd(_) => { + in_frame = false; + frame_ended = true; + continue; + } + ProgressiveBlock::Region(r) if in_frame => r, _ => continue, }; - let quant_vals = ®ion.quant_vals; - let prog_quant_vals = ®ion.quant_prog_vals; - + let mut region_tiles = BTreeMap::new(); for tile_block in ®ion.tiles { let tiles = decode_tile_block( &mut context.surface, tile_block, - quant_vals, - prog_quant_vals, + ®ion.quant_vals, + ®ion.quant_prog_vals, use_reduce_extrapolate, )?; - decoded_tiles.extend(tiles); + for tile in tiles { + let key = (tile.x_idx, tile.y_idx); + frame_tiles.insert(key); + region_tiles.insert(key, tile); + } + } + + let mut clipping_region = Region::new(); + for rectangle in ®ion.rects { + let left = rectangle.x.min(surface_width); + let top = rectangle.y.min(surface_height); + let right = rectangle.x.saturating_add(rectangle.width).min(surface_width); + let bottom = rectangle.y.saturating_add(rectangle.height).min(surface_height); + if left < right && top < bottom { + clipping_region.union_rectangle(InclusiveRectangle { + left, + top, + right: right - 1, + bottom: bottom - 1, + }); + } + } + + // REGION rectangles may be covered by tiles sent in an earlier REGION + // within the same frame, so clip them against every tile currently + // available for this frame rather than only the newly decoded tiles. + for &(x_idx, y_idx) in frame_tiles.iter() { + let left = x_idx.saturating_mul(64); + let top = y_idx.saturating_mul(64); + let right = left.saturating_add(64).min(surface_width); + let bottom = top.saturating_add(64).min(surface_height); + if left >= right || top >= bottom { + continue; + } + + let update_rectangles = clipping_region + .intersect_rectangle(&InclusiveRectangle { + left, + top, + right: right - 1, + bottom: bottom - 1, + }) + .rectangles + .into_iter() + .map(|rectangle| ExclusiveRectangle { + left: rectangle.left, + top: rectangle.top, + right: rectangle.right + 1, + bottom: rectangle.bottom + 1, + }) + .collect::>(); + if update_rectangles.is_empty() { + continue; + } + + let mut tile = if let Some(tile) = region_tiles.remove(&(x_idx, y_idx)) { + tile + } else { + let Some(tile_state) = context.surface.get(x_idx, y_idx) else { + continue; + }; + let mut pixels = vec![0u8; 64 * 64 * 4]; + tile_state.reconstruct_to_rgba(&mut pixels); + DecodedTile { + x_idx, + y_idx, + pixels, + update_rectangles: Vec::new(), + } + }; + tile.update_rectangles = update_rectangles; + decoded_tiles.push(tile); } } + if !self.frame_active { + self.frame_tiles.clear(); + } + Ok(decoded_tiles) } @@ -1244,11 +1370,25 @@ impl ProgressiveDecoder { /// `surface_id` and `codec_context_id`. pub fn delete_context(&mut self, surface_id: u16, codec_context_id: u32) { self.contexts.remove(&(surface_id, codec_context_id)); + self.frame_tiles.remove(&(surface_id, codec_context_id)); + } + + /// Delete every codec context associated with a surface. + /// + /// Called when the server deletes a surface so a later surface reusing the + /// same ID cannot inherit stale progressive tile state. + pub fn delete_surface(&mut self, surface_id: u16) { + self.contexts + .retain(|(context_surface_id, _), _| *context_surface_id != surface_id); + self.frame_tiles + .retain(|(context_surface_id, _), _| *context_surface_id != surface_id); } /// Reset all contexts (e.g., on EGFX channel reset). pub fn reset(&mut self) { self.contexts.clear(); + self.frame_tiles.clear(); + self.frame_active = false; } } @@ -1300,7 +1440,12 @@ fn decode_tile_block( let mut pixels = vec![0u8; 64 * 64 * 4]; tile_state.reconstruct_to_rgba(&mut pixels); - Ok(vec![DecodedTile { x_idx, y_idx, pixels }]) + Ok(vec![DecodedTile { + x_idx, + y_idx, + pixels, + update_rectangles: Vec::new(), + }]) } ProgressiveTile::First(tile) => { @@ -1343,7 +1488,12 @@ fn decode_tile_block( let mut pixels = vec![0u8; 64 * 64 * 4]; tile_state.reconstruct_to_rgba(&mut pixels); - Ok(vec![DecodedTile { x_idx, y_idx, pixels }]) + Ok(vec![DecodedTile { + x_idx, + y_idx, + pixels, + update_rectangles: Vec::new(), + }]) } ProgressiveTile::Upgrade(tile) => { @@ -1378,7 +1528,12 @@ fn decode_tile_block( let mut pixels = vec![0u8; 64 * 64 * 4]; tile_state.reconstruct_to_rgba(&mut pixels); - Ok(vec![DecodedTile { x_idx, y_idx, pixels }]) + Ok(vec![DecodedTile { + x_idx, + y_idx, + pixels, + update_rectangles: Vec::new(), + }]) } } } @@ -1797,6 +1952,107 @@ mod tests { decoder.delete_context(1, 0); assert_eq!(decoder.contexts.len(), 1); assert!(decoder.contexts.contains_key(&(2, 0))); + + // Deleting a surface removes all of its contexts without disturbing + // contexts owned by another surface. + assert!(decoder.decode_bitmap(1, 0, 640, 480, &minimal_stream()).is_ok()); + assert!(decoder.decode_bitmap(1, 1, 640, 480, &minimal_stream()).is_ok()); + assert_eq!(decoder.contexts.len(), 3); + + decoder.delete_surface(1); + assert_eq!(decoder.contexts.len(), 1); + assert!(decoder.contexts.contains_key(&(2, 0))); + } + + #[test] + fn decoder_ignores_regions_outside_frame() { + use ironrdp_pdu::codecs::rfx::RfxRectangle; + use ironrdp_pdu::codecs::rfx::progressive::{ + ComponentCodecQuant, ProgressiveBlock, ProgressiveContextPdu, ProgressiveFrameBeginPdu, + ProgressiveFrameEndPdu, ProgressiveRegion, ProgressiveSyncPdu, ProgressiveTile, TileSimple, + encode_progressive_stream, + }; + + fn invalid_region() -> ProgressiveRegion<'static> { + let base_quant = ComponentCodecQuant { + ll3: 6, + hl3: 6, + lh3: 6, + hh3: 6, + hl2: 6, + lh2: 6, + hh2: 6, + hl1: 6, + lh1: 6, + hh1: 6, + }; + ProgressiveRegion { + tile_size: 0x40, + rects: vec![RfxRectangle { + x: 0, + y: 0, + width: 64, + height: 64, + }], + quant_vals: vec![base_quant], + quant_prog_vals: vec![], + flags: 0, + tiles: vec![ProgressiveTile::Simple(TileSimple { + quant_idx_y: 0, + quant_idx_cb: 0, + quant_idx_cr: 0, + x_idx: 1, + y_idx: 0, + flags: 0, + y_data: &[], + cb_data: &[], + cr_data: &[], + tail_data: &[], + })], + } + } + + let context = ProgressiveBlock::Context(ProgressiveContextPdu { + context_id: 0, + tile_size: 0x0040, + flags: 0, + }); + let empty_frame_begin = ProgressiveBlock::FrameBegin(ProgressiveFrameBeginPdu { + frame_index: 0, + region_count: 0, + }); + + let outside = encode_progressive_stream(&[ + ProgressiveBlock::Sync(ProgressiveSyncPdu), + context.clone(), + ProgressiveBlock::Region(invalid_region()), + empty_frame_begin, + ProgressiveBlock::FrameEnd(ProgressiveFrameEndPdu), + ProgressiveBlock::Region(invalid_region()), + ]) + .unwrap(); + + let mut decoder = ProgressiveDecoder::new(); + let tiles = decoder.decode_bitmap(1, 10, 64, 64, &outside).unwrap(); + assert!(tiles.is_empty(), "out-of-frame regions must not produce tiles"); + + // The same deliberately out-of-bounds REGION must still be decoded, + // and fail, when it appears inside the frame. + let inside = encode_progressive_stream(&[ + ProgressiveBlock::Sync(ProgressiveSyncPdu), + context, + ProgressiveBlock::FrameBegin(ProgressiveFrameBeginPdu { + frame_index: 0, + region_count: 1, + }), + ProgressiveBlock::Region(invalid_region()), + ProgressiveBlock::FrameEnd(ProgressiveFrameEndPdu), + ]) + .unwrap(); + assert!(matches!( + decoder.decode_bitmap(1, 11, 64, 64, &inside), + Err(ProgressiveDecodeError::TileOutOfBounds { .. }) + )); } #[test] From 6d89e8d7458073061364d9b69035261d4900bae8 Mon Sep 17 00:00:00 2001 From: truebest Date: Wed, 5 Aug 2026 21:48:17 -0700 Subject: [PATCH 5/5] test(egfx): reduce progressive fixture boilerplate --- crates/ironrdp-egfx/src/client.rs | 366 ++++++--------------- crates/ironrdp-graphics/src/progressive.rs | 150 +++------ 2 files changed, 158 insertions(+), 358 deletions(-) diff --git a/crates/ironrdp-egfx/src/client.rs b/crates/ironrdp-egfx/src/client.rs index 63652e1dc..52698dbed 100644 --- a/crates/ironrdp-egfx/src/client.rs +++ b/crates/ironrdp-egfx/src/client.rs @@ -1239,6 +1239,8 @@ mod tests { use std::sync::{Arc, Mutex}; use super::*; + use crate::pdu::Codec2Type; + use ironrdp_pdu::codecs::rfx::RfxRectangle; struct TestHandler; impl GraphicsPipelineHandler for TestHandler { @@ -1253,6 +1255,78 @@ mod tests { fn on_unhandled_pdu(&mut self, _pdu: &GfxPdu) {} } + fn reset_graphics(client: &mut GraphicsPipelineClient, width: u32, height: u32) { + client + .handle_pdu(GfxPdu::ResetGraphics(crate::pdu::ResetGraphicsPdu { + width, + height, + monitors: vec![], + })) + .unwrap(); + } + + fn create_surface(client: &mut GraphicsPipelineClient, width: u16, height: u16) { + client + .handle_pdu(GfxPdu::CreateSurface(crate::pdu::CreateSurfacePdu { + surface_id: 1, + width, + height, + pixel_format: PixelFormat::XRgb, + })) + .unwrap(); + } + + fn progressive_client(width: u16, height: u16) -> GraphicsPipelineClient { + let mut client = GraphicsPipelineClient::new(Box::new(TestHandler), None); + reset_graphics(&mut client, u32::from(width), u32::from(height)); + create_surface(&mut client, width, height); + client + } + + fn start_frame(client: &mut GraphicsPipelineClient, frame_id: u32) { + client + .handle_pdu(GfxPdu::StartFrame(crate::pdu::StartFramePdu { + timestamp: crate::pdu::Timestamp { + milliseconds: 0, + seconds: 0, + minutes: 0, + hours: 0, + }, + frame_id, + })) + .unwrap(); + } + + fn end_frame(client: &mut GraphicsPipelineClient, frame_id: u32) { + client + .handle_pdu(GfxPdu::EndFrame(crate::pdu::EndFramePdu { frame_id })) + .unwrap(); + } + + fn wire_progressive(client: &mut GraphicsPipelineClient, bitmap_data: Vec) -> PduResult> { + client.handle_pdu(GfxPdu::WireToSurface2(WireToSurface2Pdu { + surface_id: 1, + codec_id: Codec2Type::RemoteFxProgressive, + codec_context_id: 7, + pixel_format: PixelFormat::XRgb, + bitmap_data, + })) + } + + fn assert_progressive_context_cleared(operation: &str, clear: impl FnOnce(&mut GraphicsPipelineClient)) { + let mut client = progressive_client(640, 480); + wire_progressive(&mut client, build_progressive_stream(true)).unwrap(); + clear(&mut client); + assert!( + wire_progressive(&mut client, build_progressive_stream(false)).is_err(), + "progressive decoder context must not survive {operation}" + ); + } + + fn rect(x: u16, y: u16, width: u16, height: u16) -> RfxRectangle { + RfxRectangle { x, y, width, height } + } + /// Captures bitmap updates and unhandled PDUs so a test can tell decode from /// fallthrough. /// `(codec_id, width, height, rgba)` extracted from each update, since @@ -1505,7 +1579,6 @@ mod tests { /// Builds a minimal single-tile RFX Progressive stream, optionally opening a codec /// context (SYNC+CONTEXT) first. fn build_progressive_stream(with_context: bool) -> Vec { - use ironrdp_pdu::codecs::rfx::RfxRectangle; use ironrdp_pdu::codecs::rfx::progressive::{ ProgressiveBlock, ProgressiveContextPdu, ProgressiveFrameBeginPdu, ProgressiveFrameEndPdu, ProgressiveRegion, ProgressiveSyncPdu, encode_progressive_stream, @@ -1513,12 +1586,7 @@ mod tests { let region = ProgressiveRegion { tile_size: 0x40, - rects: vec![RfxRectangle { - x: 0, - y: 0, - width: 64, - height: 64, - }], + rects: vec![rect(0, 0, 64, 64)], quant_vals: vec![], quant_prog_vals: vec![], flags: 0, @@ -1546,10 +1614,7 @@ mod tests { #[test] fn wire_to_surface2_clips_compositor_output_to_region() { - use crate::pdu::Codec2Type; - use ironrdp_graphics::progressive::{COEFFICIENTS_PER_COMPONENT, encode_first_pass}; - use ironrdp_pdu::codecs::rfx::RfxRectangle; use ironrdp_pdu::codecs::rfx::progressive::{ ComponentCodecQuant, ProgressiveBlock, ProgressiveContextPdu, ProgressiveFrameBeginPdu, ProgressiveFrameEndPdu, ProgressiveRegion, ProgressiveSyncPdu, ProgressiveTile, TileSimple, @@ -1559,18 +1624,7 @@ mod tests { // Send one real, neutral-gray TILE_SIMPLE, then reference it from a // second Progressive payload in the same RDPGFX frame. let (first_bitmap_data, shared_bitmap_data) = { - let base_quant = ComponentCodecQuant { - ll3: 6, - hl3: 6, - lh3: 6, - hh3: 6, - hl2: 6, - lh2: 6, - hh2: 6, - hl1: 6, - lh1: 6, - hh1: 6, - }; + let base_quant = ComponentCodecQuant::LOSSLESS; let mut component = [0i16; COEFFICIENTS_PER_COMPONENT]; let mut component_data = [0u8; 8192]; let component_len = encode_first_pass( @@ -1585,12 +1639,7 @@ mod tests { let region = ProgressiveRegion { tile_size: 0x40, - rects: vec![RfxRectangle { - x: 8, - y: 12, - width: 16, - height: 20, - }], + rects: vec![rect(8, 12, 16, 20)], quant_vals: vec![base_quant], quant_prog_vals: vec![], flags: 0, @@ -1609,12 +1658,7 @@ mod tests { }; let shared_tile_region = ProgressiveRegion { tile_size: 0x40, - rects: vec![RfxRectangle { - x: 32, - y: 40, - width: 8, - height: 10, - }], + rects: vec![rect(32, 40, 8, 10)], quant_vals: vec![], quant_prog_vals: vec![], flags: 0, @@ -1649,36 +1693,11 @@ mod tests { (first_bitmap_data, shared_bitmap_data) }; - let mut client = GraphicsPipelineClient::new(Box::new(TestHandler), None); - client - .handle_pdu(GfxPdu::ResetGraphics(crate::pdu::ResetGraphicsPdu { - width: 64, - height: 64, - monitors: vec![], - })) - .unwrap(); - client - .handle_pdu(GfxPdu::CreateSurface(crate::pdu::CreateSurfacePdu { - surface_id: 1, - width: 64, - height: 64, - pixel_format: PixelFormat::XRgb, - })) - .unwrap(); + let mut client = progressive_client(64, 64); // Commit and drain the initial map separately so the next output can // only have been produced by WireToSurface2. - client - .handle_pdu(GfxPdu::StartFrame(crate::pdu::StartFramePdu { - timestamp: crate::pdu::Timestamp { - milliseconds: 0, - seconds: 0, - minutes: 0, - hours: 0, - }, - frame_id: 1, - })) - .unwrap(); + start_frame(&mut client, 1); client .handle_pdu(GfxPdu::MapSurfaceToOutput(crate::pdu::MapSurfaceToOutputPdu { surface_id: 1, @@ -1686,43 +1705,13 @@ mod tests { output_origin_y: 0, })) .unwrap(); - client - .handle_pdu(GfxPdu::EndFrame(crate::pdu::EndFramePdu { frame_id: 1 })) - .unwrap(); + end_frame(&mut client, 1); let _ = client.drain_output(); - client - .handle_pdu(GfxPdu::StartFrame(crate::pdu::StartFramePdu { - timestamp: crate::pdu::Timestamp { - milliseconds: 0, - seconds: 0, - minutes: 0, - hours: 0, - }, - frame_id: 2, - })) - .unwrap(); - client - .handle_pdu(GfxPdu::WireToSurface2(WireToSurface2Pdu { - surface_id: 1, - codec_id: Codec2Type::RemoteFxProgressive, - codec_context_id: 7, - pixel_format: PixelFormat::XRgb, - bitmap_data: first_bitmap_data, - })) - .unwrap(); - client - .handle_pdu(GfxPdu::WireToSurface2(WireToSurface2Pdu { - surface_id: 1, - codec_id: Codec2Type::RemoteFxProgressive, - codec_context_id: 7, - pixel_format: PixelFormat::XRgb, - bitmap_data: shared_bitmap_data.clone(), - })) - .unwrap(); - client - .handle_pdu(GfxPdu::EndFrame(crate::pdu::EndFramePdu { frame_id: 2 })) - .unwrap(); + start_frame(&mut client, 2); + wire_progressive(&mut client, first_bitmap_data).unwrap(); + wire_progressive(&mut client, shared_bitmap_data.clone()).unwrap(); + end_frame(&mut client, 2); let output = client.drain_output(); assert_eq!(output.len(), 2); @@ -1750,51 +1739,16 @@ mod tests { assert!(output[1].data.iter().any(|&value| value != 0)); // A later RDPGFX frame cannot reference tiles from this completed frame. - client - .handle_pdu(GfxPdu::StartFrame(crate::pdu::StartFramePdu { - timestamp: crate::pdu::Timestamp { - milliseconds: 0, - seconds: 0, - minutes: 0, - hours: 0, - }, - frame_id: 3, - })) - .unwrap(); - client - .handle_pdu(GfxPdu::WireToSurface2(WireToSurface2Pdu { - surface_id: 1, - codec_id: Codec2Type::RemoteFxProgressive, - codec_context_id: 7, - pixel_format: PixelFormat::XRgb, - bitmap_data: shared_bitmap_data, - })) - .unwrap(); - client - .handle_pdu(GfxPdu::EndFrame(crate::pdu::EndFramePdu { frame_id: 3 })) - .unwrap(); + start_frame(&mut client, 3); + wire_progressive(&mut client, shared_bitmap_data).unwrap(); + end_frame(&mut client, 3); assert!(client.drain_output().is_empty()); } #[test] fn wire_to_surface2_decode_failure_propagates_error() { - use crate::pdu::Codec2Type; - - let mut client = GraphicsPipelineClient::new(Box::new(TestHandler), None); - let _ = client.handle_pdu(GfxPdu::CreateSurface(crate::pdu::CreateSurfacePdu { - surface_id: 1, - width: 640, - height: 480, - pixel_format: PixelFormat::XRgb, - })); - - let result = client.handle_pdu(GfxPdu::WireToSurface2(WireToSurface2Pdu { - surface_id: 1, - codec_id: Codec2Type::RemoteFxProgressive, - codec_context_id: 7, - pixel_format: PixelFormat::XRgb, - bitmap_data: build_progressive_stream(false), - })); + let mut client = progressive_client(640, 480); + let result = wire_progressive(&mut client, build_progressive_stream(false)); assert!( result.is_err(), @@ -1804,137 +1758,29 @@ mod tests { #[test] fn reset_graphics_clears_progressive_decoder_context() { - use crate::pdu::Codec2Type; - - let mut client = GraphicsPipelineClient::new(Box::new(TestHandler), None); - let _ = client.handle_pdu(GfxPdu::CreateSurface(crate::pdu::CreateSurfacePdu { - surface_id: 1, - width: 640, - height: 480, - pixel_format: PixelFormat::XRgb, - })); - - let result = client.handle_pdu(GfxPdu::WireToSurface2(WireToSurface2Pdu { - surface_id: 1, - codec_id: Codec2Type::RemoteFxProgressive, - codec_context_id: 7, - pixel_format: PixelFormat::XRgb, - bitmap_data: build_progressive_stream(true), - })); - assert!( - result.is_ok(), - "establishing the context should succeed: {:?}", - result.as_ref().err() - ); - - let _ = client.handle_pdu(GfxPdu::ResetGraphics(crate::pdu::ResetGraphicsPdu { - width: 1920, - height: 1080, - monitors: vec![], - })); - let _ = client.handle_pdu(GfxPdu::CreateSurface(crate::pdu::CreateSurfacePdu { - surface_id: 1, - width: 640, - height: 480, - pixel_format: PixelFormat::XRgb, - })); - - let result = client.handle_pdu(GfxPdu::WireToSurface2(WireToSurface2Pdu { - surface_id: 1, - codec_id: Codec2Type::RemoteFxProgressive, - codec_context_id: 7, - pixel_format: PixelFormat::XRgb, - bitmap_data: build_progressive_stream(false), - })); - assert!( - result.is_err(), - "progressive decoder context must not survive ResetGraphics" - ); + assert_progressive_context_cleared("ResetGraphics", |client| { + reset_graphics(client, 1920, 1080); + create_surface(client, 640, 480); + }); } #[test] fn delete_encoding_context_clears_progressive_decoder_context() { - use crate::pdu::Codec2Type; - - let mut client = GraphicsPipelineClient::new(Box::new(TestHandler), None); - let _ = client.handle_pdu(GfxPdu::CreateSurface(crate::pdu::CreateSurfacePdu { - surface_id: 1, - width: 640, - height: 480, - pixel_format: PixelFormat::XRgb, - })); - - let result = client.handle_pdu(GfxPdu::WireToSurface2(WireToSurface2Pdu { - surface_id: 1, - codec_id: Codec2Type::RemoteFxProgressive, - codec_context_id: 7, - pixel_format: PixelFormat::XRgb, - bitmap_data: build_progressive_stream(true), - })); - assert!( - result.is_ok(), - "establishing the context should succeed: {:?}", - result.as_ref().err() - ); - - let _ = client.handle_pdu(GfxPdu::DeleteEncodingContext(DeleteEncodingContextPdu { - surface_id: 1, - codec_context_id: 7, - })); - - let result = client.handle_pdu(GfxPdu::WireToSurface2(WireToSurface2Pdu { - surface_id: 1, - codec_id: Codec2Type::RemoteFxProgressive, - codec_context_id: 7, - pixel_format: PixelFormat::XRgb, - bitmap_data: build_progressive_stream(false), - })); - assert!( - result.is_err(), - "progressive decoder context must not survive DeleteEncodingContext" - ); + assert_progressive_context_cleared("DeleteEncodingContext", |client| { + let _ = client.handle_pdu(GfxPdu::DeleteEncodingContext(DeleteEncodingContextPdu { + surface_id: 1, + codec_context_id: 7, + })); + }); } #[test] fn delete_surface_clears_progressive_decoder_context() { - use crate::pdu::Codec2Type; - - let mut client = GraphicsPipelineClient::new(Box::new(TestHandler), None); - let create_surface = || { - GfxPdu::CreateSurface(crate::pdu::CreateSurfacePdu { - surface_id: 1, - width: 640, - height: 480, - pixel_format: PixelFormat::XRgb, - }) - }; - client.handle_pdu(create_surface()).unwrap(); - - client - .handle_pdu(GfxPdu::WireToSurface2(WireToSurface2Pdu { - surface_id: 1, - codec_id: Codec2Type::RemoteFxProgressive, - codec_context_id: 7, - pixel_format: PixelFormat::XRgb, - bitmap_data: build_progressive_stream(true), - })) - .unwrap(); - - client - .handle_pdu(GfxPdu::DeleteSurface(crate::pdu::DeleteSurfacePdu { surface_id: 1 })) - .unwrap(); - client.handle_pdu(create_surface()).unwrap(); - - let result = client.handle_pdu(GfxPdu::WireToSurface2(WireToSurface2Pdu { - surface_id: 1, - codec_id: Codec2Type::RemoteFxProgressive, - codec_context_id: 7, - pixel_format: PixelFormat::XRgb, - bitmap_data: build_progressive_stream(false), - })); - assert!( - result.is_err(), - "progressive decoder context must not survive DeleteSurface" - ); + assert_progressive_context_cleared("DeleteSurface", |client| { + client + .handle_pdu(GfxPdu::DeleteSurface(crate::pdu::DeleteSurfacePdu { surface_id: 1 })) + .unwrap(); + create_surface(client, 640, 480); + }); } } diff --git a/crates/ironrdp-graphics/src/progressive.rs b/crates/ironrdp-graphics/src/progressive.rs index e9c8c3c1f..8d53f3e2e 100644 --- a/crates/ironrdp-graphics/src/progressive.rs +++ b/crates/ironrdp-graphics/src/progressive.rs @@ -1552,6 +1552,43 @@ impl Default for ProgressiveDecoder { #[expect(clippy::as_conversions, clippy::cast_possible_truncation, clippy::cast_possible_wrap)] mod tests { use super::*; + use ironrdp_pdu::codecs::rfx::RfxRectangle; + + fn rect(x: u16, y: u16, width: u16, height: u16) -> RfxRectangle { + RfxRectangle { x, y, width, height } + } + + fn minimal_progressive_stream() -> Vec { + use ironrdp_pdu::codecs::rfx::progressive::{ + ProgressiveBlock, ProgressiveContextPdu, ProgressiveFrameBeginPdu, ProgressiveFrameEndPdu, + ProgressiveRegion, ProgressiveSyncPdu, encode_progressive_stream, + }; + + let region = ProgressiveRegion { + tile_size: 0x40, + rects: vec![rect(0, 0, 64, 64)], + quant_vals: vec![], + quant_prog_vals: vec![], + flags: 0, + tiles: vec![], + }; + let blocks = [ + ProgressiveBlock::Sync(ProgressiveSyncPdu), + ProgressiveBlock::Context(ProgressiveContextPdu { + context_id: 0, + tile_size: 0x0040, + flags: 0, + }), + ProgressiveBlock::FrameBegin(ProgressiveFrameBeginPdu { + frame_index: 0, + region_count: 1, + }), + ProgressiveBlock::Region(region), + ProgressiveBlock::FrameEnd(ProgressiveFrameEndPdu), + ]; + + encode_progressive_stream(&blocks).unwrap() + } #[test] fn surface_tiles_rejects_over_cap_dimensions() { @@ -1856,43 +1893,7 @@ mod tests { let mut decoder = ProgressiveDecoder::new(); // Decode a minimal valid stream to create a context - use ironrdp_pdu::codecs::rfx::RfxRectangle; - use ironrdp_pdu::codecs::rfx::progressive::{ - ProgressiveBlock, ProgressiveContextPdu, ProgressiveFrameBeginPdu, ProgressiveFrameEndPdu, - ProgressiveRegion, ProgressiveSyncPdu, encode_progressive_stream, - }; - - let 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![], - }; - - let blocks = vec![ - ProgressiveBlock::Sync(ProgressiveSyncPdu), - ProgressiveBlock::Context(ProgressiveContextPdu { - context_id: 0, - tile_size: 0x0040, - flags: 0, - }), - ProgressiveBlock::FrameBegin(ProgressiveFrameBeginPdu { - frame_index: 0, - region_count: 1, - }), - ProgressiveBlock::Region(region), - ProgressiveBlock::FrameEnd(ProgressiveFrameEndPdu), - ]; - - let encoded = encode_progressive_stream(&blocks).unwrap(); - let result = decoder.decode_bitmap(1, 1, 640, 480, &encoded); + let result = decoder.decode_bitmap(1, 1, 640, 480, &minimal_progressive_stream()); assert!(result.is_ok()); assert_eq!(decoder.contexts.len(), 1); @@ -1902,48 +1903,11 @@ mod tests { #[test] fn decoder_contexts_scoped_by_surface() { - use ironrdp_pdu::codecs::rfx::RfxRectangle; - use ironrdp_pdu::codecs::rfx::progressive::{ - ProgressiveBlock, ProgressiveContextPdu, ProgressiveFrameBeginPdu, ProgressiveFrameEndPdu, - ProgressiveRegion, ProgressiveSyncPdu, encode_progressive_stream, - }; - - fn minimal_stream() -> Vec { - let 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![], - }; - let blocks = vec![ - ProgressiveBlock::Sync(ProgressiveSyncPdu), - ProgressiveBlock::Context(ProgressiveContextPdu { - context_id: 0, - tile_size: 0x0040, - flags: 0, - }), - ProgressiveBlock::FrameBegin(ProgressiveFrameBeginPdu { - frame_index: 0, - region_count: 1, - }), - ProgressiveBlock::Region(region), - ProgressiveBlock::FrameEnd(ProgressiveFrameEndPdu), - ]; - encode_progressive_stream(&blocks).unwrap() - } - let mut decoder = ProgressiveDecoder::new(); // Two different surfaces reusing the same codec_context_id must not collide. - let result_a = decoder.decode_bitmap(1, 0, 640, 480, &minimal_stream()); - let result_b = decoder.decode_bitmap(2, 0, 800, 600, &minimal_stream()); + let result_a = decoder.decode_bitmap(1, 0, 640, 480, &minimal_progressive_stream()); + let result_b = decoder.decode_bitmap(2, 0, 800, 600, &minimal_progressive_stream()); assert!(result_a.is_ok()); assert!(result_b.is_ok()); assert_eq!(decoder.contexts.len(), 2); @@ -1955,8 +1919,16 @@ mod tests { // Deleting a surface removes all of its contexts without disturbing // contexts owned by another surface. - assert!(decoder.decode_bitmap(1, 0, 640, 480, &minimal_stream()).is_ok()); - assert!(decoder.decode_bitmap(1, 1, 640, 480, &minimal_stream()).is_ok()); + assert!( + decoder + .decode_bitmap(1, 0, 640, 480, &minimal_progressive_stream()) + .is_ok() + ); + assert!( + decoder + .decode_bitmap(1, 1, 640, 480, &minimal_progressive_stream()) + .is_ok() + ); assert_eq!(decoder.contexts.len(), 3); decoder.delete_surface(1); @@ -1966,7 +1938,6 @@ mod tests { #[test] fn decoder_ignores_regions_outside_frame() { - use ironrdp_pdu::codecs::rfx::RfxRectangle; use ironrdp_pdu::codecs::rfx::progressive::{ ComponentCodecQuant, ProgressiveBlock, ProgressiveContextPdu, ProgressiveFrameBeginPdu, ProgressiveFrameEndPdu, ProgressiveRegion, ProgressiveSyncPdu, ProgressiveTile, TileSimple, @@ -1974,27 +1945,10 @@ mod tests { }; fn invalid_region() -> ProgressiveRegion<'static> { - let base_quant = ComponentCodecQuant { - ll3: 6, - hl3: 6, - lh3: 6, - hh3: 6, - hl2: 6, - lh2: 6, - hh2: 6, - hl1: 6, - lh1: 6, - hh1: 6, - }; ProgressiveRegion { tile_size: 0x40, - rects: vec![RfxRectangle { - x: 0, - y: 0, - width: 64, - height: 64, - }], - quant_vals: vec![base_quant], + rects: vec![rect(0, 0, 64, 64)], + quant_vals: vec![ComponentCodecQuant::LOSSLESS], quant_prog_vals: vec![], flags: 0, tiles: vec![ProgressiveTile::Simple(TileSimple {