From 4eceb75d8a899b35c28383501f4beca06c9f8a3d Mon Sep 17 00:00:00 2001 From: Gabriel Date: Fri, 3 Jul 2026 20:49:15 -0300 Subject: fix: kitty graphics streams freeze on partial updates and leak host images (#948) * fix: request a render on kitty graphics writes pty output containing a kitty graphics sequence only scheduled the deferred settle render. that render's wake-up notifies the render loop only when the dirty flag was not already set, so under a continuous stream of image updates the notify can be dropped and the pane keeps showing an old frame until something else forces a full render. request an immediate render like any other output. the dirty flag and the minimum render interval still coalesce chunked writes, and the settle render stays as a backstop. refs #947 * fix: fingerprint kitty images exactly across retransmissions the image fingerprint hashed only the first, middle, and last 4096 bytes of pixel data. a program that streams updates by retransmitting one image id keeps the length and dimensions constant, so a frame whose changed pixels all fall outside those three windows gets the same fingerprint as the frame before it. the encoder then treats the image as already uploaded, emits no graphics bytes, and the frame is dropped as identical to the previous render. the pane stays stuck on the last uploaded image until a resize resets the graphics surface. hash the full payload instead. to keep that off the render hot path, cache the fingerprint per image id and recompute it only when the image's transmit time changes. libghostty-vt already refreshes the transmit time on every transmission; vendor patch 0002 exposes it through a new GHOSTTY_KITTY_IMAGE_DATA_TRANSMIT_TIME_NS accessor. a static image costs one map lookup per render and a streaming image one hash per transmitted frame. refs #947 * fix: delete superseded host kitty images host image ids derive from image content, so a pane image whose pixels change maps to a new host id on every update. nothing deleted the host image it replaced, and a streaming pane left one full-size image behind in the host terminal per changed frame until the graphics surface was reset. track which host image backs each pane image and delete the replaced one once no source references it. refs #947 * fix: prune stale kitty graphics source entries a source entry whose pane closed or whose placement disappeared stayed in the cache forever. through the shared-reference check it could keep an old host image alive that nothing visible used anymore, and the entries themselves accumulated without bound. prune sources against the visible set at the start of every encode pass. refs #947 * docs: link issue and upstream discussion for vendor patch 0002 the transmit time accessor now has a removal path: the ghostty discussion proposing it upstream is linked in the patch ledger. refs #947 --------- Co-authored-by: Can Celik --- src/kitty_graphics.rs | 269 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 262 insertions(+), 7 deletions(-) (limited to 'src/kitty_graphics.rs') diff --git a/src/kitty_graphics.rs b/src/kitty_graphics.rs index 4564bde6..2e83841c 100644 --- a/src/kitty_graphics.rs +++ b/src/kitty_graphics.rs @@ -120,6 +120,8 @@ struct ClippedPlacement { pub(crate) struct HostGraphicsCache { images: HashMap, placements: HashMap<(u32, u32), PlacementSignature>, + /// Host image currently backing each (pane, source image id) pair. + sources: HashMap<(PaneId, u32), u32>, view: Option, } @@ -204,6 +206,7 @@ pub(crate) fn encode_local_pane_graphics( view_changed, &mut cache.images, &mut cache.placements, + &mut cache.sources, ); tracing::debug!( placements = placements.len(), @@ -267,7 +270,16 @@ fn encode_graphics_update( view_changed: bool, host_images: &mut HashMap, host_placements: &mut HashMap<(u32, u32), PlacementSignature>, + sources: &mut HashMap<(PaneId, u32), u32>, ) { + // Prune sources that are no longer visible: a stale entry would keep its + // old host image referenced and block the superseded-image delete. + let current_sources: HashSet<(PaneId, u32)> = placements + .iter() + .map(|placement| (placement.pane_id, placement.placement.image_id)) + .collect(); + sources.retain(|source, _| current_sources.contains(source)); + let mut current_placements = HashSet::new(); for placement in placements { let clipped = clipped_placement(placement); @@ -318,6 +330,16 @@ fn encode_graphics_update( } } + release_superseded_source_image( + bytes, + sources, + host_images, + host_placements, + &mut current_placements, + (placement.pane_id, placement.placement.image_id), + host_id, + ); + // A different view can repaint the same cells with text or overlays and // leave the host-side Kitty placement state out of sync with this cache. // Re-emit the placement even when its geometry signature is unchanged. @@ -359,6 +381,36 @@ fn encode_graphics_update( } } +/// Records that `source` is now backed by `host_id` and deletes the host +/// image it previously pointed at once no other source references it. +fn release_superseded_source_image( + bytes: &mut Vec, + sources: &mut HashMap<(PaneId, u32), u32>, + host_images: &mut HashMap, + host_placements: &mut HashMap<(u32, u32), PlacementSignature>, + current_placements: &mut HashSet<(u32, u32)>, + source: (PaneId, u32), + host_id: u32, +) { + let Some(previous) = sources.insert(source, host_id) else { + return; + }; + if previous == host_id || sources.values().any(|id| *id == previous) { + return; + } + encode_delete_image(bytes, previous); + host_images.remove(&previous); + // The `d=I` delete also removes the image's placements host-side. + host_placements.retain(|(image_id, placement_id), _| { + if *image_id == previous { + current_placements.remove(&(*image_id, *placement_id)); + false + } else { + true + } + }); +} + pub(crate) fn clear_all_host_graphics() -> io::Result<()> { let cache = LOCAL_HOST_GRAPHICS.get_or_init(|| Mutex::new(HostGraphicsCache::default())); let mut bytes = Vec::new(); @@ -399,6 +451,7 @@ impl HostGraphicsCache { } self.images.clear(); self.placements.clear(); + self.sources.clear(); self.view = None; bytes } @@ -865,6 +918,7 @@ mod tests { fn graphics_update_uploads_once_then_repositions_only() { let mut images = HashMap::new(); let mut placements = HashMap::new(); + let mut sources = HashMap::new(); let mut bytes = Vec::new(); let placement = test_placement(0, 0); @@ -874,6 +928,7 @@ mod tests { false, &mut images, &mut placements, + &mut sources, ); let first = String::from_utf8_lossy(&bytes); assert!(first.contains("a=t")); @@ -881,7 +936,14 @@ mod tests { bytes.clear(); let same = test_placement(0, 0); - encode_graphics_update(&mut bytes, &[same], false, &mut images, &mut placements); + encode_graphics_update( + &mut bytes, + &[same], + false, + &mut images, + &mut placements, + &mut sources, + ); assert!(bytes.is_empty()); let mut z_changed = test_placement(0, 0); @@ -892,6 +954,7 @@ mod tests { false, &mut images, &mut placements, + &mut sources, ); let z_changed_bytes = String::from_utf8_lossy(&bytes); assert!(!z_changed_bytes.contains("a=t")); @@ -899,7 +962,14 @@ mod tests { bytes.clear(); let moved = test_placement(0, 1); - encode_graphics_update(&mut bytes, &[moved], false, &mut images, &mut placements); + encode_graphics_update( + &mut bytes, + &[moved], + false, + &mut images, + &mut placements, + &mut sources, + ); let moved_bytes = String::from_utf8_lossy(&bytes); assert!(!moved_bytes.contains("a=t")); assert!(moved_bytes.contains("a=p")); @@ -909,6 +979,7 @@ mod tests { fn view_change_redisplays_unchanged_visible_placement() { let mut images = HashMap::new(); let mut placements = HashMap::new(); + let mut sources = HashMap::new(); let mut bytes = Vec::new(); let placement = test_placement(0, 0); @@ -918,12 +989,20 @@ mod tests { false, &mut images, &mut placements, + &mut sources, ); assert_eq!(placements.len(), 1); bytes.clear(); let same = test_placement(0, 0); - encode_graphics_update(&mut bytes, &[same], true, &mut images, &mut placements); + encode_graphics_update( + &mut bytes, + &[same], + true, + &mut images, + &mut placements, + &mut sources, + ); let redisplay = String::from_utf8_lossy(&bytes); assert!(!redisplay.contains("a=t")); assert!(redisplay.contains("a=p")); @@ -942,6 +1021,7 @@ mod tests { false, &mut cache.images, &mut cache.placements, + &mut cache.sources, ); assert_eq!(cache.images.len(), 1); assert_eq!(cache.placements.len(), 1); @@ -954,6 +1034,7 @@ mod tests { false, &mut cache.images, &mut cache.placements, + &mut cache.sources, ); let redisplay = String::from_utf8_lossy(&bytes); @@ -968,6 +1049,7 @@ mod tests { fn scrollback_offset_change_redisplays_placement() { let mut images = HashMap::new(); let mut placements = HashMap::new(); + let mut sources = HashMap::new(); let mut bytes = Vec::new(); let placement = test_placement(0, 0); @@ -977,12 +1059,20 @@ mod tests { false, &mut images, &mut placements, + &mut sources, ); bytes.clear(); let mut scrolled = test_placement(0, 0); scrolled.scrollback_offset = 3; - encode_graphics_update(&mut bytes, &[scrolled], false, &mut images, &mut placements); + encode_graphics_update( + &mut bytes, + &[scrolled], + false, + &mut images, + &mut placements, + &mut sources, + ); let redisplay = String::from_utf8_lossy(&bytes); assert!(!redisplay.contains("a=t")); assert!(redisplay.contains("a=p")); @@ -992,6 +1082,7 @@ mod tests { fn empty_image_data_does_not_mark_image_uploaded() { let mut images = HashMap::new(); let mut placements = HashMap::new(); + let mut sources = HashMap::new(); let mut bytes = Vec::new(); let mut placement = test_placement(0, 0); placement.placement.data.clear(); @@ -1002,6 +1093,7 @@ mod tests { false, &mut images, &mut placements, + &mut sources, ); assert!(bytes.is_empty()); @@ -1013,10 +1105,18 @@ mod tests { fn same_image_signature_reuses_host_upload_across_source_image_ids() { let mut images = HashMap::new(); let mut placements = HashMap::new(); + let mut sources = HashMap::new(); let mut bytes = Vec::new(); let first = test_placement(0, 0); - encode_graphics_update(&mut bytes, &[first], false, &mut images, &mut placements); + encode_graphics_update( + &mut bytes, + &[first], + false, + &mut images, + &mut placements, + &mut sources, + ); assert_eq!(images.len(), 1); assert_eq!(placements.len(), 1); @@ -1031,6 +1131,7 @@ mod tests { false, &mut images, &mut placements, + &mut sources, ); let reused = String::from_utf8_lossy(&bytes); @@ -1040,10 +1141,147 @@ mod tests { assert_eq!(placements.len(), 1); } + #[test] + fn replaced_image_content_deletes_superseded_host_image() { + let mut images = HashMap::new(); + let mut placements = HashMap::new(); + let mut sources = HashMap::new(); + let mut bytes = Vec::new(); + let first = test_placement(0, 0); + + encode_graphics_update( + &mut bytes, + &[first], + false, + &mut images, + &mut placements, + &mut sources, + ); + assert_eq!(images.len(), 1); + let superseded_host_id = *images.keys().next().expect("uploaded host image"); + + // Same source image id, new pixel content: the fresh content maps to + // a fresh host image id, so the replaced one must be deleted. + bytes.clear(); + let mut changed = test_placement(0, 0); + changed.placement.data_fingerprint = 43; + encode_graphics_update( + &mut bytes, + &[changed], + false, + &mut images, + &mut placements, + &mut sources, + ); + + let update = String::from_utf8_lossy(&bytes); + assert!(update.contains("a=t"), "changed content re-uploads"); + assert!( + update.contains(&format!("a=d,d=I,i={superseded_host_id}")), + "superseded host image is deleted" + ); + assert_eq!(images.len(), 1); + assert_eq!(placements.len(), 1); + } + + #[test] + fn shared_host_image_survives_while_another_source_references_it() { + fn twin_placement() -> HostPlacement { + let mut twin = test_placement(5, 5); + twin.placement.image_id = 8; + twin.placement.placement_id = 4; + twin + } + + let mut images = HashMap::new(); + let mut placements = HashMap::new(); + let mut sources = HashMap::new(); + let mut bytes = Vec::new(); + + encode_graphics_update( + &mut bytes, + &[test_placement(0, 0), twin_placement()], + false, + &mut images, + &mut placements, + &mut sources, + ); + assert_eq!(images.len(), 1, "same content dedups to one host image"); + + // One source moves to new content while the other still shows the + // old image: the shared host image must survive. + bytes.clear(); + let mut changed = test_placement(0, 0); + changed.placement.data_fingerprint = 43; + encode_graphics_update( + &mut bytes, + &[changed, twin_placement()], + false, + &mut images, + &mut placements, + &mut sources, + ); + + let update = String::from_utf8_lossy(&bytes); + assert!(!update.contains("a=d,d=I"), "shared host image survives"); + assert_eq!(images.len(), 2); + } + + #[test] + fn stale_source_entry_does_not_block_superseded_image_delete() { + fn twin_placement() -> HostPlacement { + let mut twin = test_placement(5, 5); + twin.placement.image_id = 8; + twin.placement.placement_id = 4; + twin + } + + let mut images = HashMap::new(); + let mut placements = HashMap::new(); + let mut sources = HashMap::new(); + let mut bytes = Vec::new(); + + encode_graphics_update( + &mut bytes, + &[test_placement(0, 0), twin_placement()], + false, + &mut images, + &mut placements, + &mut sources, + ); + assert_eq!(images.len(), 1); + assert_eq!(sources.len(), 2); + let shared_host_id = *images.keys().next().expect("uploaded host image"); + + // The twin source is gone and the survivor changed content: the + // vanished source's stale entry must not keep the old host image + // alive. + bytes.clear(); + let mut changed = test_placement(0, 0); + changed.placement.data_fingerprint = 43; + encode_graphics_update( + &mut bytes, + &[changed], + false, + &mut images, + &mut placements, + &mut sources, + ); + + let update = String::from_utf8_lossy(&bytes); + assert!( + update.contains(&format!("a=d,d=I,i={shared_host_id}")), + "old host image is deleted once its last live source moves on" + ); + assert_eq!(images.len(), 1); + assert_eq!(sources.len(), 1); + } + #[test] fn stale_placement_deletes_placement_not_image_immediately() { let mut images = HashMap::new(); let mut placements = HashMap::new(); + let mut sources = HashMap::new(); let mut bytes = Vec::new(); let placement = test_placement(0, 0); @@ -1053,11 +1291,19 @@ mod tests { false, &mut images, &mut placements, + &mut sources, ); assert_eq!(placements.len(), 1); bytes.clear(); - encode_graphics_update(&mut bytes, &[], false, &mut images, &mut placements); + encode_graphics_update( + &mut bytes, + &[], + false, + &mut images, + &mut placements, + &mut sources, + ); let delete = String::from_utf8_lossy(&bytes); assert!(delete.contains("a=d,d=i")); assert!(!delete.contains("d=I")); @@ -1069,6 +1315,7 @@ mod tests { fn view_change_deletes_stale_placement_immediately() { let mut images = HashMap::new(); let mut placements = HashMap::new(); + let mut sources = HashMap::new(); let mut bytes = Vec::new(); let placement = test_placement(0, 0); @@ -1078,9 +1325,17 @@ mod tests { false, &mut images, &mut placements, + &mut sources, ); bytes.clear(); - encode_graphics_update(&mut bytes, &[], true, &mut images, &mut placements); + encode_graphics_update( + &mut bytes, + &[], + true, + &mut images, + &mut placements, + &mut sources, + ); let delete = String::from_utf8_lossy(&bytes); assert!(delete.contains("a=d,d=i")); -- cgit v1.2.3