Add support for clientRefreshRateX100, enabling streaming at 59.94 or 119.88 hz - #200
Conversation
WalkthroughA new mechanism for determining and accessing the display refresh rate was added to the device resource management system. The refresh rate is now detected at construction, stored in a member variable, and exposed via accessor methods. The streaming client logic now uses this refresh rate if available, logging the requested value. Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
Common/DeviceResources.cpp (1)
821-837: Fix float literal inconsistency and approve the implementation approach.The platform detection logic correctly distinguishes between Xbox and Windows, and the Xbox implementation properly retrieves the display refresh rate. However, there's a minor type inconsistency.
Apply this diff to fix the type consistency:
- double refreshRate = 0.0f; + double refreshRate = 0.0;The overall approach aligns well with the PR objectives to support fractional refresh rates like 59.94 Hz and 119.88 Hz on Xbox devices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Common/DeviceResources.cpp(1 hunks)Common/DeviceResources.h(2 hunks)State/MoonlightClient.cpp(1 hunks)Streaming/moonlight_xbox_dxMain.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
Streaming/moonlight_xbox_dxMain.cpp (5)
Learnt from: LAGonauta
PR: TheElixZammuto/moonlight-xbox#167
File: State/MoonlightHost.h:38-38
Timestamp: 2025-02-27T02:36:15.388Z
Learning: When using hdmiDisplayMode in the MoonlightHost::Connect() method, a null check should be performed since hdmiDisplayMode might be null when loading previously saved hosts without display information. The MoonlightClient constructor should either handle null parameters or the Connect() method should initialize hdmiDisplayMode with a default value before passing it.
Learnt from: LAGonauta
PR: TheElixZammuto/moonlight-xbox#167
File: State/MoonlightHost.h:38-38
Timestamp: 2025-02-27T02:36:15.388Z
Learning: In MoonlightHost class, hdmiDisplayMode should be explicitly initialized to nullptr in the constructor, and null checks should be added before accessing it in methods like Connect(). This is particularly important because hdmiDisplayMode might not be initialized when loading previously saved hosts that don't contain display information.
Learnt from: LAGonauta
PR: TheElixZammuto/moonlight-xbox#167
File: State/MoonlightHost.h:38-38
Timestamp: 2025-02-27T02:36:15.388Z
Learning: In C++/CX, reference type member variables like HdmiDisplayModeWrapper^ should be explicitly initialized to nullptr in constructors if they don't have a valid value at construction time. Leaving them uninitialized can lead to null-pointer exceptions when they're accessed without checks.
Learnt from: andygrundman
PR: TheElixZammuto/moonlight-xbox#178
File: Streaming/moonlight_xbox_dxMain.cpp:444-455
Timestamp: 2025-03-31T11:54:31.863Z
Learning: In the moonlight-xbox project, using Sleep() inside concurrency::create_async() is an acceptable pattern when the intention is to specifically block that background thread while not affecting the UI responsiveness, such as when simulating key sequences that require specific timing.
Learnt from: LAGonauta
PR: TheElixZammuto/moonlight-xbox#167
File: State/MoonlightHost.h:38-38
Timestamp: 2025-02-27T02:36:15.388Z
Learning: In MoonlightHost class, hdmiDisplayMode should be initialized in the constructor to prevent null-pointer exceptions. This is particularly important when connecting to previously saved hosts where hdmiDisplayMode data might not be available.
Common/DeviceResources.h (1)
Learnt from: LAGonauta
PR: TheElixZammuto/moonlight-xbox#167
File: State/MoonlightHost.h:38-38
Timestamp: 2025-02-27T02:36:15.388Z
Learning: In C++/CX, reference type member variables like HdmiDisplayModeWrapper^ should be explicitly initialized to nullptr in constructors if they don't have a valid value at construction time. Leaving them uninitialized can lead to null-pointer exceptions when they're accessed without checks.
🧬 Code Graph Analysis (1)
State/MoonlightClient.cpp (3)
State/MoonlightClient.h (1)
res(23-23)Utils.hpp (1)
Logf(15-15)Utils.cpp (2)
Logf(90-99)Logf(90-90)
🔇 Additional comments (2)
Streaming/moonlight_xbox_dxMain.cpp (1)
76-77: LGTM! Proper refresh rate initialization.The refresh rate initialization correctly retrieves the UWP refresh rate and stores it in device resources for use by the streaming configuration. This establishes the foundation for fractional refresh rate support.
State/MoonlightClient.cpp (1)
221-225: LGTM! Correctly implements fractional refresh rate streaming.The conditional logic properly checks for valid refresh rate availability before setting
clientRefreshRateX100. The multiplication by 100 and integer cast correctly formats the value for the streaming protocol. The logging will help verify the feature works as described in the PR objectives.This change enables the core functionality of requesting streams at exact fractional refresh rates like 59.94 Hz and 119.88 Hz, which aligns perfectly with the PR goals.
|
Can you send me the build files to test it too? I tested the version launched with V-Sync and didn't worked the 120fps. |
b09e6b4 to
01a4452
Compare
|
Sorry, do you mean a Sunshine build that supports this option? |
3 participants
I've submitted a Sunshine PR [1] to support an old unused GFE parameter for fractional refresh rates. The easiest client that could test this is Xbox and the patch is very small. You can verify that it's working in the Sunshine log by looking for a line like this:
Info: Requested frame rate [60000/1001 exactly 59.94 fps]I've also modified dregu's frameskip tool [2] to support fractional rates. Whether or not it's accurate enough to matter, I don't know, but it's better than nothing. Xbox still needs better frame pacing which I'm working on, but this may be a small improvement in the meantime. I have tested this at 119.88 and 59.94.
[1] LizardByte/Sunshine#4019
[2] https://andygrundman.github.io/frameskip/
Summary by CodeRabbit
New Features
Bug Fixes