Resolution mode bug fixes #172
Conversation
…into resolution-mode-bug-fixes
WalkthroughThis pull request performs several updates across the codebase. It adjusts a warning message in the host settings to clarify HDR availability, updates text formatting in the README, refines the logic for handling frames per second and HDMI display mode resolution settings, and modifies the HDR/SDR mode selection process in the video renderer, including enhancements to error logging and mutex management. Changes
Sequence Diagram(s)Dynamic HDMI Display Mode Resolution (MoonlightHost)sequenceDiagram
participant Host as MoonlightHost
participant DMI as HDMI Display Information
participant DM as Supported Modes
participant Mode as HdmiDisplayModeWrapper
Host->>Host: Call HdmiDisplayMode getter
alt hdmiDisplayMode is null
Host->>DMI: Retrieve current display info
DMI->>DM: Provide supported display modes
alt Matching mode found
DM->>Host: Return matching mode
Host->>Mode: Instantiate HdmiDisplayModeWrapper with match
else
Host->>Host: Use default mode (zero resolution)
end
else
Host->>Host: Return existing hdmiDisplayMode
end
HDR/SDR Mode Selection Flow (VideoRenderer)sequenceDiagram
participant VR as VideoRenderer
participant RM as requestedModeWrapper
participant DI as HdmiDisplayInformation
participant Log as Logger
VR->>VR: Declare requestedModeWrapper (pre-check)
alt HDR is enabled
VR->>RM: Check HDR compatibility
alt Not compatible
VR->>Log: Log incompatibility message
VR-->>VR: Return early
else
VR->>DI: Retrieve current display mode
end
else HDR is disabled
VR->>VR: Validate requestedMode
alt requestedMode invalid
VR->>VR: Set default display mode
else
VR->>VR: Check if requestedMode is SDR
VR->>VR: Iterate supported modes for a match
VR->>VR: Update requestedMode if match found
end
VR->>VR: Request asynchronous display mode change
end
Note over VR: In Stop method, mutex locking is applied,<br/>default display mode and sRGB are restored, and errors are logged.
Possibly related PRs
Poem
✨ 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:
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: 0
🧹 Nitpick comments (5)
State/ApplicationState.cpp (2)
48-54: Refactor conditionals for better readability.The nested
if/elselogic for picking betweendisplayHeight/displayWidthandwidth/heightis correct but can be condensed or documented for clarity. Consider a helper function to unify the resolution selection logic.- else if (a.contains("width") && a.contains("height")) - { - h->HdmiDisplayMode = ref new HdmiDisplayModeWrapper(ResolveResolution(a["height"].get<int>(), a["width"].get<int>(), nullptr, 0, fps)); - } - else { - h->HdmiDisplayMode = ref new HdmiDisplayModeWrapper(ResolveResolution(0,0,nullptr,0,fps)); - } + else { + int w = 0, h = 0, bpp = 0; + Platform::String^ space = nullptr; + if (a.contains("width") && a.contains("height")) { + w = a["width"].get<int>(); + h = a["height"].get<int>(); + } else if (a.contains("displayWidth") && a.contains("displayHeight")) { + w = a["displayWidth"].get<int>(); + h = a["displayHeight"].get<int>(); + space = Utils::StringFromStdString(a["colorSpace"].get<std::string>()); + bpp = a["bitsPerPixel"].get<int>(); + } + h->HdmiDisplayMode = ref new HdmiDisplayModeWrapper(ResolveResolution(h, w, space, bpp, fps)); + }
179-179: Retaining modes[0] for zero dimensions might skip color space logic.Returning
modes[0]is a reasonable fallback for invalid resolutions, but it silently ignores color space and bit depth input parameters. Consider logging or otherwise notifying the user that the zero-dimension case defaults to the first supported mode.State/MoonlightHost.h (1)
155-181: Late initialization of HdmiDisplayMode is beneficial but consider an explicit fallback.Dynamically loading the HDMI display mode if
hdmiDisplayModeis null is helpful. However, if no matching resolution is found among the modes, the code currently selects the first mode. Consider logging the mismatch case explicitly.+ // If no matching resolution was found, explicitly log the fallback + if (mode == nullptr) { + mode = modes[0]; + Utils::Log("No exact match found. Using first mode as fallback.\n"); + }Streaming/VideoRenderer.cpp (2)
633-654: Setting SDR display mode after HDR is disabled.The fallback logic for selecting a matching SDR resolution looks correct, but it’s fairly verbose. You could break this out into a small helper to maintain clarity and reduce nested loops.
679-701: Lock usage in Stop() is appropriate but consider awaiting SetDefaultDisplayModeAsync.Locking the FFmpeg decoder before changing the display mode is correct for thread safety. However, the
SetDefaultDisplayModeAsync()call is not awaited; if any failure occurs during the async operation, the method won't see that result before unlocking. Logging an error callback or awaiting might help catch issues in real time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Pages/HostSettingsPage.xaml.cpp(1 hunks)README.md(1 hunks)State/ApplicationState.cpp(2 hunks)State/MoonlightHost.h(2 hunks)Streaming/VideoRenderer.cpp(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🔇 Additional comments (6)
Pages/HostSettingsPage.xaml.cpp (1)
270-270: Improved clarity in HDR availability message.The modification to the warning message is a good improvement, as it clarifies that HDR availability is specifically tied to display resolution settings, not just refresh rates. This change aligns with the PR objectives of fixing bugs related to display modes and HDR functionality, providing users with more precise information about HDR availability.
State/ApplicationState.cpp (3)
37-39: Ensure fps is validated for unexpected values.While setting a default fps of 30 is good, consider verifying that any user-provided fps is within a reasonable range (e.g., > 0) to avoid potential anomalies or division-by-zero errors elsewhere.
46-46: Use of fps for ResolveResolution looks consistent.The
fpsparameter aligns with the fifth argument inResolveResolution(). No immediate issues found. Ensure that theRefreshRateproperty, which is adouble, correctly accepts all valid fps values.
56-56: No concerns with newly added comment.The comment line “//Resolve StreamResolution” is consistent with the surrounding logic. No actionable issues.
State/MoonlightHost.h (1)
26-26: Default fps changed to 30.This aligns with typical streaming scenarios. Ensure any existing references in the codebase that assumed a default of 60 are updated accordingly.
Streaming/VideoRenderer.cpp (1)
563-563: Retrieving the HDR display mode externally.
auto requestedModeWrapper = client->GetDisplayMode();nicely centralizes retrieval. Confirm that this call never returns null. Ifclient->GetDisplayMode()can fail or return a null wrapper, ensure a fallback is used.
|
|
While building this for testing, I happened to run it on the local Windows PC. I think all the HDMI-related code will need to be inside checks for Xbox hardware. The first place I ran into a crash was after adding a host and trying to view host settings. |
|
I built this and did some testing. Most of my concerns are around the UX where the user should not be changing their system resolution within an app. My Xbox got hit with the factory reset bug but it let me see the defaults for a brand new user. Before launching the app I set the system to 4k120, all video options enabled, and 10-bit color. Here is the default set of host options for a brand new user:
Now it gets worse because the FPS option is gone and all we have is this refresh rate selection. The user also has to know what bitrate to use, the default of 8Mbps makes no sense and there is no default calculation. It takes so long to drag the slider up from 8 to 80. The settings page also lets you enable HDR for h264. There's just so many ways the average user will mess up these settings.
The same value is used for both the TV's actual refresh rate and the value sent to Sunshine. The settings in this menu have state issues and it's easy to have options disappear or no longer work right. I got stuck at 24hz once for example. From the Sunshine log, note the wrong rounding being done here. Same thing happens at 119.88 asking for 119fps. At 24hz the UI is painful to use and everything sucks, there should never be a reason for someone's TV to switch to 24hz. Sometimes I get a washed out image that looks like this (captured from Xbox in SDR mode but this is about how it looks):
I'm not sure exactly what causes this but one thing it could be are the 2 calls to (HDR also has another issue not caused by this patch which is the use of an 8-bit back buffer but it's hard to see the effects of this. I think it might just mean HDR operates similar to the "8-bit with dithering" mode in Windows operates for certain low-bandwidth monitors. I started trying to fix this but didn't get very far.) Finally, the stats. If no one minds I'd like to refactor and add to the stats so that they function as close to Qt as possible. |
Fixed translating old config files and display modes not setting without HDR.
Summary by CodeRabbit
Refactor
Documentation