feat(layer): pre-record swapchain image copy commands#14
Conversation
📝 WalkthroughWalkthroughReplaces per-frame resources with pre-recorded per-swapchain copy command buffers ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Layer as CaptureLayer
participant GPU as GPU/Vulkan
participant Host as Host IO
Note over Layer,GPU: Initialization
Layer->>GPU: init_copy_cmds(swap) -- create pools & pre-record copy cmd buffers
GPU-->>Layer: pre-recorded CmdBuffers
Note over App,Layer: Presentation / Capture
App->>Layer: on_present()
Layer->>Layer: select CopyCmd (not busy)
alt Async path
Layer->>GPU: submit pre-recorded CmdBuffer + timeline semaphore increment
GPU-->>Layer: timeline value updated (in-flight)
Layer->>Layer: mark CopyCmd.busy
par After GPU completes
GPU->>Layer: timeline signal (timeline semaphore)
Layer->>Host: schedule readback (async)
Layer->>Layer: mark CopyCmd.busy = false
end
else Sync path
Layer->>GPU: submit pre-recorded CmdBuffer + wait export fence
GPU-->>Layer: export fence signaled
Layer->>Host: stream texture data synchronously
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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/vk_capture.cpp (1)
653-659: No fallback ifinit_copy_cmdsfails silently.After
init_export_imagesucceeds,init_copy_cmdsis called unconditionally. If command pool/buffer creation fails insideinit_copy_cmds,copy_cmdsmay be empty or contain invalid handles, causingcapture_frameto either early-return (line 683) or crash.Consider returning a boolean from
init_copy_cmdsand handling failure:Suggested approach
- init_copy_cmds(swap, dev_data); + if (!init_copy_cmds(swap, dev_data)) { + LAYER_DEBUG("Copy command buffer init FAILED"); + // Optionally clean up export resources or mark as failed + }
🧹 Nitpick comments (3)
src/capture/vk_layer/vk_capture.hpp (1)
85-86: Consider returningboolfrominit_copy_cmdsfor error propagation.Unlike
init_export_imageandinit_sync_primitiveswhich returnbool,init_copy_cmdsreturnsvoid. If Vulkan calls fail during command pool/buffer creation, the caller has no way to detect partial initialization and may proceed with invalid handles.Suggested signature change
- void init_copy_cmds(SwapData* swap, VkDeviceData* dev_data); - void destroy_copy_cmds(SwapData* swap, VkDeviceData* dev_data); + bool init_copy_cmds(SwapData* swap, VkDeviceData* dev_data); + void destroy_copy_cmds(SwapData* swap, VkDeviceData* dev_data);src/capture/vk_layer/vk_capture.cpp (2)
530-533: Add error checking for command buffer recording.
BeginCommandBufferandEndCommandBuffer(line 603) can fail. Consider checking their return values for robustness.Suggested fix
- funcs.BeginCommandBuffer(cmd.cmd, &begin_info); + res = funcs.BeginCommandBuffer(cmd.cmd, &begin_info); + if (res != VK_SUCCESS) { + LAYER_DEBUG("BeginCommandBuffer failed for image %zu: %d", i, res); + // Handle cleanup... + return false; + }
598-601:VK_PIPELINE_STAGE_TOP_OF_PIPE_BITindstStageMaskis a no-op.Using
TOP_OF_PIPE_BITas a destination stage means "no stage waits for this barrier" which is effectively ignored. For the post-copy barrier before presentation,BOTTOM_OF_PIPE_BITalone is sufficient and clearer.Suggested simplification
funcs.CmdPipelineBarrier(cmd.cmd, VK_PIPELINE_STAGE_TRANSFER_BIT, - VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT | - VK_PIPELINE_STAGE_BOTTOM_OF_PIPE_BIT, + VK_PIPELINE_STAGE_BOTTOM_OF_PIPE_BIT, 0, 0, nullptr, 0, nullptr, 2, barriers);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/capture/vk_layer/vk_capture.cppsrc/capture/vk_layer/vk_capture.hpp
🧰 Additional context used
🧬 Code graph analysis (2)
src/capture/vk_layer/vk_capture.hpp (1)
src/render/backend/vulkan_backend.hpp (1)
struct FrameResources {(88-92)
src/capture/vk_layer/vk_capture.cpp (1)
src/capture/vk_layer/vk_capture.hpp (6)
swap(83-83)swap(84-84)swap(85-85)swap(86-86)swap(87-88)swap(89-89)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and Test
- GitHub Check: Static Analysis (clang-tidy)
🔇 Additional comments (5)
src/capture/vk_layer/vk_capture.hpp (1)
18-23: LGTM! Clean struct design for pre-recorded command buffers.The
CopyCmdstruct appropriately encapsulates per-swapchain-image resources needed for the pre-recording optimization: dedicated pool (avoidingVK_COMMAND_POOL_CREATE_RESET_COMMAND_BUFFER_BIT), the pre-recorded command buffer, timeline tracking, and busy state.src/capture/vk_layer/vk_capture.cpp (4)
609-628: LGTM! Proper cleanup with in-flight command handling.The destruction logic correctly waits for busy command buffers via timeline semaphore before destroying pools. This prevents destroying resources that are still in use by the GPU.
683-698: LGTM! Correct bounds checking and synchronization for pre-recorded buffers.The bounds check on line 683 guards against invalid indices, and the wait logic (lines 690-698) properly synchronizes with in-flight command buffers before reuse. The transition to using pre-recorded
CopyCmdreferences is clean.
705-768: LGTM! Async capture path properly tracks command buffer state.The async path correctly:
- Increments and assigns
timeline_valuefor synchronization- Submits the pre-recorded command buffer with timeline semaphore signaling
- Queues the capture item for the worker thread
- Marks
cmd.busy = trueto prevent reuse before completion
807-807: LGTM! Cleanup correctly callsdestroy_copy_cmds.The cleanup path properly invokes
destroy_copy_cmdsbefore releasing other swap resources, ensuring command buffers are waited on and pools are destroyed in the correct order.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/capture/vk_layer/vk_capture.cpp (1)
516-533: Good fix! Error handling added for command pool and buffer allocation.The error handling for
CreateCommandPoolandAllocateCommandBuffershas been properly implemented, addressing the previous review concern. Both failures now trigger cleanup viadestroy_copy_cmdsand return false.
🧹 Nitpick comments (1)
src/capture/vk_layer/vk_capture.cpp (1)
541-541: Consider clarifying the "Reusable" comment.The command buffer flags are set to 0, meaning the buffer can be reused but not while in the pending state (no
VK_COMMAND_BUFFER_USAGE_SIMULTANEOUS_USE_BIT). Yourbusyflag synchronization correctly prevents concurrent submission. Consider clarifying the comment to reflect this, e.g., "Sequential reuse" or "Reusable after completion".
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/capture/vk_layer/vk_capture.cppsrc/capture/vk_layer/vk_capture.hpp
🧰 Additional context used
🧬 Code graph analysis (1)
src/capture/vk_layer/vk_capture.cpp (2)
src/capture/vk_layer/vk_capture.hpp (6)
swap(83-83)swap(84-84)swap(85-85)swap(86-86)swap(87-88)swap(89-89)src/render/backend/vulkan_backend.hpp (3)
cmd(61-62)cmd(63-64)image_index(65-65)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Static Analysis (clang-tidy)
- GitHub Check: Build and Test
🔇 Additional comments (7)
src/capture/vk_layer/vk_capture.hpp (3)
18-23: LGTM! Well-designed struct for pre-recorded commands.The
CopyCmdstruct appropriately captures per-command-buffer state with safe default initialization. Thebusyflag enables proper synchronization to prevent resubmission of in-flight commands.
62-63: LGTM! Clear transition to per-swapchain-image command buffers.The updated data structure aligns with the PR objective of pre-recording one command buffer per swapchain image.
85-86: LGTM! Clear and descriptive method names.The renamed methods accurately reflect their purpose in the pre-recorded command buffer lifecycle.
src/capture/vk_layer/vk_capture.cpp (4)
619-638: LGTM! Proper cleanup with synchronization.The function correctly waits for in-flight command buffers (using the
busyflag and timeline values) before destroying resources. Destroying the command pool implicitly frees the command buffers, so the cleanup is complete.
696-714: LGTM! Clean pre-recorded command buffer usage.The bounds check, busy-wait synchronization, and pre-recorded command submission flow are all correctly implemented. This achieves the PR objective of eliminating per-frame command recording overhead.
669-672: LGTM! Proper initialization flow.The copy commands are initialized at the appropriate point—after the export image is ready—ensuring all resources are available for command recording.
820-820: LGTM! Cleanup properly integrated.The
destroy_copy_cmdscall ensures pre-recorded command buffers are cleaned up with proper synchronization during swapchain destruction.
Records copy commands once per swapchain image at init
1 participant
Records copy commands once per swapchain image at init
Performance Impact
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.