Skip to content

fix(graphics): scope image store keys per terminal#1752

Open
cantona wants to merge 1 commit into
raphamorim:mainfrom
cantona:fix/multi-tab-image-key
Open

fix(graphics): scope image store keys per terminal#1752
cantona wants to merge 1 commit into
raphamorim:mainfrom
cantona:fix/multi-tab-image-key

Conversation

@cantona

@cantona cantona commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

fix(graphics): scope image store keys per terminal

Problem

The window-level image stores — Sugarloaf::image_data (CPU pixels) and
the renderer's per-image GPU texture cache — are keyed by a bare numeric
id:

  • kitty images by their protocol image_id verbatim (kitty_image_key)
  • atlas graphics (sixel/iTerm2) by their GraphicId shifted above the
    u32 range (atlas_image_key)

But one window hosts many terminals (tabs and splits) that all share
these stores, and both id spaces restart per terminal: a kitty
client in each tab numbers its images from 1, and each terminal's atlas
GraphicIds start over too.

So two tabs displaying images collide:

  • Tab B transmits kitty image id 1 → overwrites Tab A's image id 1 in
    the shared store. Switch back to Tab A and its image is now B's.
  • A removal built from a bare id frees the wrong tab's texture.
  • A kitty id and an atlas id can also alias each other within a tab
    under some numbering, since the "disjoint namespace" was only a
    numeric shift.

To reproduce: open two tabs, icat/imgcat a different image in each,
then switch between them — the images bleed across tabs.

Fix

Replace the flat u64 key with a composite key:

pub struct ImageKey {
    pub owner: usize,      // originating route id (per terminal)
    pub source: ImageSource, // Atlas | Kitty
    pub id: u64,
}
  • owner scopes every entry to the terminal that produced it, so tabs
    no longer share id space.
  • source distinguishes the atlas and kitty id spaces, so disjointness
    comes from the discriminant rather than a numeric shift — atlas keys
    now carry the raw GraphicId with no 1 << 32 offset.

The backend removal queue changes from Vec<u64> to
Vec<GraphicRemoval> (Atlas(GraphicId) | Kitty(u32)), because the
backend has no notion of the frontend's route id. The frontend, which
owns the route id, builds the matching ImageKey when draining the
queue.

Removals are now applied before inserts in the frontend handler, so
an evict-then-retransmit of the same id landing in one batch keeps the
freshly transmitted pixels instead of dropping them.

Scope

Pure re-keying of the shared image stores and the removal queue. No
change to byte accounting, eviction policy, or the render loop.

Testing

  • cargo fmt --all -- --check — clean
  • cargo clippy -p rioterm --all-features — no new warnings
  • cargo test --features wgpu — all suites pass (0 failed)
  • New unit test image_keys_discriminate_source_and_owner pins that the
    same protocol id from two owners / two sources maps to distinct keys,
    and that identity holds within one owner+source.
  • Existing atlas/overlay/eviction tests updated for the new key type.

The window-level image stores (Sugarloaf::image_data and the renderer's
per-image GPU texture cache) were keyed by a bare numeric id: kitty
images by their protocol id verbatim, atlas graphics (sixel/iTerm2) by
their GraphicId shifted above the u32 range. One window hosts many
terminals (tabs and splits) that all share these stores, but both id
spaces restart per terminal -- a kitty client in each tab numbers its
images from 1, and each terminal's atlas GraphicIds start over too. So
two tabs displaying images collided: tab B's image id 1 overwrote tab
A's, and removals could free the wrong tab's texture.

Replace the flat u64 key with ImageKey { owner, source, id }, where
owner is the originating route id and source distinguishes the atlas
and kitty id spaces. Disjointness now comes from the source
discriminant rather than a numeric shift, so atlas keys carry the raw
GraphicId. The removal queue changes from Vec<u64> to
Vec<GraphicRemoval> (Atlas(GraphicId) | Kitty(u32)) so the frontend,
which owns the route id, builds the matching key; removals are applied
before inserts so an evict-then-retransmit of the same id in one batch
keeps the fresh pixels.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant