Skip to content

Commit 0282d18

Browse files
glambersonglamberson
authored
feat(egfx)!: switch WireToSurface1Pdu rectangles to exclusive bounds (#1238)
Updates EGFX WireToSurface1 destination rectangles to use exclusive right/bottom bounds so Rust geometry semantics match MS-RDPEGFX wire semantics.
1 parent 9cb5439 commit 0282d18

4 files changed

Lines changed: 51 additions & 41 deletions

File tree

‎crates/ironrdp-egfx/src/client.rs‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ use std::collections::BTreeMap;
5858
use ironrdp_core::{Decode as _, ReadCursor, impl_as_any};
5959
use ironrdp_dvc::{DvcClientProcessor, DvcMessage, DvcProcessor};
6060
use ironrdp_graphics::zgfx;
61-
use ironrdp_pdu::geometry::{InclusiveRectangle, Rectangle as _};
61+
use ironrdp_pdu::geometry::{ExclusiveRectangle, Rectangle as _};
6262
use ironrdp_pdu::{PduResult, decode_cursor, decode_err, pdu_other_err};
6363
use tracing::{debug, trace, warn};
6464

@@ -186,8 +186,8 @@ impl CodecCapabilities {
186186
pub struct BitmapUpdate {
187187
/// Surface this update applies to
188188
pub surface_id: u16,
189-
/// Destination rectangle within the surface
190-
pub destination_rectangle: InclusiveRectangle,
189+
/// Destination rectangle within the surface (exclusive `right`/`bottom`)
190+
pub destination_rectangle: ExclusiveRectangle,
191191
/// Codec that produced this update
192192
pub codec_id: Codec1Type,
193193
/// RGBA pixel data (4 bytes per pixel), row-major
@@ -674,8 +674,10 @@ impl GraphicsPipelineClient {
674674
return Err(pdu_other_err!("invalid destination rectangle ordering"));
675675
}
676676

677-
// Validate destination rectangle against surface bounds
678-
if rect.right >= surface.width || rect.bottom >= surface.height {
677+
// Validate destination rectangle against surface bounds. The rectangle
678+
// uses exclusive `right`/`bottom`, so a full-surface update has
679+
// `right == surface.width` and `bottom == surface.height`, which is valid.
680+
if rect.right > surface.width || rect.bottom > surface.height {
679681
warn!(
680682
surface_id = pdu.surface_id,
681683
rect_right = rect.right,
@@ -706,7 +708,7 @@ impl GraphicsPipelineClient {
706708
Ok(())
707709
}
708710

709-
fn decode_avc420(&mut self, surface_id: u16, dest_rect: &InclusiveRectangle, bitmap_data: &[u8]) -> PduResult<()> {
711+
fn decode_avc420(&mut self, surface_id: u16, dest_rect: &ExclusiveRectangle, bitmap_data: &[u8]) -> PduResult<()> {
710712
let mut cursor = ReadCursor::new(bitmap_data);
711713
let stream = Avc420BitmapStream::decode(&mut cursor).map_err(|e| decode_err!(e))?;
712714

‎crates/ironrdp-egfx/src/pdu/cmd.rs‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ use ironrdp_core::{
77
};
88
use ironrdp_dvc::DvcEncode;
99
use ironrdp_pdu::gcc::Monitor;
10-
use ironrdp_pdu::geometry::{ExclusiveRectangle, InclusiveRectangle};
10+
use ironrdp_pdu::geometry::ExclusiveRectangle;
1111
use ironrdp_pdu::{DecodeError, cast_length, ensure_size, read_padding, write_padding};
1212
use tracing::warn;
1313

@@ -306,13 +306,17 @@ impl<'de> Decode<'de> for GfxPdu {
306306

307307
/// 2.2.2.1 RDPGFX_WIRE_TO_SURFACE_PDU_1
308308
///
309+
/// `destination_rectangle` is an [`ExclusiveRectangle`] because MS-RDPEGFX
310+
/// 2.2.1.4.1 RDPGFX_RECT16 specifies `right` and `bottom` as exclusive
311+
/// (one-past-end), matching FreeRDP and the Windows reference clients.
312+
///
309313
/// [2.2.2.1]: <https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpegfx/fb919fce-cc97-4d2b-8cf5-a737a00ef1a6>
310314
#[derive(Clone, PartialEq, Eq)]
311315
pub struct WireToSurface1Pdu {
312316
pub surface_id: u16,
313317
pub codec_id: Codec1Type,
314318
pub pixel_format: PixelFormat,
315-
pub destination_rectangle: InclusiveRectangle,
319+
pub destination_rectangle: ExclusiveRectangle,
316320
pub bitmap_data: Vec<u8>,
317321
}
318322

@@ -331,7 +335,7 @@ impl fmt::Debug for WireToSurface1Pdu {
331335
impl WireToSurface1Pdu {
332336
const NAME: &'static str = "WireToSurface1Pdu";
333337

334-
const FIXED_PART_SIZE: usize = 2 /* SurfaceId */ + 2 /* CodecId */ + 1 /* PixelFormat */ + InclusiveRectangle::FIXED_PART_SIZE /* Dest */ + 4 /* BitmapDataLen */;
338+
const FIXED_PART_SIZE: usize = 2 /* SurfaceId */ + 2 /* CodecId */ + 1 /* PixelFormat */ + ExclusiveRectangle::ENCODED_SIZE /* Dest */ + 4 /* BitmapDataLen */;
335339
}
336340

337341
impl Encode for WireToSurface1Pdu {
@@ -363,7 +367,7 @@ impl<'a> Decode<'a> for WireToSurface1Pdu {
363367
let surface_id = src.read_u16();
364368
let codec_id = Codec1Type::try_from(src.read_u16())?;
365369
let pixel_format = PixelFormat::try_from(src.read_u8())?;
366-
let destination_rectangle = InclusiveRectangle::decode(src)?;
370+
let destination_rectangle = ExclusiveRectangle::decode(src)?;
367371
let bitmap_data_length = cast_length!("BitmapDataLen", src.read_u32())?;
368372

369373
ensure_size!(in: src, size: bitmap_data_length);

‎crates/ironrdp-egfx/src/server.rs‎

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ use ironrdp_core::{Encode, EncodeResult, WriteCursor, decode, impl_as_any};
6262
use ironrdp_dvc::{DvcEncode, DvcMessage, DvcProcessor, DvcServerProcessor};
6363
use ironrdp_graphics::zgfx::{CompressionMode, Compressor, compress_and_wrap_egfx, wrap_uncompressed};
6464
use ironrdp_pdu::gcc::Monitor;
65-
use ironrdp_pdu::geometry::InclusiveRectangle;
65+
use ironrdp_pdu::geometry::ExclusiveRectangle;
6666
use ironrdp_pdu::{PduResult, decode_err};
6767
use tracing::{debug, trace, warn};
6868

@@ -1197,8 +1197,12 @@ impl GraphicsPipelineServer {
11971197
}
11981198
}
11991199

1200-
/// Compute bounding rectangle from regions
1201-
fn compute_dest_rect(regions: &[Avc420Region], default_width: u16, default_height: u16) -> InclusiveRectangle {
1200+
/// Compute bounding rectangle from regions.
1201+
///
1202+
/// Avc420Region uses inclusive bounds; the wire format (RDPGFX_RECT16) is
1203+
/// exclusive, so the returned ExclusiveRectangle adds 1 to the max right
1204+
/// and bottom of the inclusive bounding box.
1205+
fn compute_dest_rect(regions: &[Avc420Region], default_width: u16, default_height: u16) -> ExclusiveRectangle {
12021206
if let Some(first) = regions.first() {
12031207
let mut left = first.left;
12041208
let mut top = first.top;
@@ -1212,18 +1216,18 @@ impl GraphicsPipelineServer {
12121216
bottom = bottom.max(r.bottom);
12131217
}
12141218

1215-
InclusiveRectangle {
1219+
ExclusiveRectangle {
12161220
left,
12171221
top,
1218-
right,
1219-
bottom,
1222+
right: right.saturating_add(1),
1223+
bottom: bottom.saturating_add(1),
12201224
}
12211225
} else {
1222-
InclusiveRectangle {
1226+
ExclusiveRectangle {
12231227
left: 0,
12241228
top: 0,
1225-
right: default_width.saturating_sub(1),
1226-
bottom: default_height.saturating_sub(1),
1229+
right: default_width,
1230+
bottom: default_height,
12271231
}
12281232
}
12291233
}
@@ -1385,11 +1389,11 @@ impl GraphicsPipelineServer {
13851389
let timestamp = Self::make_timestamp(timestamp_ms);
13861390
let frame_id = self.frames.begin_frame(timestamp);
13871391

1388-
let dest_rect = InclusiveRectangle {
1392+
let dest_rect = ExclusiveRectangle {
13891393
left: 0,
13901394
top: 0,
1391-
right: dest_width.saturating_sub(1),
1392-
bottom: dest_height.saturating_sub(1),
1395+
right: dest_width,
1396+
bottom: dest_height,
13931397
};
13941398

13951399
self.output_queue

‎crates/ironrdp-testsuite-core/tests/egfx/client.rs‎

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ use ironrdp_egfx::pdu::{
77
DeleteSurfacePdu, EndFramePdu, GfxPdu, PixelFormat, ResetGraphicsPdu, StartFramePdu, Timestamp, WireToSurface1Pdu,
88
};
99
use ironrdp_graphics::zgfx::wrap_uncompressed;
10-
use ironrdp_pdu::geometry::InclusiveRectangle;
10+
use ironrdp_pdu::geometry::ExclusiveRectangle;
1111

1212
// ============================================================================
1313
// Test Handler
@@ -213,11 +213,11 @@ fn client_handles_uncompressed_via_process() {
213213
surface_id: 1,
214214
codec_id: Codec1Type::Uncompressed,
215215
pixel_format: PixelFormat::XRgb,
216-
destination_rectangle: InclusiveRectangle {
216+
destination_rectangle: ExclusiveRectangle {
217217
left: 0,
218218
top: 0,
219-
right: 3,
220-
bottom: 3,
219+
right: 4,
220+
bottom: 4,
221221
},
222222
bitmap_data: vec![0u8; 4 * 4 * 4],
223223
});
@@ -245,11 +245,11 @@ fn client_dispatches_avc420_via_process() {
245245
surface_id: 1,
246246
codec_id: Codec1Type::Avc420,
247247
pixel_format: PixelFormat::XRgb,
248-
destination_rectangle: InclusiveRectangle {
248+
destination_rectangle: ExclusiveRectangle {
249249
left: 0,
250250
top: 0,
251-
right: 15,
252-
bottom: 15,
251+
right: 16,
252+
bottom: 16,
253253
},
254254
bitmap_data,
255255
});
@@ -276,11 +276,11 @@ fn client_skips_avc420_without_decoder() {
276276
surface_id: 1,
277277
codec_id: Codec1Type::Avc420,
278278
pixel_format: PixelFormat::XRgb,
279-
destination_rectangle: InclusiveRectangle {
279+
destination_rectangle: ExclusiveRectangle {
280280
left: 0,
281281
top: 0,
282-
right: 15,
283-
bottom: 15,
282+
right: 16,
283+
bottom: 16,
284284
},
285285
bitmap_data,
286286
});
@@ -310,11 +310,11 @@ fn client_frame_ordering_via_process() {
310310
surface_id: 1,
311311
codec_id: Codec1Type::Uncompressed,
312312
pixel_format: PixelFormat::XRgb,
313-
destination_rectangle: InclusiveRectangle {
313+
destination_rectangle: ExclusiveRectangle {
314314
left: 0,
315315
top: 0,
316-
right: 3,
317-
bottom: 3,
316+
right: 4,
317+
bottom: 4,
318318
},
319319
bitmap_data: vec![0u8; 4 * 4 * 4],
320320
});
@@ -403,11 +403,11 @@ fn client_rejects_wire_to_unknown_surface() {
403403
surface_id: 99, // does not exist
404404
codec_id: Codec1Type::Uncompressed,
405405
pixel_format: PixelFormat::XRgb,
406-
destination_rectangle: InclusiveRectangle {
406+
destination_rectangle: ExclusiveRectangle {
407407
left: 0,
408408
top: 0,
409-
right: 3,
410-
bottom: 3,
409+
right: 4,
410+
bottom: 4,
411411
},
412412
bitmap_data: vec![0u8; 4 * 4 * 4],
413413
});
@@ -424,7 +424,7 @@ fn client_rejects_invalid_rectangle_ordering() {
424424
surface_id: 1,
425425
codec_id: Codec1Type::Uncompressed,
426426
pixel_format: PixelFormat::XRgb,
427-
destination_rectangle: InclusiveRectangle {
427+
destination_rectangle: ExclusiveRectangle {
428428
left: 50,
429429
top: 0,
430430
right: 10,
@@ -447,13 +447,13 @@ fn client_tolerates_out_of_bounds_rectangle() {
447447
surface_id: 1,
448448
codec_id: Codec1Type::Uncompressed,
449449
pixel_format: PixelFormat::XRgb,
450-
destination_rectangle: InclusiveRectangle {
450+
destination_rectangle: ExclusiveRectangle {
451451
left: 0,
452452
top: 0,
453453
right: 200, // exceeds surface width of 100
454454
bottom: 50,
455455
},
456-
bitmap_data: vec![0u8; 201 * 51 * 4],
456+
bitmap_data: vec![0u8; 200 * 50 * 4],
457457
});
458458
let result = client.process(0, &encode_for_process(&pdu));
459459
assert!(

0 commit comments

Comments
 (0)