feat(layer): cross-process semaphore export

[BANANASJIM]

Why

Layer uses internal timeline semaphore for GPU sync, with Worker thread waiting on CPU-side before sending DMA-BUF FD. This adds latency and cannot achieve true back-pressure.

What Changes

• Export two timeline semaphores: frame_ready (Layer→Goggles) and frame_consumed (Goggles→Layer)
• Use VK_EXTERNAL_SEMAPHORE_HANDLE_TYPE_OPAQUE_FD_BIT
• Add IPC message types for semaphore FD transfer
• Implement bidirectional GPU sync with back-pressure
• Move semaphores to device level for proper lifecycle management
• Handle reconnection: reset copy_cmds state on sync timeout
• Use UniqueFd for type-safe FD ownership transfer

Testing

• Manual testing with vkcube
• Reconnection scenarios (goggles close/restart, app close/restart)
• Added capture protocol unit tests (8 test cases)

Files Changed

• src/capture/capture_protocol.hpp - New message types
• src/capture/vk_layer/vk_capture.cpp - Semaphore creation and sync
• src/capture/vk_layer/ipc_socket.cpp - FD transfer
• src/capture/capture_receiver.cpp - Receive semaphores
• src/render/backend/vulkan_backend.cpp - Import and use semaphores
• tests/capture/test_capture_protocol.cpp - Protocol tests

Summary by CodeRabbit

• New Features

  • Cross-process GPU synchronization using exportable timeline semaphores with per-frame metadata and FD transfer to enable GPU-to-GPU back-pressure and reduced CPU waits.
  • Presentation now prefers Mailbox when available for lower latency.

• Documentation

  • Added design, proposal, specification, and task docs describing semaphore export, IPC protocol, reconnection and robustness considerations.

• Tests

  • Unit tests validating capture protocol wire formats and message sizes.

• Chores

  • .gitignore updated to exclude local AI-related files.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Implements cross-process timeline semaphore export and IPC transfer: introduces two timeline semaphores (frame_ready, frame_consumed), new IPC messages (semaphore_init, frame_metadata), per-device DeviceSyncState, FD transfer via SCM_RIGHTS, and import/use in the Goggles Vulkan backend.

Changes

Cohort / File(s)	Change Summary
Repo metadata /.gitignore	Ignore .claude/, CLAUDE.md, and AGENTS.md.
Design & planning openspec/.../design.md, openspec/.../proposal.md, openspec/.../spec.md, openspec/.../tasks.md	New design/proposal/spec/tasks detailing two-semaphore model, IPC wire messages, Vulkan extension requirements, reconnection semantics, and testing/robustness tasks.
Protocol definitions src/capture/capture_protocol.hpp	Added semaphore_init and frame_metadata message types and two new wire structs: CaptureSemaphoreInit (16 bytes) and CaptureFrameMetadata (40 bytes).
Capture receiver (IPC read path) src/capture/capture_receiver.hpp, src/capture/capture_receiver.cpp	Buffered recv with SCM_RIGHTS FD extraction, multi-message processing loop, new process_message(...), handling for semaphore_init (consume 2 FDs) and frame_metadata (optional dmabuf FD), added capture frame_number, clear_sync_semaphores(), and recv-buffer state.
Layer IPC (socket send) src/capture/vk_layer/ipc_socket.hpp, src/capture/vk_layer/ipc_socket.cpp	Added send_semaphores(frame_ready_fd, frame_consumed_fd), send_texture_with_fd(metadata, dmabuf_fd), and send_frame_metadata(metadata) to send ancillary FDs via SCM_RIGHTS; preserves existing error handling.
Vulkan capture layer (layer-side sync) src/capture/vk_layer/vk_capture.hpp, src/capture/vk_layer/vk_capture.cpp	Introduced DeviceSyncState and per-device sync lifecycle (init_device_sync, reset_device_sync, cleanup_device_sync, on_device_destroyed), create/export timeline semaphores and export FDs, per-frame use of frame_ready/frame_consumed, and DMA-BUF/frame metadata send logic.
Dispatch & hooks src/capture/vk_layer/vk_dispatch.hpp, src/capture/vk_layer/vk_hooks.cpp	Added PFN_vkGetSemaphoreFdKHR to device dispatch table; require/load VK_KHR_external_semaphore and VK_KHR_external_semaphore_fd at device creation; notify capture manager on device destroy.
Goggles Vulkan backend (import & submit) src/render/backend/vulkan_backend.hpp, src/render/backend/vulkan_backend.cpp	Public API: import_sync_semaphores(...), has_sync_semaphores(), cleanup_sync_semaphores(); import timeline semaphores from FDs, timeline wait on frame_ready, timeline signal frame_consumed with frame numbers, update submit/present flow, and strengthen DMA-BUF import/validation.
App integration src/app/main.cpp	When capture_receiver.semaphores_updated() is true, duplicate FDs and call import_sync_semaphores; handle import failures and clear semaphores_updated.
Tests tests/CMakeLists.txt, tests/capture/test_capture_protocol.cpp	Added unit test verifying CaptureMessageType values and wire struct sizes/defaults (including new structs) and linked goggles_capture into tests.
Minor utilities / wsi src/capture/vk_layer/wsi_virtual.cpp	Small refactor: include <utility> and use std::cmp_less/std::cmp_greater in env parsing (no behavior change).

Sequence Diagram(s)

sequenceDiagram
    participant Layer as Capture Layer
    participant Socket as IPC Socket
    participant Receiver as Goggles Receiver
    participant App as Application
    participant Backend as Vulkan Backend

    rect rgb(240,248,255)
    Note over Layer: Layer-side device sync init
    Layer->>Layer: create two timeline semaphores
    Layer->>Layer: export FDs via vkGetSemaphoreFdKHR
    Layer->>Socket: send_semaphores(fd_ready, fd_consumed) (SCM_RIGHTS)
    Socket->>Receiver: deliver semaphore_init + FDs
    Receiver->>Receiver: process_message -> store FDs, semaphores_updated=true
    end

    rect rgb(255,250,240)
    Note over App,Backend: Goggles import & runtime loop
    App->>Receiver: poll semaphores_updated?
    Receiver-->>App: true + FDs
    App->>App: dup FDs
    App->>Backend: import_sync_semaphores(dup_ready, dup_consumed)
    Backend->>Backend: import as timeline semaphores
    loop per frame
      Backend->>Backend: wait timeline on frame_ready (with timeout)
      Backend->>Backend: render frame
      Backend->>Layer: (implicit) signal frame_consumed with frame number
    end
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• feat(layer): pre-record swapchain image copy commands #14: Touches vk_capture capture_frame and per-frame/copy submission paths — closely related to the per-device sync and per-frame changes here.
• fix(capture): optimize present check to skip init when disconnected #9: Modifies vk_capture on_present/init ordering and IPC connection flow — overlaps with semaphore export/init ordering and IPC behavior.

Suggested reviewers

• K1ngst0m

Poem

🐇 I hopped an FD through socket air,
Two semaphores — a synchronized pair,
Frame ready, frame consumed — we play,
GPUs whisper, then leap away,
A rabbit cheers this synced display!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 6.38% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'feat(layer): cross-process semaphore export' directly and clearly summarizes the main change: implementing cross-process semaphore export for GPU synchronization between Layer and Goggles.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/semaphore-export

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/capture/vk_layer/vk_capture.hpp (1)

75-113: Fix excessive padding in CaptureManager class.

The pipeline failure indicates 71 bytes of padding where only 7 is optimal. Reorder the member fields as suggested by clang-tidy to fix the build error.

🔎 Proposed fix: Reorder fields for optimal padding

 private:
+    // Async worker state
+    util::SPSCQueue<AsyncCaptureItem> async_queue_{16};
+    std::thread worker_thread_;
+
+    // Mutexes and synchronization
     std::mutex mutex_;
+    std::mutex cv_mutex_;
+    std::condition_variable cv_;
+
+    // Data maps
     std::unordered_map<VkSwapchainKHR, SwapData> swaps_;
     std::unordered_map<VkDevice, DeviceSyncState> device_sync_;
 
-    // Async worker state
-    util::SPSCQueue<AsyncCaptureItem> async_queue_{16};
-    std::thread worker_thread_;
     std::atomic<bool> shutdown_{false};
-    std::mutex cv_mutex_;
-    std::condition_variable cv_;

🧹 Nitpick comments (6)

openspec/changes/add-semaphore-export/specs/vk-layer-capture/spec.md (1)

57-61: Consider hyphenating compound modifier (optional).

For consistency with technical writing conventions, consider "Frame-ready signal" instead of "Frame ready signal" when used as a compound modifier.

📝 Suggested edit

-#### Scenario: Frame ready signal
+#### Scenario: Frame-ready signal

openspec/changes/add-semaphore-export/tasks.md (1)

45-45: Reminder: Task 8.2 remains incomplete.

The back-pressure testing with a slow Goggles receiver is marked as incomplete. Consider prioritizing this test to validate the timeout and reconnection behavior under load.

Would you like me to generate a test plan or open an issue to track this remaining test coverage?

src/capture/capture_receiver.cpp (2)

265-280: Semaphore FDs are stored as raw int without RAII wrapper, risking leaks.

Unlike m_frame.dmabuf_fd which uses UniqueFd, the semaphore FDs (m_frame_ready_fd, m_frame_consumed_fd) are stored as raw int. If clear_sync_semaphores() is not called in all error paths, these FDs may leak. Consider using UniqueFd for consistency and safety.

---

283-304: frame_metadata handler logs info on every new DMA-BUF, which may be noisy.

The GOGGLES_LOG_INFO at line 293 logs every time a new DMA-BUF FD is received. In high-frame-rate scenarios, this could flood logs. Consider using GOGGLES_LOG_DEBUG or GOGGLES_LOG_TRACE instead.

🔎 Proposed fix

         if (fd_index < fds.size()) {
             int new_fd = fds[fd_index++];
             m_frame.dmabuf_fd = util::UniqueFd{new_fd};
-            GOGGLES_LOG_INFO("Received new DMA-BUF fd={}", new_fd);
+            GOGGLES_LOG_DEBUG("Received new DMA-BUF fd={}", new_fd);
         }

src/capture/vk_layer/vk_capture.cpp (1)

545-552: Loop iterates all swaps for device, consider using device-keyed swap lookup.

The loop searches all swapchains to find those belonging to a specific device. This O(n) scan is fine for typical use cases (few swapchains) but could be optimized with a device-to-swapchains index if needed in the future.

src/render/backend/vulkan_backend.cpp (1)

1021-1036: Semaphore wait timeout of 100ms may be too aggressive for normal operation.

The 100ms timeout for frame_ready semaphore wait could trigger false positives under heavy system load, causing premature cleanup of sync semaphores. Consider:

1. Using a longer timeout (e.g., 500ms to match the layer side)
2. Adding a retry before cleanup

🔎 Proposed fix - align timeout with layer side

-        constexpr uint64_t TIMEOUT_NS = 100'000'000;
+        constexpr uint64_t TIMEOUT_NS = 500'000'000;  // 500ms, aligned with layer timeout

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d6677db and c5a66ec.

📒 Files selected for processing (19)

• .gitignore
• openspec/changes/add-semaphore-export/design.md
• openspec/changes/add-semaphore-export/proposal.md
• openspec/changes/add-semaphore-export/specs/vk-layer-capture/spec.md
• openspec/changes/add-semaphore-export/tasks.md
• src/app/main.cpp
• src/capture/capture_protocol.hpp
• src/capture/capture_receiver.cpp
• src/capture/capture_receiver.hpp
• src/capture/vk_layer/ipc_socket.cpp
• src/capture/vk_layer/ipc_socket.hpp
• src/capture/vk_layer/vk_capture.cpp
• src/capture/vk_layer/vk_capture.hpp
• src/capture/vk_layer/vk_dispatch.hpp
• src/capture/vk_layer/vk_hooks.cpp
• src/render/backend/vulkan_backend.cpp
• src/render/backend/vulkan_backend.hpp
• tests/CMakeLists.txt
• tests/capture/test_capture_protocol.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-semaphore-export/design.md
• openspec/changes/add-semaphore-export/specs/vk-layer-capture/spec.md
• openspec/changes/add-semaphore-export/tasks.md
• openspec/changes/add-semaphore-export/proposal.md

openspec/changes/**/specs/**/*.md

📄 CodeRabbit inference engine (openspec/AGENTS.md)

openspec/changes/**/specs/**/*.md: Write spec deltas using ## ADDED|MODIFIED|REMOVED|RENAMED Requirements format 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-semaphore-export/specs/vk-layer-capture/spec.md

openspec/changes/**/proposal.md

📄 CodeRabbit inference engine (openspec/AGENTS.md)

List spec deltas in proposal.md under 'What Changes' section, marking breaking changes with BREAKING

Files:

• openspec/changes/add-semaphore-export/proposal.md

🧬 Code graph analysis (8)

src/capture/capture_receiver.cpp (3)

src/util/unique_fd.hpp (1)

• fd (34-34)

src/capture/vk_layer/ipc_socket.hpp (4)

• control (24-24)
• data (20-20)
• metadata (22-22)
• metadata (23-23)

src/capture/capture_receiver.hpp (2)

• data (51-51)
• m_semaphores_updated (43-43)

src/capture/vk_layer/vk_hooks.cpp (3)

src/capture/vk_layer/vk_capture.cpp (2)

• get_capture_manager (115-118)
• get_capture_manager (115-115)

src/capture/vk_layer/vk_capture.hpp (8)

• get_capture_manager (115-115)
• device (80-81)
• device (82-82)
• device (83-83)
• device (90-90)
• device (91-91)
• device (92-92)
• device (99-99)

src/capture/vk_layer/vk_dispatch.hpp (5)

• device (111-111)
• device (114-114)
• device (115-115)
• device (117-117)
• device (120-120)

src/capture/vk_layer/ipc_socket.cpp (1)

src/capture/vk_layer/ipc_socket.hpp (3)

• frame_ready_fd (21-21)
• metadata (22-22)
• metadata (23-23)

src/capture/capture_receiver.hpp (3)

src/render/backend/vulkan_backend.hpp (2)

• nodiscard (38-38)
• nodiscard (44-44)

src/util/unique_fd.hpp (4)

• nodiscard (34-39)
• nodiscard (41-46)
• nodiscard (48-48)
• nodiscard (50-50)

src/capture/vk_layer/ipc_socket.hpp (1)

• data (20-20)

src/capture/vk_layer/vk_capture.hpp (2)

src/capture/vk_layer/vk_dispatch.hpp (7)

• device (111-111)
• device (114-114)
• device (115-115)
• device (117-117)
• device (120-120)
• queue (116-116)
• queue (119-119)

src/render/backend/vulkan_backend.hpp (1)

• image_index (70-70)

src/render/backend/vulkan_backend.cpp (4)

src/render/backend/vulkan_backend.hpp (4)

• frame (35-35)
• frame (62-62)
• frame_ready_fd (42-43)
• image_index (70-70)

src/util/error.hpp (1)

• make_error (45-47)

src/capture/vk_layer/ipc_socket.hpp (1)

• frame_ready_fd (21-21)

src/util/config.hpp (1)

• to_string (19-19)

src/capture/vk_layer/ipc_socket.hpp (1)

src/render/backend/vulkan_backend.hpp (1)

• frame_ready_fd (42-43)

src/capture/vk_layer/vk_capture.cpp (3)

src/capture/vk_layer/vk_capture.hpp (7)

• device (80-81)
• device (82-82)
• device (83-83)
• device (90-90)
• device (91-91)
• device (92-92)
• device (99-99)

src/capture/vk_layer/ipc_socket.cpp (2)

• get_layer_socket (16-19)
• get_layer_socket (16-16)

src/capture/vk_layer/ipc_socket.hpp (3)

• get_layer_socket (34-34)
• metadata (22-22)
• metadata (23-23)

🪛 GitHub Actions: CI

src/capture/vk_layer/vk_hooks.cpp

[error] 1-1: Excessive padding in 'class goggles::capture::CaptureManager' (71 padding bytes, where 7 is optimal). Optimal fields order: async_queue_, worker_thread_, mutex_, cv_mutex_, cv_, swaps_, device_sync_, shutdown_, consider reordering the fields or adding explicit padding members [clang-analyzer-optin.performance.Padding,-warnings-as-errors]

src/capture/vk_layer/vk_capture.hpp

[error] 75-90: Excessive padding in 'class goggles::capture::CaptureManager' (71 padding bytes, where 7 is optimal). Optimal fields order: async_queue_, worker_thread_, mutex_, cv_mutex_, cv_, swaps_, device_sync_, shutdown_. clang-tidy performance padding warning treated as error. Failing compile command: "/usr/local/bin/cmake -E __run_co_compile --launcher=/usr/bin/ccache --tidy="/usr/bin/clang-tidy;--header-filter=/home/runner/work/Goggles/Goggles/src/.*;--extra-arg-before=--driver-mode=g++" --source=/home/runner/work/Goggles/Goggles/src/capture/vk_layer/vk_capture.cpp -- /usr/bin/c++ -DGOGGLES_PROJECT_NAME="Goggles" -DGOGGLES_VERSION="0.1.0" -DGOGGLES_VERSION_MAJOR=0 -DGOGGLES_VERSION_MINOR=1 -DGOGGLES_VERSION_PATCH=0 -Dgoggles_vklayer_EXPORTS -I/home/runner/work/Goggles/Goggles/src -I/home/runner/work/Goggles/Goggles/src/capture/vk_layer -g -std=c++20 -fPIC -Wall -Wextra -Wshadow -Wconversion -fsanitize=address -fno-omit-frame-pointer -MD -MT src/capture/CMakeFiles/goggles_vklayer.dir/vk_layer/vk_capture.cpp.o -MF src/capture/CMakeFiles/goggles_vklayer.dir/vk_layer/vk_capture.cpp.o.d -o src/capture/CMakeFiles/goggles_vklayer.dir/vk_layer/vk_capture.cpp.o -c /home/runner/work/Goggles/Goggles/src/capture/vk_layer/vk_capture.cpp".

---

[error] 75-90: Excessive padding in 'class goggles::capture::CaptureManager' (71 padding bytes, where 7 is optimal). Optimal fields order: async_queue_, worker_thread_, mutex_, cv_mutex_, cv_, swaps_, device_sync_, shutdown_. clang-tidy performance padding warning treated as error. Failing compile command: "/usr/local/bin/cmake -E __run_co_compile --launcher=/usr/bin/ccache --tidy="/usr/bin/clang-tidy;--header-filter=/home/runner/work/Goggles/Goggles/src/.*;--extra-arg-before=--driver-mode=g++" --source=/home/runner/work/Goggles/Goggles/src/capture/vk_layer/vk_capture.cpp -- /usr/bin/c++ -DGOGGLES_PROJECT_NAME="Goggles" -DGOGGLES_VERSION="0.1.0" -DGOGGLES_VERSION_MAJOR=0 -DGOGGLES_VERSION_MINOR=1 -DGOGGLES_VERSION_PATCH=0 -Dgoggles_vklayer_EXPORTS -I/home/runner/work/Goggles/Goggles/src -I/home/runner/work/Goggles/Goggles/src/capture/vk_layer -g -std=c++20 -fPIC -Wall -Wextra -Wshadow -Wconversion -fsanitize=address -fno-omit-frame-pointer -MD -MT src/capture/CMakeFiles/goggles_vklayer.dir/vk_layer/vk_capture.cpp.o -MF src/capture/CMakeFiles/goggles_vklayer.dir/vk_layer/vk_capture.cpp.o.d -o src/capture/CMakeFiles/goggles_vklayer.dir/vk_layer/vk_capture.cpp.o -c /home/runner/work/Goggles/Goggles/src/capture/vk_layer/vk_capture.cpp".

🪛 LanguageTool

openspec/changes/add-semaphore-export/specs/vk-layer-capture/spec.md

[grammar] ~57-~57: Use a hyphen to join words.
Context: ... if timeout occurs #### Scenario: Frame ready signal - WHEN the copy comman...

(QB_NEW_EN_HYPHEN)

🔇 Additional comments (39)

.gitignore (1)

87-91: LGTM! Standard IDE ignore entries.

The Claude Code workspace entries are properly added to prevent tracking IDE-specific files.

src/capture/vk_layer/vk_dispatch.hpp (1)

59-60: LGTM! Function pointer properly placed.

The GetSemaphoreFdKHR function pointer is correctly added alongside other external memory/semaphore functions and is properly loaded in vk_hooks.cpp at line 231.

openspec/changes/add-semaphore-export/specs/vk-layer-capture/spec.md (1)

1-103: Comprehensive spec with proper OpenSpec formatting.

The spec correctly uses the required format with ## ADDED Requirements, ## MODIFIED Requirements, #### Scenario: headers, and WHEN/THEN structure. The cross-process semaphore export requirements are well-documented with clear scenarios.

tests/CMakeLists.txt (2)

33-34: LGTM! Test source properly added.

The new capture protocol test is correctly added to the test sources.

---

49-49: LGTM! Required library linked.

The goggles_capture library is properly linked to enable testing of capture protocol types.

src/capture/vk_layer/vk_hooks.cpp (3)

170-171: LGTM! Required semaphore extensions added.

The external semaphore extensions are correctly added to enable cross-process synchronization via FD export.

---

231-231: LGTM! Function pointer properly loaded.

The GetSemaphoreFdKHR function address is correctly loaded into the dispatch table, corresponding to the declaration in vk_dispatch.hpp at line 60.

---

294-294: LGTM! Device destruction hook correctly placed.

The on_device_destroyed callback is invoked before device cleanup, ensuring the capture manager can properly release device-level synchronization resources while device data is still valid.

tests/capture/test_capture_protocol.cpp (1)

1-73: LGTM! Comprehensive protocol test coverage.

The test suite properly validates:

• Message type enum values for wire format stability
• Struct sizes (critical for IPC compatibility)
• Default initialization values
• Socket path format

This provides good coverage for the new cross-process synchronization protocol.

src/app/main.cpp (1)

49-68: LGTM! Semaphore import logic is sound.

The implementation correctly:

• Cleans up old semaphores before importing new ones
• Uses UniqueFd::dup_from() for proper FD ownership transfer
• Handles errors by clearing receiver-side semaphores
• Logs success/failure appropriately

The placement after poll_frame() ensures semaphores are imported before rendering begins.

src/capture/capture_receiver.hpp (1)

7-7: LGTM! Clean API design for semaphore synchronization.

The additions properly support cross-process synchronization:

• frame_number field enables timeline semaphore tracking
• Public accessors provide read-only access to sync state
• Private members maintain encapsulation
• m_recv_buf supports buffered message processing

The design aligns well with the broader synchronization flow implemented across the PR.

Also applies to: 18-18, 37-44, 51-51, 57-62

src/capture/vk_layer/ipc_socket.hpp (1)

21-23: LGTM: New IPC methods follow existing patterns.

The three new methods extend the IPC interface consistently with the existing send_texture method, using raw FD integers appropriate for SCM_RIGHTS transfer.

src/capture/vk_layer/vk_capture.hpp (2)

33-42: LGTM: DeviceSyncState structure is well-designed.

The per-device synchronization state correctly encapsulates frame-ready and frame-consumed semaphores with their FDs, along with the necessary tracking state for cross-process synchronization.

---

90-99: LGTM: Device-level sync management methods are appropriate.

The transition from init_sync_primitives to init_device_sync with corresponding reset_device_sync, cleanup_device_sync, and get_device_sync methods correctly reflects the shift to per-device synchronization lifecycle management.

openspec/changes/add-semaphore-export/design.md (1)

1-91: LGTM: Design document is comprehensive and well-structured.

The design clearly articulates the two-semaphore synchronization model, protocol changes, required extensions, and risk mitigation strategies. The sync flow diagram and protocol structure examples effectively communicate the cross-process synchronization approach.

openspec/changes/add-semaphore-export/proposal.md (1)

1-28: LGTM: Proposal follows the required format.

The proposal correctly includes the "What Changes" section with clear specification deltas. The changes are appropriately identified as additive (no breaking changes marked), and the impact section clearly maps affected specs and code areas.

src/capture/vk_layer/ipc_socket.cpp (3)

138-180: LGTM: send_semaphores correctly implements dual-FD transfer.

The method properly sends two FD handles via SCM_RIGHTS alongside the CaptureSemaphoreInit payload, with consistent error handling that distinguishes between transient (EAGAIN/EWOULDBLOCK) and fatal errors.

---

182-218: LGTM: send_texture_with_fd correctly combines metadata with FD.

The implementation properly sends frame metadata alongside a DMA-BUF FD via SCM_RIGHTS, following the same error-handling pattern as other IPC methods.

---

220-239: LGTM: send_frame_metadata correctly sends metadata without FD.

The method uses plain send() (no ancillary data) for frame metadata, maintaining consistent error handling with the other IPC methods.

src/render/backend/vulkan_backend.hpp (2)

42-45: LGTM: Sync semaphore API uses proper FD ownership semantics.

The import_sync_semaphores method correctly uses util::UniqueFd for RAII-based FD ownership transfer, ensuring no leaks. The companion methods has_sync_semaphores and cleanup_sync_semaphores provide appropriate lifecycle management.

---

123-127: LGTM: Sync state members are appropriately designed.

The member variables correctly track imported semaphores, synchronization state, and frame counters needed for cross-process GPU synchronization.

src/capture/capture_protocol.hpp (2)

18-20: LGTM: New message types extend the protocol cleanly.

The semaphore_init and frame_metadata message types are sequentially numbered and integrate well with the existing protocol enumeration.

---

50-70: LGTM: New protocol structures are well-designed with size validation.

Both CaptureSemaphoreInit (16 bytes) and CaptureFrameMetadata (40 bytes) have appropriate static_assert checks for wire-format stability. The comment clarifying SCM_RIGHTS FD transfer in CaptureSemaphoreInit is helpful, and CaptureFrameMetadata correctly extends the texture metadata with frame_number for timeline synchronization.

src/capture/capture_receiver.cpp (4)

189-192: Unknown message type clears buffer but continues loop, causing infinite loop.

When an unknown message type is encountered, the buffer is cleared but no break follows, causing the while loop to re-evaluate with an empty buffer. However, the break at line 192 is inside the switch statement and only exits the switch, not the while loop. Since msg_size remains 0 after clearing the buffer, the check at line 195 will break out of the while loop correctly.

---

149-160: FD extraction logic is correct and handles multiple FDs properly.

The loop correctly extracts all FDs from ancillary data using CMSG_LEN(0) for header size calculation and memcpy for safe alignment handling.

---

206-209: Closing unused FDs prevents resource leaks.

Good practice to close any FDs that weren't consumed during message processing. This ensures no file descriptor leaks when more FDs are received than expected.

---

120-121: Buffer size is adequate for all defined message types.

The 256-byte buffer is sufficient. The largest message type is CaptureClientHello at 72 bytes, well within capacity. All message types have their sizes enforced via static_assert in capture_protocol.hpp. The persistent buffer pattern (m_recv_buf) handles message fragmentation across multiple recv() calls, so the single-call buffer size does not constrain message reception.

Likely an incorrect or invalid review comment.

src/capture/vk_layer/vk_capture.cpp (6)

502-528: FD export transfers ownership; FDs should not be closed on failure after export.

When GetSemaphoreFdKHR succeeds, Vulkan transfers ownership of the FD to the caller. However, if the second FD export (line 518) fails, the code correctly closes sync.frame_ready_fd at line 521. This is correct behavior.

---

813-818: Semaphore FDs sent only once; reconnection requires reset_device_sync.

The semaphores_sent flag prevents re-sending FDs after reconnection. This is handled correctly via reset_device_sync which resets the flag at line 584. The design ensures FDs are re-exported and re-sent on reconnection.

---

820-836: Back-pressure wait uses 500ms timeout; consider making this configurable.

The 500ms timeout for frame_consumed semaphore wait is reasonable for detecting disconnection but may cause visible stutter if Goggles is temporarily slow. The current approach of resetting sync primitives on timeout is a safe recovery mechanism.

---

838-848: Potential deadlock if cmd.busy and sync semaphores were reset.

After reset_device_sync at line 833, cmd.busy flags are reset (lines 547-550), so line 839's if (cmd.busy) check should be false after a reset. However, there's a subtle issue: if reset_device_sync returns early (sync not found), cmd.busy might still be true while the semaphore is destroyed. The current code at line 723 handles this by checking sync && sync->frame_ready_sem != VK_NULL_HANDLE.

---

886-891: Frame number 1 always sends DMA-BUF FD, ensuring initial frame transfer.

The condition current_frame == 1 || !swap->dmabuf_sent ensures the DMA-BUF FD is sent on the first frame and after any reset. The dmabuf_sent flag is cleared in cleanup_swap_data (line 920), ensuring proper behavior on swapchain recreation.

---

589-593: Device destruction cleanup correctly removes device sync state.

The on_device_destroyed hook properly cleans up sync resources and removes the device from the map. The mutex lock ensures thread safety.

src/render/backend/vulkan_backend.cpp (6)

788-845: Import semaphore logic is correct with proper FD ownership transfer.

The code correctly:

1. Creates timeline semaphores with initial value 0
2. Imports FDs using vkImportSemaphoreFdKHR
3. Releases UniqueFd ownership only after successful import (lines 824, 836)
4. Cleans up both semaphores on any import failure

One consideration: the FD import is done with flags = 0, meaning the semaphore will take permanent ownership. This is correct for this use case.

---

1053-1066: Timeline semaphore signaling logic has correct guard for duplicate signals.

The condition m_last_frame_number > m_last_signaled_frame correctly prevents signaling the same frame number twice, which would be a Vulkan error for timeline semaphores. The m_last_signaled_frame is updated at line 1076 only after successful submission.

---

847-864: waitIdle before destroying semaphores ensures GPU operations complete.

The waitIdle call at line 850 ensures any pending GPU work referencing these semaphores completes before destruction. The cast to void explicitly discards the result, which is acceptable during cleanup.

---

1108-1109: Frame number tracking enables correct timeline semaphore synchronization.

Setting m_last_frame_number at the start of render_frame ensures the value is available for the wait operation in submit_and_present. This establishes the correct frame-to-frame synchronization handshake.

---

686-695: Manual VkMemoryFdPropertiesKHR handling bypasses vulkan.hpp wrapper.

The code uses the raw Vulkan API call via the dispatcher rather than vulkan.hpp's getMemoryFdPropertiesKHR. This is likely intentional to handle the sType initialization explicitly. The error message "Stale DMA-BUF fd, skipping frame" is appropriate for this failure case.

---

771-786: Handle reset after destruction prevents use-after-free.

Setting handles to nullptr after destruction is good defensive practice. The vulkan.hpp destroyImageView, freeMemory, and destroyImage methods are null-safe, so this also protects against double-free if cleanup_imported_image is called multiple times.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

src/render/backend/vulkan_backend.cpp (1)

390-398: Unchecked getSurfacePresentModesKHR result.

The pm_result from getSurfacePresentModesKHR is not validated. While the fallback to FIFO is safe (guaranteed available), the call could fail on device loss.

🔎 Suggested fix

     auto [pm_result, present_modes] = m_physical_device.getSurfacePresentModesKHR(*m_surface);
     vk::PresentModeKHR chosen_mode = vk::PresentModeKHR::eFifo;
-    for (auto mode : present_modes) {
-        if (mode == vk::PresentModeKHR::eMailbox) {
-            chosen_mode = vk::PresentModeKHR::eMailbox;
-            break;
+    if (pm_result == vk::Result::eSuccess) {
+        for (auto mode : present_modes) {
+            if (mode == vk::PresentModeKHR::eMailbox) {
+                chosen_mode = vk::PresentModeKHR::eMailbox;
+                break;
+            }
         }
     }

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7d8dc1f and fdeaac8.

📒 Files selected for processing (4)

• src/app/main.cpp
• src/capture/capture_receiver.cpp
• src/capture/vk_layer/vk_hooks.cpp
• src/render/backend/vulkan_backend.cpp

🚧 Files skipped from review as they are similar to previous changes (1)

• src/capture/vk_layer/vk_hooks.cpp

🧰 Additional context used

🧬 Code graph analysis (2)

src/capture/capture_receiver.cpp (2)

src/capture/capture_receiver.hpp (4)

• data (51-51)
• CaptureReceiver (8-46)
• CaptureReceiver (19-44)
• struct CaptureFrame { (10-17)

src/capture/capture_protocol.hpp (1)

• CAPTURE_SOCKET_PATH (7-49)

src/render/backend/vulkan_backend.cpp (3)

src/render/backend/vulkan_backend.hpp (4)

• frame (35-35)
• frame (62-62)
• frame_ready_fd (42-43)
• image_index (70-70)

src/util/error.hpp (1)

• make_error (45-47)

src/capture/vk_layer/ipc_socket.hpp (1)

• frame_ready_fd (21-21)

⏰ 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

🔇 Additional comments (20)

src/app/main.cpp (3)

14-14: LGTM!

The include is properly added to support the new UniqueFd usage for semaphore FD handling.

---

49-68: LGTM!

The semaphore synchronization handling follows a proper cleanup-before-import pattern for reconnection scenarios. Error paths correctly clear semaphores on the receiver, and UniqueFd ownership is properly transferred via std::move().

---

180-180: LGTM!

The updated log message is more informative and accurately describes the fallback behavior.

src/capture/capture_receiver.cpp (7)

41-45: LGTM!

Good UX improvement—providing a specific error message for EADDRINUSE helps users diagnose the common case of another instance already running.

---

103-107: LGTM!

Correct single-client semantics with proper FD cleanup when rejecting additional connections.

---

125-126: LGTM!

Buffer sizes are appropriately increased to handle the new message types and FD counts (up to 4 FDs).

---

154-165: LGTM!

FD extraction from ancillary data is correctly implemented with proper iteration and safe memory copying.

---

265-286: LGTM!

The semaphore initialization correctly validates FD availability, cleans up existing semaphores, and stores the new FDs. The lifecycle is properly managed through clear_sync_semaphores() called in cleanup paths.

---

314-330: LGTM!

Cleanup functions properly manage FD lifecycle and reset all relevant state including the new receive buffer and semaphore FDs.

---

306-306: No issue found.

The CaptureFrame struct includes a frame_number field (line 18 of capture_receiver.hpp), so the assignment at line 306 is valid.

src/render/backend/vulkan_backend.cpp (10)

30-31: LGTM!

Required extensions for cross-process semaphore import are correctly added.

---

107-107: LGTM!

Proper cleanup ordering—sync semaphores are cleaned up before device destruction.

---

633-635: LGTM!

Good defensive validation of the DMA-BUF FD before import.

---

695-704: LGTM!

Using raw Vulkan types for vkGetMemoryFdPropertiesKHR is appropriate for extension functions, and the error message is now more descriptive.

---

780-794: LGTM!

Explicitly nullifying handles after destruction prevents use-after-free bugs.

---

797-854: LGTM!

The semaphore import logic correctly:

• Cleans up existing semaphores before reimport
• Creates timeline semaphores with proper type info
• Handles failures by cleaning up partially created resources
• Releases UniqueFd ownership after successful import (Vulkan takes ownership)

---

856-873: LGTM!

Cleanup properly waits for idle before destroying semaphores and resets all associated state.

---

885-888: LGTM!

Using the raw dispatcher call for vkAcquireNextImageKHR is consistent with other extension function calls in this file.

---

1030-1086: LGTM!

The timeline semaphore synchronization logic correctly implements:

• Back-pressure via waiting on frame_ready_sem before rendering
• Graceful degradation on timeout (cleans up sync state, assumes layer disconnected)
• Deduplication of signal operations via m_last_signaled_frame tracking
• Proper chaining of TimelineSemaphoreSubmitInfo only when signaling

---

1117-1118: LGTM!

Frame number is correctly captured early in render_frame for use in timeline semaphore synchronization.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/capture/capture_receiver.cpp (1)

288-289: Consider using UniqueFd for semaphore FDs for consistency and safety.

Lines 288-289 store semaphore FDs as raw integers (m_frame_ready_fd, m_frame_consumed_fd), unlike m_frame.dmabuf_fd which uses UniqueFd. This creates inconsistent ownership semantics and increases the risk of double-close or leaks if error paths are added later.

Using UniqueFd would:

• Provide automatic cleanup on destruction
• Make ownership transfer explicit
• Align with the pattern used for DMA-BUF FDs

🔎 Proposed refactor to use UniqueFd

In the header file (capture_receiver.hpp), change the member declarations:

-    int m_frame_ready_fd = -1;
-    int m_frame_consumed_fd = -1;
+    util::UniqueFd m_frame_ready_fd;
+    util::UniqueFd m_frame_consumed_fd;

Then update this code:

         int ready_fd = fds[fd_index++];
         int consumed_fd = fds[fd_index++];

         clear_sync_semaphores();
         m_frame.dmabuf_fd = util::UniqueFd{};
-        m_frame_ready_fd = ready_fd;
-        m_frame_consumed_fd = consumed_fd;
+        m_frame_ready_fd = util::UniqueFd{ready_fd};
+        m_frame_consumed_fd = util::UniqueFd{consumed_fd};
         m_semaphores_updated = true;
-        GOGGLES_LOG_INFO("Received sync semaphores: ready_fd={}, consumed_fd={}",
-                         m_frame_ready_fd, m_frame_consumed_fd);
+        GOGGLES_LOG_INFO("Received sync semaphores: ready_fd={}, consumed_fd={}",
+                         m_frame_ready_fd.get(), m_frame_consumed_fd.get());

And simplify clear_sync_semaphores():

 void CaptureReceiver::clear_sync_semaphores() {
-    if (m_frame_ready_fd >= 0) {
-        close(m_frame_ready_fd);
-        m_frame_ready_fd = -1;
-    }
-    if (m_frame_consumed_fd >= 0) {
-        close(m_frame_consumed_fd);
-        m_frame_consumed_fd = -1;
-    }
+    m_frame_ready_fd.reset();
+    m_frame_consumed_fd.reset();
 }

Note: Verify that UniqueFd supports the required operations (get(), reset(), valid()) based on unique_fd.hpp.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fdeaac8 and b16e8b7.

📒 Files selected for processing (1)

• src/capture/capture_receiver.cpp

🧰 Additional context used

🧬 Code graph analysis (1)

src/capture/capture_receiver.cpp (3)

src/util/unique_fd.hpp (1)

• fd (34-34)

src/capture/vk_layer/ipc_socket.hpp (4)

• control (24-24)
• data (20-20)
• metadata (22-22)
• metadata (23-23)

src/capture/capture_receiver.hpp (2)

• data (51-51)
• m_semaphores_updated (43-43)

⏰ 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
• GitHub Check: Static Analysis (clang-tidy)

🔇 Additional comments (6)

src/capture/capture_receiver.cpp (6)

41-45: LGTM: Enhanced error diagnostics.

The specific handling of EADDRINUSE provides clearer feedback when multiple instances are running.

---

103-107: LGTM: Proper duplicate client handling.

Correctly rejects new connections when already connected and prevents FD leaks.

---

154-223: LGTM: Robust message processing with FD leak prevention.

The upfront FD extraction and cleanup at line 217-220 prevent leaks. The disconnect-on-unknown-message (lines 194-203) properly addresses the past review comment by closing remaining FDs and disconnecting rather than just clearing the buffer.

---

229-237: LGTM: Safe string handling.

The use of strnlen correctly addresses the past review comment about potential buffer over-read of exe_name.

---

333-338: LGTM: Complete cleanup of all frame state.

The addition of m_recv_buf.clear() and clear_sync_semaphores() ensures all frame-related state is reset on disconnection.

---

287-287: Line 287 is in on_frame_metadata(), not a semaphore initialization handler. Review the actual message ordering and dmabuf_fd ownership semantics.

Line 287 unconditionally clears m_frame.dmabuf_fd within the on_frame_metadata() method. If texture_data() is processed first and provides a valid dmabuf fd, and then frame_metadata() arrives without a dmabuf fd, the fd from texture_data is lost before being restored at line 303 (which only sets it if frame_metadata.has_dmabuf_fd() is true).

This suggests either: (1) frame_metadata is expected to be the authoritative source for dmabuf_fd and should always come first or provide this field, or (2) the design should preserve the fd from texture_data if frame_metadata doesn't provide one. The code lacks documentation clarifying which message is responsible for dmabuf_fd and whether message ordering is guaranteed.

Consider adding:

• A comment documenting which message type is responsible for dmabuf_fd ownership
• Conditional logic to preserve the fd if frame_metadata doesn't provide one, or assert that it must, or ensure frame_metadata always arrives first
• Frame identifiers if texture_data and frame_metadata can belong to different logical frames