Add some developer debug around missed present deadlines - #245
TheElixZammuto merged 3 commits into
Conversation
📝 WalkthroughWalkthroughState and streaming code now track whether frame-present deadlines are met. Pacer.waitBeforePresent returns a boolean indicating whether a wait occurred, callers propagate that as hitDeadline into SubmitRenderStats, VIDEO_STATS gains hitDeadlines/missedDeadlines counters which are aggregated and surfaced in debug output. Changes
Sequence Diagram(s)sequenceDiagram
participant Pacer
participant MainLoop as App/Main
participant Stats
participant DeviceRes as DeviceResources
App/Main->>Pacer: waitBeforePresent(deadline)
Pacer-->>App/Main: bool waited (hitDeadline)
App/Main->>DeviceRes: Present()
App/Main->>Stats: SubmitRenderStats(preWaitUs, renderUs, presentUs, hitDeadline)
Stats->>Stats: increment hitDeadlines/missedDeadlines and aggregate VIDEO_STATS
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-12-14T22:36:59.945ZApplied to files:
📚 Learning: 2026-01-10T18:24:38.508ZApplied to files:
📚 Learning: 2025-12-14T22:37:06.337ZApplied to files:
🧬 Code graph analysis (1)Streaming/moonlight_xbox_dxMain.cpp (1)
🔇 Additional comments (4)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @State/Stats.cpp:
- Line 399: The missed-present-rate calculation uses `stats.hitDeadlines ? ... :
0.0f` which returns 0 when there are only missed deadlines; change the
conditional to check the total (`stats.missedDeadlines + stats.hitDeadlines`)
and compute `((double)stats.missedDeadlines / (stats.missedDeadlines +
stats.hitDeadlines)) * 100` when the total is non-zero, otherwise return 0.0f,
ensuring the numerator/denominator use a floating-point cast to avoid integer
division.
In @Streaming/moonlight_xbox_dxMain.cpp:
- Around line 111-115: Remove the unused local variables hits and misses
declared alongside ewmaRenderMs; they are vestigial and not referenced anywhere
(deadline tracking is done via Stats::SubmitRenderStats). Edit
moonlight_xbox_dxMain.cpp to delete the declarations "int hits = 0, misses = 0"
(or remove hits and misses from that variable declaration) so only ewmaRenderMs
remains, and run a quick build to ensure no remaining references to hits/misses
exist.
🧹 Nitpick comments (1)
Streaming/FFmpegDecoder.cpp (1)
294-303: Consider removing the commented-out workaround code.The commented-out Xbox One tearing workaround has been replaced by deadline-based timing. While the comments provide historical context, keeping dead code can clutter the codebase. Consider either:
- Removing the block entirely and documenting the rationale in the commit message or a code comment
- If this is intentionally kept for potential rollback during testing, add a TODO comment with a tracking issue
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
State/Stats.cppState/Stats.hStreaming/FFmpegDecoder.cppStreaming/FFmpegDecoder.hStreaming/Pacer.cppStreaming/Pacer.hStreaming/moonlight_xbox_dxMain.cpp
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: andygrundman
Repo: TheElixZammuto/moonlight-xbox PR: 239
File: Streaming/moonlight_xbox_dxMain.cpp:150-159
Timestamp: 2025-12-14T22:37:06.337Z
Learning: In the moonlight-xbox streaming application, the first decoded frame's PTS (presentation timestamp) will never be 0, so initializing lastFramePts to 0 is safe for detecting frame changes in the render loop.
📚 Learning: 2025-12-14T22:37:06.337Z
Learnt from: andygrundman
Repo: TheElixZammuto/moonlight-xbox PR: 239
File: Streaming/moonlight_xbox_dxMain.cpp:150-159
Timestamp: 2025-12-14T22:37:06.337Z
Learning: In the moonlight-xbox streaming application, the first decoded frame's PTS (presentation timestamp) will never be 0, so initializing lastFramePts to 0 is safe for detecting frame changes in the render loop.
Applied to files:
Streaming/Pacer.hStreaming/FFmpegDecoder.cpp
📚 Learning: 2025-12-14T22:36:59.945Z
Learnt from: andygrundman
Repo: TheElixZammuto/moonlight-xbox PR: 239
File: Streaming/moonlight_xbox_dxMain.cpp:150-159
Timestamp: 2025-12-14T22:36:59.945Z
Learning: In Streaming/moonlight_xbox_dxMain.cpp, initialize lastFramePts to 0 because the first decoded frame's PTS is never 0, enabling safe detection of frame changes in the render loop. Include a clarifying comment about this assumption. If this assumption could change in the future, add a one-time initialization using the first non-zero PTS to guard against regressions.
Applied to files:
Streaming/moonlight_xbox_dxMain.cpp
📚 Learning: 2026-01-08T15:15:34.393Z
Learnt from: mpotrykus
Repo: TheElixZammuto/moonlight-xbox PR: 243
File: moonlight-xbox-dx.vcxproj:661-663
Timestamp: 2026-01-08T15:15:34.393Z
Learning: In the moonlight-xbox project (moonlight-xbox-dx.vcxproj), the ProjectReference to third_party\DirectXTK\DirectXTK_Windows10_2022.vcxproj is intentionally added before the file exists. The setup-dev.ps1 script generates this project file during setup, and the reference ensures the correct build order (DirectXTK before main project) once everything is generated, avoiding manual build requirements.
Applied to files:
Streaming/moonlight_xbox_dxMain.cpp
🧬 Code graph analysis (3)
State/Stats.h (1)
State/Stats.cpp (2)
SubmitRenderStats(136-156)SubmitRenderStats(136-136)
Streaming/Pacer.h (1)
Streaming/Pacer.cpp (2)
waitBeforePresent(330-347)waitBeforePresent(330-330)
Streaming/moonlight_xbox_dxMain.cpp (2)
Streaming/Pacer.cpp (2)
instance(40-43)instance(40-40)pch.h (1)
QpcToUs(68-77)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (11)
Streaming/FFmpegDecoder.h (1)
39-39: LGTM!The addition of the
fpsfield follows the existing pattern for other stream configuration values (videoFormat,width,height). This enables deadline-based timing calculations elsewhere in the codebase.Streaming/Pacer.h (1)
25-25: LGTM!The signature change from
voidtoboolenables callers to determine whether the wait succeeded, which is essential for the deadline tracking feature being introduced.Streaming/Pacer.cpp (1)
329-347: LGTM!The implementation correctly returns
truewhen a wait occurred (target was in the future) andfalsewhen no wait happened (either not running or already past the deadline). The comment accurately describes the semantics.State/Stats.h (2)
29-30: LGTM!The new counter fields are appropriately typed as
uint32_tand placed logically near other frame-related statistics.
66-66: LGTM!The signature update to include the
hitDeadlineparameter aligns with the implementation and enables deadline tracking throughout the stats pipeline.Streaming/FFmpegDecoder.cpp (2)
89-91: LGTM!The initialization of
fpsfrom the configuration is correctly placed alongside the Pacer initialization which usesconfig->fps.
98-98: LGTM!Setting a default of 60 fps is a reasonable fallback until
CompleteInitializationsets the actual value from config.Streaming/moonlight_xbox_dxMain.cpp (2)
142-142: LGTM!Correctly captures the boolean return from
waitBeforePresentto track whether the deadline was met.
174-178: LGTM!The
hitDeadlineflag is properly passed toSubmitRenderStats, enabling centralized deadline tracking in the Stats class.State/Stats.cpp (2)
136-156: LGTM!The deadline tracking logic correctly increments the appropriate counter and includes debug logging for missed deadlines to aid in diagnosing timing issues.
167-168: LGTM!Correctly propagates the deadline counters during statistics window aggregation.
| "------\n" | ||
| "Missed present rate: %.2f%%\n" | ||
| "PreWait/Render: %.2f/%.2f ms\n", | ||
| stats.hitDeadlines ? ((double)stats.missedDeadlines / (stats.missedDeadlines + stats.hitDeadlines)) * 100 : 0.0f, |
There was a problem hiding this comment.
Logic issue in missed present rate calculation.
The condition stats.hitDeadlines ? ... : 0.0f will incorrectly show 0% if there are only missed deadlines (hitDeadlines=0, missedDeadlines>0). The check should verify the total is non-zero instead.
🐛 Proposed fix
- stats.hitDeadlines ? ((double)stats.missedDeadlines / (stats.missedDeadlines + stats.hitDeadlines)) * 100 : 0.0f,
+ (stats.hitDeadlines + stats.missedDeadlines) ? ((double)stats.missedDeadlines / (stats.missedDeadlines + stats.hitDeadlines)) * 100 : 0.0f,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| stats.hitDeadlines ? ((double)stats.missedDeadlines / (stats.missedDeadlines + stats.hitDeadlines)) * 100 : 0.0f, | |
| (stats.hitDeadlines + stats.missedDeadlines) ? ((double)stats.missedDeadlines / (stats.missedDeadlines + stats.hitDeadlines)) * 100 : 0.0f, |
🤖 Prompt for AI Agents
In @State/Stats.cpp at line 399, The missed-present-rate calculation uses
`stats.hitDeadlines ? ... : 0.0f` which returns 0 when there are only missed
deadlines; change the conditional to check the total (`stats.missedDeadlines +
stats.hitDeadlines`) and compute `((double)stats.missedDeadlines /
(stats.missedDeadlines + stats.hitDeadlines)) * 100` when the total is non-zero,
otherwise return 0.0f, ensuring the numerator/denominator use a floating-point
cast to avoid integer division.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @Streaming/moonlight_xbox_dxMain.cpp:
- Line 115: Remove the unused local variables `hits` and `misses` declared in
the render loop (the `int hits = 0, misses = 0;` line) — they are redundant
because deadline tracking is already done by the `Stats` class (`hitDeadlines` /
`missedDeadlines`) when `SubmitRenderStats` is called; simply delete that
declaration from moonlight_xbox_dxMain.cpp and ensure there are no remaining
references to `hits` or `misses` elsewhere in the render loop.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Common/DeviceResources.cppStreaming/Pacer.cppStreaming/moonlight_xbox_dxMain.cpp
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: andygrundman
Repo: TheElixZammuto/moonlight-xbox PR: 239
File: Streaming/moonlight_xbox_dxMain.cpp:150-159
Timestamp: 2025-12-14T22:37:06.337Z
Learning: In the moonlight-xbox streaming application, the first decoded frame's PTS (presentation timestamp) will never be 0, so initializing lastFramePts to 0 is safe for detecting frame changes in the render loop.
📚 Learning: 2025-12-14T22:36:59.945Z
Learnt from: andygrundman
Repo: TheElixZammuto/moonlight-xbox PR: 239
File: Streaming/moonlight_xbox_dxMain.cpp:150-159
Timestamp: 2025-12-14T22:36:59.945Z
Learning: In Streaming/moonlight_xbox_dxMain.cpp, initialize lastFramePts to 0 because the first decoded frame's PTS is never 0, enabling safe detection of frame changes in the render loop. Include a clarifying comment about this assumption. If this assumption could change in the future, add a one-time initialization using the first non-zero PTS to guard against regressions.
Applied to files:
Streaming/moonlight_xbox_dxMain.cpp
📚 Learning: 2026-01-10T18:24:38.508Z
Learnt from: mpotrykus
Repo: TheElixZammuto/moonlight-xbox PR: 246
File: Pages/StreamPage.xaml.cpp:77-91
Timestamp: 2026-01-10T18:24:38.508Z
Learning: In the moonlight-xbox project's StreamPage::Page_Loaded, the inner DISPATCH_UI macro (lines 83-91 in StreamPage.xaml.cpp) is necessary to ensure the loading screen displays correctly. Even though the outer RunAsync executes on the UI thread, wrapping the heavy initialization (CreateDeviceDependentResources, CreateWindowSizeDependentResources, StartRenderLoop) in a separate DISPATCH_UI breaks the work into two message queue items, allowing the UI to update and show the loading screen between the moonlight_xbox_dxMain construction and the resource initialization.
Applied to files:
Streaming/moonlight_xbox_dxMain.cpp
📚 Learning: 2025-12-14T22:37:06.337Z
Learnt from: andygrundman
Repo: TheElixZammuto/moonlight-xbox PR: 239
File: Streaming/moonlight_xbox_dxMain.cpp:150-159
Timestamp: 2025-12-14T22:37:06.337Z
Learning: In the moonlight-xbox streaming application, the first decoded frame's PTS (presentation timestamp) will never be 0, so initializing lastFramePts to 0 is safe for detecting frame changes in the render loop.
Applied to files:
Streaming/Pacer.cpp
🧬 Code graph analysis (1)
Common/DeviceResources.cpp (2)
Utils.cpp (2)
Logf(90-99)Logf(90-90)Utils.hpp (1)
Logf(15-15)
🔇 Additional comments (4)
Common/DeviceResources.cpp (1)
377-379: LGTM! Helpful debug logging for swap chain configuration.The logging statement is well-placed and captures useful information about the SwapChainPanel's logical size and composition scales right before resource creation. This aligns with the PR's debugging objectives and follows the existing logging pattern in this file (similar to lines 223-224 and 289-290). The format specifiers correctly match the data types, and the performance impact is negligible since this isn't a hot path.
Streaming/moonlight_xbox_dxMain.cpp (1)
142-142: LGTM! Deadline tracking integration looks correct.The implementation properly captures the deadline hit status from
waitBeforePresentand propagates it toSubmitRenderStats, enabling the tracking of present deadline performance as described in the PR objectives.Also applies to: 174-178
Streaming/Pacer.cpp (2)
89-91: LGTM! Useful debug logging added.The initialization logging provides valuable context about frame pacing configuration, which aligns well with the PR's goal of adding developer debug information around frame timing.
332-350: LGTM! Return value semantics are correct.The boolean return logic correctly indicates whether the deadline was met:
- Returns
truewhen there's time to wait before the deadline (hit)- Returns
falsewhen the deadline has already passed (missed)This aligns properly with the caller's expectations in
moonlight_xbox_dxMain.cppwhere the result is captured ashitDeadline.
4c4719e to
296ba54
Compare
2 participants
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.