refactor(chain): split filter runtime by responsibility#126
Merged
Conversation
- Replace the facade/core split with runtime-owned chain components - Separate build, resources, execution, and control responsibilities - Update the C ABI and C++ wrapper to route through ChainRuntime - Retire swapped runtimes through a bounded cleanup helper
📝 WalkthroughWalkthroughMajor refactor splitting the monolithic filter-chain into ChainRuntime, ChainResources, ChainBuilder, ChainExecutor, and ChainControls; updates controller to use retirement-based runtime tracking and authoritative control overrides; adapts C API and backend wiring; adds design/spec artifacts and updates tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant C_API as C API (goggles)
participant Controller as FilterChainController
participant Runtime as ChainRuntime
participant Builder as ChainBuilder
participant Resources as ChainResources
participant Executor as ChainExecutor
participant Backend as VulkanBackend
App->>C_API: create_chain(...)
C_API->>Controller: request_create_runtime(...)
Controller->>Runtime: ChainRuntime::create(...)
Runtime->>Builder: build(preset)
Builder-->>Resources: CompiledChain
Runtime->>Resources: install(CompiledChain)
Controller-->>Runtime: set_stage_policy / set_prechain_resolution
Note right of Runtime: Runtime owns resources, controls, executor
App->>C_API: record(frame)
C_API->>Controller: record/frame tick
Controller->>Runtime: record(...)
Runtime->>Executor: record(...command buffer...)
Executor->>Resources: bind textures / run passes
Executor-->>Backend: submit commands
App->>C_API: swap_preset(new_preset)
C_API->>Controller: request_rebuild
Controller->>Runtime: create pending ChainRuntime
Runtime->>Builder: build(new_preset)
Builder-->>Resources: CompiledChain
Runtime->>Resources: install(CompiledChain)
Controller->>Controller: retire_runtime_with_bounded_fallback(old_runtime)
Controller->>Runtime: activate pending -> authoritative overrides applied
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 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 |
Review Summary by QodoRefactor filter-chain runtime with responsibility-scoped components behind stable gates
WalkthroughsDescription• **Refactored filter-chain runtime architecture** by replacing the facade/core split with a unified ChainRuntime class that owns all runtime responsibilities behind stable C++ and C ABI gates • **Decomposed runtime into four responsibility-scoped components**: ChainBuilder (preset compilation and shader preprocessing), ChainResources (GPU allocations and framebuffer lifecycle), ChainExecutor (per-frame command recording), and ChainControls (control descriptor and state management) • **Simplified controller orchestration** by moving stage-policy, prechain resolution, and control semantics behind the runtime gate, reducing FilterChainController to async submission and retirement management only • **Implemented bounded runtime retirement safety** with RetiredRuntimeTracker helper and fallback cleanup mechanism to ensure safe async swaps and frame-based resource cleanup • **Enhanced prechain resolution flexibility** to support partial resolution (width-only, height-only, or disabled) with proper state preservation across chain rebuilds • **Updated C ABI and C++ wrapper** to route through ChainRuntime while maintaining backward compatibility and contract continuity • **Added comprehensive design documentation** including proposal, specification, task breakdown, and implementation context with verification contracts • **Updated all render tests** to validate new architecture boundaries, control replay semantics, retirement safety, and C API contracts Diagramflowchart LR
A["C ABI<br/>goggles_filter_chain.h"]
B["C++ Wrapper<br/>ChainRuntime"]
C["ChainBuilder<br/>Preset Compilation"]
D["ChainResources<br/>GPU Allocations"]
E["ChainExecutor<br/>Command Recording"]
F["ChainControls<br/>Control State"]
G["FilterChainController<br/>Orchestration"]
H["VulkanBackend<br/>Vulkan State"]
A -- "stable gate" --> B
B -- "owns" --> C
B -- "owns" --> D
B -- "owns" --> E
B -- "owns" --> F
G -- "creates/swaps" --> B
G -- "retirement helper" --> H
H -- "handoff seam" --> B
File Changes1. src/render/backend/filter_chain_controller.cpp
|
Code Review by Qodo
1. Framebuffer layout reset
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+231
to
+234
| transition_image_layout( | ||
| cmd, resources.m_framebuffers[i]->image(), | ||
| {.from = vk::ImageLayout::eUndefined, .to = vk::ImageLayout::eColorAttachmentOptimal}); | ||
|
|
There was a problem hiding this comment.
1. Framebuffer layout reset 🐞 Bug ✓ Correctness
ChainExecutor transitions reusable pass/prechain/postchain framebuffer images from vk::ImageLayout::eUndefined to eColorAttachmentOptimal on every record(), even though it transitions those same images to eShaderReadOnlyOptimal after rendering and ChainResources reuses the VkImage handles across frames. This inconsistent layout history can trigger Vulkan validation errors or undefined rendering when recording subsequent frames.
Agent Prompt
### Issue description
`ChainExecutor` records Vulkan image memory barriers that always use `vk::ImageLayout::eUndefined` as the old layout for pass/prechain/postchain framebuffer images. But the same function transitions those images to `vk::ImageLayout::eShaderReadOnlyOptimal` after rendering, and `ChainResources` reuses the same framebuffer objects across frames, so subsequent frames cannot legally start from `eUndefined`.
### Issue Context
This affects all intermediate framebuffer images (`m_prechain_framebuffers`, `m_framebuffers`, and `m_postchain_framebuffers`). The code already has a pattern for per-image initialization tracking (`m_feedback_initialized`), which can be reused for these framebuffers.
### Fix Focus Areas
- src/render/chain/chain_executor.cpp[61-64]
- src/render/chain/chain_executor.cpp[122-125]
- src/render/chain/chain_executor.cpp[231-234]
- src/render/chain/chain_resources.cpp[153-159]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (9)
openspec/specs/packaging/spec.md (1)
43-63: Well-structured requirement with clear testable scenarios.The new "Cursor Theme Asset Exclusion" requirement follows the document's established format consistently, with properly defined SHALL statements and testable scenarios covering both the exclusion verification and runtime behavior.
Minor optional refinements for consistency:
- Line 59: Consider using "a Goggles AppImage" instead of "a packaged Goggles runtime" to match the terminology used in other scenarios (e.g., lines 10, 30).
- Lines 51, 62: The phrase "runtime/system/fallback cursor sourcing" is somewhat technical. If this spec is also consumed by non-domain experts, a brief parenthetical explanation might help (e.g., "through runtime/system/fallback cursor sourcing (system cursor themes or built-in fallbacks)").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/specs/packaging/spec.md` around lines 43 - 63, Update the spec text to use consistent terminology and clarify a technical phrase: replace "a packaged Goggles runtime" (in the Viewer scenario) with "a Goggles AppImage" to match other scenarios, and add a short parenthetical to the phrase "runtime/system/fallback cursor sourcing" (e.g., "(system cursor themes or built-in fallbacks)") where it appears to aid non-domain readers; ensure these edits occur in the "Cursor Theme Asset Exclusion" section around the lines containing those phrases so the SHALL statements and scenarios retain exact meaning.tests/render/test_filter_controls.cpp (1)
73-99: This test doesn't isolate the stage key yet.The two descriptors differ by both
stageandname, so a cache keyed only by name would still pass. If the goal is to lock in stage scoping, make the descriptors collide on every other dimension and vary only the stage (or the explicit control id, if that's whatChainControlskeys on).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/render/test_filter_controls.cpp` around lines 73 - 99, The test currently varies both stage and name so it doesn’t prove stage-scoped keys; update the descriptors used in TEST_CASE so they collide on every key except stage (e.g., give effect_descriptor the same name as prechain_descriptor or use the same explicit control id if ChainControls keys on an id) and then call remember_control_value, replay_value_for, and forget_control_value to assert that forgetting an effect-stage entry does not evict the prechain-stage entry; keep the value ranges and asserts otherwise the same and reference ChainControls, FilterControlDescriptor, remember_control_value, replay_value_for, and forget_control_value when making the change.tests/render/test_filter_chain.cpp (1)
90-93: Relax these source-text anchors.These assertions now depend on exact spellings like
resources.m_passes.size()andresources.m_feedback_initialized[pass_idx] = true;, so a harmless loop or local-variable refactor can fail the audit without changing the contract. Prefer anchoring on the surrounding stage/helper calls or a smaller invariant.Also applies to: 108-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/render/test_filter_chain.cpp` around lines 90 - 93, The current source_text anchors are too brittle because they rely on exact internal names like "resources.m_passes.size()" and "resources.m_feedback_initialized[pass_idx] = true;"; update the searches to anchor on higher-level, stable calls instead (e.g., find the positions using "record_prechain(" and the start of the passes loop using a generic "for (" or the helper call that indicates effects processing, then search for "record_postchain(" after that position) so the test looks for the surrounding stage/helper calls (record_prechain and record_postchain and the generic loop start) rather than exact member/access spellings; apply the same relaxation to the other occurrences around the second block (the anchors referenced at lines 108-111).src/render/backend/filter_chain_controller.cpp (1)
131-190: Add compile-time validation and documentation for the fallback retirement delay bound.The numeric relationship appears correct (
FALLBACK_RETIRE_DELAY_FRAMES = 3 > MAX_FRAMES_IN_FLIGHT = 2), but the safety assumption is not explicitly validated or documented. The bound prevents destruction while the GPU owns frames, but relies on undocumented assumptions about howframe_count(incremented at render start) correlates with actual GPU submission and completion.Add a
static_assertin theRetiredRuntimeTrackerstruct verifying thatFALLBACK_RETIRE_DELAY_FRAMES > RenderOutput::MAX_FRAMES_IN_FLIGHT, and document the assumption thatframe_countincrements synchronously with GPU frame pacing. This addresses the openspec requirement: "verification coverage SHALL name the fallback behavior and its safety assumptions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/backend/filter_chain_controller.cpp` around lines 131 - 190, Add a compile-time check and a short doc comment in the RetiredRuntimeTracker definition to assert the safety bound: insert a static_assert that RetiredRuntimeTracker::FALLBACK_RETIRE_DELAY_FRAMES > RenderOutput::MAX_FRAMES_IN_FLIGHT and add a brief comment that FALLBACK_RETIRE_DELAY_FRAMES is chosen so frame_count (which increments per render start) will outlive GPU in-flight frames; reference RetiredRuntimeTracker, FALLBACK_RETIRE_DELAY_FRAMES, RenderOutput::MAX_FRAMES_IN_FLIGHT and frame_count so the assumption is explicit and verified at compile time.src/render/chain/chain_controls.cpp (1)
105-109: Minor: Double normalization of control values.
set_control_valuealready normalizes the value before callingremember_control_value, which then normalizes again. This is harmless but redundant.♻️ Simplify to avoid redundant normalization
void ChainControls::remember_control_value(const FilterControlDescriptor& descriptor, float value) { auto& overrides = descriptor.stage == FilterControlStage::prechain ? m_prechain_overrides : m_effect_overrides; - overrides[descriptor.name] = normalize_control_value(descriptor, value); + overrides[descriptor.name] = value; // Caller is responsible for normalization }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/chain/chain_controls.cpp` around lines 105 - 109, The value is being normalized twice: set_control_value calls normalize_control_value before calling ChainControls::remember_control_value, which calls normalize_control_value again; update ChainControls::remember_control_value to assume the incoming float is already normalized and store it directly into the appropriate overrides map (m_prechain_overrides or m_effect_overrides) using descriptor.name, removing the extra call to normalize_control_value (leave FilterControlDescriptor, FilterControlStage checks and map selection intact).src/render/chain/chain_resources.hpp (1)
95-133: Consider documenting the public member access pattern.The public member variables are intentionally exposed for direct access by
ChainExecutor. While this simplifies inter-component communication, a brief comment clarifying this design decision would help future maintainers understand the intended usage.📝 Documentation suggestion
+ // ------------------------------------------------------------------------- + // Public state - accessed directly by ChainExecutor during command recording + // ------------------------------------------------------------------------- + VulkanContext m_vk_ctx; vk::Format m_swapchain_format = vk::Format::eUndefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/chain/chain_resources.hpp` around lines 95 - 133, Add a brief comment above the public member block in chain_resources.hpp explaining that these members (e.g., VulkanContext m_vk_ctx, m_passes, m_framebuffers, m_texture_registry, m_prechain_passes, m_postchain_passes, etc.) are intentionally public to allow direct access by ChainExecutor for performance and simplicity; mention that ChainExecutor is the intended consumer, that invariants/state ownership (like lifetime of ShaderRuntime* m_shader_runtime and ownership of textures/framebuffers) must be respected by callers, and point to any expected synchronization rules (e.g., access from the render thread only or atomic flags like m_bypass_enabled/m_prechain_enabled) so future maintainers understand the design decision and safe usage.src/render/chain/chain_runtime.cpp (1)
74-84: Policy state may not be applied to newly created resources.The
set_stage_policymethod stores the policy in member variables and applies to current resources. However, ifm_resourcesis null when called, the policy is stored butcreate()callsset_stage_policy(true, true)unconditionally at line 20, which would overwrite any previously set policy.This is likely acceptable given the intended usage (create then configure), but worth noting if the order might vary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/chain/chain_runtime.cpp` around lines 74 - 84, The policy stored by ChainRuntime::set_stage_policy may never be applied if m_resources is null when called because create() currently calls set_stage_policy(true, true) unconditionally and can overwrite previously saved state; change create() (or the resource initialization path) so that after constructing m_resources it applies the stored member flags (m_prechain_policy_enabled, m_effect_stage_policy_enabled) instead of hardcoding true/true, or update create() to call set_stage_policy with those member variables so any policy set before resources exist is honored.src/render/chain/chain_executor.cpp (1)
27-39: Layout transition logic assumes only two cases.The
transition_image_layouthelper only handles transitions toeColorAttachmentOptimaland implicitly assumes everything else is transitioning from it. If called with other layouts, the access masks and pipeline stages would be incorrect.Consider adding explicit handling or an assertion for unexpected layouts to avoid silent misuse.
🛡️ Defensive improvement
vk::PipelineStageFlags src_stage; vk::PipelineStageFlags dst_stage; if (transition.to == vk::ImageLayout::eColorAttachmentOptimal) { barrier.srcAccessMask = vk::AccessFlagBits::eShaderRead; barrier.dstAccessMask = vk::AccessFlagBits::eColorAttachmentWrite; src_stage = vk::PipelineStageFlagBits::eFragmentShader; dst_stage = vk::PipelineStageFlagBits::eColorAttachmentOutput; - } else { + } else if (transition.to == vk::ImageLayout::eShaderReadOnlyOptimal) { barrier.srcAccessMask = vk::AccessFlagBits::eColorAttachmentWrite; barrier.dstAccessMask = vk::AccessFlagBits::eShaderRead; src_stage = vk::PipelineStageFlagBits::eColorAttachmentOutput; dst_stage = vk::PipelineStageFlagBits::eFragmentShader; + } else { + assert(false && "Unsupported layout transition"); + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/chain/chain_executor.cpp` around lines 27 - 39, The layout transition code in transition_image_layout currently only handles when transition.to == vk::ImageLayout::eColorAttachmentOptimal and treats all other cases as the reverse, which can produce wrong barrier.srcAccessMask/dstAccessMask and src_stage/dst_stage for other layouts; update transition_image_layout (and the use of transition.to and the barrier, src_stage, dst_stage variables) to explicitly handle all expected vk::ImageLayout values (e.g., eUndefined, eTransferDstOptimal, eShaderReadOnlyOptimal, ePresentSrcKHR, etc.) using a switch or if/else branches mapping correct src/dst access masks and pipeline stages for each pair, and add a default branch that either asserts/fails (e.g., throw or assert) or logs an error for unsupported layouts so misuse is not silent.src/render/chain/chain_resources.cpp (1)
318-333: Minor inefficiency: Parameter override applied to all passes unconditionally.
set_parameter_overrideandupdate_ubo_parametersare called on every pass even if the parameter doesn't exist in that pass. While functionally correct (passes ignore unknown parameters), this could be optimized.♻️ Optional optimization
void ChainResources::set_parameter(const std::string& name, float value) { bool found = false; for (auto& pass : m_passes) { + bool pass_has_param = false; for (const auto& param : pass->parameters()) { if (param.name == name) { found = true; + pass_has_param = true; break; } } - pass->set_parameter_override(name, value); - GOGGLES_MUST(pass->update_ubo_parameters()); + if (pass_has_param) { + pass->set_parameter_override(name, value); + GOGGLES_MUST(pass->update_ubo_parameters()); + } } if (!found) { GOGGLES_LOG_WARN("set_parameter: '{}' not found in any pass", name); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/chain/chain_resources.cpp` around lines 318 - 333, The set_parameter implementation currently calls pass->set_parameter_override and pass->update_ubo_parameters for every pass even when the parameter isn't present; modify ChainResources::set_parameter so that for each pass you first scan pass->parameters() for a matching param.name and only if found call pass->set_parameter_override(name, value) and GOGGLES_MUST(pass->update_ubo_parameters()); set the found flag when any pass contains the parameter and keep the existing warning if none are found. Use the existing symbols (ChainResources::set_parameter, pass->parameters(), pass->set_parameter_override, pass->update_ubo_parameters) to locate and change the logic, leaving the warning behavior unchanged.
🤖 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/filter-chain-gate-refactor/implementation-context.json`:
- Around line 159-190: The controller/swap-safety code still calls the old
FilterChain methods (FilterChain::set_stage_policy,
FilterChain::set_prechain_resolution, FilterChain::set_control_value) so update
the orchestration to use the new ChainRuntime APIs and the retirement helper:
locate usage sites in FilterChainController::check_pending_chain_swap,
FilterChainController::reload_shader_preset, and
FilterChainController::cleanup_deferred_destroys (and any call sites in
VulkanBackend::make_filter_chain_build_config or tests) and replace direct
FilterChain control mutations with ChainRuntime calls that schedule changes and
invoke the retirement helper to retire old runtimes; ensure any logic that
relied on immediate mutation now waits for ChainRuntime-confirmed
swap/retirement before proceeding.
- Around line 112-139: Update the implementation-context JSON to reference the
new runtime surface: replace stale entries that point to filter_chain*.{hpp,cpp}
and FilterChainCore::* with the new runtime files and symbols (e.g., add
src/render/chain/chain_runtime.hpp, src/render/chain/chain_runtime.cpp,
src/render/chain/chain_resources.hpp, src/render/chain/chain_executor.cpp,
src/render/chain/chain_controls.hpp) and update candidate_symbols to include
goggles::render::ChainRuntime, goggles::render::ChainResources,
goggles::render::ChainExecutor, and goggles::render::ChainControls (and their
key methods such as ChainRuntime::record, ChainExecutor::execute,
ChainResources::load, ChainControls::apply); also update first_reads to point to
the primary read locations in the new files instead of filter_chain*.{hpp,cpp}
so verification targets the code this PR actually changes.
In `@src/render/backend/filter_chain_controller.cpp`:
- Around line 102-129: apply_runtime_state currently only logs failures from
chain.set_stage_policy and chain.set_prechain_resolution, which lets callers
proceed with swaps even when restore fails; change apply_runtime_state so it
returns a boolean (or a Result) indicating overall success, check the
vk::Extent2D prechain_resolution and ChainStagePolicy restore results (from
chain.set_stage_policy and chain.set_prechain_resolution) and treat any failure
as fatal (return false or an error) before attempting control restores, and
update callers that invoke apply_runtime_state to abort the async/swap paths
when it reports failure so the swap does not proceed on restore failure; keep
the per-control logging/continue behavior for set_control_value but ensure
stage-policy/prechain-resolution failures block the swap.
- Around line 79-100: snapshot_runtime_controls currently collapses "error" and
"empty list" into an empty vector, causing transient list_controls() failures to
silently clear authoritative_control_overrides; change snapshot_runtime_controls
to return a nullable/explicit status (e.g.,
std::optional<std::vector<FilterChainController::ControlOverride>> or a Result
type), return std::nullopt on chain==nullptr or on controls_result error (after
logging via GOGGLES_LOG_WARN), and return the vector only on successful
controls_result; then update the call sites that assign
authoritative_control_overrides to only overwrite it when the snapshot function
returns a value (i.e., check has_value()/is_ok() before assignment), using the
existing symbols snapshot_runtime_controls, list_controls, controls_result, and
authoritative_control_overrides to locate and modify code.
In `@src/render/backend/filter_chain_controller.hpp`:
- Around line 30-36: RetiredRuntimeTracker currently hardcodes
FALLBACK_RETIRE_DELAY_FRAMES and MAX_RETIRED_RUNTIMES; change it to derive both
from the configured sync depth instead of fixed literals. Replace the constexprs
with members initialized from RuntimeBuildConfig::num_sync_indices (or accept
sync_depth in RetiredRuntimeTracker’s constructor), set the retirement delay to
that sync depth (or appropriate function of it) and size the retired_runtimes
buffer based on that depth (e.g., sync_depth or sync_depth+1), or alternatively
retire using the same fence/epoch source that RuntimeBuildConfig uses to gate
frame completion; update all uses of RetiredRuntimeTracker accordingly so no
fixed “3”/“4” assumptions remain.
In `@src/render/chain/chain_builder.cpp`:
- Around line 165-203: The load_preset_textures function has two issues: it
destroys memory before images and leaks already-loaded textures on error; fix
ChainBuilder::load_preset_textures by adding a small helper (e.g.,
cleanup_loaded_textures) that takes the VulkanContext and the registry (and
optionally the current LoadedTexture) and destroys resources in the correct
order (destroy ImageView, destroy Image, destroy Sampler, then free Memory)
consistent with ChainResources::cleanup_texture_registry, and call that helper
before every early return (after texture_loader.load_from_file failure and after
create_texture_sampler failure) so all previously stored entries in registry are
cleaned up and the current partially-created texture is also cleaned up to
prevent leaks.
- Around line 15-30: The parsing helpers parse_original_history_index and
parse_pass_feedback_index must also check the std::from_chars error code and
reject overflowed values; after calling std::from_chars, if ec ==
std::errc::result_out_of_range (or ec != std::errc{}), return std::nullopt
instead of accepting the default-initialized index; update both functions to
validate ec in addition to confirming ptr == end so overflowed numeric suffixes
are not treated as valid zero indices.
In `@tests/render/test_filter_chain_c_api_contracts.cpp`:
- Around line 629-654: The test currently only checks that
goggles_chain_create_vk accepts prechain resolutions with zero width/height but
doesn't verify the runtime preserved those values; after each successful create
(for height_only_prechain, width_only_prechain, and zero_prechain with runtime
set by goggles_chain_create_vk) call
goggles_chain_prechain_resolution_get(runtime, &out_resolution) and REQUIRE that
out_resolution.width and out_resolution.height exactly equal the original
initial_prechain_resolution values from
height_only_prechain/width_only_prechain/zero_prechain before calling
goggles_chain_destroy; apply the same round-trip checks for the vk_ex variants
as well.
---
Nitpick comments:
In `@openspec/specs/packaging/spec.md`:
- Around line 43-63: Update the spec text to use consistent terminology and
clarify a technical phrase: replace "a packaged Goggles runtime" (in the Viewer
scenario) with "a Goggles AppImage" to match other scenarios, and add a short
parenthetical to the phrase "runtime/system/fallback cursor sourcing" (e.g.,
"(system cursor themes or built-in fallbacks)") where it appears to aid
non-domain readers; ensure these edits occur in the "Cursor Theme Asset
Exclusion" section around the lines containing those phrases so the SHALL
statements and scenarios retain exact meaning.
In `@src/render/backend/filter_chain_controller.cpp`:
- Around line 131-190: Add a compile-time check and a short doc comment in the
RetiredRuntimeTracker definition to assert the safety bound: insert a
static_assert that RetiredRuntimeTracker::FALLBACK_RETIRE_DELAY_FRAMES >
RenderOutput::MAX_FRAMES_IN_FLIGHT and add a brief comment that
FALLBACK_RETIRE_DELAY_FRAMES is chosen so frame_count (which increments per
render start) will outlive GPU in-flight frames; reference
RetiredRuntimeTracker, FALLBACK_RETIRE_DELAY_FRAMES,
RenderOutput::MAX_FRAMES_IN_FLIGHT and frame_count so the assumption is explicit
and verified at compile time.
In `@src/render/chain/chain_controls.cpp`:
- Around line 105-109: The value is being normalized twice: set_control_value
calls normalize_control_value before calling
ChainControls::remember_control_value, which calls normalize_control_value
again; update ChainControls::remember_control_value to assume the incoming float
is already normalized and store it directly into the appropriate overrides map
(m_prechain_overrides or m_effect_overrides) using descriptor.name, removing the
extra call to normalize_control_value (leave FilterControlDescriptor,
FilterControlStage checks and map selection intact).
In `@src/render/chain/chain_executor.cpp`:
- Around line 27-39: The layout transition code in transition_image_layout
currently only handles when transition.to ==
vk::ImageLayout::eColorAttachmentOptimal and treats all other cases as the
reverse, which can produce wrong barrier.srcAccessMask/dstAccessMask and
src_stage/dst_stage for other layouts; update transition_image_layout (and the
use of transition.to and the barrier, src_stage, dst_stage variables) to
explicitly handle all expected vk::ImageLayout values (e.g., eUndefined,
eTransferDstOptimal, eShaderReadOnlyOptimal, ePresentSrcKHR, etc.) using a
switch or if/else branches mapping correct src/dst access masks and pipeline
stages for each pair, and add a default branch that either asserts/fails (e.g.,
throw or assert) or logs an error for unsupported layouts so misuse is not
silent.
In `@src/render/chain/chain_resources.cpp`:
- Around line 318-333: The set_parameter implementation currently calls
pass->set_parameter_override and pass->update_ubo_parameters for every pass even
when the parameter isn't present; modify ChainResources::set_parameter so that
for each pass you first scan pass->parameters() for a matching param.name and
only if found call pass->set_parameter_override(name, value) and
GOGGLES_MUST(pass->update_ubo_parameters()); set the found flag when any pass
contains the parameter and keep the existing warning if none are found. Use the
existing symbols (ChainResources::set_parameter, pass->parameters(),
pass->set_parameter_override, pass->update_ubo_parameters) to locate and change
the logic, leaving the warning behavior unchanged.
In `@src/render/chain/chain_resources.hpp`:
- Around line 95-133: Add a brief comment above the public member block in
chain_resources.hpp explaining that these members (e.g., VulkanContext m_vk_ctx,
m_passes, m_framebuffers, m_texture_registry, m_prechain_passes,
m_postchain_passes, etc.) are intentionally public to allow direct access by
ChainExecutor for performance and simplicity; mention that ChainExecutor is the
intended consumer, that invariants/state ownership (like lifetime of
ShaderRuntime* m_shader_runtime and ownership of textures/framebuffers) must be
respected by callers, and point to any expected synchronization rules (e.g.,
access from the render thread only or atomic flags like
m_bypass_enabled/m_prechain_enabled) so future maintainers understand the design
decision and safe usage.
In `@src/render/chain/chain_runtime.cpp`:
- Around line 74-84: The policy stored by ChainRuntime::set_stage_policy may
never be applied if m_resources is null when called because create() currently
calls set_stage_policy(true, true) unconditionally and can overwrite previously
saved state; change create() (or the resource initialization path) so that after
constructing m_resources it applies the stored member flags
(m_prechain_policy_enabled, m_effect_stage_policy_enabled) instead of hardcoding
true/true, or update create() to call set_stage_policy with those member
variables so any policy set before resources exist is honored.
In `@tests/render/test_filter_chain.cpp`:
- Around line 90-93: The current source_text anchors are too brittle because
they rely on exact internal names like "resources.m_passes.size()" and
"resources.m_feedback_initialized[pass_idx] = true;"; update the searches to
anchor on higher-level, stable calls instead (e.g., find the positions using
"record_prechain(" and the start of the passes loop using a generic "for (" or
the helper call that indicates effects processing, then search for
"record_postchain(" after that position) so the test looks for the surrounding
stage/helper calls (record_prechain and record_postchain and the generic loop
start) rather than exact member/access spellings; apply the same relaxation to
the other occurrences around the second block (the anchors referenced at lines
108-111).
In `@tests/render/test_filter_controls.cpp`:
- Around line 73-99: The test currently varies both stage and name so it doesn’t
prove stage-scoped keys; update the descriptors used in TEST_CASE so they
collide on every key except stage (e.g., give effect_descriptor the same name as
prechain_descriptor or use the same explicit control id if ChainControls keys on
an id) and then call remember_control_value, replay_value_for, and
forget_control_value to assert that forgetting an effect-stage entry does not
evict the prechain-stage entry; keep the value ranges and asserts otherwise the
same and reference ChainControls, FilterControlDescriptor,
remember_control_value, replay_value_for, and forget_control_value when making
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd7b2bf5-f8af-4628-9368-cd3a64ebf329
📒 Files selected for processing (37)
openspec/changes/archive/2026-03-10-remove-gpl-cursor-theme-assets/.openspec.yamlopenspec/changes/archive/2026-03-10-remove-gpl-cursor-theme-assets/proposal.mdopenspec/changes/archive/2026-03-10-remove-gpl-cursor-theme-assets/specs/input-forwarding/spec.mdopenspec/changes/archive/2026-03-10-remove-gpl-cursor-theme-assets/specs/packaging/spec.mdopenspec/changes/archive/2026-03-10-remove-gpl-cursor-theme-assets/tasks.mdopenspec/changes/filter-chain-gate-refactor/.openspec.yamlopenspec/changes/filter-chain-gate-refactor/design.mdopenspec/changes/filter-chain-gate-refactor/implementation-context.jsonopenspec/changes/filter-chain-gate-refactor/proposal.mdopenspec/changes/filter-chain-gate-refactor/specs/filter-chain-cpp-wrapper/spec.mdopenspec/changes/filter-chain-gate-refactor/specs/filter-chain-runtime-boundary/spec.mdopenspec/changes/filter-chain-gate-refactor/tasks.mdopenspec/specs/input-forwarding/spec.mdopenspec/specs/packaging/spec.mdsrc/render/backend/filter_chain_controller.cppsrc/render/backend/filter_chain_controller.hppsrc/render/backend/vulkan_backend.cppsrc/render/chain/CMakeLists.txtsrc/render/chain/api/c/goggles_filter_chain.cppsrc/render/chain/api/c/goggles_filter_chain.hsrc/render/chain/chain_builder.cppsrc/render/chain/chain_builder.hppsrc/render/chain/chain_controls.cppsrc/render/chain/chain_controls.hppsrc/render/chain/chain_executor.cppsrc/render/chain/chain_executor.hppsrc/render/chain/chain_resources.cppsrc/render/chain/chain_resources.hppsrc/render/chain/chain_runtime.cppsrc/render/chain/chain_runtime.hppsrc/render/chain/filter_chain.cppsrc/render/chain/filter_chain_core.cpptests/render/test_filter_boundary_contracts.cpptests/render/test_filter_chain.cpptests/render/test_filter_chain_c_api_contracts.cpptests/render/test_filter_controls.cpptests/render/test_vulkan_backend_subsystem_contracts.cpp
💤 Files with no reviewable changes (2)
- src/render/chain/filter_chain.cpp
- src/render/chain/filter_chain_core.cpp
Comment on lines
+112
to
+139
| "candidate_paths": [ | ||
| "src/render/chain/filter_chain.hpp", | ||
| "src/render/chain/filter_chain.cpp", | ||
| "src/render/chain/filter_chain_core.hpp", | ||
| "src/render/chain/filter_chain_core.cpp", | ||
| "src/render/chain/preset_parser.cpp", | ||
| "src/render/chain/output_pass.cpp", | ||
| "src/render/chain/frame_history.cpp", | ||
| "src/render/chain/framebuffer.cpp", | ||
| "tests/render/test_filter_chain.cpp", | ||
| "tests/render/test_filter_controls.cpp" | ||
| ], | ||
| "candidate_symbols": [ | ||
| "goggles::render::FilterChain", | ||
| "goggles::render::FilterChainCore", | ||
| "goggles::render::FilterChain::record", | ||
| "goggles::render::FilterChainCore::load_preset", | ||
| "goggles::render::FilterChainCore::record", | ||
| "goggles::render::FilterChainCore::handle_resize" | ||
| ], | ||
| "first_reads": [ | ||
| "src/render/chain/filter_chain.hpp:23", | ||
| "src/render/chain/filter_chain_core.hpp:41", | ||
| "src/render/chain/filter_chain_core.cpp:386", | ||
| "src/render/chain/CMakeLists.txt:3", | ||
| "tests/render/test_filter_chain.cpp:84", | ||
| "tests/render/test_filter_controls.cpp:12" | ||
| ], |
There was a problem hiding this comment.
Retarget task group 2 to the new runtime files.
candidate_paths, candidate_symbols, and first_reads still point at filter_chain*.{hpp,cpp} and FilterChainCore::*, but this refactor replaces that surface with ChainRuntime, ChainResources, ChainExecutor, and ChainControls. Since this file is the implementation guide, those stale references will steer verification away from the code this PR actually changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/changes/filter-chain-gate-refactor/implementation-context.json`
around lines 112 - 139, Update the implementation-context JSON to reference the
new runtime surface: replace stale entries that point to filter_chain*.{hpp,cpp}
and FilterChainCore::* with the new runtime files and symbols (e.g., add
src/render/chain/chain_runtime.hpp, src/render/chain/chain_runtime.cpp,
src/render/chain/chain_resources.hpp, src/render/chain/chain_executor.cpp,
src/render/chain/chain_controls.hpp) and update candidate_symbols to include
goggles::render::ChainRuntime, goggles::render::ChainResources,
goggles::render::ChainExecutor, and goggles::render::ChainControls (and their
key methods such as ChainRuntime::record, ChainExecutor::execute,
ChainResources::load, ChainControls::apply); also update first_reads to point to
the primary read locations in the new files instead of filter_chain*.{hpp,cpp}
so verification targets the code this PR actually changes.
Comment on lines
+159
to
+190
| "candidate_paths": [ | ||
| "src/render/backend/filter_chain_controller.hpp", | ||
| "src/render/backend/filter_chain_controller.cpp", | ||
| "src/render/backend/vulkan_backend.cpp", | ||
| "src/render/chain/filter_chain.hpp", | ||
| "src/render/chain/filter_chain.cpp", | ||
| "src/render/chain/api/c/goggles_filter_chain.cpp", | ||
| "src/render/backend/render_output.cpp", | ||
| "tests/render/test_filter_boundary_contracts.cpp" | ||
| ], | ||
| "candidate_symbols": [ | ||
| "goggles::render::backend_internal::FilterChainController", | ||
| "FilterChainController::reload_shader_preset", | ||
| "FilterChainController::check_pending_chain_swap", | ||
| "FilterChainController::cleanup_deferred_destroys", | ||
| "goggles::render::FilterChain::set_stage_policy", | ||
| "goggles::render::FilterChain::set_prechain_resolution", | ||
| "goggles::render::FilterChain::set_control_value", | ||
| "goggles::render::FilterChain::reset_controls", | ||
| "VulkanBackend::reload_shader_preset", | ||
| "VulkanBackend::make_filter_chain_build_config" | ||
| ], | ||
| "first_reads": [ | ||
| "src/render/backend/filter_chain_controller.hpp:18", | ||
| "src/render/backend/filter_chain_controller.cpp:81", | ||
| "src/render/backend/filter_chain_controller.cpp:175", | ||
| "src/render/backend/filter_chain_controller.cpp:237", | ||
| "src/render/backend/vulkan_backend.cpp:574", | ||
| "src/render/chain/filter_chain.hpp:40", | ||
| "src/render/chain/filter_chain.cpp:137", | ||
| "src/render/chain/api/c/goggles_filter_chain.cpp:51", | ||
| "tests/render/test_filter_boundary_contracts.cpp:223" |
There was a problem hiding this comment.
Task group 3 is still auditing the pre-refactor control surface.
The controller/swap-safety group still keys off FilterChain::set_stage_policy, set_prechain_resolution, and set_control_value instead of the new ChainRuntime APIs and retirement helper. As written, the orchestration review can miss the new swap/retirement path entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/changes/filter-chain-gate-refactor/implementation-context.json`
around lines 159 - 190, The controller/swap-safety code still calls the old
FilterChain methods (FilterChain::set_stage_policy,
FilterChain::set_prechain_resolution, FilterChain::set_control_value) so update
the orchestration to use the new ChainRuntime APIs and the retirement helper:
locate usage sites in FilterChainController::check_pending_chain_swap,
FilterChainController::reload_shader_preset, and
FilterChainController::cleanup_deferred_destroys (and any call sites in
VulkanBackend::make_filter_chain_build_config or tests) and replace direct
FilterChain control mutations with ChainRuntime calls that schedule changes and
invoke the retirement helper to retire old runtimes; ensure any logic that
relied on immediate mutation now waits for ChainRuntime-confirmed
swap/retirement before proceeding.
Comment on lines
+79
to
+100
| auto snapshot_runtime_controls(const FilterChainRuntime& chain) | ||
| -> std::vector<FilterChainController::ControlOverride> { | ||
| if (!chain) { | ||
| return {}; | ||
| } | ||
|
|
||
| auto controls_result = chain.list_controls(); | ||
| if (!controls_result) { | ||
| GOGGLES_LOG_WARN("Failed to snapshot filter controls: {}", controls_result.error().message); | ||
| return {}; | ||
| } | ||
|
|
||
| std::vector<FilterChainController::ControlOverride> controls; | ||
| controls.reserve(controls_result->size()); | ||
| for (const auto& descriptor : controls_result.value()) { | ||
| controls.push_back(FilterChainController::ControlOverride{ | ||
| .control_id = descriptor.control_id, | ||
| .value = descriptor.current_value, | ||
| }); | ||
| } | ||
| return controls; | ||
| } |
There was a problem hiding this comment.
Don't collapse snapshot errors into “no overrides”.
snapshot_runtime_controls() returns {} both when the chain genuinely has no controls and when list_controls() fails. Lines 232, 290, 399, 507, 523, and 539 then overwrite authoritative_control_overrides with that empty result, so any transient wrapper error silently drops the state this refactor is trying to preserve across swaps.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/render/backend/filter_chain_controller.cpp` around lines 79 - 100,
snapshot_runtime_controls currently collapses "error" and "empty list" into an
empty vector, causing transient list_controls() failures to silently clear
authoritative_control_overrides; change snapshot_runtime_controls to return a
nullable/explicit status (e.g.,
std::optional<std::vector<FilterChainController::ControlOverride>> or a Result
type), return std::nullopt on chain==nullptr or on controls_result error (after
logging via GOGGLES_LOG_WARN), and return the vector only on successful
controls_result; then update the call sites that assign
authoritative_control_overrides to only overwrite it when the snapshot function
returns a value (i.e., check has_value()/is_ok() before assignment), using the
existing symbols snapshot_runtime_controls, list_controls, controls_result, and
authoritative_control_overrides to locate and modify code.
Comment on lines
+30
to
+36
| struct RetiredRuntimeTracker { | ||
| static constexpr size_t MAX_RETIRED_RUNTIMES = 4; | ||
| static constexpr uint64_t FALLBACK_RETIRE_DELAY_FRAMES = 3; | ||
|
|
||
| std::array<RetiredRuntime, MAX_RETIRED_RUNTIMES> retired_runtimes{}; | ||
| size_t retired_count = 0; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Tie the fallback retirement window to the configured sync depth.
FALLBACK_RETIRE_DELAY_FRAMES = 3 plus a fixed 4-slot queue bakes in the current frames-in-flight assumption instead of deriving it from RuntimeBuildConfig::num_sync_indices. If the backend ever runs deeper than three in-flight submissions, this helper can either exhaust its retirement budget or destroy a swapped runtime while queued GPU work still references it. Please derive both the delay and the queue bound from the actual sync depth, or retire from the same fence/epoch source that gates frame completion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/render/backend/filter_chain_controller.hpp` around lines 30 - 36,
RetiredRuntimeTracker currently hardcodes FALLBACK_RETIRE_DELAY_FRAMES and
MAX_RETIRED_RUNTIMES; change it to derive both from the configured sync depth
instead of fixed literals. Replace the constexprs with members initialized from
RuntimeBuildConfig::num_sync_indices (or accept sync_depth in
RetiredRuntimeTracker’s constructor), set the retirement delay to that sync
depth (or appropriate function of it) and size the retired_runtimes buffer based
on that depth (e.g., sync_depth or sync_depth+1), or alternatively retire using
the same fence/epoch source that RuntimeBuildConfig uses to gate frame
completion; update all uses of RetiredRuntimeTracker accordingly so no fixed
“3”/“4” assumptions remain.
Comment on lines
+15
to
+30
| auto parse_original_history_index(std::string_view name) -> std::optional<uint32_t> { | ||
| constexpr std::string_view PREFIX = "OriginalHistory"; | ||
| if (!name.starts_with(PREFIX)) { | ||
| return std::nullopt; | ||
| } | ||
| auto suffix = name.substr(PREFIX.size()); | ||
| if (suffix.empty()) { | ||
| return std::nullopt; | ||
| } | ||
| uint32_t index = 0; | ||
| const auto* end = suffix.data() + suffix.size(); | ||
| auto [ptr, ec] = std::from_chars(suffix.data(), end, index); | ||
| if (ptr != end) { | ||
| return std::nullopt; | ||
| } | ||
| return index; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "chain_builder.cpp" -type f 2>/dev/null | head -5Repository: goggles-dev/Goggles
Length of output: 45
🏁 Script executed:
find / -name "chain_builder.cpp" -type f 2>/dev/null | head -5Repository: goggles-dev/Goggles
Length of output: 116
🏁 Script executed:
git ls-files | grep -i chain_builderRepository: goggles-dev/Goggles
Length of output: 132
🏁 Script executed:
git ls-files | head -20Repository: goggles-dev/Goggles
Length of output: 430
🏁 Script executed:
git ls-files | grep renderRepository: goggles-dev/Goggles
Length of output: 6000
🏁 Script executed:
cat -n src/render/chain/chain_builder.cpp | head -80Repository: goggles-dev/Goggles
Length of output: 3019
🏁 Script executed:
cat -n tests/render/test_filter_chain.cpp | grep -A 10 -B 5 "from_chars\|parse_original_history\|parse_pass_feedback" | head -50Repository: goggles-dev/Goggles
Length of output: 2001
🏁 Script executed:
# Let's verify std::from_chars behavior with overflow
cat > /tmp/test_from_chars.cpp << 'EOF'
`#include` <charconv>
`#include` <iostream>
`#include` <string>
int main() {
// Test overflow case
std::string str = "999999999999";
uint32_t value = 0;
auto [ptr, ec] = std::from_chars(str.data(), str.data() + str.size(), value);
std::cout << "Input: " << str << "\n";
std::cout << "Parsed value: " << value << "\n";
std::cout << "Error code: " << static_cast<int>(ec) << "\n";
std::cout << "Ptr reached end: " << (ptr == str.data() + str.size()) << "\n";
std::cout << "std::errc::result_out_of_range: " << static_cast<int>(std::errc::result_out_of_range) << "\n";
return 0;
}
EOF
g++ -std=c++20 /tmp/test_from_chars.cpp -o /tmp/test_from_chars && /tmp/test_from_charsRepository: goggles-dev/Goggles
Length of output: 107
🏁 Script executed:
cat -n tests/render/test_filter_chain.cpp | sed -n '50,65p'Repository: goggles-dev/Goggles
Length of output: 731
🏁 Script executed:
web_search
std::from_chars overflow behavior uint32_tRepository: goggles-dev/Goggles
Length of output: 167
🏁 Script executed:
cat -n tests/render/test_filter_chain.cpp | grep -A 20 "OriginalHistory999\|PassFeedback999\|overflow"Repository: goggles-dev/Goggles
Length of output: 45
🌐 Web query:
std::from_chars overflow behavior result_out_of_range C++17
💡 Result:
In C++17, std::from_chars does not overflow in the sense of producing a wrapped/UB value. If the parsed numeric value is not representable in the destination type:
- The returned
from_chars_result.ecisstd::errc::result_out_of_range. from_chars_result.ptrpoints to the first character after the parsed pattern (i.e., where parsing stopped).- The output parameter
valueis left unmodified. [1][2]
So you typically write:
auto r = std::from_chars(first, last, value);
if (r.ec == std::errc::result_out_of_range) {
// overflow/underflow for the destination type; value unchanged
}Sources: cppreference std::from_chars page [1] and the C++ draft wording for [charconv.from.chars] [2].
References: [1] (en.cppreference.com) [2] (eel.is)
Citations:
- 1: https://en.cppreference.com/w/cpp/utility/from_chars.html?utm_source=openai
- 2: https://eel.is/c%2B%2Bdraft/charconv?utm_source=openai
Reject overflowed history and feedback indices by checking the error code.
Both parse_original_history_index and parse_pass_feedback_index only check whether ptr reached the end. With std::from_chars, when the numeric suffix overflows the destination type (e.g., OriginalHistory999999999999 for uint32_t), the function parses the entire input, advances ptr to the end, sets ec to result_out_of_range, but leaves the output parameter unmodified. Since index is initialized to 0, the overflowed input silently parses as a successful 0 instead of being rejected, potentially mis-detecting history depth or pass feedback indices.
🛠️ Proposed fix
auto [ptr, ec] = std::from_chars(suffix.data(), end, index);
- if (ptr != end) {
+ if (ec != std::errc{} || ptr != end) {
return std::nullopt;
}Apply the same guard in both parsing helpers.
Also applies to: 46-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/render/chain/chain_builder.cpp` around lines 15 - 30, The parsing helpers
parse_original_history_index and parse_pass_feedback_index must also check the
std::from_chars error code and reject overflowed values; after calling
std::from_chars, if ec == std::errc::result_out_of_range (or ec != std::errc{}),
return std::nullopt instead of accepting the default-initialized index; update
both functions to validate ec in addition to confirming ptr == end so overflowed
numeric suffixes are not treated as valid zero indices.
Comment on lines
+165
to
+203
| auto ChainBuilder::load_preset_textures(const VulkanContext& vk_ctx, TextureLoader& texture_loader, | ||
| const PresetConfig& preset) | ||
| -> Result<std::unordered_map<std::string, LoadedTexture>> { | ||
| GOGGLES_PROFILE_SCOPE("LoadPresetTextures"); | ||
|
|
||
| std::unordered_map<std::string, LoadedTexture> registry; | ||
|
|
||
| for (const auto& tex_config : preset.textures) { | ||
| TextureLoadConfig load_cfg{.generate_mipmaps = tex_config.mipmap, | ||
| .linear = tex_config.linear}; | ||
|
|
||
| auto tex_data_result = texture_loader.load_from_file(tex_config.path, load_cfg); | ||
| if (!tex_data_result) { | ||
| return nonstd::make_unexpected(tex_data_result.error()); | ||
| } | ||
|
|
||
| auto sampler_result = create_texture_sampler(vk_ctx, tex_config); | ||
| if (!sampler_result) { | ||
| auto& loaded = tex_data_result.value(); | ||
| if (loaded.view) { | ||
| vk_ctx.device.destroyImageView(loaded.view); | ||
| } | ||
| if (loaded.memory) { | ||
| vk_ctx.device.freeMemory(loaded.memory); | ||
| } | ||
| if (loaded.image) { | ||
| vk_ctx.device.destroyImage(loaded.image); | ||
| } | ||
| return nonstd::make_unexpected(sampler_result.error()); | ||
| } | ||
| auto sampler = sampler_result.value(); | ||
|
|
||
| auto texture_data = tex_data_result.value(); | ||
| registry[tex_config.name] = LoadedTexture{.data = texture_data, .sampler = sampler}; | ||
|
|
||
| GOGGLES_LOG_DEBUG("Loaded texture '{}' from {}", tex_config.name, | ||
| tex_config.path.filename().string()); | ||
| } | ||
| return registry; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/render/chain/chain_builder.cpp | sed -n '165,203p'Repository: goggles-dev/Goggles
Length of output: 1982
🏁 Script executed:
# Find the LoadedTexture definition to understand its structure
rg "struct LoadedTexture|class LoadedTexture" -A 5Repository: goggles-dev/Goggles
Length of output: 413
🏁 Script executed:
# Check if there are any Vulkan API calls in the same file for reference
rg "destroyImage|destroySampler|freeMemory|destroyImageView" src/render/chain/chain_builder.cpp -B 2 -A 2Repository: goggles-dev/Goggles
Length of output: 496
🏁 Script executed:
# Check if LoadedTexture or std::unordered_map have destructors or cleanup
rg "~LoadedTexture|cleanup|destroy" src/render/chain/ -B 2 -A 3Repository: goggles-dev/Goggles
Length of output: 26105
🏁 Script executed:
# Check TextureData structure to understand what needs cleanup
rg "struct TextureData|class TextureData" -A 8Repository: goggles-dev/Goggles
Length of output: 632
🏁 Script executed:
# Look for other similar error handling patterns in the codebase
rg "for.*registry|for.*loaded" src/render/chain/ -B 2 -A 2Repository: goggles-dev/Goggles
Length of output: 761
Fix Vulkan teardown order and prevent texture registry leaks on load failures.
Two critical issues in the error handling paths:
-
Invalid Vulkan teardown order (lines 187–191): Memory is freed before the image is destroyed, violating Vulkan semantics. Per the existing
ChainResources::cleanup_texture_registry()pattern, image views and images must be destroyed before their memory is freed. -
Resource leak on failures (lines 177–193): Both error paths return without cleaning up previously loaded textures and samplers already stored in
registry:- Line 177–178: Early return on texture load failure
- Line 182–193: Sampler creation failure only cleans the current texture, not the registry
The proposed fix should:
- Establish correct destruction order: view → image → sampler → memory
- Clean all textures in
registryon any failure - Use a helper function to avoid duplication
Proposed fix
auto ChainBuilder::load_preset_textures(const VulkanContext& vk_ctx, TextureLoader& texture_loader,
const PresetConfig& preset)
-> Result<std::unordered_map<std::string, LoadedTexture>> {
GOGGLES_PROFILE_SCOPE("LoadPresetTextures");
std::unordered_map<std::string, LoadedTexture> registry;
+ auto destroy_loaded_texture = [&](LoadedTexture& loaded) {
+ if (loaded.data.view) {
+ vk_ctx.device.destroyImageView(loaded.data.view);
+ }
+ if (loaded.data.image) {
+ vk_ctx.device.destroyImage(loaded.data.image);
+ }
+ if (loaded.sampler) {
+ vk_ctx.device.destroySampler(loaded.sampler);
+ }
+ if (loaded.data.memory) {
+ vk_ctx.device.freeMemory(loaded.data.memory);
+ }
+ };
for (const auto& tex_config : preset.textures) {
auto tex_data_result = texture_loader.load_from_file(tex_config.path, load_cfg);
if (!tex_data_result) {
+ for (auto& [_, loaded] : registry) {
+ destroy_loaded_texture(loaded);
+ }
return nonstd::make_unexpected(tex_data_result.error());
}
auto sampler_result = create_texture_sampler(vk_ctx, tex_config);
if (!sampler_result) {
- auto& loaded = tex_data_result.value();
- if (loaded.view) {
- vk_ctx.device.destroyImageView(loaded.view);
- }
- if (loaded.memory) {
- vk_ctx.device.freeMemory(loaded.memory);
- }
- if (loaded.image) {
- vk_ctx.device.destroyImage(loaded.image);
- }
+ LoadedTexture loaded{.data = std::move(tex_data_result.value()), .sampler = {}};
+ destroy_loaded_texture(loaded);
+ for (auto& [_, existing] : registry) {
+ destroy_loaded_texture(existing);
+ }
return nonstd::make_unexpected(sampler_result.error());
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/render/chain/chain_builder.cpp` around lines 165 - 203, The
load_preset_textures function has two issues: it destroys memory before images
and leaks already-loaded textures on error; fix
ChainBuilder::load_preset_textures by adding a small helper (e.g.,
cleanup_loaded_textures) that takes the VulkanContext and the registry (and
optionally the current LoadedTexture) and destroys resources in the correct
order (destroy ImageView, destroy Image, destroy Sampler, then free Memory)
consistent with ChainResources::cleanup_texture_registry, and call that helper
before every early return (after texture_loader.load_from_file failure and after
create_texture_sampler failure) so all previously stored entries in registry are
cleaned up and the current partially-created texture is also cleaned up to
prevent leaks.
Comment on lines
+629
to
+654
| auto height_only_prechain = create_info; | ||
| runtime = reinterpret_cast<goggles_chain_t*>(0x1); | ||
| height_only_prechain.initial_prechain_resolution = {.width = 0u, .height = 1u}; | ||
| REQUIRE(goggles_chain_create_vk(&vk_context, &height_only_prechain, &runtime) == | ||
| GOGGLES_CHAIN_STATUS_OK); | ||
| REQUIRE(runtime != nullptr); | ||
| REQUIRE(goggles_chain_destroy(&runtime) == GOGGLES_CHAIN_STATUS_OK); | ||
| REQUIRE(runtime == nullptr); | ||
|
|
||
| invalid.initial_prechain_resolution = {.width = 1u, .height = 0u}; | ||
| REQUIRE(goggles_chain_create_vk(&vk_context, &invalid, &runtime) == | ||
| GOGGLES_CHAIN_STATUS_INVALID_ARGUMENT); | ||
| auto width_only_prechain = create_info; | ||
| runtime = reinterpret_cast<goggles_chain_t*>(0x1); | ||
| width_only_prechain.initial_prechain_resolution = {.width = 1u, .height = 0u}; | ||
| REQUIRE(goggles_chain_create_vk(&vk_context, &width_only_prechain, &runtime) == | ||
| GOGGLES_CHAIN_STATUS_OK); | ||
| REQUIRE(runtime != nullptr); | ||
| REQUIRE(goggles_chain_destroy(&runtime) == GOGGLES_CHAIN_STATUS_OK); | ||
| REQUIRE(runtime == nullptr); | ||
|
|
||
| auto zero_prechain = create_info; | ||
| runtime = reinterpret_cast<goggles_chain_t*>(0x1); | ||
| zero_prechain.initial_prechain_resolution = {.width = 0u, .height = 0u}; | ||
| REQUIRE(goggles_chain_create_vk(&vk_context, &zero_prechain, &runtime) == | ||
| GOGGLES_CHAIN_STATUS_OK); | ||
| REQUIRE(runtime != nullptr); | ||
| REQUIRE(goggles_chain_destroy(&runtime) == GOGGLES_CHAIN_STATUS_OK); | ||
| REQUIRE(runtime == nullptr); |
There was a problem hiding this comment.
Round-trip the accepted create-time prechain resolutions.
These cases only prove goggles_chain_create_vk() / goggles_chain_create_vk_ex() stopped rejecting {0,h}, {w,0}, and {0,0}. A runtime that silently normalizes those values would still pass, so please read them back with goggles_chain_prechain_resolution_get() before destroy.
🧪 Suggested assertion pattern
auto zero_prechain = create_info;
runtime = reinterpret_cast<goggles_chain_t*>(0x1);
zero_prechain.initial_prechain_resolution = {.width = 0u, .height = 0u};
REQUIRE(goggles_chain_create_vk(&vk_context, &zero_prechain, &runtime) ==
GOGGLES_CHAIN_STATUS_OK);
REQUIRE(runtime != nullptr);
+goggles_chain_extent2d_t resolution{};
+REQUIRE(goggles_chain_prechain_resolution_get(runtime, &resolution) ==
+ GOGGLES_CHAIN_STATUS_OK);
+REQUIRE(resolution.width == 0u);
+REQUIRE(resolution.height == 0u);
REQUIRE(goggles_chain_destroy(&runtime) == GOGGLES_CHAIN_STATUS_OK);
REQUIRE(runtime == nullptr);Repeat the same round-trip check for the width-only/height-only cases and the vk_ex variants.
Also applies to: 706-731
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/render/test_filter_chain_c_api_contracts.cpp` around lines 629 - 654,
The test currently only checks that goggles_chain_create_vk accepts prechain
resolutions with zero width/height but doesn't verify the runtime preserved
those values; after each successful create (for height_only_prechain,
width_only_prechain, and zero_prechain with runtime set by
goggles_chain_create_vk) call goggles_chain_prechain_resolution_get(runtime,
&out_resolution) and REQUIRE that out_resolution.width and out_resolution.height
exactly equal the original initial_prechain_resolution values from
height_only_prechain/width_only_prechain/zero_prechain before calling
goggles_chain_destroy; apply the same round-trip checks for the vk_ex variants
as well.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/render/backend/filter_chain_controller.cpp (1)
79-100:⚠️ Potential issue | 🟠 MajorTransient errors still collapse into empty overrides, silently dropping state.
The previous review flagged this:
snapshot_runtime_controls()returns{}both when the chain genuinely has no controls and whenlist_controls()fails (line 88). This causes all call sites (lines 203, 242, 300, 325, 418, 526, 542, 558) to overwriteauthoritative_control_overrideswith an empty vector on transient errors, silently discarding the state this refactor aims to preserve.Consider returning
std::optional<std::vector<ControlOverride>>and only overwritingauthoritative_control_overrideswhen the snapshot succeeds.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/backend/filter_chain_controller.cpp` around lines 79 - 100, snapshot_runtime_controls currently returns an empty vector for both "no controls" and "list_controls() failed", causing transient errors to erase authoritative_control_overrides; change snapshot_runtime_controls signature to return std::optional<std::vector<FilterChainController::ControlOverride>> and have it return nullopt on errors from FilterChainRuntime::list_controls() (use controls_result.error() path) and return an empty vector only when the chain exists but has no descriptors; update call sites that assign authoritative_control_overrides (referencing authoritative_control_overrides at the call locations) to only overwrite when the optional has a value, leaving existing overrides untouched on nullopt.
🤖 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/filter_chain_controller.cpp`:
- Around line 79-100: snapshot_runtime_controls currently returns an empty
vector for both "no controls" and "list_controls() failed", causing transient
errors to erase authoritative_control_overrides; change
snapshot_runtime_controls signature to return
std::optional<std::vector<FilterChainController::ControlOverride>> and have it
return nullopt on errors from FilterChainRuntime::list_controls() (use
controls_result.error() path) and return an empty vector only when the chain
exists but has no descriptors; update call sites that assign
authoritative_control_overrides (referencing authoritative_control_overrides at
the call locations) to only overwrite when the optional has a value, leaving
existing overrides untouched on nullopt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f485a866-26ee-4d72-be39-5b87ceb5a50d
📒 Files selected for processing (2)
src/render/backend/filter_chain_controller.cpptests/render/test_filter_boundary_contracts.cpp
This was referenced Mar 12, 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
Bug Fixes
Chores