Repository navigation
Conversation
|
TBH, not too happy with |
The EnumAdapters1()/EnumOutputs() loops only stopped on DXGI_ERROR_NOT_FOUND and used the returned pointer after any other error, which crashed Sunshine and dxgi-info in sessions without a desktop. Log the error and stop instead.
f9a5d39 to
fd55804
Compare
|
ReenigneArcher
left a comment
ReenigneArcher
left a comment
There was a problem hiding this comment.
Reviewed current head fd55804b873b2f1244d97f15d624324d55819a9f against master at ee31a6aa252153df7c9f693ca62e6167cf39ca3f. The original unchecked-error path is still present on the target branch, so this fix remains relevant. The success guards address invalid-result use, but please resolve the partial display-list regression and add the required automated regression coverage before merging.
Validation: both changed C++ files passed UCRT64 GCC 16.2 syntax checks using the existing workspace dependency headers; the complete diff passed git diff --check. An extracted-function harness reproduced three partial-list failures on this head, and all seven scenarios passed with the suggested early returns. Full Sunshine tests and the SSH/session-without-desktop scenario were not run locally.
Verification limits: CI, CodeQL, common lint, and GH-Pages workflows await maintainer approval. The failed GH-Pages Read the Docs build 35722539 timed out waiting for check runs; its log does not show a change-related compilation failure. SonarCloud and the Sunshine documentation status passed. GitHub currently reports the PR mergeable.
Review completed by Codex | Model: gpt-6.1-sol | Thinking: xhigh
| if (FAILED(status) && status != DXGI_ERROR_NOT_FOUND) { | ||
| BOOST_LOG(error) << "Failed to enumerate DXGI outputs [0x"sv << util::hex(status).to_string_view() << ']'; | ||
| } | ||
| } | ||
| if (FAILED(status) && status != DXGI_ERROR_NOT_FOUND) { | ||
| BOOST_LOG(error) << "Failed to enumerate DXGI adapters [0x"sv << util::hex(status).to_string_view() << ']'; | ||
| } |
There was a problem hiding this comment.
[P2] Discard the display list when enumeration fails
If one display has already been appended and a later EnumOutputs() or EnumAdapters1() returns an unexpected failure, these handlers only log it and display_names() returns the earlier displays as a successful, nonempty result. For example, PRIMARY succeeds and enumeration fails before the active SECONDARY display. video::refresh_displays() then replaces its cached list with [PRIMARY], cannot find SECONDARY, and resets the capture selection to PRIMARY. Returning an empty list instead lets that caller retain its previous list and active display. This also preserves the all-or-nothing requirement documented at the start of this function.
Return immediately from both unexpected-error handlers:
| if (FAILED(status) && status != DXGI_ERROR_NOT_FOUND) { | |
| BOOST_LOG(error) << "Failed to enumerate DXGI outputs [0x"sv << util::hex(status).to_string_view() << ']'; | |
| } | |
| } | |
| if (FAILED(status) && status != DXGI_ERROR_NOT_FOUND) { | |
| BOOST_LOG(error) << "Failed to enumerate DXGI adapters [0x"sv << util::hex(status).to_string_view() << ']'; | |
| } | |
| if (FAILED(status) && status != DXGI_ERROR_NOT_FOUND) { | |
| BOOST_LOG(error) << "Failed to enumerate DXGI outputs [0x"sv << util::hex(status).to_string_view() << ']'; | |
| return {}; | |
| } | |
| } | |
| if (FAILED(status) && status != DXGI_ERROR_NOT_FOUND) { | |
| BOOST_LOG(error) << "Failed to enumerate DXGI adapters [0x"sv << util::hex(status).to_string_view() << ']'; | |
| return {}; | |
| } |
I reproduced this using the exact function from this head with fake DXGI objects: failure after an output, failure after an adapter, and output failure on a later adapter all returned [PRIMARY]. With these two returns, all seven harness scenarios passed. This was an extracted-function harness, not a hardware or full Sunshine test run.
Review completed by Codex | Model: gpt-6.1-sol | Thinking: xhigh
| for (int tries = 0; tries < 2; ++tries) { | ||
| for (int x = 0; factory->EnumAdapters1(x, &adapter_p) != DXGI_ERROR_NOT_FOUND; ++x) { | ||
| int x = 0; | ||
| while (SUCCEEDED(status = factory->EnumAdapters1(x++, &adapter_p))) { |
There was a problem hiding this comment.
[P2] Add regression coverage for the failed-enumeration paths
The PR changes six production enumeration loops but adds no automated tests. The manual SSH check does not exercise a failure after a successful result, which is the partial-list defect above. Regression tests are required for changed source behavior.
Add tests/unit/platform/windows/test_dxgi_enumeration.cpp, using ../../../tests_common.h in the test include group and the existing GoogleTest framework. Introduce an injectable factory/adapter seam or a shared production helper so these tests exercise the actual enumeration code without requiring SSH or a particular GPU. There is currently no enumeration injection seam, so a test that merely copies the loops would not protect the fix.
Concrete cases for DxgiEnumerationTest:
- Make the first adapter call fail with
E_FAIL, and the first output call fail withDXGI_ERROR_NOT_CURRENTLY_AVAILABLE; assert no descriptor/pointer access occurs after failure and the call terminates. - Return a capturable PRIMARY first, then fail adapter enumeration, output enumeration, or output enumeration on a later adapter; assert the result is empty.
- Return PRIMARY and SECONDARY followed by
DXGI_ERROR_NOT_FOUND; assert both names remain and normal exhaustion produces no error log. - Exercise the initialization and diagnostic-tool paths through the same seam/helper, checking that unexpected failures stop enumeration and report the appropriate error.
Document the failure contract in Doxygen alongside the changed methods/helper. A focused test command after building the test target is:
C:\msys64\msys2_shell.cmd -defterm -here -no-start -ucrt64 -c './cmake-build-release/tests/test_sunshine.exe --gtest_filter=DxgiEnumerationTest.*'These are proposed repository tests; they have not been added or run. The separate review harness covered the seven display-list scenarios described above.
Review completed by Codex | Model: gpt-6.1-sol | Thinking: xhigh
Description
The EnumAdapters1()/EnumOutputs() loops only stopped on DXGI_ERROR_NOT_FOUND and used the returned pointer after any other error, which crashed Sunshine and dxgi-info in sessions without a desktop. Log the error and stop instead.
I caught this when I tried to run Sunshine via SSH.
Screenshot
Issues Fixed or Closed
Roadmap Issues
Testing
It no longer crashes.
Type of Change
Checklist
AI Usage
See our AI usage policy.
Debugged and found all places to patch with Claude Opus 5.5.