diff --git a/crates/ironrdp-egfx/src/client.rs b/crates/ironrdp-egfx/src/client.rs index 519a9730d..1df62bc2e 100644 --- a/crates/ironrdp-egfx/src/client.rs +++ b/crates/ironrdp-egfx/src/client.rs @@ -58,7 +58,7 @@ use std::collections::BTreeMap; use ironrdp_core::{Decode as _, ReadCursor, impl_as_any}; use ironrdp_dvc::{DvcClientProcessor, DvcMessage, DvcProcessor}; use ironrdp_graphics::zgfx; -use ironrdp_pdu::geometry::{InclusiveRectangle, Rectangle as _}; +use ironrdp_pdu::geometry::{ExclusiveRectangle, Rectangle as _}; use ironrdp_pdu::{PduResult, decode_cursor, decode_err, pdu_other_err}; use tracing::{debug, trace, warn}; @@ -186,8 +186,8 @@ impl CodecCapabilities { pub struct BitmapUpdate { /// Surface this update applies to pub surface_id: u16, - /// Destination rectangle within the surface - pub destination_rectangle: InclusiveRectangle, + /// Destination rectangle within the surface (exclusive `right`/`bottom`) + pub destination_rectangle: ExclusiveRectangle, /// Codec that produced this update pub codec_id: Codec1Type, /// RGBA pixel data (4 bytes per pixel), row-major @@ -674,8 +674,10 @@ impl GraphicsPipelineClient { return Err(pdu_other_err!("invalid destination rectangle ordering")); } - // Validate destination rectangle against surface bounds - if rect.right >= surface.width || rect.bottom >= surface.height { + // Validate destination rectangle against surface bounds. The rectangle + // uses exclusive `right`/`bottom`, so a full-surface update has + // `right == surface.width` and `bottom == surface.height`, which is valid. + if rect.right > surface.width || rect.bottom > surface.height { warn!( surface_id = pdu.surface_id, rect_right = rect.right, @@ -706,7 +708,7 @@ impl GraphicsPipelineClient { Ok(()) } - fn decode_avc420(&mut self, surface_id: u16, dest_rect: &InclusiveRectangle, bitmap_data: &[u8]) -> PduResult<()> { + 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))?; diff --git a/crates/ironrdp-egfx/src/pdu/cmd.rs b/crates/ironrdp-egfx/src/pdu/cmd.rs index 950946f9a..9549ae344 100644 --- a/crates/ironrdp-egfx/src/pdu/cmd.rs +++ b/crates/ironrdp-egfx/src/pdu/cmd.rs @@ -7,7 +7,7 @@ use ironrdp_core::{ }; use ironrdp_dvc::DvcEncode; use ironrdp_pdu::gcc::Monitor; -use ironrdp_pdu::geometry::{ExclusiveRectangle, InclusiveRectangle}; +use ironrdp_pdu::geometry::ExclusiveRectangle; use ironrdp_pdu::{DecodeError, cast_length, ensure_size, read_padding, write_padding}; use tracing::warn; @@ -306,13 +306,17 @@ impl<'de> Decode<'de> for GfxPdu { /// 2.2.2.1 RDPGFX_WIRE_TO_SURFACE_PDU_1 /// +/// `destination_rectangle` is an [`ExclusiveRectangle`] because MS-RDPEGFX +/// 2.2.1.4.1 RDPGFX_RECT16 specifies `right` and `bottom` as exclusive +/// (one-past-end), matching FreeRDP and the Windows reference clients. +/// /// [2.2.2.1]: #[derive(Clone, PartialEq, Eq)] pub struct WireToSurface1Pdu { pub surface_id: u16, pub codec_id: Codec1Type, pub pixel_format: PixelFormat, - pub destination_rectangle: InclusiveRectangle, + pub destination_rectangle: ExclusiveRectangle, pub bitmap_data: Vec, } @@ -331,7 +335,7 @@ impl fmt::Debug for WireToSurface1Pdu { impl WireToSurface1Pdu { const NAME: &'static str = "WireToSurface1Pdu"; - const FIXED_PART_SIZE: usize = 2 /* SurfaceId */ + 2 /* CodecId */ + 1 /* PixelFormat */ + InclusiveRectangle::FIXED_PART_SIZE /* Dest */ + 4 /* BitmapDataLen */; + const FIXED_PART_SIZE: usize = 2 /* SurfaceId */ + 2 /* CodecId */ + 1 /* PixelFormat */ + ExclusiveRectangle::ENCODED_SIZE /* Dest */ + 4 /* BitmapDataLen */; } impl Encode for WireToSurface1Pdu { @@ -363,7 +367,7 @@ impl<'a> Decode<'a> for WireToSurface1Pdu { let surface_id = src.read_u16(); let codec_id = Codec1Type::try_from(src.read_u16())?; let pixel_format = PixelFormat::try_from(src.read_u8())?; - let destination_rectangle = InclusiveRectangle::decode(src)?; + let destination_rectangle = ExclusiveRectangle::decode(src)?; let bitmap_data_length = cast_length!("BitmapDataLen", src.read_u32())?; ensure_size!(in: src, size: bitmap_data_length); diff --git a/crates/ironrdp-egfx/src/server.rs b/crates/ironrdp-egfx/src/server.rs index 380e61dd5..916da9b14 100644 --- a/crates/ironrdp-egfx/src/server.rs +++ b/crates/ironrdp-egfx/src/server.rs @@ -62,7 +62,7 @@ use ironrdp_core::{Encode, EncodeResult, WriteCursor, decode, impl_as_any}; use ironrdp_dvc::{DvcEncode, DvcMessage, DvcProcessor, DvcServerProcessor}; use ironrdp_graphics::zgfx::{CompressionMode, Compressor, compress_and_wrap_egfx, wrap_uncompressed}; use ironrdp_pdu::gcc::Monitor; -use ironrdp_pdu::geometry::InclusiveRectangle; +use ironrdp_pdu::geometry::ExclusiveRectangle; use ironrdp_pdu::{PduResult, decode_err}; use tracing::{debug, trace, warn}; @@ -1197,8 +1197,12 @@ impl GraphicsPipelineServer { } } - /// Compute bounding rectangle from regions - fn compute_dest_rect(regions: &[Avc420Region], default_width: u16, default_height: u16) -> InclusiveRectangle { + /// Compute bounding rectangle from regions. + /// + /// Avc420Region uses inclusive bounds; the wire format (RDPGFX_RECT16) is + /// exclusive, so the returned ExclusiveRectangle adds 1 to the max right + /// and bottom of the inclusive bounding box. + fn compute_dest_rect(regions: &[Avc420Region], default_width: u16, default_height: u16) -> ExclusiveRectangle { if let Some(first) = regions.first() { let mut left = first.left; let mut top = first.top; @@ -1212,18 +1216,18 @@ impl GraphicsPipelineServer { bottom = bottom.max(r.bottom); } - InclusiveRectangle { + ExclusiveRectangle { left, top, - right, - bottom, + right: right.saturating_add(1), + bottom: bottom.saturating_add(1), } } else { - InclusiveRectangle { + ExclusiveRectangle { left: 0, top: 0, - right: default_width.saturating_sub(1), - bottom: default_height.saturating_sub(1), + right: default_width, + bottom: default_height, } } } @@ -1385,11 +1389,11 @@ impl GraphicsPipelineServer { let timestamp = Self::make_timestamp(timestamp_ms); let frame_id = self.frames.begin_frame(timestamp); - let dest_rect = InclusiveRectangle { + let dest_rect = ExclusiveRectangle { left: 0, top: 0, - right: dest_width.saturating_sub(1), - bottom: dest_height.saturating_sub(1), + right: dest_width, + bottom: dest_height, }; self.output_queue diff --git a/crates/ironrdp-testsuite-core/tests/egfx/client.rs b/crates/ironrdp-testsuite-core/tests/egfx/client.rs index 946d428c3..5a87a536f 100644 --- a/crates/ironrdp-testsuite-core/tests/egfx/client.rs +++ b/crates/ironrdp-testsuite-core/tests/egfx/client.rs @@ -7,7 +7,7 @@ use ironrdp_egfx::pdu::{ DeleteSurfacePdu, EndFramePdu, GfxPdu, PixelFormat, ResetGraphicsPdu, StartFramePdu, Timestamp, WireToSurface1Pdu, }; use ironrdp_graphics::zgfx::wrap_uncompressed; -use ironrdp_pdu::geometry::InclusiveRectangle; +use ironrdp_pdu::geometry::ExclusiveRectangle; // ============================================================================ // Test Handler @@ -213,11 +213,11 @@ fn client_handles_uncompressed_via_process() { surface_id: 1, codec_id: Codec1Type::Uncompressed, pixel_format: PixelFormat::XRgb, - destination_rectangle: InclusiveRectangle { + destination_rectangle: ExclusiveRectangle { left: 0, top: 0, - right: 3, - bottom: 3, + right: 4, + bottom: 4, }, bitmap_data: vec![0u8; 4 * 4 * 4], }); @@ -245,11 +245,11 @@ fn client_dispatches_avc420_via_process() { surface_id: 1, codec_id: Codec1Type::Avc420, pixel_format: PixelFormat::XRgb, - destination_rectangle: InclusiveRectangle { + destination_rectangle: ExclusiveRectangle { left: 0, top: 0, - right: 15, - bottom: 15, + right: 16, + bottom: 16, }, bitmap_data, }); @@ -276,11 +276,11 @@ fn client_skips_avc420_without_decoder() { surface_id: 1, codec_id: Codec1Type::Avc420, pixel_format: PixelFormat::XRgb, - destination_rectangle: InclusiveRectangle { + destination_rectangle: ExclusiveRectangle { left: 0, top: 0, - right: 15, - bottom: 15, + right: 16, + bottom: 16, }, bitmap_data, }); @@ -310,11 +310,11 @@ fn client_frame_ordering_via_process() { surface_id: 1, codec_id: Codec1Type::Uncompressed, pixel_format: PixelFormat::XRgb, - destination_rectangle: InclusiveRectangle { + destination_rectangle: ExclusiveRectangle { left: 0, top: 0, - right: 3, - bottom: 3, + right: 4, + bottom: 4, }, bitmap_data: vec![0u8; 4 * 4 * 4], }); @@ -403,11 +403,11 @@ fn client_rejects_wire_to_unknown_surface() { surface_id: 99, // does not exist codec_id: Codec1Type::Uncompressed, pixel_format: PixelFormat::XRgb, - destination_rectangle: InclusiveRectangle { + destination_rectangle: ExclusiveRectangle { left: 0, top: 0, - right: 3, - bottom: 3, + right: 4, + bottom: 4, }, bitmap_data: vec![0u8; 4 * 4 * 4], }); @@ -424,7 +424,7 @@ fn client_rejects_invalid_rectangle_ordering() { surface_id: 1, codec_id: Codec1Type::Uncompressed, pixel_format: PixelFormat::XRgb, - destination_rectangle: InclusiveRectangle { + destination_rectangle: ExclusiveRectangle { left: 50, top: 0, right: 10, @@ -447,13 +447,13 @@ fn client_tolerates_out_of_bounds_rectangle() { surface_id: 1, codec_id: Codec1Type::Uncompressed, pixel_format: PixelFormat::XRgb, - destination_rectangle: InclusiveRectangle { + destination_rectangle: ExclusiveRectangle { left: 0, top: 0, right: 200, // exceeds surface width of 100 bottom: 50, }, - bitmap_data: vec![0u8; 201 * 51 * 4], + bitmap_data: vec![0u8; 200 * 50 * 4], }); let result = client.process(0, &encode_for_process(&pdu)); assert!(