Repository navigation
Add external Metal presenter ABI - #122
Conversation
📝 WalkthroughWalkthroughThe PR adds callback-driven external Metal presentation, PTY tee configuration and output sequencing, configuration serialization, and CLI action parsing to the public and embedded APIs. ChangesExternal Metal presentation
PTY output interception and sequencing
Configuration serialization
Binding CLI parsing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Host
participant EmbeddedSurface
participant Metal
participant IOSurface
participant PresentCallback
Host->>EmbeddedSurface: configure metal_external
EmbeddedSurface->>Metal: initialize external presenter
Metal->>IOSurface: render frame and obtain dimensions
Metal->>PresentCallback: deliver IOSurface frame
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/input/Binding.zig (1)
1327-1338: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct tests for
Action.parseCLI.The new bridge should test missing input (
null→Error.InvalidFormat), comma-containing parameters such aswrite_screen_file:copy,vt, and successful cloning into the supplied allocator. The existing tests exercise the underlying parser but not this new API boundary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/input/Binding.zig` around lines 1327 - 1338, Add focused tests for Action.parseCLI covering null input returning Error.InvalidFormat, comma-containing actions such as write_screen_file:copy,vt parsing successfully, and successful cloning into the provided allocator. Verify the cloned action is usable and owned by the supplied allocator, while leaving the existing parser tests unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@include/ghostty.h`:
- Around line 476-479: Version the public surface configuration ABI rather than
expanding it in place: in include/ghostty.h lines 476-479, keep the platform
payload pointer-sized or introduce a versioned configuration type; in
include/ghostty.h lines 531-534, move the appended PTY fields into a size-gated
extension or versioned options struct; mirror that representation in
src/apprt/embedded.zig lines 397-414; and update the surface_new_ parsing path
at src/apprt/embedded.zig lines 589-595 to read only fields covered by the
caller-provided size or version.
In `@src/renderer/Metal.zig`:
- Around line 291-297: Update the .external surface-size handling in Metal
renderer state to avoid directly reading external.surface.getSize(), since
Surface.size is concurrently updated by the embedder. Reuse the existing
synchronized renderer mailbox/state or atomic resize snapshot to obtain the
latest width and height, preserving the current size return structure.
In `@src/termio/Termio.zig`:
- Around line 79-88: Synchronize access to pty_tee_cb and pty_tee_userdata
across the compatibility setter and processOutput, preventing races and
mismatched callback/userdata pairs. Prefer routing setter updates through the IO
thread mailbox, or otherwise protect both fields as one atomic pair with a
shared lock; update the relevant setter and processOutput paths while preserving
callback execution behavior.
---
Nitpick comments:
In `@src/input/Binding.zig`:
- Around line 1327-1338: Add focused tests for Action.parseCLI covering null
input returning Error.InvalidFormat, comma-containing actions such as
write_screen_file:copy,vt parsing successfully, and successful cloning into the
provided allocator. Verify the cloned action is usable and owned by the supplied
allocator, while leaving the existing parser tests unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d000984-1ee7-497d-866b-1419eb2c4cb4
📒 Files selected for processing (8)
include/ghostty.hsrc/Surface.zigsrc/apprt/embedded.zigsrc/config/CApi.zigsrc/input/Binding.zigsrc/renderer/Metal.zigsrc/termio/Options.zigsrc/termio/Termio.zig
| typedef union { | ||
| ghostty_platform_macos_s macos; | ||
| ghostty_platform_ios_s ios; | ||
| ghostty_platform_metal_external_s metal_external; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Version the public surface configuration instead of changing its layout in place.
The union expansion shifts existing fields, and the appended PTY fields increase the allocation size; surface_new_ then copies the complete new struct from callers that may still provide the old ABI layout.
include/ghostty.h#L476-L479: keep the platform payload pointer-sized or introduce a versioned configuration type.include/ghostty.h#L531-L534: move these fields into a size-gated extension or versioned options struct.src/apprt/embedded.zig#L397-L414: mirror the pointer-sized/versioned C representation.src/apprt/embedded.zig#L589-L595: parse only fields guaranteed by the caller-provided size/version.
📍 Affects 2 files
include/ghostty.h#L476-L479(this comment)include/ghostty.h#L531-L534src/apprt/embedded.zig#L397-L414src/apprt/embedded.zig#L589-L595
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@include/ghostty.h` around lines 476 - 479, Version the public surface
configuration ABI rather than expanding it in place: in include/ghostty.h lines
476-479, keep the platform payload pointer-sized or introduce a versioned
configuration type; in include/ghostty.h lines 531-534, move the appended PTY
fields into a size-gated extension or versioned options struct; mirror that
representation in src/apprt/embedded.zig lines 397-414; and update the
surface_new_ parsing path at src/apprt/embedded.zig lines 589-595 to read only
fields covered by the caller-provided size or version.
| .external => |external| external_size: { | ||
| const value = try external.surface.getSize(); | ||
| break :external_size .{ | ||
| .width = value.width, | ||
| .height = value.height, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Publish external surface dimensions through synchronized renderer state.
external.surface.getSize() reads Surface.size on the renderer thread while Surface.updateSize writes it from the embedder thread without synchronization. Publish resize values through the renderer mailbox/state or an atomic snapshot rather than reading the runtime struct directly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/Metal.zig` around lines 291 - 297, Update the .external
surface-size handling in Metal renderer state to avoid directly reading
external.surface.getSize(), since Surface.size is concurrently updated by the
embedder. Reuse the existing synchronized renderer mailbox/state or atomic
resize snapshot to obtain the latest width and height, preserving the current
size return structure.
| /// thread. The embedded runtime installs initial values before starting that | ||
| /// thread; the compatibility setter may replace them after surface creation. | ||
| pty_tee_cb: ?PtyTeeCallback = null, | ||
| pty_tee_userdata: ?*anyopaque = null, | ||
|
|
||
| /// Number of PTY-output bytes fully applied to terminal state. This is read | ||
| /// and updated only while renderer_state.mutex is held. Addition wraps so the | ||
| /// value remains a stable modulo-u64 stream position across long sessions. | ||
| processed_output_bytes: u64 = 0, | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Data race hazard and torn read risk on pty_tee_cb and pty_tee_userdata.
The comment indicates that a compatibility setter may replace these fields after surface creation. However, processOutput reads both fields on the IO read thread without synchronization (before locking the renderer mutex).
If the setter mutates these fields from a different thread while the IO thread is running, this causes a data race. Furthermore, because the callback and the userdata are separate fields, concurrent modification could lead to a torn read where the new callback is invoked with the old userdata, likely causing a crash.
Consider one of the following fixes:
- Require the compatibility setter to dispatch the update via the IO thread's mailbox (so mutation and execution happen on the same thread).
- Protect these fields with an
std.Thread.RwLockor a dedicated mutex. - Bundle the callback and userdata into a single struct and update it using an atomic pointer exchange (if supported by the allocator pattern) or a lock.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/termio/Termio.zig` around lines 79 - 88, Synchronize access to pty_tee_cb
and pty_tee_userdata across the compatibility setter and processOutput,
preventing races and mismatched callback/userdata pairs. Prefer routing setter
updates through the IO thread mailbox, or otherwise protect both fields as one
atomic pair with a shared lock; update the relevant setter and processOutput
paths while preserving callback execution behavior.
Greptile SummaryThis PR adds a Metal "external presenter" platform mode that lets an out-of-process embedder (e.g. cmux) receive rendered IOSurfaces via a callback rather than owning a window or CALayer, along with three companion features: a pre-surface-init PTY tee to prevent missed startup bytes, a
Confidence Score: 4/5Safe to merge; the core renderer, tee, and sequence-counter paths are well-tested and the mutex discipline is consistent throughout. The Metal external presenter refactoring is clean and the threading contract holds for all realistic call paths. Two non-blocking issues remain: the C header for ghostty_config_serialize does not document the unconditional command-palette-entry = clear prefix the implementation emits, and the paired ptyTeeCallback/ptyTeeUserdata accessors use independent @hasDecl checks that could silently produce a mismatched pair. include/ghostty.h (ghostty_config_serialize doc) and src/Surface.zig (paired hasDecl guards) Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Embedder as Embedder (cmux)
participant CAPI as libghostty C API
participant Surface as Surface.zig
participant Termio as Termio.zig
participant Metal as Metal.zig (Renderer)
participant MTL as Metal GPU
Embedder->>CAPI: ghostty_surface_new(config) pty_tee_cb set in config
CAPI->>Surface: init() opts.pty_tee_cb Termio.init
Surface->>Termio: IO thread starts with tee pre-installed
Termio-->>Embedder: pty_tee_cb(bytes) before mutex raw PTY bytes
Note over Metal,MTL: External presenter path
Metal->>MTL: beginFrame commit command buffer
MTL-->>Metal: addCompletedHandler fires completion thread
Metal->>Metal: "present(target, sync=false)"
Metal->>Embedder: present_callback(userdata, iosurface, w, h) borrowed IOSurfaceRef
Note over CAPI,Termio: Atomic VT tail + sequence snapshot
Embedder->>CAPI: ghostty_surface_read_screen_tail_vt_with_output_sequence
CAPI->>CAPI: acquire renderer_state.mutex
CAPI->>CAPI: readScreenTailVTLocked format terminal
CAPI->>Termio: read processed_output_bytes mutex held
CAPI-->>Embedder: returns vt_text next_sequence
Note over CAPI: Config serialization
Embedder->>CAPI: ghostty_config_serialize(config)
CAPI->>CAPI: "prepend command-palette-entry = clear"
CAPI->>CAPI: FileFormatter.format() all effective values
CAPI-->>Embedder: ghostty_string_s caller frees with ghostty_string_free
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Embedder as Embedder (cmux)
participant CAPI as libghostty C API
participant Surface as Surface.zig
participant Termio as Termio.zig
participant Metal as Metal.zig (Renderer)
participant MTL as Metal GPU
Embedder->>CAPI: ghostty_surface_new(config) pty_tee_cb set in config
CAPI->>Surface: init() opts.pty_tee_cb Termio.init
Surface->>Termio: IO thread starts with tee pre-installed
Termio-->>Embedder: pty_tee_cb(bytes) before mutex raw PTY bytes
Note over Metal,MTL: External presenter path
Metal->>MTL: beginFrame commit command buffer
MTL-->>Metal: addCompletedHandler fires completion thread
Metal->>Metal: "present(target, sync=false)"
Metal->>Embedder: present_callback(userdata, iosurface, w, h) borrowed IOSurfaceRef
Note over CAPI,Termio: Atomic VT tail + sequence snapshot
Embedder->>CAPI: ghostty_surface_read_screen_tail_vt_with_output_sequence
CAPI->>CAPI: acquire renderer_state.mutex
CAPI->>CAPI: readScreenTailVTLocked format terminal
CAPI->>Termio: read processed_output_bytes mutex held
CAPI-->>Embedder: returns vt_text next_sequence
Note over CAPI: Config serialization
Embedder->>CAPI: ghostty_config_serialize(config)
CAPI->>CAPI: "prepend command-palette-entry = clear"
CAPI->>CAPI: FileFormatter.format() all effective values
CAPI-->>Embedder: ghostty_string_s caller frees with ghostty_string_free
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| GHOSTTY_API ghostty_config_t ghostty_config_new(); | ||
| GHOSTTY_API void ghostty_config_free(ghostty_config_t); | ||
| GHOSTTY_API ghostty_config_t ghostty_config_clone(ghostty_config_t); | ||
| // Serialize the effective configuration as valid Ghostty config-file syntax. |
There was a problem hiding this comment.
The header doc says the returned bytes contain "all public values", but serializeConfig always prepends command-palette-entry = clear\n before the formatter runs. An embedder that pipes this output to a config diff tool or displays it as the "effective configuration" will see an unexpected directive at byte 0 that is not a public value. The source-level comment in CApi.zig explains the reason (clearing append-accumulation before re-emitting), but the C header—the contract for embedders—should make this explicit so callers are not surprised.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
2 participants
Summary
Verification
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Add a Metal external-presenter mode that renders to IOSurfaces without owning a window and delivers completed frames via a callback. Also adds config serialization and an atomic VT+output-sequence snapshot for reliable out-of-process worker recovery, plus an initial PTY tee to avoid losing startup bytes.
New Features
GHOSTTY_PLATFORM_METAL_EXTERNALwithghostty_platform_metal_external_sandghostty_metal_external_present_cbfor IOSurface presentation after GPU completion.NSView/UIView); size comes from the runtime surface; present path invokes the embedder callback.ghostty_surface_config_s(pty_tee_cb,pty_tee_userdata); setter still supported.ghostty_config_serialize(returns config-file syntax).ghostty_surface_read_screen_tail_vt_with_output_sequence; sequence advances only after bytes are applied and wraps onu64.Bug Fixes
Binding.Action.parseCLI(e.g.write_screen_file:copy,vt).Written for commit 0f400d0. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements