refactor(render): split monolithic backend into subcomponents#113
Merged
refactor(render): split monolithic backend into subcomponents#113
Conversation
- Introduce VulkanContext for instance/device/queue bring-up and capability facts - Introduce RenderOutput for windowed/headless targets and frame retirement - Introduce ExternalFrameImporter for DMA-BUF image lifetime and wait-semaphore ownership - Introduce FilterChainController for boundary-facing filter coordination and preset reload - Register new translation units in CMakeLists and expand test coverage
📝 WalkthroughWalkthroughThe PR refactors VulkanBackend internals into four backend-internal subsystems—VulkanContext, RenderOutput, ExternalFrameImporter, and FilterChainController—adds specifications, tasks, and tests defining boundaries, extraction/verification order, and shutdown sequencing, and wires the new subsystems into the VulkanBackend facade. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant VulkanBackend as VulkanBackend (facade)
participant VulkanContext as VulkanContext
participant ExternalImporter as ExternalFrameImporter
participant FilterController as FilterChainController
participant RenderOutput as RenderOutput
App->>VulkanBackend: request frame / submit external image
VulkanBackend->>VulkanContext: ensure device & queues initialized
VulkanBackend->>ExternalImporter: import_external_image / prepare_wait_semaphore
ExternalImporter-->>VulkanBackend: ImportedSource / wait semaphore
VulkanBackend->>FilterController: ensure filter chain / resolve preset
FilterController-->>VulkanBackend: filter chain ready / consume_chain_swapped
VulkanBackend->>RenderOutput: acquire_next_image / prepare_headless_frame
RenderOutput-->>VulkanBackend: image index / command buffer
VulkanBackend->>RenderOutput: submit_and_present (with wait semaphores)
RenderOutput-->>VulkanBackend: present complete / optionally readback
VulkanBackend->>FilterController: advance_frame / cleanup_deferred_destroys
VulkanBackend->>ExternalImporter: retire_wait_semaphore / destroy (on shutdown)
VulkanBackend->>VulkanContext: destroy (on shutdown)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
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. ✨ 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: 4
🧹 Nitpick comments (1)
tests/render/test_vulkan_backend_subsystem_contracts.cpp (1)
19-27: Consider extractingread_text_fileinto a shared test utility.This helper function is duplicated in
tests/render/test_filter_boundary_contracts.cpp(lines 20-26). Extract it to a common test utilities header to reduce duplication across test files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/render/test_vulkan_backend_subsystem_contracts.cpp` around lines 19 - 27, The helper function read_text_file is duplicated across tests; extract it into a shared test utilities header (e.g., test_utilities.h) and replace the local copies with an `#include` of that header; move the implementation of read_text_file into the header (or a corresponding .cpp with a header declaration) and update both test files to call the common read_text_file function instead of redefining it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@openspec/changes/archive/2026-03-08-vulkan-backend-refactor-subsystems/tasks.md`:
- Line 87: The table cell containing the regex
`^(headless_smoke|goggles_headless_integration)$` is being split by the pipe
character; update that cell so the pipe is escaped (e.g.
`^(headless_smoke\|goggles_headless_integration)$`) or wrap the pattern in an
inline code span that preserves the pipe, ensuring the table row `Backend
Behavior Is Preserved Across the Split | 3.2, 4.3, 5.2, 6.1, 7.2, 7.3 | ...` no
longer produces the "too many cells" warning.
In `@src/render/backend/render_output.cpp`:
- Around line 534-540: You currently call destroy_offscreen_target(...) and
immediately set offscreen_extent, swapchain_format, swapchain_extent and
headless before any fallible creation steps, which can leave the object in a
headless/initialized state on failure; change to a commit-on-success flow:
perform all image/memory/view creation into local/temp variables and only if all
creation steps succeed assign the class members (offscreen_extent,
swapchain_format, swapchain_extent, headless) and call
destroy_offscreen_target(...) for the old target (or swap-in the new resources),
otherwise clean up the temporaries and leave the existing offscreen target
intact (or on failure explicitly roll back by restoring previous members).
Ensure you reference destroy_offscreen_target, offscreen_extent,
swapchain_format, swapchain_extent and headless when implementing the
swap/rollback.
- Around line 646-655: RenderOutput::wait_all_frames currently passes all
entries from frames[].in_flight_fence to context.device.waitForFences even when
some are VK_NULL_HANDLE; update this function to first verify context.device is
valid, collect only non-null vk::Fence handles from frames[i].in_flight_fence
(iterate MAX_FRAMES_IN_FLIGHT), and call context.device.waitForFences with that
filtered list; if the filtered list is empty, skip the wait entirely and
otherwise preserve the same flags (VK_TRUE, UINT64_MAX) and existing error
logging for the vk::Result.
In `@src/render/backend/render_output.hpp`:
- Around line 38-45: The two functions submit_and_present and submit_headless
currently allow passing a non-null acquire_wait_semaphore while leaving
acquire_wait_stage default-initialized ({}), which can produce an empty
pWaitDstStageMask and trigger Vulkan validation errors; fix by providing
overloads that force callers to supply a stage when providing a semaphore (e.g.,
keep a no-semaphore overload with no stage param, and add overloads taking
vk::Semaphore plus a required vk::PipelineStageFlags with no default), or
alternatively remove the default for acquire_wait_stage so it is mandatory
whenever a semaphore parameter is used; update all call sites to use the new
overloads/signatures.
---
Nitpick comments:
In `@tests/render/test_vulkan_backend_subsystem_contracts.cpp`:
- Around line 19-27: The helper function read_text_file is duplicated across
tests; extract it into a shared test utilities header (e.g., test_utilities.h)
and replace the local copies with an `#include` of that header; move the
implementation of read_text_file into the header (or a corresponding .cpp with a
header declaration) and update both test files to call the common read_text_file
function instead of redefining it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c28a357e-5eff-4752-a776-4e08af5ea0e4
📒 Files selected for processing (20)
openspec/changes/archive/2026-03-08-vulkan-backend-refactor-subsystems/.openspec.yamlopenspec/changes/archive/2026-03-08-vulkan-backend-refactor-subsystems/design.mdopenspec/changes/archive/2026-03-08-vulkan-backend-refactor-subsystems/proposal.mdopenspec/changes/archive/2026-03-08-vulkan-backend-refactor-subsystems/specs/vulkan-backend-module-layout/spec.mdopenspec/changes/archive/2026-03-08-vulkan-backend-refactor-subsystems/tasks.mdopenspec/specs/vulkan-backend-module-layout/spec.mdsrc/render/backend/CMakeLists.txtsrc/render/backend/external_frame_importer.cppsrc/render/backend/external_frame_importer.hppsrc/render/backend/filter_chain_controller.cppsrc/render/backend/filter_chain_controller.hppsrc/render/backend/render_output.cppsrc/render/backend/render_output.hppsrc/render/backend/vulkan_backend.cppsrc/render/backend/vulkan_backend.hppsrc/render/backend/vulkan_context.cppsrc/render/backend/vulkan_context.hpptests/CMakeLists.txttests/render/test_filter_boundary_contracts.cpptests/render/test_vulkan_backend_subsystem_contracts.cpp
openspec/changes/archive/2026-03-08-vulkan-backend-refactor-subsystems/tasks.md
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/render/backend/render_output.cpp (1)
668-677:⚠️ Potential issue | 🔴 CriticalStill passes default-initialized fences into
waitForFences().
framesstarts zero-initialized anddestroy()resets it to{}, but this still forwards every slot straight intocontext.device.waitForFences(...). That can feedVK_NULL_HANDLEduring partial init/teardown, which Vulkan does not allow here. Also return early whencontext.deviceis null.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/backend/render_output.cpp` around lines 668 - 677, In RenderOutput::wait_all_frames, avoid passing default/NULL fences into context.device.waitForFences: first check that context.device is valid and return early if null, then collect only non-null in_flight_fence handles from frames (referencing frames[i].in_flight_fence) into a temporary container and call context.device.waitForFences only when that container is non-empty; this prevents VK_NULL_HANDLE from being forwarded during partial init/teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/render/backend/render_output.cpp`:
- Around line 448-463: The code currently leaks partially-created Vulkan handles
when initialization fails (e.g., after createCommandPool()/command_pool
assignment or after allocating some fences/semaphores), so modify the init path
in RenderOutput to either stage newly-created handles in local variables and
only assign to member fields after all steps succeed, or explicitly
destroy/cleanup any handles created in this attempt before returning an error;
specifically handle cleanup for command_pool (created by createCommandPool),
command buffers allocated via device.allocateCommandBuffers, and any
fences/semaphores created in the later block so that a failed allocate/creation
path calls the same teardown logic used by destroy() (or invokes destroy()
safely) to leave RenderOutput in a clean state.
- Around line 375-418: The code currently assigns members swapchain,
swapchain_format, swapchain_extent, headless early and then performs fallible
steps (device.getSwapchainImagesKHR, device.createImageView,
recreate_render_finished_semaphores), so failures leave the object in a
partially-updated state; change the routine to perform all swapchain-dependent
operations into local temporaries (local_swapchain, local_format, local_extent,
local_images, local_image_views, local_render_finished_semaphores) and only
after getSwapchainImagesKHR returns success, all image views are created, and
recreate_render_finished_semaphores succeeds, atomically move/assign those
locals into the class members swapchain, swapchain_format, swapchain_extent,
swapchain_images, swapchain_image_views and the semaphores; ensure on the
success path you also clear needs_resize and set present_id/last_present_time as
before, and keep calling cleanup_swapchain(context) on failure without
publishing the locals.
- Around line 748-751: submit_and_present and submit_headless currently forward
an empty acquire_wait_stage when acquire_wait_semaphore is provided, allowing a
zero VkPipelineStageFlags mask which violates Vulkan; update the submit path to
validate that when acquire_wait_semaphore != VK_NULL_HANDLE the corresponding
acquire_wait_stage is non-zero (or replace it with a sensible default such as
VK_PIPELINE_STAGE_COLOR_ATTACHMENT_OUTPUT_BIT), and reject/log/assert the call
before calling graphics_queue.submit() if the stage mask is zero; adjust the
logic around wait_stages/wait_count (and the acquire_wait_stage parameter
declarations) to enforce this invariant so a zero-stage mask is never passed to
graphics_queue.submit().
- Around line 886-888: stbi_write_png is being passed output.c_str(), but
std::filesystem::path::c_str() can return const wchar_t* on Windows causing a
compile error; change the argument to pass a UTF-8 narrow C string by using
output.string().c_str() (i.e., call stbi_write_png(..., output.string().c_str(),
...)) so the filename is a const char* as expected; update the call to
stbi_write_png where output, width, height, and data are used to use
output.string().c_str() instead of output.c_str().
---
Duplicate comments:
In `@src/render/backend/render_output.cpp`:
- Around line 668-677: In RenderOutput::wait_all_frames, avoid passing
default/NULL fences into context.device.waitForFences: first check that
context.device is valid and return early if null, then collect only non-null
in_flight_fence handles from frames (referencing frames[i].in_flight_fence) into
a temporary container and call context.device.waitForFences only when that
container is non-empty; this prevents VK_NULL_HANDLE from being forwarded during
partial init/teardown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8422a83c-ad92-490a-9ff6-8ff26ce92344
📒 Files selected for processing (1)
src/render/backend/render_output.cpp
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/render/backend/render_output.hpp (1)
38-46:⚠️ Potential issue | 🟠 MajorMake the external wait stage explicit.
vk::PipelineStageFlagBits::eColorAttachmentOutputis only correct when the waited work is first consumed there. This API is also paired withExternalFrameImporter::prepare_wait_semaphore(...); imported sources are typically read earlier in the pipeline, so the default here can silently under-synchronize. Please remove the default and the implementation-side fallback so callers must choose the stage that matches the first consumer.In Vulkan `vkQueueSubmit` / `VkSubmitInfo`, if a waited semaphore protects an imported image that is first read in the fragment shader, should `pWaitDstStageMask` use the earliest consuming stage (for example `VK_PIPELINE_STAGE_FRAGMENT_SHADER_BIT`) instead of `VK_PIPELINE_STAGE_COLOR_ATTACHMENT_OUTPUT_BIT`?Possible API shape
- [[nodiscard]] auto submit_and_present(VulkanContext& context, uint32_t image_index, - vk::Semaphore acquire_wait_semaphore = nullptr, - vk::PipelineStageFlags acquire_wait_stage = - vk::PipelineStageFlagBits::eColorAttachmentOutput) - -> Result<void>; - [[nodiscard]] auto submit_headless(VulkanContext& context, - vk::Semaphore acquire_wait_semaphore = nullptr, - vk::PipelineStageFlags acquire_wait_stage = - vk::PipelineStageFlagBits::eColorAttachmentOutput) - -> Result<void>; + [[nodiscard]] auto submit_and_present(VulkanContext& context, uint32_t image_index) + -> Result<void>; + [[nodiscard]] auto submit_and_present(VulkanContext& context, uint32_t image_index, + vk::Semaphore acquire_wait_semaphore, + vk::PipelineStageFlags acquire_wait_stage) + -> Result<void>; + [[nodiscard]] auto submit_headless(VulkanContext& context) -> Result<void>; + [[nodiscard]] auto submit_headless(VulkanContext& context, + vk::Semaphore acquire_wait_semaphore, + vk::PipelineStageFlags acquire_wait_stage) + -> Result<void>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/backend/render_output.hpp` around lines 38 - 46, The API currently defaults acquire_wait_stage to vk::PipelineStageFlagBits::eColorAttachmentOutput which can under-synchronize imported external waits; update the declarations and corresponding definitions for submit_and_present and submit_headless (and any overloads) to remove the default value for acquire_wait_stage so callers must pass an explicit vk::PipelineStageFlags, and remove any implementation-side fallback logic (e.g., in submit_and_present, submit_headless or any use of ExternalFrameImporter::prepare_wait_semaphore) that substitutes eColorAttachmentOutput when none is provided; ensure call sites are updated to pass the proper earliest-consuming stage (e.g., fragment shader) and adjust any documentation/comments to state that the caller must choose the correct first-consumer stage.src/render/backend/render_output.cpp (1)
1001-1003:⚠️ Potential issue | 🟠 MajorPass a narrow path string to
stbi_write_png().
std::filesystem::path::c_str()isconst wchar_t*on Windows, whilestbi_write_png()expectsconst char*. This still breaks Windows builds; materializeoutput.string()first.tests/visual/image_compare.cppalready uses that pattern.What is the signature of `stbi_write_png`, and what does `std::filesystem::path::c_str()` return on Windows in C++17/C++20?Patch
- const int png_result = - stbi_write_png(output.c_str(), static_cast<int>(width), static_cast<int>(height), 4, data, - static_cast<int>(width * 4)); + const auto output_path = output.string(); + const int png_result = + stbi_write_png(output_path.c_str(), static_cast<int>(width), static_cast<int>(height), 4, + data, static_cast<int>(width * 4));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/backend/render_output.cpp` around lines 1001 - 1003, stbi_write_png() expects a narrow const char* filename but the code passes output.c_str(), which on Windows yields const wchar_t* for std::filesystem::path; change the call to pass a narrow string by materializing output.string() (e.g., use output.string().c_str()) before calling stbi_write_png, mirroring the pattern used in tests/visual/image_compare.cpp and update the call site where stbi_write_png(...) is invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/render/backend/render_output.cpp`:
- Around line 1001-1003: stbi_write_png() expects a narrow const char* filename
but the code passes output.c_str(), which on Windows yields const wchar_t* for
std::filesystem::path; change the call to pass a narrow string by materializing
output.string() (e.g., use output.string().c_str()) before calling
stbi_write_png, mirroring the pattern used in tests/visual/image_compare.cpp and
update the call site where stbi_write_png(...) is invoked.
In `@src/render/backend/render_output.hpp`:
- Around line 38-46: The API currently defaults acquire_wait_stage to
vk::PipelineStageFlagBits::eColorAttachmentOutput which can under-synchronize
imported external waits; update the declarations and corresponding definitions
for submit_and_present and submit_headless (and any overloads) to remove the
default value for acquire_wait_stage so callers must pass an explicit
vk::PipelineStageFlags, and remove any implementation-side fallback logic (e.g.,
in submit_and_present, submit_headless or any use of
ExternalFrameImporter::prepare_wait_semaphore) that substitutes
eColorAttachmentOutput when none is provided; ensure call sites are updated to
pass the proper earliest-consuming stage (e.g., fragment shader) and adjust any
documentation/comments to state that the caller must choose the correct
first-consumer stage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39f959dc-264a-4aaf-a453-38de8f403343
📒 Files selected for processing (2)
src/render/backend/render_output.cppsrc/render/backend/render_output.hpp
This was referenced Mar 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests