fix(wsi): export DMA-BUF stride/offset/modifier in wsi proxy mode#44
fix(wsi): export DMA-BUF stride/offset/modifier in wsi proxy mode#44
Conversation
|
Warning Rate limit exceeded@K1ngst0m has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 11 minutes and 43 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds per-image DMA-BUF metadata propagation for WSI-proxy virtual swapchains: discover and store DRM modifiers and per-image offsets/strides, extend virtual-swapchain/frame IPC to include modifier/offset, add offset to CaptureFrame, and use provided offset/modifier during Vulkan DMA-BUF import. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Vulkan App
participant Layer as vk-layer-capture
participant WSI as WSI Virtualizer
participant IPC as Capture Receiver
participant Backend as Vulkan Backend
App->>Layer: vkCreateSwapchainKHR
Layer->>WSI: create virtual swapchain
WSI->>WSI: query_export_modifiers()
WSI->>WSI: create exportable images (DRM modifier tiling if available)
WSI->>Layer: record per-image fd, stride, offset, modifier
App->>Layer: vkQueuePresentKHR
Layer->>WSI: get_frame_data()
WSI-->>Layer: SwapchainFrameData (fd, stride, offset, modifier)
Layer->>IPC: send CaptureTextureData (fd + stride/offset/modifier) via IPC
IPC->>IPC: populate CaptureFrame.offset/stride/modifier
IPC-->>Backend: deliver CaptureFrame
Backend->>Backend: import_dmabuf() using frame.offset and modifier
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
PR Compliance Guide 🔍(Compliance updated until commit 4201f4a)Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label Previous compliance checksCompliance check up to commit aaba692
|
|||||||||||||||||||||||||||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
![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
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/wsi_virtual.cpp (1)
1-1: Address pipeline failure: Code formatting required.The CI pipeline reports that the code needs formatting. Please run
pixi run formatlocally to fix the formatting issues before merging.Based on pipeline failures.
🧹 Nitpick comments (1)
src/capture/vk_layer/wsi_virtual.cpp (1)
221-270: Code duplication: query_export_modifiers appears in multiple files.The
query_export_modifiersfunction (lines 231-270) appears to be duplicated fromsrc/capture/vk_layer/vk_capture.cpp(lines 240-278) based on the relevant code snippets. The signatures differ slightly (required_featuresparameter is added here), but the core logic is similar.Consider extracting this into a shared utility header to avoid maintenance issues and ensure consistency.
♻️ Proposed refactor: Extract to shared utility
Create a new file
src/capture/vk_layer/drm_modifier_utils.hpp:#pragma once #include <vector> #include <vulkan/vulkan.h> #include "vk_dispatch.hpp" namespace goggles::capture { // DRM format modifier constants constexpr uint64_t DRM_FORMAT_MOD_LINEAR = 0; constexpr uint64_t DRM_FORMAT_MOD_INVALID = 0xffffffffffffffULL; struct ModifierInfo { uint64_t modifier; VkFormatFeatureFlags features; uint32_t plane_count; }; // Query DRM format modifiers for a given format std::vector<ModifierInfo> query_export_modifiers( VkPhysicalDevice phys_device, VkFormat format, VkInstFuncs* inst_funcs, VkFormatFeatureFlags required_features = VK_FORMAT_FEATURE_TRANSFER_DST_BIT ); } // namespace goggles::captureThen include this header in both files and remove the duplicate definitions.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
openspec/changes/fix-wsi-proxy-dmabuf-metadata/proposal.mdopenspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/render-pipeline/spec.mdopenspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.mdopenspec/changes/fix-wsi-proxy-dmabuf-metadata/tasks.mdsrc/capture/capture_receiver.cppsrc/capture/capture_receiver.hppsrc/capture/vk_layer/vk_hooks.cppsrc/capture/vk_layer/wsi_virtual.cppsrc/capture/vk_layer/wsi_virtual.hppsrc/render/backend/vulkan_backend.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/fix-wsi-proxy-dmabuf-metadata/proposal.mdopenspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.mdopenspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/render-pipeline/spec.mdopenspec/changes/fix-wsi-proxy-dmabuf-metadata/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/fix-wsi-proxy-dmabuf-metadata/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/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.mdopenspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/render-pipeline/spec.md
🧠 Learnings (15)
📓 Common learnings
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)
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
📚 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:
openspec/changes/fix-wsi-proxy-dmabuf-metadata/proposal.mdopenspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.mdopenspec/changes/fix-wsi-proxy-dmabuf-metadata/tasks.mdsrc/capture/vk_layer/vk_hooks.cppsrc/capture/vk_layer/wsi_virtual.hppsrc/capture/vk_layer/wsi_virtual.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:
openspec/changes/fix-wsi-proxy-dmabuf-metadata/proposal.mdopenspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.mdsrc/capture/vk_layer/vk_hooks.cppsrc/capture/vk_layer/wsi_virtual.hpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer threading: Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR` hot path. Use atomics or locks for thread-safe layer state.
Applied to files:
openspec/changes/fix-wsi-proxy-dmabuf-metadata/proposal.mdopenspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.mdsrc/capture/vk_layer/vk_hooks.cppsrc/capture/vk_layer/wsi_virtual.hpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Capture layer exception: The Vulkan capture layer (`src/capture/vk_layer/`) is exempt and MUST use the raw Vulkan C API because layer dispatch tables require C function pointers and the layer must intercept C API calls.
Applied to files:
openspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.mdsrc/capture/vk_layer/vk_hooks.cppsrc/capture/vk_layer/wsi_virtual.hppsrc/capture/vk_layer/wsi_virtual.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:
openspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.mdsrc/capture/vk_layer/vk_hooks.cppsrc/capture/vk_layer/wsi_virtual.hppsrc/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.053Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.053Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer 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:
openspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.mdsrc/capture/vk_layer/vk_hooks.cppsrc/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Main thread owns: Vulkan instance, device, swapchain lifecycle; queue submission; window events and user input; job coordination. Main thread MUST NOT: block on I/O, perform heavy computation (>1ms), allocate memory in per-frame code paths.
Applied to files:
openspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.mdopenspec/changes/fix-wsi-proxy-dmabuf-metadata/tasks.mdsrc/capture/vk_layer/vk_hooks.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Vulkan resource management: Follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types. Call `device.waitIdle()` or wait on fences before destroying GPU-async resources. Store creation info with resources for debugging/recreation. Never leak Vulkan objects. Member ordering: declare in reverse destruction order (device before instance).
Applied to files:
openspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.mdsrc/render/backend/vulkan_backend.cppsrc/capture/vk_layer/wsi_virtual.hppsrc/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : RAII handle guidelines for Vulkan resources: Use `vk::Unique*` only for appropriate resource types: Instance, Device, Surface (long-lived singletons); Swapchain, Pipelines, Layouts (created once). Use plain `vk::` handles for command buffers (pooled lifetime), per-frame sync primitives (reused), and imported external images (requires explicit sync).
Applied to files:
openspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.mdsrc/capture/vk_layer/wsi_virtual.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/**/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/fix-wsi-proxy-dmabuf-metadata/specs/render-pipeline/spec.mdopenspec/changes/fix-wsi-proxy-dmabuf-metadata/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: 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/fix-wsi-proxy-dmabuf-metadata/specs/render-pipeline/spec.md
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Testing scope: Only non-GPU logic tested initially. In scope: utility functions, configuration parsing, error handling, pipeline graph logic. Out of scope: Vulkan initialization, GPU resource management, rendering, capture layer.
Applied to files:
openspec/changes/fix-wsi-proxy-dmabuf-metadata/tasks.mdsrc/capture/vk_layer/wsi_virtual.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: Run `openspec validate [change-id] --strict` before requesting approval and do not start implementation until the proposal is reviewed and approved
Applied to files:
openspec/changes/fix-wsi-proxy-dmabuf-metadata/tasks.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} : Use `vk::Unique*` for long-lived Vulkan resources; use plain handles for per-frame/GPU-async resources
Applied to files:
src/capture/vk_layer/vk_hooks.cppsrc/render/backend/vulkan_backend.cppsrc/capture/vk_layer/wsi_virtual.hpp
🧬 Code graph analysis (4)
src/capture/vk_layer/vk_hooks.cpp (1)
src/render/backend/vulkan_backend.hpp (3)
frame(38-38)frame(52-53)frame(103-103)
src/render/backend/vulkan_backend.cpp (1)
src/render/backend/vulkan_backend.hpp (3)
frame(38-38)frame(52-53)frame(103-103)
src/capture/capture_receiver.cpp (1)
src/capture/vk_layer/ipc_socket.hpp (2)
metadata(22-22)metadata(23-23)
src/capture/vk_layer/wsi_virtual.cpp (1)
src/capture/vk_layer/vk_capture.cpp (2)
query_export_modifiers(241-279)query_export_modifiers(241-242)
🪛 GitHub Actions: CI
src/capture/vk_layer/wsi_virtual.cpp
[error] 1-1: Code needs formatting. Run 'pixi run format' locally.
🔇 Additional comments (16)
openspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.md (1)
1-28: LGTM! Spec format and content are correct.The spec delta follows the required format with proper section headers, scenario formatting (4 hashtags), and GIVEN/WHEN/THEN structure. The requirements clearly document the DMA-BUF metadata propagation for virtual swapchains, including preference for DRM format modifier tiling, stride/offset capture via
vkGetImageSubresourceLayout, and modifier capture viavkGetImageDrmFormatModifierPropertiesEXT.src/capture/capture_receiver.hpp (1)
16-16: LGTM! Clean addition of offset field to CaptureFrame.The field is properly initialized to 0, has the correct type (
uint32_tmatching Vulkan'sVkSubresourceLayout.offset), and integrates cleanly with the existing DMA-BUF metadata fields.src/capture/vk_layer/vk_hooks.cpp (1)
702-703: LGTM! Correct propagation of per-frame DMA-BUF metadata.The changes replace hardcoded zero values with the actual per-frame offset and modifier from the virtual swapchain. These are simple field copies with no blocking operations, making them safe for the
vkQueuePresentKHRhot path.Based on learnings, no blocking operations are permitted in this hot path.
src/render/backend/vulkan_backend.cpp (1)
740-740: LGTM! Correct use of frame-provided DMA-BUF offset.The change uses the actual offset from the capture layer instead of a hardcoded 0, enabling proper DMA-BUF plane addressing. The offset is validated implicitly by Vulkan during image creation/import, and error handling via
cleanup_imported_image()is already in place for invalid imports.openspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/render-pipeline/spec.md (1)
1-13: LGTM! Spec format and content are correct.The spec delta follows the required format with proper "## ADDED Requirements" header, scenario formatting (4 hashtags), and GIVEN/WHEN/THEN structure. The requirement clearly documents how the render backend SHALL import DMA-BUF textures using the plane layout metadata (stride→rowPitch, offset→offset, modifier→drmFormatModifier) provided by the capture layer.
src/capture/capture_receiver.cpp (2)
254-254: LGTM: Offset propagation in texture_data path.The addition of offset to texture change detection and frame data assignment is correct and consistent with the existing stride/modifier handling.
Also applies to: 261-261
313-313: LGTM: Offset propagation in frame_metadata path.The offset field is correctly assigned from metadata, aligning with the other frame metadata fields.
openspec/changes/fix-wsi-proxy-dmabuf-metadata/proposal.md (1)
1-16: LGTM: Well-structured proposal.The proposal clearly documents the motivation, changes, and impact. The "What Changes" section properly lists spec deltas as required by coding guidelines.
openspec/changes/fix-wsi-proxy-dmabuf-metadata/tasks.md (1)
1-10: LGTM: Comprehensive task tracking.The task document clearly tracks implementation, validation (including specific verification with vkcube), and spec hygiene. All items are marked complete and align with the proposal.
src/capture/vk_layer/wsi_virtual.hpp (2)
31-32: LGTM: Per-image metadata storage.The addition of
offsetsandmodifiersvectors toVirtualSwapchainis consistent with the existing per-image metadata pattern (strides, dmabuf_fds).
44-45: LGTM: Frame data fields.The new
offsetandmodifierfields inSwapchainFrameDatawith zero defaults are appropriate and align with the PR objective to propagate DMA-BUF layout metadata.src/capture/vk_layer/wsi_virtual.cpp (5)
280-298: LGTM: DRM modifier preference with LINEAR fallback.The logic correctly prefers DRM format modifier tiling when suitable modifiers are available, with a graceful fallback to LINEAR tiling. The required features (COLOR_ATTACHMENT | SAMPLED_IMAGE) are appropriate for virtual swapchain usage.
305-324: LGTM: Conditional modifier list attachment and tiling selection.The image creation correctly attaches the modifier list via
pNextonly when using DRM modifier tiling, and appropriately selectsVK_IMAGE_TILING_DRM_FORMAT_MODIFIER_EXTvsVK_IMAGE_TILING_LINEAR.
336-349: LGTM: Per-image modifier query with fallback.The code correctly queries the actual DRM modifier chosen by the driver for each image, with appropriate fallback to
DRM_FORMAT_MOD_INVALIDon failure andDRM_FORMAT_MOD_LINEARwhen not using modifier tiling.
389-389: LGTM: Offset storage.The per-image offset from
VkSubresourceLayoutis correctly stored alongside stride, completing the plane layout metadata.
574-587: LGTM: Frame data validation and return.The bounds checking correctly includes the new
offsetsandmodifiersvectors, and the frame data structure properly returns all DMA-BUF layout metadata.
K1ngst0m
force-pushed
the
dev/wsi-drm-modifier
branch
from
aaba692 to
b5ad44f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/capture/vk_layer/wsi_virtual.cpp (1)
221-270: Well-structured DRM modifier querying with appropriate filtering.The implementation correctly:
- Uses
VK_STRUCTURE_TYPE_DRM_FORMAT_MODIFIER_PROPERTIES_LIST_EXTfor querying available modifiers- Filters modifiers by
required_featuresand single-plane support- Makes the two-pass query pattern to first get count, then populate data
One observation: there's a similar
query_export_modifiersfunction insrc/capture/vk_layer/vk_capture.cpp(lines 240-278 per the relevant snippets). The difference is this version accepts arequired_featuresparameter for flexible filtering while the other hardcodesVK_FORMAT_FEATURE_TRANSFER_DST_BIT. Consider consolidating these into a shared helper to avoid code duplication.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
openspec/changes/fix-wsi-proxy-dmabuf-metadata/proposal.mdopenspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/render-pipeline/spec.mdopenspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.mdopenspec/changes/fix-wsi-proxy-dmabuf-metadata/tasks.mdsrc/capture/capture_receiver.cppsrc/capture/capture_receiver.hppsrc/capture/vk_layer/vk_hooks.cppsrc/capture/vk_layer/wsi_virtual.cppsrc/capture/vk_layer/wsi_virtual.hppsrc/render/backend/vulkan_backend.cpp
✅ Files skipped from review due to trivial changes (2)
- openspec/changes/fix-wsi-proxy-dmabuf-metadata/tasks.md
- openspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/render-pipeline/spec.md
🚧 Files skipped from review as they are similar to previous changes (5)
- openspec/changes/fix-wsi-proxy-dmabuf-metadata/proposal.md
- openspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.md
- src/capture/vk_layer/wsi_virtual.hpp
- src/capture/vk_layer/vk_hooks.cpp
- src/capture/capture_receiver.hpp
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
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)
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
📚 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:
src/capture/vk_layer/wsi_virtual.cppsrc/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Capture layer exception: The Vulkan capture layer (`src/capture/vk_layer/`) is exempt and MUST use the raw Vulkan C API because layer dispatch tables require C function pointers and the layer must intercept C API calls.
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : RAII handle guidelines for Vulkan resources: Use `vk::Unique*` only for appropriate resource types: Instance, Device, Surface (long-lived singletons); Swapchain, Pipelines, Layouts (created once). Use plain `vk::` handles for command buffers (pooled lifetime), per-frame sync primitives (reused), and imported external images (requires explicit sync).
Applied to files:
src/capture/vk_layer/wsi_virtual.cppsrc/render/backend/vulkan_backend.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/capture/vk_layer/wsi_virtual.cppsrc/render/backend/vulkan_backend.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/render/**/*.{cpp,hpp} : Use `vk::Unique*` for long-lived Vulkan resources; use plain handles for per-frame/GPU-async resources
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Main thread owns: Vulkan instance, device, swapchain lifecycle; queue submission; window events and user input; job coordination. Main thread MUST NOT: block on I/O, perform heavy computation (>1ms), allocate memory in per-frame code paths.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Vulkan resource management: Follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types. Call `device.waitIdle()` or wait on fences before destroying GPU-async resources. Store creation info with resources for debugging/recreation. Never leak Vulkan objects. Member ordering: declare in reverse destruction order (device before instance).
Applied to files:
src/render/backend/vulkan_backend.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/render/**/*.{cpp,hpp} : Application code MUST use vulkan-hpp with `VULKAN_HPP_NO_EXCEPTIONS` and dynamic dispatch
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Application code MUST use vulkan-hpp (C++ bindings), NOT raw Vulkan C API. Use `vk::` types (e.g., `vk::Instance`, `vk::Device`). Do not use raw C handles like `VkInstance`, `VkDevice`. Required configuration: `#define VULKAN_HPP_NO_EXCEPTIONS`, `#define VULKAN_HPP_DISPATCH_LOADER_DYNAMIC 1`, `#include <vulkan/vulkan.hpp>`.
Applied to files:
src/render/backend/vulkan_backend.cpp
🧬 Code graph analysis (3)
src/capture/capture_receiver.cpp (1)
src/capture/vk_layer/ipc_socket.hpp (2)
metadata(22-22)metadata(23-23)
src/capture/vk_layer/wsi_virtual.cpp (1)
src/capture/vk_layer/vk_capture.cpp (2)
query_export_modifiers(241-279)query_export_modifiers(241-242)
src/render/backend/vulkan_backend.cpp (1)
src/render/backend/vulkan_backend.hpp (3)
frame(38-38)frame(52-53)frame(103-103)
⏰ 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 (8)
src/capture/capture_receiver.cpp (2)
251-264: LGTM! Offset propagation correctly integrated into texture change detection and frame data.The
offsetfield is properly added to thetexture_changedcomparison alongside the existing fields, andm_frame.offsetis correctly assigned fromtex_data->offset. This maintains consistency with the existing pattern for width, height, format, stride, and modifier.
310-316: LGTM! Frame metadata path correctly propagates the offset field.The
frame_metadatamessage handler now assignsm_frame.offsetfrommetadata->offset, consistent with thetexture_datapath above. Both IPC message types now propagate the DMA-BUF offset symmetrically.src/render/backend/vulkan_backend.cpp (1)
738-748: LGTM! Plane offset now correctly uses the frame-provided value.Using
frame.offsetinstead of a hardcoded0ensures the DMA-BUF plane layout matches the actual memory layout exported by the WSI proxy layer. This is essential for correct sampling of images with non-zero plane offsets, particularly when using DRM format modifiers.src/capture/vk_layer/wsi_virtual.cpp (5)
280-298: LGTM! Modifier tiling decision logic is sound.The code correctly queries modifiers requiring both
COLOR_ATTACHMENT_BITandSAMPLED_IMAGE_BITfeatures, which are appropriate for swapchain images that will be rendered to and sampled by the viewer. The fallback to LINEAR tiling when no suitable modifiers are found ensures compatibility across different GPU/driver combinations.
305-327: LGTM! Modifier list chaining and tiling mode selection are correct.The
VkImageDrmFormatModifierListCreateInfoEXTis properly chained toext_mem_info.pNextonly when using modifier tiling, and the image tiling mode is conditionally set toVK_IMAGE_TILING_DRM_FORMAT_MODIFIER_EXTorVK_IMAGE_TILING_LINEARaccordingly.
336-349: LGTM! Per-image modifier query handles failure gracefully.The code correctly queries the actual DRM modifier chosen by the driver using
GetImageDrmFormatModifierPropertiesEXTafter image creation. UsingDRM_FORMAT_MOD_INVALIDas fallback on query failure is appropriate, as it signals to the viewer that the modifier is unknown.
386-404: LGTM! Per-image offset storage and enhanced logging.The offset is correctly extracted from the subresource layout and stored per-image. The debug log now includes all relevant DMA-BUF metadata (fd, stride, offset, modifier), which aids troubleshooting.
574-588: LGTM! Validation and frame data emission correctly extended.The bounds checking now validates all four per-image vectors (
dmabuf_fds,strides,offsets,modifiers), andSwapchainFrameDatacorrectly includes the newoffsetandmodifierfields.
K1ngst0m
force-pushed
the
dev/wsi-drm-modifier
branch
from
b5ad44f to
d760062
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/capture/vk_layer/wsi_virtual.cpp:
- Line 389: The cast of layout.offset (a VkDeviceSize/uint64_t) to uint32_t in
swap.offsets.push_back(static_cast<uint32_t>(layout.offset)) can truncate large
offsets; add a defensive check that layout.offset <= UINT32_MAX before casting
and handle the overflow case (log an error via your logger, return an error, or
skip the plane) so you never silently truncate, or if truncation is an
acceptable documented assumption, add a clear comment documenting that
layout.offset is guaranteed < 2^32; reference swap.offsets and layout.offset
when locating the change.
🧹 Nitpick comments (1)
src/capture/vk_layer/wsi_virtual.cpp (1)
403-404: Consider logging prefix consistency.The extended logging provides valuable debugging information. However, the log prefix
[goggles-layer]differs from the guideline[goggles_vklayer]mentioned in project policies. While this is not a functional issue, consistency aids log filtering.Based on learnings, capture layer logging should use prefix
[goggles_vklayer].
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
openspec/changes/fix-wsi-proxy-dmabuf-metadata/proposal.mdopenspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/render-pipeline/spec.mdopenspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.mdopenspec/changes/fix-wsi-proxy-dmabuf-metadata/tasks.mdsrc/capture/capture_receiver.cppsrc/capture/capture_receiver.hppsrc/capture/vk_layer/vk_hooks.cppsrc/capture/vk_layer/wsi_virtual.cppsrc/capture/vk_layer/wsi_virtual.hppsrc/render/backend/vulkan_backend.cpp
🚧 Files skipped from review as they are similar to previous changes (7)
- openspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.md
- openspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/render-pipeline/spec.md
- src/capture/vk_layer/vk_hooks.cpp
- src/capture/vk_layer/wsi_virtual.hpp
- src/capture/capture_receiver.cpp
- src/render/backend/vulkan_backend.cpp
- openspec/changes/fix-wsi-proxy-dmabuf-metadata/proposal.md
🧰 Additional context used
📓 Path-based instructions (1)
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/fix-wsi-proxy-dmabuf-metadata/tasks.md
🧠 Learnings (9)
📓 Common learnings
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)
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
📚 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/fix-wsi-proxy-dmabuf-metadata/tasks.md
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Testing scope: Only non-GPU logic tested initially. In scope: utility functions, configuration parsing, error handling, pipeline graph logic. Out of scope: Vulkan initialization, GPU resource management, rendering, capture layer.
Applied to files:
openspec/changes/fix-wsi-proxy-dmabuf-metadata/tasks.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:
openspec/changes/fix-wsi-proxy-dmabuf-metadata/tasks.mdsrc/capture/vk_layer/wsi_virtual.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: Run `openspec validate [change-id] --strict` before requesting approval and do not start implementation until the proposal is reviewed and approved
Applied to files:
openspec/changes/fix-wsi-proxy-dmabuf-metadata/tasks.md
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Main thread owns: Vulkan instance, device, swapchain lifecycle; queue submission; window events and user input; job coordination. Main thread MUST NOT: block on I/O, perform heavy computation (>1ms), allocate memory in per-frame code paths.
Applied to files:
openspec/changes/fix-wsi-proxy-dmabuf-metadata/tasks.md
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Capture layer exception: The Vulkan capture layer (`src/capture/vk_layer/`) is exempt and MUST use the raw Vulkan C API because layer dispatch tables require C function pointers and the layer must intercept C API calls.
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.053Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.053Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer 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:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : RAII handle guidelines for Vulkan resources: Use `vk::Unique*` only for appropriate resource types: Instance, Device, Surface (long-lived singletons); Swapchain, Pipelines, Layouts (created once). Use plain `vk::` handles for command buffers (pooled lifetime), per-frame sync primitives (reused), and imported external images (requires explicit sync).
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
🧬 Code graph analysis (1)
src/capture/vk_layer/wsi_virtual.cpp (2)
src/capture/vk_layer/vk_capture.cpp (2)
query_export_modifiers(258-296)query_export_modifiers(258-259)src/capture/vk_layer/vk_capture.hpp (7)
device(88-89)device(90-90)device(91-91)device(100-100)device(101-101)device(102-102)device(109-109)
⏰ 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 (9)
openspec/changes/fix-wsi-proxy-dmabuf-metadata/tasks.md (1)
1-10: File structure and content look appropriate for a task tracking document.This tasks.md file correctly documents implementation, validation, and spec hygiene checkpoints for the DMA-BUF metadata propagation work. All items are marked complete and properly cross-reference the related changes (modifier tiling, IPC extensions, CaptureFrame offset). The inline code references (backticks for symbols) enhance clarity.
src/capture/capture_receiver.hpp (1)
16-16: LGTM: Clean addition of offset metadata.The new
offsetfield is appropriately typed, default-initialized, and logically positioned alongside other DMA-BUF layout fields (stride, modifier).src/capture/vk_layer/wsi_virtual.cpp (7)
231-270: LGTM: Solid modifier query implementation.The function correctly queries DRM format modifiers using the standard two-pass Vulkan pattern, with appropriate null checks and feature filtering. The single-plane restriction (line 260) is documented and reasonable for the current use case.
280-298: LGTM: Appropriate modifier discovery with clear fallback.The required features (
COLOR_ATTACHMENT_BIT | SAMPLED_IMAGE_BIT) correctly reflect swapchain image requirements. The logging clearly indicates whether DRM modifiers are used or LINEAR fallback is engaged.
305-312: LGTM: Correct conditional DRM modifier list attachment.The DRM modifier list is properly attached to the pNext chain only when modifiers are available, avoiding unnecessary structures in the fallback case.
323-324: LGTM: Correct tiling mode selection.The tiling mode correctly follows the modifier availability, using DRM modifier tiling when modifiers are present and LINEAR otherwise.
574-576: LGTM: Defensive validation extended for new metadata.The validation correctly includes the new
offsetsandmodifiersvectors. While redundant by construction (all vectors populated in the same loop), this defensive check prevents potential out-of-bounds access if future changes break the invariant.
585-586: LGTM: Frame data extended with offset and modifier.The return structure now correctly provides
offsetandmodifierfields alongside existing DMA-BUF metadata, enabling proper import in the viewer.
221-229: LGTM: DRM constants and ModifierInfo struct correctly defined.The local DRM modifier constants correctly match the drm_fourcc.h definitions:
DRM_FORMAT_MOD_LINEAR = 0andDRM_FORMAT_MOD_INVALID = 0xffffffffffffff(representing all 56 reserved bits set). This approach avoids adding an extra header dependency while maintaining correctness. TheModifierInfostruct is a clean data holder for modifier properties.
K1ngst0m
force-pushed
the
dev/wsi-drm-modifier
branch
from
d760062 to
1528223
Compare
WSI proxy virtual swapchain frames need correct DMA-BUF plane layout metadata so the viewer can import/samples them reliably. - Prefer `VK_IMAGE_TILING_DRM_FORMAT_MODIFIER_EXT` for exportable virtual swapchain images when supported - Record per-image `stride`, `offset`, and selected DRM `modifier` (fallback to LINEAR/INVALID as needed) - Plumb `offset`/`modifier` through present IPC into `CaptureFrame` - Use exported `offset` during Vulkan DMA-BUF import
There was a problem hiding this comment.
Actionable comments posted: 1
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/wsi_virtual.cpp (1)
280-405: Fix incorrect modifier fallback, truncation vulnerability, and missing feature requirement
Modifier fallback bug: If
use_modifier_tilingis true butfuncs.GetImageDrmFormatModifierPropertiesEXTis null or fails,modifierincorrectly remainsDRM_FORMAT_MOD_LINEARinstead ofDRM_FORMAT_MOD_INVALID. This misleads the viewer into decoding with the wrong modifier. Initialize asuse_modifier_tiling ? DRM_FORMAT_MOD_INVALID : DRM_FORMAT_MOD_LINEARto signal unknown modifier when tiling was attempted but query failed.Silent truncation:
layout.rowPitchandlayout.offsetareVkDeviceSize(64-bit), cast touint32_twithout bounds checking. Values exceedingUINT32_MAXsilently truncate, corrupting the metadata sent to the viewer. Add range validation before casting and fail cleanly if out of bounds.Missing feature requirement:
required_featureslacksVK_FORMAT_FEATURE_TRANSFER_SRC_BIT, yet the image usage includesVK_IMAGE_USAGE_TRANSFER_SRC_BIT. If the producer uses transfer-src operations, add this feature to the modifier selection criteria.
🧹 Nitpick comments (1)
src/capture/vk_layer/wsi_virtual.cpp (1)
221-270: Avoid helper divergence + ensure extension/device-enable gating is handled somewhere
query_export_modifiers(...)is effectively duplicated logic fromsrc/capture/vk_layer/vk_capture.cpp(with slightly different filtering), which is easy to drift over time. Consider consolidating into one shared helper (even if it’s a small internal.cppshared by the layer) to keep filtering/feature requirements consistent.
Also, this helper only proves the physical device advertises DRM modifiers; it does not proveVK_EXT_image_drm_format_modifieris enabled on the logical device (needed forVK_IMAGE_TILING_DRM_FORMAT_MODIFIER_EXT/ list-create-info / query props). Please ensure the call site gates on device-enabled extension, not just this query.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
openspec/changes/fix-wsi-proxy-dmabuf-metadata/proposal.mdopenspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/render-pipeline/spec.mdopenspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.mdopenspec/changes/fix-wsi-proxy-dmabuf-metadata/tasks.mdsrc/capture/capture_receiver.cppsrc/capture/capture_receiver.hppsrc/capture/vk_layer/vk_capture.cppsrc/capture/vk_layer/vk_capture.hppsrc/capture/vk_layer/vk_hooks.cppsrc/capture/vk_layer/wsi_virtual.cppsrc/capture/vk_layer/wsi_virtual.hppsrc/render/backend/vulkan_backend.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- openspec/changes/fix-wsi-proxy-dmabuf-metadata/tasks.md
- src/capture/vk_layer/wsi_virtual.hpp
- src/capture/capture_receiver.hpp
- openspec/changes/fix-wsi-proxy-dmabuf-metadata/proposal.md
- src/capture/vk_layer/vk_hooks.cpp
- openspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/render-pipeline/spec.md
🧰 Additional context used
📓 Path-based instructions (2)
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/fix-wsi-proxy-dmabuf-metadata/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/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.md
🧠 Learnings (15)
📓 Common learnings
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)
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
📚 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:
src/capture/vk_layer/vk_capture.cppopenspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.mdsrc/capture/vk_layer/vk_capture.hppsrc/capture/vk_layer/wsi_virtual.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/capture/vk_layer/vk_capture.cppsrc/render/backend/vulkan_backend.cppopenspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.mdsrc/capture/vk_layer/vk_capture.hppsrc/capture/vk_layer/wsi_virtual.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/capture/vk_layer/vk_capture.cppopenspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.mdsrc/capture/vk_layer/vk_capture.hpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Capture layer exception: The Vulkan capture layer (`src/capture/vk_layer/`) is exempt and MUST use the raw Vulkan C API because layer dispatch tables require C function pointers and the layer must intercept C API calls.
Applied to files:
src/capture/vk_layer/vk_capture.cppopenspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.mdsrc/capture/vk_layer/vk_capture.hppsrc/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer threading: Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR` hot path. Use atomics or locks for thread-safe layer state.
Applied to files:
src/capture/vk_layer/vk_capture.cppopenspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.mdsrc/capture/vk_layer/vk_capture.hppsrc/capture/vk_layer/wsi_virtual.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/render/**/*.{cpp,hpp} : Use `vk::Unique*` for long-lived Vulkan resources; use plain handles for per-frame/GPU-async resources
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Vulkan resource management: Follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types. Call `device.waitIdle()` or wait on fences before destroying GPU-async resources. Store creation info with resources for debugging/recreation. Never leak Vulkan objects. Member ordering: declare in reverse destruction order (device before instance).
Applied to files:
src/render/backend/vulkan_backend.cppopenspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.mdsrc/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Application code MUST use vulkan-hpp (C++ bindings), NOT raw Vulkan C API. Use `vk::` types (e.g., `vk::Instance`, `vk::Device`). Do not use raw C handles like `VkInstance`, `VkDevice`. Required configuration: `#define VULKAN_HPP_NO_EXCEPTIONS`, `#define VULKAN_HPP_DISPATCH_LOADER_DYNAMIC 1`, `#include <vulkan/vulkan.hpp>`.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Main thread owns: Vulkan instance, device, swapchain lifecycle; queue submission; window events and user input; job coordination. Main thread MUST NOT: block on I/O, perform heavy computation (>1ms), allocate memory in per-frame code paths.
Applied to files:
src/render/backend/vulkan_backend.cppopenspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.md
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : RAII handle guidelines for Vulkan resources: Use `vk::Unique*` only for appropriate resource types: Instance, Device, Surface (long-lived singletons); Swapchain, Pipelines, Layouts (created once). Use plain `vk::` handles for command buffers (pooled lifetime), per-frame sync primitives (reused), and imported external images (requires explicit sync).
Applied to files:
src/render/backend/vulkan_backend.cppsrc/capture/vk_layer/wsi_virtual.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/render/**/*.{cpp,hpp} : Application code MUST use vulkan-hpp with `VULKAN_HPP_NO_EXCEPTIONS` and dynamic dispatch
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.053Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.053Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer 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:
openspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.mdsrc/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : All vulkan-hpp calls returning `vk::Result` must be explicitly checked. Prohibited pattern: `static_cast<void>(device.waitIdle())`. Use macro `VK_TRY(call, ErrorCode, "message")` for vk::Result early return or manual pattern with error handling.
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/**/*.cpp : Error handling macros: Use `VK_TRY` for vk::Result early return in Vulkan code. Use `GOGGLES_TRY` for Result<T> propagation (both as statement and expression). Use `GOGGLES_MUST` for internal invariants that should never fail. Use manual pattern for destructors/cleanup functions where propagation is impossible.
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
🧬 Code graph analysis (4)
src/capture/vk_layer/vk_capture.cpp (3)
src/capture/vk_layer/ipc_socket.hpp (2)
metadata(22-22)metadata(23-23)src/capture/vk_layer/vk_capture.hpp (1)
frame(98-98)src/render/backend/vulkan_backend.hpp (3)
frame(38-38)frame(52-53)frame(103-103)
src/render/backend/vulkan_backend.cpp (1)
src/render/backend/vulkan_backend.hpp (3)
frame(38-38)frame(52-53)frame(103-103)
src/capture/capture_receiver.cpp (1)
src/capture/vk_layer/ipc_socket.hpp (2)
metadata(22-22)metadata(23-23)
src/capture/vk_layer/wsi_virtual.cpp (1)
src/capture/vk_layer/vk_capture.cpp (2)
query_export_modifiers(258-296)query_export_modifiers(258-259)
⏰ 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/capture/capture_receiver.cpp (3)
251-255: LGTM! Offset field added to texture change detection.The addition of
offsetto the texture change detection is correct and consistent with the existing pattern for other metadata fields.
257-264: LGTM! Frame offset correctly populated from texture data.The assignment of
m_frame.offsetfromtex_data->offsetis correctly placed and follows the existing pattern for metadata propagation.
297-318: LGTM! Frame metadata path correctly propagates offset.The assignment of
m_frame.offsetfrommetadata->offsetensures offset propagation through the frame_metadata path, maintaining consistency with the texture_data path.src/render/backend/vulkan_backend.cpp (1)
736-748: LGTM! DMA-BUF plane offset now correctly sourced from frame metadata.The change from hardcoded
0toframe.offsetis critical for proper DMA-BUF import when the exporter uses non-zero plane offsets. This aligns with the PR objective to propagate complete DMA-BUF layout metadata through the WSI proxy path.src/capture/vk_layer/vk_capture.hpp (1)
25-33: LGTM! VirtualFrameInfo extended with DMA-BUF layout metadata.The addition of
offsetandmodifierfields extends the struct to carry complete DMA-BUF plane layout information. Field types and placement are appropriate, and the zero-initialization is correct.Note: This is a public struct extension that affects ABI compatibility.
src/capture/vk_layer/vk_capture.cpp (1)
917-942: LGTM! Virtual frame metadata now includes offset and modifier.The changes ensure that
offsetandmodifierfromVirtualFrameInfoare correctly propagated toCaptureFrameMetadata, replacing the previous hardcoded zero values. This is essential for correct DMA-BUF metadata propagation in the WSI proxy virtual swapchain path.openspec/changes/fix-wsi-proxy-dmabuf-metadata/specs/vk-layer-capture/spec.md (2)
3-15: LGTM! Virtual Swapchain Creation requirement accurately documented.The requirement correctly specifies:
- DMA-BUF exportable image creation
- Preference for DRM format modifier tiling (SHOULD, not SHALL)
- Per-image stride and offset retrieval via
vkGetImageSubresourceLayout- Conditional DRM modifier retrieval when modifier tiling is used
These specifications align with the implementation changes in the capture layer.
17-27: LGTM! Virtual Swapchain Presentation requirement correctly specified.The requirement properly documents the presentation flow:
- DMA-BUF FD transmission to viewer
- Inclusion of
stride,offset, andmodifiermetadata (core PR objective)- Virtual-only presentation (no physical display)
- Standard success return
The specification accurately reflects the implementation changes across the capture and rendering paths.
src/capture/vk_layer/wsi_virtual.cpp (1)
574-588: Frame-data bounds checks + new offset/modifier fields look correctGood: the additional size checks prevent mismatched vector access, and returning
{offset, modifier}alongside stride/fd matches the intended IPC expansion.
| LAYER_DEBUG("Virtual swapchain: using DRM modifier list with %zu modifiers for format %d", | ||
| modifier_list.size(), swap.format); | ||
| } else { | ||
| LAYER_DEBUG("Virtual swapchain: no suitable DRM modifiers found, falling back to LINEAR"); | ||
| } |
There was a problem hiding this comment.
Capture-layer logging policy mismatch (levels/prefix + added verbosity)
The newly-added logs use LAYER_DEBUG/LAYER_WARN with fprintf(stderr, ...) and [goggles-layer] prefix. The project policies for src/capture/vk_layer/**/* require [goggles_vklayer] prefix and only error/critical levels, and prohibit logging in hot paths. Even if these aren’t vkQueuePresentKHR, this change increases layer-side I/O/verbosity and appears out-of-policy. Based on learnings.
Also applies to: 331-347, 403-405
There was a problem hiding this comment.
Actionable comments posted: 3
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/wsi_virtual.cpp (1)
280-325:required_featuresshould includeVK_FORMAT_FEATURE_TRANSFER_SRC_BIT(matches the image usage) anduse_modifier_tilingshould not be true unless we can report the chosen modifier.
As written, you can select a modifier lacking TRANSFER_SRC support (thenCreateImagefails), and you can also end up later exporting an incorrect modifier (see next comment).Proposed fix (tighten feature filter + avoid modifier-tiling when modifier can’t be reported)
- constexpr VkFormatFeatureFlags required_features = - VK_FORMAT_FEATURE_COLOR_ATTACHMENT_BIT | VK_FORMAT_FEATURE_SAMPLED_IMAGE_BIT; + constexpr VkFormatFeatureFlags required_features = + VK_FORMAT_FEATURE_COLOR_ATTACHMENT_BIT | + VK_FORMAT_FEATURE_SAMPLED_IMAGE_BIT | + VK_FORMAT_FEATURE_TRANSFER_SRC_BIT; auto modifiers = query_export_modifiers(dev_data->physical_device, swap.format, &inst_data->funcs, required_features); - bool use_modifier_tiling = !modifiers.empty(); + // If we can't query the selected modifier per-image, we can't safely export metadata to viewer. + bool use_modifier_tiling = + !modifiers.empty() && (funcs.GetImageDrmFormatModifierPropertiesEXT != nullptr);
🤖 Fix all issues with AI agents
In @src/capture/vk_layer/wsi_virtual.cpp:
- Around line 280-299: The new LAYER_DEBUG calls in wsi_virtual.cpp (around the
modifier handling: query_export_modifiers, use_modifier_tiling, modifier_list
and the additional debug at ~415-416) violate the capture-layer logging policy;
replace these debug logs with the layer-approved logging mechanism or remove
them: use the capture layer's approved macro that emits only error/critical
severity and prefixes messages with "[goggles_vklayer]" (or if such a macro does
not exist, create/route via the existing capture-layer logger), and avoid
emitting from hot paths (guard with a one-time/initialized flag or remove the
log entirely).
- Around line 336-350: The code currently defaults modifier to
DRM_FORMAT_MOD_LINEAR even when use_modifier_tiling is true and
funcs.GetImageDrmFormatModifierPropertiesEXT is null; change the logic so that
when use_modifier_tiling is true the initial modifier is DRM_FORMAT_MOD_INVALID
(only set to drmFormatModifier on successful
GetImageDrmFormatModifierPropertiesEXT), otherwise default to
DRM_FORMAT_MOD_LINEAR; update the block around modifier, use_modifier_tiling,
and funcs.GetImageDrmFormatModifierPropertiesEXT to reflect this and then push
the resulting modifier into swap.modifiers.
- Around line 221-270: query_export_modifiers() can return DRM modifiers even
when VK_EXT_image_drm_format_modifier isn't enabled, causing vkCreateImage to
hard-fail when using VK_IMAGE_TILING_DRM_FORMAT_MODIFIER_EXT; fix by either (A)
recording enabled device extensions at CreateDevice (store in VkDeviceData) and
check that VK_EXT_image_drm_format_modifier is present before calling
query_export_modifiers() or selecting VK_IMAGE_TILING_DRM_FORMAT_MODIFIER_EXT,
or (B) catch vkCreateImage failures that indicate the tiling is unsupported and
retry image creation using VK_IMAGE_TILING_LINEAR; update code paths that call
query_export_modifiers(), the image creation logic that sets
VkImageCreateInfo::tiling, and VkDeviceData to include an enabled-extensions
flag so the presence check is available.
🧹 Nitpick comments (1)
src/capture/vk_layer/wsi_virtual.cpp (1)
389-402: Stride/offset overflow guards look good; consider clearingswap.strides/offsets/modifiersindestroy_swapchain_resources()for symmetry.
Not required for correctness today (swap gets erased), but makes partial-failure cleanup less error-prone.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/capture/vk_layer/wsi_virtual.cpp
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
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)
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
📚 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:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Capture layer exception: The Vulkan capture layer (`src/capture/vk_layer/`) is exempt and MUST use the raw Vulkan C API because layer dispatch tables require C function pointers and the layer must intercept C API calls.
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.053Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.053Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer 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:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : All vulkan-hpp calls returning `vk::Result` must be explicitly checked. Prohibited pattern: `static_cast<void>(device.waitIdle())`. Use macro `VK_TRY(call, ErrorCode, "message")` for vk::Result early return or manual pattern with error handling.
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/**/*.cpp : Error handling macros: Use `VK_TRY` for vk::Result early return in Vulkan code. Use `GOGGLES_TRY` for Result<T> propagation (both as statement and expression). Use `GOGGLES_MUST` for internal invariants that should never fail. Use manual pattern for destructors/cleanup functions where propagation is impossible.
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : RAII handle guidelines for Vulkan resources: Use `vk::Unique*` only for appropriate resource types: Instance, Device, Surface (long-lived singletons); Swapchain, Pipelines, Layouts (created once). Use plain `vk::` handles for command buffers (pooled lifetime), per-frame sync primitives (reused), and imported external images (requires explicit sync).
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer threading: Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR` hot path. Use atomics or locks for thread-safe layer state.
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Vulkan resource management: Follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types. Call `device.waitIdle()` or wait on fences before destroying GPU-async resources. Store creation info with resources for debugging/recreation. Never leak Vulkan objects. Member ordering: declare in reverse destruction order (device before instance).
Applied to files:
src/capture/vk_layer/wsi_virtual.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/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.053Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.053Z
Learning: Applies to docs/**/*.{cpp,hpp} : Use standard spdlog severity levels: trace, debug, info, warn, error, critical. Define project-specific logging macros (`GOGGLES_LOG_TRACE`, `GOGGLES_LOG_DEBUG`, etc.) wrapping spdlog.
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.053Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.053Z
Learning: Applies to docs/**/*.cpp : Initialize logger once at application startup for console output (development) or file output (optional). For capture layer, initialize logger lazily on first `vkCreateInstance` hook.
Applied to files:
src/capture/vk_layer/wsi_virtual.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/capture/vk_layer/wsi_virtual.cpp
🧬 Code graph analysis (1)
src/capture/vk_layer/wsi_virtual.cpp (1)
src/capture/vk_layer/vk_capture.cpp (2)
query_export_modifiers(258-296)query_export_modifiers(258-259)
⏰ 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 (1)
src/capture/vk_layer/wsi_virtual.cpp (1)
586-600: Bounds guard + new frame fields (offset/modifier) look consistent.
| // Prefer DRM modifier tiling to keep the format stable (e.g. SRGB) and to export a correct | ||
| // modifier to the viewer. Fall back to LINEAR only if no suitable modifier is available. | ||
| constexpr VkFormatFeatureFlags required_features = | ||
| VK_FORMAT_FEATURE_COLOR_ATTACHMENT_BIT | VK_FORMAT_FEATURE_SAMPLED_IMAGE_BIT; | ||
| auto modifiers = query_export_modifiers(dev_data->physical_device, swap.format, | ||
| &inst_data->funcs, required_features); | ||
|
|
||
| bool use_modifier_tiling = !modifiers.empty(); | ||
| std::vector<uint64_t> modifier_list; | ||
| if (use_modifier_tiling) { | ||
| modifier_list.reserve(modifiers.size()); | ||
| for (const auto& mod : modifiers) { | ||
| modifier_list.push_back(mod.modifier); | ||
| } | ||
| LAYER_DEBUG("Virtual swapchain: using DRM modifier list with %zu modifiers for format %d", | ||
| modifier_list.size(), swap.format); | ||
| } else { | ||
| LAYER_DEBUG("Virtual swapchain: no suitable DRM modifiers found, falling back to LINEAR"); | ||
| } | ||
|
|
There was a problem hiding this comment.
New logging doesn’t match capture-layer logging policy (prefix + allowed severities).
Per learnings, capture layer logs should be error/critical only, prefixed with [goggles_vklayer], and avoid hot paths. The added LAYER_DEBUG(...) calls violate that. Based on learnings, please route through the layer-approved logging macros (or remove these logs).
Also applies to: 415-416
🤖 Prompt for AI Agents
In @src/capture/vk_layer/wsi_virtual.cpp around lines 280 - 299, The new
LAYER_DEBUG calls in wsi_virtual.cpp (around the modifier handling:
query_export_modifiers, use_modifier_tiling, modifier_list and the additional
debug at ~415-416) violate the capture-layer logging policy; replace these debug
logs with the layer-approved logging mechanism or remove them: use the capture
layer's approved macro that emits only error/critical severity and prefixes
messages with "[goggles_vklayer]" (or if such a macro does not exist,
create/route via the existing capture-layer logger), and avoid emitting from hot
paths (guard with a one-time/initialized flag or remove the log entirely).
dd07c97 to
4201f4a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/capture/vk_layer/wsi_virtual.cpp (3)
418-486: LGTM: Comprehensive image creation with robust fallback.The error handling is thorough, with proper cleanup on all failure paths. The fallback from modifier tiling to LINEAR (lines 436-454) provides good resilience.
Minor clarity improvement for fallback path
Line 446 passes
modifier_listtocreate_swapchain_imageeven thoughuse_modifier_tilingis false. While harmless (the parameter is ignored), passing an empty vector would make the intent clearer:create_res = - create_swapchain_image(device, dev_data, swap, false, modifier_list, &image); + create_swapchain_image(device, dev_data, swap, false, {}, &image);
488-531: Consider adding TRANSFER_SRC_BIT to required_features.Lines 498-499 define
required_featuresasCOLOR_ATTACHMENT_BIT | SAMPLED_IMAGE_BIT, but line 301 creates images withCOLOR_ATTACHMENT_BIT | TRANSFER_SRC_BITusage. The mismatch could cause unnecessary fallback to LINEAR tiling even when suitable modifiers are available that support transfers.Recommended: Include TRANSFER_SRC_BIT in required features
constexpr VkFormatFeatureFlags required_features = - VK_FORMAT_FEATURE_COLOR_ATTACHMENT_BIT | VK_FORMAT_FEATURE_SAMPLED_IMAGE_BIT; + VK_FORMAT_FEATURE_COLOR_ATTACHMENT_BIT | VK_FORMAT_FEATURE_SAMPLED_IMAGE_BIT | + VK_FORMAT_FEATURE_TRANSFER_SRC_BIT;This ensures the selected modifiers support all operations the image will perform.
498-501: Consider documenting why SAMPLED_IMAGE_BIT is required.The
required_featuresincludeSAMPLED_IMAGE_BITeven though the image usage (line 301) doesn't includeVK_IMAGE_USAGE_SAMPLED_BIT. This is correct because the viewer imports the DMA-BUF and needs to sample it, but a brief comment would clarify the design intent.Optional: Add explanatory comment
+// COLOR_ATTACHMENT: capture-side rendering; SAMPLED_IMAGE: viewer-side import/sampling; +// TRANSFER_SRC: potential copy operations constexpr VkFormatFeatureFlags required_features = VK_FORMAT_FEATURE_COLOR_ATTACHMENT_BIT | VK_FORMAT_FEATURE_SAMPLED_IMAGE_BIT;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/capture/vk_layer/wsi_virtual.cpp
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
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)
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
📚 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:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Capture layer exception: The Vulkan capture layer (`src/capture/vk_layer/`) is exempt and MUST use the raw Vulkan C API because layer dispatch tables require C function pointers and the layer must intercept C API calls.
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Testing scope: Only non-GPU logic tested initially. In scope: utility functions, configuration parsing, error handling, pipeline graph logic. Out of scope: Vulkan initialization, GPU resource management, rendering, capture layer.
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.053Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.053Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer 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:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : All vulkan-hpp calls returning `vk::Result` must be explicitly checked. Prohibited pattern: `static_cast<void>(device.waitIdle())`. Use macro `VK_TRY(call, ErrorCode, "message")` for vk::Result early return or manual pattern with error handling.
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/**/*.cpp : Error handling macros: Use `VK_TRY` for vk::Result early return in Vulkan code. Use `GOGGLES_TRY` for Result<T> propagation (both as statement and expression). Use `GOGGLES_MUST` for internal invariants that should never fail. Use manual pattern for destructors/cleanup functions where propagation is impossible.
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : RAII handle guidelines for Vulkan resources: Use `vk::Unique*` only for appropriate resource types: Instance, Device, Surface (long-lived singletons); Swapchain, Pipelines, Layouts (created once). Use plain `vk::` handles for command buffers (pooled lifetime), per-frame sync primitives (reused), and imported external images (requires explicit sync).
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Vulkan resource management: Follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types. Call `device.waitIdle()` or wait on fences before destroying GPU-async resources. Store creation info with resources for debugging/recreation. Never leak Vulkan objects. Member ordering: declare in reverse destruction order (device before instance).
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer threading: Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR` hot path. Use atomics or locks for thread-safe layer state.
Applied to files:
src/capture/vk_layer/wsi_virtual.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/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.053Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.053Z
Learning: Applies to docs/**/*.{cpp,hpp} : Use standard spdlog severity levels: trace, debug, info, warn, error, critical. Define project-specific logging macros (`GOGGLES_LOG_TRACE`, `GOGGLES_LOG_DEBUG`, etc.) wrapping spdlog.
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.053Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.053Z
Learning: Applies to docs/**/*.cpp : Initialize logger once at application startup for console output (development) or file output (optional). For capture layer, initialize logger lazily on first `vkCreateInstance` hook.
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.053Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.053Z
Learning: Applies to docs/**/*.cpp : No silent failures. Every error must be either handled or propagated upward. Log errors exactly once at subsystem boundaries (capture → pipeline). Do not log the same error at every level of the call stack. Prefer monadic operations (`.and_then()`, `.or_else()`, `.transform()`) for chaining.
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
🧬 Code graph analysis (1)
src/capture/vk_layer/wsi_virtual.cpp (2)
src/render/backend/vulkan_backend.cpp (2)
find_memory_type(35-43)find_memory_type(35-36)src/capture/vk_layer/vk_capture.cpp (2)
query_export_modifiers(258-296)query_export_modifiers(258-259)
⏰ 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 (6)
src/capture/vk_layer/wsi_virtual.cpp (6)
221-223: LGTM: DRM constants correctly defined locally.The values match drm_fourcc.h (LINEAR=0, INVALID=0xffffffffffffff), and keeping them local appropriately avoids adding header dependencies to the layer build.
231-270: LGTM: Modifier query implementation is correct.The two-phase query pattern and bitwise feature filtering are correct. The single-plane restriction (line 260) is appropriately documented with an inline comment.
272-306: LGTM: Image creation with modifier support is correct.The conditional pNext chaining (lines 283-288) and tiling selection (lines 299-300) properly handle both modifier and LINEAR tiling paths.
308-356: LGTM: Layout and modifier query helpers are well-implemented.The uint32_t bounds validation (lines 341-350) is critical for IPC compatibility, and returning
std::optionalprovides clean error handling.
358-416: LGTM: DMA-BUF export and memory allocation are correctly implemented.The cleanup path in
alloc_exportable_memory(line 409) properly frees memory on bind failure, and the memory type selection prioritizes device-local memory for optimal performance.
697-711: LGTM: Frame data properly extended with offset and modifier.The extended bounds check (lines 697-699) appropriately validates all metadata vectors before access, and the new fields (offset, modifier) are correctly populated from per-image data.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/capture/vk_layer/wsi_virtual.cpp:
- Around line 225-270: The modifier filtering currently only requires
VK_FORMAT_FEATURE_COLOR_ATTACHMENT_BIT and VK_FORMAT_FEATURE_SAMPLED_IMAGE_BIT,
which can miss modifiers that lack transfer support; update the required feature
flags passed into query_export_modifiers (or the constexpr used to build them)
to also include VK_FORMAT_FEATURE_TRANSFER_SRC_BIT so the modifier selection
requires COLOR_ATTACHMENT | SAMPLED_IMAGE | TRANSFER_SRC (use a bitwise OR of
the three flags); change the constant/argument used where query_export_modifiers
is invoked (and any local required_features definitions) to include
VK_FORMAT_FEATURE_TRANSFER_SRC_BIT.
🧹 Nitpick comments (1)
src/render/backend/vulkan_backend.cpp (1)
91-104: Error message is misleading for non-stale fd failures.The error message "Stale DMA-BUF fd, skipping frame" is only accurate for some failure modes.
vkGetMemoryFdPropertiesKHRcan fail for other reasons (e.g., unsupported handle type, device lost).📝 Suggested improvement
if (fd_props_result != vk::Result::eSuccess) { return make_error<uint32_t>(ErrorCode::vulkan_init_failed, - "Stale DMA-BUF fd, skipping frame"); + "Failed to query DMA-BUF fd properties: " + + vk::to_string(fd_props_result)); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/capture/vk_layer/wsi_virtual.cppsrc/render/backend/vulkan_backend.cpp
🧰 Additional context used
🧠 Learnings (21)
📓 Common learnings
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
📚 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:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Capture layer exception: The Vulkan capture layer (`src/capture/vk_layer/`) is exempt and MUST use the raw Vulkan C API because layer dispatch tables require C function pointers and the layer must intercept C API calls.
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Testing scope: Only non-GPU logic tested initially. In scope: utility functions, configuration parsing, error handling, pipeline graph logic. Out of scope: Vulkan initialization, GPU resource management, rendering, capture layer.
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.053Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.053Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer 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:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : All vulkan-hpp calls returning `vk::Result` must be explicitly checked. Prohibited pattern: `static_cast<void>(device.waitIdle())`. Use macro `VK_TRY(call, ErrorCode, "message")` for vk::Result early return or manual pattern with error handling.
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/**/*.cpp : Error handling macros: Use `VK_TRY` for vk::Result early return in Vulkan code. Use `GOGGLES_TRY` for Result<T> propagation (both as statement and expression). Use `GOGGLES_MUST` for internal invariants that should never fail. Use manual pattern for destructors/cleanup functions where propagation is impossible.
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : RAII handle guidelines for Vulkan resources: Use `vk::Unique*` only for appropriate resource types: Instance, Device, Surface (long-lived singletons); Swapchain, Pipelines, Layouts (created once). Use plain `vk::` handles for command buffers (pooled lifetime), per-frame sync primitives (reused), and imported external images (requires explicit sync).
Applied to files:
src/capture/vk_layer/wsi_virtual.cppsrc/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Vulkan resource management: Follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types. Call `device.waitIdle()` or wait on fences before destroying GPU-async resources. Store creation info with resources for debugging/recreation. Never leak Vulkan objects. Member ordering: declare in reverse destruction order (device before instance).
Applied to files:
src/capture/vk_layer/wsi_virtual.cppsrc/render/backend/vulkan_backend.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/capture/vk_layer/wsi_virtual.cppsrc/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.053Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.053Z
Learning: Applies to docs/**/*.{cpp,hpp} : Use standard spdlog severity levels: trace, debug, info, warn, error, critical. Define project-specific logging macros (`GOGGLES_LOG_TRACE`, `GOGGLES_LOG_DEBUG`, etc.) wrapping spdlog.
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer threading: Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR` hot path. Use atomics or locks for thread-safe layer state.
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.053Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.053Z
Learning: Applies to docs/**/*.cpp : Initialize logger once at application startup for console output (development) or file output (optional). For capture layer, initialize logger lazily on first `vkCreateInstance` hook.
Applied to files:
src/capture/vk_layer/wsi_virtual.cpp
📚 Learning: 2026-01-07T07:20:57.053Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.053Z
Learning: Applies to docs/**/*.cpp : No silent failures. Every error must be either handled or propagated upward. Log errors exactly once at subsystem boundaries (capture → pipeline). Do not log the same error at every level of the call stack. Prefer monadic operations (`.and_then()`, `.or_else()`, `.transform()`) for chaining.
Applied to files:
src/capture/vk_layer/wsi_virtual.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/render/**/*.{cpp,hpp} : Use `vk::Unique*` for long-lived Vulkan resources; use plain handles for per-frame/GPU-async resources
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Main thread owns: Vulkan instance, device, swapchain lifecycle; queue submission; window events and user input; job coordination. Main thread MUST NOT: block on I/O, perform heavy computation (>1ms), allocate memory in per-frame code paths.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Per-frame code paths on main thread MUST NOT: perform dynamic memory allocations (`new`, `malloc`, `std::make_shared`); use blocking synchronization primitives (`std::mutex`, `std::condition_variable`); exceed 8ms CPU time budget (excluding GPU sync).
Applied to files:
src/render/backend/vulkan_backend.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/render/**/*.{cpp,hpp} : Application code MUST use vulkan-hpp with `VULKAN_HPP_NO_EXCEPTIONS` and dynamic dispatch
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Single-threaded render loop on main thread by default. Render backend (`goggles::render`) runs on main thread. Pipeline execution runs on main thread. Job system for non-render work on worker threads. Threading introduced only when profiling justifies it (main thread CPU consistently >8ms).
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Application code MUST use vulkan-hpp (C++ bindings), NOT raw Vulkan C API. Use `vk::` types (e.g., `vk::Instance`, `vk::Device`). Do not use raw C handles like `VkInstance`, `VkDevice`. Required configuration: `#define VULKAN_HPP_NO_EXCEPTIONS`, `#define VULKAN_HPP_DISPATCH_LOADER_DYNAMIC 1`, `#include <vulkan/vulkan.hpp>`.
Applied to files:
src/render/backend/vulkan_backend.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/render/backend/vulkan_backend.cpp
🧬 Code graph analysis (2)
src/capture/vk_layer/wsi_virtual.cpp (2)
src/render/backend/vulkan_backend.cpp (2)
find_memory_type(39-47)find_memory_type(39-40)src/capture/vk_layer/vk_capture.cpp (2)
query_export_modifiers(258-296)query_export_modifiers(258-259)
src/render/backend/vulkan_backend.cpp (1)
src/util/error.hpp (1)
make_error(50-52)
⏰ 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 (17)
src/render/backend/vulkan_backend.cpp (8)
36-37: LGTM: Local constant avoids external header dependency.Defining
DRM_FORMAT_MOD_INVALIDlocally is a pragmatic approach to avoid pulling indrm_fourcc.h. The value matches the standard definition.
49-89: LGTM: Well-structured chain initialization for DMA-BUF image creation.The
DmabufImageCreateChainstruct andinit_dmabuf_image_create_chainfunction cleanly encapsulate the pNext chain setup. The plane layout correctly wiresframe.offsetandframe.stride.
106-135: LGTM: Memory import helper with proper fd ownership semantics.The function correctly:
- Chains dedicated allocation info only when required/preferred
- Releases fd ownership to Vulkan on success (line 133)
- Returns error with descriptive message on failure
137-156: LGTM: Clean image view creation helper.Simple and correct encapsulation of image view creation with proper error handling.
840-846: LGTM: Early validation prevents invalid imports.Rejecting
DRM_FORMAT_MOD_INVALIDearly with a clear error message prevents downstream failures and aids debugging.
852-861: LGTM: Refactored image creation with improved error messages.The error message now includes format and modifier information, which is valuable for debugging DMA-BUF import failures across different GPU/driver combinations.
874-906: LGTM: Memory import path correctly uses new helpers.The refactored flow properly:
- Queries memory type bits from the DMA-BUF fd
- Combines with image memory requirements
- Uses the new
allocate_imported_dmabuf_memoryhelper- Cleans up on failure
915-921: LGTM: View creation uses new helper with proper cleanup.The refactored path correctly uses
create_imported_image_viewand stores the result.src/capture/vk_layer/wsi_virtual.cpp (9)
221-223: LGTM: DRM modifier constants defined locally.Consistent with the vulkan_backend.cpp approach, avoiding the need for
drm_fourcc.h.
272-306: LGTM: Flexible image creation with modifier/linear tiling support.The function cleanly handles both modifier tiling and linear tiling paths. The pNext chain is correctly constructed only when
use_modifier_tilingis true.
308-323: LGTM: Modifier query with proper fallback.Returns
DRM_FORMAT_MOD_INVALIDwhen the extension function is unavailable or the query fails, enabling the caller to handle gracefully.
325-356: LGTM: Layout query with overflow protection.The 32-bit bounds checks prevent silent truncation of large stride/offset values, which could cause incorrect DMA-BUF imports on the viewer side.
358-375: LGTM: DMA-BUF export with proper error handling.The function correctly checks both the Vulkan result and validates the fd is non-negative.
377-416: LGTM: Memory allocation with cleanup on bind failure.The function properly frees allocated memory if
BindImageMemoryfails (line 409).
418-486: Fallback path reusesmodifier_listparameter which is ignored for LINEAR tiling.On line 446, when falling back to LINEAR tiling, the
modifier_listis still passed tocreate_swapchain_imagebut withuse_modifier_tiling=false. This works correctly since the parameter is ignored whenuse_modifier_tilingis false, but it's slightly confusing.The fallback logic is otherwise well-structured:
- Detects invalid modifier after creation
- Destroys the failed image
- Recreates with LINEAR tiling
- Proper resource cleanup on any failure
488-531: LGTM: Modifier selection with comprehensive fallback strategy.The function correctly:
- Queries available modifiers with required features
- Checks for
GetImageDrmFormatModifierPropertiesEXTavailability before using modifier tiling- Falls back to LINEAR if no suitable modifiers or if the query function is unavailable
- Logs the decision for debugging
697-711: LGTM: Frame data now includes complete DMA-BUF metadata.The guard check correctly validates all four per-image vectors before accessing. The returned struct includes offset and modifier, completing the metadata needed for proper DMA-BUF import on the viewer side.
K1ngst0m
force-pushed
the
dev/wsi-drm-modifier
branch
from
4368ef5 to
eddf555
Compare
1 participant
Why
WSI proxy virtual swapchain frames need to carry correct DMA-BUF layout metadata (DRM format modifier + offset) so the viewer can import and sample them reliably.
What Changes
VK_IMAGE_TILING_DRM_FORMAT_MODIFIER_EXTwhen supported, and record the selected DRM modifier per image.stride,offset, andmodifierfor each virtual swapchain frame.CaptureFrameto store the DMA-BUFoffsetand plumb it into Vulkan import.Impact
vk-layer-capture,render-pipelinesrc/capture/vk_layer/wsi_virtual.*,src/capture/vk_layer/vk_hooks.cppsrc/capture/capture_receiver.*src/render/backend/vulkan_backend.cppPR Type
Bug fix, Enhancement
Description
Export DMA-BUF stride, offset, and modifier metadata in WSI proxy mode
Prefer DRM format modifier tiling for virtual swapchain images when supported
Query and record per-image DRM modifier, stride, and offset during allocation
Plumb offset and modifier through IPC and Vulkan import pipeline
Diagram Walkthrough
File Walkthrough
3 files
Add offset and modifier storage to swapchain structuresQuery DRM modifiers and export plane layout metadataAdd offset field to CaptureFrame structure3 files
Send offset and modifier in texture data IPCStore and track offset in frame metadataUse exported offset during DMA-BUF import4 files
Document change rationale and affected componentsDefine virtual swapchain creation and presentation requirementsDefine DMA-BUF import plane layout requirementsTrack implementation, validation, and spec tasksSummary by CodeRabbit
Bug Fixes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.