diff --git a/crates/ironrdp-client/src/rdp.rs b/crates/ironrdp-client/src/rdp.rs index d184f13b11..ddb31358a0 100644 --- a/crates/ironrdp-client/src/rdp.rs +++ b/crates/ironrdp-client/src/rdp.rs @@ -94,7 +94,7 @@ pub enum DisplayResizeFallbackReason { DisplayControlUnavailable, /// The server did not send the required Display Control capabilities PDU in time. CapabilitiesTimedOut, - /// The server did not reactivate the session after a monitor-layout request in time. + /// The server did not apply the requested desktop size through reactivation or graphics reset in time. ReactivationTimedOut, } @@ -640,6 +640,9 @@ const DISPLAY_CONTROL_READY_TIMEOUT: Duration = Duration::from_secs(3); struct ResizeQueue { in_flight: Option, pending: Option, + /// The last request followed by a matching output reset or reactivation. Connection + /// parameters alone do not establish which layout the server accepted. + confirmed_layout: Option, } impl ResizeQueue { @@ -666,26 +669,82 @@ impl ResizeQueue { }); } - fn completed(&mut self) { - self.in_flight = None; + fn completed(&mut self, desktop_size: (u16, u16)) -> bool { + if self + .in_flight + .as_ref() + .is_some_and(|in_flight| (in_flight.request.width, in_flight.request.height) == desktop_size) + { + self.confirmed_layout = self.in_flight.take().map(|in_flight| in_flight.request); + return true; + } + + // An unsolicited or adjusted layout does not confirm an outstanding request. + self.confirmed_layout = None; + false + } + + /// Whether a request repeats the last completed layout, with nothing in flight + /// that could change it. Servers can ignore such requests without sending a reset. + fn asks_for_current_layout(&self, request: &ResizeRequest, desktop_size: (u16, u16)) -> bool { + self.in_flight.is_none() + && (request.width, request.height) == desktop_size + && self.confirmed_layout == Some(*request) + } + + fn pending_request(&mut self, desktop_size: (u16, u16)) -> Option { + let request = self.pending.as_ref()?.request; + if self.asks_for_current_layout(&request, desktop_size) { + self.pending = None; + None + } else { + Some(request) + } } - fn timed_out_request(&self, now: tokio::time::Instant) -> Option<(ResizeRequest, DisplayResizeFallbackReason)> { - if let Some(in_flight) = self.in_flight.as_ref() + fn timed_out_request( + &mut self, + now: tokio::time::Instant, + desktop_size: (u16, u16), + ) -> Option<(ResizeRequest, DisplayResizeFallbackReason)> { + let expired = if let Some(in_flight) = self.in_flight.as_ref() && now >= in_flight.deadline { - return Some(( + Some(( self.pending .as_ref() .map_or(in_flight.request, |pending| pending.request), DisplayResizeFallbackReason::ReactivationTimedOut, - )); - } + )) + } else { + self.pending + .as_ref() + .filter(|pending| now >= pending.deadline) + .map(|pending| (pending.request, DisplayResizeFallbackReason::CapabilitiesTimedOut)) + }; - self.pending - .as_ref() - .filter(|pending| now >= pending.deadline) - .map(|pending| (pending.request, DisplayResizeFallbackReason::CapabilitiesTimedOut)) + let (request, reason) = expired?; + if (request.width, request.height) == desktop_size + && self + .in_flight + .as_ref() + .is_none_or(|in_flight| (in_flight.request.width, in_flight.request.height) == desktop_size) + { + // Display Control has no explicit acknowledgement. A server can ignore + // scale/physical-size changes or a layout it already has (MS-RDPEDISP 1.3). + // Reconnecting to the same pixel size cannot recover those changes. Leave + // metadata unconfirmed so a later request can retry it. + debug!("Display layout metadata was not confirmed; keeping the current connection"); + if self.in_flight.take().is_none() { + // Capabilities never arrived, so the metadata update cannot be sent. + self.pending = None; + } + // A newer deferred request still needs to be promoted and sent. + self.confirmed_layout = None; + None + } else { + Some((request, reason)) + } } } @@ -3119,6 +3178,7 @@ async fn active_session( let _ = clipboard_event_receiver; let disconnect_reason = 'outer: loop { + let framebuffer_size = (image.width(), image.height()); let resize_deadline = resize_queue.deadline(); let input_batch_deadline = input_batcher.deadline(); let mut malformed_bitmap_redraw_queued = false; @@ -3335,7 +3395,12 @@ async fn active_session( scale_factor, physical_size, }; - if resize_queue.in_flight.is_some() || active_stage.display_control_ready() == Some(false) { + if resize_queue.asks_for_current_layout(&request, framebuffer_size) { + // This request also supersedes a deferred one. + debug!(width, height, "Display already has the requested layout"); + resize_queue.pending = None; + ActiveSessionIteration::outputs(Vec::new()) + } else if resize_queue.in_flight.is_some() || active_stage.display_control_ready() == Some(false) { resize_queue.defer(request); ActiveSessionIteration::outputs(Vec::new()) } else if let Some(dvc_batch) = active_stage.prepare_resize( @@ -3356,6 +3421,10 @@ async fn active_session( } } ActiveSessionIteration::with_outputs(dvc_batch?, outputs) + } else if (request.width, request.height) == framebuffer_size { + debug!("Display Control is unavailable for a metadata-only update"); + resize_queue.pending = None; + ActiveSessionIteration::outputs(Vec::new()) } else { // TODO(#271): use the "auto-reconnect cookie": https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpbcgr/15b0d1c9-2891-4adb-a45e-deb4aeeeab7c debug!("Reconnecting with new size"); @@ -3636,14 +3705,14 @@ async fn active_session( None => core::future::pending().await, } } => { - let (request, reason) = resize_queue - .timed_out_request(tokio::time::Instant::now()) - .expect("resize deadline must correspond to a queued request"); - return Ok(RdpControlFlow::ReconnectWithNewSize { - width: request.width, - height: request.height, - reason, - }); + if let Some((request, reason)) = resize_queue.timed_out_request(tokio::time::Instant::now(), framebuffer_size) { + return Ok(RdpControlFlow::ReconnectWithNewSize { + width: request.width, + height: request.height, + reason, + }); + } + ActiveSessionIteration::outputs(Vec::new()) } _ = async { match rail_queue_release_deadline { @@ -3705,6 +3774,19 @@ async fn active_session( } }; + // ResetGraphics is an explicit completion signal even when a scale-only + // change leaves the pixel dimensions unchanged. Out-of-band resets must + // match the outstanding request before its layout is recorded as applied. + if let Some(output_size) = active_stage.take_graphics_output_reset() + && resize_queue.completed(output_size) + { + debug!( + width = output_size.0, + height = output_size.1, + "Graphics pipeline output reset completed the resize" + ); + } + if let Some(batch) = iteration.dvc_batch { let channel_id = batch.channel_id(); let messages = batch.into_messages(); @@ -4060,7 +4142,7 @@ async fn active_session( debug!(?desktop_size, "Deactivation-Reactivation Sequence completed"); image = DecodedImage::new(PixelFormat::RgbA32, desktop_size.width, desktop_size.height); desktop_update_extent = None; - resize_queue.completed(); + resize_queue.completed((desktop_size.width, desktop_size.height)); if !active_stage.reactivate( connection_activation.io_channel_id(), connection_activation.user_channel_id(), @@ -4266,9 +4348,8 @@ async fn active_session( } if resize_queue.in_flight.is_none() - && let Some(pending) = resize_queue.pending.as_ref() + && let Some(request) = resize_queue.pending_request((image.width(), image.height())) { - let request = pending.request; match active_stage.display_control_ready() { Some(true) => { let batch = active_stage @@ -4334,6 +4415,10 @@ async fn active_session( } } } + None if (request.width, request.height) == (image.width(), image.height()) => { + debug!("Display Control is unavailable for a metadata-only update"); + resize_queue.pending = None; + } None => { debug!("Reconnecting because Display Control is unavailable"); return Ok(RdpControlFlow::ReconnectWithNewSize { @@ -4720,10 +4805,10 @@ mod tests { queue.defer(latest); assert_eq!( - queue.timed_out_request(deadline), + queue.timed_out_request(deadline, (800, 600)), Some((latest, DisplayResizeFallbackReason::ReactivationTimedOut)) ); - queue.completed(); + assert!(queue.completed((1024, 768))); assert!(queue.in_flight.is_none()); assert_eq!(queue.pending.as_ref().map(|pending| pending.request), Some(latest)); } @@ -4735,13 +4820,155 @@ mod tests { queue.defer(request); let deadline = queue.deadline().expect("pending resize must have a deadline"); - assert_eq!(queue.timed_out_request(deadline - Duration::from_millis(1)), None); assert_eq!( - queue.timed_out_request(deadline), + queue.timed_out_request(deadline - Duration::from_millis(1), (800, 600)), + None + ); + assert_eq!( + queue.timed_out_request(deadline, (800, 600)), Some((request, DisplayResizeFallbackReason::CapabilitiesTimedOut)) ); } + #[test] + fn resize_queue_recognizes_a_request_for_the_current_layout() { + let mut queue = ResizeQueue { + confirmed_layout: Some(resize_request(1024, 768)), + ..ResizeQueue::default() + }; + let current = resize_request(1024, 768); + assert_eq!((current.scale_factor, current.physical_size), (100, None)); + + assert!(queue.asks_for_current_layout(¤t, (1024, 768))); + assert!(!queue.asks_for_current_layout(¤t, (1280, 720))); + let rescaled = ResizeRequest { + scale_factor: 150, + ..current + }; + assert!(!queue.asks_for_current_layout(&rescaled, (1024, 768))); + + // A request in flight can still change the layout, so nothing is a no-op meanwhile. + queue.mark_in_flight(rescaled); + assert!(!queue.asks_for_current_layout(&rescaled, (1024, 768))); + assert!(queue.completed((1024, 768))); + assert!(queue.asks_for_current_layout(&rescaled, (1024, 768))); + assert!(!queue.asks_for_current_layout(¤t, (1024, 768))); + } + + #[test] + fn resize_queue_requires_a_matching_server_size_before_recording_a_layout() { + let request = resize_request(1280, 720); + let mut queue = ResizeQueue::default(); + assert!(!queue.asks_for_current_layout(&request, (1280, 720))); + queue.mark_in_flight(request); + assert_eq!(queue.confirmed_layout, None); + assert!(!queue.completed((1024, 768))); + assert_eq!(queue.in_flight.as_ref().map(|flight| flight.request), Some(request)); + assert_eq!(queue.confirmed_layout, None); + assert!(queue.completed((1280, 720))); + assert!(queue.asks_for_current_layout(&request, (1280, 720))); + } + + #[test] + fn resize_queue_drops_a_deferred_duplicate_after_completion() { + let request = resize_request(1280, 720); + let mut queue = ResizeQueue::default(); + queue.mark_in_flight(request); + queue.defer(request); + assert!(queue.completed((1280, 720))); + assert_eq!(queue.pending_request((1280, 720)), None); + assert_eq!(queue.deadline(), None); + + // A different deferred layout still needs to be sent. + let next = resize_request(1600, 900); + queue.defer(next); + assert_eq!(queue.pending_request((1280, 720)), Some(next)); + } + + #[test] + fn resize_queue_completes_scale_only_and_physical_size_changes() { + let current = resize_request(1024, 768); + for request in [ + ResizeRequest { + scale_factor: 150, + ..current + }, + ResizeRequest { + physical_size: Some((300, 200)), + ..current + }, + ] { + let mut queue = ResizeQueue { + confirmed_layout: Some(current), + ..ResizeQueue::default() + }; + assert!(!queue.asks_for_current_layout(&request, (1024, 768))); + queue.mark_in_flight(request); + // An explicit ResetGraphics can complete a request without changing pixels. + assert!(queue.completed((1024, 768))); + assert_eq!(queue.deadline(), None); + assert!(queue.asks_for_current_layout(&request, (1024, 768))); + } + } + + #[test] + fn resize_queue_keeps_unacknowledged_metadata_retryable_without_reconnecting() { + let request = ResizeRequest { + scale_factor: 150, + ..resize_request(1024, 768) + }; + let mut queue = ResizeQueue::default(); + queue.mark_in_flight(request); + let deadline = queue.deadline().unwrap(); + assert_eq!(queue.timed_out_request(deadline, (1024, 768)), None); + assert_eq!(queue.deadline(), None); + assert!(!queue.asks_for_current_layout(&request, (1024, 768))); + + // A later pixel resize still uses the reconnect fallback if it times out. + queue.mark_in_flight(request); + let next = resize_request(1600, 900); + queue.defer(next); + let deadline = queue.in_flight.as_ref().unwrap().deadline; + assert_eq!( + queue.timed_out_request(deadline, (1024, 768)), + Some((next, DisplayResizeFallbackReason::ReactivationTimedOut)) + ); + } + + #[test] + fn resize_queue_sends_newer_metadata_after_an_ignored_request() { + let first = ResizeRequest { + scale_factor: 150, + ..resize_request(1024, 768) + }; + let latest = ResizeRequest { + scale_factor: 175, + ..first + }; + let mut queue = ResizeQueue::default(); + queue.mark_in_flight(first); + queue.defer(latest); + let deadline = queue.in_flight.as_ref().unwrap().deadline; + assert_eq!(queue.timed_out_request(deadline, (1024, 768)), None); + assert!(queue.in_flight.is_none()); + assert_eq!(queue.pending_request((1024, 768)), Some(latest)); + } + + #[test] + fn resize_queue_preserves_fallback_for_a_return_to_the_original_size() { + let mut queue = ResizeQueue::default(); + queue.mark_in_flight(resize_request(1280, 720)); + let current = resize_request(1024, 768); + queue.defer(current); + let deadline = queue.in_flight.as_ref().unwrap().deadline; + // The earlier request can still change the desktop, so the pending request + // for its original size is not merely an unacknowledged metadata update. + assert_eq!( + queue.timed_out_request(deadline, (1024, 768)), + Some((current, DisplayResizeFallbackReason::ReactivationTimedOut)) + ); + } + #[test] fn auto_reconnect_policy_requires_a_cookie_and_respects_its_limit() { let policy = AutoReconnectPolicy::new(2); diff --git a/crates/ironrdp-session/src/active_stage.rs b/crates/ironrdp-session/src/active_stage.rs index 050111d751..d6159cb08e 100644 --- a/crates/ironrdp-session/src/active_stage.rs +++ b/crates/ironrdp-session/src/active_stage.rs @@ -48,6 +48,7 @@ pub struct ActiveStage { enable_server_pointer: bool, window_support_level: Option, graphics_output_needs_full_refresh: bool, + graphics_output_reset: Option<(u16, u16)>, damage_regions: Vec, } @@ -106,6 +107,7 @@ impl ActiveStageBuilder { enable_server_pointer, window_support_level: None, graphics_output_needs_full_refresh: false, + graphics_output_reset: None, damage_regions: Vec::new(), } } @@ -132,6 +134,14 @@ impl ActiveStage { core::mem::take(&mut self.damage_regions) } + /// Takes the most recent successfully applied `ResetGraphics` output size. + /// + /// Unlike a framebuffer size comparison, this also reports resets that keep the + /// same dimensions, such as Display Control requests that only change scaling. + pub fn take_graphics_output_reset(&mut self) -> Option<(u16, u16)> { + self.graphics_output_reset.take() + } + /// Encodes outgoing input events and modifies image if necessary (e.g for client-side pointer /// rendering). pub fn process_fastpath_input( @@ -290,6 +300,7 @@ impl ActiveStage { .unwrap_or_default(); if let Some((width, height)) = output_reset { image.reset_preserving_pointer(width, height)?; + self.graphics_output_reset = Some((width, height)); self.graphics_output_needs_full_refresh = true; } let (region, damage_regions) = @@ -391,6 +402,7 @@ impl ActiveStage { // The x224 processor encodes ShareDataPdu with the server's (possibly new) share_id. self.x224_processor.set_share_id(share_id); self.enable_server_pointer = enable_server_pointer; + self.graphics_output_reset = None; true } diff --git a/crates/ironrdp-testsuite-core/tests/session/active_stage.rs b/crates/ironrdp-testsuite-core/tests/session/active_stage.rs index d8939ad535..3d9a0507a1 100644 --- a/crates/ironrdp-testsuite-core/tests/session/active_stage.rs +++ b/crates/ironrdp-testsuite-core/tests/session/active_stage.rs @@ -204,3 +204,112 @@ fn reset_graphics_clips_a_hotspot_cursor_to_one_pixel() { assert_eq!(image.data(), &[0xFF, 0xFF, 0xFF, 0]); } + +/// Same-size resets must reach the client resize queue even without a completed +/// graphics frame, otherwise scale-only Display Control requests time out. +#[test] +fn output_reset_is_reported_even_when_the_framebuffer_size_is_unchanged() { + use core::any::TypeId; + use std::borrow::Cow; + + use ironrdp_core::encode_vec; + use ironrdp_dvc::DrdynvcClient; + use ironrdp_dvc::pdu::{ + CreateRequestPdu, DataPdu, DrdynvcDataPdu, DrdynvcServerPdu, SoftSyncChannelList, SoftSyncRequestPdu, + SoftSyncTunnelType, + }; + use ironrdp_egfx::client::{GraphicsPipelineClient, GraphicsPipelineHandler}; + use ironrdp_egfx::pdu::{GfxPdu, ResetGraphicsPdu}; + use ironrdp_graphics::zgfx::wrap_uncompressed; + use ironrdp_pdu::Action; + use ironrdp_pdu::mcs::{McsMessage, SendDataIndication}; + use ironrdp_pdu::rdp::vc::{ChannelControlFlags, ChannelPduHeader}; + use ironrdp_pdu::x224::X224; + use ironrdp_session::ActiveStageBuilder; + use ironrdp_svc::{StaticChannelSet, SvcProcessor as _}; + + struct Handler; + impl GraphicsPipelineHandler for Handler {} + + let mut drdynvc = DrdynvcClient::new().with_dynamic_channel(GraphicsPipelineClient::new(Box::new(Handler), None)); + drdynvc + .process( + &encode_vec(&DrdynvcServerPdu::Create(CreateRequestPdu::new( + 1, + ironrdp_egfx::CHANNEL_NAME.to_owned(), + ))) + .unwrap(), + ) + .unwrap(); + let mut static_channels = StaticChannelSet::new(); + assert!(static_channels.insert(drdynvc).is_none()); + assert!( + static_channels + .attach_channel_id(TypeId::of::(), 1004) + .is_none() + ); + let mut stage = ActiveStageBuilder { + static_channels, + user_channel_id: 1001, + io_channel_id: 1003, + message_channel_id: None, + share_id: 1, + compression_type: None, + enable_server_pointer: false, + pointer_software_rendering: false, + } + .build(); + let mut image = DecodedImage::new(PixelFormat::RgbA32, 800, 600); + assert_eq!(stage.take_graphics_output_reset(), None); + + for tunneled in [false, true] { + if tunneled { + stage.enable_reliable_udp_dvc_tunnel().unwrap(); + let soft_sync = encode_vec(&DrdynvcServerPdu::SoftSyncRequest(SoftSyncRequestPdu::new(vec![ + SoftSyncChannelList::new(SoftSyncTunnelType::RELIABLE_UDP, vec![1]), + ]))) + .unwrap(); + stage + .get_svc_processor_mut::() + .unwrap() + .process(&soft_sync) + .unwrap(); + } + + for (width, height) in [(800, 600), (1024, 768), (1024, 768)] { + let reset = encode_vec(&GfxPdu::ResetGraphics(ResetGraphicsPdu { + width: u32::from(width), + height: u32::from(height), + monitors: Vec::new(), + })) + .unwrap(); + let data = encode_vec(&DrdynvcServerPdu::Data(DrdynvcDataPdu::Data(DataPdu::new( + 1, + wrap_uncompressed(&reset), + )))) + .unwrap(); + if tunneled { + stage + .process_dvc_tunnel(&mut image, SoftSyncTunnelType::RELIABLE_UDP, &data) + .unwrap(); + } else { + let mut user_data = encode_vec(&ChannelPduHeader { + length: u32::try_from(data.len()).unwrap(), + flags: ChannelControlFlags::FLAG_FIRST | ChannelControlFlags::FLAG_LAST, + }) + .unwrap(); + user_data.extend(data); + let frame = encode_vec(&X224(McsMessage::SendDataIndication(SendDataIndication { + initiator_id: 1001, + channel_id: 1004, + user_data: Cow::Owned(user_data), + }))) + .unwrap(); + stage.process(&mut image, Action::X224, &frame).unwrap(); + } + assert_eq!((image.width(), image.height()), (width, height)); + assert_eq!(stage.take_graphics_output_reset(), Some((width, height))); + assert_eq!(stage.take_graphics_output_reset(), None); + } + } +}