Open
Conversation
Summary by CodeRabbit发布说明
概览修改了 变更
代码审查估计🎯 3 (Moderate) | ⏱️ ~20 分钟 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/platform/windows/display_device/settings.cpp`:
- Around line 985-994: The current logic ignores failures from
remove_file(filepath) and save_settings(filepath, *persistent_data), which can
cause loss of the correct retry state; update the block around
success/data_updated to check and handle each persistence call's return value:
call remove_file(filepath) and only set persistent_data = nullptr and log
success if remove_file returns true, otherwise log an error and keep
persistent_data so restart can retry; similarly, capture the return of
save_settings(...) when data_updated is true, log an error and avoid discarding
or overwriting the in-memory state on failure, and propagate a failure status
(e.g., return/throw or set an error flag) instead of only using the top-level
success boolean. Ensure you reference and update the existing symbols
remove_file, save_settings, persistent_data, filepath, success, and data_updated
so callers can respond to persistence failures.
- Around line 998-1006: The code currently releases audio_data regardless of
operation success (inside the block guarded by
revert_reason_e::topology_switch), which breaks retries that rely on
apply_config() holding the session; change the release logic so audio_data is
only cleared when the operation succeeded (i.e., require success == true in
addition to reason != revert_reason_e::topology_switch) or move the release into
the success path before return; update the release check around audio_data in
settings.cpp (the block handling revert_reason_e and audio_data) to depend on
success so persistent_data retries retain the audio session.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1985c2e4-ccd6-4ae2-a2fa-3c37e958a990
📒 Files selected for processing (1)
src/platform/windows/display_device/settings.cpp
📜 Review details
⏰ 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: Windows
🧰 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_device/settings.cpp
src/platform/**
⚙️ CodeRabbit configuration file
src/platform/**: 平台抽象层代码（Windows/Linux/macOS）。确保各平台实现一致， 注意 Windows API 调用的错误处理和资源释放。
Files:
src/platform/windows/display_device/settings.cpp
Comment on lines
+985
to
994
| if (success) { | ||
| // 恢复成功，清理持久化数据 | ||
| remove_file(filepath); | ||
| persistent_data = nullptr; | ||
| BOOST_LOG(info) << "显示设备配置已恢复"; | ||
| } | ||
| else { | ||
| if (data_updated) { | ||
| save_settings(filepath, *persistent_data); // 忽略返回值 | ||
| save_settings(filepath, *persistent_data); // 保存部分恢复的状态以便重试 | ||
| } |
There was a problem hiding this comment.
持久化操作的失败不能被静默忽略。
这段新逻辑把“可重试”建立在 persistent_data 的最新快照之上，但成功分支里无论 remove_file(filepath) 是否真的删掉文件都会立刻清掉内存态，失败分支里 save_settings() 的返回值也被忽略。只要任一持久化操作失败，进程重启后就可能按过期 JSON 继续回滚，重复执行已经成功的步骤。建议把删除/保存失败显式纳入结果处理，而不是只保留一个普通的 success/false。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/platform/windows/display_device/settings.cpp` around lines 985 - 994, The
current logic ignores failures from remove_file(filepath) and
save_settings(filepath, *persistent_data), which can cause loss of the correct
retry state; update the block around success/data_updated to check and handle
each persistence call's return value: call remove_file(filepath) and only set
persistent_data = nullptr and log success if remove_file returns true, otherwise
log an error and keep persistent_data so restart can retry; similarly, capture
the return of save_settings(...) when data_updated is true, log an error and
avoid discarding or overwriting the in-memory state on failure, and propagate a
failure status (e.g., return/throw or set an error flag) instead of only using
the top-level success boolean. Ensure you reference and update the existing
symbols remove_file, save_settings, persistent_data, filepath, success, and
data_updated so callers can respond to persistence failures.
Comment on lines
+998
to
+1006
| // 不管成败都释放音频数据（串流已结束） | ||
| if (reason != revert_reason_e::topology_switch) { | ||
| if (audio_data) { | ||
| BOOST_LOG(debug) << "释放捕获的音频接收器"; | ||
| audio_data = nullptr; | ||
| } | ||
| } | ||
|
|
||
| if (success) { | ||
| BOOST_LOG(info) << "显示设备配置已恢复"; | ||
| } | ||
| return success; |
There was a problem hiding this comment.
失败后仍释放 audio_data，会把后续重试需要的音频会话提前拆掉。
apply_config() 里持有 audio_data 的目的，是把音频会话延长到“显示设置已经恢复”为止。现在失败时会保留 persistent_data 供后续重试，但这里仍然在非 topology_switch 场景下释放 audio_data；如果失败点恰好依赖这段保活，下一次重试就失去了原本的兜底。建议至少只在 success == true 时再释放它。
可参考的最小改动
- // 不管成败都释放音频数据（串流已结束）
- if (reason != revert_reason_e::topology_switch) {
+ // 仅在恢复完成后释放音频数据；失败时保留，便于调用方继续重试
+ if (success && reason != revert_reason_e::topology_switch) {
if (audio_data) {
BOOST_LOG(debug) << "释放捕获的音频接收器";
audio_data = nullptr;
}
}As per coding guidelines, src/**/*.{cpp,c,h}: Sunshine 核心 C++ 源码，自托管游戏串流服务器。审查要点：内存安全、 线程安全、RAII 资源管理、安全漏洞。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/platform/windows/display_device/settings.cpp` around lines 998 - 1006,
The code currently releases audio_data regardless of operation success (inside
the block guarded by revert_reason_e::topology_switch), which breaks retries
that rely on apply_config() holding the session; change the release logic so
audio_data is only cleared when the operation succeeded (i.e., require success
== true in addition to reason != revert_reason_e::topology_switch) or move the
release into the success path before return; update the release check around
audio_data in settings.cpp (the block handling revert_reason_e and audio_data)
to depend on success so persistent_data retries retain the audio session.
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.
No description provided.