fix(egfx): stop the desktop from tearing when the server resizes graphics output

crates/ironrdp-egfx/src/client.rs

@@ -502,6 +502,13 @@ impl GraphicsPipelineClient {
     ///
     /// The returned dimensions satisfy the protocol's output limit and the
     /// compositor's output-framebuffer allocation limit and are reported once.
+    ///
+    /// The framebuffer must follow this size *before* the compositor deltas drained by
+    /// [`Self::drain_output`] are applied, because a reset and the deltas that repaint the
+    /// new output arrive in the same payload and the server will not send them again.
+    ///
+    /// A same-size reset is still reported when valid. Per MS-RDPEGFX 3.3.5.14 the reset
+    /// destroys every surface even when the output dimensions are unchanged.
     #[must_use]
     pub fn take_output_reset(&mut self) -> Option<(u16, u16)> {
         self.pending_output_reset.take()
@@ -694,6 +701,13 @@ impl GraphicsPipelineClient {
         let output_size = Compositor::materializable_output_size(width, height);
 
         // Per spec, ResetGraphics implicitly destroys all surfaces
+        let surface_count = self.surfaces.len();
+        // Tile coefficient buffers belong to those surfaces. Keeping them lets a
+        // later surface reuse the same id and difference against the previous
+        // desktop, which decodes as torn or duplicated tiles.
+        // CONTEXT / ClearCodec glyph cache stay: 3.3.5.14 only redefines the
+        // output buffer, and Windows will not re-send SYNC + CONTEXT.
+        self.progressive_decoder.clear_tile_references();
         self.surfaces.clear();
         self.compositor.reset(width, height);
         self.pending_output_reset = output_size;
@@ -722,8 +736,12 @@ impl GraphicsPipelineClient {
         // drop the glyph cache, so a legitimate post-reset GLYPH_HIT would fail unless the
         // server redundantly re-sent every glyph.
 
+        debug!(
+            width,
+            height, surface_count, "ResetGraphics: surfaces destroyed; tile refs dropped; progressive CONTEXT retained"
+        );
+
         if output_size.is_some() {
-            debug!(width, height, "Graphics reset");
             self.handler.on_reset_graphics(width, height);
             Ok(())
         } else {
@@ -757,13 +775,19 @@ impl GraphicsPipelineClient {
     }
 
     fn handle_delete_surface(&mut self, surface_id: u16) {
+        // MS-RDPEGFX: deleting a surface drops that surface's progressive tile
+        // references. A following difference tile before a new base tile will
+        // correctly fail with MissingTileReference.
         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");
+            debug!(surface_id, "DeleteSurface cleared progressive tile references");
             self.handler.on_surface_deleted(surface_id);
         } else {
-            warn!(surface_id, "DeleteSurface for unknown surface");
+            warn!(
+                surface_id,
+                "DeleteSurface for unknown surface (progressive refs cleared)"
+            );
         }
     }
 
@@ -1424,6 +1448,25 @@ mod tests {
                 pixel_format: PixelFormat::XRgb,
             }))
             .unwrap();
+        // Give the surface content first: mapping a surface that has never been painted
+        // publishes nothing, so the delta asserted below would not exist.
+        client
+            .handle_pdu(GfxPdu::SolidFill(SolidFillPdu {
+                surface_id: 1,
+                fill_pixel: crate::pdu::Color {
+                    b: 0x33,
+                    g: 0x22,
+                    r: 0x11,
+                    xa: 0,
+                },
+                rectangles: vec![ExclusiveRectangle {
+                    left: 0,
+                    top: 0,
+                    right: 2,
+                    bottom: 2,
+                }],
+            }))
+            .unwrap();
         client
             .handle_pdu(GfxPdu::MapSurfaceToScaledOutput(MapSurfaceToScaledOutputPdu {
                 surface_id: 1,
@@ -1717,6 +1760,37 @@ mod tests {
         assert_eq!(client.frames_queued, 0, "frame queue should be reset");
     }
 
+    /// Same-size `ResetGraphics` must still be observable: it destroys every surface, so a
+    /// consumer has to re-blit even when the output dimensions did not change.
+    #[test]
+    fn take_output_reset_reports_same_size_resets() {
+        let mut client = GraphicsPipelineClient::new(Box::new(TestHandler), None);
+        assert!(client.take_output_reset().is_none());
+
+        client
+            .handle_pdu(GfxPdu::ResetGraphics(crate::pdu::ResetGraphicsPdu {
+                width: 800,
+                height: 600,
+                monitors: vec![],
+            }))
+            .expect("valid reset dimensions");
+        assert_eq!(client.take_output_reset(), Some((800, 600)));
+        assert!(client.take_output_reset().is_none(), "flag is one-shot");
+
+        client
+            .handle_pdu(GfxPdu::ResetGraphics(crate::pdu::ResetGraphicsPdu {
+                width: 800,
+                height: 600,
+                monitors: vec![],
+            }))
+            .expect("valid reset dimensions");
+        assert_eq!(
+            client.take_output_reset(),
+            Some((800, 600)),
+            "same-size ResetGraphics must still signal a full client re-blit"
+        );
+    }
+
     #[test]
     fn crop_decoded_frame_identity() {
         let data = vec![0xFFu8; 4 * 4 * 4];
@@ -2245,6 +2319,40 @@ mod tests {
         assert!(wire_progressive(&mut client, progressive_context_stream(false)).is_ok());
     }
 
+    #[test]
+    fn reset_graphics_drops_tile_references_of_implicitly_destroyed_surfaces() {
+        let mut client = progressive_client();
+        wire_progressive(&mut client, progressive_context_stream(true)).unwrap();
+        wire_progressive(&mut client, progressive_tile_stream(0, 0, 64, 64)).unwrap();
+        assert!(
+            client.progressive_decoder.total_reference_count() > 0,
+            "first-pass tile must leave a difference reference"
+        );
+
+        client
+            .handle_pdu(GfxPdu::ResetGraphics(crate::pdu::ResetGraphicsPdu {
+                width: 128,
+                height: 96,
+                monitors: vec![],
+            }))
+            .unwrap();
+
+        // MS-RDPEGFX 2.2.2.14 / 3.3.5.14: ResetGraphics destroys every surface.
+        // Tile coefficient buffers belong to those surfaces. Reusing surface id 0
+        // after a Display Control resize must not difference against the old desktop.
+        assert_eq!(client.progressive_decoder.total_reference_count(), 0);
+
+        client
+            .handle_pdu(GfxPdu::CreateSurface(crate::pdu::CreateSurfacePdu {
+                surface_id: 1,
+                width: 128,
+                height: 96,
+                pixel_format: PixelFormat::XRgb,
+            }))
+            .unwrap();
+        assert!(wire_progressive(&mut client, progressive_context_stream(false)).is_ok());
+    }
+
     #[test]
     fn progressive_context_is_deleted_with_encoding_context() {
         let mut client = progressive_client();

crates/ironrdp-egfx/src/compositor.rs

@@ -162,8 +162,8 @@ impl Compositor {
             .then_some((width, height))
     }
 
-    /// Handle `ResetGraphics`: set the output size and drop all surfaces, cache and
-    /// pending output.
+    /// Handle `ResetGraphics`: set the output size and drop all surfaces and pending
+    /// output, keeping the bitmap cache.
     ///
     /// Per MS-RDPEGFX 2.2.2.14 a reset implicitly destroys every surface and
     /// redefines the graphics output, so deltas produced before it are discarded
@@ -172,15 +172,21 @@ impl Compositor {
     /// `ResetGraphics` together, and those deltas were clipped against the previous
     /// output, so painting them into the new one repaints stale pixels and, after a
     /// shrink, addresses a region the new output no longer contains.
+    ///
+    /// Cache slots are not surfaces. They are connection-scoped and 3.3.5.14
+    /// redefines only the output buffer, so they survive — the same reasoning that
+    /// keeps the progressive CONTEXT and the ClearCodec glyph cache. Windows depends
+    /// on it: after a resolution change it restores the desktop almost entirely from
+    /// slots filled before the reset, and a dropped slot makes every one of those
+    /// blits a silent no-op that leaves the old picture on screen.
     pub(crate) fn reset(&mut self, width: u32, height: u32) {
         self.output_width = u16::try_from(width).unwrap_or(u16::MAX);
         self.output_height = u16::try_from(height).unwrap_or(u16::MAX);
         self.surfaces.clear();
-        self.cache.clear();
         self.frame.clear();
         self.ready.clear();
-        // Every charged allocation lived in one of those, so the whole charge goes.
-        self.allocated_bytes = 0;
+        // Everything charged outside the surviving cache lived in those.
+        self.allocated_bytes = self.cache.values().map(|tile| tile.data.len()).sum();
     }
 
     /// Reserve `len` pixel bytes, or refuse if that would exceed the budget.
@@ -979,6 +985,44 @@ mod tests {
         assert_eq!(&u.data[0..4], &[0x30, 0x20, 0x10, 0xFF]);
     }
 
+    /// The bitmap cache outlives `ResetGraphics`.
+    ///
+    /// Windows fills cache slots before a resolution change and then restores the
+    /// desktop from them afterwards, with hundreds of `CacheToSurface` PDUs against
+    /// slots filled before the reset. MS-RDPEGFX 3.3.5.14 only redefines the graphics
+    /// output buffer; cache slots are connection-scoped. Dropping them here makes
+    /// every one of those blits a silent no-op, so the desktop keeps whatever the
+    /// client last invented for those pixels.
+    #[test]
+    fn reset_keeps_the_bitmap_cache() {
+        let mut c = Compositor::default();
+        c.reset(128, 128);
+        c.create_surface(1, 16, 16);
+        c.solid_fill(
+            1,
+            &Color {
+                b: 0x10,
+                g: 0x20,
+                r: 0x30,
+                xa: 0,
+            },
+            &[rect(0, 0, 8, 8)],
+        );
+        c.surface_to_cache(1, 7, &rect(0, 0, 8, 8));
+        c.end_frame();
+        let _ = c.drain_output();
+
+        c.reset(256, 256);
+        c.create_surface(2, 16, 16);
+        c.map_surface(2, 0, 0);
+        c.cache_to_surface(7, 2, &[Point { x: 0, y: 0 }]);
+        c.end_frame();
+
+        let updates = c.drain_output();
+        assert_eq!(updates.len(), 1, "cached tile must still paint after a reset");
+        assert_eq!(&updates[0].data[0..4], &[0x30, 0x20, 0x10, 0xFF]);
+    }
+
     /// A destination rectangle larger than the surface is clipped, not panicked.
     #[test]
     fn oversized_rect_is_clipped() {

crates/ironrdp-graphics/src/progressive.rs

@@ -1218,6 +1218,17 @@ impl core::fmt::Display for ProgressiveDecodeError {
     }
 }
 
+impl core::error::Error for ProgressiveDecodeError {
+    fn source(&self) -> Option<&(dyn core::error::Error + 'static)> {
+        match self {
+            Self::Pdu(e) => Some(e),
+            Self::Rlgr(e) => Some(e),
+            Self::Srl(e) => Some(e),
+            _ => None,
+        }
+    }
+}
+
 impl From<ironrdp_core::DecodeError> for ProgressiveDecodeError {
     fn from(e: ironrdp_core::DecodeError) -> Self {
         Self::Pdu(e)
@@ -1547,6 +1558,21 @@ impl ProgressiveDecoder {
         self.surface_context_flags.remove(&surface_id);
     }
 
+    /// Total retained difference-tile coefficient buffers across all surfaces.
+    #[must_use]
+    pub fn total_reference_count(&self) -> usize {
+        self.references.len()
+    }
+
+    /// Drop every surface's difference-tile coefficient buffers.
+    ///
+    /// `ResetGraphics` implicitly destroys all surfaces (MS-RDPEGFX 2.2.2.14)
+    /// but does not re-negotiate progressive CONTEXT. Tile references belong
+    /// to the destroyed surfaces; CONTEXT does not.
+    pub fn clear_tile_references(&mut self) {
+        self.references.clear();
+    }
+
     /// Reset codec-context state while retaining surface sub-band references.
     pub fn reset(&mut self) {
         self.contexts.clear();
@@ -2075,14 +2101,17 @@ mod tests {
     }
 
     #[test]
-    fn upgrade_pass_rejects_truncated_srl() {
+    fn upgrade_pass_completes_when_the_srl_stream_ends_early() {
         let mut coefficients = [0i16; COEFFICIENTS_PER_COMPONENT];
         let mut sign = [SIGN_POSITIVE; COEFFICIENTS_PER_COMPONENT];
         sign[0] = SIGN_ZERO;
 
         let mut prev_prog_quant = ComponentCodecQuant::LOSSLESS;
         prev_prog_quant.hl1 = 4;
 
+        // Windows stops emitting once the remaining coefficients are all zero, so the SRL
+        // stream is shorter than the band's coefficient count. This used to drop the whole
+        // tile update, which on screen is a block that never refreshes.
         assert_eq!(
             decode_upgrade_pass(
                 &[0x80, 0x00],
@@ -2093,7 +2122,7 @@ mod tests {
                 &mut coefficients,
                 &mut sign,
             ),
-            Err(SrlError::Truncated)
+            Ok(())
         );
     }
 
@@ -2102,28 +2131,40 @@ mod tests {
         let mut tile = TileState::new();
         let mut prev_prog_quant = ComponentCodecQuant::LOSSLESS;
         prev_prog_quant.hl1 = 4;
-        tile.prog_quant = [prev_prog_quant; 3];
+
+        // The third component's quantization bit width is out of range, so its magnitude
+        // decode must fail. On the wire a quant is a 4-bit nibble (see
+        // `ComponentCodecQuant::decode`) and cannot reach 16; this goes through a struct
+        // literal instead, for the same reason as the `quant_validate` tests in rfx.rs: a
+        // literal-constructed value deserves to be rejected before it is used.
+        let mut out_of_range_quant = ComponentCodecQuant::LOSSLESS;
+        out_of_range_quant.hl1 = 20;
+
+        tile.prog_quant = [prev_prog_quant, prev_prog_quant, out_of_range_quant];
         tile.pass = 1;
         tile.quality = 50;
+        // Each component leaves one SRL entry in HL1, so the first two decode successfully.
         tile.sign[0][0] = SIGN_ZERO;
         tile.sign[1][0] = SIGN_ZERO;
+        tile.sign[2][0] = SIGN_ZERO;
 
         let coefficients = tile.coefficients;
         let sign = tile.sign;
+        let prog_quant = tile.prog_quant;
 
         assert_eq!(
             tile.decode_upgrade(
-                [&[0x90, 0x00], &[0x80, 0x00], &[]],
+                [&[0x90, 0x00], &[0x80, 0x00], &[0x90, 0x00]],
                 [&[], &[], &[]],
                 [ComponentCodecQuant::LOSSLESS; 3],
                 75,
             ),
-            Err(SrlError::Truncated)
+            Err(SrlError::InvalidBitCount(20))
         );
 
         assert_eq!(tile.coefficients, coefficients);
         assert_eq!(tile.sign, sign);
-        assert_eq!(tile.prog_quant, [prev_prog_quant; 3]);
+        assert_eq!(tile.prog_quant, prog_quant);
         assert_eq!(tile.pass, 1);
         assert_eq!(tile.quality, 50);
     }