Conversation
📝 WalkthroughWalkthroughThis PR introduces a complete input forwarding subsystem that enables SDL input event forwarding into captured applications via a nested XWayland server. It adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant IF as InputForwarder
participant XWS as XWaylandServer
participant Comp as Compositor Thread
participant Wayland as Wayland Display
participant XWayland as XWayland
Note over App,XWayland: Initialization Phase
App->>IF: create()
activate IF
IF->>XWS: start()
activate XWS
XWS->>Wayland: wl_display_create()
XWS->>Wayland: wlr_backend_create(headless)
XWS->>Wayland: bind_socket(DISPLAY:1-9)
XWS->>XWayland: wlr_xwayland_create()
XWS->>Comp: spawn compositor thread
activate Comp
Comp->>Wayland: drive event loop
XWS-->>IF: return DISPLAY number
deactivate XWS
IF->>IF: XOpenDisplay(DISPLAY:N)
IF-->>App: return InputForwarder
deactivate IF
Note over App,XWayland: Runtime Event Forwarding Phase
App->>App: SDL event received
App->>IF: forward_key(SDL_KeyboardEvent)
activate IF
IF->>IF: translate SDL scancode → Linux keycode
IF->>IF: convert Linux keycode → X11 keycode
IF->>XWayland: XTest keyboard event
IF->>XWayland: XSync(display)
IF-->>App: Result<void>
deactivate IF
XWayland->>Wayland: deliver to target app
Note over App,XWayland: Shutdown Phase
App->>IF: ~InputForwarder()
activate IF
IF->>IF: XCloseDisplay()
IF->>XWS: stop()
activate XWS
XWS->>Wayland: wl_display_terminate()
XWS->>Comp: join compositor thread
deactivate Comp
Comp-->>XWS: thread exits
XWS->>XWayland: destroy
XWS->>Wayland: wlr_* cleanup
deactivate XWS
deactivate IF
sequenceDiagram
participant SDL as SDL Event Loop
participant Main as main.cpp
participant IF as InputForwarder
participant X11 as X11/XTest
participant XWL as XWayland
participant App as Target App
Note over SDL,App: Mouse Button Event Flow
SDL->>SDL: SDL_MOUSEBUTTONDOWN
SDL->>Main: poll event
Main->>Main: check input_forwarder != nullptr
rect rgb(200, 220, 240)
Note over Main,IF: Event Forwarding
Main->>IF: forward_mouse_button(SDL_MouseButtonEvent)
activate IF
IF->>IF: translate button & state
IF->>X11: XTestFakeButtonEvent(button, is_pressed)
IF->>X11: XSync(display)
IF-->>Main: Result::ok()
deactivate IF
end
Main->>Main: log if error
rect rgb(240, 200, 200)
Note over X11,App: X11 Delivery
X11->>XWL: synthetic button event
XWL->>App: deliver to window
App->>App: handle button press
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The review requires careful attention to wlroots integration (compositor lifecycle, resource cleanup order, thread safety), X11/XTest interaction (keycode translation, display management), SDL-to-X11 event mapping, and integration points in the main event loop. Multiple files with interconnected responsibilities and some unfamiliar domain-specific code (wlroots API) increase complexity despite clean organization. Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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: 0
🧹 Nitpick comments (4)
docs/inputtino_evaluation.md (2)
90-90: Add language identifiers to fenced code blocks and format bare URLs.For markdown best practices:
- Line 90: Format the bare URL as
[games-on-whales/inputtino](https://github.com/games-on-whales/inputtino)- Lines 23, 48, 129, 169, 328: Add language specifiers to code blocks (e.g.,
```shell,```cpp,```json)These improve readability and syntax highlighting in rendered markdown.
Also applies to: 23-23, 48-48, 129-129, 169-169, 328-328
125-125: Consider using markdown headings for verdict/verdict-like sections instead of bold emphasis.Lines 125 and 162 use
**Verdict: ...**formatting. For consistency with the rest of the document structure and better markdown semantics, consider promoting these to heading level (e.g.,#### Verdict: Fully Agnostic). This improves document navigation and outline generation.Also applies to: 162-162
openspec/changes/add-input-forwarding/proposal.md (2)
25-25: Format the bare URL as a markdown link.Line 25 should be:
[Inputtino repository](https://github.com/games-on-whales/inputtino)This improves markdown rendering and follows best practices used elsewhere in the PR documentation.
7-20: Consider more explicit specification of spec deltas.The proposal lists code/module additions but could be clearer about the associated spec changes. The "What Changes" section could explicitly reference the new spec file or structure entries to indicate both code and spec changes:
For example:
## What Changes **Spec:** - **ADDED:** New `input-forwarding` capability specification (see `specs/input-forwarding/spec.md`) **Code & Dependencies:** - **ADDED:** New `src/input/` module for input handling - ... (etc)This makes it explicit to readers that both specs and code are being added, which helps with compliance tracking and proposal comprehensiveness.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/inputtino_evaluation.mdopenspec/changes/add-input-forwarding/design.mdopenspec/changes/add-input-forwarding/proposal.mdopenspec/changes/add-input-forwarding/specs/input-forwarding/spec.mdopenspec/changes/add-input-forwarding/tasks.md
🧰 Additional context used
📓 Path-based instructions (3)
openspec/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/**/*.md: Use#### Scenario:format (4 hashtags) for scenario headers in requirements, not bullets or bold text, with at least one scenario per requirement
Use format- **WHEN** [condition] - **THEN** [expected result]for scenario steps in requirements
Files:
openspec/changes/add-input-forwarding/tasks.mdopenspec/changes/add-input-forwarding/proposal.mdopenspec/changes/add-input-forwarding/design.mdopenspec/changes/add-input-forwarding/specs/input-forwarding/spec.md
openspec/changes/**/proposal.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
List spec deltas in
proposal.mdunder 'What Changes' section, marking breaking changes with BREAKING
Files:
openspec/changes/add-input-forwarding/proposal.md
openspec/changes/**/specs/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/changes/**/specs/**/*.md: Write spec deltas using## ADDED|MODIFIED|REMOVED|RENAMED Requirementsformat with at least one#### Scenario:per requirement in spec files
When modifying existing requirements in a MODIFIED delta, paste the full requirement block including the header and all scenarios, as the archiver will replace the entire requirement
Use ADDED for new capabilities that can stand alone; use MODIFIED for changes to existing requirement behavior, scope, or acceptance criteria; use RENAMED for name-only changes
Files:
openspec/changes/add-input-forwarding/specs/input-forwarding/spec.md
🧠 Learnings (3)
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Applies to openspec/changes/**/specs/**/*.md : Use ADDED for new capabilities that can stand alone; use MODIFIED for changes to existing requirement behavior, scope, or acceptance criteria; use RENAMED for name-only changes
Applied to files:
openspec/changes/add-input-forwarding/tasks.mdopenspec/changes/add-input-forwarding/specs/input-forwarding/spec.md
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Read `proposal.md`, `design.md` (if exists), and `tasks.md` in order, then implement tasks sequentially before updating checklist status to completed
Applied to files:
openspec/changes/add-input-forwarding/tasks.md
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Create a change proposal with `proposal.md`, `tasks.md`, and optional `design.md` under `openspec/changes/<change-id>/` before implementing changes to add features, make breaking changes, or update architecture
Applied to files:
openspec/changes/add-input-forwarding/tasks.md
🪛 LanguageTool
docs/inputtino_evaluation.md
[grammar] ~1-~1: Ensure spelling is correct
Context: # Inputtino Evaluation for Goggles Input Forwarding...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~7-~7: Ensure spelling is correct
Context: ...adding input forwarding to Goggles. Its uinput-based approach provides true desktop en...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~81-~81: Ensure spelling is correct
Context: ... display server (X11/Wayland). --- ## Inputtino Overview ### What is Inputtino? Input...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~111-~111: Ensure spelling is correct
Context: ...yleft restrictions. Compatible with any Goggles licensing model. ### Production Usage ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~320-~320: Ensure spelling is correct
Context: ...sing a tested library Verdict: Use inputtino rather than reinventing the wheel. ---...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~363-~363: Consider an alternative for the overused word “exactly”.
Context: ...erialize, inject via inputtino This is exactly how Wolf/Sunshine work today. ### Inpu...
(EXACTLY_PRECISELY)
[grammar] ~413-~413: Ensure spelling is correct
Context: ...omplex deployments --- ## Conclusion Inputtino is the right choice for Goggles input f...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
openspec/changes/add-input-forwarding/design.md
[grammar] ~83-~83: Use a hyphen to join words.
Context: ...v translation ``` ### Decision 4: Error handling strategy What: Return `tl:...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
docs/inputtino_evaluation.md
23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
48-48: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
90-90: Bare URL used
(MD034, no-bare-urls)
125-125: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
129-129: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
162-162: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
169-169: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
328-328: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
openspec/changes/add-input-forwarding/proposal.md
25-25: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (5)
openspec/changes/add-input-forwarding/specs/input-forwarding/spec.md (1)
1-102: Spec format and structure look excellent.The specification correctly follows all coding guidelines:
## ADDED Requirementsheader,#### Scenario:sections (4 hashtags), and proper- **WHEN** [condition] - **THEN** [result]format throughout. Requirements are clear, testable, and cover success, failure, and focus-based behavior scenarios comprehensively.docs/inputtino_evaluation.md (1)
1-425: Comprehensive and well-structured evaluation document.The evaluation provides excellent context for the inputtino integration decision, covering technical fit, license compatibility, privilege models, and risk assessment. Architecture diagrams are particularly helpful for understanding the kernel-level injection approach. The comparison with alternatives (libei, ydotool, xdotool) clearly justifies the choice. This document should serve well as reference for future maintainers.
openspec/changes/add-input-forwarding/tasks.md (1)
1-47: Task checklist is well-organized and aligned with design decisions.The 26 tasks are sequenced logically from dependencies through integration to documentation and testing. Tasks directly map to the architecture decisions in design.md (inputtino integration, input module structure, SDL event flow) and requirements in spec.md (configuration, privilege management, focus-based behavior). The optional deferral of gamepad support (task 2.5) appropriately reflects design non-goals. This serves as a solid project plan for implementation.
openspec/changes/add-input-forwarding/proposal.md (1)
1-25: Proposal clearly articulates motivation, scope, and impact.The proposal effectively communicates the gap (lack of input forwarding) and the solution scope. The "Impact" section is well-organized with clear sub-sections for affected specs, code, external dependencies, and privilege requirements. The references to evaluation documentation and inputtino repository are helpful for decision justification.
openspec/changes/add-input-forwarding/design.md (1)
1-158: Excellent architectural decision documentation.The design document provides clear context, well-reasoned decisions with alternatives considered, and appropriate risk/mitigation analysis. The five key decisions are well-justified and the code examples (especially the error handling pattern) serve as good implementation guidance. The migration plan provides a clear incremental path with rollback strategy. The open questions demonstrate thoughtful consideration of scope boundaries.
This design effectively bridges from the evaluation (why inputtino) to the specification (what requirements) and tasks (how to implement).
K1ngst0m
force-pushed
the
dev/kingstom/add-input-forwarding
branch
3 times, most recently
from
fbf469c to
3b7d231
Compare
K1ngst0m
changed the title
K1ngst0m
changed the title
K1ngst0m
force-pushed
the
dev/kingstom/add-input-forwarding
branch
from
3b7d231 to
ada473a
Compare
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
✅ Actions performedSequence diagram generated. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/util/error.hpp (1)
30-31: Stray semicolon after enum closing brace.There's an extra semicolon after the enum definition that should be removed.
}; -;
🧹 Nitpick comments (14)
openspec/changes/add-input-forwarding/proposal.md (2)
1-120: Missing 'What Changes' section per coding guidelines.As per the coding guidelines for
openspec/changes/**/proposal.mdfiles, proposals should include a "What Changes" section listing spec deltas, with breaking changes marked with BREAKING. This document is missing that section.Consider adding a section like:
## What Changes - **NEW**: `goggles::input::InputForwarder` class with `init()`, `forward_key()`, `display_number()` APIs - **NEW**: `goggles::input::XWaylandServer` class with `start()` and `stop()` APIs - **NEW**: `CaptureMessageType::config_request` (6) and `input_display_ready` (7) enum values - **NEW**: `CaptureConfigRequest` and `CaptureInputDisplayReady` structs (16-byte wire format) - **NEW**: `layer_early_init()` constructor function for early DISPLAY setup - **NEW**: `CaptureReceiver::set_input_display(int)` and `handle_config_request(int)` methods
20-20: Consider adding language identifier to fenced code block.The static analysis tool flagged this code block as missing a language identifier. For ASCII art diagrams, you can use
textor leave it empty if the tool is overly pedantic for this use case.-``` +```textsrc/capture/vk_layer/vk_capture.cpp (2)
28-46: Socket connection logic duplicatesLayerSocketClient::connect().This code duplicates the socket creation and connection logic from
LayerSocketClient::connect()(as shown in the relevant code snippets). Consider extracting a shared helper or adding a blocking connect mode toLayerSocketClientto reduce duplication.That said, the use of a blocking socket (no
SOCK_NONBLOCK) for the config handshake is intentional and appropriate here since we need synchronous communication before the app starts.
60-70: 100ms timeout is reasonable but consider making it configurable.The 100ms timeout should be sufficient for local IPC. However, on heavily loaded systems, this might occasionally be too short. Consider allowing configuration via an environment variable for debugging purposes, or at minimum document this timeout in the design docs.
src/capture/vk_layer/ipc_socket.hpp (1)
27-29: Consider handlingEINTRand partial reads.The implementation has two potential edge cases:
EINTR handling:
poll()can return -1 witherrno == EINTRif interrupted by a signal. This should be retried.Partial reads: If
recv()returns a value between 0 andsizeof(T), the message is lost and the socket state becomes inconsistent. For fixed-size IPC messages over a local Unix socket, this is unlikely but not impossible under heavy load.🔎 Suggested improvement for robustness
template<typename T> bool LayerSocketClient::receive_with_timeout(T& msg, int timeout_ms) { std::lock_guard lock(mutex_); if (socket_fd_ < 0) { return false; } // Use poll() to wait for data with timeout pollfd pfd{}; pfd.fd = socket_fd_; pfd.events = POLLIN; - int poll_result = poll(&pfd, 1, timeout_ms); - if (poll_result <= 0) { - // Timeout or error - return false; + int poll_result; + do { + poll_result = poll(&pfd, 1, timeout_ms); + } while (poll_result < 0 && errno == EINTR); + + if (poll_result <= 0) { + return false; } // Data available, read it ssize_t received = recv(socket_fd_, &msg, sizeof(T), MSG_DONTWAIT); return received == sizeof(T); }Also applies to: 40-63
tests/CMakeLists.txt (1)
86-104: Missing link togoggles_inputlibrary.The test is named
goggles_input_testbut doesn't link againstgoggles_input. Looking at the test source, it currently only exercises SDL/Vulkan initialization without actually testing theInputForwarder. If the intent is to eventually test input forwarding, consider linking the library now.Also, consider adding
target_compile_features(goggles_input_test PRIVATE cxx_std_20)for consistency with the main test target.🔎 Proposed fix
target_link_libraries(goggles_input_test PRIVATE SDL3::SDL3-static Vulkan::Vulkan goggles_util + goggles_input ) + +target_compile_features(goggles_input_test PRIVATE cxx_std_20)src/capture/capture_receiver.cpp (1)
231-258: Missing version validation.The summary mentions validating the version, but the code only validates the message type. Consider adding version validation to ensure protocol compatibility.
🔎 Proposed fix
if (request.type != CaptureMessageType::config_request) { GOGGLES_LOG_WARN("Config request: unexpected message type"); return; } + if (request.version != CAPTURE_PROTOCOL_VERSION) { + GOGGLES_LOG_WARN("Config request: version mismatch (got {}, expected {})", + request.version, CAPTURE_PROTOCOL_VERSION); + return; + } + // Send response with input display numberopenspec/changes/add-input-forwarding/specs/input-forwarding/spec.md (1)
78-82: Clarify return value fordisplay_number()edge cases.The scenario states
display_number()"SHALL return a positive integer N" after successful init, but it's worth clarifying what happens ifinit()wasn't called or failed. Based on the implementation (m_display_numberstarts at -1), consider adding this clarification.- **WHEN** `InputForwarder::init()` succeeds - **THEN** `InputForwarder::display_number()` SHALL return a positive integer N - **AND** callers MAY pass N to other subsystems (e.g. capture config handshakes) +- **WHEN** `InputForwarder::init()` was not called or failed +- **THEN** `InputForwarder::display_number()` SHALL return -1src/input/xwayland_server.cpp (2)
89-89: Return value fromwlr_xdg_shell_createis ignored.The XDG shell creation could fail (return NULL), but the result isn't checked. While this may be non-critical for XWayland operation, it's good practice to validate.
🔎 Proposed fix
- wlr_xdg_shell_create(m_display, 3); + if (!wlr_xdg_shell_create(m_display, 3)) { + GOGGLES_LOG_WARN("Failed to create XDG shell (non-fatal for XWayland)"); + }
46-168: Consider reducing cleanup code repetition.The repeated cleanup blocks on each failure path are correct but verbose. A scope guard or RAII wrapper could simplify this while maintaining safety.
This is optional since the current explicit cleanup is clear and correct. If you'd like to reduce repetition, a scope guard pattern could help:
auto cleanup_on_failure = [&](bool success) { if (success) return; // cleanup code here };docs/input_forwarding.md (2)
19-49: Add language specifier to fenced code block.Per the static analysis hint, fenced code blocks should have a language specified. For ASCII diagrams, use
textor```(blank) to satisfy the linter.-``` +```text ┌────────────────────────────────────────────────────────────┐
157-177: Add language specifier to remaining diagram blocks.Lines 157, 165, and 173 have the same issue. Add
textlanguage specifier for consistency.src/input/xwayland_server.hpp (1)
32-32: Consider thread safety for display_number() accessor.The
display_number()accessor readsm_display_numberwithout synchronization. Ifstart()modifies this value on the compositor thread while the main thread reads it, there could be a data race. Consider usingstd::atomic<int>form_display_numberor documenting thatdisplay_number()should only be called afterstart()completes.🔎 Proposed fix using atomic
private: struct wl_display* m_display = nullptr; struct wl_event_loop* m_event_loop = nullptr; struct wlr_backend* m_backend = nullptr; struct wlr_renderer* m_renderer = nullptr; struct wlr_allocator* m_allocator = nullptr; struct wlr_compositor* m_compositor = nullptr; struct wlr_seat* m_seat = nullptr; struct wlr_xwayland* m_xwayland = nullptr; std::jthread m_compositor_thread; - int m_display_number = -1; + std::atomic<int> m_display_number = -1; };Also applies to: 44-44
openspec/changes/add-input-forwarding/design.md (1)
120-120: Optional: Use markdown headings instead of bold for subsections.Lines 120 and 127 use bold emphasis for "Phase 1" and "Phase 2" subsection headers. Consider using proper markdown heading syntax (
####or#####) for better document structure and accessibility.🔎 Proposed fix
-**Phase 1: Config Handshake (Layer Constructor)** +#### Phase 1: Config Handshake (Layer Constructor) 1. Layer constructor connects to socket 2. Sends `config_request` message 3. Goggles responds with `input_display_ready` containing DISPLAY number 4. Layer sets `DISPLAY=:N` environment variable 5. Layer closes config connection -**Phase 2: Frame Capture (on_present)** +#### Phase 2: Frame Capture (on_present) 6. Layer reconnects on first `vkQueuePresentKHR`Also applies to: 127-127
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
cmake/Dependencies.cmakedocs/input_forwarding.mdopenspec/changes/add-input-forwarding/design.mdopenspec/changes/add-input-forwarding/proposal.mdopenspec/changes/add-input-forwarding/specs/input-forwarding/spec.mdopenspec/changes/add-input-forwarding/specs/vk-layer-capture/spec.mdopenspec/changes/add-input-forwarding/tasks.mdsrc/CMakeLists.txtsrc/app/CMakeLists.txtsrc/app/main.cppsrc/capture/capture_protocol.hppsrc/capture/capture_receiver.cppsrc/capture/capture_receiver.hppsrc/capture/vk_layer/ipc_socket.hppsrc/capture/vk_layer/vk_capture.cppsrc/input/CMakeLists.txtsrc/input/input_forwarder.cppsrc/input/input_forwarder.hppsrc/input/xwayland_server.cppsrc/input/xwayland_server.hppsrc/util/error.hpptests/CMakeLists.txttests/input/goggles_input_test.cpp
✅ Files skipped from review due to trivial changes (1)
- cmake/Dependencies.cmake
🚧 Files skipped from review as they are similar to previous changes (1)
- openspec/changes/add-input-forwarding/tasks.md
🧰 Additional context used
📓 Path-based instructions (3)
openspec/changes/**/specs/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/changes/**/specs/**/*.md: Write spec deltas using## ADDED|MODIFIED|REMOVED|RENAMED Requirementsformat with at least one#### Scenario:per requirement in spec files
When modifying existing requirements in a MODIFIED delta, paste the full requirement block including the header and all scenarios, as the archiver will replace the entire requirement
Use ADDED for new capabilities that can stand alone; use MODIFIED for changes to existing requirement behavior, scope, or acceptance criteria; use RENAMED for name-only changes
Files:
openspec/changes/add-input-forwarding/specs/input-forwarding/spec.mdopenspec/changes/add-input-forwarding/specs/vk-layer-capture/spec.md
openspec/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/**/*.md: Use#### Scenario:format (4 hashtags) for scenario headers in requirements, not bullets or bold text, with at least one scenario per requirement
Use format- **WHEN** [condition] - **THEN** [expected result]for scenario steps in requirements
Files:
openspec/changes/add-input-forwarding/specs/input-forwarding/spec.mdopenspec/changes/add-input-forwarding/specs/vk-layer-capture/spec.mdopenspec/changes/add-input-forwarding/design.mdopenspec/changes/add-input-forwarding/proposal.md
openspec/changes/**/proposal.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
List spec deltas in
proposal.mdunder 'What Changes' section, marking breaking changes with BREAKING
Files:
openspec/changes/add-input-forwarding/proposal.md
🧬 Code graph analysis (8)
src/capture/capture_receiver.cpp (2)
src/capture/capture_receiver.hpp (1)
fd(42-42)src/util/unique_fd.hpp (1)
fd(34-34)
src/input/input_forwarder.hpp (2)
src/input/input_forwarder.cpp (2)
InputForwarder(91-91)InputForwarder(93-93)src/input/xwayland_server.hpp (1)
nodiscard(32-32)
src/input/xwayland_server.hpp (2)
src/input/xwayland_server.cpp (2)
XWaylandServer(27-27)XWaylandServer(29-31)src/util/error.hpp (2)
nodiscard(47-51)nodiscard(53-89)
src/input/xwayland_server.cpp (3)
src/input/xwayland_server.hpp (4)
XWaylandServer(22-22)XWaylandServer(23-23)XWaylandServer(25-25)XWaylandServer(27-27)src/util/error.hpp (1)
make_error(47-49)src/capture/capture_receiver.hpp (2)
display_num(36-36)display_num(36-36)
src/input/input_forwarder.cpp (2)
src/input/input_forwarder.hpp (8)
InputForwarder(11-11)InputForwarder(12-12)InputForwarder(14-14)InputForwarder(16-16)event(20-20)event(21-21)event(22-22)event(23-23)src/util/error.hpp (1)
make_error(47-49)
src/capture/vk_layer/vk_capture.cpp (1)
src/capture/vk_layer/ipc_socket.cpp (2)
connect(25-78)connect(25-25)
src/app/main.cpp (2)
src/input/input_forwarder.hpp (4)
event(20-20)event(21-21)event(22-22)event(23-23)src/capture/capture_receiver.hpp (2)
display_num(36-36)display_num(36-36)
tests/input/goggles_input_test.cpp (1)
src/input/input_forwarder.hpp (4)
event(20-20)event(21-21)event(22-22)event(23-23)
🪛 markdownlint-cli2 (0.18.1)
docs/input_forwarding.md
19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
157-157: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
165-165: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
173-173: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
openspec/changes/add-input-forwarding/design.md
11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
120-120: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
178-178: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
openspec/changes/add-input-forwarding/proposal.md
20-20: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (37)
src/CMakeLists.txt (1)
6-6: LGTM!The addition of the
inputsubdirectory follows the existing pattern and is appropriately placed.src/app/CMakeLists.txt (1)
15-15: LGTM!The
goggles_inputlibrary is correctly linked to the main application target.src/util/error.hpp (1)
26-27: LGTM!The new error codes
input_init_failedandinput_socket_send_failedare appropriately added and follow the existing naming conventions. The correspondingerror_code_nameswitch cases are correctly implemented.Also applies to: 79-82
src/capture/capture_protocol.hpp (2)
18-20: LGTM on protocol enum additions.The new
config_requestandinput_display_readymessage types are appropriately added with clear documentation about reserved values for PR #16.
52-66: LGTM on new wire-format structures.The
CaptureConfigRequestandCaptureInputDisplayReadystructures are well-designed with:
- Default initializers for safety
- Reserved arrays for forward compatibility
- Static assertions ensuring 16-byte wire format stability
src/capture/vk_layer/vk_capture.cpp (3)
17-22: Good use of constructor priority for early initialization.Using
__attribute__((constructor(101)))ensures this runs before the application'smain(), allowing the DISPLAY environment variable to be set early. The logging of the original DISPLAY value is helpful for debugging.
72-85: LGTM on response handling.Good validation of the response:
- Checks received bytes match expected size
- Validates message type matches expected
input_display_ready- Falls back gracefully to original DISPLAY on any failure
87-101: LGTM on DISPLAY environment variable handling.The validation of
display_number >= 0before callingsetenvis correct. The buffer size of 32 bytes is more than sufficient for the:Nformat even with large display numbers.src/capture/vk_layer/ipc_socket.hpp (1)
6-8: LGTM on header includes.The additional system headers are necessary for the
poll()andrecv()functionality in the template method.src/capture/capture_receiver.hpp (3)
35-36: LGTM onset_input_displayAPI.The inline setter is clean and appropriate for configuring the input display number before clients connect.
42-42: LGTM onhandle_config_requestdeclaration.The method signature appropriately takes the file descriptor for the config connection to send the response.
46-46: LGTM on member variable.Using
-1as the default/sentinel value for an unset display number is appropriate and consistent with how DISPLAY numbers work (valid values are non-negative).tests/input/goggles_input_test.cpp (1)
164-182: LGTM!The main function correctly initializes Vulkan first to trigger layer loading before SDL, then logs the DISPLAY value to verify the layer's effect. Good test structure for manual validation of input forwarding.
src/capture/capture_receiver.cpp (1)
100-109: LGTM!Good approach using
MSG_PEEKto differentiate config requests from normal capture connections without consuming the data. The early detection allows proper routing of different connection types.openspec/changes/add-input-forwarding/specs/input-forwarding/spec.md (1)
1-200: LGTM!The specification is well-structured with clear requirements and scenarios following the prescribed format. Good coverage of the XWayland lifecycle, input forwarding, error handling, and integration patterns.
src/input/CMakeLists.txt (1)
1-30: LGTM!The CMake configuration properly separates public and private dependencies, keeping wlroots/Wayland/X11 details private to support the PIMPL pattern in the public API. The
WLR_USE_UNSTABLEdefine is required for accessing wlroots APIs.src/input/xwayland_server.cpp (1)
177-226: LGTM!The shutdown sequence correctly terminates the display loop, joins the compositor thread before destroying resources, and follows the proper destruction order to prevent use-after-free. Good RAII-style cleanup with clear comments explaining the constraints.
docs/input_forwarding.md (1)
108-114: Documentation outdated regarding mouse support.The limitations state "Keyboard only: Mouse forwarding not yet implemented" and "Mouse forwarding" as a future enhancement, but the
InputForwarderAPI includesforward_mouse_button(),forward_mouse_motion(), andforward_mouse_wheel()methods. Consider updating this section to reflect the current implementation.src/input/input_forwarder.hpp (1)
1-31: LGTM!Clean PIMPL implementation that correctly hides wlroots/X11 details from the public header. The
[[nodiscard]]attributes ensure callers handleResultreturn values, and deleted copy/move operations prevent accidental ownership issues with the underlying X11 connection and XWayland server.src/input/xwayland_server.hpp (1)
1-33: LGTM! Clean header design.The forward declarations, API structure, and copy/move semantics are well-designed. The NOLINT suppressions are appropriate for external wlroots types.
src/input/input_forwarder.cpp (8)
14-75: Approve keycode translation logic with note on coverage.The SDL to Linux to X11 keycode translation is correct for the covered keys. Returning 0 for unmapped keys is handled properly in
forward_key(). Note that this covers basic alphanumeric and modifier keys but excludes function keys, numpad, and other extended keys—acceptable for an MVP.
80-89: LGTM! Proper RAII cleanup.The
Impldestructor correctly closes the X11 display before theXWaylandServerdestructor runs. The cleanup order is appropriate.
151-167: Coordinate mapping TODO is documented.The TODO at Lines 159-162 correctly identifies the missing coordinate mapping. This aligns with the PR's documented non-goal of accurate mouse coordinate mapping. The current implementation forwards raw SDL coordinates, which will be incorrect when viewer and target window sizes differ.
169-194: Mouse wheel implementation is correct.The wheel-to-button mapping (4=up, 5=down, 6=left, 7=right) follows standard X11 protocol, and the press-release sequence is appropriate for wheel events.
196-198: Thread safety depends on XWaylandServer.This accessor delegates to
XWaylandServer::display_number(). Ensure the thread safety concern raised for that method is addressed.
95-115: Code is correct; TODO comment is stale.The init logic is correct with proper error handling and cleanup. The TODO on line 112 about sending DISPLAY to the layer via IPC is misleading—this functionality is already implemented. The DISPLAY number is transmitted to the layer through the config handshake: the receiver stores it via
set_input_display(), then sends it back in theCaptureInputDisplayReadyresponse message when the layer initiates theconfig_request. Consider removing or updating the TODO comment since Task 16 is complete.
117-134: No action needed. SDL3'sSDL_KeyboardEventstructure includes abool downfield, which is correctly used at line 128. The codeevent.down ? True : Falseis valid SDL3 API usage.Likely an incorrect or invalid review comment.
136-149: No issue. SDL3SDL_MouseButtonEventhas adownfield (typebool), and line 143 correctly usesevent.down ? True : Falseto convert it to X11'sBooltype.src/app/main.cpp (2)
197-214: LGTM! Proper graceful degradation.The InputForwarder initialization correctly implements graceful degradation: on failure, logs a warning and continues without input forwarding. On success, integrates with the capture receiver and enables forwarding. The stack allocation with pointer approach is clean.
35-66: Event forwarding integration is correct.The main loop properly checks for input_forwarder availability before forwarding and logs errors without disrupting the render loop. All major event types (keyboard, mouse button, motion, wheel) are covered.
openspec/changes/add-input-forwarding/specs/vk-layer-capture/spec.md (3)
5-40: LGTM! Requirement format and content are correct.The Config Handshake Protocol requirement properly uses the OpenSpec format with
#### Scenario:headers andWHEN/THEN/ANDstructure. The scenarios are clear, testable, and align with the implementation design.
42-59: Protocol message types are well-specified.The requirement properly documents the new message types and their structure. The use of GIVEN for preconditions (Lines 47, 52, 57) is acceptable, though WHEN is preferred per coding guidelines—this is a minor style point.
61-76: CaptureReceiver requirements are clear and testable.The config handling scenarios properly specify the two-phase protocol and connection lifecycle. The requirements align with the design documentation.
openspec/changes/add-input-forwarding/design.md (4)
1-51: LGTM! Overview and Layer 1-2 documentation is accurate.The architectural overview and InputForwarder module description accurately reflect the implementation in
src/input/input_forwarder.hppand.cpp.
53-199: Architecture layers and data flow documentation is accurate.Layers 3-5 descriptions match the implementation in
xwayland_server.hpp, the protocol specs inspec.md, and the expected layer integration. The data flow trace and latency estimate are technically sound.
215-318: Error handling and resource management patterns are sound.The documented error handling (graceful degradation, error propagation) and RAII patterns match the actual implementation in
input_forwarder.cppand align with best practices.
319-384: Performance and dependency documentation looks reasonable.The performance estimates (memory, CPU, latency), testing strategy, and dependency justification appear sound based on the architecture described.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
src/capture/vk_layer/vk_capture.cpp (1)
24-25: Unused variable still present.The
LayerSocketClient config_socketis declared but never used; the socket operations are performed manually with raw file descriptors. This was flagged in a previous review.src/capture/capture_protocol.hpp (1)
21-22: Stray semicolon already flagged in previous review.There's an extra semicolon after the enum closing brace that should be removed.
tests/input/goggles_input_test.cpp (2)
8-23: Defaulted move operations cause double-free risk.The defaulted move constructor and assignment operator do not transfer ownership properly. After a move, both the source and destination objects will have the same
handlevalue, leading to double-free when both destructors run.🔎 Proposed fix
- VulkanInstance(VulkanInstance&&) = default; - VulkanInstance& operator=(VulkanInstance&&) = default; + VulkanInstance(VulkanInstance&& other) noexcept : handle(other.handle) { + other.handle = VK_NULL_HANDLE; + } + VulkanInstance& operator=(VulkanInstance&& other) noexcept { + if (this != &other) { + if (handle != VK_NULL_HANDLE) { + vkDestroyInstance(handle, nullptr); + } + handle = other.handle; + other.handle = VK_NULL_HANDLE; + } + return *this; + }
25-45: Same double-free risk in SDLContext, plus unconditional SDL_Quit.The defaulted move operations cause the same double-free issue as
VulkanInstance. Additionally,SDL_Quit()is called unconditionally in the destructor, which is problematic ifSDL_Initfailed or the object was moved-from.🔎 Proposed fix
+ bool initialized = false; + SDLContext() = default; ~SDLContext() { if (renderer) { SDL_DestroyRenderer(renderer); } if (window) { SDL_DestroyWindow(window); } - SDL_Quit(); + if (initialized) { + SDL_Quit(); + } } SDLContext(const SDLContext&) = delete; SDLContext& operator=(const SDLContext&) = delete; - SDLContext(SDLContext&&) = default; - SDLContext& operator=(SDLContext&&) = default; + SDLContext(SDLContext&& other) noexcept + : window(other.window), renderer(other.renderer), initialized(other.initialized) { + other.window = nullptr; + other.renderer = nullptr; + other.initialized = false; + } + SDLContext& operator=(SDLContext&& other) noexcept { + if (this != &other) { + if (renderer) SDL_DestroyRenderer(renderer); + if (window) SDL_DestroyWindow(window); + if (initialized) SDL_Quit(); + window = other.window; + renderer = other.renderer; + initialized = other.initialized; + other.window = nullptr; + other.renderer = nullptr; + other.initialized = false; + } + return *this; + }Then set
initialized = trueafterSDL_Initsucceeds ininit_sdl()(line 76).
🧹 Nitpick comments (3)
openspec/changes/add-input-forwarding/proposal.md (1)
1-120: Missing "What Changes" section per coding guidelines.The proposal document should include a "What Changes" section listing spec deltas, with any breaking changes marked with BREAKING. This helps reviewers understand which specifications are affected by this proposal.
Consider adding a section like:
## What Changes - **NEW** `openspec/specs/input-forwarding/` - Input forwarding module specification - **MODIFIED** `src/capture/capture_protocol.hpp` - Extended IPC protocol with `config_request` and `input_display_ready` message types - **MODIFIED** `src/capture/capture_receiver` - Added config handshake handlingAs per coding guidelines for
openspec/changes/**/proposal.md.src/app/main.cpp (1)
51-58: Consider rate-limiting error logs for high-frequency events.Mouse motion events can fire hundreds of times per second. If forwarding fails persistently (e.g., XWayland crashes), this will flood the logs with error messages. Consider logging the first failure and then suppressing subsequent failures for the same event type, or using
LOG_WARNwith rate limiting.// Example approach: log once per failure state transition static bool motion_error_logged = false; if (!forward_result) { if (!motion_error_logged) { GOGGLES_LOG_WARN("Mouse motion forwarding failed: {}", forward_result.error().message); motion_error_logged = true; } } else { motion_error_logged = false; // Reset on success }src/input/input_forwarder.cpp (1)
112-112: Remove obsolete TODO comment.This TODO references "Task 16" for sending the DISPLAY number to the layer via IPC. However, the PR already implements this functionality through the config handshake (CaptureConfigRequest/CaptureInputDisplayReady) and the
display_number()method that's used byCaptureReceiver. This comment should be removed.🔎 Proposed fix
- // TODO: Send DISPLAY number to layer via IPC (Task 16) - return {};
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
cmake/Dependencies.cmakedocs/input_forwarding.mdopenspec/changes/add-input-forwarding/design.mdopenspec/changes/add-input-forwarding/proposal.mdopenspec/changes/add-input-forwarding/specs/input-forwarding/spec.mdopenspec/changes/add-input-forwarding/specs/vk-layer-capture/spec.mdopenspec/changes/add-input-forwarding/tasks.mdsrc/CMakeLists.txtsrc/app/CMakeLists.txtsrc/app/main.cppsrc/capture/capture_protocol.hppsrc/capture/capture_receiver.cppsrc/capture/capture_receiver.hppsrc/capture/vk_layer/ipc_socket.hppsrc/capture/vk_layer/vk_capture.cppsrc/input/CMakeLists.txtsrc/input/input_forwarder.cppsrc/input/input_forwarder.hppsrc/input/xwayland_server.cppsrc/input/xwayland_server.hppsrc/util/error.hpptests/CMakeLists.txttests/input/goggles_input_test.cpp
🧰 Additional context used
📓 Path-based instructions (3)
openspec/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/**/*.md: Use#### Scenario:format (4 hashtags) for scenario headers in requirements, not bullets or bold text, with at least one scenario per requirement
Use format- **WHEN** [condition] - **THEN** [expected result]for scenario steps in requirements
Files:
openspec/changes/add-input-forwarding/tasks.mdopenspec/changes/add-input-forwarding/specs/vk-layer-capture/spec.mdopenspec/changes/add-input-forwarding/design.mdopenspec/changes/add-input-forwarding/specs/input-forwarding/spec.mdopenspec/changes/add-input-forwarding/proposal.md
openspec/changes/**/specs/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/changes/**/specs/**/*.md: Write spec deltas using## ADDED|MODIFIED|REMOVED|RENAMED Requirementsformat with at least one#### Scenario:per requirement in spec files
When modifying existing requirements in a MODIFIED delta, paste the full requirement block including the header and all scenarios, as the archiver will replace the entire requirement
Use ADDED for new capabilities that can stand alone; use MODIFIED for changes to existing requirement behavior, scope, or acceptance criteria; use RENAMED for name-only changes
Files:
openspec/changes/add-input-forwarding/specs/vk-layer-capture/spec.mdopenspec/changes/add-input-forwarding/specs/input-forwarding/spec.md
openspec/changes/**/proposal.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
List spec deltas in
proposal.mdunder 'What Changes' section, marking breaking changes with BREAKING
Files:
openspec/changes/add-input-forwarding/proposal.md
🧠 Learnings (2)
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Applies to openspec/changes/**/specs/**/*.md : Use ADDED for new capabilities that can stand alone; use MODIFIED for changes to existing requirement behavior, scope, or acceptance criteria; use RENAMED for name-only changes
Applied to files:
openspec/changes/add-input-forwarding/tasks.md
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Create a change proposal with `proposal.md`, `tasks.md`, and optional `design.md` under `openspec/changes/<change-id>/` before implementing changes to add features, make breaking changes, or update architecture
Applied to files:
openspec/changes/add-input-forwarding/tasks.md
🧬 Code graph analysis (9)
src/input/input_forwarder.hpp (3)
src/input/input_forwarder.cpp (2)
InputForwarder(91-91)InputForwarder(93-93)src/input/xwayland_server.hpp (1)
nodiscard(32-32)src/util/error.hpp (2)
nodiscard(47-51)nodiscard(53-89)
src/capture/capture_receiver.hpp (1)
src/util/unique_fd.hpp (1)
fd(34-34)
tests/input/goggles_input_test.cpp (1)
src/input/input_forwarder.hpp (4)
event(20-20)event(21-21)event(22-22)event(23-23)
src/input/xwayland_server.cpp (2)
src/input/xwayland_server.hpp (4)
XWaylandServer(22-22)XWaylandServer(23-23)XWaylandServer(25-25)XWaylandServer(27-27)src/util/error.hpp (1)
make_error(47-49)
src/capture/capture_receiver.cpp (2)
src/capture/capture_receiver.hpp (1)
fd(42-42)src/util/unique_fd.hpp (1)
fd(34-34)
src/input/xwayland_server.hpp (2)
src/input/xwayland_server.cpp (2)
XWaylandServer(27-27)XWaylandServer(29-31)src/util/error.hpp (2)
nodiscard(47-51)nodiscard(53-89)
src/input/input_forwarder.cpp (3)
src/input/input_forwarder.hpp (8)
InputForwarder(11-11)InputForwarder(12-12)InputForwarder(14-14)InputForwarder(16-16)event(20-20)event(21-21)event(22-22)event(23-23)src/util/error.hpp (1)
make_error(47-49)src/capture/capture_receiver.hpp (2)
display_num(36-36)display_num(36-36)
src/capture/vk_layer/vk_capture.cpp (1)
src/capture/vk_layer/ipc_socket.cpp (2)
connect(25-78)connect(25-25)
src/app/main.cpp (2)
src/input/input_forwarder.hpp (4)
event(20-20)event(21-21)event(22-22)event(23-23)src/capture/capture_receiver.hpp (2)
display_num(36-36)display_num(36-36)
🔇 Additional comments (30)
openspec/changes/add-input-forwarding/specs/input-forwarding/spec.md (1)
1-200: Spec structure follows coding guidelines correctly.The specification uses the correct
## ADDED Requirementsformat,#### Scenario:headers (4 hashtags), and- **WHEN** ... - **THEN** ...format for scenario steps. Each requirement has at least one scenario as required. The requirements comprehensively cover the input forwarding module's lifecycle, event forwarding, error handling, and integration points.src/CMakeLists.txt (1)
6-6: LGTM!The
add_subdirectory(input)follows the existing pattern for including source subdirectories.src/app/CMakeLists.txt (1)
15-15: LGTM!Linking
goggles_inputprivately follows the existing pattern for internal library dependencies.cmake/Dependencies.cmake (1)
172-182: LGTM!The input forwarding dependencies are correctly configured using PkgConfig for wlroots/wayland/xkbcommon and find_package for X11. The fatal error for missing libXtst ensures early build failure with a clear message.
Consider documenting these system dependencies in the project README or build documentation, as users will need to install
wlroots,wayland-server,xkbcommon, andlibXtstdevelopment packages.src/util/error.hpp (1)
26-27: LGTM!The new error codes
input_init_failedandinput_socket_send_failedare appropriately named following the existing pattern, and theerror_code_nameswitch correctly maps them to string representations.Also applies to: 79-82
src/capture/vk_layer/vk_capture.cpp (1)
17-101: Config handshake implementation is well-structured.The
layer_early_init()constructor correctly:
- Preserves the original DISPLAY on any failure path
- Uses a blocking socket with poll() timeout for synchronous handshake
- Validates the response type and display_number before modifying the environment
- Properly closes the socket on all paths
src/capture/capture_receiver.hpp (1)
35-36: LGTM!The additions cleanly extend
CaptureReceiverfor config handshake support:
set_input_display()provides a simple setter for the display number fromInputForwarderhandle_config_request()encapsulates config request handlingm_input_display_numberwith -1 default correctly indicates "not configured"Also applies to: 42-42, 46-46
openspec/changes/add-input-forwarding/tasks.md (1)
1-17: LGTM!Task list is well-structured with clear separation between completed implementation work and pending follow-up/validation items.
src/input/CMakeLists.txt (1)
8-14: Consider simplifying include directories.When using
PkgConfig::wlrootsas an IMPORTED target, include directories are typically propagated automatically. The explicit${wlroots_INCLUDE_DIRS}etc. may be redundant if the pkg-config targets are properly defined.However, this works correctly as-is and ensures backward compatibility with older CMake/pkg-config setups.
src/capture/capture_receiver.cpp (1)
100-109: LGTM on the peek-based config request detection.The approach of peeking at the message type to distinguish config requests from normal client connections is clean and allows proper routing without consuming the message prematurely.
src/capture/capture_protocol.hpp (1)
52-66: LGTM on the new IPC message types.The
CaptureConfigRequestandCaptureInputDisplayReadystructures are well-designed:
- Fixed 16-byte size with proper alignment
int32_tfordisplay_numbercorrectly allows -1 to signal "not ready"- Reserved fields provide future extensibility
static_assertensures wire format stabilitysrc/app/main.cpp (1)
197-214: LGTM on InputForwarder initialization and integration.Good design choices:
- Graceful degradation when input forwarding fails (logs warning, continues without it)
- Display number handshake properly wired to
CaptureReceiver- Stack allocation with pointer passing ensures proper lifetime management
tests/CMakeLists.txt (1)
86-94: Thegoggles_input_testexecutable does not need to link againstgoggles_input. The test file contains only SDL and Vulkan initialization code with a simple input event loop that prints SDL events. It does not include or use any classes/functions from thegoggles_inputlibrary (noInputForwarder,XWaylandServer, or other module symbols). The current link dependencies are correct.Likely an incorrect or invalid review comment.
openspec/changes/add-input-forwarding/specs/vk-layer-capture/spec.md (3)
5-40: LGTM!The Config Handshake Protocol requirement is well-structured with comprehensive scenarios covering the constructor flow, message format, timeout handling, priority ordering, and connection independence. The specification correctly uses the
#### Scenario:format withWHEN/THEN/ANDstructure as required by the coding guidelines.
42-60: LGTM!The Protocol Message Types requirement clearly defines the new IPC message structures with explicit size requirements (16 bytes) and proper numbering to avoid conflicts. The scenarios correctly document the wire format and reserved ranges.
62-76: LGTM!The CaptureReceiver Config Handling requirement properly specifies the receiver's branching logic and response construction. The scenarios clearly describe the connection lifecycle and integration with InputForwarder.
src/input/xwayland_server.cpp (3)
27-31: LGTM!The constructor and destructor pattern is correct. The destructor safely calls
stop(), which is idempotent and handles the case wherestart()was never called or failed.
33-175: LGTM with one verification note.The
start()method follows a solid initialization pattern with comprehensive error handling. Each failure triggers appropriate partial cleanup in reverse dependency order. The socket binding loop (1-9) is reasonable, and the compositor thread is correctly spawned after all resources are initialized.
177-226: Excellent resource cleanup ordering.The
stop()method demonstrates careful attention to destruction order. The explicit thread join before destroying objects (line 184-187) prevents use-after-free, and the comments clearly document dependencies (e.g., XWayland must be destroyed before compositor, seat before display). All pointers are defensively nulled after destruction.src/input/input_forwarder.hpp (1)
1-31: LGTM!The header demonstrates excellent API design with proper PIMPL pattern, appropriate use of
[[nodiscard]]on all public methods, and correctly deleted copy/move operations. The interface is clean and well-organized.tests/input/goggles_input_test.cpp (2)
49-99: LGTM!The initialization functions are straightforward and appropriate for a test harness. The forced X11 video driver (line 74) is necessary for testing the XWayland integration. Error handling via
std::exit(1)is acceptable for a test binary.
101-182: LGTM!The event logging and input loop are well-structured. The initialization order in
main()(Vulkan first to trigger layer loading, then SDL) is correctly documented and necessary for testing the DISPLAY handshake flow.src/input/input_forwarder.cpp (6)
11-77: LGTM!The keycode translation tables correctly map SDL scancodes to Linux input event codes and then to X11 keycodes (+8 offset). The coverage is limited to common keys, which is appropriate for the initial implementation. Unmapped keys return 0 and are safely handled by the caller.
80-93: LGTM!The
Implstruct demonstrates proper RAII with correct destruction order: X11 display is closed explicitly, then theXWaylandServeris destroyed automatically. The constructor and destructor implementations are correct.
95-115: LGTM with one cleanup note.The
init()method correctly starts the XWayland server, opens the X11 connection, and properly cleans up on failure. Error propagation is handled well through theResult<void>type. See separate comment about the outdated TODO.
117-149: LGTM!Both methods properly guard against uninitialized state and correctly use XTest to generate events. The comment on line 141 correctly notes that SDL and X11 use the same button numbering. The
XFlushcalls ensure events are delivered immediately.
151-167: LGTM - acknowledged limitation.The mouse motion forwarding has a known limitation (no coordinate scaling) that's documented in both the code (lines 159-162) and the PR objectives. The 1:1 mapping is acceptable for the initial implementation, with proper scaling deferred to Phase 2 as noted in the design doc.
169-199: LGTM!The mouse wheel forwarding correctly maps scroll directions to X11 button events (4=up, 5=down, 6/7=horizontal). The press+release pair (lines 189-190) is the correct X11 protocol for wheel events. The
display_number()method properly delegates to the server.src/input/xwayland_server.hpp (1)
1-47: LGTM!The header demonstrates excellent design with minimal dependencies through forward declarations, appropriate NOLINT suppressions for external types, and a clean public interface. The use of
std::jthread(line 43) ensures automatic thread joining, and the inlinedisplay_number()getter is appropriate for a trivial accessor.openspec/changes/add-input-forwarding/design.md (1)
1-385: Excellent design documentation.The design document is comprehensive, well-organized, and accurately reflects the implementation. The architecture layers are clearly explained, the IPC protocol extension is properly documented, and the RAII resource management strategy matches the implementation. The comparisons with alternatives and dependency justifications provide valuable context for the design decisions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/capture/capture_receiver.cpp (1)
357-360: Duplicate: Distinguish EAGAIN from genuinely incomplete messages.This recv() error-handling pattern has the same issue flagged in the previous review. The socket is non-blocking, so
recv()can return-1withEAGAIN/EWOULDBLOCKwhen data isn't ready yet, but the current code logs "incomplete message received" for any return value other thansizeof(request), which is misleading.Refer to the past review comment on lines 352-365 for the detailed fix. The suggested approach is to explicitly check for
recv() == -1and inspecterrnoto differentiate betweenEAGAIN/EWOULDBLOCK(data not ready) versus actual recv errors, and handle partial reads separately.
🧹 Nitpick comments (1)
src/capture/capture_receiver.cpp (1)
103-112: Consider logging peek failures for easier debugging.When the
recvwithMSG_PEEKfails (e.g., due to errors other thanEAGAIN), the code silently falls through to the normal frame capture flow. While this is functionally safe, adding debug-level logging for unexpected peek failures would help diagnose issues where config requests are missed.🔎 Optional enhancement
// Peek at message type to detect config requests CaptureMessageType msg_type{}; ssize_t peeked = recv(new_fd, &msg_type, sizeof(msg_type), MSG_PEEK | MSG_DONTWAIT); +if (peeked < 0 && errno != EAGAIN && errno != EWOULDBLOCK) { + GOGGLES_LOG_DEBUG("Peek failed on new connection: {}", strerror(errno)); +} + if (peeked == sizeof(msg_type) && msg_type == CaptureMessageType::config_request) { // This is a config request connection - handle and close handle_config_request(new_fd); close(new_fd); return false; // Don't set as client_fd }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cmake/Dependencies.cmakesrc/app/main.cppsrc/capture/capture_protocol.hppsrc/capture/capture_receiver.cppsrc/capture/capture_receiver.hppsrc/capture/vk_layer/ipc_socket.hppsrc/capture/vk_layer/vk_capture.cppsrc/input/CMakeLists.txttests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (8)
- cmake/Dependencies.cmake
- tests/CMakeLists.txt
- src/capture/capture_receiver.hpp
- src/capture/vk_layer/ipc_socket.hpp
- src/input/CMakeLists.txt
- src/capture/vk_layer/vk_capture.cpp
- src/capture/capture_protocol.hpp
- src/app/main.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
src/capture/capture_receiver.cpp (2)
src/capture/capture_receiver.hpp (1)
fd(52-52)src/util/unique_fd.hpp (1)
fd(34-34)
🔇 Additional comments (1)
src/capture/capture_receiver.cpp (1)
368-370: Layer already validates and handles negative display_number gracefully.The vk_capture layer explicitly validates
display_number < 0invk_capture.cpp:88-92and keeps the original DISPLAY setting when the value is invalid, rather than failing or retrying. No action needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/input_forwarding.md (2)
19-49: Consider adding language identifiers to fenced code blocks.The static analysis tool flags code blocks without language specifiers. For ASCII diagrams and text flow diagrams, you can use
textorplaintextas the language identifier:-``` +```text ┌────────────────────────────────────────────────────────────┐This applies to blocks at lines 19, 158, 166, and 174.
140-152: Documentation example differs from actual implementation.The code example shows a simplified version that hardcodes
setenv("DISPLAY", ":1", 1), but the actual implementation invk_capture.cppperforms an IPC handshake to dynamically obtain the display number. Consider updating the example to reflect the actual behavior or add a note clarifying this is a simplified illustration:```cpp __attribute__((constructor)) static void layer_early_init() { const char* old_display = getenv("DISPLAY"); - setenv("DISPLAY", ":1", 1); - // App now connects to XWayland on :1 instead of host X server on :0 + // Performs IPC handshake with Goggles to get display number + // Then sets DISPLAY=:N where N is the negotiated display + // (simplified - actual implementation uses socket handshake) }</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between c257b8efde0512a2d33ec9fb510b2dae69a17f97 and 4810663152dd0d82a17a76598243155191a86aaf. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `docs/input_forwarding.md` * `src/capture/vk_layer/vk_capture.cpp` * `tests/input/goggles_input_test.cpp` </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (2)</summary> <details> <summary>src/capture/vk_layer/vk_capture.cpp (1)</summary><blockquote> <details> <summary>src/capture/vk_layer/ipc_socket.cpp (2)</summary> * `connect` (25-78) * `connect` (25-25) </details> </blockquote></details> <details> <summary>tests/input/goggles_input_test.cpp (1)</summary><blockquote> <details> <summary>src/input/input_forwarder.hpp (4)</summary> * `event` (20-20) * `event` (21-21) * `event` (22-22) * `event` (23-23) </details> </blockquote></details> </details><details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>docs/input_forwarding.md</summary> 19-19: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 158-158: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 166-166: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 174-174: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)</summary> * GitHub Check: Static Analysis (clang-tidy) * GitHub Check: Build and Test </details> <details> <summary>🔇 Additional comments (6)</summary><blockquote> <details> <summary>src/capture/vk_layer/vk_capture.cpp (1)</summary><blockquote> `17-98`: **Well-structured early initialization with proper error handling.** The `layer_early_init()` constructor correctly: - Uses a blocking socket (no `SOCK_NONBLOCK`) suitable for synchronous config handshake - Validates `display_number >= 0` before setting DISPLAY - Closes the socket on all error paths - Uses appropriate 100ms timeout for the handshake The past review comment about the unused `LayerSocketClient config_socket` variable has been addressed — it's no longer present. </blockquote></details> <details> <summary>docs/input_forwarding.md (1)</summary><blockquote> `106-123`: **Documentation now accurately reflects current mouse support.** The limitations section correctly states "Basic mouse support: Mouse button, motion, and wheel events are forwarded but coordinate mapping is 1:1", which aligns with the implementation. The past review comment about outdated mouse support documentation has been addressed. </blockquote></details> <details> <summary>tests/input/goggles_input_test.cpp (4)</summary><blockquote> `8-36`: **Move semantics correctly implemented.** The `VulkanInstance` now properly implements move operations that null out the source handle, preventing the double-free issue flagged in the past review. The move assignment also correctly destroys any existing handle before taking ownership. --- `38-92`: **SDLContext move semantics and initialization tracking correctly implemented.** The past review concerns have been addressed: - Added `initialized` flag (line 41) that tracks whether `SDL_Init` was called - Move operations properly transfer all state including `initialized` and null out the source - Destructor only calls `SDL_Quit()` if `initialized` is true (lines 52-54) --- `96-148`: **Initialization functions are well-structured.** Both `init_vulkan()` and `init_sdl()` properly: - Handle errors with informative messages - Use RAII wrappers that ensure cleanup even on early exit - Set `sdl.initialized = true` only after `SDL_Init` succeeds (line 128) The forced X11 backend via `SDL_VIDEODRIVER=x11` (line 121) is appropriate for testing the XWayland input forwarding path. --- `213-231`: **Correct initialization order for layer testing.** The deliberate ordering—Vulkan initialization first (line 217) followed by SDL (line 221)—ensures the Vulkan layer's `layer_early_init()` constructor runs and sets `DISPLAY` before SDL attempts to connect to the X server. The comments clearly document this design intent. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/input/xwayland_server.cpp (1)
120-131: Consider expanding the display number range.The current range of 1-9 provides only 9 possible display numbers. On systems with many X servers or Wayland compositors, this might be limiting. Consider extending to a larger range (e.g., 1-99) to reduce collision probability.
🔎 Suggested change
- for (int display_num = 1; display_num < 10; ++display_num) { + for (int display_num = 1; display_num < 100; ++display_num) {And update the error message:
return make_error<int>(ErrorCode::input_init_failed, - "No available DISPLAY numbers (1-9 all bound)"); + "No available DISPLAY numbers (1-99 all bound)");src/input/xwayland_server.hpp (1)
21-34: Add class documentation.The class design is sound with proper RAII semantics (deleted copy/move operations) and good error handling (Result return type). However, the class lacks documentation explaining its purpose, lifecycle, and thread safety characteristics.
🔎 Suggested documentation
namespace goggles::input { +/** + * @brief Manages a headless Wayland compositor with nested XWayland server. + * + * Creates a wlroots-based headless Wayland compositor and spawns an XWayland + * server, automatically selecting an available DISPLAY number (1-9). + * Runs the compositor in a background thread. + * + * Lifecycle: + * - Construct → start() → use display_number() → stop() or destructor + * - start() may be called only once; subsequent calls are undefined + * - stop() is idempotent and called automatically by the destructor + * + * Thread safety: + * - The compositor runs in m_compositor_thread + * - stop() synchronizes by terminating the display and joining the thread + * - Do not call start()/stop() concurrently from multiple threads + */ class XWaylandServer {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/input/xwayland_server.cppsrc/input/xwayland_server.hpp
🧰 Additional context used
🧬 Code graph analysis (2)
src/input/xwayland_server.cpp (3)
src/input/xwayland_server.hpp (4)
XWaylandServer(23-23)XWaylandServer(24-24)XWaylandServer(26-26)XWaylandServer(28-28)src/util/error.hpp (1)
make_error(47-49)src/capture/capture_receiver.hpp (2)
display_num(38-38)display_num(38-38)
src/input/xwayland_server.hpp (2)
src/input/xwayland_server.cpp (2)
XWaylandServer(27-27)XWaylandServer(29-31)src/util/error.hpp (2)
nodiscard(47-51)nodiscard(53-89)
🔇 Additional comments (9)
src/input/xwayland_server.cpp (7)
1-24: LGTM!The includes and external declarations are well-organized. The manual forward declarations for
wlr_xdg_shellfunctions and theclasskeyword workaround forxwayland.hare appropriate solutions for C/C++ interop with wlroots.
27-31: LGTM!The default constructor and destructor design is sound. The destructor calling
stop()ensures proper RAII cleanup of resources.
89-101: Previous issue resolved.The past review comment about missing error handling for
wlr_xdg_shell_createhas been addressed. The code now properly checks for NULL and returns an error with appropriate cleanup.
120-146: LGTM!The socket binding logic correctly tries display numbers 1-9 and returns an appropriate error if none are available. The error handling is thorough.
148-187: LGTM!The XWayland creation, backend startup, and compositor thread initialization are well-structured with proper error handling and cleanup on failure.
189-243: LGTM!The
stop()method demonstrates excellent resource management:
- Explicitly joins the compositor thread before destroying objects to prevent use-after-free
- Destroys objects in the correct order with clear comments explaining dependencies
- Properly resets all pointers to nullptr
- Handles the case where stop() is called multiple times
118-118: No error checking needed forwlr_seat_set_capabilities— the function returnsvoid.
wlr_seat_set_capabilitiesdoes not return an error code; it has avoidreturn type and cannot fail in a way that produces a checkable return value. The function simply updates the seat's capability flags and sends the update to clients. Responsibility for validity (e.g., ensuringm_seatis non-NULL) rests with the caller.Likely an incorrect or invalid review comment.
src/input/xwayland_server.hpp (2)
1-18: LGTM!The header structure is clean with appropriate includes and forward declarations. The
NOLINTcomments for wlroots types correctly suppress naming convention warnings for external library types.
35-47: LGTM!The private member variables are well-organized:
- All resource pointers properly initialized to nullptr
m_display_numberuses -1 as a sentinel valuestd::jthreadautomatically joins on destruction, complementing the RAII design
K1ngst0m
force-pushed
the
dev/kingstom/add-input-forwarding
branch
from
d3088a9 to
7782f7a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/input/xwayland_server.cpp (1)
62-63: Add error handling forwlr_renderer_init_wl_display.The return value of
wlr_renderer_init_wl_displayis ignored. This function returnsbooland can fail (e.g., whenwl_display_init_shm()fails orwlr_renderer_get_formats()returns NULL). All other wlroots initialization calls in this function check for errors, making this omission inconsistent and potentially unsafe.🔎 Proposed fix
- // XWayland crashes (segfault in wl_proxy_get_version) without protocol init - wlr_renderer_init_wl_display(m_renderer, m_display); + // XWayland crashes (segfault in wl_proxy_get_version) without protocol init + if (!wlr_renderer_init_wl_display(m_renderer, m_display)) { + wlr_renderer_destroy(m_renderer); + wlr_backend_destroy(m_backend); + wl_display_destroy(m_display); + m_renderer = nullptr; + m_backend = nullptr; + m_display = nullptr; + return make_error<int>(ErrorCode::input_init_failed, "Failed to initialize renderer for Wayland display"); + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/capture/vk_layer/ipc_socket.hppsrc/input/xwayland_server.cppsrc/input/xwayland_server.hpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/capture/vk_layer/ipc_socket.hpp
🧰 Additional context used
🧬 Code graph analysis (2)
src/input/xwayland_server.cpp (3)
src/input/xwayland_server.hpp (4)
XWaylandServer(23-23)XWaylandServer(24-24)XWaylandServer(26-26)XWaylandServer(28-28)src/util/error.hpp (1)
make_error(47-49)src/capture/capture_receiver.hpp (2)
display_num(38-38)display_num(38-38)
src/input/xwayland_server.hpp (2)
src/input/xwayland_server.cpp (2)
XWaylandServer(27-27)XWaylandServer(29-31)src/util/error.hpp (2)
nodiscard(47-51)nodiscard(53-89)
🔇 Additional comments (3)
src/input/xwayland_server.hpp (1)
1-49: LGTM: Well-designed RAII header.The header demonstrates solid design:
- Forward declarations minimize compilation dependencies.
- Non-copyable/non-movable semantics are appropriate for managing external C resources.
- Member initialization (m_display_number = -1, pointers = nullptr) prevents undefined behavior.
- The inline getter is clean and appropriate.
src/input/xwayland_server.cpp (2)
27-31: LGTM: Proper RAII pattern.The destructor correctly delegates cleanup to
stop(), ensuring resources are released even if the caller forgets to callstop()explicitly.
189-243: LGTM: Correct and well-ordered cleanup.The
stop()method demonstrates careful attention to destruction order:
- Terminates the display and joins the thread before destroying objects (preventing use-after-free).
- Destroys XWayland before compositor, seat before display, as required by wlroots lifecycle constraints.
- Explicitly nulls all pointers for idempotency.
The inline comments documenting ordering constraints are valuable.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/app/CMakeLists.txt (1)
15-15: Pipeline failure due to missing wlroots dependency.The
goggles_inputlinkage is correct, but the build fails because the input subdirectory cannot be built without wlroots-0.18. See the review comment onsrc/CMakeLists.txtfor details on addressing the missing dependency.src/capture/capture_receiver.cpp (1)
352-379: Address past review comments on recv/send error handling.The implementation of
handle_config_requesthas the same recv/send error handling issues identified in past review comments:
Lines 355-360:
recv()error handling doesn't distinguish betweenEAGAIN/EWOULDBLOCK(no data ready) and genuine errors or incomplete reads on a non-blocking socket.Lines 372-376:
send()error handling conflates send errors (sent == -1) with partial sends, making diagnostics unclear.The past review comments provide detailed fixes for both issues. Please implement those suggestions to properly handle non-blocking socket behavior and provide clear error diagnostics.
🧹 Nitpick comments (4)
docs/input_forwarding.md (1)
11-11: Minor markdown formatting issues flagged by linter.Static analysis identified two formatting issues:
- Line 11: Fenced code block missing language specifier
- Line 178:
**Advantages**:uses emphasis instead of a proper heading🔎 Proposed markdown fixes
Fix line 11 (add language specifier):
-``` +```text ┌────────────────────────────────────────────────────────────┐ │ Goggles Application (DISPLAY=:0, viewer window) │Fix line 178 (use proper heading):
-**Advantages**: No special permissions, works with Wine, minimal dependencies, no display output. +#### Advantages + +No special permissions, works with Wine, minimal dependencies, no display output.Also applies to: 178-178
openspec/changes/add-input-forwarding/proposal.md (2)
20-47: Add language identifier to architecture diagram.The fenced code block containing the ASCII architecture diagram should specify a language for proper rendering. Consider using
textormermaidif converting to a proper diagram format.🔎 Proposed fix
-``` +```text ┌─────────────────────────────────────────┐ │ Goggles (DISPLAY=:0) │Based on static analysis hints.
1-120: Consider adding 'What Changes' section per coding guidelines.The coding guidelines specify that
openspec/changes/**/proposal.mdfiles should list spec deltas under a 'What Changes' section, marking breaking changes with BREAKING. Consider adding this section to document the protocol and API additions (e.g., new message types, InputForwarder API, XWaylandServer).As per coding guidelines.
src/input/input_forwarder.cpp (1)
95-115: LGTM: Robust initialization with proper cleanup on failure.The
init()method demonstrates good error handling:
- Starts XWayland server and checks result
- Opens X11 display connection
- Stops the server if display connection fails (prevents resource leak)
- Clear error messages with context
Line 112: The TODO comment appears outdated—the display number is already communicated via
display_number()method and used byCaptureReceiver(per the integration in main.cpp).🔎 Remove outdated TODO
- // TODO: Send DISPLAY number to layer via IPC (Task 16) - return {};
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
cmake/Dependencies.cmakedocs/input_forwarding.mdopenspec/changes/add-input-forwarding/design.mdopenspec/changes/add-input-forwarding/proposal.mdopenspec/changes/add-input-forwarding/specs/input-forwarding/spec.mdopenspec/changes/add-input-forwarding/specs/vk-layer-capture/spec.mdopenspec/changes/add-input-forwarding/tasks.mdsrc/CMakeLists.txtsrc/app/CMakeLists.txtsrc/app/main.cppsrc/capture/capture_protocol.hppsrc/capture/capture_receiver.cppsrc/capture/capture_receiver.hppsrc/capture/vk_layer/ipc_socket.hppsrc/capture/vk_layer/vk_capture.cppsrc/input/CMakeLists.txtsrc/input/input_forwarder.cppsrc/input/input_forwarder.hppsrc/input/xwayland_server.cppsrc/input/xwayland_server.hppsrc/util/error.hpptests/CMakeLists.txttests/input/goggles_input_test.cpp
🧰 Additional context used
📓 Path-based instructions (3)
openspec/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/**/*.md: Use#### Scenario:format (4 hashtags) for scenario headers in requirements, not bullets or bold text, with at least one scenario per requirement
Use format- **WHEN** [condition] - **THEN** [expected result]for scenario steps in requirements
Files:
openspec/changes/add-input-forwarding/tasks.mdopenspec/changes/add-input-forwarding/proposal.mdopenspec/changes/add-input-forwarding/design.mdopenspec/changes/add-input-forwarding/specs/input-forwarding/spec.mdopenspec/changes/add-input-forwarding/specs/vk-layer-capture/spec.md
openspec/changes/**/proposal.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
List spec deltas in
proposal.mdunder 'What Changes' section, marking breaking changes with BREAKING
Files:
openspec/changes/add-input-forwarding/proposal.md
openspec/changes/**/specs/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/changes/**/specs/**/*.md: Write spec deltas using## ADDED|MODIFIED|REMOVED|RENAMED Requirementsformat with at least one#### Scenario:per requirement in spec files
When modifying existing requirements in a MODIFIED delta, paste the full requirement block including the header and all scenarios, as the archiver will replace the entire requirement
Use ADDED for new capabilities that can stand alone; use MODIFIED for changes to existing requirement behavior, scope, or acceptance criteria; use RENAMED for name-only changes
Files:
openspec/changes/add-input-forwarding/specs/input-forwarding/spec.mdopenspec/changes/add-input-forwarding/specs/vk-layer-capture/spec.md
🧠 Learnings (2)
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Applies to openspec/changes/**/specs/**/*.md : Use ADDED for new capabilities that can stand alone; use MODIFIED for changes to existing requirement behavior, scope, or acceptance criteria; use RENAMED for name-only changes
Applied to files:
openspec/changes/add-input-forwarding/tasks.md
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Create a change proposal with `proposal.md`, `tasks.md`, and optional `design.md` under `openspec/changes/<change-id>/` before implementing changes to add features, make breaking changes, or update architecture
Applied to files:
openspec/changes/add-input-forwarding/tasks.md
🧬 Code graph analysis (9)
src/input/input_forwarder.hpp (3)
src/input/input_forwarder.cpp (2)
InputForwarder(91-91)InputForwarder(93-93)src/input/xwayland_server.hpp (1)
nodiscard(33-33)src/util/error.hpp (2)
nodiscard(47-51)nodiscard(53-89)
src/app/main.cpp (2)
src/input/input_forwarder.hpp (4)
event(20-20)event(21-21)event(22-22)event(23-23)src/capture/capture_receiver.hpp (2)
display_num(38-38)display_num(38-38)
src/input/xwayland_server.cpp (3)
src/input/xwayland_server.hpp (4)
XWaylandServer(23-23)XWaylandServer(24-24)XWaylandServer(26-26)XWaylandServer(28-28)src/util/error.hpp (1)
make_error(47-49)src/capture/capture_receiver.hpp (2)
display_num(38-38)display_num(38-38)
src/capture/vk_layer/vk_capture.cpp (1)
src/capture/vk_layer/ipc_socket.cpp (2)
connect(25-78)connect(25-25)
tests/input/goggles_input_test.cpp (1)
src/input/input_forwarder.hpp (4)
event(20-20)event(21-21)event(22-22)event(23-23)
src/capture/capture_receiver.cpp (1)
src/capture/capture_receiver.hpp (1)
fd(52-52)
src/input/input_forwarder.cpp (4)
src/input/input_forwarder.hpp (8)
InputForwarder(11-11)InputForwarder(12-12)InputForwarder(14-14)InputForwarder(16-16)event(20-20)event(21-21)event(22-22)event(23-23)src/capture/capture_receiver.cpp (2)
init(22-60)init(22-22)src/util/error.hpp (1)
make_error(47-49)src/capture/capture_receiver.hpp (2)
display_num(38-38)display_num(38-38)
src/input/xwayland_server.hpp (2)
src/input/xwayland_server.cpp (2)
XWaylandServer(27-27)XWaylandServer(29-31)src/util/error.hpp (2)
nodiscard(47-51)nodiscard(53-89)
src/capture/capture_receiver.hpp (2)
src/util/unique_fd.hpp (1)
fd(34-34)src/capture/vk_layer/ipc_socket.hpp (1)
data(23-23)
🪛 GitHub Actions: CI
src/input/CMakeLists.txt
[error] 15-15: CMake configuration failed. The following required packages were not found: wlroots-0.18.
tests/CMakeLists.txt
[error] 15-15: CMake configuration failed. The following required packages were not found: wlroots-0.18.
src/CMakeLists.txt
[error] 15-15: CMake configuration failed. The following required packages were not found: wlroots-0.18.
src/app/CMakeLists.txt
[error] 15-15: CMake configuration failed. The following required packages were not found: wlroots-0.18.
🪛 markdownlint-cli2 (0.18.1)
openspec/changes/add-input-forwarding/proposal.md
11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
120-120: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
docs/input_forwarding.md
11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
120-120: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
178-178: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (30)
openspec/changes/add-input-forwarding/specs/vk-layer-capture/spec.md (1)
1-76: LGTM! Clean separation of config and frame capture protocols.The spec follows the required format with
## ADDED Requirementsand#### Scenario:headers. The two-phase handshake design (config connection in constructor, frame capture on present) elegantly solves the timing problem where DISPLAY must be set before app initialization. The message type numbering (6, 7) appropriately skips 4-5 for future use, and the 100ms timeout for config response is reasonable for local IPC.openspec/changes/add-input-forwarding/design.md (1)
1-385: Excellent design documentation with comprehensive error handling.The design document thoroughly covers all aspects of the input forwarding architecture:
- Clear API surface with PIMPL pattern for
InputForwarder- Well-defined layer architecture (SDL → InputForwarder → XWaylandServer → IPC → VK Layer)
- Robust error handling with
Result<T>pattern and graceful degradation- RAII-based resource management with explicit cleanup order for wlroots objects
- Performance characteristics documented (~1 MB memory, <1% CPU, ~1-2 ms latency)
- Comprehensive comparison with rejected alternatives (uinput, Cage)
The two-phase IPC protocol (config handshake in constructor, frame capture on present) elegantly solves the DISPLAY timing requirement.
src/capture/vk_layer/ipc_socket.hpp (1)
6-8: LGTM! Headers required for config handshake.The added POSIX headers (
poll.h,sys/socket.h,unistd.h) are necessary for the config handshake socket operations performed inlayer_early_init()(poll timeout, socket creation, connect, send/recv, close).src/capture/vk_layer/vk_capture.cpp (1)
17-98: Excellent implementation with robust error handling.The
layer_early_init()constructor implements the config handshake exactly as specified:
- Constructor priority
101ensures execution before default constructors- Clean error handling on every operation with early returns
- Validates
display_number >= 0before use (line 85)- Preserves original
DISPLAYon any failure (graceful degradation)- 100ms timeout matches spec (line 62)
- Comprehensive logging for debugging
The implementation correctly uses
poll()for the timeout and validates the message type before processing the response.src/capture/capture_receiver.hpp (3)
37-38: LGTM! Simple setter for config handshake.The inline setter is appropriate for passing the DISPLAY number from InputForwarder to the capture receiver. The comment clearly documents its purpose.
52-52: LGTM! Private method for config handshake.The private declaration is appropriate for handling configuration requests separately from the main frame capture flow.
58-58: LGTM! State for input display number.The default value of -1 serves as a clear sentinel for "not set", and the comment documents the data source.
src/util/error.hpp (2)
26-27: LGTM! Error codes for input forwarding.The new error codes follow the existing naming convention and cover input initialization and IPC socket failures.
79-82: LGTM! Error code name mappings.The switch cases correctly map the new error codes to their string representations.
tests/CMakeLists.txt (1)
88-108: LGTM! Test executable setup is correct.The
goggles_input_testexecutable is properly configured with SDL3, Vulkan, and goggles_util dependencies. The build configuration, output directory, and sanitizer setup follow project conventions.However, note the pipeline failure indicating missing wlroots-0.18 dependency. This is expected for the new input forwarding feature and should be resolved by installing the required system packages on the CI environment.
Pipeline failure: Missing wlroots dependency.
The CI build fails because wlroots-0.18 is not installed. This is a CI environment issue, not a code issue. Ensure the CI environment is updated to install wlroots-0.18 and its dependencies (wayland-server, xkbcommon, libX11, libXtst) as documented in the PR.
openspec/changes/add-input-forwarding/specs/input-forwarding/spec.md (1)
1-200: LGTM! Comprehensive specification with proper structure.The specification correctly follows the required format:
- Uses
## ADDED Requirementsfor new capabilities- Each requirement has at least one
#### Scenario:with WHEN/THEN format- Covers all key aspects: lifecycle, forwarding, error handling, integration, and conventions
- Scenarios provide clear acceptance criteria
The spec provides a solid foundation for validating the implementation.
openspec/changes/add-input-forwarding/tasks.md (1)
1-17: LGTM! Clear task tracking with appropriate follow-ups.The task list effectively documents:
- Completed implementation items (1.1-1.6)
- Identified follow-ups for future work (2.1-2.4)
- Validation steps (3.1-3.2)
The follow-up items appropriately defer non-critical refinements (coordinate mapping, handshake hardening, optional dependencies) while delivering the core functionality.
src/input/CMakeLists.txt (1)
1-30: LGTM! Proper library configuration for input forwarding.The
goggles_inputlibrary is correctly set up:
- Static library with appropriate sources
- Public/private include directories properly separated
- Dependencies correctly linked (wlroots, wayland-server, xkbcommon, X11, XTest)
- Required
WLR_USE_UNSTABLEdefinition for wlroots API access- Tooling enabled (clang-tidy, sanitizers, profiling)
The pipeline failure for missing wlroots-0.18 is a CI environment configuration issue, not a code issue. The CMake configuration itself is correct.
Pipeline failure: Missing wlroots dependency (duplicate of tests/CMakeLists.txt issue).
Same CI environment issue as noted in tests/CMakeLists.txt. The build configuration is correct; the CI environment needs wlroots-0.18 and related packages installed.
src/capture/capture_receiver.cpp (1)
103-112: LGTM! Config request detection with MSG_PEEK.The peek logic correctly routes config_request connections separately from normal frame capture connections:
- Uses
MSG_PEEK | MSG_DONTWAITto inspect message type without consuming data- Delegates to
handle_config_requestand closes the fd for config connections- Returns false to avoid treating config connections as persistent clients
The approach cleanly separates the config handshake from the frame capture protocol.
src/app/main.cpp (3)
19-21: LGTM! Optional InputForwarder parameter for graceful degradation.The signature change to accept an optional
InputForwarder*parameter enables the application to run without input forwarding if initialization fails. This is the right design choice.
36-68: LGTM! Consistent event forwarding with proper error handling.The SDL event forwarding implementation is well-structured:
- Each event type (key, mouse button, motion, wheel) is handled consistently
- Properly guarded with null-check on
input_forwarder- Errors are logged but don't terminate the application
- Pattern is clear and maintainable
The per-event error handling prevents cascading failures while providing diagnostic information.
219-234: LGTM! Proper InputForwarder initialization and lifecycle.The initialization flow correctly implements graceful degradation:
- Stack-allocated
InputForwarderwith appropriate lifetime (entire main loop)- Error handling logs the failure but allows the app to continue
- On success, the display number is propagated to
capture_receiverviaset_input_display- Pointer is passed to
run_main_looponly when initialization succeedsThe lifecycle management is correct: the forwarder lives for the duration of
run_main_loopand is automatically cleaned up on function exit.src/input/xwayland_server.cpp (2)
33-206: LGTM: Comprehensive initialization with proper error handling.The
start()method demonstrates solid resource management:
- Sequential initialization with proper error checking at each step
- Consistent cleanup on all error paths (including m_xdg_shell and m_seat that were missing in earlier commits)
- RAII thread management with
std::jthread- Clear separation of concerns
The past review issues regarding resource leaks have been properly addressed.
208-262: LGTM: Well-ordered resource cleanup.The
stop()method properly manages shutdown:
- Guards against double-cleanup with null check
- Explicitly joins compositor thread before destroying objects (prevents use-after-free)
- Destruction order respects wlroots object dependencies
- All pointers are nulled after destruction
The comment at line 237 correctly notes that compositor is auto-destroyed, matching the implementation.
src/capture/capture_protocol.hpp (2)
20-21: LGTM: Wire protocol extension follows established pattern.The new message types (
config_requestandinput_display_ready) are properly integrated into the existing protocol enum with sequential numbering and consistent uint32_t backing for wire format stability.
74-88: LGTM: Well-designed protocol structs with forward compatibility.Both new structs follow the established pattern:
- Explicit type tag and version fields
- Reserved space for future extensions
- Static size assertions for wire format stability
display_numberasint32_tappropriately allows -1 for error casessrc/input/input_forwarder.hpp (1)
1-31: LGTM: Clean PIMPL-based public API with proper error handling.The interface design demonstrates good practices:
- PImpl pattern provides ABI stability and encapsulation
- Deleted copy/move operations prevent resource management issues
[[nodiscard]]attributes enforce error checking on all fallible operations- Consistent use of
Result<void>for error propagation- Clear separation between initialization (
init()), forwarding operations, and query (display_number())tests/input/goggles_input_test.cpp (3)
8-36: LGTM: Proper RAII with correct move semantics.The
VulkanInstancewrapper correctly implements resource management:
- Move constructor transfers ownership and nulls the source
- Move assignment performs cleanup before transfer
- Destructor safely checks for valid handle
- Uses
VK_NULL_HANDLEfor moved-from stateThe past double-free issue has been properly resolved.
38-92: LGTM: Proper SDL resource management with move semantics.The
SDLContextwrapper correctly handles SDL resources:
initializedflag prevents callingSDL_Quit()on moved-from or uninitialized objects- Move operations transfer all state (window, renderer, initialized)
- Destructor conditionally cleans up each resource
- Move assignment properly cleans up existing resources before transfer
The past issue regarding double-cleanup has been properly resolved.
96-231: LGTM: Well-structured test harness for input forwarding.The test implementation properly exercises the input forwarding stack:
- Correct initialization order (Vulkan first to trigger layer, then SDL)
- Proper RAII with RAII wrappers for resources
- Clear logging for debugging
- Simple but effective event loop
Line 121 uses
setenv()which is POSIX-specific. This is acceptable for a Linux-focused input forwarding feature (requires X11/Wayland), but note that the test won't compile on Windows.src/input/input_forwarder.cpp (4)
14-75: LGTM: Correct keycode mappings for basic input.The keycode translation functions are properly implemented:
sdl_to_linux_keycode()correctly maps to Linux input-event-codes valueslinux_to_x11_keycode()applies the documented X11 protocol offset (+8)- Returns 0 for unmapped keys, handled gracefully by callers
- Coverage includes essential keys (A-Z, 0-9, arrows, modifiers, common special keys)
Limited coverage (no F-keys, numpad, etc.) aligns with the proposal's basic input goals.
117-134: LGTM: Correct keyboard event forwarding via XTest.The keyboard forwarding implementation is solid:
- Gracefully handles uninitialized display (early return)
- Maps SDL scancodes to X11 keycodes
- Silently ignores unmapped keys (returns success)
- Uses
XTestFakeKeyEvent()to generate real X11 eventsXFlush()ensures immediate delivery
136-194: LGTM: Mouse event forwarding with documented limitations.The mouse event forwarding is properly implemented:
- Button events (lines 136-149): Direct mapping between SDL and X11 button numbers
- Motion events (lines 151-167): Basic 1:1 coordinate forwarding with TODO at line 159 correctly noting the limitation (coordinate mapping is listed as a non-goal in the proposal)
- Wheel events (lines 169-194): Correct X11 protocol usage (buttons 4/5/6/7 for wheel directions)
All methods properly check for initialized display and use
XFlush()for immediate delivery.
196-198: LGTM: Clean delegation to underlying server.The
display_number()method correctly delegates to the XWayland server.src/input/xwayland_server.hpp (1)
1-49: LGTM: Well-designed header with proper encapsulation.The header demonstrates good design practices:
- Forward declarations minimize compile-time dependencies
- NOLINT suppressions appropriately document external library naming conventions
- Deleted copy/move operations prevent resource management issues
- Raw pointers for wlroots types with manual lifetime management (RAII patterns in .cpp)
std::jthreadfor automatic thread joining- Clean public interface with
[[nodiscard]]on fallible operations
- Documented cross-process GPU synchronization. - Revised DMA-BUF sharing narrative and simplified filter-chain diagrams to reflect new headless and sync workflows.
- Add `goggles_input` library with headless wlroots XWayland server - Inject keyboard/mouse events via X11 XTest - Wire SDL event loop to `InputForwarder` for key/mouse forwarding - Extend capture protocol with config messages so the layer can set DISPLAY early - Add `goggles_input_test` to validate end-to-end forwarding
K1ngst0m
force-pushed
the
dev/kingstom/add-input-forwarding
branch
from
9b9ac28 to
da4b842
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
cmake/Dependencies.cmake (1)
64-74: Hard dependency on wlroots-0.18 already flagged in previous review.The past review comment comprehensively addresses making input forwarding optional to avoid build breakage on systems without wlroots-0.18.
tests/CMakeLists.txt (1)
96-100: Missing goggles_input library link already flagged in previous review.The past review comment correctly identifies that the test will fail to link without the goggles_input library.
src/capture/vk_layer/vk_capture.cpp (1)
34-34: Fix potential NULL dereference in LAYER_DEBUG after setenv.Line 34 passes
getenv("DISPLAY")directly tofprintf(via LAYER_DEBUG macro) without checking for NULL. Whilesetenvon line 32 should succeed, defensive coding requires a NULL check to prevent a crash if the environment variable is unexpectedly unset. This issue was flagged in a previous review but remains unresolved in the current code.🔎 Proposed fix
snprintf(display_str, sizeof(display_str), ":%d", response.display_number); setenv("DISPLAY", display_str, 1); - LAYER_DEBUG("layer_early_init: DISPLAY now '%s'", getenv("DISPLAY")); + const char* new_display = getenv("DISPLAY"); + LAYER_DEBUG("layer_early_init: DISPLAY now '%s'", new_display ? new_display : "(unset)"); }Based on static analysis hints and past review comments.
openspec/changes/add-input-forwarding/proposal.md (2)
97-97: Add BREAKING marker to IPC handshake change.Past review comment (already raised) noted that the new IPC handshake phase breaks backward compatibility:
- Old layers won't send
config_request.- Old viewers won't understand new message types.
- Protocol sequence fundamentally altered.
This change must be marked as BREAKING per coding guidelines for
openspec/changes/**/proposal.md.🔎 Proposed fix
### Protocol / IPC - New `CaptureMessageType` enum: `config_request`, `input_display_ready` - New message struct: `CaptureConfigRequest` (layer → viewer) - New message struct: `CaptureInputDisplayReady` (viewer → layer, contains display number) -- New IPC handshake phase: config negotiation before frame streaming +- **BREAKING** New IPC handshake phase: config negotiation before frame streaming
102-106: Update method signatures to match implemented API.Past review comment (already raised) noted that the documented method signatures don't match the actual implementation in
src/input/input_forwarder.hpp:Proposal (lines 102-106):
forward_key(uint32_t scancode, bool pressed) -> voidforward_mouse_button(uint8_t button, bool pressed) -> voidforward_mouse_motion(int32_t x, int32_t y) -> voidforward_mouse_wheel(int32_t x, int32_t y) -> voidActual implementation (from input_forwarder.cpp lines 170-247):
forward_key(const SDL_KeyboardEvent& event) -> Result<void>forward_mouse_button(const SDL_MouseButtonEvent& event) -> Result<void>forward_mouse_motion(const SDL_MouseMotionEvent& event) -> Result<void>forward_mouse_wheel(const SDL_MouseWheelEvent& event) -> Result<void>🔎 Proposed fix
### Public API - New class `goggles::input::InputForwarder` (PIMPL pattern) - `init() -> Result<void>` - - `forward_key(uint32_t scancode, bool pressed) -> void` - - `forward_mouse_button(uint8_t button, bool pressed) -> void` - - `forward_mouse_motion(int32_t x, int32_t y) -> void` - - `forward_mouse_wheel(int32_t x, int32_t y) -> void` + - `forward_key(const SDL_KeyboardEvent& event) -> Result<void>` + - `forward_mouse_button(const SDL_MouseButtonEvent& event) -> Result<void>` + - `forward_mouse_motion(const SDL_MouseMotionEvent& event) -> Result<void>` + - `forward_mouse_wheel(const SDL_MouseWheelEvent& event) -> Result<void>` - `display_number() -> int`
🧹 Nitpick comments (4)
src/input/xwayland_server.hpp (1)
21-47: Consider adding brief documentation comments for public API.While the implementation is clean, adding minimal documentation for the public methods would clarify:
- Thread-safety guarantees for
display_number()- Expected call order (
start()beforedisplay_number())- Resource cleanup guarantees in
stop()and destructorExample documentation style
/// Manages a headless Wayland compositor with nested XWayland server. /// Not copyable or movable due to native resource ownership. class XWaylandServer { public: XWaylandServer(); ~XWaylandServer(); // ... deleted operations ... /// Starts the compositor and XWayland server on an available DISPLAY. /// @return Selected display number (e.g., 1 for :1) or error. [[nodiscard]] auto start() -> Result<int>; /// Stops the compositor thread and cleans up wlroots resources. /// Safe to call multiple times. Called automatically by destructor. void stop(); /// Returns the selected display number after start() succeeds. /// @return Display number (e.g., 1 for :1), or -1 if not started. /// Thread-safe to call after start() completes. [[nodiscard]] auto display_number() const -> int { return m_display_number; } private: // ... };docs/input_forwarding.md (1)
19-19: Optional: Add language hints to fenced code blocks for markdown linter compliance.The markdown linter flags these fenced code blocks for missing language specifiers. While the content (ASCII diagrams and pseudo-code flows) doesn't map to a specific language, you can add
textto satisfy the linter:🔎 Example fix for line 19
-``` +```text ┌────────────────────────────────────────────────────────────┐ │ Goggles Application (DISPLAY=:0, viewer window) │Apply similar changes to lines 158, 166, and 174.
Based on static analysis hints.
Also applies to: 158-158, 166-166, 174-174
src/input/input_forwarder.cpp (2)
170-187: Consider batching XFlush calls for multiple events.The current implementation calls
XFlush()after every key event (line 184). For rapid key sequences or simultaneous key presses, this may generate unnecessary X11 protocol round-trips.If input forwarding performance becomes a concern, consider:
- Batching multiple events before flushing.
- Flushing once per SDL event loop iteration instead of per event.
For the initial implementation, the current approach is acceptable and ensures immediate event delivery.
204-220: TODO acknowledged: coordinate mapping required for production use.The TODO comment (lines 212-215) correctly identifies that 1:1 coordinate mapping will be incorrect when viewer and target windows have different sizes. This is documented as a non-goal in the proposal but will be essential for practical use.
The comment references "OpenSpec for coordinate mapping task" — ensure this task is tracked in the appropriate backlog.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
README.mdcmake/Dependencies.cmakedocs/input_forwarding.mdopenspec/changes/add-input-forwarding/design.mdopenspec/changes/add-input-forwarding/proposal.mdopenspec/changes/add-input-forwarding/specs/input-forwarding/spec.mdopenspec/changes/add-input-forwarding/specs/vk-layer-capture/spec.mdopenspec/changes/add-input-forwarding/tasks.mdpackages/wlroots_0_18/pixi.tomlpackages/wlroots_0_18/recipe.yamlpixi.tomlsrc/CMakeLists.txtsrc/app/CMakeLists.txtsrc/app/main.cppsrc/capture/capture_protocol.hppsrc/capture/capture_receiver.cppsrc/capture/capture_receiver.hppsrc/capture/vk_layer/ipc_socket.cppsrc/capture/vk_layer/ipc_socket.hppsrc/capture/vk_layer/vk_capture.cppsrc/input/CMakeLists.txtsrc/input/input_forwarder.cppsrc/input/input_forwarder.hppsrc/input/xwayland_server.cppsrc/input/xwayland_server.hppsrc/util/error.hpptests/CMakeLists.txttests/input/goggles_input_test.cpp
🚧 Files skipped from review as they are similar to previous changes (9)
- src/CMakeLists.txt
- src/capture/vk_layer/ipc_socket.hpp
- packages/wlroots_0_18/pixi.toml
- README.md
- src/app/CMakeLists.txt
- src/capture/capture_receiver.cpp
- packages/wlroots_0_18/recipe.yaml
- pixi.toml
- tests/input/goggles_input_test.cpp
🧰 Additional context used
📓 Path-based instructions (3)
openspec/changes/**/specs/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/changes/**/specs/**/*.md: Write spec deltas using## ADDED|MODIFIED|REMOVED|RENAMED Requirementsformat with at least one#### Scenario:per requirement in spec files
When modifying existing requirements in a MODIFIED delta, paste the full requirement block including the header and all scenarios, as the archiver will replace the entire requirement
Use ADDED for new capabilities that can stand alone; use MODIFIED for changes to existing requirement behavior, scope, or acceptance criteria; use RENAMED for name-only changes
Files:
openspec/changes/add-input-forwarding/specs/input-forwarding/spec.mdopenspec/changes/add-input-forwarding/specs/vk-layer-capture/spec.md
openspec/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/**/*.md: Use#### Scenario:format (4 hashtags) for scenario headers in requirements, not bullets or bold text, with at least one scenario per requirement
Use format- **WHEN** [condition] - **THEN** [expected result]for scenario steps in requirements
Files:
openspec/changes/add-input-forwarding/specs/input-forwarding/spec.mdopenspec/changes/add-input-forwarding/specs/vk-layer-capture/spec.mdopenspec/changes/add-input-forwarding/design.mdopenspec/changes/add-input-forwarding/proposal.mdopenspec/changes/add-input-forwarding/tasks.md
openspec/changes/**/proposal.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
List spec deltas in
proposal.mdunder 'What Changes' section, marking breaking changes with BREAKING
Files:
openspec/changes/add-input-forwarding/proposal.md
🧠 Learnings (24)
📚 Learning: 2026-01-02T07:02:32.708Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.708Z
Learning: Applies to docs/config/goggles.toml : Development/testing configuration: `config/goggles.toml` in repository root. Structure should include sections for [capture], [pipeline], [render], and [logging].
Applied to files:
docs/input_forwarding.md
📚 Learning: 2026-01-02T07:02:32.708Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.708Z
Learning: Applies to docs/**/{CMakeLists.txt,conanfile.txt,conanfile.py} : All dependencies must have explicit versions: CPM specifies `VERSION` parameter (Git tag or commit hash), Conan uses versioning in conanfile. Document rationale for version changes in commit messages.
Applied to files:
docs/input_forwarding.md
📚 Learning: 2025-12-29T15:44:30.499Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.499Z
Learning: Applies to openspec/src/capture/vk_layer/**/*.{cpp,hpp} : Do not perform blocking operations or file I/O in capture layer performance-critical paths (e.g., vkQueuePresentKHR)
Applied to files:
docs/input_forwarding.mdopenspec/changes/add-input-forwarding/specs/vk-layer-capture/spec.mdsrc/capture/vk_layer/ipc_socket.cppsrc/app/main.cppopenspec/changes/add-input-forwarding/design.mdopenspec/changes/add-input-forwarding/proposal.mdsrc/capture/vk_layer/vk_capture.cpp
📚 Learning: 2025-12-29T15:44:30.499Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.499Z
Learning: Vulkan capture layer is the primary frame capture method, intercepting rendered frames before presentation for zero-copy access
Applied to files:
docs/input_forwarding.mdopenspec/changes/add-input-forwarding/specs/vk-layer-capture/spec.mdsrc/app/main.cppsrc/capture/vk_layer/vk_capture.cpp
📚 Learning: 2026-01-02T07:02:32.708Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.708Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan layer code in `src/capture/vk_layer/` has strict logging constraints: use only `error` and `critical` levels, no file I/O or blocking operations in hot paths, prefix all logs with `[goggles_vklayer]`, never log in `vkQueuePresentKHR` hot path.
Applied to files:
docs/input_forwarding.mdopenspec/changes/add-input-forwarding/specs/input-forwarding/spec.mdopenspec/changes/add-input-forwarding/specs/vk-layer-capture/spec.mdtests/CMakeLists.txtsrc/app/main.cppopenspec/changes/add-input-forwarding/design.mdopenspec/changes/add-input-forwarding/proposal.mdsrc/capture/vk_layer/vk_capture.cpp
📚 Learning: 2026-01-02T07:02:32.708Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.708Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan layer runs in host application threads (no control). Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR`. Use atomics or locks for thread-safe shared layer state.
Applied to files:
docs/input_forwarding.mdopenspec/changes/add-input-forwarding/specs/vk-layer-capture/spec.mdsrc/app/main.cppsrc/capture/vk_layer/vk_capture.cpp
📚 Learning: 2026-01-02T07:02:32.708Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.708Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Exception - Capture Layer Code (`src/capture/vk_layer/`) is exempt and MUST use the raw Vulkan C API because layer dispatch tables require C function pointers, layer must intercept C API calls from host applications, and minimal dependencies required.
Applied to files:
docs/input_forwarding.mdopenspec/changes/add-input-forwarding/specs/vk-layer-capture/spec.mdsrc/capture/vk_layer/ipc_socket.cppsrc/app/main.cppopenspec/changes/add-input-forwarding/design.mdsrc/capture/vk_layer/vk_capture.cpp
📚 Learning: 2026-01-02T07:02:32.708Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.708Z
Learning: Define a lightweight `Error` struct with `ErrorCode` enum, `std::string message`, and optional `std::source_location` in namespace `goggles`.
Applied to files:
src/util/error.hpp
📚 Learning: 2025-12-29T15:44:30.499Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.499Z
Learning: Applies to openspec/src/capture/vk_layer/**/*.{cpp,hpp} : Capture layer (vk_layer/) MUST use raw Vulkan C API, not vulkan-hpp
Applied to files:
openspec/changes/add-input-forwarding/specs/vk-layer-capture/spec.mdsrc/app/main.cppsrc/capture/vk_layer/vk_capture.cpp
📚 Learning: 2026-01-02T07:02:32.708Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.708Z
Learning: Applies to docs/tests/**/*.cpp : Only non-GPU logic is tested initially: in scope are utility functions, configuration parsing, error handling, pipeline graph logic. Out of scope: Vulkan initialization, GPU resource management, rendering, capture layer.
Applied to files:
tests/CMakeLists.txt
📚 Learning: 2025-12-29T15:44:30.499Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.499Z
Learning: Applies to openspec/tests/**/*.{cpp,hpp} : Focus unit tests on non-GPU logic: utility functions, configuration parsing, and pipeline graph logic
Applied to files:
tests/CMakeLists.txt
📚 Learning: 2025-12-29T15:44:30.499Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.499Z
Learning: Applies to openspec/tests/**/*.{cpp,hpp} : Use Catch2 v3 for unit testing with test files mirroring the `src/` directory structure
Applied to files:
tests/CMakeLists.txt
📚 Learning: 2026-01-02T07:02:32.708Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.708Z
Learning: Applies to docs/tests/**/*.{cpp,hpp} : Use Catch2 v3 as the project-wide testing framework.
Applied to files:
tests/CMakeLists.txt
📚 Learning: 2026-01-02T07:02:32.708Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.708Z
Learning: Applies to docs/**/*.{cpp,hpp} : All `vulkan-hpp` calls returning `vk::Result` must be explicitly checked. Prohibited: masking with `static_cast<void>()`. Use `VK_TRY` macro for early return with `vk::Result`, `GOGGLES_TRY` for Result<T> propagation (expression-style), `GOGGLES_MUST` for internal invariants (aborts on failure).
Applied to files:
tests/CMakeLists.txtsrc/input/xwayland_server.cpp
📚 Learning: 2026-01-02T07:02:32.708Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.708Z
Learning: Applies to docs/**/*.{cpp,hpp} : Define project-specific logging macros (`GOGGLES_LOG_TRACE`, `GOGGLES_LOG_DEBUG`, `GOGGLES_LOG_INFO`, `GOGGLES_LOG_WARN`, `GOGGLES_LOG_ERROR`, `GOGGLES_LOG_CRITICAL`) wrapping spdlog for consistent usage.
Applied to files:
tests/CMakeLists.txt
📚 Learning: 2025-12-29T15:44:30.499Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.499Z
Learning: Applies to openspec/src/render/**/*.{cpp,hpp} : Application code MUST use vulkan-hpp with `VULKAN_HPP_NO_EXCEPTIONS` and dynamic dispatch
Applied to files:
tests/CMakeLists.txtsrc/app/main.cpp
📚 Learning: 2025-12-29T15:44:30.499Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.499Z
Learning: Default to single-threaded render loop on main thread; use central job system (goggles::util::JobSystem) for any task parallelism
Applied to files:
src/app/main.cpp
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Applies to openspec/changes/**/proposal.md : List spec deltas in `proposal.md` under 'What Changes' section, marking breaking changes with **BREAKING**
Applied to files:
openspec/changes/add-input-forwarding/proposal.md
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Applies to openspec/changes/**/specs/**/*.md : Use ADDED for new capabilities that can stand alone; use MODIFIED for changes to existing requirement behavior, scope, or acceptance criteria; use RENAMED for name-only changes
Applied to files:
openspec/changes/add-input-forwarding/proposal.mdopenspec/changes/add-input-forwarding/tasks.md
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Create a change proposal with `proposal.md`, `tasks.md`, and optional `design.md` under `openspec/changes/<change-id>/` before implementing changes to add features, make breaking changes, or update architecture
Applied to files:
openspec/changes/add-input-forwarding/proposal.md
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Applies to openspec/changes/**/specs/**/*.md : Write spec deltas using `## ADDED|MODIFIED|REMOVED|RENAMED Requirements` format with at least one `#### Scenario:` per requirement in spec files
Applied to files:
openspec/changes/add-input-forwarding/proposal.md
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Applies to openspec/changes/**/specs/**/*.md : When modifying existing requirements in a MODIFIED delta, paste the full requirement block including the header and all scenarios, as the archiver will replace the entire requirement
Applied to files:
openspec/changes/add-input-forwarding/proposal.md
📚 Learning: 2026-01-02T07:02:32.708Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.708Z
Learning: Applies to docs/**/*.{cpp,hpp} : Banned patterns: No `bool` returns for operations that can fail in multiple ways, no `-1`/`0`/`nullptr` as error indicators, no ignored return values. Mark all result-returning functions with `[[nodiscard]]`.
Applied to files:
src/input/xwayland_server.cpp
📚 Learning: 2026-01-02T07:02:32.708Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.708Z
Learning: Initialize logger once at application startup on the main thread. Capture layer should initialize logger lazily on first `vkCreateInstance` hook.
Applied to files:
src/capture/vk_layer/vk_capture.cpp
🧬 Code graph analysis (8)
src/capture/capture_receiver.hpp (3)
src/util/unique_fd.hpp (1)
fd(34-34)src/capture/vk_layer/ipc_socket.hpp (1)
data(20-20)src/util/serializer.hpp (2)
data(18-21)data(18-18)
src/capture/vk_layer/ipc_socket.cpp (2)
src/capture/vk_layer/ipc_socket.hpp (1)
response(25-25)src/capture/capture_receiver.hpp (1)
fd(54-54)
src/input/input_forwarder.cpp (3)
src/input/input_forwarder.hpp (8)
InputForwarder(11-11)InputForwarder(13-13)InputForwarder(15-15)InputForwarder(26-26)event(19-19)event(20-20)event(21-21)event(22-22)src/util/error.hpp (2)
make_result_ptr_error(63-64)make_result_ptr(57-57)src/capture/capture_receiver.hpp (2)
display_num(46-46)display_num(46-46)
src/app/main.cpp (3)
src/input/input_forwarder.hpp (4)
event(19-19)event(20-20)event(21-21)event(22-22)src/input/input_forwarder.cpp (2)
create(146-168)create(146-146)src/capture/capture_receiver.hpp (2)
display_num(46-46)display_num(46-46)
src/input/xwayland_server.hpp (2)
src/input/xwayland_server.cpp (2)
XWaylandServer(27-27)XWaylandServer(29-31)src/util/error.hpp (4)
nodiscard(50-54)nodiscard(57-59)nodiscard(62-66)nodiscard(68-104)
src/input/xwayland_server.cpp (3)
src/input/xwayland_server.hpp (4)
XWaylandServer(23-23)XWaylandServer(24-24)XWaylandServer(26-26)XWaylandServer(28-28)src/util/error.hpp (1)
make_error(50-52)src/capture/capture_receiver.hpp (2)
display_num(46-46)display_num(46-46)
src/input/input_forwarder.hpp (2)
src/input/input_forwarder.cpp (2)
InputForwarder(142-142)InputForwarder(144-144)src/input/xwayland_server.hpp (1)
nodiscard(33-33)
src/capture/vk_layer/vk_capture.cpp (2)
src/capture/vk_layer/ipc_socket.hpp (2)
response(25-25)get_layer_socket(35-35)src/capture/vk_layer/ipc_socket.cpp (2)
get_layer_socket(17-20)get_layer_socket(17-17)
🪛 ast-grep (0.40.3)
src/capture/vk_layer/vk_capture.cpp
[warning] 33-33: The getenv function returns NULL on error and this line dereferences the return value without checking for NULL.
Context: LAYER_DEBUG("layer_early_init: DISPLAY now '%s'", getenv("DISPLAY"))
Note: [CWE-476] NULL Pointer Dereference. [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers
(null-library-function-cpp)
🪛 markdownlint-cli2 (0.18.1)
docs/input_forwarding.md
19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
158-158: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
166-166: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
174-174: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and Test (test preset)
- GitHub Check: Static Analysis (clang-tidy)
🔇 Additional comments (36)
src/util/error.hpp (2)
26-27: LGTM: New error codes for input forwarding.The additions
input_init_failedandinput_socket_send_failedfollow the existing enum pattern and support the new input-forwarding subsystem.
94-97: LGTM: Error code string mappings are correct.The string mappings for the new error codes match the enum names and follow the established convention.
openspec/changes/add-input-forwarding/specs/input-forwarding/spec.md (1)
1-200: LGTM: Comprehensive specification following openspec format guidelines.The specification correctly uses:
## ADDED Requirementsformat for the delta section#### Scenario:format (4 hashtags) for all scenario headers- **WHEN** ... - **THEN** ...format for scenario stepsThe requirements comprehensively cover:
- XWayland server lifecycle and resource cleanup order
- Keyboard and mouse event forwarding with proper translation maps
- X11 connection management
- Error handling and graceful degradation
- PIMPL pattern for implementation hiding
- Integration points with main application
- Namespace and module placement conventions
Each requirement includes at least one scenario with clear acceptance criteria.
src/input/xwayland_server.hpp (4)
1-17: LGTM: Proper forward declarations and header structure.The header correctly:
- Uses
#pragma once- Places wlroots forward declarations in
extern "C"block- Includes necessary dependencies (thread, error.hpp)
- Uses NOLINT directives for external library naming conventions
21-29: LGTM: Proper resource management semantics.The class correctly disables copy and move operations, which is appropriate for a class managing native wlroots resources and a compositor thread that cannot be safely transferred.
35-47: LGTM: Thread lifecycle and resource cleanup properly managed.The implementation correctly handles thread termination before destruction:
wl_display_terminate()signals the event loop to exit- Explicit
join()call (withjoinable()check) ensures the thread fully completes before any resource destruction- Wlroots resources are destroyed in reverse initialization order, preventing use-after-free
- Pointers are properly nullified after destruction
The pattern successfully prevents race conditions and resource leaks.
31-33: The thread safety concern is mitigated by the sequential initialization pattern, and the referenced specification does not exist in the repository.The
display_number()accessor does not require additional synchronization given the current usage:
InputForwarder::create()synchronously callsstart()and setsm_display_numberbefore the compositor thread launches (line 111), preventing concurrent writes.- Callers access
display_number()only after initialization completes (e.g., main.cpp line 235), not during concurrent operations.- The class's non-copyable, non-movable design enforces single ownership and sequential lifecycle.
However, the justification citing "specification (line 80-81 in spec.md)" is invalid—spec.md does not exist in the repository. If thread-safety expectations should be documented, consider adding a brief comment. Adding
noexceptis optional but harmless for clarity.src/capture/vk_layer/ipc_socket.cpp (1)
274-324: LGTM! Config handshake implementation is sound.The
request_display_config()method correctly implements the pre-main IPC handshake to negotiate DISPLAY. The 100ms blocking poll (line 306) is acceptable in this constructor-time context, and error handling consistently closes the socket on all failure paths. Not acquiringmutex_is correct since this uses a separate temporary socket (fd) rather than the sharedsocket_fd_.src/capture/capture_receiver.hpp (1)
46-46: LGTM! Clean additions for input display config.The new members integrate cleanly:
set_input_display()provides a simple public setter,handle_config_request()encapsulates the handshake logic, andm_input_display_number = -1correctly defaults to "disabled." No validation is needed in the setter since negative values have semantic meaning.Also applies to: 54-54, 61-61
src/input/CMakeLists.txt (1)
1-27: LGTM! Build configuration for goggles_input is correct.The CMakeLists.txt properly configures the input forwarding library with all required dependencies (wlroots, wayland-server, xkbcommon, libX11, libXtst) and sets
WLR_USE_UNSTABLEfor wlroots API access. The past review comment about missing wlroots pertains topixi.toml(external to this file), not the CMake configuration itself.src/app/main.cpp (4)
19-21: LGTM: Signature change enables optional input forwarding.Changing both
capture_receiverandinput_forwarderto pointers allows them to be optional (nullable), which is appropriate for this design where input forwarding may not be available.
36-68: LGTM: Event forwarding implementation is correct.The event forwarding logic is well-implemented with:
- Consistent null-pointer guards for optional input forwarding
- Appropriate error handling (log errors without crashing the main loop)
- Coverage of all relevant input event types (keyboard, mouse buttons, motion, wheel)
227-242: LGTM: InputForwarder initialization is well-structured.The initialization sequence correctly:
- Uses the factory pattern with proper error handling
- Logs both success and failure cases appropriately
- Propagates the display number to
capture_receiverfor the IPC handshake- Handles the case where
capture_receivermay not be available
244-244: LGTM: Function invocation correctly passes optional components.The call correctly dereferences
vulkan_backend(required) and passes raw pointers for optionalcapture_receiverandinput_forwardercomponents.openspec/changes/add-input-forwarding/tasks.md (1)
1-17: LGTM: Task tracking document is clear and comprehensive.The task breakdown is well-structured with:
- Clear implementation milestones (all completed)
- Reasonable follow-up items for future work
- Validation steps to ensure quality
The documented follow-ups (display validation, coordinate mapping, handshake hardening, optional deps) are appropriate for future iterations.
src/capture/capture_protocol.hpp (3)
20-22: LGTM: Enum values follow existing pattern.The new
config_requestandinput_display_readymessage types are correctly added with sequential values (6, 7) following the existing numbering scheme.
72-78: LGTM: CaptureConfigRequest follows protocol struct pattern.The struct is correctly defined with:
- Default-initialized type field
- Version field for protocol evolution
- Reserved fields for future extensions
- Proper size assertion (16 bytes)
80-86: LGTM: CaptureInputDisplayReady is correctly defined.The struct appropriately:
- Uses
int32_tfordisplay_number(allows negative values for error signaling if needed)- Includes reserved fields for future protocol extensions
- Maintains 16-byte alignment with proper size assertion
src/input/xwayland_server.cpp (4)
27-31: LGTM: RAII pattern correctly implemented.The constructor/destructor pair follows RAII principles with the destructor ensuring cleanup via
stop(), which is idempotent and safe to call multiple times.
33-115: LGTM: Comprehensive initialization with proper error handling.The
start()method is well-implemented with:
- RAII guard ensuring cleanup on any failure path
- Thorough error checking at each initialization step with descriptive messages
- Automatic display number selection (1-9) to avoid conflicts
- Separate compositor thread for event loop processing
- Correct sequencing of wlroots object initialization
117-166: LGTM: Cleanup sequence is correct and safe.The
stop()method properly:
- Terminates the display and joins the compositor thread before destroying objects (prevents use-after-free)
- Follows the correct destruction order (XWayland before compositor, seat before display, etc.)
- Correctly omits explicit destruction of
m_xdg_shellandm_compositoras they're managed by wlroots- Is idempotent with early return check
- Nulls out all pointers after cleanup
21-22: Forward declaration forwlr_xdg_shellis appropriate and sufficient.The forward declaration at lines 21-22 correctly supports the usage at line 72 (
wlr_xdg_shell_create), where only a pointer is stored and no struct members are accessed. This pattern is valid and intentional, reducing header dependencies. No changes needed.src/input/input_forwarder.hpp (1)
9-32: LGTM: Well-designed PImpl interface with proper error handling.The
InputForwarderclass interface is excellent:
- PImpl pattern appropriately hides wlroots/X11 implementation details
- Factory method enables complex initialization with error reporting
- All result-returning methods are marked
[[nodiscard]](follows project policy)- Non-copyable/non-movable design is appropriate for RAII resource management
- Consistent
Result<void>return type for all forwarding operations enables proper error handlingopenspec/changes/add-input-forwarding/specs/vk-layer-capture/spec.md (2)
42-60: LGTM!The Protocol Message Types requirement is well-structured with clear scenarios covering message type definitions, struct layouts, and numbering scheme. The reserved value range (4-5) for future features is explicitly documented.
62-76: LGTM!The CaptureReceiver Config Handling requirement clearly separates config request handling from frame capture flow. The two-connection model (config then frame) is well-specified.
src/input/input_forwarder.cpp (6)
15-127: LGTM! Keycode translation is appropriately scoped.The SDL→Linux→X11 keycode translation covers common input (letters, numbers, arrows, modifiers) and gracefully handles unmapped keys by returning 0, which
forward_key()checks and no-ops on (lines 176-178). The X11 +8 offset is correct per X11 protocol.
131-140: LGTM! Proper RAII resource cleanup.The Impl destructor correctly closes the X11 display connection. The XWaylandServer member will be cleaned up automatically via its own destructor.
142-144: LGTM! Standard PIMPL pattern.Constructor and destructor follow the PIMPL idiom correctly. The defaulted destructor ensures proper Impl cleanup.
146-168: LGTM! Robust error handling in factory.The
create()method properly handles initialization failures:
- Stops the XWaylandServer if X11 connection fails (line 161).
- Propagates errors via
Result<T>pattern.- RAII ensures cleanup on all paths.
222-247: LGTM! Mouse wheel implementation is correct.The button mapping for wheel events (4=up, 5=down, 6=left, 7=right) correctly follows the X11 protocol. The press-release pair (lines 242-243) properly simulates wheel scroll events.
249-251: LGTM! Simple delegation to XWaylandServer.openspec/changes/add-input-forwarding/proposal.md (1)
1-90: LGTM! Proposal content is well-structured.The problem statement, architecture, benefits, non-goals, alternatives, risks, and success criteria are clearly documented and align with the implementation. The ASCII diagram is properly formatted with a language identifier.
Also applies to: 111-154
openspec/changes/add-input-forwarding/design.md (4)
22-86: LGTM! Layer 2 and 3 definitions match implementation.The InputForwarder and XWaylandServer class definitions correctly show the implemented API with SDL event parameters and Result return types.
88-136: LGTM! IPC protocol extension accurately documented.The message types, structs, and two-phase handshake flow are correctly specified and align with the implementation in capture_protocol.hpp and the spec delta.
215-272: LGTM! Error handling strategy is well-designed.The error handling approach with Result types, proper cleanup on failure, and graceful degradation aligns with the implementation. The auto-retry logic for DISPLAY number selection (lines 244-258) handles socket conflicts appropriately.
273-384: LGTM! Resource management, performance, and testing sections are comprehensive.The RAII patterns, performance characteristics, testing strategy, and future enhancements are well-documented and align with the implementation approach.
| ### Layer 5: Vulkan Layer Integration (Modified) | ||
|
|
||
| **Current**: Layer loads, intercepts `vkQueuePresentKHR`, sends frames | ||
|
|
||
| **New**: Layer performs config handshake in constructor before app initializes | ||
|
|
||
| ```cpp | ||
| __attribute__((constructor(101))) static void | ||
| layer_early_init() | ||
| { | ||
| // Connect to `\0goggles/vkcapture` (best-effort; if unavailable, keep existing DISPLAY) | ||
| int fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0); | ||
| if (fd < 0) return; | ||
| // connect(fd, CAPTURE_SOCKET_PATH, CAPTURE_SOCKET_PATH_LEN) | ||
|
|
||
| CaptureConfigRequest request{}; | ||
| request.type = CaptureMessageType::config_request; | ||
| request.version = 1; | ||
|
|
||
| if (send(fd, &request, sizeof(request), MSG_NOSIGNAL) != sizeof(request)) return; | ||
|
|
||
| CaptureInputDisplayReady response{}; | ||
| // poll(fd, 1, 100) then recv(fd, &response, sizeof(response), 0) | ||
|
|
||
| char display_str[32]; | ||
| snprintf(display_str, sizeof(display_str), ":%d", response.display_number); | ||
| setenv("DISPLAY", display_str, 1); | ||
|
|
||
| close(fd); | ||
| } | ||
| ``` | ||
|
|
||
| **Constructor priority**: `101` (before default `100` for other constructors, ensures DISPLAY set first) | ||
|
|
||
| **Connection lifecycle**: | ||
| - Config connection: Created and destroyed in constructor | ||
| - Frame capture connection: Created later in `on_present()` (existing code unchanged) | ||
|
|
There was a problem hiding this comment.
Fix constructor priority documentation.
Line 170 incorrectly states "Constructor priority: 101 (before default 100 for other constructors)".
This is the same critical error as in the spec file:
- Default constructor priority is 65535, not 100.
- Priority 101 runs much earlier than default constructors, not "before priority 100."
🔎 Proposed fix
-**Constructor priority**: `101` (before default `100` for other constructors, ensures DISPLAY set first)
+**Constructor priority**: `101` (before default `65535` for other constructors, ensures DISPLAY set first)🤖 Prompt for AI Agents
In openspec/changes/add-input-forwarding/design.md around lines 138 to 175,
update the constructor priority documentation: replace the incorrect statement
that default constructor priority is 100 and that 101 runs "before default 100"
with the correct facts — the default constructor priority is 65535 and a
constructor with priority 101 runs much earlier than default constructors;
clarify that a lower numeric priority value causes earlier execution and that
using 101 will run long before typical default constructors. Keep the note that
101 is intended to run early, but correct the numeric default and the wording
about ordering to avoid confusion.
openspec/changes/add-input-forwarding/specs/vk-layer-capture/spec.md
Outdated
Show resolved
Hide resolved
| #include "input/input_forwarder.hpp" | ||
|
|
||
| #include <SDL3/SDL.h> | ||
| #include <chrono> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'std::chrono|chrono::' src/app/main.cppRepository: K1ngst0m/Goggles
Length of output: 42
🏁 Script executed:
cat -n src/app/main.cppRepository: K1ngst0m/Goggles
Length of output: 13420
Remove unused <chrono> include.
The <chrono> header at line 6 is not used anywhere in the file and should be removed.
🤖 Prompt for AI Agents
In src/app/main.cpp around line 6, the #include <chrono> directive is unused;
remove that include line from the file and verify there are no remaining
references to std::chrono or chrono types—if you do need chrono elsewhere,
replace with the specific includes used; otherwise delete the include to clean
up unused headers.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/app/main.cpp (1)
6-6: Remove unused<chrono>include.The
<chrono>header is not used in this file. This was previously flagged but remains unaddressed.🔎 Proposed fix
#include <SDL3/SDL.h> -#include <chrono> #include <cstdio>
🧹 Nitpick comments (1)
docs/input_forwarding.md (1)
19-19: Add language specifiers to fenced code blocks.Markdown best practices recommend specifying a language identifier for fenced code blocks. For ASCII diagrams like these, use
textor leave the identifier empty if the linter accepts it.🔎 Proposed fix
-``` +```text ┌────────────────────────────────────────────────────────────┐ │ Goggles Application (DISPLAY=:0, viewer window) │Apply the same change to the code blocks at lines 144, 152, and 160.
Also applies to: 144-144, 152-152, 160-160
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
config/goggles.tomldocs/input_forwarding.mdopenspec/changes/archive/2026-01-04-add-input-forwarding-x11/design.mdopenspec/changes/archive/2026-01-04-add-input-forwarding-x11/proposal.mdopenspec/changes/archive/2026-01-04-add-input-forwarding-x11/specs/input-forwarding/spec.mdopenspec/changes/archive/2026-01-04-add-input-forwarding-x11/tasks.mdsrc/app/cli.hppsrc/app/main.cppsrc/capture/capture_protocol.hppsrc/capture/capture_receiver.cppsrc/capture/capture_receiver.hppsrc/capture/vk_layer/ipc_socket.cppsrc/util/config.cppsrc/util/config.hpp
💤 Files with no reviewable changes (2)
- src/capture/capture_receiver.hpp
- src/capture/capture_protocol.hpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/capture/vk_layer/ipc_socket.cpp
- src/capture/capture_receiver.cpp
🧰 Additional context used
📓 Path-based instructions (3)
openspec/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/**/*.md: Use#### Scenario:format (4 hashtags) for scenario headers in requirements, not bullets or bold text, with at least one scenario per requirement
Use format- **WHEN** [condition] - **THEN** [expected result]for scenario steps in requirements
Files:
openspec/changes/archive/2026-01-04-add-input-forwarding-x11/tasks.mdopenspec/changes/archive/2026-01-04-add-input-forwarding-x11/proposal.mdopenspec/changes/archive/2026-01-04-add-input-forwarding-x11/design.mdopenspec/changes/archive/2026-01-04-add-input-forwarding-x11/specs/input-forwarding/spec.md
openspec/changes/**/proposal.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
List spec deltas in
proposal.mdunder 'What Changes' section, marking breaking changes with BREAKING
Files:
openspec/changes/archive/2026-01-04-add-input-forwarding-x11/proposal.md
openspec/changes/**/specs/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/changes/**/specs/**/*.md: Write spec deltas using## ADDED|MODIFIED|REMOVED|RENAMED Requirementsformat with at least one#### Scenario:per requirement in spec files
When modifying existing requirements in a MODIFIED delta, paste the full requirement block including the header and all scenarios, as the archiver will replace the entire requirement
Use ADDED for new capabilities that can stand alone; use MODIFIED for changes to existing requirement behavior, scope, or acceptance criteria; use RENAMED for name-only changes
Files:
openspec/changes/archive/2026-01-04-add-input-forwarding-x11/specs/input-forwarding/spec.md
🧠 Learnings (15)
📚 Learning: 2026-01-02T07:02:32.720Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/config/goggles.toml : Development/testing configuration: `config/goggles.toml` in repository root. Structure should include sections for [capture], [pipeline], [render], and [logging].
Applied to files:
config/goggles.tomlsrc/app/main.cpp
📚 Learning: 2026-01-02T07:02:32.720Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/**/{CMakeLists.txt,conanfile.txt,conanfile.py} : All dependencies must have explicit versions: CPM specifies `VERSION` parameter (Git tag or commit hash), Conan uses versioning in conanfile. Document rationale for version changes in commit messages.
Applied to files:
docs/input_forwarding.mdopenspec/changes/archive/2026-01-04-add-input-forwarding-x11/proposal.md
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/capture/vk_layer/**/*.{cpp,hpp} : Do not perform blocking operations or file I/O in capture layer performance-critical paths (e.g., vkQueuePresentKHR)
Applied to files:
docs/input_forwarding.mdsrc/app/main.cpp
📚 Learning: 2026-01-02T07:02:32.720Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan layer code in `src/capture/vk_layer/` has strict logging constraints: use only `error` and `critical` levels, no file I/O or blocking operations in hot paths, prefix all logs with `[goggles_vklayer]`, never log in `vkQueuePresentKHR` hot path.
Applied to files:
docs/input_forwarding.mdopenspec/changes/archive/2026-01-04-add-input-forwarding-x11/specs/input-forwarding/spec.mdsrc/app/main.cpp
📚 Learning: 2026-01-02T07:02:32.720Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan layer runs in host application threads (no control). Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR`. Use atomics or locks for thread-safe shared layer state.
Applied to files:
docs/input_forwarding.mdsrc/util/config.hppsrc/app/main.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Vulkan capture layer is the primary frame capture method, intercepting rendered frames before presentation for zero-copy access
Applied to files:
src/util/config.hppsrc/app/main.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/capture/vk_layer/**/*.{cpp,hpp} : Capture layer (vk_layer/) MUST use raw Vulkan C API, not vulkan-hpp
Applied to files:
src/util/config.hppsrc/app/main.cpp
📚 Learning: 2026-01-02T07:02:32.720Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Exception - Capture Layer Code (`src/capture/vk_layer/`) is exempt and MUST use the raw Vulkan C API because layer dispatch tables require C function pointers, layer must intercept C API calls from host applications, and minimal dependencies required.
Applied to files:
src/util/config.hppsrc/app/main.cpp
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Applies to openspec/changes/**/proposal.md : List spec deltas in `proposal.md` under 'What Changes' section, marking breaking changes with **BREAKING**
Applied to files:
openspec/changes/archive/2026-01-04-add-input-forwarding-x11/proposal.md
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Applies to openspec/changes/**/specs/**/*.md : Use ADDED for new capabilities that can stand alone; use MODIFIED for changes to existing requirement behavior, scope, or acceptance criteria; use RENAMED for name-only changes
Applied to files:
openspec/changes/archive/2026-01-04-add-input-forwarding-x11/proposal.md
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Create a change proposal with `proposal.md`, `tasks.md`, and optional `design.md` under `openspec/changes/<change-id>/` before implementing changes to add features, make breaking changes, or update architecture
Applied to files:
openspec/changes/archive/2026-01-04-add-input-forwarding-x11/proposal.md
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Applies to openspec/changes/**/specs/**/*.md : Write spec deltas using `## ADDED|MODIFIED|REMOVED|RENAMED Requirements` format with at least one `#### Scenario:` per requirement in spec files
Applied to files:
openspec/changes/archive/2026-01-04-add-input-forwarding-x11/proposal.md
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/render/**/*.{cpp,hpp} : Application code MUST use vulkan-hpp with `VULKAN_HPP_NO_EXCEPTIONS` and dynamic dispatch
Applied to files:
src/app/main.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Default to single-threaded render loop on main thread; use central job system (goggles::util::JobSystem) for any task parallelism
Applied to files:
src/app/main.cpp
📚 Learning: 2026-01-02T07:02:32.720Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/**/*.{cpp,hpp} : Configuration loading: Read once at startup (no hot-reload initially). Validate all values after parsing. Provide defaults for all optional values. Fail fast on invalid config (don't silently ignore).
Applied to files:
src/app/main.cpp
🪛 markdownlint-cli2 (0.18.1)
docs/input_forwarding.md
19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
144-144: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
152-152: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
160-160: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Static Analysis (clang-tidy)
- GitHub Check: Build and Test (test preset)
🔇 Additional comments (9)
src/util/config.cpp (1)
43-48: LGTM! Clean configuration parsing.The input table parsing follows the established pattern used for other configuration sections and correctly populates
config.input.forwarding. Type validation is handled implicitly by the toml library, consistent with other boolean fields likerender.vsync.src/app/cli.hpp (2)
14-14: LGTM! Appropriate default value.The new
enable_input_forwardingfield defaults tofalse, ensuring opt-in behavior that maintains backward compatibility with existing usage.
31-32: LGTM! Clear CLI integration.The
--input-forwardingflag is well-documented with a concise description of the underlying mechanism (nested XWayland + XTest injection), helping users understand what they're enabling.src/util/config.hpp (1)
38-40: LGTM! Consistent configuration structure.The new
Config::Inputstruct follows the established pattern used by other configuration sections, with an explicit default value that matches the TOML default.config/goggles.toml (1)
12-19: LGTM! Well-documented configuration.The new
[input]section includes clear, comprehensive documentation that explains what the feature does (nested XWayland + XTest forwarding), when it applies, and why the default isfalse. This helps users make informed decisions about enabling the feature.openspec/changes/archive/2026-01-04-add-input-forwarding-x11/tasks.md (1)
1-6: LGTM! Comprehensive task tracking.All implementation tasks are marked complete, covering the key aspects of the input forwarding feature: module creation, InputForwarder implementation, SDL integration, testing, opt-in configuration, and documentation. The checklist provides a clear audit trail of the work completed in this PR.
src/app/main.cpp (2)
36-69: LGTM: Input event forwarding implementation.The input forwarding integration is well-implemented with proper null checks, error handling, and logging. The design correctly forwards SDL events only when an InputForwarder instance is available, ensuring graceful behavior when input forwarding is disabled.
228-249: LGTM: InputForwarder initialization with graceful degradation.The initialization logic properly handles errors from
InputForwarder::create(), logs the display number on success, and continues without input forwarding on failure. This aligns with the design goal of graceful degradation.openspec/changes/archive/2026-01-04-add-input-forwarding-x11/design.md (1)
1-309: LGTM: Comprehensive and well-structured design document.This design document is excellent. It clearly explains the layered architecture, provides detailed data flow and threading models, demonstrates proper error handling and resource management patterns, and includes practical performance characteristics and testing strategies. The technical decisions are well-justified with comparisons to alternatives.
| ## What Changes | ||
|
|
||
| ### Configuration / CLI | ||
| - New config option: `input.forwarding` (default: false) | ||
| - New CLI flag: `--input-forwarding` (overrides config to enable) | ||
| - Input forwarding only works for targets launched with `DISPLAY=:N` (nested XWayland) | ||
| - Wayland input forwarding is not supported yet | ||
|
|
||
| ### Public API | ||
| - New class `goggles::input::InputForwarder` (PIMPL pattern) | ||
| - `create() -> ResultPtr<InputForwarder>` | ||
| - `forward_key(const SDL_KeyboardEvent&) -> Result<void>` | ||
| - `forward_mouse_button(const SDL_MouseButtonEvent&) -> Result<void>` | ||
| - `forward_mouse_motion(const SDL_MouseMotionEvent&) -> Result<void>` | ||
| - `forward_mouse_wheel(const SDL_MouseWheelEvent&) -> Result<void>` | ||
| - `display_number() -> int` | ||
| - New class `goggles::input::XWaylandServer` (internal) | ||
| - `start() -> Result<int>` | ||
| - `stop() -> void` | ||
|
|
||
| ### Error Codes | ||
| - `ErrorCode::input_init_failed` - XWayland/compositor startup failure | ||
| - `ErrorCode::input_socket_send_failed` - IPC message send failure | ||
|
|
||
| ### Files Added | ||
| - `src/input/input_forwarder.cpp/hpp` | ||
| - `src/input/xwayland_server.cpp/hpp` | ||
|
|
||
| ### Files Modified | ||
| - `src/app/main.cpp` - InputForwarder instantiation | ||
| - `src/app/cli.hpp` - `--input-forwarding` flag | ||
| - `src/util/config.*` - `input.forwarding` config option | ||
| - `docs/input_forwarding.md` - usage instructions | ||
|
|
There was a problem hiding this comment.
Document protocol/IPC changes in the "What Changes" section.
The PR summary explicitly mentions "IPC extension: small config handshake over existing Unix socket (config_request → input_display_ready) to communicate the selected DISPLAY," but the "What Changes" section does not document these protocol additions. Per coding guidelines for openspec/changes/**/proposal.md, all spec deltas must be listed, and breaking changes must be marked with BREAKING.
The following protocol changes are missing from this section:
- New
CaptureMessageTypeenum values:config_request,input_display_ready - New message structures:
CaptureConfigRequest(layer → viewer),CaptureInputDisplayReady(viewer → layer) - BREAKING New IPC handshake phase: config negotiation before frame streaming (breaks backward compatibility with old layers/viewers)
As per coding guidelines, list spec deltas in proposal.md under 'What Changes' section, marking breaking changes with BREAKING.
🔎 Proposed fix
Add a "Protocol / IPC" subsection after line 95:
- Wayland input forwarding is not supported yet
+### Protocol / IPC
+- New `CaptureMessageType` enum values: `config_request`, `input_display_ready`
+- New message structures: `CaptureConfigRequest` (layer → viewer), `CaptureInputDisplayReady` (viewer → layer, contains display number)
+- **BREAKING** New IPC handshake phase: config negotiation before frame streaming (old layers/viewers incompatible)
+
### Public API
- New class `goggles::input::InputForwarder` (PIMPL pattern)| #### Scenario: Press and release events | ||
| - **WHEN** `SDL_EVENT_KEY_DOWN` is received | ||
| - **THEN** `XTestFakeKeyEvent(..., True, ...)` SHALL be called (key press) | ||
| - **WHEN** `SDL_EVENT_KEY_UP` is received | ||
| - **THEN** `XTestFakeKeyEvent(..., False, ...)` SHALL be called (key release) |
There was a problem hiding this comment.
Split scenario into separate conditions or combine into single WHEN/THEN/AND structure.
The scenario contains two distinct WHEN/THEN pairs (one for KEY_DOWN, one for KEY_UP) which violates the single-WHEN-per-scenario format. Either split into two separate scenarios or restructure with a single WHEN followed by multiple THEN/AND steps.
🔎 Proposed restructure
Option 1: Split into two scenarios:
### Scenario: Press and release events
-#### Scenario: Press and release events
+#### Scenario: Key press event
- **WHEN** `SDL_EVENT_KEY_DOWN` is received
- **THEN** `XTestFakeKeyEvent(..., True, ...)` SHALL be called (key press)
- **WHEN** `SDL_EVENT_KEY_UP` is received
- **THEN** `XTestFakeKeyEvent(..., False, ...)` SHALL be called (key release)
+
+#### Scenario: Key release event
+- **WHEN** `SDL_EVENT_KEY_UP` is received
+- **THEN** `XTestFakeKeyEvent(..., False, ...)` SHALL be called (key release)Option 2: Combine with single WHEN and multiple THEN/AND:
#### Scenario: Press and release events
- **WHEN** `SDL_EVENT_KEY_DOWN` is received
- **THEN** `XTestFakeKeyEvent(..., True, ...)` SHALL be called (key press)
- **WHEN** `SDL_EVENT_KEY_UP` is received
- **THEN** `XTestFakeKeyEvent(..., False, ...)` SHALL be called (key release)
+- **WHEN** a keyboard event (`SDL_EVENT_KEY_DOWN` or `SDL_EVENT_KEY_UP`) is received
+- **THEN** `XTestFakeKeyEvent(...)` SHALL be called with `True` for press and `False` for releaseAs per coding guidelines, ...
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #### Scenario: Press and release events | |
| - **WHEN** `SDL_EVENT_KEY_DOWN` is received | |
| - **THEN** `XTestFakeKeyEvent(..., True, ...)` SHALL be called (key press) | |
| - **WHEN** `SDL_EVENT_KEY_UP` is received | |
| - **THEN** `XTestFakeKeyEvent(..., False, ...)` SHALL be called (key release) | |
| #### Scenario: Press and release events | |
| - **WHEN** a keyboard event (`SDL_EVENT_KEY_DOWN` or `SDL_EVENT_KEY_UP`) is received | |
| - **THEN** `XTestFakeKeyEvent(...)` SHALL be called with `True` for press and `False` for release |
🤖 Prompt for AI Agents
In
openspec/changes/archive/2026-01-04-add-input-forwarding-x11/specs/input-forwarding/spec.md
around lines 53-57, the "Press and release events" scenario contains two
separate WHEN/THEN pairs which violates the single-WHEN-per-scenario guideline;
fix by either splitting this into two distinct scenarios (one for
SDL_EVENT_KEY_DOWN -> XTestFakeKeyEvent(..., True, ...) and one for
SDL_EVENT_KEY_UP -> XTestFakeKeyEvent(..., False, ...)) or by combining into a
single scenario with one WHEN (e.g., "WHEN an SDL key event is received")
followed by multiple THEN/AND steps that assert the press and release calls
respectively; update wording and headings accordingly to reflect the chosen
structure.
| #### Scenario: Wayland-native target | ||
| - **GIVEN** a target application uses a Wayland backend | ||
| - **WHEN** the user attempts to use input forwarding | ||
| - **THEN** input forwarding is not supported for that target (X11-only) |
There was a problem hiding this comment.
Use WHEN/THEN format instead of GIVEN/WHEN/THEN.
This scenario mixes GIVEN and WHEN keywords. Per coding guidelines, scenario steps should use the format - **WHEN** [condition] - **THEN** [expected result]. Rewrite to use WHEN/THEN structure.
🔎 Proposed fix
#### Scenario: Wayland-native target
-- **GIVEN** a target application uses a Wayland backend
-- **WHEN** the user attempts to use input forwarding
-- **THEN** input forwarding is not supported for that target (X11-only)
+- **WHEN** a target application uses a Wayland backend
+- **THEN** input forwarding is not supported for that target (X11-only)As per coding guidelines, ...
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #### Scenario: Wayland-native target | |
| - **GIVEN** a target application uses a Wayland backend | |
| - **WHEN** the user attempts to use input forwarding | |
| - **THEN** input forwarding is not supported for that target (X11-only) | |
| #### Scenario: Wayland-native target | |
| - **WHEN** a target application uses a Wayland backend | |
| - **THEN** input forwarding is not supported for that target (X11-only) |
🤖 Prompt for AI Agents
In
openspec/changes/archive/2026-01-04-add-input-forwarding-x11/specs/input-forwarding/spec.md
around lines 164–167, the scenario mixes GIVEN with WHEN/THEN; rewrite the step
to use the WHEN/THEN format only by replacing the two-step GIVEN + WHEN with a
single WHEN that includes both conditions (e.g., "WHEN the target application
uses a Wayland backend and the user attempts to use input forwarding") and keep
the THEN as the expected result ("THEN input forwarding is not supported for
that target (X11-only)").
| #### Scenario: Common key mappings | ||
| - **GIVEN** SDL_SCANCODE_W (26) | ||
| - **THEN** SHALL translate to KEY_W (17) | ||
| - **GIVEN** SDL_SCANCODE_A (4) | ||
| - **THEN** SHALL translate to KEY_A (30) | ||
| - **GIVEN** SDL_SCANCODE_ESCAPE (41) | ||
| - **THEN** SHALL translate to KEY_ESC (1) | ||
|
|
||
| #### Scenario: Unmapped scancode | ||
| - **WHEN** an SDL scancode has no Linux keycode mapping | ||
| - **THEN** `forward_key()` SHALL return immediately (no-op) | ||
| - **AND** no error SHALL be logged | ||
|
|
||
| #### Scenario: X11 keycode offset | ||
| - **WHEN** translating Linux keycode to X11 keycode | ||
| - **THEN** 8 SHALL be added to the Linux keycode | ||
| - **AND** the resulting X11 keycode SHALL be passed to XTest |
There was a problem hiding this comment.
Restructure "Common key mappings" scenario to follow single WHEN/THEN pattern.
This scenario contains three separate GIVEN/THEN pairs for different SDL scancodes. The guideline format specifies a single WHEN condition per scenario followed by THEN/AND steps. Restructure into either multiple separate scenarios (one per mapping) or combine into one scenario with unified structure.
🔎 Proposed restructure
Option 1: Create separate scenarios for each mapping:
#### Scenario: Common key mappings
-- **GIVEN** SDL_SCANCODE_W (26)
-- **THEN** SHALL translate to KEY_W (17)
-- **GIVEN** SDL_SCANCODE_A (4)
-- **THEN** SHALL translate to KEY_A (30)
-- **GIVEN** SDL_SCANCODE_ESCAPE (41)
-- **THEN** SHALL translate to KEY_ESC (1)
+
+#### Scenario: Map SDL_SCANCODE_W to KEY_W
+- **WHEN** SDL_SCANCODE_W (26) is translated to Linux keycode
+- **THEN** it SHALL map to KEY_W (17)
+
+#### Scenario: Map SDL_SCANCODE_A to KEY_A
+- **WHEN** SDL_SCANCODE_A (4) is translated to Linux keycode
+- **THEN** it SHALL map to KEY_A (30)
+
+#### Scenario: Map SDL_SCANCODE_ESCAPE to KEY_ESC
+- **WHEN** SDL_SCANCODE_ESCAPE (41) is translated to Linux keycode
+- **THEN** it SHALL map to KEY_ESC (1)Option 2: Combine into a single scenario with description:
#### Scenario: Common key mappings
-- **GIVEN** SDL_SCANCODE_W (26)
-- **THEN** SHALL translate to KEY_W (17)
-- **GIVEN** SDL_SCANCODE_A (4)
-- **THEN** SHALL translate to KEY_A (30)
-- **GIVEN** SDL_SCANCODE_ESCAPE (41)
-- **THEN** SHALL translate to KEY_ESC (1)
+- **WHEN** common SDL scancodes are translated to Linux keycodes
+- **THEN** SDL_SCANCODE_W (26) SHALL map to KEY_W (17)
+- **AND** SDL_SCANCODE_A (4) SHALL map to KEY_A (30)
+- **AND** SDL_SCANCODE_ESCAPE (41) SHALL map to KEY_ESC (1)As per coding guidelines, ...
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #### Scenario: Common key mappings | |
| - **GIVEN** SDL_SCANCODE_W (26) | |
| - **THEN** SHALL translate to KEY_W (17) | |
| - **GIVEN** SDL_SCANCODE_A (4) | |
| - **THEN** SHALL translate to KEY_A (30) | |
| - **GIVEN** SDL_SCANCODE_ESCAPE (41) | |
| - **THEN** SHALL translate to KEY_ESC (1) | |
| #### Scenario: Unmapped scancode | |
| - **WHEN** an SDL scancode has no Linux keycode mapping | |
| - **THEN** `forward_key()` SHALL return immediately (no-op) | |
| - **AND** no error SHALL be logged | |
| #### Scenario: X11 keycode offset | |
| - **WHEN** translating Linux keycode to X11 keycode | |
| - **THEN** 8 SHALL be added to the Linux keycode | |
| - **AND** the resulting X11 keycode SHALL be passed to XTest | |
| #### Scenario: Common key mappings | |
| - **WHEN** common SDL scancodes are translated to Linux keycodes | |
| - **THEN** SDL_SCANCODE_W (26) SHALL map to KEY_W (17) | |
| - **AND** SDL_SCANCODE_A (4) SHALL map to KEY_A (30) | |
| - **AND** SDL_SCANCODE_ESCAPE (41) SHALL map to KEY_ESC (1) | |
| #### Scenario: Unmapped scancode | |
| - **WHEN** an SDL scancode has no Linux keycode mapping | |
| - **THEN** `forward_key()` SHALL return immediately (no-op) | |
| - **AND** no error SHALL be logged | |
| #### Scenario: X11 keycode offset | |
| - **WHEN** translating Linux keycode to X11 keycode | |
| - **THEN** 8 SHALL be added to the Linux keycode | |
| - **AND** the resulting X11 keycode SHALL be passed to XTest |
🤖 Prompt for AI Agents
In
openspec/changes/archive/2026-01-04-add-input-forwarding-x11/specs/input-forwarding/spec.md
around lines 173-189, the "Common key mappings" scenario uses multiple
GIVEN/THEN pairs which violates the single WHEN/THEN guideline; rewrite it so
each scenario has one WHEN and corresponding THEN/AND steps by either (A)
splitting into three distinct scenarios (one per mapping: SDL_SCANCODE_W ->
KEY_W, SDL_SCANCODE_A -> KEY_A, SDL_SCANCODE_ESCAPE -> KEY_ESC) or (B) combining
into a single scenario with one WHEN that covers "translating an SDL scancode"
and then three THEN/AND lines that assert each specific mapping, ensuring
consistent WHEN/THEN structure and preserving the mapping details.
1 participant
Proposal: Add Input Forwarding
Why
When Goggles captures frames from a Vulkan application via the layer, users cannot control the captured application by pressing keys in the Goggles viewer window. Input events go to the Goggles window instead of the captured application.
Standard input forwarding approaches fail:
Users currently have no way to interact with captured applications while viewing through Goggles.
Proposed Solution
Introduce an input forwarding module (
src/input/) that creates a nested XWayland server for captured applications and forwards key + mouse events from the Goggles SDL window via XTest injection into the nested server.Architecture
Key insight: XTest injection into XWayland generates real X11 protocol events (KeyPress/KeyRelease), indistinguishable from physical keyboard, bypassing synthetic event filters.
Components
src/input/xwayland_server.cpp/hpp: Manages headless Wayland compositor (wlroots) and spawns XWayland processsrc/input/input_forwarder.cpp/hpp: Public API class (PIMPL pattern), forwards SDL key + mouse events via XTestDISPLAY=:N)Integration Points
src/app/main.cpp: Optionally initializeInputForwarder(viacreate()) and forward SDL input events via XTestdocs/input_forwarding.md: Document how to launch the target inside the nested XWayland session (DISPLAY=:N)Benefits
--input-forwardingNon-Goals
Alternatives Considered
uinput Injection
Rejected: Requires root/uinput group, filtered by Wine
Cage Compositor
Rejected: Full compositor with display output we don't need, complex integration
Direct SDL Input Forwarding
Rejected: No way to inject into different DISPLAY without XTest
What Changes
Configuration / CLI
input.forwarding(default: false)--input-forwarding(overrides config to enable)DISPLAY=:N(nested XWayland)Public API
goggles::input::InputForwarder(PIMPL pattern)create() -> ResultPtr<InputForwarder>forward_key(const SDL_KeyboardEvent&) -> Result<void>forward_mouse_button(const SDL_MouseButtonEvent&) -> Result<void>forward_mouse_motion(const SDL_MouseMotionEvent&) -> Result<void>forward_mouse_wheel(const SDL_MouseWheelEvent&) -> Result<void>display_number() -> intgoggles::input::XWaylandServer(internal)start() -> Result<int>stop() -> voidError Codes
ErrorCode::input_init_failed- XWayland/compositor startup failureErrorCode::input_socket_send_failed- IPC message send failureFiles Added
src/input/input_forwarder.cpp/hppsrc/input/xwayland_server.cpp/hppFiles Modified
src/app/main.cpp- InputForwarder instantiationsrc/app/cli.hpp---input-forwardingflagsrc/util/config.*-input.forwardingconfig optiondocs/input_forwarding.md- usage instructionsDependencies
All system packages (no CPM changes required):
SDL3 already in project.
Risks and Mitigations
Result<void>, log failure, disable input forwardingDISPLAY=:NSuccess Criteria
openspec validate add-input-forwarding --strict