terminal: add retainable Kitty image payloads#13285
Conversation
mitchellh
left a comment
There was a problem hiding this comment.
I know its a draft but just marking this red too so it doesn't get merged.
Good ideas, but I think we should clean it up in various ways.
I don't have time for a full full review so high level: I think Image having data and ImagePayload also existing is a red flag. If we're going to introduce ref-counted images (a good thing!) I think we should just own it all the way and make that the primary interface to images.
The C API: We can just break compatibility at the moment, so we should move to a full ref counted/retained model.
The renderer: Generic having a render thread is a red flag, but haven't dove into that deeper. As I mentioned in Discord, would be curious if we can update RenderState to do something like an options struct to note what features we support (so a renderer can opt out of images if it wants) and then store the ref counted images directly on it some how. Since we have to traverse the images/placements in the lock anyways it'd be better to just store it on render state.
|
I agree, these also introduced retry machinery which I don't think belongs in this PR. I'll do some cleanup and see if we can get this closer. |
0212b71 to
86c3ef6
Compare
Asynchronous image consumers cannot safely keep borrowed Kitty image data after terminal storage is unlocked. Replacement, deletion, eviction, or shutdown may otherwise free the pixels. Make Image itself the immutable, atomically reference-counted owner of its metadata and pixels. Store image pointers directly and release the storage reference on every removal path, while retained references remain valid independently. Reserve fallible insertion resources before eviction so failed additions preserve caller and storage ownership. Keep ordinary lookups and the existing C image lookup borrowed in this commit.
Borrowed Kitty image handles cannot safely cross into asynchronous render or raster work because terminal mutation may invalidate them. Make GhosttyKittyGraphicsImage an owned reference to Image itself. Image lookup now retains the handle, retain increments the same reference count, and free releases it without a wrapper allocation or caller-provided allocator. Keep image metadata and pixel getters read-only, with pixel pointers valid for the lifetime of their image handle.
86c3ef6 to
472367c
Compare
Kitty image preparation copied terminal-owned pixels and converted them while the renderer state mutex excluded terminal parsing. Large images and video frames therefore extended the critical section with a full copy and format conversion. Add an opt-in Kitty graphics snapshot to RenderState. Retain images and copy placement geometry while the terminal is locked, then update renderer image state under the draw mutex after releasing the terminal lock. Only placed images are prepared for upload. Track retained and renderer-owned pending data explicitly, and release the retained image after synchronous texture creation consumes it. Failed uploads remain pending for a later natural draw without a retry scheduler.
472367c to
2a4db8d
Compare
Kitty image lookups currently return storage-owned pixel data. The data is
only valid until the next terminal mutation, so asynchronous consumers must
copy every image generation while holding the terminal state lock.
This is particularly costly for video workloads. The renderer copies the
entire frame and performs pixel conversion while terminal parsing is excluded,
then uploads the copied data after releasing the lock.
This series gives completed Kitty images an immutable, atomically
reference-counted payload. Consumers can retain a specific
(image_id, generation)while terminal state is locked and safely use those bytes afterunlocking, even if storage replaces, deletes, evicts, or shuts down the image.
The first patch adds the ownership primitive and generation-checked retain
operation to terminal storage. The second exposes it through an additive C
API while leaving the existing borrowed API and ABI unchanged. The final
patch converts the in-tree renderer to retain Kitty payloads under the state
lock, then convert and upload them after unlocking. Generic and overlay
images continue to use renderer-owned storage.
Upload-failure paths release retained data and re-retain the exact generation
on a later update. Retry wakeups are coalesced so a persistent GPU failure
cannot create a hot loop.
The series is split as follows:
terminal: add retainable Kitty image payloadsterminal: expose retained Kitty images through C APIrenderer: retain kitty images for uploadPerformance
I compared patch 2, where the ref-counted storage and C API are present but
the renderer still copies, against patch 3. This isolates the renderer
handoff from the prerequisite storage changes.
The workload was five interleaved runs per revision of
notcurses-demo -c xin a 1920x1080@60 Hz GPU-backed headless Sway session.Both revisions were ReleaseFast builds. The compositor and Ghostty used the
Mesa
irisdriver on Intel Arc 130V/140V graphics.perfsampled userspaceCPU at 499 Hz, hardware counters ran for 17 seconds, and RSS was sampled every
20 ms.
memcpysample sharememcpysample shareThe I/O reader's unrelated
memcpyshare remained effectively unchanged(17.0% before, 17.7% after). The renderer's median share fell from 6.25% to
0.18%, which attributes the whole-process reduction to the copy removed by
this series. Both binaries had the same roughly 224 MiB idle RSS floor; the
14 MiB difference appeared during the Kitty workload.
Visible throughput was noisy because unrelated build, browser, and agent
processes were active. Frames visible at approximately 14 seconds were:
The paired median was four more displayed frames and six fewer dropped frames
with retained payloads, but later runs degraded sharply. I do not consider
that sufficient evidence of a user-visible throughput change. The copy,
memory, and process-counter reductions were repeatable across the series.
Testing
zig build test --summary all: 3,036 passed, 33 skippedAI Disclosure: GPT 5.6 did the implementation, Fable 5 reviewed. A few minor findings which I (the human) disagreed with regarding retries.