Add Video Super Resolution for Xbox#267
Open
linckosz wants to merge 9 commits intoTheElixZammuto:masterfrom
Open
Add Video Super Resolution for Xbox#267linckosz wants to merge 9 commits intoTheElixZammuto:masterfrom
linckosz wants to merge 9 commits intoTheElixZammuto:masterfrom
Conversation
This commit introduces a high-quality upscaling pipeline for Xbox using the NVIDIA Image Scaling (NIS) algorithm. - Implement NIS (Nvidia Image Scaling) Shader algorthim to upscale with reconstruction algorithm. - Implement DirectX 11 capability checks for Half-Precision (FP16) shader support to optimize compute performance. - Add a user preference (Video Super Resolution) to toggle between NIS and standard rendering. - Add a new class VideoUpscaler to load and apply the shader to limit the changes in VideoRenderer - Add the video enhancement information to the overlay Ref: https://github.com/NVIDIAGameWorks/NVIDIAImageScaling
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds optional Video Super Resolution (VSR) via NVIDIA Image Scaling: new host setting persisted in state.json, StreamConfiguration flag, stats plumbing for scale ratio, a D3D11 VideoUpscaler (NIS) and integration into VideoRenderer render loop, plus project/shader/submodule and UI changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Frame as Frame Source
participant Renderer as VideoRenderer
participant Upscaler as VideoUpscaler
participant Backbuffer as Backbuffer
participant Stats as StatsSubsystem
Frame->>Renderer: Deliver decoded YUV frame
Note over Renderer: Check StreamConfiguration.videoSuperResolution
alt VSR enabled
Renderer->>Renderer: Render YUV→RGB into intermediate RTV (stream size)
Renderer->>Upscaler: ProcessFrame(intermediate SRV)
Upscaler->>Upscaler: Run NIS compute shader (coef SRVs, CB)
Upscaler-->>Renderer: Return upscaled SRV
Renderer->>Backbuffer: Copy upscaled region (fsrDst) to backbuffer
Renderer->>Stats: SubmitScaleRatio(fsrDst.height / frame.height)
else VSR disabled
Renderer->>Backbuffer: Render YUV→RGB directly to backbuffer
end
Stats->>Stats: Aggregate and include "Video Enhancement" when present
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
Streaming/VideoRenderer.h (1)
8-8: Forward-declareVideoUpscalerinstead of including its header.
VideoRenderer.his included broadly, andVideoUpscaler.hcurrently pulls inpch.hand<NIS_Config.h>, which leaks NIS internals and increases build times across the project. Since only astd::unique_ptr<VideoUpscaler>member is used here, a forward declaration is sufficient (the destructor definition inVideoRenderer.cppwill see the full type).🔧 Proposed change
-#include "VideoUpscaler.h" ... +namespace moonlight_xbox_dx { class VideoUpscaler; }Move
#include "VideoUpscaler.h"intoVideoRenderer.cpp.Note: if
VideoRenderer's destructor is implicitly defined in the header, you'll need to declare~VideoRenderer()and define it out-of-line in the.cppsounique_ptr<VideoUpscaler>can see the complete type at destruction.Also applies to: 102-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Streaming/VideoRenderer.h` at line 8, Replace the `#include` "VideoUpscaler.h" in VideoRenderer.h with a forward declaration "class VideoUpscaler;" and keep the std::unique_ptr<VideoUpscaler> member as-is; move the `#include` "VideoUpscaler.h" into VideoRenderer.cpp so the destructor sees the complete type. If VideoRenderer currently has an implicitly-defined destructor in the header, add an explicit declaration ~VideoRenderer(); in the class and define it out-of-line in VideoRenderer.cpp (after including VideoUpscaler.h) so unique_ptr<VideoUpscaler> can destroy the pointed-to object without leaking NIS headers into the public header..gitmodules (1)
13-15: Consider a shallow/sparse clone for the NIS submodule.
NVIDIAGameWorks/NVIDIAImageScalingcontains samples and binary blobs that bloat the repo checkout while this project only needs the three files underNIS/(NIS_Main.hlsl,NIS_Scaler.h,NIS_Config.h). If feasible, considershallow = truehere and/or a sparse-checkout insetup-dev.ps1to reduce clone size/time for contributors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitmodules around lines 13 - 15, The NVIDIAImageScaling submodule pulls the whole upstream repo including samples/binaries but we only need the three files under NIS/; update the submodule and setup script to reduce clone size by adding shallow = true to the submodule entry for "third_party/NVIDIAImageScaling" in .gitmodules and modify setup-dev.ps1 to perform a sparse-checkout (or sparse clone/partial clone) that only checks out NIS/NIS_Main.hlsl, NIS/NIS_Scaler.h, and NIS/NIS_Config.h from the submodule URL, ensuring the submodule init/update commands use --depth=1 (or git sparse-checkout set) so contributors avoid downloading unnecessary blobs.Streaming/VideoUpscaler.cpp (2)
190-215:ProcessFramedoesn't save/restore prior CS bindings.
ProcessFrameoverwrites CS sampler/CB/SRV/UAV slot 0..2 and only clears SRV/UAV slots on exit (not the sampler or constant buffer, and not the shader). Currently this is fine because nothing else inVideoRendereruses the compute stage, but it's a subtle coupling — any future compute pass invoked in the same frame will inherit NIS's sampler/CB/shader. Consider clearing the remaining slots (or documenting the invariant) to keep this self-contained:ctx->CSSetShaderResources(0, 3, nullSRVs); + ID3D11Buffer* nullCB[] = { nullptr }; + ctx->CSSetConstantBuffers(0, 1, nullCB); + ctx->CSSetShader(nullptr, nullptr, 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Streaming/VideoUpscaler.cpp` around lines 190 - 215, ProcessFrame overwrites compute-stage bindings (sampler, constant buffer, shader, SRVs, UAV) but only clears SRVs/UAVs; fix by saving/restoring or explicitly clearing the remaining CS bindings after the dispatch: read current bindings via GetD3DDeviceContext() before setting (store prior sampler(s), prior constant buffer(s), and prior compute shader), then call CSSetSamplers/CSSetConstantBuffers/CSSetShader to restore the saved originals (or set them to null) after the Dispatch; target symbols: ProcessFrame, m_sampler, m_nisCB, m_nisShader and the context methods CSSetSamplers, CSSetConstantBuffers, CSSetShader (and existing CSSetShaderResources/CSSetUnorderedAccessViews) so the compute stage state is self-contained.
67-70: IgnoringCheckFeatureSupportHRESULT is fine but worth a log.If the call fails,
featureSupportstays zero-initialized sosupportsFP16falls back tofalse, which is safe. Consider logging a warning onFAILED(hr)to aid diagnostics — otherwise you'll silently compile the slower FP32 path on hardware that actually supports FP16 when the query transiently fails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Streaming/VideoUpscaler.cpp` around lines 67 - 70, Capture the HRESULT returned by device->CheckFeatureSupport when querying D3D11_FEATURE_SHADER_MIN_PRECISION_SUPPORT (the D3D11_FEATURE_DATA_SHADER_MIN_PRECISION_SUPPORT struct and supportsFP16 logic), check FAILED(hr) and emit a warning log (or debug output) including the HRESULT so transient failures are visible; keep the existing fallback to false for supportsFP16 but ensure the warning provides context that the feature query failed for diagnostics.Streaming/VideoUpscaler.h (1)
3-10: Avoid includingpch.hand NIS headers from a public header.Two concerns:
#include "pch.h"in a header is non-idiomatic — PCH should be included as the first line of each.cpp, not transitively through headers. It also breaks when this header is included from a TU that uses a different/no PCH.- Including
<NIS_Config.h>here leaks NIS symbols/macros into every translation unit that includesVideoUpscaler.h(e.g.VideoRenderer.h, which in turn is included widely). None of the declarations in this header actually needNIS_Config.h— the members areComPtrs and ints. Move the NIS include toVideoUpscaler.cpp.🔧 Proposed change
-#include "pch.h" `#include` <d3d11.h> `#include` <wrl/client.h> `#include` <memory> `#include` "..\Common\DeviceResources.h" - -// Define NIS generic architecture mapping based on GPU -#include <NIS_Config.h>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Streaming/VideoUpscaler.h` around lines 3 - 10, Remove the non-idiomatic and leaking includes from VideoUpscaler.h: delete `#include` "pch.h" and `#include` <NIS_Config.h> from the header and keep only the necessary declarations (e.g., <d3d11.h>, <wrl/client.h>, <memory>, and "..\Common\DeviceResources.h"); then add `#include` "pch.h" (if your TU uses a PCH) and `#include` <NIS_Config.h> into VideoUpscaler.cpp so NIS macros/symbols are confined to the implementation and do not pollute every TU that includes VideoUpscaler.h or VideoRenderer.h.Streaming/VideoRenderer.cpp (1)
142-168: Avoid keeping two VSR initialization implementations.The inline initialization in
Render()duplicatesInitializeUpscaler(), which makes fixes to resource creation, logging, and fallback behavior easy to apply in only one path. Prefer calling the helper fromRender()or removing the unused helper.Also applies to: 726-773
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Streaming/VideoRenderer.cpp` around lines 142 - 168, The Render() method contains an inline VideoUpscaler initialization that duplicates the logic in InitializeUpscaler(); remove the duplicate block in Render() and call InitializeUpscaler() instead (or consolidate all initialization into InitializeUpscaler()), ensuring you use the same resource creation, error handling and fallback paths as InitializeUpscaler() for m_upscaler, m_intermediateTex, m_intermediateRTV and m_intermediateSRV; update any parameters passed (src.w/src.h, fsrDst.w/fsrDst.h, renderFormat, isHDR) to match InitializeUpscaler()’s signature and ensure failures reset m_upscaler and log/handle errors exactly as the helper does (also apply the same change to the other duplicated region around the 726-773 area).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@State/Stats.cpp`:
- Around line 314-344: formatVideoStats() (or the StatsRenderer code path)
dereferences GetStreamConfig()->videoSuperResolution without checking
GetStreamConfig() for null, which can crash if called before config
initialization; fix by obtaining a StreamConfig* cfg = GetStreamConfig() and
guarding its use (e.g., treat cfg == nullptr as videoSuperResolution == false or
choose the safe branch) before accessing cfg->videoSuperResolution, and update
the conditional that currently reads GetStreamConfig()->videoSuperResolution to
use the guarded cfg variable so both branches (super resolution and non-super
resolution) are safe when GetStreamConfig() is null.
In `@Streaming/VideoRenderer.cpp`:
- Around line 194-217: Compute a single bool like vsrActive that requires
configuration->videoSuperResolution && m_upscaler && m_intermediateRTV &&
m_intermediateSRV (i.e., VSR enabled and all resources/objects successfully
initialized), then use vsrActive in this render-target/viewport selection block
instead of checking configuration->videoSuperResolution alone, and also update
setupVertexBuffer() to switch to the full-screen quad only when vsrActive is
true so the vertex layout matches the actual render target and preserves aspect
ratio when the upscaler or intermediate resources are missing.
- Around line 121-146: The upscaler (m_upscaler / VideoUpscaler::Initialize) is
only recreated when hasFrameFormatChanged(frame) is true, but Initialize also
depends on fsrDst.w/fsrDst.h (output pixel size) and isHDR (HDR/SDR), so a
display-size or HDR mode change can leave the upscaler and its cached state
stale; update the logic so that you also detect changes to the output rectangle
(fsrDst.w, fsrDst.h) and HDR flag (isHDR) in addition to hasFrameFormatChanged,
call m_upscaler->Initialize(...) whenever any of these differ (recreating
m_upscaler if null), and only update the cached upscaler dimensions/HDR flag
after Initialize returns success to avoid applying stale textures/shader modes
or scale ratios (refer to hasFrameFormatChanged, m_upscaler,
VideoUpscaler::Initialize, fsrDst, isHDR).
- Around line 157-164: When recreating the intermediate texture and views in
VideoRenderer.cpp, ensure you release existing COM pointers first and check each
dependent HRESULT separately: call ReleaseAndGetAddressOf() on
m_intermediateTex, m_intermediateRTV, and m_intermediateSRV before
CreateTexture2D/CreateRenderTargetView/CreateShaderResourceView to avoid leaks,
check the HRESULT returned from CreateTexture2D and bail out (reset m_upscaler)
if it fails, then call CreateRenderTargetView and verify its HRESULT before
proceeding to CreateShaderResourceView and verify that HRESULT as well; on any
failure, release any partially created resources and set m_upscaler.reset() (use
the same pattern at both the texture creation site and the second site
mentioned).
In `@Streaming/VideoUpscaler.cpp`:
- Around line 165-180: Add HRESULT checks and error handling for
device->CreateShaderResourceView and device->CreateSamplerState after
CreateTexture2D/CreateUnorderedAccessView so failures don't leave
m_finalOutputSRV or m_sampler null (log the HRESULT and return false), and in
VideoRenderer::InitializeUpscaler validate or coerce the caller-supplied format
used with CreateTexture2D+D3D11_BIND_UNORDERED_ACCESS to a UAV-capable format
(e.g., pick R16G16B16A16_FLOAT for HDR or R8G8B8A8_UNORM / R10G10B10A2_UNORM for
SDR) before creating m_finalOutputTex so CreateUnorderedAccessView won't fail on
formats like DXGI_FORMAT_B8G8R8A8_UNORM; ensure ProcessFrame uses non-null
m_finalOutputSRV/m_sampler or bails early if initialization failed.
- Around line 86-132: Replace the runtime HLSL compilation call
D3DCompileFromFile for "NIS_Main.hlsl" (the try/catch block that currently
compiles and then calls device->CreateComputeShader to populate m_nisShader)
with loading a precompiled .cso blob via D3DReadFileToBlob and creating the
compute shader from that blob; stop invoking D3DCompileFromFile at Initialize
(or wherever this block lives), and instead generate offline .cso variants at
build time keyed by supportsFP16/isHDR/blockW/blockH/threadGroupSize (the same
symbols used in the current D3D_SHADER_MACRO defines) so the runtime code opens
the matching .cso file, calls D3DReadFileToBlob, and then
device->CreateComputeShader with the blob, removing the runtime d3dcompiler
dependency and avoiding repeated heavy compilation on reconfigure.
---
Nitpick comments:
In @.gitmodules:
- Around line 13-15: The NVIDIAImageScaling submodule pulls the whole upstream
repo including samples/binaries but we only need the three files under NIS/;
update the submodule and setup script to reduce clone size by adding shallow =
true to the submodule entry for "third_party/NVIDIAImageScaling" in .gitmodules
and modify setup-dev.ps1 to perform a sparse-checkout (or sparse clone/partial
clone) that only checks out NIS/NIS_Main.hlsl, NIS/NIS_Scaler.h, and
NIS/NIS_Config.h from the submodule URL, ensuring the submodule init/update
commands use --depth=1 (or git sparse-checkout set) so contributors avoid
downloading unnecessary blobs.
In `@Streaming/VideoRenderer.cpp`:
- Around line 142-168: The Render() method contains an inline VideoUpscaler
initialization that duplicates the logic in InitializeUpscaler(); remove the
duplicate block in Render() and call InitializeUpscaler() instead (or
consolidate all initialization into InitializeUpscaler()), ensuring you use the
same resource creation, error handling and fallback paths as
InitializeUpscaler() for m_upscaler, m_intermediateTex, m_intermediateRTV and
m_intermediateSRV; update any parameters passed (src.w/src.h, fsrDst.w/fsrDst.h,
renderFormat, isHDR) to match InitializeUpscaler()’s signature and ensure
failures reset m_upscaler and log/handle errors exactly as the helper does (also
apply the same change to the other duplicated region around the 726-773 area).
In `@Streaming/VideoRenderer.h`:
- Line 8: Replace the `#include` "VideoUpscaler.h" in VideoRenderer.h with a
forward declaration "class VideoUpscaler;" and keep the
std::unique_ptr<VideoUpscaler> member as-is; move the `#include` "VideoUpscaler.h"
into VideoRenderer.cpp so the destructor sees the complete type. If
VideoRenderer currently has an implicitly-defined destructor in the header, add
an explicit declaration ~VideoRenderer(); in the class and define it out-of-line
in VideoRenderer.cpp (after including VideoUpscaler.h) so
unique_ptr<VideoUpscaler> can destroy the pointed-to object without leaking NIS
headers into the public header.
In `@Streaming/VideoUpscaler.cpp`:
- Around line 190-215: ProcessFrame overwrites compute-stage bindings (sampler,
constant buffer, shader, SRVs, UAV) but only clears SRVs/UAVs; fix by
saving/restoring or explicitly clearing the remaining CS bindings after the
dispatch: read current bindings via GetD3DDeviceContext() before setting (store
prior sampler(s), prior constant buffer(s), and prior compute shader), then call
CSSetSamplers/CSSetConstantBuffers/CSSetShader to restore the saved originals
(or set them to null) after the Dispatch; target symbols: ProcessFrame,
m_sampler, m_nisCB, m_nisShader and the context methods CSSetSamplers,
CSSetConstantBuffers, CSSetShader (and existing
CSSetShaderResources/CSSetUnorderedAccessViews) so the compute stage state is
self-contained.
- Around line 67-70: Capture the HRESULT returned by device->CheckFeatureSupport
when querying D3D11_FEATURE_SHADER_MIN_PRECISION_SUPPORT (the
D3D11_FEATURE_DATA_SHADER_MIN_PRECISION_SUPPORT struct and supportsFP16 logic),
check FAILED(hr) and emit a warning log (or debug output) including the HRESULT
so transient failures are visible; keep the existing fallback to false for
supportsFP16 but ensure the warning provides context that the feature query
failed for diagnostics.
In `@Streaming/VideoUpscaler.h`:
- Around line 3-10: Remove the non-idiomatic and leaking includes from
VideoUpscaler.h: delete `#include` "pch.h" and `#include` <NIS_Config.h> from the
header and keep only the necessary declarations (e.g., <d3d11.h>,
<wrl/client.h>, <memory>, and "..\Common\DeviceResources.h"); then add `#include`
"pch.h" (if your TU uses a PCH) and `#include` <NIS_Config.h> into
VideoUpscaler.cpp so NIS macros/symbols are confined to the implementation and
do not pollute every TU that includes VideoUpscaler.h or VideoRenderer.h.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0fe04746-0ba9-4fef-b110-0196f5197fbc
📒 Files selected for processing (16)
.gitmodulesPages/AppPage.xaml.cppPages/HostSettingsPage.xamlState/ApplicationState.cppState/MoonlightHost.hState/Stats.cppState/Stats.hState/StreamConfiguration.hStreaming/StatsRenderer.cppStreaming/VideoRenderer.cppStreaming/VideoRenderer.hStreaming/VideoUpscaler.cppStreaming/VideoUpscaler.hmoonlight-xbox-dx.vcxprojmoonlight-xbox-dx.vcxproj.filtersthird_party/NVIDIAImageScaling
Contributor
|
I'm a bit confused about the use case for this, people streaming to their Xbox are streaming at full resolution, I don't think anyone would choose to stream at a lower resolution just so it could be upscaled. Any upscaling is done on the host PC and will be much higher quality (in-game DLSS, etc). |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Streaming/VideoUpscaler.cpp (1)
149-173:⚠️ Potential issue | 🟡 MinorReturn
nullptrwhen the upscaler is not fully ready.Returning
inputSRVon an unready upscaler can makeVideoRenderercopy the stream-sized intermediate texture as if it were the upscaled output. Prefer a strict readiness guard and let the caller skip the final copy.Proposed guard
ID3D11ShaderResourceView* VideoUpscaler::ProcessFrame(ID3D11ShaderResourceView* inputSRV) { - if (!m_nisShader || !inputSRV) return inputSRV; + if (!inputSRV || + !m_nisShader || + !m_nisCB || + !m_sampler || + !m_coefScaleSRV || + !m_coefUsmSRV || + !m_finalOutputUAV || + !m_finalOutputSRV) { + return nullptr; + }Verification:
#!/bin/bash # Expect callers of ProcessFrame to treat nullptr as "do not copy/use upscaled output". rg -n -C5 '\bProcessFrame\s*\(|finalSRV' Streaming🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Streaming/VideoUpscaler.cpp` around lines 149 - 173, The readiness guard in VideoUpscaler::ProcessFrame currently returns inputSRV when the upscaler isn't ready, which causes callers (e.g. VideoRenderer) to treat the un-upscaled texture as final; change the guard so that if !m_nisShader or !inputSRV the function returns nullptr instead of inputSRV, ensuring callers skip any final copy/use of the upscaled output; update references to the symbols VideoUpscaler::ProcessFrame, m_nisShader and inputSRV accordingly and ensure the early return happens before any device context work (so no samplers/SRVs/UAVs are bound when not ready).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Assets/Shader/build_nis.bat`:
- Around line 1-6: The batch script uses relative paths and unquoted variables
which break when run from another CWD and has LF endings; update the
build_nis.bat to start with setlocal and pushd %~dp0 to lock the script
directory, ensure all path variables like HLSL and all /Fo outputs are quoted
(eg "%HLSL%" and "/Fo \"nis_sdr_fp16.cso\""), append error handling (|| exit /b
1) to every fxc invocation, and ensure the file is saved with CRLF line endings;
also add a popd and endlocal before exit so functions like the fxc commands and
variables (HLSL, fxc invocations) run reliably from any working directory.
In `@Streaming/VideoRenderer.cpp`:
- Around line 143-168: ReleaseDeviceDependentResources() must tear down
D3D-dependent objects to avoid stale resources after device loss: call
m_upscaler.reset() to destroy the VideoUpscaler instance and Reset() the COM
pointers m_intermediateSRV, m_intermediateRTV, and m_intermediateTex (or
ReleaseAndGetAddressOf/Reset on their ComPtr types) inside
ReleaseDeviceDependentResources(), ensuring all device-dependent state used by
Initialize (m_upscaler, m_intermediateTex, m_intermediateRTV, m_intermediateSRV)
is cleared when the device is torn down.
---
Duplicate comments:
In `@Streaming/VideoUpscaler.cpp`:
- Around line 149-173: The readiness guard in VideoUpscaler::ProcessFrame
currently returns inputSRV when the upscaler isn't ready, which causes callers
(e.g. VideoRenderer) to treat the un-upscaled texture as final; change the guard
so that if !m_nisShader or !inputSRV the function returns nullptr instead of
inputSRV, ensuring callers skip any final copy/use of the upscaled output;
update references to the symbols VideoUpscaler::ProcessFrame, m_nisShader and
inputSRV accordingly and ensure the early return happens before any device
context work (so no samplers/SRVs/UAVs are bound when not ready).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 165e1d33-75c8-401b-953c-b29bae32b17c
📒 Files selected for processing (11)
Assets/Shader/build_nis.batAssets/Shader/nis_hdr_fp16.csoAssets/Shader/nis_hdr_fp32.csoAssets/Shader/nis_sdr_fp16.csoAssets/Shader/nis_sdr_fp32.csoState/Stats.cppStreaming/VideoRenderer.cppStreaming/VideoRenderer.hStreaming/VideoUpscaler.cppmoonlight-xbox-dx.vcxprojmoonlight-xbox-dx.vcxproj.filters
✅ Files skipped from review due to trivial changes (1)
- moonlight-xbox-dx.vcxproj.filters
🚧 Files skipped from review as they are similar to previous changes (3)
- Streaming/VideoRenderer.h
- State/Stats.cpp
- moonlight-xbox-dx.vcxproj
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Assets/Shader/build_nis.bat (1)
1-1:⚠️ Potential issue | 🟡 MinorSave this batch file with CRLF line endings.
The file is still detected as LF-only, which keeps a known
cmd.exeparsing risk for.batfiles. Please convert it to Windows CRLF before merging.#!/bin/bash # Verifies that every LF byte in the batch file is part of a CRLF sequence. # Expected result: bare_LF=0. python - <<'PY' from pathlib import Path p = Path("Assets/Shader/build_nis.bat") data = p.read_bytes() lf = data.count(b"\n") crlf = data.count(b"\r\n") bare_lf = lf - crlf print(f"{p}: CRLF={crlf} LF={lf} bare_LF={bare_lf}") raise SystemExit(0 if bare_lf == 0 else 1) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Assets/Shader/build_nis.bat` at line 1, The batch file Assets/Shader/build_nis.bat currently contains LF-only line endings; convert the file to Windows CRLF line endings so there are no bare LF bytes (ensure every LF is part of a CRLF sequence), then re-save and commit the file; after converting, run the provided verification script (or check counts of b"\r\n" vs b"\n") to confirm bare_LF == 0 before merging.
🧹 Nitpick comments (1)
Streaming/VideoRenderer.cpp (1)
250-253: RedundantOMSetRenderTargetsbeforeCopySubresourceRegion.
CopySubresourceRegiondoes not require the destination to be bound as a render target, and binding it immediately before a copy can trigger D3D11 debug-layer hazard messages (the resource being bound as RTV while it is simultaneously the copy destination). The copy below only needs the underlying resource, not an active RTV binding.♻️ Proposed simplification
- if (finalSRV) { - // Now we need to render the final upscaled texture to the actual backbuffer - ID3D11RenderTargetView* backBufferRTV[] = { m_deviceResources->GetBackBufferRenderTargetView() }; - ctx->OMSetRenderTargets(1, backBufferRTV, nullptr); - - // For simplicity and to ensure it displays, if m_upscaler gives us an SRV, we can retrieve its texture - // and copy it to the backbuffer if dimensions match. - Microsoft::WRL::ComPtr<ID3D11Resource> upscaledRes; - finalSRV->GetResource(upscaledRes.ReleaseAndGetAddressOf()); - - Microsoft::WRL::ComPtr<ID3D11Texture2D> upscaledTex; - upscaledRes.As(&upscaledTex); - - if (upscaledTex) { - Microsoft::WRL::ComPtr<ID3D11Resource> backBufferRes; - backBufferRTV[0]->GetResource(backBufferRes.ReleaseAndGetAddressOf()); - - // Copy the upscaled texture to the backbuffer, using calculated offsets for centering - ctx->CopySubresourceRegion(backBufferRes.Get(), 0, fsrDst.x, fsrDst.y, 0, upscaledTex.Get(), 0, nullptr); - } - } + if (finalSRV) { + Microsoft::WRL::ComPtr<ID3D11Resource> upscaledRes; + finalSRV->GetResource(upscaledRes.ReleaseAndGetAddressOf()); + + Microsoft::WRL::ComPtr<ID3D11Resource> backBufferRes; + m_deviceResources->GetBackBufferRenderTargetView()->GetResource(backBufferRes.ReleaseAndGetAddressOf()); + + if (upscaledRes && backBufferRes) { + // Copy the upscaled texture to the backbuffer, using calculated offsets for centering + ctx->CopySubresourceRegion(backBufferRes.Get(), 0, fsrDst.x, fsrDst.y, 0, upscaledRes.Get(), 0, nullptr); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Streaming/VideoRenderer.cpp` around lines 250 - 253, The OMSetRenderTargets call binding backBufferRTV immediately before the CopySubresourceRegion is unnecessary and can cause D3D11 debug-layer hazards because the backbuffer is both an RTV and a copy destination; remove the ctx->OMSetRenderTargets(1, backBufferRTV, nullptr) call (the backBufferRTV local and the call referencing m_deviceResources->GetBackBufferRenderTargetView()) so the CopySubresourceRegion operates on the resource only, and only bind the RTV later if actual rendering to the backbuffer is required after the copy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Assets/Shader/build_nis.bat`:
- Around line 5-10: The batch file currently uses multiple lines like "fxc ...
|| exit /b 1" which bypass the final popd and endlocal on failure; change the
control flow to route failures to a cleanup label instead: replace each "|| exit
/b 1" with "|| goto :cleanup" (or check ERRORLEVEL and goto), add a ":cleanup"
label that runs popd and endlocal and then exits with the appropriate error code
(use %ERRORLEVEL% or set a variable before jumping), and ensure successful paths
also jump to the same :cleanup to centralize cleanup after the fxc invocations
so popd always executes.
---
Duplicate comments:
In `@Assets/Shader/build_nis.bat`:
- Line 1: The batch file Assets/Shader/build_nis.bat currently contains LF-only
line endings; convert the file to Windows CRLF line endings so there are no bare
LF bytes (ensure every LF is part of a CRLF sequence), then re-save and commit
the file; after converting, run the provided verification script (or check
counts of b"\r\n" vs b"\n") to confirm bare_LF == 0 before merging.
---
Nitpick comments:
In `@Streaming/VideoRenderer.cpp`:
- Around line 250-253: The OMSetRenderTargets call binding backBufferRTV
immediately before the CopySubresourceRegion is unnecessary and can cause D3D11
debug-layer hazards because the backbuffer is both an RTV and a copy
destination; remove the ctx->OMSetRenderTargets(1, backBufferRTV, nullptr) call
(the backBufferRTV local and the call referencing
m_deviceResources->GetBackBufferRenderTargetView()) so the CopySubresourceRegion
operates on the resource only, and only bind the RTV later if actual rendering
to the backbuffer is required after the copy.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e350d60-aed9-47a4-8c81-789eb27a922b
📒 Files selected for processing (2)
Assets/Shader/build_nis.batStreaming/VideoRenderer.cpp
Author
You’re right that people streaming to an Xbox will most likely target native resolution. VSR serves a different purpose. It’s not about replacing DLSS, but about compensating for network constraints. On mobile or laptops, this is a common use case due to variable network conditions and on-the-go usage. On Xbox, it’s less the case since it’s typically used on a stable home network, but it could still help in weaker Wi-Fi conditions. I implemented this feature on Xbox mostly to make it available across all platforms, but from a product owner’s point of view, it’s perfectly fine to exclude it if it doesn’t bring value to the user. |
Contributor
As another use-case. Could this be used to upscale a stream if the PC host can only play the game at a lower resolution? I assume you might have to force your stream to that lower resolution though for this to pick up. |
Author
Yes, it can be like this:
|
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
3 participants
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.
Video Super Resolution (VSR) for Moonlight Xbox
Video Super Resolution (VSR) brings to video what DLSS/FSR bring to real-time 3D rendering.
This PR introduces high-quality spatial upscaling on Xbox consoles and Windows 10/11 UWP devices using a highly optimized DirectX 11 pipeline powered by NVIDIA Image Scaling (NIS).
Xbox consoles and low-power UWP devices can greatly benefit from receiving a lower-resolution video stream (e.g., 720p or 1080p) while reconstructing a sharper final image locally on-device up to 4K.
This PR was made possible thanks to the foundational work previously done on adding VSR to the DirectX 11 pipeline of Moonlight-Qt (Ref https://github.com/linckosz/moonlight-qt/blob/DX11-Upscaler/app/streaming/video/ffmpeg-renderers/d3d11va.cpp#L1107).
Technical Overview
To integrate spatial upscaling cleanly within the existing DirectX 11 framework of moonlight-xbox, this PR introduces the
VideoUpscalerclass. This isolates the upscaling complexity and minimizes invasive changes to the core VideoRenderer.When VSR is enabled, the pipeline uses an efficient GPU-based two-pass structure that applies the upscaling shader before compositing the result into the backbuffer.
Testing
400% Zoom
Original
Video Super Resolution
Other VSR Implementations
VSR is also available in other Moonlight ports:
Windows, macOS, Linux: Add Video Super Resolution for Windows x64 (AMD, Intel and NVIDIA), Windows ARM (Snapdragon), Linux and MacOS moonlight-stream/moonlight-qt#1557
iOS and tvOS: Video Super Resolution for Moonlight iOS/tvOS moonlight-stream/moonlight-ios#704
Android: Add Video Super Resolution moonlight-stream/moonlight-android#1567
Summary by CodeRabbit
New Features
Chores