diff --git a/crates/ironrdp-capture-replay/src/routing.rs b/crates/ironrdp-capture-replay/src/routing.rs index 1eaa8c8654..4eab203327 100644 --- a/crates/ironrdp-capture-replay/src/routing.rs +++ b/crates/ironrdp-capture-replay/src/routing.rs @@ -384,6 +384,16 @@ impl ReplayRouter { pixels: self.image.data().to_vec(), })?; } + // The EGFX client drops an update whose codec rejected it instead of failing + // the channel, so an unsupported codec has to be read from the decoder. + if self.take_unsupported_egfx_codec() { + report.gaps.push(ReplayGap { + packet: message.packet, + direction: ReplayDirection::Server, + kind: ReplayGapKind::Unsupported, + skipped_bytes: 0, + }); + } self.drain_egfx_output(message.packet, report, frame_sink)?; Ok(( route, diff --git a/crates/ironrdp-egfx/src/client.rs b/crates/ironrdp-egfx/src/client.rs index 67830ece1f..1b21d77061 100644 --- a/crates/ironrdp-egfx/src/client.rs +++ b/crates/ironrdp-egfx/src/client.rs @@ -76,6 +76,9 @@ use crate::pdu::{ SurfaceToSurfacePdu, WireToSurface2Pdu, }; +/// Consecutive undecodable updates from one codec that force its state to be cleared. +const MAX_DROPPED_UPDATES_IN_A_ROW: u64 = 16; + /// Max capacity to keep for decompressed buffer when cleared. const MAX_DECOMPRESSED_BUFFER_CAPACITY: usize = 16384; // 16 KiB @@ -405,6 +408,10 @@ pub struct GraphicsPipelineClient { clearcodec_decoder: Option, planar_decoder: BitmapStreamDecoder, progressive_decoder: ProgressiveDecoder, + /// Updates dropped because their codec rejected them, and the streak the current codec + /// is on. A streak long enough to look permanent clears that codec's state. + dropped_updates: u64, + dropped_in_a_row: (Codec1Type, u64), decompressor: zgfx::Decompressor, decompressed_buffer: Vec, @@ -434,6 +441,8 @@ impl GraphicsPipelineClient { clearcodec_decoder: None, planar_decoder: BitmapStreamDecoder::default(), progressive_decoder: ProgressiveDecoder::new(), + dropped_updates: 0, + dropped_in_a_row: (Codec1Type::Uncompressed, 0), decompressor: zgfx::Decompressor::new(), decompressed_buffer: Vec::new(), state: ClientState::WaitingForConfirm, @@ -815,17 +824,28 @@ impl GraphicsPipelineClient { match pdu.codec_id { Codec1Type::Avc420 => { - self.decode_avc420(pdu.surface_id, &pdu.destination_rectangle, &pdu.bitmap_data)?; + match self.decode_avc420(pdu.surface_id, &pdu.destination_rectangle, &pdu.bitmap_data) { + Ok(()) => self.dropped_in_a_row.1 = 0, + Err(error) => self.drop_update(Codec1Type::Avc420, pdu.surface_id, pdu.bitmap_data.len(), &error), + } } Codec1Type::Avc444 | Codec1Type::Avc444v2 => { debug!("AVC444 codec not yet implemented, forwarding to handler"); self.handler.on_unhandled_pdu(&GfxPdu::WireToSurface1(pdu)); } Codec1Type::ClearCodec => { - self.decode_clearcodec(pdu.surface_id, &pdu.destination_rectangle, &pdu.bitmap_data)?; + match self.decode_clearcodec(pdu.surface_id, &pdu.destination_rectangle, &pdu.bitmap_data) { + Ok(()) => self.dropped_in_a_row.1 = 0, + Err(error) => { + self.drop_update(Codec1Type::ClearCodec, pdu.surface_id, pdu.bitmap_data.len(), &error) + } + } } Codec1Type::Planar => { - self.decode_planar(pdu.surface_id, &pdu.destination_rectangle, &pdu.bitmap_data)?; + match self.decode_planar(pdu.surface_id, &pdu.destination_rectangle, &pdu.bitmap_data) { + Ok(()) => self.dropped_in_a_row.1 = 0, + Err(error) => self.drop_update(Codec1Type::Planar, pdu.surface_id, pdu.bitmap_data.len(), &error), + } } Codec1Type::Uncompressed => { self.handle_uncompressed(pdu); @@ -848,19 +868,25 @@ impl GraphicsPipelineClient { .ok_or_else(|| pdu_other_err!("unknown surface in WireToSurface2"))?; let (surface_width, surface_height) = (surface.width, surface.height); - let tiles = self - .progressive_decoder - .decode_bitmap( - pdu.surface_id, - pdu.codec_context_id, - surface_width, - surface_height, - &pdu.bitmap_data, - ) - .map_err(|error| { - warn!(?error, "rfx progressive decode failed"); - pdu_other_err!("rfx progressive decode failed") - })?; + // A payload this client cannot decode costs the tiles it carried, which the next + // update repaints; ending the session over it costs the session. Tile state is + // unchanged on failure, so later payloads still decode against valid coefficients. + let tiles = match self.progressive_decoder.decode_bitmap( + pdu.surface_id, + pdu.codec_context_id, + surface_width, + surface_height, + &pdu.bitmap_data, + ) { + Ok(tiles) => { + self.dropped_in_a_row.1 = 0; + tiles + } + Err(error) => { + self.drop_update(Codec1Type::RemoteFx, pdu.surface_id, pdu.bitmap_data.len(), &error); + return Ok(()); + } + }; for tile in tiles { let left = tile.x_idx.saturating_mul(64); @@ -898,6 +924,51 @@ impl GraphicsPipelineClient { Ok(()) } + /// Drop one update whose codec rejected it. + /// + /// The update carried a region the next repaint covers, so losing it degrades the + /// picture; ending the session over server-controlled data loses the session. A streak + /// long enough to look permanent clears that codec's state, since a frozen region with + /// no way back is worse than the cost of rebuilding from the next keyframe or CONTEXT. + fn drop_update(&mut self, codec: Codec1Type, surface_id: u16, bytes: usize, error: &dyn core::fmt::Debug) { + self.dropped_updates = self.dropped_updates.saturating_add(1); + if self.dropped_in_a_row.0 == codec { + self.dropped_in_a_row.1 = self.dropped_in_a_row.1.saturating_add(1); + } else { + self.dropped_in_a_row = (codec, 1); + } + + if self.dropped_updates == 1 || self.dropped_updates.is_multiple_of(64) { + warn!( + ?error, + ?codec, + surface_id, + bytes, + dropped_total = self.dropped_updates, + "dropping an undecodable surface update" + ); + } + + if self.dropped_in_a_row.1 < MAX_DROPPED_UPDATES_IN_A_ROW { + return; + } + + warn!(?codec, dropped = self.dropped_in_a_row.1, "clearing decoder state"); + self.dropped_in_a_row.1 = 0; + match codec { + Codec1Type::Avc420 | Codec1Type::Avc444 | Codec1Type::Avc444v2 => { + if let Some(ref mut decoder) = self.h264_decoder { + decoder.reset(); + } + } + // Drops the V-bar and glyph caches the stream builds up. + Codec1Type::ClearCodec => self.clearcodec_decoder = None, + Codec1Type::RemoteFx => self.progressive_decoder.reset(), + // Planar decoding carries no state across updates. + _ => {} + } + } + 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))?; @@ -1850,11 +1921,57 @@ mod tests { assert_eq!(output[0].data.len(), 36 * 36 * 4); } + /// The continuation is rejected by the decoder, proving the context is gone, but the + /// session survives it: an undecodable payload is skipped, not fatal. fn assert_progressive_context_is_deleted(clear: impl FnOnce(&mut GraphicsPipelineClient)) { let mut client = progressive_client(); wire_progressive(&mut client, progressive_context_stream(true)).unwrap(); clear(&mut client); - assert!(wire_progressive(&mut client, progressive_context_stream(false)).is_err()); + assert!(wire_progressive(&mut client, progressive_context_stream(false)).is_ok()); + assert_eq!(client.dropped_updates, 1); + } + + #[test] + fn a_streak_of_dropped_updates_clears_the_codec_state() { + let mut client = progressive_client(); + wire_progressive(&mut client, progressive_context_stream(true)).unwrap(); + + for _ in 0..MAX_DROPPED_UPDATES_IN_A_ROW { + assert!(wire_progressive(&mut client, vec![0xFF; 32]).is_ok()); + } + assert_eq!(client.dropped_updates, MAX_DROPPED_UPDATES_IN_A_ROW); + assert_eq!(client.dropped_in_a_row.1, 0); + + // The context established before the streak went with the state, so a continuation + // that relied on it is dropped too. + assert!(wire_progressive(&mut client, progressive_context_stream(false)).is_ok()); + assert_eq!(client.dropped_updates, MAX_DROPPED_UPDATES_IN_A_ROW + 1); + + // A decodable payload clears the streak. + wire_progressive(&mut client, progressive_context_stream(true)).unwrap(); + assert_eq!(client.dropped_in_a_row.1, 0); + } + + #[test] + fn an_undecodable_clearcodec_update_is_dropped_not_fatal() { + let mut client = progressive_client(); + + let result = client.handle_pdu(GfxPdu::WireToSurface1(crate::pdu::WireToSurface1Pdu { + surface_id: 1, + codec_id: Codec1Type::ClearCodec, + pixel_format: PixelFormat::XRgb, + destination_rectangle: ExclusiveRectangle { + left: 0, + top: 0, + right: 64, + bottom: 64, + }, + bitmap_data: vec![0xFF; 16], + })); + + assert!(result.is_ok()); + assert_eq!(client.dropped_updates, 1); + assert_eq!(client.dropped_in_a_row, (Codec1Type::ClearCodec, 1)); } #[test]