Expose more nvenc and nvprefs options #1798
Conversation
1. Spatial AQ, for some reason NVENC have problems recovering details on flat regions of static images over multiple frames, official docs recommend to enable it for "game-streaming" 2. Override default DPB size in situations when moonlight doesn't request one explicitly, can improve the window of instant recovery from dropped or corrupted frames with Reference Frame Invalidation 3. Percentage increase of default single-frame VBV/HRD, can act as low latency variable bitrate substitute
| "nvenc_preset": "1", | ||
| "nvenc_realtime_hags": "enabled", | ||
| "nvenc_spatial_aq": "disabled", |
There was a problem hiding this comment.
| "nvenc_spatial_aq": "disabled", | |
| "nvenc_spatial_aq": "disabled", | |
| "nvenc_vbv_increase": 0, |
| .. Caution:: Applies to Windows only. | ||
|
|
||
| **Default** | ||
| Variable is unset and sunshine will pick whatever it thinks is the best. |
There was a problem hiding this comment.
So the default is not 5?
Generally, placeholders in the config ui match the default.
There was a problem hiding this comment.
Current default is constant 5, but I can't guarantee it will stay as single constant value in the future and not change dynamically depending on let's say video resolution.
There was a problem hiding this comment.
Okay, if it changes in the future to an auto mode this can be adjusted accordingly.
For now, I think this should be 5. Also, as long as the config UI uses the correct defaults, then it allows us to modify the default when a user updates. As long as they are using our old default, they will also start using our new default.
| "nvenc_latency_over_power": "enabled", | ||
| "nvenc_opengl_vulkan_on_dxgi": "enabled", |
There was a problem hiding this comment.
| "nvenc_latency_over_power": "enabled", | |
| "nvenc_opengl_vulkan_on_dxgi": "enabled", | |
| "nvenc_h264_hevc_ref_frames": "5", | |
| "nvenc_latency_over_power": "enabled", | |
| "nvenc_opengl_vulkan_on_dxgi": "enabled", |
| .. Caution:: Applies to Windows only. | ||
|
|
||
| **Default** | ||
| Variable is unset and sunshine will pick whatever it thinks is the best. |
There was a problem hiding this comment.
| Variable is unset and sunshine will pick whatever it thinks is the best. | |
| ``5`` |
| @@ -1077,6 +1130,96 @@ nvenc_realtime_hags | |||
|
|
|||
| nvenc_realtime_hags = enabled | |||
|
|
|||
| nvenc_h264_hevc_ref_frames | |||
There was a problem hiding this comment.
I'm not sure we should expose this option to users (at least in this fully-configurable form). The number of reference frames is limited by the codec specification and is resolution-dependent (as you mentioned below).
It would be easy for a user to end up in a situation where they specify a value here that works for 720p then later switch to 4K and wonder why things are totally broken, or they specify a value that works for HEVC but not H.264. The issues caused by getting it wrong are going to be really hard to diagnose both for us and for users, especially since it may only manifest with packet loss.
I think if we want to expose this option, we should do so in a way that only exposes a few known good options. For now, maybe we expose "Auto" (our current behavior) and "1". If we later decide to increase reference frames up to the codec maximum for each resolution, we can either change the behavior of "Auto" and explicitly add "5" as an option or add an option for "Maximum", depending on how the decoder compatibility looks in our testing.
There was a problem hiding this comment.
I'm back from the dead, somewhat. Sorry for leaving it to you to deal with.
Now to remember my reasoning behind this option, I think it went like this
- While "5" is a safe default value, it comes with the cost of needless memory overhead (on both server and client) when packet loss never happens. And can be considered a regression for such scenarios since we used "1" before RFI support was implemented.
- But "5" is only barely enough for RFI, because of the latencies involved and that the wireless network instabilities usually last longer than a couple of milliseconds. So on unstable networks you would generally want the highest number your decoder can handle (for given resolution).
As for packet loss, I don't think it can happen because of this option since it doesn't increase the amount of data sent over the network. Decoder artifacts on the other hand are fully expected, if it goes above the maximum supported codec profile level for given decoder hardware.
For now, maybe we expose "Auto" (our current behavior) and "1"
Maybe just use "RFI support on/off" server-side option, with the description of what it is and the memory overhead.
But ideally, this selection should be moved to the client side, since only that side has the information about the decoder and expected network stability. It's just easier to patch one server than 10 clients, even though it's not the "proper" solution. @cgutman I'm unable allocate time to patch every moonlight client, but can make quick sunshine PR if you're ok with it? This option can be useful right now for vm-to-vm looking glass and similar scenarios.
@ReenigneArcher Sorry for disappearing (this project was consistently eating up more time I was allocating to it, so I had to pull a hard stop), but can I have the discord access back so I'm aware of the recent developments? Pretty please with a cherry on top.
Description
Screenshot
Issues Fixed or Closed
Type of Change
.github/...)Checklist
Branch Updates
LizardByte requires that branches be up-to-date before merging. This means that after any PR is merged, this branch
must be updated before it can be merged. You must also
Allow edits from maintainers.