fix(graphics): repair chafa rendering and mpv kitty streaming - #8
Merged
Conversation
Four related bugs surfaced while testing `chafa -f sixel`, `chafa -f kitty`, and `mpv --vo=kitty` on bcon: 1. Kitty graphics: decode chunked base64 per chunk chafa emits each chunk as a self-contained base64 unit (often ending in `=` padding). bcon used to concatenate raw base64 across chunks and skip `=` during decode, which leaked the trailing 2 bits of each chunk's final group into the next chunk and produced 600 extra bytes for a 640x480 RGBA image (2400 chunks x 2 bits). The decoder now decodes each chunk on arrival and accumulates the decoded bytes. Added a regression test reproducing chafa-style chunking. 2. Kitty graphics: gate responses on client-provided image id The protocol says terminals must not emit responses unless the client identified the command with `i=` or `I=`. chafa sets neither, so bcon's "OK" reply was leaking into the shell input as visible text (`^[_Gi=1;OK^[\` after every image). Introduced `Terminal::maybe_send_kitty_response` to gate all seven response sites in one place. 3. Grid: keep the cursor out of placed image area `place_image` advanced the cursor to the last row occupied by the image (clamped to `rows - 1` in both the scroll and non-scroll paths). The very next `put_char` on that row then triggered `remove_images_at_cell`, which discarded the placement we had just created — observed as the second `chafa -f sixel` run rendering nothing at all. Unified the branch so the cursor always lands one row past the bottom of the image, scrolling additionally if needed. 4. Grid: dedupe fully-covered overlay placements on replacement `mpv --vo=kitty` streams identically sized overlay frames (a=T,C=1) at the same cell position without ever issuing a delete. bcon accumulated one placement and one 3MB image per frame, stalling within ~4 seconds because the PTY read loop could not drain fast enough. `place_image` now returns the ids of any overlay placements fully covered by the new one (same z-index) and the Kitty handler drops those ids from the image registry so GPU textures and RGBA buffers are reclaimed every frame. Also marks sixel-decoded images with `dirty_image_ids.push(img_id)` so the renderer uploads the new texture — previously the sixel decode completed but no GPU upload ever ran, leaving a black rect. Known remaining issue: `mpv --vo=sixel` streaming still breaks the DCS parser mid-playback (sixel data leaks as printable text); static sixel and Kitty graphics video are unaffected. Tracking separately. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
vte 0.13's DCS state machine intermittently fails to enter DcsPassthrough under streaming sixel load (mpv --vo=sixel), causing entire frames of sixel data to leak through as printed text and fill the screen with garbage characters. Root cause: despite clean PTY data (no stray control bytes, verified via raw PTY dump), vte occasionally processes ESC P q sequences in Ground state instead of transitioning to DcsPassthrough. The exact trigger is unclear but appears related to read-boundary timing under high throughput. Fix: handle DCS sixel entirely in bcon's own state machine (extending the existing APC handler) instead of relying on vte. When ESC P is detected, the new DcsParams/DcsData/DcsEscape states route sixel bytes directly to SixelDecoder.push() without creating a Performer or touching vte. Non-sixel DCS sequences (DECRQSS, XTGETTCAP) are replayed to vte for standard handling. Additional fixes in this commit: - Sixel images placed at z=-1 so they survive text overwrites from mpv's interleaved status-line writes (remove_images_at_cell skips z<0 placements, matching xterm/foot behavior) - Non-overlay placement dedup by (z, width, height) to prevent accumulation under streaming; reuses the first frame's position so cursor drift from status text doesn't shift later frames Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
mpv slightly varies frame dimensions between sixel frames (e.g. 96x36 vs 99x36), so the exact size match prevented position reuse and let cursor-polluted positions through. Match by z-index alone for non-overlay placements. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sixel DCS started/complete, place_image, Kitty APC/decode, and texture upload logs fire every frame during video streaming. Under RUST_LOG=info these add measurable write(2) overhead per frame. Move to trace! so they only appear with RUST_LOG=trace. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reduces read(2) syscall count by 16x during streaming sixel/kitty video. A 315KB sixel frame previously needed ~77 reads; now ~5. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When in DcsData state, bypass the outer match/Option check per byte and loop directly feeding bytes to SixelDecoder.push(). Only break out when ESC or 0x9C is found. This eliminates ~315K match + enum pattern-match operations per sixel frame. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
draw_sixel: single ensure_size per RLE run instead of per pixel, skip pattern==0 (no-op), direct pixel buffer indexing without set_pixel function call overhead. finish (RGBA conversion): pre-build 256-entry RGBA LUT, then direct slice write instead of per-pixel extend_from_slice. Eliminates Vec capacity checks and branch per pixel. These two paths dominate CPU time during sixel video streaming (17s user vs kitty's 5s for same content). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
push, handle_normal, handle_color, handle_rle, handle_raster_attr, draw_sixel, ensure_size — all marked #[inline(always)]. Called 315K+ times per frame, function call overhead dominates. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The z-index-only dedup with position reuse was designed for mpv sixel streaming (z=-1) where cursor drift makes positions unreliable. But it also fired for z=0 Kitty images (yazi), causing old images to reuse stale positions and persist on screen when navigating to different files. Split the logic: z<0 uses aggressive dedup with position reuse (sixel video), z>=0 uses conservative fully-covered dedup without position reuse (Kitty/static sixel). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The aggressive z<0 dedup removed ALL z=-1 placements on every new sixel image, breaking consecutive `chafa -f sixel` and `img2sixel` calls: the second image superseded the first instead of stacking below it. Now only supersede placements whose row ranges actually overlap with the new image — mpv's same-position frames still dedup, but vertically stacked images coexist. Added test: sixel_consecutive_images_stack_vertically. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
yazi uses sixel (not Kitty) for image preview. With z=-1, text writes couldn't remove the image (remove_images_at_cell skips z<0), so images persisted when navigating to text files. Changed sixel back to z=0 for both parser.rs and mod.rs paths. mpv --vo=sixel works with --really-quiet (no status text to trigger removal). Updated tests and removed debug logs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
New features: - mpv --vo=kitty video playback at near-realtime speed - mpv --vo=sixel video playback support - Streaming sixel/kitty frame deduplication and position tracking Bug fixes: - Kitty graphics: decode chunked base64 per chunk (fixes chafa) - Kitty graphics: gate responses on client-provided image id - Sixel: bypass vte DCS parser for reliable streaming - Grid: cursor lands below placed images, not inside - Grid: overlay dedup for mpv --vo=kitty streaming - Grid: sixel dedup with row-overlap check Performance: - PTY read buffer 4KB → 256KB - Sixel decoder: single ensure_size per RLE run, RGBA LUT - Per-frame logs downgraded to trace level - Tight inner loop for DCS sixel data (no Performer overhead) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Four graphics-protocol bugs found while debugging issue #7 (mplayer fbdev2 playback) by exercising
chafa -f sixel,chafa -f kitty, andmpv --vo=kittyon bcon.=padding. bcon was concatenating raw base64 across chunks and skipping=during decode, which leaked 2 leftover bits per chunk into the next, producing 600 extra bytes for a 640×480 RGBA image (2400 chunks × 2 bits). Now each chunk is decoded on arrival. Regression test added.i=orI=. chafa sets neither, so bcon'sOKreply was leaking into the shell input as visible text. IntroducedTerminal::maybe_send_kitty_responseto collapse seven nearly-identical call sites into one gated helper.place_imageadvanced the cursor onto the last row occupied by the image (both the scroll and non-scroll paths clamped torows - 1). The very nextput_charthen calledremove_images_at_cell, discarding the placement we had just created — observed as the secondchafa -f sixelrun rendering nothing. Unified the branch so the cursor always lands one row past the image, scrolling additionally if needed.mpv --vo=kittystreams identically sized overlay frames (a=T,C=1) at the same cell without ever issuing a delete. bcon accumulated one placement and one ~3MB image per frame, stalling within ~4 seconds because the PTY read loop couldn't drain.place_imagenow returns the ids of any overlay placements fully covered by the new one at the same z-index, and the Kitty handler drops those ids from the image registry so GPU textures and RGBA buffers are reclaimed every frame.Also marks sixel-decoded images with
dirty_image_ids.push(img_id)so the renderer actually uploads the new texture — previously the sixel decode completed but no GPU upload ever ran, so the placement rendered as a solid black rectangle.Test plan
cargo test --release terminal::kitty— new chunked-base64 regression test passeschafa -f kitty test.pngdisplays the image with noOKreply leaking into the shellchafa -f sixel test.pngdisplays the image; running it a second time still displaysmpv --no-audio --vo=kitty test.mp4plays to completion without stalling (throughput is still heavy at full resolution; lower with--vf=scale=480:-2,fps=15for smoother playback)mpv --no-audio --vo=sixel test.mp4— still broken (DCS parser leaks sixel bytes as printable text mid-stream). Static sixel and Kitty graphics video are unaffected. Tracked as follow-up.Known follow-ups (not in this PR)
mpv --vo=sixelbreaks the DCS parser mid-playbackrgb_to_rgba, faster base64, shared-memoryt=spath) to make full-resolution video smoother