Use VK_KHR_present_wait for Frame Pacing#47
Conversation
|
Warning Rate limit exceeded@K1ngst0m has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 8 minutes and 42 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughConsolidates render settings into a new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Parser
participant App as Application
participant Config as Config System
participant Backend as Vulkan Backend
participant Device as GPU/Device
participant Swapchain as Swapchain/Queue
CLI->>Config: Parse/override target_fps (--target-fps)
App->>Config: Load config (render.target_fps)
App->>Backend: create(RenderSettings)
Backend->>Device: Query VK_KHR_present_wait support
Device-->>Backend: present_wait_supported flag
Backend->>Swapchain: Create swapchain (FIFO if present_wait else MAILBOX/FIFO)
loop Frame loop
App->>Backend: submit_and_present()
alt present_wait_supported
Backend->>Swapchain: Submit with present ID
Backend->>Backend: apply_present_wait(present_id)
Swapchain-->>Backend: Present completion
else
Backend->>Swapchain: Submit frame
Backend->>Backend: throttle_present() per target_fps
Swapchain-->>Backend: Present completion
end
end
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. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
K1ngst0m
force-pushed
the
dev/improve-frame-pacing
branch
from
5bd2b8f to
25f927b
Compare
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/app/main.cpp:
- Around line 380-387: There is a duplicated CLI override block for target_fps
in main.cpp causing double assignment and duplicate logs; remove the redundant
if (cli_opts.target_fps.has_value()) { ... } block so only one check sets
config.render.target_fps = *cli_opts.target_fps and calls
GOGGLES_LOG_INFO("Target FPS overridden by CLI: {}", config.render.target_fps);
keep the first (or preferred) occurrence and delete the second to avoid repeated
assignment/logging.
🧹 Nitpick comments (3)
openspec/changes/update-present-wait-frame-pacing/tasks.md (1)
1-14: Clarify that this is a task checklist (not a requirements/spec doc) to avoid openspec scenario-format expectations.This lives under
openspec/**/*.md, where we have scenario-format guidelines for requirements/specs; a one-liner note (or moving requirement-like statements into the spec doc) can prevent future “format drift” confusion. Based on coding guidelines / learnings.tests/util/test_config.cpp (1)
198-214: Good coverage oftarget_fps = 0(uncapped). Consider unique temp paths to avoid parallel-test collisions.
Using fixed names underutil/test_data/*.tomlcan be flaky if tests ever run concurrently; generating a unique filename instd::filesystem::temp_directory_path()would make this more robust.src/render/backend/vulkan_backend.cpp (1)
1361-1382: CPU-side frame pacing fallback looks correct.The implementation properly handles:
- Uncapped mode (
m_target_fps == 0) with early return- First-frame initialization
- Catching up when behind schedule
One minor note: the check
m_last_present_time.time_since_epoch().count() == 0works for detecting default-constructedtime_point, but could be fragile if the epoch happens to be crossed. Consider usingstd::optional<time_point>for clearer semantics, though this is unlikely to be an issue in practice.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
openspec/changes/update-present-wait-frame-pacing/proposal.mdopenspec/changes/update-present-wait-frame-pacing/specs/app-window/spec.mdopenspec/changes/update-present-wait-frame-pacing/specs/render-pipeline/spec.mdopenspec/changes/update-present-wait-frame-pacing/tasks.mdsrc/app/application.cppsrc/app/cli.hppsrc/app/main.cppsrc/render/backend/vulkan_backend.cppsrc/render/backend/vulkan_backend.hppsrc/util/config.cppsrc/util/config.hpptests/util/test_config.cpp
🧰 Additional context used
📓 Path-based instructions (3)
openspec/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/**/*.md: Use#### Scenario:format (4 hashtags) for scenario headers in requirements, not bullets or bold text, with at least one scenario per requirement
Use format- **WHEN** [condition] - **THEN** [expected result]for scenario steps in requirements
Files:
openspec/changes/update-present-wait-frame-pacing/proposal.mdopenspec/changes/update-present-wait-frame-pacing/specs/app-window/spec.mdopenspec/changes/update-present-wait-frame-pacing/tasks.mdopenspec/changes/update-present-wait-frame-pacing/specs/render-pipeline/spec.md
openspec/changes/**/proposal.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
List spec deltas in
proposal.mdunder 'What Changes' section, marking breaking changes with BREAKING
Files:
openspec/changes/update-present-wait-frame-pacing/proposal.md
openspec/changes/**/specs/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/changes/**/specs/**/*.md: Write spec deltas using## ADDED|MODIFIED|REMOVED|RENAMED Requirementsformat with at least one#### Scenario:per requirement in spec files
When modifying existing requirements in a MODIFIED delta, paste the full requirement block including the header and all scenarios, as the archiver will replace the entire requirement
Use ADDED for new capabilities that can stand alone; use MODIFIED for changes to existing requirement behavior, scope, or acceptance criteria; use RENAMED for name-only changes
Files:
openspec/changes/update-present-wait-frame-pacing/specs/app-window/spec.mdopenspec/changes/update-present-wait-frame-pacing/specs/render-pipeline/spec.md
🧠 Learnings (22)
📓 Common learnings
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/capture/vk_layer/**/*.{cpp,hpp} : Do not perform blocking operations or file I/O in capture layer performance-critical paths (e.g., vkQueuePresentKHR)
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Main thread owns: Vulkan instance, device, swapchain lifecycle; queue submission; window events and user input; job coordination. Main thread MUST NOT: block on I/O, perform heavy computation (>1ms), allocate memory in per-frame code paths.
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/render/**/*.{cpp,hpp} : Use `vk::Unique*` for long-lived Vulkan resources; use plain handles for per-frame/GPU-async resources
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Vulkan resource management: Follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types. Call `device.waitIdle()` or wait on fences before destroying GPU-async resources. Store creation info with resources for debugging/recreation. Never leak Vulkan objects. Member ordering: declare in reverse destruction order (device before instance).
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer threading: Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR` hot path. Use atomics or locks for thread-safe layer state.
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Single-threaded render loop on main thread by default. Render backend (`goggles::render`) runs on main thread. Pipeline execution runs on main thread. Job system for non-render work on worker threads. Threading introduced only when profiling justifies it (main thread CPU consistently >8ms).
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Vulkan capture layer is the primary frame capture method, intercepting rendered frames before presentation for zero-copy access
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/tests/**/*.{cpp,hpp} : Focus unit tests on non-GPU logic: utility functions, configuration parsing, and pipeline graph logic
Applied to files:
tests/util/test_config.cppsrc/app/main.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/tests/**/*.cpp : Use Catch2 v3 as the project-wide testing framework. Header-only option, provided by Pixi environment, loaded via `find_package`.
Applied to files:
tests/util/test_config.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Per-frame code paths on main thread MUST NOT: perform dynamic memory allocations (`new`, `malloc`, `std::make_shared`); use blocking synchronization primitives (`std::mutex`, `std::condition_variable`); exceed 8ms CPU time budget (excluding GPU sync).
Applied to files:
src/util/config.cppopenspec/changes/update-present-wait-frame-pacing/proposal.mdsrc/render/backend/vulkan_backend.cppsrc/util/config.hppopenspec/changes/update-present-wait-frame-pacing/tasks.mdopenspec/changes/update-present-wait-frame-pacing/specs/render-pipeline/spec.mdsrc/app/main.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/capture/vk_layer/**/*.{cpp,hpp} : Do not perform blocking operations or file I/O in capture layer performance-critical paths (e.g., vkQueuePresentKHR)
Applied to files:
openspec/changes/update-present-wait-frame-pacing/proposal.mdsrc/render/backend/vulkan_backend.cppopenspec/changes/update-present-wait-frame-pacing/tasks.mdopenspec/changes/update-present-wait-frame-pacing/specs/render-pipeline/spec.mdsrc/render/backend/vulkan_backend.hpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Main thread owns: Vulkan instance, device, swapchain lifecycle; queue submission; window events and user input; job coordination. Main thread MUST NOT: block on I/O, perform heavy computation (>1ms), allocate memory in per-frame code paths.
Applied to files:
openspec/changes/update-present-wait-frame-pacing/proposal.mdsrc/render/backend/vulkan_backend.cppopenspec/changes/update-present-wait-frame-pacing/tasks.mdopenspec/changes/update-present-wait-frame-pacing/specs/render-pipeline/spec.mdsrc/app/application.cppsrc/render/backend/vulkan_backend.hpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/render/**/*.{cpp,hpp} : Use `vk::Unique*` for long-lived Vulkan resources; use plain handles for per-frame/GPU-async resources
Applied to files:
openspec/changes/update-present-wait-frame-pacing/proposal.mdsrc/render/backend/vulkan_backend.cppsrc/app/application.cppsrc/render/backend/vulkan_backend.hpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer threading: Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR` hot path. Use atomics or locks for thread-safe layer state.
Applied to files:
openspec/changes/update-present-wait-frame-pacing/proposal.mdsrc/render/backend/vulkan_backend.cppopenspec/changes/update-present-wait-frame-pacing/specs/render-pipeline/spec.mdsrc/render/backend/vulkan_backend.hpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Vulkan resource management: Follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types. Call `device.waitIdle()` or wait on fences before destroying GPU-async resources. Store creation info with resources for debugging/recreation. Never leak Vulkan objects. Member ordering: declare in reverse destruction order (device before instance).
Applied to files:
openspec/changes/update-present-wait-frame-pacing/proposal.mdsrc/render/backend/vulkan_backend.cppopenspec/changes/update-present-wait-frame-pacing/specs/render-pipeline/spec.mdsrc/app/application.cppsrc/render/backend/vulkan_backend.hpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Single-threaded render loop on main thread by default. Render backend (`goggles::render`) runs on main thread. Pipeline execution runs on main thread. Job system for non-render work on worker threads. Threading introduced only when profiling justifies it (main thread CPU consistently >8ms).
Applied to files:
openspec/changes/update-present-wait-frame-pacing/proposal.mdsrc/render/backend/vulkan_backend.cppopenspec/changes/update-present-wait-frame-pacing/specs/render-pipeline/spec.mdsrc/app/application.cppsrc/render/backend/vulkan_backend.hpp
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Applies to openspec/changes/**/specs/**/*.md : Use ADDED for new capabilities that can stand alone; use MODIFIED for changes to existing requirement behavior, scope, or acceptance criteria; use RENAMED for name-only changes
Applied to files:
openspec/changes/update-present-wait-frame-pacing/specs/app-window/spec.mdopenspec/changes/update-present-wait-frame-pacing/tasks.mdopenspec/changes/update-present-wait-frame-pacing/specs/render-pipeline/spec.md
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Application code MUST use vulkan-hpp (C++ bindings), NOT raw Vulkan C API. Use `vk::` types (e.g., `vk::Instance`, `vk::Device`). Do not use raw C handles like `VkInstance`, `VkDevice`. Required configuration: `#define VULKAN_HPP_NO_EXCEPTIONS`, `#define VULKAN_HPP_DISPATCH_LOADER_DYNAMIC 1`, `#include <vulkan/vulkan.hpp>`.
Applied to files:
src/render/backend/vulkan_backend.cppsrc/app/application.cppsrc/render/backend/vulkan_backend.hpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : All vulkan-hpp calls returning `vk::Result` must be explicitly checked. Prohibited pattern: `static_cast<void>(device.waitIdle())`. Use macro `VK_TRY(call, ErrorCode, "message")` for vk::Result early return or manual pattern with error handling.
Applied to files:
src/render/backend/vulkan_backend.cppopenspec/changes/update-present-wait-frame-pacing/specs/render-pipeline/spec.mdsrc/render/backend/vulkan_backend.hpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/render/**/*.{cpp,hpp} : Application code MUST use vulkan-hpp with `VULKAN_HPP_NO_EXCEPTIONS` and dynamic dispatch
Applied to files:
src/render/backend/vulkan_backend.cppsrc/app/application.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Default to single-threaded render loop on main thread; use central job system (goggles::util::JobSystem) for any task parallelism
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/**/*.cpp : All concurrent processing MUST use the project's central job system (`goggles::util::JobSystem`). Phase 1: BS::thread_pool. Phase 2: Taskflow. Direct use of `std::thread` or `std::jthread` for pipeline work is PROHIBITED. Exception: External integration code (networking, IPC) outside real-time path may use `std::jthread` with RAII.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/app/**/*.{cpp,hpp} : Do not perform dynamic memory allocations or use blocking synchronization primitives in per-frame main render loop code paths
Applied to files:
src/render/backend/vulkan_backend.cppsrc/util/config.hppsrc/app/main.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Vulkan capture layer is the primary frame capture method, intercepting rendered frames before presentation for zero-copy access
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Create a change proposal with `proposal.md`, `tasks.md`, and optional `design.md` under `openspec/changes/<change-id>/` before implementing changes to add features, make breaking changes, or update architecture
Applied to files:
openspec/changes/update-present-wait-frame-pacing/tasks.md
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Testing scope: Only non-GPU logic tested initially. In scope: utility functions, configuration parsing, error handling, pipeline graph logic. Out of scope: Vulkan initialization, GPU resource management, rendering, capture layer.
Applied to files:
openspec/changes/update-present-wait-frame-pacing/tasks.mdsrc/render/backend/vulkan_backend.hpp
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Read `proposal.md`, `design.md` (if exists), and `tasks.md` in order, then implement tasks sequentially before updating checklist status to completed
Applied to files:
openspec/changes/update-present-wait-frame-pacing/tasks.md
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : RAII handle guidelines for Vulkan resources: Use `vk::Unique*` only for appropriate resource types: Instance, Device, Surface (long-lived singletons); Swapchain, Pipelines, Layouts (created once). Use plain `vk::` handles for command buffers (pooled lifetime), per-frame sync primitives (reused), and imported external images (requires explicit sync).
Applied to files:
src/render/backend/vulkan_backend.hpp
🧬 Code graph analysis (2)
src/app/application.cpp (2)
src/app/application.hpp (1)
config(30-31)src/render/backend/vulkan_backend.cpp (2)
create(170-213)create(170-172)
src/render/backend/vulkan_backend.hpp (2)
src/util/config.hpp (1)
nodiscard(19-31)src/render/chain/pass.hpp (1)
nodiscard(54-130)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and Test (test preset)
- GitHub Check: Static Analysis (clang-tidy)
🔇 Additional comments (23)
src/util/config.hpp (1)
46-52: Comment update matches new semantics (0 = uncapped).tests/util/test_config.cpp (1)
94-111: Updated assertion matches new valid range (0-1000).src/app/cli.hpp (1)
7-65: CLI11 correctly supports this pattern—no action required.Verification confirms that CLI11 supports
std::optional<T>option types and validators (includingRange) are applied only when the option is provided. TheRange(0u, 1000u)validator is inclusive on both bounds and correctly accepts--target-fps 0. The codebase validates this behavior at the config file level (tests include 0 as a valid FPS value), and the code properly handles the optional with.has_value()checks. No functional issues exist.openspec/changes/update-present-wait-frame-pacing/specs/app-window/spec.md (1)
1-17: LGTM! Spec format is correct.The spec follows the required format with
## ADDED Requirements,#### Scenario:headers, and proper GIVEN-WHEN-THEN structure. The requirements clearly document the CLI override behavior for target FPS with appropriate scenarios for both setting a specific value and disabling the frame cap.As per coding guidelines, the spec delta format is correctly applied.
src/util/config.cpp (1)
64-67: LGTM! Validation correctly allows 0 for uncapped mode.The change properly updates the validation bounds to allow
target_fps = 0for uncapped presentation pacing while maintaining the upper bound of 1000. The error message clearly documents the valid range and the meaning of 0.openspec/changes/update-present-wait-frame-pacing/specs/render-pipeline/spec.md (1)
1-31: LGTM! Spec format and content are correct.The spec follows the required format with
## ADDED Requirements,#### Scenario:headers, and proper GIVEN-WHEN-THEN structure. The requirements comprehensively document the Present Wait Frame Pacing feature with scenarios covering:
- Present wait with FIFO mode when supported
- Uncapped behavior (target_fps = 0)
- Fallback to MAILBOX with CPU-side capping
- Config-driven target fps changes
All scenarios have clear acceptance criteria.
As per coding guidelines, the spec delta format is correctly applied.
openspec/changes/update-present-wait-frame-pacing/proposal.md (1)
1-25: LGTM! Proposal is well-structured and comprehensive.The proposal clearly documents the motivation, changes, and impact for the VK_KHR_present_wait frame pacing feature. The "What Changes" section appropriately lists the spec deltas, and the "Impact" section identifies all affected specs and code files. The proposal provides sufficient context for reviewers to understand the scope and rationale.
src/app/application.cpp (1)
57-63: LGTM! Clean consolidation of render settings.The refactor appropriately replaces separate
scale_modeandinteger_scaleparameters with a unifiedRenderSettingsstruct that also includestarget_fps. This improves the API design by grouping related configuration and simplifies future extensions. All values are correctly transferred from the config.src/render/backend/vulkan_backend.hpp (5)
23-27: LGTM!The
RenderSettingsstruct cleanly consolidates render configuration. The defaulttarget_fps = 60provides sensible frame pacing out of the box, with0available for uncapped mode as documented in the PR objectives.
31-33: LGTM!The consolidated
RenderSettingsparameter simplifies the API and makes it easier to add future render settings without further signature changes.
91-94: LGTM!Resetting
m_last_present_timewhentarget_fpschanges ensures correct frame pacing behavior after configuration updates.
101-102: LGTM!The helper declarations are well-designed:
is_present_wait_ready()for capability queries,apply_present_wait()with error handling for Vulkan calls, andthrottle_present()as a simple void function for CPU-side timing.Also applies to: 127-128
183-186: LGTM!Member declarations are appropriate. Using
std::chrono::steady_clock::time_pointfor timing is correct as it provides monotonic time suitable for frame pacing.src/render/backend/vulkan_backend.cpp (10)
11-11: Verify intentional blocking on main thread for frame pacing.The
<thread>include enablesstd::this_thread::sleep_until()for CPU-side throttling. Based on learnings, the main thread should avoid blocking >1ms. However, frame pacing is an intentional exception to reduce CPU/GPU usage when present-wait is unavailable. Ensure this fallback path is acceptable for the project's performance requirements.
37-40: LGTM!Optional extensions are correctly declared separately from required extensions, allowing graceful fallback when present-wait is unavailable.
170-190: LGTM!The creation flow properly extracts settings and initializes the target FPS. Calling
update_target_fps()ensures bothm_target_fpsandm_last_present_timeare initialized correctly.
403-416: LGTM!Correctly checking for both
VK_KHR_PRESENT_IDandVK_KHR_PRESENT_WAITextensions, as both are required for the present-wait functionality to work.
485-538: LGTM!The feature chain is correctly constructed, and optional extensions are only added when present-wait is supported. The feature enable pattern properly matches the query pattern.
613-630: LGTM!Present mode selection correctly implements the PR objectives: FIFO with present-wait when available (for explicit pacing), MAILBOX fallback (for lower latency without present-wait), and FIFO as the final fallback.
669-671: LGTM!Resetting present-wait state during swapchain creation ensures correct behavior after swapchain recreation.
1316-1323: LGTM!The present ID is correctly incremented and attached to the present info. This enables tracking which presents have completed via
vkWaitForPresentKHR.
1336-1343: LGTM!The pacing logic correctly uses
apply_present_waitwhen available, falling back tothrottle_presentotherwise. Error handling properly propagates failures.
1349-1359: LGTM!The implementation correctly handles the timeout calculation and treats
eTimeoutas success (expected behavior when pacing). Using the raw dispatcher call is appropriate for this extension function.
K1ngst0m
force-pushed
the
dev/improve-frame-pacing
branch
from
25f927b to
c260c74
Compare
- enable present_id/present_wait extensions when available - use present wait timeout from target_fps - throttle via sleep when present wait isn’t supported - refactor backend init with RenderSettings
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/render/backend/vulkan_backend.cpp (1)
1336-1336: Remove redundant checkpresent_value > 0.Since
present_value = ++m_present_idat line 1319 guaranteespresent_value > 0(starting from 1), the conditionpresent_value > 0at line 1336 is always true and can be simplified to justif (is_present_wait_ready()).♻️ Simplify condition
- if (is_present_wait_ready() && present_value > 0) { + if (is_present_wait_ready()) { auto wait_result = apply_present_wait(present_value);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
openspec/changes/archive/2026-01-10-update-present-wait-frame-pacing/proposal.mdopenspec/changes/archive/2026-01-10-update-present-wait-frame-pacing/specs/app-window/spec.mdopenspec/changes/archive/2026-01-10-update-present-wait-frame-pacing/specs/render-pipeline/spec.mdopenspec/changes/archive/2026-01-10-update-present-wait-frame-pacing/tasks.mdopenspec/specs/app-window/spec.mdopenspec/specs/render-pipeline/spec.mdsrc/app/application.cppsrc/app/cli.hppsrc/app/main.cppsrc/render/backend/vulkan_backend.cppsrc/render/backend/vulkan_backend.hppsrc/util/config.cppsrc/util/config.hpptests/util/test_config.cpp
✅ Files skipped from review due to trivial changes (3)
- openspec/changes/archive/2026-01-10-update-present-wait-frame-pacing/specs/render-pipeline/spec.md
- openspec/changes/archive/2026-01-10-update-present-wait-frame-pacing/tasks.md
- openspec/changes/archive/2026-01-10-update-present-wait-frame-pacing/specs/app-window/spec.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/util/config.hpp
- tests/util/test_config.cpp
- src/app/main.cpp
🧰 Additional context used
📓 Path-based instructions (3)
openspec/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/**/*.md: Use#### Scenario:format (4 hashtags) for scenario headers in requirements, not bullets or bold text, with at least one scenario per requirement
Use format- **WHEN** [condition] - **THEN** [expected result]for scenario steps in requirements
Files:
openspec/changes/archive/2026-01-10-update-present-wait-frame-pacing/proposal.mdopenspec/specs/render-pipeline/spec.mdopenspec/specs/app-window/spec.md
openspec/changes/**/proposal.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
List spec deltas in
proposal.mdunder 'What Changes' section, marking breaking changes with BREAKING
Files:
openspec/changes/archive/2026-01-10-update-present-wait-frame-pacing/proposal.md
openspec/specs/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
Use SHALL/MUST for normative requirements in specifications, avoiding 'should' or 'may' unless intentionally non-normative
Files:
openspec/specs/render-pipeline/spec.mdopenspec/specs/app-window/spec.md
🧠 Learnings (20)
📓 Common learnings
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/capture/vk_layer/**/*.{cpp,hpp} : Do not perform blocking operations or file I/O in capture layer performance-critical paths (e.g., vkQueuePresentKHR)
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Main thread owns: Vulkan instance, device, swapchain lifecycle; queue submission; window events and user input; job coordination. Main thread MUST NOT: block on I/O, perform heavy computation (>1ms), allocate memory in per-frame code paths.
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Single-threaded render loop on main thread by default. Render backend (`goggles::render`) runs on main thread. Pipeline execution runs on main thread. Job system for non-render work on worker threads. Threading introduced only when profiling justifies it (main thread CPU consistently >8ms).
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Vulkan resource management: Follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types. Call `device.waitIdle()` or wait on fences before destroying GPU-async resources. Store creation info with resources for debugging/recreation. Never leak Vulkan objects. Member ordering: declare in reverse destruction order (device before instance).
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/render/**/*.{cpp,hpp} : Use `vk::Unique*` for long-lived Vulkan resources; use plain handles for per-frame/GPU-async resources
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Testing scope: Only non-GPU logic tested initially. In scope: utility functions, configuration parsing, error handling, pipeline graph logic. Out of scope: Vulkan initialization, GPU resource management, rendering, capture layer.
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/capture/vk_layer/**/*.{cpp,hpp} : Do not perform blocking operations or file I/O in capture layer performance-critical paths (e.g., vkQueuePresentKHR)
Applied to files:
openspec/changes/archive/2026-01-10-update-present-wait-frame-pacing/proposal.mdopenspec/specs/render-pipeline/spec.mdsrc/render/backend/vulkan_backend.hppsrc/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Main thread owns: Vulkan instance, device, swapchain lifecycle; queue submission; window events and user input; job coordination. Main thread MUST NOT: block on I/O, perform heavy computation (>1ms), allocate memory in per-frame code paths.
Applied to files:
openspec/changes/archive/2026-01-10-update-present-wait-frame-pacing/proposal.mdsrc/app/application.cppopenspec/specs/render-pipeline/spec.mdsrc/render/backend/vulkan_backend.hppsrc/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer threading: Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR` hot path. Use atomics or locks for thread-safe layer state.
Applied to files:
openspec/changes/archive/2026-01-10-update-present-wait-frame-pacing/proposal.mdopenspec/specs/render-pipeline/spec.mdsrc/render/backend/vulkan_backend.hppsrc/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/render/**/*.{cpp,hpp} : Use `vk::Unique*` for long-lived Vulkan resources; use plain handles for per-frame/GPU-async resources
Applied to files:
openspec/changes/archive/2026-01-10-update-present-wait-frame-pacing/proposal.mdsrc/app/application.cppopenspec/specs/render-pipeline/spec.mdsrc/render/backend/vulkan_backend.hppsrc/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Per-frame code paths on main thread MUST NOT: perform dynamic memory allocations (`new`, `malloc`, `std::make_shared`); use blocking synchronization primitives (`std::mutex`, `std::condition_variable`); exceed 8ms CPU time budget (excluding GPU sync).
Applied to files:
openspec/changes/archive/2026-01-10-update-present-wait-frame-pacing/proposal.mdopenspec/specs/render-pipeline/spec.mdsrc/render/backend/vulkan_backend.cppsrc/util/config.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Vulkan resource management: Follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types. Call `device.waitIdle()` or wait on fences before destroying GPU-async resources. Store creation info with resources for debugging/recreation. Never leak Vulkan objects. Member ordering: declare in reverse destruction order (device before instance).
Applied to files:
openspec/changes/archive/2026-01-10-update-present-wait-frame-pacing/proposal.mdsrc/app/application.cppopenspec/specs/render-pipeline/spec.mdsrc/render/backend/vulkan_backend.hppsrc/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Single-threaded render loop on main thread by default. Render backend (`goggles::render`) runs on main thread. Pipeline execution runs on main thread. Job system for non-render work on worker threads. Threading introduced only when profiling justifies it (main thread CPU consistently >8ms).
Applied to files:
openspec/changes/archive/2026-01-10-update-present-wait-frame-pacing/proposal.mdsrc/app/application.cppopenspec/specs/render-pipeline/spec.mdsrc/render/backend/vulkan_backend.hppsrc/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Vulkan capture layer is the primary frame capture method, intercepting rendered frames before presentation for zero-copy access
Applied to files:
openspec/changes/archive/2026-01-10-update-present-wait-frame-pacing/proposal.mdsrc/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Testing scope: Only non-GPU logic tested initially. In scope: utility functions, configuration parsing, error handling, pipeline graph logic. Out of scope: Vulkan initialization, GPU resource management, rendering, capture layer.
Applied to files:
openspec/changes/archive/2026-01-10-update-present-wait-frame-pacing/proposal.mdopenspec/specs/render-pipeline/spec.mdsrc/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Application code MUST use vulkan-hpp (C++ bindings), NOT raw Vulkan C API. Use `vk::` types (e.g., `vk::Instance`, `vk::Device`). Do not use raw C handles like `VkInstance`, `VkDevice`. Required configuration: `#define VULKAN_HPP_NO_EXCEPTIONS`, `#define VULKAN_HPP_DISPATCH_LOADER_DYNAMIC 1`, `#include <vulkan/vulkan.hpp>`.
Applied to files:
src/app/application.cppopenspec/specs/render-pipeline/spec.mdsrc/render/backend/vulkan_backend.hppsrc/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/render/**/*.{cpp,hpp} : Application code MUST use vulkan-hpp with `VULKAN_HPP_NO_EXCEPTIONS` and dynamic dispatch
Applied to files:
src/app/application.cppsrc/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Applies to openspec/changes/**/specs/**/*.md : When modifying existing requirements in a MODIFIED delta, paste the full requirement block including the header and all scenarios, as the archiver will replace the entire requirement
Applied to files:
openspec/specs/render-pipeline/spec.md
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Applies to openspec/changes/**/specs/**/*.md : Use ADDED for new capabilities that can stand alone; use MODIFIED for changes to existing requirement behavior, scope, or acceptance criteria; use RENAMED for name-only changes
Applied to files:
openspec/specs/render-pipeline/spec.mdopenspec/specs/app-window/spec.md
📚 Learning: 2026-01-07T07:20:57.053Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.053Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer logging constraints: Use only `error` and `critical` levels. No file I/O or blocking operations in hot paths. Prefix all logs with `[goggles_vklayer]`. Never log in `vkQueuePresentKHR` hot path.
Applied to files:
openspec/specs/render-pipeline/spec.md
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : All vulkan-hpp calls returning `vk::Result` must be explicitly checked. Prohibited pattern: `static_cast<void>(device.waitIdle())`. Use macro `VK_TRY(call, ErrorCode, "message")` for vk::Result early return or manual pattern with error handling.
Applied to files:
openspec/specs/render-pipeline/spec.mdsrc/render/backend/vulkan_backend.hppsrc/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : RAII handle guidelines for Vulkan resources: Use `vk::Unique*` only for appropriate resource types: Instance, Device, Surface (long-lived singletons); Swapchain, Pipelines, Layouts (created once). Use plain `vk::` handles for command buffers (pooled lifetime), per-frame sync primitives (reused), and imported external images (requires explicit sync).
Applied to files:
src/render/backend/vulkan_backend.hpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Default to single-threaded render loop on main thread; use central job system (goggles::util::JobSystem) for any task parallelism
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/**/*.cpp : All concurrent processing MUST use the project's central job system (`goggles::util::JobSystem`). Phase 1: BS::thread_pool. Phase 2: Taskflow. Direct use of `std::thread` or `std::jthread` for pipeline work is PROHIBITED. Exception: External integration code (networking, IPC) outside real-time path may use `std::jthread` with RAII.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/app/**/*.{cpp,hpp} : Do not perform dynamic memory allocations or use blocking synchronization primitives in per-frame main render loop code paths
Applied to files:
src/render/backend/vulkan_backend.cpp
🧬 Code graph analysis (3)
src/app/application.cpp (1)
src/render/backend/vulkan_backend.cpp (2)
create(170-213)create(170-172)
src/render/backend/vulkan_backend.cpp (2)
src/render/backend/vulkan_backend.hpp (1)
present_id(127-127)src/util/error.hpp (1)
make_error(50-52)
src/util/config.cpp (1)
src/util/error.hpp (1)
make_error(50-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Static Analysis (clang-tidy)
- GitHub Check: Build and Test (test preset)
🔇 Additional comments (9)
src/util/config.cpp (1)
64-69: LGTM! Validation correctly allows uncapped FPS.The change from
fps <= 0tofps < 0correctly permits 0 as a valid value for uncapped frame pacing. The error message clearly communicates the intent, and the cast fromint64_ttouint32_tis safe since the validation ensuresfps >= 0.src/app/cli.hpp (1)
7-7: LGTM! CLI option properly structured.The
std::optional<uint32_t>type correctly represents an optional override, and the range validationCLI::Range(0u, 1000u)matches the config validation. The description clearly indicates that 0 means uncapped.Also applies to: 21-21, 63-64
openspec/specs/app-window/spec.md (1)
92-106: LGTM! Specification follows coding guidelines.The new requirement uses the correct format with
#### Scenario:(4 hashtags), SHALL for normative requirements, and the- **WHEN**/- **THEN**structure. Both scenarios clearly specify the expected behavior for CLI override and uncapped mode.As per coding guidelines, the specification format is correct.
openspec/specs/render-pipeline/spec.md (1)
823-851: LGTM! Comprehensive frame pacing specification.The new requirement properly documents the present wait frame pacing behavior with clear fallback logic. The scenarios cover:
- Present wait with FIFO when extension is supported
- Uncapped behavior when target_fps = 0
- MAILBOX + CPU throttling fallback when present_wait is unavailable
- FIFO as ultimate fallback if MAILBOX is also unavailable
The specification follows coding guidelines with proper use of SHALL and scenario formatting.
As per coding guidelines, the specification structure is correct.
src/render/backend/vulkan_backend.hpp (2)
23-27: LGTM! Well-structured render settings.The
RenderSettingsstruct effectively consolidates render configuration parameters. The defaulttarget_fps = 60provides a reasonable fallback, and the struct aligns with the broader API simplification of passing a single settings object instead of multiple parameters.
91-94: LGTM! Present wait integration follows project patterns.The present wait capability tracking and frame pacing methods are appropriately designed:
is_present_wait_ready()checks extension support capabilityupdate_target_fps()correctly resets timing state when FPS changesapply_present_wait()returnsResult<void>for proper error propagation with manual error checking pattern- New members (
m_present_wait_supported,m_target_fps,m_present_id,m_last_present_time) are POD types at the end, which doesn't affect destruction orderThe blocking nature of
apply_present_wait(callingvkWaitForPresentKHR) andthrottle_present(usingsleep_for) is acceptable on the main thread for frame pacing, as these are intentional synchronization/throttling operations, not I/O or computation. Error handling is properly implemented at call sites,target_fps = 0correctly yields uncapped behavior withUINT64_MAXtimeout, andRenderSettingsis properly constructed fromConfigduring initialization.src/app/application.cpp (1)
57-63: LGTM - Clean API refactoring.The consolidation of render configuration into a
RenderSettingsstruct improves the VulkanBackend::create API by reducing parameter count and making future extensions easier.src/render/backend/vulkan_backend.cpp (2)
625-629: Verify FIFO + CPU throttle interaction when target_fps differs from display refresh rate.When
is_present_wait_ready()is false and MAILBOX is unavailable, FIFO mode is used with CPU-side throttling. FIFO inherently paces to display vsync, so ensure the CPU throttle inthrottle_present()doesn't conflict with FIFO's natural pacing whentarget_fpsdoesn't match the display refresh rate (e.g., 60 FPS target on 144Hz display).Based on learnings, this PR introduces frame pacing that affects the main thread's render loop timing.
1361-1382: Consider the main thread blocking implications of CPU-side throttling.The
throttle_present()function callsstd::this_thread::sleep_until()(line 1377), which blocks the main thread for up to ~16.67ms at 60 FPS. While this is intentional frame pacing (CPU-side fallback when VK_KHR_present_wait is unavailable), ensure this aligns with the application's threading model and doesn't interfere with other main thread responsibilities like event processing.The implementation correctly handles the case where frame processing exceeds the target duration (lines 1376-1381), preventing a catch-up spiral.
Based on learnings, this is a per-frame code path on the main thread that introduces blocking sleep. While intentional for frame pacing, verify this is acceptable for the application's event loop and input responsiveness.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/render/backend/vulkan_backend.cpp (2)
1349-1362: Reconsider treating VK_TIMEOUT as success.The implementation treats
vk::Result::eTimeoutas success (line 1357-1358), allowing the frame loop to continue. While this prevents indefinite hangs (as noted in the commit message), silently ignoring timeouts may mask presentation issues or compositor problems.Consider logging a warning when timeout occurs, as it indicates the present may not have completed within the expected frame time, which could signal a problem that warrants investigation.
💡 Suggested improvement
auto wait_result = static_cast<vk::Result>(VULKAN_HPP_DEFAULT_DISPATCHER.vkWaitForPresentKHR( *m_device, *m_swapchain, present_id, timeout_ns)); - if (wait_result == vk::Result::eSuccess || wait_result == vk::Result::eTimeout) { + if (wait_result == vk::Result::eSuccess) { return {}; } + if (wait_result == vk::Result::eTimeout) { + GOGGLES_LOG_WARN("vkWaitForPresentKHR timed out after {}ms", timeout_ns / 1'000'000); + return {}; + } return make_error<void>(ErrorCode::vulkan_device_lost, "vkWaitForPresentKHR failed: " + vk::to_string(wait_result));
1364-1385: Document the trade-off of CPU-based throttling fallback.This CPU-based frame pacing is the fallback when VK_KHR_present_wait is unavailable. The
std::this_thread::sleep_untilblocks the main render thread, which conflicts with the 8ms per-frame budget from learnings. However, this is an intentional trade-off: without this throttling, MAILBOX mode would run uncapped whentarget_fpsis set.Consider adding a comment documenting this trade-off and noting that GPU-based pacing via VK_KHR_present_wait is preferred when available.
Based on learnings, main thread threading constraints apply.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/render/backend/vulkan_backend.cpp
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/capture/vk_layer/**/*.{cpp,hpp} : Do not perform blocking operations or file I/O in capture layer performance-critical paths (e.g., vkQueuePresentKHR)
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Main thread owns: Vulkan instance, device, swapchain lifecycle; queue submission; window events and user input; job coordination. Main thread MUST NOT: block on I/O, perform heavy computation (>1ms), allocate memory in per-frame code paths.
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Single-threaded render loop on main thread by default. Render backend (`goggles::render`) runs on main thread. Pipeline execution runs on main thread. Job system for non-render work on worker threads. Threading introduced only when profiling justifies it (main thread CPU consistently >8ms).
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer threading: Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR` hot path. Use atomics or locks for thread-safe layer state.
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/render/**/*.{cpp,hpp} : Use `vk::Unique*` for long-lived Vulkan resources; use plain handles for per-frame/GPU-async resources
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Vulkan resource management: Follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types. Call `device.waitIdle()` or wait on fences before destroying GPU-async resources. Store creation info with resources for debugging/recreation. Never leak Vulkan objects. Member ordering: declare in reverse destruction order (device before instance).
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Vulkan resource management: Follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types. Call `device.waitIdle()` or wait on fences before destroying GPU-async resources. Store creation info with resources for debugging/recreation. Never leak Vulkan objects. Member ordering: declare in reverse destruction order (device before instance).
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Main thread owns: Vulkan instance, device, swapchain lifecycle; queue submission; window events and user input; job coordination. Main thread MUST NOT: block on I/O, perform heavy computation (>1ms), allocate memory in per-frame code paths.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/render/**/*.{cpp,hpp} : Use `vk::Unique*` for long-lived Vulkan resources; use plain handles for per-frame/GPU-async resources
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Application code MUST use vulkan-hpp (C++ bindings), NOT raw Vulkan C API. Use `vk::` types (e.g., `vk::Instance`, `vk::Device`). Do not use raw C handles like `VkInstance`, `VkDevice`. Required configuration: `#define VULKAN_HPP_NO_EXCEPTIONS`, `#define VULKAN_HPP_DISPATCH_LOADER_DYNAMIC 1`, `#include <vulkan/vulkan.hpp>`.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : All vulkan-hpp calls returning `vk::Result` must be explicitly checked. Prohibited pattern: `static_cast<void>(device.waitIdle())`. Use macro `VK_TRY(call, ErrorCode, "message")` for vk::Result early return or manual pattern with error handling.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/capture/vk_layer/**/*.{cpp,hpp} : Do not perform blocking operations or file I/O in capture layer performance-critical paths (e.g., vkQueuePresentKHR)
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/render/**/*.{cpp,hpp} : Application code MUST use vulkan-hpp with `VULKAN_HPP_NO_EXCEPTIONS` and dynamic dispatch
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Testing scope: Only non-GPU logic tested initially. In scope: utility functions, configuration parsing, error handling, pipeline graph logic. Out of scope: Vulkan initialization, GPU resource management, rendering, capture layer.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Per-frame code paths on main thread MUST NOT: perform dynamic memory allocations (`new`, `malloc`, `std::make_shared`); use blocking synchronization primitives (`std::mutex`, `std::condition_variable`); exceed 8ms CPU time budget (excluding GPU sync).
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer threading: Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR` hot path. Use atomics or locks for thread-safe layer state.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/app/**/*.{cpp,hpp} : Do not perform dynamic memory allocations or use blocking synchronization primitives in per-frame main render loop code paths
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : RAII handle guidelines for Vulkan resources: Use `vk::Unique*` only for appropriate resource types: Instance, Device, Surface (long-lived singletons); Swapchain, Pipelines, Layouts (created once). Use plain `vk::` handles for command buffers (pooled lifetime), per-frame sync primitives (reused), and imported external images (requires explicit sync).
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.053Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.053Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer logging constraints: Use only `error` and `critical` levels. No file I/O or blocking operations in hot paths. Prefix all logs with `[goggles_vklayer]`. Never log in `vkQueuePresentKHR` hot path.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Single-threaded render loop on main thread by default. Render backend (`goggles::render`) runs on main thread. Pipeline execution runs on main thread. Job system for non-render work on worker threads. Threading introduced only when profiling justifies it (main thread CPU consistently >8ms).
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Default to single-threaded render loop on main thread; use central job system (goggles::util::JobSystem) for any task parallelism
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/**/*.cpp : All concurrent processing MUST use the project's central job system (`goggles::util::JobSystem`). Phase 1: BS::thread_pool. Phase 2: Taskflow. Direct use of `std::thread` or `std::jthread` for pipeline work is PROHIBITED. Exception: External integration code (networking, IPC) outside real-time path may use `std::jthread` with RAII.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Vulkan capture layer is the primary frame capture method, intercepting rendered frames before presentation for zero-copy access
Applied to files:
src/render/backend/vulkan_backend.cpp
🧬 Code graph analysis (1)
src/render/backend/vulkan_backend.cpp (3)
src/render/backend/vulkan_backend.hpp (1)
present_id(127-127)src/util/error.hpp (1)
make_error(50-52)src/util/config.hpp (1)
to_string(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Static Analysis (clang-tidy)
- GitHub Check: Build and Test (test preset)
🔇 Additional comments (10)
src/render/backend/vulkan_backend.cpp (10)
11-11: Verify blocking sleep on main thread aligns with threading policy.The include suggests
std::this_thread::sleep_untilis used for frame throttling (line 1380). Based on learnings, the main thread MUST NOT use blocking synchronization primitives or exceed 8ms CPU time budget in per-frame paths. Whenthrottle_present()executes with a low target FPS (e.g., 30 FPS = ~33ms per frame), the sleep can significantly exceed the 8ms budget.Consider whether this blocking approach is acceptable for the fallback path when VK_KHR_present_wait is unavailable, or if an alternative non-blocking pacing mechanism should be used.
Based on learnings, main thread threading constraints apply.
37-40: LGTM!The optional extensions are correctly paired (VK_KHR_present_id is a prerequisite for VK_KHR_present_wait) and properly checked together during device selection.
170-190: LGTM!The consolidation of render settings into a
RenderSettingsstruct improves API clarity and reduces parameter count. The extraction and application of settings is straightforward.
355-452: LGTM!The present-wait capability detection correctly requires both VK_KHR_present_id and VK_KHR_present_wait extensions, and the capability is properly propagated to the selected device.
475-559: LGTM!The device creation correctly chains present-wait feature structs, enables the required features, and conditionally includes the optional extensions only when both are supported. The pattern follows proper Vulkan initialization order.
613-632: LGTM!The present mode selection logic is sound: prefer FIFO with present-wait for precise frame pacing when available, otherwise prefer MAILBOX for lower latency, with FIFO as the guaranteed fallback. This aligns with the PR objectives.
669-670: LGTM!Initialization of present-pacing state during swapchain creation is correct and uses appropriate zero values.
1316-1343: Verify the present ID increment and conditional pacing.The present path correctly chains
PresentIdKHRand increments the present ID when VK_KHR_present_wait is supported. The conditional branching betweenapply_present_wait()andthrottle_present()implements the fallback strategy described in the PR objectives.However, verify the implementations of
apply_present_wait()andthrottle_present()to ensure they handle error conditions and timeouts correctly (reviewed separately below).
1351-1354: LGTM!The timeout calculation correctly handles both capped and uncapped modes, with the 1-second maximum providing a safety net against indefinite hangs. The integer arithmetic is safe:
1'000'000'000ULL / m_target_fpscannot overflow for reasonable FPS values (target_fps ≥ 1), and thestd::minensures the result is bounded.
489-489: Helper method implementations are present and correct.is_present_wait_ready()is a straightforward const getter in the header returningm_present_wait_supported, andupdate_target_fps()correctly updates the target FPS and resets the present time tracking. Both are properly defined and accessible for all usage sites.
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/render/backend/vulkan_backend.cpp (1)
1328-1356: Present-wait path doesn’t actually honortarget_fps(and “uncapped” still blocks).
vkWaitForPresentKHR(present_id, timeout)returns as soon as the present happens; the timeout is a maximum, so this won’t enforce a lower FPS cap (e.g., 30 FPS on a 60Hz FIFO swapchain).- When
m_target_fps == 0,apply_present_wait()still waits up to 1s every frame, which contradicts “0 = uncapped”.- Treating
eTimeoutas success can silently disable the “GPU-assisted” pacing under load.A minimal fix is to: (1) skip all pacing when
target_fps == 0, and (2) apply a real interval-based throttle even in the present-wait path (you can still use wait-for-present as a “present completed” anchor). For example:Proposed fix (make target_fps effective; keep present-wait as an anchor)
@@ - if (m_present_wait_supported && present_value > 0) { - auto wait_result = apply_present_wait(present_value); - if (!wait_result) { - return make_error<bool>(wait_result.error().code, wait_result.error().message); - } - } else { - throttle_present(); - } + if (m_target_fps != 0) { + if (m_present_wait_supported && present_value > 0) { + auto wait_result = apply_present_wait(present_value); + if (!wait_result) { + return make_error<bool>(wait_result.error().code, wait_result.error().message); + } + } + // Enforce the actual FPS interval regardless of present-wait availability. + throttle_present(); + } @@ auto VulkanBackend::apply_present_wait(uint64_t present_id) -> Result<void> { - constexpr uint64_t MAX_TIMEOUT_NS = 1'000'000'000ULL; // 1 second max - const uint64_t timeout_ns = - (m_target_fps == 0) - ? MAX_TIMEOUT_NS - : std::min(MAX_TIMEOUT_NS, static_cast<uint64_t>(1'000'000'000ULL / m_target_fps)); + // Uncapped: do not introduce extra waits. + if (m_target_fps == 0) { + return {}; + } + + constexpr uint64_t MAX_TIMEOUT_NS = 1'000'000'000ULL; // avoid unbounded stalls + const uint64_t timeout_ns = MAX_TIMEOUT_NS; auto wait_result = static_cast<vk::Result>(VULKAN_HPP_DEFAULT_DISPATCHER.vkWaitForPresentKHR( *m_device, *m_swapchain, present_id, timeout_ns)); if (wait_result == vk::Result::eSuccess || wait_result == vk::Result::eTimeout) { return {}; } return make_error<void>(ErrorCode::vulkan_device_lost, "vkWaitForPresentKHR failed: " + vk::to_string(wait_result)); }Also:
std::this_thread::sleep_until()is now in the per-frame hot path. That’s probably intended for the MAILBOX/no-present-wait fallback, but please sanity-check this against the “main thread must not block” guidance (frame pacing may be the exception, but it should be explicit).
Based on learnings, per-frame main-thread blocking should be minimized/justified.Also applies to: 1361-1397
🤖 Fix all issues with AI agents
In @src/render/backend/vulkan_backend.hpp:
- Around line 91-95: The pacing currently treats 0 as “uncapped” but still
allows apply_present_wait() and throttle_present() to introduce waits; change
both functions (apply_present_wait and throttle_present) to immediately no-op
when m_target_fps == 0 and update every call site (including the places
referenced around lines 125-127) to skip calling them when m_target_fps == 0 so
uncapped mode truly performs no extra waits; keep the existing reset of
m_last_present_time in update_target_fps but ensure it does not re-enable waits
for target_fps == 0.
🧹 Nitpick comments (2)
src/render/backend/vulkan_backend.cpp (1)
355-416: GPU selection ignores present-wait capability when choosing between “suitable” devices.
Right now a discrete GPU without present-wait will always beat an integrated GPU that supports it, even if the user explicitly cares about pacing. Consider adding a small score bump forpresent_wait_supported(or a config/CLI preference) so the feature is actually preferred when available.
Based on learnings, render-thread behavior should stay predictable and policy-driven.Also applies to: 449-453
src/render/backend/vulkan_backend.hpp (1)
9-34: RenderSettings API change is clean; consider the default fortarget_fps.
Header defaultsRenderSettings::target_fps = 60whilem_target_fpsdefaults to0. That’s fine as long as all creation paths callupdate_target_fps()(they do in this PR), but it’s easy to regress later—consider initializingm_target_fpsto 60 as well, or documenting the invariant (“always set via update_target_fps on create”).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/render/backend/vulkan_backend.cppsrc/render/backend/vulkan_backend.hpp
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/capture/vk_layer/**/*.{cpp,hpp} : Do not perform blocking operations or file I/O in capture layer performance-critical paths (e.g., vkQueuePresentKHR)
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Main thread owns: Vulkan instance, device, swapchain lifecycle; queue submission; window events and user input; job coordination. Main thread MUST NOT: block on I/O, perform heavy computation (>1ms), allocate memory in per-frame code paths.
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Single-threaded render loop on main thread by default. Render backend (`goggles::render`) runs on main thread. Pipeline execution runs on main thread. Job system for non-render work on worker threads. Threading introduced only when profiling justifies it (main thread CPU consistently >8ms).
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/render/**/*.{cpp,hpp} : Use `vk::Unique*` for long-lived Vulkan resources; use plain handles for per-frame/GPU-async resources
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer threading: Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR` hot path. Use atomics or locks for thread-safe layer state.
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Vulkan resource management: Follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types. Call `device.waitIdle()` or wait on fences before destroying GPU-async resources. Store creation info with resources for debugging/recreation. Never leak Vulkan objects. Member ordering: declare in reverse destruction order (device before instance).
Applied to files:
src/render/backend/vulkan_backend.hppsrc/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Single-threaded render loop on main thread by default. Render backend (`goggles::render`) runs on main thread. Pipeline execution runs on main thread. Job system for non-render work on worker threads. Threading introduced only when profiling justifies it (main thread CPU consistently >8ms).
Applied to files:
src/render/backend/vulkan_backend.hppsrc/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Main thread owns: Vulkan instance, device, swapchain lifecycle; queue submission; window events and user input; job coordination. Main thread MUST NOT: block on I/O, perform heavy computation (>1ms), allocate memory in per-frame code paths.
Applied to files:
src/render/backend/vulkan_backend.hppsrc/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/render/**/*.{cpp,hpp} : Use `vk::Unique*` for long-lived Vulkan resources; use plain handles for per-frame/GPU-async resources
Applied to files:
src/render/backend/vulkan_backend.hppsrc/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Application code MUST use vulkan-hpp (C++ bindings), NOT raw Vulkan C API. Use `vk::` types (e.g., `vk::Instance`, `vk::Device`). Do not use raw C handles like `VkInstance`, `VkDevice`. Required configuration: `#define VULKAN_HPP_NO_EXCEPTIONS`, `#define VULKAN_HPP_DISPATCH_LOADER_DYNAMIC 1`, `#include <vulkan/vulkan.hpp>`.
Applied to files:
src/render/backend/vulkan_backend.hppsrc/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Testing scope: Only non-GPU logic tested initially. In scope: utility functions, configuration parsing, error handling, pipeline graph logic. Out of scope: Vulkan initialization, GPU resource management, rendering, capture layer.
Applied to files:
src/render/backend/vulkan_backend.hppsrc/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/capture/vk_layer/**/*.{cpp,hpp} : Do not perform blocking operations or file I/O in capture layer performance-critical paths (e.g., vkQueuePresentKHR)
Applied to files:
src/render/backend/vulkan_backend.hppsrc/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : All vulkan-hpp calls returning `vk::Result` must be explicitly checked. Prohibited pattern: `static_cast<void>(device.waitIdle())`. Use macro `VK_TRY(call, ErrorCode, "message")` for vk::Result early return or manual pattern with error handling.
Applied to files:
src/render/backend/vulkan_backend.hppsrc/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer threading: Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR` hot path. Use atomics or locks for thread-safe layer state.
Applied to files:
src/render/backend/vulkan_backend.hppsrc/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : RAII handle guidelines for Vulkan resources: Use `vk::Unique*` only for appropriate resource types: Instance, Device, Surface (long-lived singletons); Swapchain, Pipelines, Layouts (created once). Use plain `vk::` handles for command buffers (pooled lifetime), per-frame sync primitives (reused), and imported external images (requires explicit sync).
Applied to files:
src/render/backend/vulkan_backend.hpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/render/**/*.{cpp,hpp} : Application code MUST use vulkan-hpp with `VULKAN_HPP_NO_EXCEPTIONS` and dynamic dispatch
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Per-frame code paths on main thread MUST NOT: perform dynamic memory allocations (`new`, `malloc`, `std::make_shared`); use blocking synchronization primitives (`std::mutex`, `std::condition_variable`); exceed 8ms CPU time budget (excluding GPU sync).
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/app/**/*.{cpp,hpp} : Do not perform dynamic memory allocations or use blocking synchronization primitives in per-frame main render loop code paths
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Default to single-threaded render loop on main thread; use central job system (goggles::util::JobSystem) for any task parallelism
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/**/*.cpp : All concurrent processing MUST use the project's central job system (`goggles::util::JobSystem`). Phase 1: BS::thread_pool. Phase 2: Taskflow. Direct use of `std::thread` or `std::jthread` for pipeline work is PROHIBITED. Exception: External integration code (networking, IPC) outside real-time path may use `std::jthread` with RAII.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Vulkan capture layer is the primary frame capture method, intercepting rendered frames before presentation for zero-copy access
Applied to files:
src/render/backend/vulkan_backend.cpp
🧬 Code graph analysis (2)
src/render/backend/vulkan_backend.hpp (2)
src/util/config.hpp (1)
nodiscard(19-31)src/render/chain/pass.hpp (1)
nodiscard(54-130)
src/render/backend/vulkan_backend.cpp (2)
src/render/backend/vulkan_backend.hpp (1)
present_id(125-125)src/util/error.hpp (1)
make_error(50-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Static Analysis (clang-tidy)
- GitHub Check: Build and Test (test preset)
🔇 Additional comments (5)
src/render/backend/vulkan_backend.cpp (4)
37-41: Optional extension list looks correct (present_id + present_wait as a pair).
Good to keep these as an explicit bundle since present-wait depends on present-id.
170-191: RenderSettings plumb-through is straightforward.
update_target_fps()resettingm_last_present_timeon create is a good touch for deterministic pacing state.
625-642: Present-mode fallback matches the PR description.
FIFO when present-wait is ready, otherwise MAILBOX if supported, else FIFO. Resettingm_present_id/m_last_present_timeon swapchain creation is correct.Also applies to: 681-683
482-571: No raw dispatcher calls to refactor in this section — code is already vulkan-hpp compliant.The code at lines 482–571 correctly uses vulkan-hpp throughout (
getFeatures2(),createDeviceUnique(),getQueue(),waitIdle()). TheVULKAN_HPP_DEFAULT_DISPATCHER.init(*m_device)call is the proper pattern for initializing the dynamic dispatcher with vulkan-hpp whenVULKAN_HPP_DISPATCH_LOADER_DYNAMICis enabled. No changes needed here.src/render/backend/vulkan_backend.hpp (1)
181-185: Present-wait state additions look reasonable.
m_present_idandm_last_present_timeare the right minimal state for pacing; just ensure they’re reset on swapchain recreation and on target-fps changes (both appear covered in this PR).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/render/backend/vulkan_backend.cpp (1)
1371-1384: Verify timeout behavior with mismatched refresh rates.When
m_target_fpsdiffers significantly from the display refresh rate, the timeout (line 1376:1'000'000'000ULL / m_target_fps) may frequently trigger. For example, withtarget_fps=60on a 30Hz display, the ~16.67ms timeout will expire before the ~33.33ms present completes.Since
eTimeoutreturns success, this won't break functionality, but it may reduce the effectiveness of present-wait pacing. Consider using a longer timeout (e.g., 2-3x frame duration or a fixed value like 100ms) to accommodate display variance while still protecting against hangs.💡 Suggested adjustment
- const uint64_t timeout_ns = - (m_target_fps == 0) - ? MAX_TIMEOUT_NS - : std::min(MAX_TIMEOUT_NS, static_cast<uint64_t>(1'000'000'000ULL / m_target_fps)); + // Use 3x frame duration to accommodate display refresh variance + constexpr uint64_t MIN_TIMEOUT_NS = 100'000'000ULL; // 100ms floor + const uint64_t timeout_ns = + (m_target_fps == 0) + ? MAX_TIMEOUT_NS + : std::min(MAX_TIMEOUT_NS, + std::max(MIN_TIMEOUT_NS, static_cast<uint64_t>(3'000'000'000ULL / m_target_fps)));
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/render/backend/vulkan_backend.cpp
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/capture/vk_layer/**/*.{cpp,hpp} : Do not perform blocking operations or file I/O in capture layer performance-critical paths (e.g., vkQueuePresentKHR)
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Main thread owns: Vulkan instance, device, swapchain lifecycle; queue submission; window events and user input; job coordination. Main thread MUST NOT: block on I/O, perform heavy computation (>1ms), allocate memory in per-frame code paths.
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Single-threaded render loop on main thread by default. Render backend (`goggles::render`) runs on main thread. Pipeline execution runs on main thread. Job system for non-render work on worker threads. Threading introduced only when profiling justifies it (main thread CPU consistently >8ms).
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/render/**/*.{cpp,hpp} : Use `vk::Unique*` for long-lived Vulkan resources; use plain handles for per-frame/GPU-async resources
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer threading: Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR` hot path. Use atomics or locks for thread-safe layer state.
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Vulkan resource management: Follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types. Call `device.waitIdle()` or wait on fences before destroying GPU-async resources. Store creation info with resources for debugging/recreation. Never leak Vulkan objects. Member ordering: declare in reverse destruction order (device before instance).
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Main thread owns: Vulkan instance, device, swapchain lifecycle; queue submission; window events and user input; job coordination. Main thread MUST NOT: block on I/O, perform heavy computation (>1ms), allocate memory in per-frame code paths.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : All vulkan-hpp calls returning `vk::Result` must be explicitly checked. Prohibited pattern: `static_cast<void>(device.waitIdle())`. Use macro `VK_TRY(call, ErrorCode, "message")` for vk::Result early return or manual pattern with error handling.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Application code MUST use vulkan-hpp (C++ bindings), NOT raw Vulkan C API. Use `vk::` types (e.g., `vk::Instance`, `vk::Device`). Do not use raw C handles like `VkInstance`, `VkDevice`. Required configuration: `#define VULKAN_HPP_NO_EXCEPTIONS`, `#define VULKAN_HPP_DISPATCH_LOADER_DYNAMIC 1`, `#include <vulkan/vulkan.hpp>`.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/render/**/*.{cpp,hpp} : Use `vk::Unique*` for long-lived Vulkan resources; use plain handles for per-frame/GPU-async resources
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/capture/vk_layer/**/*.{cpp,hpp} : Do not perform blocking operations or file I/O in capture layer performance-critical paths (e.g., vkQueuePresentKHR)
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/render/**/*.{cpp,hpp} : Application code MUST use vulkan-hpp with `VULKAN_HPP_NO_EXCEPTIONS` and dynamic dispatch
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Testing scope: Only non-GPU logic tested initially. In scope: utility functions, configuration parsing, error handling, pipeline graph logic. Out of scope: Vulkan initialization, GPU resource management, rendering, capture layer.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Per-frame code paths on main thread MUST NOT: perform dynamic memory allocations (`new`, `malloc`, `std::make_shared`); use blocking synchronization primitives (`std::mutex`, `std::condition_variable`); exceed 8ms CPU time budget (excluding GPU sync).
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer threading: Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR` hot path. Use atomics or locks for thread-safe layer state.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/app/**/*.{cpp,hpp} : Do not perform dynamic memory allocations or use blocking synchronization primitives in per-frame main render loop code paths
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Single-threaded render loop on main thread by default. Render backend (`goggles::render`) runs on main thread. Pipeline execution runs on main thread. Job system for non-render work on worker threads. Threading introduced only when profiling justifies it (main thread CPU consistently >8ms).
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Default to single-threaded render loop on main thread; use central job system (goggles::util::JobSystem) for any task parallelism
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/**/*.cpp : All concurrent processing MUST use the project's central job system (`goggles::util::JobSystem`). Phase 1: BS::thread_pool. Phase 2: Taskflow. Direct use of `std::thread` or `std::jthread` for pipeline work is PROHIBITED. Exception: External integration code (networking, IPC) outside real-time path may use `std::jthread` with RAII.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Vulkan capture layer is the primary frame capture method, intercepting rendered frames before presentation for zero-copy access
Applied to files:
src/render/backend/vulkan_backend.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and Test (test preset)
- GitHub Check: Static Analysis (clang-tidy)
🔇 Additional comments (8)
src/render/backend/vulkan_backend.cpp (8)
37-41: LGTM!The optional extension list correctly pairs
VK_KHR_PRESENT_IDandVK_KHR_PRESENT_WAITtogether, as present-wait requires present-id functionality.
170-190: LGTM!The consolidated
RenderSettingsparameter simplifies the API and ensures all render settings are initialized atomically during backend creation.
403-416: LGTM!The extension detection correctly requires both
VK_KHR_PRESENT_IDandVK_KHR_PRESENT_WAITto be present, as the latter depends on the former.
499-508: LGTM!Good defensive validation—the code correctly verifies that both
presentIdandpresentWaitfeatures are actually enabled by the driver, not just that the extensions are advertised. The warning at line 503 provides helpful diagnostics when the fallback is triggered.
625-652: LGTM!The present mode selection strategy is well-designed:
- FIFO + present-wait for GPU-assisted frame pacing
- Mailbox + CPU throttle as fallback for reduced input latency
- FIFO without pacing as last resort
The strategy logging aids runtime diagnostics.
691-692: LGTM!Correctly resets present-wait state on swapchain recreation to prevent stale present IDs and ensure the first-frame detection in
throttle_present()works properly.
1338-1366: LGTM!The present-id submission path is correctly implemented. The
present_value > 0check at line 1358 is technically redundant (since++m_present_idguarantees a positive value), but it's harmless and provides clarity.
1386-1407: Verify intentional main-thread sleep aligns with project threading policy.
std::this_thread::sleep_until(line 1402) blocks the main thread, which the project learnings indicate should be avoided for per-frame code paths. However, this is the deliberate fallback frame-pacing mechanism whenVK_KHR_present_waitis unavailable.Please confirm this intentional blocking is acceptable given that:
- It only activates when GPU-assisted pacing is unavailable
- Without it, MAILBOX mode would spin at uncapped rates
- The sleep duration is bounded by the target frame interval
If the policy strictly prohibits main-thread blocking, an alternative would be to yield to SDL event processing in a polling loop, though this may be less precise. Based on learnings, main thread blocking should be avoided.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/render/backend/vulkan_backend.cpp:
- Around line 1373-1388: VulkanBackend::apply_present_wait currently treats
vk::Result::eSuboptimalKHR, eErrorOutOfDateKHR and eErrorSurfaceLostKHR as
fatal; update logic so eSuccess, eTimeout and eSuboptimalKHR are treated as
success (return {}), while eErrorOutOfDateKHR and eErrorSurfaceLostKHR set
m_needs_resize = true and return {} to trigger swapchain recreation, and any
other non-success result still returns
make_error<void>(ErrorCode::vulkan_device_lost, ...) with the
vk::to_string(wait_result); locate the vkWaitForPresentKHR call and adjust the
if/else checks to explicitly handle vk::Result::eSuboptimalKHR,
vk::Result::eErrorOutOfDateKHR and vk::Result::eErrorSurfaceLostKHR as
described.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/render/backend/vulkan_backend.cpp
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/capture/vk_layer/**/*.{cpp,hpp} : Do not perform blocking operations or file I/O in capture layer performance-critical paths (e.g., vkQueuePresentKHR)
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Main thread owns: Vulkan instance, device, swapchain lifecycle; queue submission; window events and user input; job coordination. Main thread MUST NOT: block on I/O, perform heavy computation (>1ms), allocate memory in per-frame code paths.
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Single-threaded render loop on main thread by default. Render backend (`goggles::render`) runs on main thread. Pipeline execution runs on main thread. Job system for non-render work on worker threads. Threading introduced only when profiling justifies it (main thread CPU consistently >8ms).
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/render/**/*.{cpp,hpp} : Use `vk::Unique*` for long-lived Vulkan resources; use plain handles for per-frame/GPU-async resources
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer threading: Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR` hot path. Use atomics or locks for thread-safe layer state.
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Vulkan resource management: Follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types. Call `device.waitIdle()` or wait on fences before destroying GPU-async resources. Store creation info with resources for debugging/recreation. Never leak Vulkan objects. Member ordering: declare in reverse destruction order (device before instance).
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Vulkan resource management: Follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types. Call `device.waitIdle()` or wait on fences before destroying GPU-async resources. Store creation info with resources for debugging/recreation. Never leak Vulkan objects. Member ordering: declare in reverse destruction order (device before instance).
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Main thread owns: Vulkan instance, device, swapchain lifecycle; queue submission; window events and user input; job coordination. Main thread MUST NOT: block on I/O, perform heavy computation (>1ms), allocate memory in per-frame code paths.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : All vulkan-hpp calls returning `vk::Result` must be explicitly checked. Prohibited pattern: `static_cast<void>(device.waitIdle())`. Use macro `VK_TRY(call, ErrorCode, "message")` for vk::Result early return or manual pattern with error handling.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/render/**/*.{cpp,hpp} : Use `vk::Unique*` for long-lived Vulkan resources; use plain handles for per-frame/GPU-async resources
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/capture/vk_layer/**/*.{cpp,hpp} : Do not perform blocking operations or file I/O in capture layer performance-critical paths (e.g., vkQueuePresentKHR)
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Application code MUST use vulkan-hpp (C++ bindings), NOT raw Vulkan C API. Use `vk::` types (e.g., `vk::Instance`, `vk::Device`). Do not use raw C handles like `VkInstance`, `VkDevice`. Required configuration: `#define VULKAN_HPP_NO_EXCEPTIONS`, `#define VULKAN_HPP_DISPATCH_LOADER_DYNAMIC 1`, `#include <vulkan/vulkan.hpp>`.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Testing scope: Only non-GPU logic tested initially. In scope: utility functions, configuration parsing, error handling, pipeline graph logic. Out of scope: Vulkan initialization, GPU resource management, rendering, capture layer.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/render/**/*.{cpp,hpp} : Application code MUST use vulkan-hpp with `VULKAN_HPP_NO_EXCEPTIONS` and dynamic dispatch
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Per-frame code paths on main thread MUST NOT: perform dynamic memory allocations (`new`, `malloc`, `std::make_shared`); use blocking synchronization primitives (`std::mutex`, `std::condition_variable`); exceed 8ms CPU time budget (excluding GPU sync).
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer threading: Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR` hot path. Use atomics or locks for thread-safe layer state.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/app/**/*.{cpp,hpp} : Do not perform dynamic memory allocations or use blocking synchronization primitives in per-frame main render loop code paths
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Single-threaded render loop on main thread by default. Render backend (`goggles::render`) runs on main thread. Pipeline execution runs on main thread. Job system for non-render work on worker threads. Threading introduced only when profiling justifies it (main thread CPU consistently >8ms).
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Default to single-threaded render loop on main thread; use central job system (goggles::util::JobSystem) for any task parallelism
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/**/*.cpp : All concurrent processing MUST use the project's central job system (`goggles::util::JobSystem`). Phase 1: BS::thread_pool. Phase 2: Taskflow. Direct use of `std::thread` or `std::jthread` for pipeline work is PROHIBITED. Exception: External integration code (networking, IPC) outside real-time path may use `std::jthread` with RAII.
Applied to files:
src/render/backend/vulkan_backend.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Vulkan capture layer is the primary frame capture method, intercepting rendered frames before presentation for zero-copy access
Applied to files:
src/render/backend/vulkan_backend.cpp
🧬 Code graph analysis (1)
src/render/backend/vulkan_backend.cpp (4)
src/app/sdl_platform.hpp (1)
create_info(26-26)src/render/backend/vulkan_backend.hpp (1)
present_id(125-125)src/util/error.hpp (1)
make_error(50-52)src/util/config.hpp (1)
to_string(19-19)
🔇 Additional comments (9)
src/render/backend/vulkan_backend.cpp (9)
11-11: LGTM!Using
<thread>forstd::this_thread::sleep_untilin CPU-based frame throttling is appropriate and doesn't violate the project policy against direct thread creation for pipeline work.
37-41: LGTM!Clean separation of optional present-wait extensions from required device extensions.
170-191: LGTM!The API consolidation to
RenderSettingsis clean and allows for easier future extension. Initialization of settings members looks correct.
403-416: LGTM!Present-wait detection correctly requires both
VK_KHR_PRESENT_IDandVK_KHR_PRESENT_WAITextensions to be available.
499-508: LGTM!Excellent defensive validation - checking that present-wait features are actually enabled even when extensions are present, with informative fallback logging.
627-651: LGTM!Present mode selection logic correctly implements the priority: FIFO with present-wait when supported, MAILBOX with CPU throttle as fallback, and plain FIFO as last resort. The logging is helpful for debugging pacing issues.
691-692: LGTM!Correctly resets present-wait state when swapchain is recreated, preventing stale present IDs from being used with the new swapchain.
1338-1367: LGTM!Present ID handling is correctly implemented. The
VkPresentIdKHRstruct is properly chained only when present-wait is supported, and the pacing logic correctly branches between uncapped, present-wait, and CPU throttle modes.
1390-1411: LGTM!The CPU-based frame throttle is a reasonable fallback when present-wait isn't available. The sleep-based pacing is acceptable per project policies, which specifically prohibit blocking on I/O and synchronization primitives—intentional frame timing via
sleep_untilis a valid use case.One minor note: the first-frame check at line 1398 using
time_since_epoch().count() == 0works correctly for default-constructedtime_point, though an explicit flag orstd::optionalcould be slightly more readable. This is fine as-is.
K1ngst0m
force-pushed
the
dev/improve-frame-pacing
branch
from
b385b8d to
88a826b
Compare
1 participant
User description
Why
Goggles currently defaults to mailbox present mode and allows uncapped frame submission. On high-end GPUs this drives extremely high FPS, wasting power and producing inconsistent frame pacing. Vulkan's VK_KHR_present_wait lets us explicitly pace presentation and reduce unnecessary GPU/CPU work.
What Changes
render.target_fps(0 = uncapped).render.target_fps(0 = uncapped).render.target_fps(mirrors config; 0 = uncapped).Impact
render-pipeline,app-window.src/render/backend/vulkan_backend.cpp- present mode selection and pacing integration.src/render/backend/vulkan_backend.hpp- present wait capability state.src/util/config.hpp+src/util/config.cpp- allowtarget_fps = 0for uncapped.src/app/cli.hpp- CLI override for target fps.src/app/main.cpp- apply CLI override and log.PR Type
Enhancement
Description
Implement VK_KHR_present_wait for GPU-assisted frame pacing
Support uncapped FPS (0) and configurable target frame rates
Add CLI override for
--target-fpsparameterFallback to mailbox/FIFO with CPU-side throttling when unavailable
Diagram Walkthrough
flowchart LR A["Config/CLI"] -->|target_fps| B["RenderSettings"] B -->|settings| C["VulkanBackend::create"] C -->|detect support| D["select_physical_device"] D -->|present_wait_supported| E["create_device"] E -->|enable extensions| F["create_swapchain"] F -->|choose mode| G{present_wait ready?} G -->|yes| H["FIFO + present_wait"] G -->|no| I["MAILBOX or FIFO"] H -->|apply_present_wait| J["Frame Pacing"] I -->|throttle_present| JFile Walkthrough
6 files
Add present wait state and frame pacing fieldsImplement present wait detection and frame pacing logicPass RenderSettings struct to VulkanBackendAdd target-fps CLI option with optional valueApply CLI target-fps override to configAllow target_fps value of zero in validation5 files
Document target_fps zero means uncappedDocument frame pacing feature proposalDefine render pipeline present wait requirementsDefine CLI target FPS override requirementsList implementation and validation tasks1 files
Update tests to include zero FPS valueSummary by CodeRabbit
New Features
Bug Fixes / Validation
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.