Add display scale control endpoints#699
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (1)src/**/*.{cpp,c,h}⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (1)
Summary by CodeRabbit
Walkthrough该 PR 将 nvhttp 的配对、应用、显示控制/缩放、动态参数、会话、剪贴板、ABR 与 AI 等内联实现拆分为独立模块，新增 Windows 显示缩放实现，更新 CMake 构建列表并将所有路由绑定改为调用对应模块函数。 变更模块化基础设施与路由绑定
简单功能模块：应用、剪贴板、会话
显示管理模块与 Windows 缩放支持
动态参数与 ABR API
AI API 与配对模块
预估审核工作量🎯 4 (复杂) | ⏱️ ~60 分钟 相关 PR
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 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.
Inline comments:
In `@src/nvhttp.cpp`:
- Around line 423-433: The code writes the same 404 payload twice (first via
response->write(data.str()) and then again via *response << "HTTP/1.1 404 NOT
FOUND\r\n" << data.str()), causing duplicate/malformed responses; fix by
choosing a single write path—either remove the initial
response->write(data.str()) and build and send the full HTTP status + body via
the response stream operator, or remove the operator<< block and send only the
XML body with response->write while ensuring the HTTP status line is set
correctly; locate usages of response->write(...) and the stream write on
*response to make the change (e.g., the pt::write_xml(data, tree);
response->write(data.str()); and the subsequent *response << "HTTP/1.1 404 NOT
FOUND..." lines) and keep only one consistent output method.
In `@src/nvhttp/abr_api.cpp`:
- Around line 199-205: 当前对 stream::session::change_dynamic_param_for_client(...)
的调用忽略了返回值；当 broadcast context 消失、会话结束或目标非 RUNNING 时该函数会返回 false，导致 API 仍然报告成功和
feedback/newBitrate 与实际会话状态不一致。请在调用
stream::session::change_dynamic_param_for_client(client.name, param)
后立即检查其返回值；若为 false，应将 API 响应设为失败或至少在响应中明确标记“ABR 已请求但码率未下发”，并确保
feedback/newBitrate 不误导（例如不回填新码率或附带 error 字段）；对文件中另一处相同调用（大约在 267-275 附近）做同样的处理。
In `@src/nvhttp/ai_api.cpp`:
- Around line 43-50: The stream branch in processAiChatStream is collapsing all
non-200 upstream responses into a 502 when headerSent is false; instead, when
headerSent is false inspect result.httpCode and reuse the non-stream
response-mapping logic (the same status-to-body mapping used in the non-stream
branch) to write the correct HTTP status and body to *response before calling
response->send(); ensure you reference result.httpCode and headerSent and avoid
unconditionally writing "502 Bad Gateway" — replicate the existing non-stream
mapping rules to preserve 400/403/etc semantics.
In `@src/nvhttp/apps.cpp`:
- Around line 100-103: The file-reading code uses std::ifstream in(app_image,
std::ios::binary) but doesn't verify the file opened successfully, so a missing
or unreadable resource will still go to response->write and return a broken
image; update the block that constructs in and calls response->write to check
in.is_open() (or in.good()) before writing and, on failure, send an appropriate
error response (e.g., SimpleWeb::StatusCode::client_error_not_found or
server_error_internal_server_error) via response->write with a short error
message; refer to the symbols std::ifstream in, response->write and app_image to
locate and implement the check and alternate error path.
In `@src/nvhttp/display_control.cpp`:
- Around line 110-113: The send_response lambda currently defaults to
SimpleWeb::StatusCode::success_ok so failure/unsupported branches still return
HTTP 200; change it to accept no default and require callers to pass the actual
status (or introduce a local variable like response_status set in each branch)
and ensure every path that sets response_json["status_code"] also sets
response_status accordingly before calling send_response; update uses around the
Windows-failure, non-Windows 501, and exception paths (references:
send_response, response->write, response_json["status_code"]) so the HTTP status
passed to response->write matches the JSON status_code.
- Around line 91-99: The catch block sets response_json["status_code"]=500 but
the code always calls response->write(SimpleWeb::StatusCode::success_ok,...), so
change the flow to send the proper HTTP status: introduce or use a status
variable (e.g., int or enum) and set it to
SimpleWeb::StatusCode::client_error_internal_server_error (500) inside the catch
in display_control.cpp (around the catch handling the std::exception) and then
call response->write(status, response_json.dump(), json_headers()) instead of
always using success_ok; ensure response_json remains populated as now and the
successful path sets the status variable to success_ok.
In `@src/nvhttp/display_scale.cpp`:
- Around line 154-160: The code currently falls back to using the GDI name as
device_id when display_info_map.find(name) misses; instead, change the fallback
so that device_id is empty (or a null/absent value) rather than name. Update the
expression that sets device_id (the ternary using display_info_map, which feeds
into json display_node) to produce an empty string or omit the device_id field
when not found so callers of /display-scale-options and /display-scale will not
send a GDI name that get_active_path() in windows_utils.cpp cannot match.
In `@src/nvhttp/dynamic_params.cpp`:
- Around line 78-115: Parsing errors from std::stoi/std::stof in the
ChangeBitrate handler are currently caught by the generic catch and return 500
with e.what(), which misclassifies client input errors and leaks internals;
update the exception handling in src/nvhttp/dynamic_params.cpp (e.g.,
ChangeBitrate and the other dynamic-param handlers in the 151-259 region) to
explicitly catch std::invalid_argument and std::out_of_range, set tree
root.<xmlattr>.status_code to 400 with a generic client-facing message like
"Invalid numeric parameter", and do not return e.what() to clients; keep a
generic std::exception catch for server errors (500) but only log e.what()
internally (BOOST_LOG) and return a non-sensitive message to the client.
- Around line 211-213: The ADAPTIVE_QUANTIZATION branch currently treats any
non-"true"/"1" value as false; update the handling in the switch case for
video::dynamic_param_type_e::ADAPTIVE_QUANTIZATION to validate param_value
explicitly (accept only "true"/"false"/"1"/"0", optionally case-insensitive),
set param.value.bool_value accordingly, and on any other value return an HTTP
400 error (or propagate a bad-request error) so malformed inputs (e.g., "ture",
"yes") are rejected; modify the code around the param_value check where
param.value.bool_value is assigned to implement this validation and error
return.
In `@src/nvhttp/pairing.cpp`:
- Around line 637-639: The code currently takes the first entry from map_id_sess
(std::begin(map_id_sess)->second) which can target the wrong pending pairing
session; update the pin-handling path to look up the correct session by its
unique session identifier (e.g., use map_id_sess.find(uniqueid) or
map_id_sess.at(uniqueid)) and validate the iterator before use, then call
getservercert(foundSess, tree, pin, name) and set foundSess.client.name = name;
also ensure any session map access uses the same synchronization (mutex/lock)
used elsewhere to avoid races when multiple pending pairings exist.
- Around line 357-364: The catch block that handles failures reading
config::nvhttp.file_state must not just return because that leaves
http::unique_id and in-memory authorized client state stale; instead, on
exception generate and assign a fresh http::unique_id (use the existing helper
or create a new random/UUID) and clear or reset the in-memory authorized client
container (e.g., the authorized_clients / auth state structure used in
pairing.cpp), then log a clear warning including the error and the fact a new
id/cleared state was created; alternatively, if policy requires, abort startup
explicitly—update the catch block around pt::read_json to perform one of these
actions rather than a silent return.
- Around line 229-243: The rate-limit check is currently called too early
(before the pairing handshake steps getservercert, clientchallenge,
serverchallengeresp, clientpairingsecret) so check_and_record() increments on
each intermediate request; change the logic to only call
pair_rate_limit.check_and_record(client_ip) when a new pairing session is being
created or when a full pairing attempt fails (e.g., after verifying
clientpairingsecret/handshake result), leaving pair_rate_limit.cleanup() where
appropriate; ensure any 429 response is produced only on those creation/failure
points and not during intermediate handshake handlers so legitimate multi-step
pairings aren't counted as multiple attempts.
In `@src/nvhttp/sessions.cpp`:
- Around line 39-47: The HTTP status is only embedded in the JSON body; update
the response object itself to set the proper HTTP status before writing the body
— e.g., call the response's status setter to set 403 for the "Access denied"
branch (where response_json["status_code"]=403) and set 500 in the error branch
(the other occurrence around lines 90-95), then write response_json.dump() and
close_connection_after_response as before; locate and modify the code that uses
response->write, response_json and response->close_connection_after_response to
add response->set_status_code(...) (or the project's equivalent status setter)
with the matching numeric code.
In `@src/nvhttp/url_utils.cpp`:
- Around line 13-20: The percent-decode loop in url_utils.cpp currently uses
std::istringstream to parse two chars after '%' and accepts partial hex parses
(e.g., "%2G"), which yields wrong bytes and still advances i by 2; modify the
logic in the function handling 'value' (the percent-decoding loop) to first
validate that both characters value[i+1] and value[i+2] are hex digits (or parse
and assert the stream consumed exactly two hex chars) before converting and
appending static_cast<char>(hex_val), and only advance i by 2 when the two
characters are valid hex; otherwise treat '%' as a literal and do not consume
the following chars.
🪄 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: 08202686-0db8-4edc-9dd0-9f3720aea1f7
📒 Files selected for processing (24)
cmake/compile_definitions/common.cmakesrc/nvhttp.cppsrc/nvhttp/abr_api.cppsrc/nvhttp/abr_api.hsrc/nvhttp/ai_api.cppsrc/nvhttp/ai_api.hsrc/nvhttp/apps.cppsrc/nvhttp/apps.hsrc/nvhttp/clipboard_api.cppsrc/nvhttp/clipboard_api.hsrc/nvhttp/display_control.cppsrc/nvhttp/display_control.hsrc/nvhttp/display_scale.cppsrc/nvhttp/display_scale.hsrc/nvhttp/dynamic_params.cppsrc/nvhttp/dynamic_params.hsrc/nvhttp/pairing.cppsrc/nvhttp/pairing.hsrc/nvhttp/sessions.cppsrc/nvhttp/sessions.hsrc/nvhttp/url_utils.cppsrc/nvhttp/url_utils.hsrc/platform/windows/display_device/windows_utils.cppsrc/platform/windows/display_device/windows_utils.h
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,c,h}
⚙️ CodeRabbit configuration file
src/**/*.{cpp,c,h}: Sunshine 核心 C++ 源码，自托管游戏串流服务器。审查要点：内存安全、 线程安全、RAII 资源管理、安全漏洞。注意预处理宏控制的平台相关代码。
Files:
src/nvhttp/display_control.hsrc/nvhttp/ai_api.hsrc/nvhttp/url_utils.hsrc/nvhttp/sessions.hsrc/nvhttp/dynamic_params.hsrc/nvhttp/clipboard_api.cppsrc/nvhttp/abr_api.hsrc/nvhttp/apps.hsrc/nvhttp/display_scale.hsrc/nvhttp/url_utils.cppsrc/nvhttp/clipboard_api.hsrc/nvhttp/pairing.hsrc/nvhttp/sessions.cppsrc/nvhttp/ai_api.cppsrc/platform/windows/display_device/windows_utils.hsrc/nvhttp/dynamic_params.cppsrc/nvhttp/apps.cppsrc/nvhttp/abr_api.cppsrc/nvhttp/display_control.cppsrc/nvhttp/display_scale.cppsrc/platform/windows/display_device/windows_utils.cppsrc/nvhttp.cppsrc/nvhttp/pairing.cpp
cmake/**
⚙️ CodeRabbit configuration file
cmake/**: CMake 构建系统文件。审查跨平台兼容性、现代 CMake 实践。
Files:
cmake/compile_definitions/common.cmake
src/platform/**
⚙️ CodeRabbit configuration file
src/platform/**: 平台抽象层代码（Windows/Linux/macOS）。确保各平台实现一致， 注意 Windows API 调用的错误处理和资源释放。
Files:
src/platform/windows/display_device/windows_utils.hsrc/platform/windows/display_device/windows_utils.cpp
🪛 Clang (14.0.6)
src/nvhttp/display_control.h
[error] 3-3: 'memory' file not found
(clang-diagnostic-error)
src/nvhttp/ai_api.h
[error] 3-3: 'memory' file not found
(clang-diagnostic-error)
src/nvhttp/url_utils.h
[error] 3-3: 'string' file not found
(clang-diagnostic-error)
src/nvhttp/sessions.h
[error] 3-3: 'memory' file not found
(clang-diagnostic-error)
src/nvhttp/dynamic_params.h
[error] 3-3: 'memory' file not found
(clang-diagnostic-error)
src/nvhttp/abr_api.h
[error] 3-3: 'memory' file not found
(clang-diagnostic-error)
src/nvhttp/apps.h
[error] 3-3: 'memory' file not found
(clang-diagnostic-error)
src/nvhttp/display_scale.h
[error] 3-3: 'memory' file not found
(clang-diagnostic-error)
src/nvhttp/clipboard_api.h
[error] 3-3: 'memory' file not found
(clang-diagnostic-error)
src/nvhttp/pairing.h
[error] 3-3: 'memory' file not found
(clang-diagnostic-error)
🔇 Additional comments (8)
src/nvhttp/apps.cpp (1)
52-90: LGTM!Also applies to: 111-128
src/nvhttp/apps.h (1)
1-21: LGTM!src/nvhttp/clipboard_api.cpp (1)
7-17: LGTM!src/nvhttp/clipboard_api.h (1)
1-18: LGTM!src/nvhttp/sessions.h (1)
1-15: LGTM!src/nvhttp/ai_api.h (1)
7-13: LGTM!src/nvhttp/dynamic_params.h (1)
1-18: LGTM!src/nvhttp/abr_api.h (1)
1-21: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/nvhttp/pairing.cpp (1)
123-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick win单个
pending_pin_unique_id仍然会把更早的待输 PIN 会话“丢”掉。这里还是只有一个全局槽位。只要第一个
getservercert还在等 PIN，第二个请求就会覆盖它；旧会话仍留在map_id_sess里并持有未完成响应，但pin()已经再也找不到它。最小修复是：发现已有待输 PIN 会话时直接拒绝新的待配对请求，而不是覆盖全局指针。As per coding guidelines,
src/**/*.{cpp,c,h}的审查要点包括“线程安全”与“内存安全”。Also applies to: 296-297, 446-448
🤖 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/nvhttp/pairing.cpp` at line 123, The code uses a single global pending_pin_unique_id which allows a new getservercert call to overwrite an existing pending PIN session and orphan the earlier session in map_id_sess; change the logic so that when pending_pin_unique_id is already set you reject new pairing requests (return an error) instead of overwriting it. Concretely: in the getservercert path that currently assigns static std::string pending_pin_unique_id, add a check for an existing pending_pin_unique_id and fail the new request early; protect accesses/updates to pending_pin_unique_id and map_id_sess with the same mutex (or convert pending_pin_unique_id to a guarded member) to ensure thread-safety; ensure pin() still looks up the session in map_id_sess and that pending_pin_unique_id is cleared under the lock when the session completes or is cancelled so no stale state remains.
🤖 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.
Inline comments:
In `@src/nvhttp/pairing.cpp`:
- Around line 358-364: catch 块目前只在内存中重置了 http::unique_id、cert_chain 和
client_root，但没有修复磁盘上损坏的状态文件 (config::nvhttp.file_state)，导致 save_state()
后续不会持久化新状态。修改为在清理内存状态后也删除或截断磁盘上的损坏文件（使用 std::filesystem::remove 或打开并截断文件），并在持有
client_state_mutex 的情况下调用 save_state()（或显式写回干净状态）以将新的 http::unique_id
和空客户端列表持久化；参考标识符：save_state(), config::nvhttp.file_state, http::unique_id,
cert_chain, client_root, client_state_mutex。
- Around line 646-648: pin() currently takes auto &sess = sess_it->second then
calls getservercert(sess, tree, pin, name); if getservercert() triggers
fail_pair()/remove_session() which erases the session from map_id_sess, sess and
sess_it become dangling and subsequent accesses (e.g., sess.client.name,
sess.async_insert_pin.response) are use-after-free; modify pin() so it does not
dereference sess after getservercert() unless the session is still valid —
either have getservercert() return a success/failure status (or set
tree.status_code) that pin() checks and returns early on failure, or re-lookup
the session in map_id_sess (verify sess_it still valid) before touching sess
members; ensure references to getservercert, pin, sess, sess_it, fail_pair,
remove_session, map_id_sess.erase, sess.client.name, and
sess.async_insert_pin.response are updated accordingly.
---
Duplicate comments:
In `@src/nvhttp/pairing.cpp`:
- Line 123: The code uses a single global pending_pin_unique_id which allows a
new getservercert call to overwrite an existing pending PIN session and orphan
the earlier session in map_id_sess; change the logic so that when
pending_pin_unique_id is already set you reject new pairing requests (return an
error) instead of overwriting it. Concretely: in the getservercert path that
currently assigns static std::string pending_pin_unique_id, add a check for an
existing pending_pin_unique_id and fail the new request early; protect
accesses/updates to pending_pin_unique_id and map_id_sess with the same mutex
(or convert pending_pin_unique_id to a guarded member) to ensure thread-safety;
ensure pin() still looks up the session in map_id_sess and that
pending_pin_unique_id is cleared under the lock when the session completes or is
cancelled so no stale state remains.
🪄 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: afdc92f2-5ab4-44a3-b2d2-d0670ed2e172
📒 Files selected for processing (10)
src/nvhttp.cppsrc/nvhttp/abr_api.cppsrc/nvhttp/ai_api.cppsrc/nvhttp/apps.cppsrc/nvhttp/display_control.cppsrc/nvhttp/display_scale.cppsrc/nvhttp/dynamic_params.cppsrc/nvhttp/pairing.cppsrc/nvhttp/sessions.cppsrc/nvhttp/url_utils.cpp
🚧 Files skipped from review as they are similar to previous changes (9)
- src/nvhttp/url_utils.cpp
- src/nvhttp/ai_api.cpp
- src/nvhttp/sessions.cpp
- src/nvhttp/apps.cpp
- src/nvhttp/display_control.cpp
- src/nvhttp/dynamic_params.cpp
- src/nvhttp/abr_api.cpp
- src/nvhttp/display_scale.cpp
- src/nvhttp.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 (1)
src/**/*.{cpp,c,h}
⚙️ CodeRabbit configuration file
src/**/*.{cpp,c,h}: Sunshine 核心 C++ 源码，自托管游戏串流服务器。审查要点：内存安全、 线程安全、RAII 资源管理、安全漏洞。注意预处理宏控制的平台相关代码。
Files:
src/nvhttp/pairing.cpp
🔇 Additional comments (1)
src/nvhttp/pairing.cpp (1)
256-262: LGTM!Also applies to: 266-266, 626-630
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.
改了啥喵
GET /displays，返回显示器缩放相关字段：当前缩放、推荐缩放、支持的缩放列表、是否支持设置、是否主显示器。GET /display-scale-options，用于按display_name/device_id查询指定显示器缩放能力。POST /display-scale，用于按显示器设置 Windows 显示缩放，并返回旧值、新值、是否立即生效和错误码。windows_utils，HTTP 层只负责参数解析和 JSON 响应，避免小端点长成杂鱼面条。为啥要改
Moonlight V+ 的 PcView 场景预设需要覆盖主机端 Windows 显示缩放。这个状态属于 Windows 主机显示配置，客户端自己不能可靠修改；模拟 Windows 设置界面也会受语言、焦点和系统版本影响。服务端暴露能力后，客户端可以保存场景 DPI 字段并安全调用。
验证
git diff --checksrc/platform/windows/display_device/windows_utils.cpp和src/nvhttp.cpp做了g++ -fsyntax-only语法检查完整
cmake --build build --target sunshine在当前本机环境仍受既有 MSYS/submodule 状态影响，之前卡在packaging/linux/flatpak/deps/flatpak-builder-tools的 git status 检查；这次 PR 分支本身是从最新origin/master干净 worktree 创建的。