From 4aa5839d34a11d39f0f8ce8b55f6ee62a5fd4ee8 Mon Sep 17 00:00:00 2001 From: AKolenda Date: Fri, 25 Sep 2026 23:37:54 -0600 Subject: [PATCH 1/3] fix(egfx): keep the bitmap cache across ResetGraphics The compositor emptied the bitmap cache on every ResetGraphics. Windows sends a ResetGraphics for every desktop resize and afterwards keeps pasting toolbars, icons and text with CacheToSurface from slots it filled before the reset, so those regions stayed black after each resize until something forced a fresh upload. The bitmap cache is not part of the graphics output: MS-RDPEGFX 3.3.5.14 only resizes the Graphics Output Buffer, and slots are released by EvictCacheEntry, a cache import or the end of the channel. This is the same reasoning the client already applies to the Progressive and ClearCodec contexts. The reset now keeps the cache and its share of the allocation budget, and still drops surfaces and pending output. --- crates/ironrdp-egfx/src/compositor.rs | 46 +++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/crates/ironrdp-egfx/src/compositor.rs b/crates/ironrdp-egfx/src/compositor.rs index ea7f6003d5..1c8f575001 100644 --- a/crates/ironrdp-egfx/src/compositor.rs +++ b/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. The bitmap cache is kept. /// /// 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,22 @@ 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. + /// + /// The bitmap cache (2.2.2.10, 2.2.2.11) is not part of the graphics output and + /// survives a reset: MS-RDPEGFX 3.3.5.14 only resizes the Graphics Output Buffer, + /// and cache slots are released by `EvictCacheEntry`, a cache import or the end + /// of the channel. Windows sends a `ResetGraphics` for every desktop resize and + /// then keeps pasting toolbars, icons and text from slots it filled before the + /// reset, so dropping them here leaves those regions black until something + /// forces a fresh upload. 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; + // Only the cache keeps its allocations, so only its charge remains. + self.allocated_bytes = self.cache.values().map(|tile| tile.data.len()).sum(); } /// Reserve `len` pixel bytes, or refuse if that would exceed the budget. @@ -1125,9 +1132,10 @@ mod tests { assert_eq!(c.surfaces.len(), 1); } - /// `ResetGraphics` empties both maps, so it must zero the charge with them. + /// `ResetGraphics` empties the surface map, so its charge goes with it; the cache + /// survives the reset and so does its charge. #[test] - fn reset_releases_the_whole_charge() { + fn reset_releases_the_surface_charge_and_keeps_the_cache() { const EDGE: u16 = 4096; let mut c = Compositor::default(); c.reset(1920, 1080); @@ -1140,6 +1148,30 @@ mod tests { c.allocated_bytes, 0, "reset drops every surface, so it drops the charge" ); + + c.create_surface(1, 16, 16); + c.map_surface(1, 0, 0); + c.surface_to_cache(1, 7, &rect(0, 0, 16, 16)); + let cached = c.cache[&7].data.len(); + assert!(cached > 0); + + c.reset(1920, 1080); + assert_eq!(c.cache.len(), 1, "reset keeps the bitmap cache"); + assert_eq!( + c.allocated_bytes, cached, + "only the cache's charge remains after a reset" + ); + + // The kept tile is still usable on a surface created after the reset. + 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(); + assert_eq!( + c.drain_output().len(), + 1, + "a cache paste after the reset still produces output" + ); } /// Cache slots are a second allocation pool keyed by `u16`. Charging them against From 568f9554aa803d9c3d3ff3b3fb77dec7902ab596 Mon Sep 17 00:00:00 2001 From: AKolenda Date: Sat, 26 Sep 2026 12:40:24 -0600 Subject: [PATCH 2/3] docs(egfx): explain why the retained cache leaves room for surfaces MS-RDPEGFX 3.3.1.4 caps the bitmap cache at 100 MB, or 16 MB with SMALL_CACHE, so the charge kept across ResetGraphics leaves a conforming server room in the budget for the surfaces it creates after the reset. --- crates/ironrdp-egfx/src/compositor.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/ironrdp-egfx/src/compositor.rs b/crates/ironrdp-egfx/src/compositor.rs index 1c8f575001..08abf715e7 100644 --- a/crates/ironrdp-egfx/src/compositor.rs +++ b/crates/ironrdp-egfx/src/compositor.rs @@ -186,7 +186,10 @@ impl Compositor { self.surfaces.clear(); self.frame.clear(); self.ready.clear(); - // Only the cache keeps its allocations, so only its charge remains. + // Only the cache keeps its allocations, so only its charge remains. The server must + // keep the cache within 100 MB, or 16 MB when it confirms SMALL_CACHE (MS-RDPEGFX + // 3.3.1.4), so a conforming server still has room in the budget for the surfaces it + // creates after the reset. self.allocated_bytes = self.cache.values().map(|tile| tile.data.len()).sum(); } From 46ee89b58d8cb48d92a04c1e2503c47d7d417a9a Mon Sep 17 00:00:00 2001 From: AKolenda Date: Sun, 27 Sep 2026 23:16:41 -0600 Subject: [PATCH 3/3] fix(egfx): drop a bitmap cache over the protocol cap on reset MS-RDPEGFX 3.3.1.4 caps the bitmap cache at 100 MB. A server that filled the cache past that cap kept it across ResetGraphics, together with its share of the compositor budget, and could leave no room for the surfaces it creates after the reset. Such a cache is now dropped with the surfaces, and a cache within the cap is kept as before. Also cite the bitmap cache as MS-RDPEGFX 3.3.1.4 and SurfaceToCache as 2.2.2.6. The comments cited 2.2.2.10 and 2.2.2.11, which are DeleteSurface and StartFrame. --- crates/ironrdp-egfx/src/compositor.rs | 57 +++++++++++++++++++++------ 1 file changed, 46 insertions(+), 11 deletions(-) diff --git a/crates/ironrdp-egfx/src/compositor.rs b/crates/ironrdp-egfx/src/compositor.rs index 08abf715e7..aa09d8767c 100644 --- a/crates/ironrdp-egfx/src/compositor.rs +++ b/crates/ironrdp-egfx/src/compositor.rs @@ -48,6 +48,11 @@ const MAX_OUTPUT_DIM: u16 = 32766; /// single-surface case (16384*16384*4) impossible to reach. const MAX_COMPOSITOR_BYTES: usize = 256 * 1024 * 1024; +/// The largest bitmap cache a server may fill: MS-RDPEGFX 3.3.1.4 caps it at 100 MB, or 16 MB +/// when the server confirms SMALL_CACHE or THINCLIENT. The compositor does not see the +/// confirmed capabilities, so it applies the larger cap. +const MAX_BITMAP_CACHE_BYTES: usize = 100 * 1024 * 1024; + /// A rectangular region of the graphics output whose pixels changed within a frame. /// /// `region` is in output space (after a surface-to-output mapping), using the @@ -114,7 +119,7 @@ struct CanvasMut<'a> { height: u16, } -/// A cached bitmap tile (MS-RDPEGFX bitmap cache, 2.2.2.10 / 2.2.2.11). +/// A cached bitmap tile (MS-RDPEGFX 3.3.1.4 bitmap cache, filled by `SurfaceToCache`, 2.2.2.6). #[derive(Debug)] struct CachedTile { width: u16, @@ -163,7 +168,7 @@ impl Compositor { } /// Handle `ResetGraphics`: set the output size and drop all surfaces and pending - /// output. The bitmap cache is kept. + /// output. The bitmap cache is kept while it stays within the protocol's cap. /// /// 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 @@ -173,10 +178,10 @@ impl Compositor { /// output, so painting them into the new one repaints stale pixels and, after a /// shrink, addresses a region the new output no longer contains. /// - /// The bitmap cache (2.2.2.10, 2.2.2.11) is not part of the graphics output and - /// survives a reset: MS-RDPEGFX 3.3.5.14 only resizes the Graphics Output Buffer, - /// and cache slots are released by `EvictCacheEntry`, a cache import or the end - /// of the channel. Windows sends a `ResetGraphics` for every desktop resize and + /// The bitmap cache (3.3.1.4) is not part of the graphics output and survives a + /// reset: MS-RDPEGFX 3.3.5.14 only resizes the Graphics Output Buffer, and cache + /// slots are released by `EvictCacheEntry`, a cache import or the end of the + /// channel. Windows sends a `ResetGraphics` for every desktop resize and /// then keeps pasting toolbars, icons and text from slots it filled before the /// reset, so dropping them here leaves those regions black until something /// forces a fresh upload. @@ -186,11 +191,22 @@ impl Compositor { self.surfaces.clear(); self.frame.clear(); self.ready.clear(); - // Only the cache keeps its allocations, so only its charge remains. The server must - // keep the cache within 100 MB, or 16 MB when it confirms SMALL_CACHE (MS-RDPEGFX - // 3.3.1.4), so a conforming server still has room in the budget for the surfaces it - // creates after the reset. - self.allocated_bytes = self.cache.values().map(|tile| tile.data.len()).sum(); + // Only the cache keeps its allocations, so only its charge remains. Within the + // 3.3.1.4 cap the cache leaves room in the budget for the surfaces the server creates + // after the reset. A server that filled the cache past the cap could otherwise hold + // that room for the rest of the session, so such a cache is dropped with the surfaces. + let cached_bytes: usize = self.cache.values().map(|tile| tile.data.len()).sum(); + if cached_bytes > MAX_BITMAP_CACHE_BYTES { + debug!( + cached_bytes, + cap = MAX_BITMAP_CACHE_BYTES, + "bitmap cache exceeds the MS-RDPEGFX cap; dropping it on reset" + ); + self.cache.clear(); + self.allocated_bytes = 0; + } else { + self.allocated_bytes = cached_bytes; + } } /// Reserve `len` pixel bytes, or refuse if that would exceed the budget. @@ -1177,6 +1193,25 @@ mod tests { ); } + /// A cache past the MS-RDPEGFX 3.3.1.4 cap is dropped on reset, so it cannot hold the + /// budget the surfaces created after the reset need. + #[test] + fn reset_drops_a_cache_over_the_protocol_cap() { + const EDGE: u16 = 4096; // 64 MiB per surface, and per full-surface tile + let mut c = Compositor::default(); + c.reset(1920, 1080); + c.create_surface(1, EDGE, EDGE); + for slot in 0..2 { + c.surface_to_cache(1, slot, &rect(0, 0, EDGE, EDGE)); + } + assert_eq!(c.cache.len(), 2); + assert!(c.cache.values().map(|tile| tile.data.len()).sum::() > MAX_BITMAP_CACHE_BYTES); + + c.reset(1920, 1080); + assert!(c.cache.is_empty(), "a cache over the cap does not survive the reset"); + assert_eq!(c.allocated_bytes, 0); + } + /// Cache slots are a second allocation pool keyed by `u16`. Charging them against /// the same budget is what stops a peer from bypassing the surface limit by /// parking the same pixels in tens of thousands of slots instead.