perf(windows): drop trailing retry backoff and instrument DDA init - #862
Open
qiin2333 wants to merge 2 commits into
Open
perf(windows): drop trailing retry backoff and instrument DDA init#862qiin2333 wants to merge 2 commits into
qiin2333 wants to merge 2 commits into
Conversation
The DDA retry loops sleep 200ms after *every* failed attempt, including the final one, where the loop exits immediately afterward and the delay cannot affect the outcome. Three sites do this: both DuplicateOutput paths in duplication_t::init and the test in test_dxgi_duplication. test_dxgi_duplication is the expensive one. display_base_t::init wraps its output scan in a three-pass retry, and calls the test once per desktop-attached output on every pass, so a system with an attached but non-duplicatable output pays the useless 200ms three times per init. That init runs on every capture reinit, since the reinit path calls refresh_displays() -> platf::display_names() (video.cpp:1745). Only sleep when another attempt follows. Modeled cost for a fully failing scan: 1200ms -> 600ms with one such output, 3600ms -> 1800ms with three. No success path changes. Verified by extracting both loop shapes into a native harness and comparing across all 16 combinations of (attempt-1 result, attempt-2 result, enumeration_only, E_ACCESSDENIED): outcome and attempt count are identical everywhere, the new loop never sleeps more than the old, and sleep counts match exactly on every path that returns success or breaks early on E_ACCESSDENIED. The only divergence is the all-attempts-failed path, which is the intent. Also add phase timing to display_base_t::init, logged once per init as a single info line: total, output_select (with pass and duplication-test counts), device_create, and hdr_probe (with retry count). Logged on success and on the three early-return failures. The motivation is that a3bd879 attributed multi-second reinit to unfair-lock starvation and added short-timeout polling to mitigate it, but this function also contains several seconds of worst-case fixed sleeps that compound inside the three-pass retry: Sleep(500) on the second pass, up to 700ms of HDR metadata backoff, and the per-output delays above. Which of the two actually dominates is not something I can determine by reading, so measure it rather than continue guessing. Two candidates were deliberately left alone pending that measurement: test_dxgi_duplication creates a fresh D3D11 device per output where one per adapter would do, and the HDR metadata backoff is not gated on the display actually being in HDR mode, so an SDR display reporting zeroed luminance would burn the full 700ms. The new hdr_probe/retries fields will show whether the latter fires in practice. Not compile-verified: Windows-only translation unit, changed on macOS. Verified structurally (braces balanced in all three edited functions, short-circuit evaluation order preserved in the refactored condition). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
Walkthrough本次修改调整 Windows DXGI 重复捕获的重试与退避行为，并在显示初始化流程中增加输出选择、设备创建、HDR 探测及失败状态的阶段耗时日志。 ChangesWindows 显示捕获初始化
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/platform/windows/display_base.cpp (1)
579-587: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win补齐所有初始化早退路径的耗时日志。
计时 lambda 只覆盖了部分失败路径；
CreateDXGIFactory1、adapter->QueryInterface和后续device->QueryInterface(IID_IDXGIDevice)失败时仍直接返回。这样会遗漏 PR 目标中“早退失败”的延迟数据。建议补充日志调用
if (FAILED(status)) { BOOST_LOG(error) << "Failed to create DXGIFactory1 ..."; + log_init_timing("factory create failed"); return -1; } if (FAILED(status)) { BOOST_LOG(error) << "Failed to query IDXGIAdapter interface"; + log_init_timing("adapter query failed"); return -1; } if (FAILED(status)) { BOOST_LOG(warning) << "Failed to query DXGI interface ..."; + log_init_timing("dxgi device query failed"); return -1; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/windows/display_base.cpp` around lines 579 - 587, 扩展 Display 初始化流程中的 log_init_timing 使用范围，确保 CreateDXGIFactory1、adapter->QueryInterface 以及 device->QueryInterface(IID_IDXGIDevice) 失败并早退前都调用该 lambda 记录耗时；为每条失败路径传入明确的 outcome，并保持现有错误处理与返回行为不变。
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/platform/windows/display_base.cpp`:
- Around line 579-587: 扩展 Display 初始化流程中的 log_init_timing 使用范围，确保
CreateDXGIFactory1、adapter->QueryInterface 以及
device->QueryInterface(IID_IDXGIDevice) 失败并早退前都调用该 lambda 记录耗时；为每条失败路径传入明确的
outcome，并保持现有错误处理与返回行为不变。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c1b32a3-82c0-4d16-8a00-6b58d9a62f4d
📒 Files selected for processing (1)
src/platform/windows/display_base.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,c,h}
⚙️ CodeRabbit configuration file
src/**/*.{cpp,c,h}: Sunshine 核心 C++ 源码，自托管游戏串流服务器。审查要点：内存安全、 线程安全、RAII 资源管理、安全漏洞。注意预处理宏控制的平台相关代码。
Files:
src/platform/windows/display_base.cpp
src/platform/**
⚙️ CodeRabbit configuration file
src/platform/**: 平台抽象层代码（Windows/Linux/macOS）。确保各平台实现一致， 注意 Windows API 调用的错误处理和资源释放。
Files:
src/platform/windows/display_base.cpp
🔇 Additional comments (3)
src/platform/windows/display_base.cpp (3)
52-55: LGTM!Also applies to: 74-84, 105-115
505-531: LGTM!
632-632: LGTM!Also applies to: 666-672, 706-710, 743-747, 875-875, 904-917
Three failure paths in display_base_t::init returned without calling log_init_timing(), so a reinit that died at factory creation, adapter QueryInterface, or IDXGIDevice QueryInterface produced no timing line at all. Those are exactly the cases where knowing how long the attempt took before failing is useful, since display_base_t::init is retried per capture backend. Add the call to all three with distinct outcome strings. Error handling, log levels, and return values are unchanged; the stamps for phases that were never reached are still initialized to init_start, so they report 0ms rather than garbage. All seven return paths in the function are now covered: factory create failed, no output found, adapter query failed, device create failed, dxgi device query failed, no timer, ok. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reduces DXGI capture reinit latency, and adds the measurement needed to decide what to do next.
The bug
All three DDA retry loops sleep 200 ms after every failed attempt — including the final one, where the loop exits immediately afterward so the delay cannot affect the outcome:
Sites: both
DuplicateOutputpaths induplication_t::init, andtest_dxgi_duplication.test_dxgi_duplicationis the expensive one.display_base_t::initwraps its output scan in a three-pass retry and calls the test once per desktop-attached output on every pass — so an attached but non-duplicatable output pays the useless 200 ms three times per init. And that init runs on every capture reinit, because the reinit path callsrefresh_displays()→platf::display_names()(video.cpp:1745).Modeled cost of a fully failing scan:
Why this is safe
No success path changes. Both loop shapes were extracted into a native harness and compared across all 16 combinations of (attempt-1 result, attempt-2 result,
enumeration_only,E_ACCESSDENIED):E_ACCESSDENIED— including "succeeds on attempt 2", which still sleeps onceThe only divergence is the all-attempts-failed path. That is the intent.
Instrumentation
One info line per init:
Emitted on success and on all three early-return failures.
Why:
a3bd8799attributed multi-second reinit to unfair-lock starvation and added short-timeout polling to mitigate it. Butdisplay_base_t::initalso contains several seconds of worst-case fixed sleeps that compound inside the three-pass retry —Sleep(500)on the second pass, up to 700 ms of HDR metadata backoff, plus the per-output delays above. Which of the two actually dominates is not determinable by reading the code, so this measures it instead of guessing further.Deliberately not done
Both are real but should be driven by the numbers above, not assumed:
test_dxgi_duplicationcreates a fresh D3D11 device per output (display_base.cpp:473) where one per adapter would do. The device is only used for theDuplicateOutputprobe.is_hdr_metadata_validonly guards luminance values, which are consumed solely byget_hdr_metadata()underis_hdr()(G2084). An SDR display reporting zeroed luminance would burn the full 700 ms for nothing. The newhdr_probe/retriesfields will show whether this fires in practice.Testing
Not compile-verified — Windows-only translation unit, changed on macOS with no build tree. Verified structurally: braces balanced in all three edited functions (
duplication_t::init,test_dxgi_duplication,display_base_t::init), and the short-circuit evaluation order is preserved in the one refactored condition (theoutput_acceptedrewrite exists only to count probe calls and keepsis_rdp_session ||/AttachedToDesktop &&ordering intact, so the expensive probe still runs in exactly the same cases).What a reviewer should check on Windows
hdr_proberetries are non-zero on an SDR display. That decides item 2 above.Not in scope
Dirty-rect capture (
GetFrameDirtyRects/GetFrameMoveRects) is still unused — every frame is a full-screenCopyResource. That is a real gap but a design change: dirty regions must accumulate across frames whenlast_frame_variantis reused, and it is coupled to the cursor-blend state machine touched by #859. Should be its own change, driven by measurement.🤖 Generated with Claude Code