Client-driven dynamic resolution: gate, advertise, and safely apply 0x5506 RESOLUTION requests#729
Open
AsafMah wants to merge 5 commits into
Open
Client-driven dynamic resolution: gate, advertise, and safely apply 0x5506 RESOLUTION requests#729AsafMah wants to merge 5 commits into
AsafMah wants to merge 5 commits into
Conversation
Implements the server-side half of AlkaidLab#728. Changes: - config: add video.allow_client_resolution_change (bool, default false). Distinct from dynamic_resolution_follow_display (host->client follow); this gates the reverse direction (client->host 0x5506 RESOLUTION). - stream: gate the IDX_DYNAMIC_PARAM_CHANGE / RESOLUTION branch on allow_client_resolution_change; log+ignore when disabled. - stream: clamp requested dims to even values (H.264/HEVC require even dimensions; done after range validation, before any other checks). - stream: add per-session leading-edge rate-limit (250 ms window) via session_t::client_res_last_apply to drop rapid duplicate requests. Clients (moonlight-qt, vplus) already debounce ~400 ms before sending one settled packet, so the first request always applies immediately. - stream: note that dd_config_revert_on_disconnect restores display mode on disconnect — no extra teardown needed here. - nvhttp: advertise <ClientResolutionChange> in /serverinfo response so clients can gate outbound 0x5506 RESOLUTION packets on host capability. The host->client follow path (dynamic_resolution_follow_display / send_resolution_change / 0x5507) is entirely unchanged. Ref: AlkaidLab#728
AlkaidLab#9 AlkaidLab#16 for client-driven resolution AlkaidLab#16 (native-int parser): Replace all reinterpret_cast<const int*> reads in the IDX_DYNAMIC_PARAM_CHANGE handler with util::endian::little() reads to guarantee correct little-endian decoding on any host regardless of native byte order. Float (FPS) is bit-cast through a LE u32 via memcpy. AlkaidLab#3 (display failures logged as success): Check the configure_result_t returned by configure_display(). On hard failure: log result code + message + hint, keep old session->config.monitor, skip IDR raise so the encoder stays consistent, and do not update any dedup timestamp (future retries are allowed). On success or deferred_retry: proceed as before. AlkaidLab#4 (capability can advertise no-op paths): Client-driven resolution requests are independent of the host's passive-follow toggle (dynamic_resolution_follow_display) and of the global video.resolution_change option. Fix: build a local config::video_t copy with resolution_change forced to automatic before passing it to configure_display, so the requested size is always applied when allow_client_resolution_change is on, regardless of whether the user set resolution_change=no_operation. Capability advertisement on allow_client_resolution_change alone is therefore correct and sufficient. AlkaidLab#5 (temp launch session loses display semantics): Save launch_use_vdd and launch_custom_screen_mode from the original launch_session_t in session_t during alloc(). Build the temp launch session with these saved values so configure_display targets the correct device (physical vs VDD) and honours the original custom screen-mode override. custom_screen_mode defaults to -1 (no override) — not 0 (which is a valid screen-mode index). enable_sops is forced true in the temp session because the client is explicitly requesting the change; the automatic path's enable_sops gate would otherwise silently drop it. AlkaidLab#9 (host rate-limit drops final size): Remove the 250 ms leading-edge rate-limit entirely. Clients (moonlight-qt, moonlight-vplus) already debounce ~400 ms before emitting one settled packet; a host-side drop window is both redundant and harmful — it would swallow the final settled size if it arrived within 250 ms of a prior one. Correctness beats incomplete storm protection.
Decode 0x5506 dynamic-resolution fields by copying bytes before little-endian conversion instead of dereferencing payload storage as int pointers. This removes the alignment/aliasing hazard from the client-driven resolution parser while keeping the existing wire format.
AlkaidLab#5 AlkaidLab#6 AlkaidLab#7 for client-driven resolution Fix AlkaidLab#1: gate ClientResolutionChange advertisement on BOTH allow_client_resolution_change AND dynamic_resolution_follow_display. Without follow_display the encoder-size update and 0x5507 notify never fire, so advertising the capability is misleading and would produce a broken client experience. Fix AlkaidLab#4: preserve launch_session.env in session_t (new launch_env field, saved in alloc()) and copy it into temp_launch_session before configure_display. Fixes SUNSHINE_CLIENT_DISPLAY_NAME routing (parsed_config.cpp:535) and SUNSHINE_CLIENT_CERT_UUID VDD client-id matching (session.cpp:189/548) which were silently lost in the zero-init temp session. Fix AlkaidLab#5: gate session->config.monitor mutation + IDR on result_e::success only. operator bool() returns true for deferred_retry too; mutating session state and raising IDR before the mode applied was premature. On deferred_retry or hard failure: log, keep old config, no IDR, allow future retry (client_res_last_apply already set so the rate-limit still applies but the state is not corrupted). Fix AlkaidLab#6: leading-edge 250 ms rate-limit in handle_resolution_change, checked after even-clamp + no-op dedupe and before configure_display. Sheds resolution-change storms; a single settled packet still applies immediately. Residual: a sub-250 ms burst of DISTINCT sizes may have one dropped. Fix AlkaidLab#7: force client_driven_video_cfg.refresh_rate_change = automatic alongside resolution_change = automatic. Without this a host setting of refresh_rate_change=no_operation would skip VDD custom-mode injection and the new fps from temp_launch_session would be ignored.
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit发行说明
功能概览新增 变更详情客户端驱动分辨率变更完整路径
交互序列图sequenceDiagram
participant Client as 客户端
participant nvhttp as serverinfo (nvhttp)
participant stream as stream (handle_resolution_change)
participant dd as configure_display
Client->>nvhttp: GET /serverinfo
nvhttp-->>Client: ClientResolutionChange=1 (当两项配置均启用)
Client->>stream: IDX_DYNAMIC_PARAM_CHANGE (0x5506 RESOLUTION, w, h)
stream->>stream: read_le_i32 解析宽高
stream->>stream: 门控检查 (allow_client_resolution_change)
stream->>stream: 偶数对齐 / 范围校验 / 无变化检测
stream->>stream: 250ms 前沿限流检查
stream->>dd: configure_display(临时 launch_session_t)
alt success
dd-->>stream: success
stream->>stream: 更新 session->config.monitor
stream->>Client: 触发 IDR
else deferred_retry / 失败
dd-->>stream: deferred_retry / failure
stream->>stream: 不更新监视配置，不触发 IDR
end
代码审查工作量估计🎯 4 (复杂) | ⏱️ ~50 分钟 可能相关的 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 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: 1
🤖 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/stream.cpp`:
- Around line 1402-1407: The capability gate for client resolution changes in
the request handler is inconsistent with the capability declaration in
src/nvhttp.cpp. Currently, the code only checks allow_client_resolution_change,
but the capability declaration specifies that ClientResolutionChange requires
both allow_client_resolution_change AND dynamic_resolution_follow_display to be
enabled. Update the condition in the if statement (where
allow_client_resolution_change is checked) to also require
dynamic_resolution_follow_display to be true. Both config options must be
enabled for the resolution change to be processed; if either is disabled, log
the rejection and return early.
🪄 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: 7b496cb4-8b17-4b5b-a715-ea096ec24c09
📒 Files selected for processing (4)
src/config.cppsrc/config.hsrc/nvhttp.cppsrc/stream.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,c,h}
⚙️ CodeRabbit configuration file
src/**/*.{cpp,c,h}: Sunshine 核心 C++ 源码，自托管游戏串流服务器。审查要点：内存安全、 线程安全、RAII 资源管理、安全漏洞。注意预处理宏控制的平台相关代码。
Files:
src/nvhttp.cppsrc/config.hsrc/config.cppsrc/stream.cpp
The serverinfo capability is advertised only when both allow_client_resolution_change and dynamic_resolution_follow_display are enabled, but the 0x5506 handler gated on the former alone. A client that ignores or caches the capability bit (or crafts the packet) could reach configure_display() with follow_display off, where the capture loop keeps the original stream resolution and never notifies the client -- a half-changed host state. Require both flags in the handler so it matches the advertised contract. Addresses CodeRabbit review on AlkaidLab#729.
Author
|
Good catch — fixed in b86e892. The |
Author
|
@qiin2333 anything I can do to promote this and the other PRs? |
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.
Summary
Implements the server side of client-driven ("RDP-style") dynamic resolution: when a client requests a stream resolution change mid-session, the host validates and applies it, then notifies the client. Closes #728.
This builds on the existing
0x5506/0x5507machinery (PRs #424/#669) and adds the gating, capability advertisement, and safety pieces that were missing for a client to safely drive it.What it does
video.allow_client_resolution_change(default off) — accept client0x5506RESOLUTION requests./serverinfo:<ClientResolutionChange>is emitted as1only whenallow_client_resolution_change && dynamic_resolution_follow_display, since the encoder-resolution update +0x5507notify in the capture loop are gated ondynamic_resolution_follow_display. Advertising honestly avoids telling clients to send requests the host would only half-apply.handle_resolution_change): even-clamp, no-op dedupe, leading-edge 250 ms rate-limit (storm protection; clients also debounce), thenconfigure_display. Session resolution is mutated and an IDR raised only onconfigure_result_t::result_e::success—deferred_retryand failures keep the old state and allow a retry.env(client display name, client cert / VDD identity),use_vdd, andcustom_screen_mode, and forcesresolution_change/refresh_rate_changetoautomaticso arbitrary client sizes inject into the VDD mode list.0x5506payload with explicit little-endian reads (no unalignedreinterpret_cast).Client side
Requires the shared common-c API and the matching clients:
LiSendResolutionChangeRequest)Known limitation
Client-driven resolution currently takes effect end-to-end only when
dynamic_resolution_follow_display=true(the capture loop's encoder-size update +0x5507notify path is gated on that flag). The capability is advertised accordingly. Wiring the capture loop to also fire for client-driven changes when follow-display is off is a sensible follow-up.Testing
Reviewed across two independent adversarial + correctness passes; all raised findings addressed. Not built in this environment (no local MSVC/CUDA/boost toolchain) — static review only. A maintainer build + a live client-resize smoke test (physical display and VDD) is the recommended gate before merge, per this repo's AI-code testing guidance.
Opened by a regular contributor (AsafMah).