120fps frame pacing, async video decoder - #215
TheElixZammuto merged 22 commits into
Conversation
…nce. Try to make the locks a bit more granular.
…dProtected mode. Also removed the force tearing setting (it's the same as disabling vsync), and changed the modify frame queue to tweak the Pacer target.
…orrectly with Xbox so it's commented out.
…e in 'front-only' pacer mode
…resh rate. Rework pacing code around this system.
…erly. Use only hardware vsync when streaming at <= 60fps.
… 30/60. Remove commented out D3D locking code.
…not at 4K, clean out vsync setting, fixup clientRefreshRateX100 for non-NTSC.
…e. Misc minor fixes.
|
Warning Rate limit exceeded@andygrundman has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (24)
WalkthroughThis PR introduces a comprehensive frame pacing architecture alongside significant refactoring of GPU resource management, FFmpeg decoder initialization, and DirectX device configuration. Key changes include a new multi-threaded Pacer system for frame synchronization, GPU performance timing utilities, singleton-based FFmpeg decoder, and removal of legacy force-tearing controls in favor of dynamic VSYNC management. The implementation also updates device feature levels, adjusts UI settings for resolution/bitrate auto-calculation, and refines stats/plotting metrics. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Main Thread
participant Pacer as Pacer (Thread)
participant BackPacer as BackPacer (Thread)
participant Decoder as FFMpegDecoder
participant Render as VideoRenderer
participant Device as DeviceResources
Main->>Pacer: init(res, fps, refreshRate)
Pacer->>Pacer: Start vsync & backPacer threads
Decoder->>Pacer: submitFrame(AVFrame)
Pacer->>Pacer: Enqueue to pacing queue
BackPacer->>Device: waitForVBlank()
Device-->>BackPacer: VSYNC signal
BackPacer->>Pacer: handleVsync(deadline)
Pacer->>Pacer: Apply frame drop logic, move to render queue
Main->>Pacer: waitForFrame()
Pacer-->>Main: Frame ready (or wait)
Main->>Pacer: renderOnMainThread(sceneRenderer)
Pacer->>Pacer: frontPacer() - timing/drop checks
Pacer->>Render: Process frame from render queue
Render->>Device: Present() via D3D
Device-->>Render: Present complete
Pacer->>Pacer: afterPresent(presentTime) - drift tracking
Pacer-->>Main: render() complete
Main->>Pacer: deinit()
Pacer->>Pacer: Stop threads, drain queues, free resources
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Streaming/LogRenderer.cpp (1)
64-74: Consider adapting warning console font size to display resolution.The main console adapts its font size based on display height (12pt for ≤1440p, 24pt for 4K), but the warning console always uses 24pt. This creates a visual inconsistency where the warning text may appear disproportionately large on lower-resolution displays.
const wchar_t* font = L"Assets\\Font\\ModeSeven-24.spritefont"; // sized for 4K + const wchar_t* warningFont = font; if (m_displayHeight <= 1440) { font = L"Assets\\Font\\ModeSeven-12.spritefont"; // for 1080p & 1440p + warningFont = font; } m_console->RestoreDevice(m_deviceResources->GetD3DDeviceContext(), font); - m_warningConsole->RestoreDevice(m_deviceResources->GetD3DDeviceContext(), L"Assets\\Font\\ModeSeven-24.spritefont"); + m_warningConsole->RestoreDevice(m_deviceResources->GetD3DDeviceContext(), warningFont);Streaming/moonlight_xbox_dxMain.cpp (1)
270-279: Wrong key released: Menu maps to Enter, but KeyUp uses RightThe release path should send KeyUp(Enter) to mirror KeyDown(Enter).
Apply this diff:
- else if (isPressed(previousReading[i].Buttons, GamepadButtons::Menu)) { - moonlightClient->KeyUp((unsigned short)Windows::System::VirtualKey::Right, 0); - } + else if (isPressed(previousReading[i].Buttons, GamepadButtons::Menu)) { + moonlightClient->KeyUp((unsigned short)Windows::System::VirtualKey::Enter, 0); + }
🧹 Nitpick comments (35)
Common/TextConsole.cpp (1)
26-26: Consider removing unused namespace declaration.The
moonlight_xbox_dxnamespace is declared but no symbols from it are used in this file. Consider removing it to keep the code clean.-using namespace moonlight_xbox_dx;Streaming/LogRenderer.cpp (2)
33-46: Optimize static warning message updates.The warning message is rewritten every second, even though it's static content. Consider writing it once when logs become visible (in
ToggleVisible) instead of every update cycle.Modify the
Updatemethod to only clear and update the main console:if (m_visible && timer.GetTotalSeconds() - lastUpdateSeconds >= 1.0) { m_console->Clear(); - m_warningConsole->Clear(); Utils::logMutex.lock(); std::vector<std::wstring> lines = Utils::GetLogLines(); for (std::wstring line : lines) { m_console->Write(line.c_str()); } Utils::logMutex.unlock(); - m_warningConsole->Write(L"Warning: Viewing logs reduces performance"); - lastUpdateSeconds = timer.GetTotalSeconds(); }Then write the warning message once in
ToggleVisible:void LogRenderer::ToggleVisible() { std::lock_guard<std::mutex> lock(m_mutex); m_visible = !m_visible; if (m_visible) { m_warningConsole->Clear(); m_warningConsole->Write(L"Warning: Viewing logs reduces performance"); } }
77-87: Consider documenting or extracting magic numbers.The hardcoded values
44and20appear to represent vertical margins or spacing but are not documented. Consider adding comments or extracting them as named constants to improve readability.+ // Reserve bottom area for warning console: 44px from bottom minus 20px margin + constexpr int CONSOLE_BOTTOM_MARGIN = 44; + constexpr int WARNING_TOP_MARGIN = 20; + // The size of our text area (left, top, right, bottom) - RECT size = {m_displayWidth * 0.5, 0, m_displayWidth - 20, m_displayHeight - 44}; + RECT size = {m_displayWidth * 0.5, 0, m_displayWidth - WARNING_TOP_MARGIN, m_displayHeight - CONSOLE_BOTTOM_MARGIN}; m_console->SetWindow(size); // The size of our text area (left, top, right, bottom) - RECT warningSize = {size.left, m_displayHeight - 44, size.right, m_displayHeight - 20}; + RECT warningSize = {size.left, m_displayHeight - CONSOLE_BOTTOM_MARGIN, size.right, m_displayHeight - WARNING_TOP_MARGIN}; m_warningConsole->SetWindow(warningSize);Pages/HostSettingsPage.xaml.h (1)
106-106: Make getDefaultBitrate static/const and align namingIt doesn’t use instance state. Consider:
- Make it static (or at least const).
- Rename to GetDefaultBitrate for consistency with surrounding PascalCase methods.
- int getDefaultBitrate(int width, int height, int fps); + static int GetDefaultBitrate(int width, int height, int fps) noexcept;.clang-format (1)
5-5: Format policy change: ensure repo-wide consistencyBrace and constructor-initializer rules changed. Re-run formatting across the tree to avoid mixed styles in future diffs.
Please run your usual clang-format step locally and commit any resultant changes.
Also applies to: 15-16
Streaming/PacerRational.h (1)
5-10: Tighten includes and guard MSVC intrinsics_immintrin.h isn’t used here; keep <intrin.h>. Also guard MSVC-specific intrinsics to avoid toolchain issues.
-#include <immintrin.h> -#include <intrin.h> -#pragma intrinsic(_umul128, _udiv128) +#include <intrin.h> +#if defined(_MSC_VER) +#pragma intrinsic(_umul128, _udiv128) +#endif.gitmodules (1)
3-3: Submodule is properly pinned; document fork rationaleThe submodule commit is pinned (
4372ab392c3532adabc3b087180d79cd011b762f), which mitigates supply-chain risk. However, using a personal fork introduces an availability risk. Consider:
- Adding a comment in
.gitmodulesor the PR description explaining why the personal fork is necessary instead of the upstream- Optionally setting up a mirror or sync mechanism to safeguard against fork unavailability
Utils/FloatBuffer.cpp (1)
56-83: Make narrowing conversion explicitThe function returns
intbutoutLenisstd::size_t, causing an implicit narrowing conversion at line 83. Whilecapacity_is validated as a power of two, there is no upper bound check to guarantee it fits inint. Make this explicit and document the assumption.- return outLen; + // outLen ≤ capacity_; capacity_ is a power of two validated at construction. + return static_cast<int>(outLen);Optional: add defensive assertions.
+ assert(outBuffer != nullptr); + assert(outSize > 0);Streaming/VideoRenderer.h (2)
33-35: Minimize public API surface: make EnsureYuvTargets private.It looks like an internal helper (called from Render). Keeping it public needlessly expands the surface area. Move it to private unless external callers need it.
- void EnsureYuvTargets(ID3D11Device* dev, DXGI_FORMAT fmt, UINT w, UINT h); +private: + void EnsureYuvTargets(ID3D11Device* dev, DXGI_FORMAT fmt, UINT w, UINT h); +public:
57-62: YUV SRVs state — good, but ensure header includes for types.Members look correct for NV12/P010 sampling. Since this header uses AVFrame and AVColorTransferCharacteristic, explicitly include avutil headers to guarantee definitions in all TU’s and speed up builds:
extern "C" { -#include <libavcodec/avcodec.h> +#include <libavcodec/avcodec.h> +#include <libavutil/frame.h> +#include <libavutil/pixfmt.h> }Pages/HostSettingsPage.xaml.cpp (3)
123-131: Resolution change: also compare height.You only compare width before recomputing bitrate. Consider width or height change.
- if (selectedResolution->Width != host->Resolution->Width) { + if (selectedResolution->Width != host->Resolution->Width || + selectedResolution->Height != host->Resolution->Height) {
180-180: Minor: trailing whitespace/blank lines.Nit: stray trailing markers after unsubscribe; no behavior change.
182-229: Include math header for sqrtf/lround and document units.getDefaultBitrate uses std::sqrtf and std::lround. Ensure is included (via pch or here) and consider a brief comment clarifying return units (kbps).
-#include "Utils.hpp" +#include "Utils.hpp" +#include <cmath> // sqrtf, lroundOptionally rename to getDefaultBitrateKbps or document that multiplier 1000 yields kbps.
Plot/PlotDesc.h (1)
35-45: Guard enum/table drift at compile timekPlotDescs must always match PlotType. Add a static_assert to catch future mismatches at build time.
Apply this diff just below the array:
inline constexpr std::array<PlotDesc, PlotCount> kPlotDescs = {{ {"Frametime", PLOT_LABEL_MIN_MAX_AVG, "ms", -0.1f, 50.0f, 0.0f, 41.0f}, {"Host Frametime", PLOT_LABEL_MIN_MAX_AVG, "ms", -0.1f, 50.0f, 0.0f, 41.0f}, {"Vsync interval", PLOT_LABEL_MIN_MAX_AVG, "ms", -0.1f, 50.0f, 0.0f, 0.0f}, {"Dropped frames (network)", PLOT_LABEL_TOTAL_INT, "", -1.0f, 3.0f, 0.0f, 0.0f}, {"Dropped frames (pacing)", PLOT_LABEL_TOTAL_INT, "", -1.0f, 3.0f, 0.0f, 0.0f}, {"Present to display latency", PLOT_LABEL_MIN_MAX_AVG, "ms", -10.0f, 25.0f, 0.0f, 0.0f}, {"Graph overhead", PLOT_LABEL_MIN_MAX_AVG, "ms", -0.1f, 6.0f, 0.0f, 0.0f}, {"Etc...", PLOT_LABEL_MIN_MAX_AVG, "ms", -0.1f, 50.0f, 0.0f, 49.9f}, }}; + +static_assert(kPlotDescs.size() == PlotCount, "Plot descriptors out of sync with PlotType enum");Streaming/moonlight_xbox_dxMain.cpp (2)
145-149: 2 kHz comment vs 2 ms period mismatch2000 microseconds = 0.002s = 500 Hz. Either update the comment to 500 Hz or change period to 500 microseconds for 2 kHz.
Option A (fix comment):
- // Target period = 2000hz + // Target period = 500 Hz (2000 us)Option B (keep 2 kHz):
- const auto period = std::chrono::microseconds(2000); + const auto period = std::chrono::microseconds(500);
1-10: Ensure Pacer header is included explicitlyThis file uses Pacer::instance() but doesn’t include Pacer.h here. If not pulled via another header, add the include to avoid fragile transitive dependencies.
#include <Pages/StreamPage.xaml.h> +#include <Streaming\Pacer.h> #include <Streaming\FFMpegDecoder.h>State/MoonlightClient.cpp (1)
222-245: Round clientRefreshRateX100 to nearest integer(int)(rr*100.0) can under/overflow by truncation (e.g., 59.94→5993). Use lround to get 5994 reliably.
Apply this diff (add if not already included):
- case 120: - config.clientRefreshRateX100 = (int)(rr * 100.0); + case 120: + config.clientRefreshRateX100 = (int)std::lround(rr * 100.0); break; @@ - } else { - config.clientRefreshRateX100 = (int)(rr * 100.0); + } else { + config.clientRefreshRateX100 = (int)std::lround(rr * 100.0); }And near the includes:
#include <atomic> +#include <cmath>Streaming/StatsRenderer.cpp (2)
66-88: Minor: avoid unused variable warningImGuiIO& io is unused; either remove or mark unused.
- ImGuiIO &io = ImGui::GetIO(); + (void)ImGui::GetIO();
70-79: Optional: prefer RAII over raw malloc for buffersUse std::array<float, 512> or std::vector to avoid manual allocation and ease lifetime management.
Common/DeviceResources.h (2)
90-92: Manage HANDLE lifetime with RAII to prevent leaks across swap-chain recreation.Wrap m_frameLatencyWaitable in a smart handle (e.g., wil::unique_handle) and close before creating a new swap chain.
- HANDLE m_frameLatencyWaitable; + wil::unique_handle m_frameLatencyWaitable;And reset appropriately when (re)creating the swap chain and on HandleDeviceLost().
34-34: Consider const correctness for isXbox().Method doesn’t mutate state; mark it const to improve API clarity.
- bool isXbox(); + bool isXbox() const;State/Stats.cpp (2)
94-100: Make RTP delta wrap-safe for long sessions.lastHostPts is uint32_t and will wrap ~13.3 hours. Compute delta in uint32_t to leverage modular arithmetic.
- static uint32_t lastHostPts = 0; - if (lastHostPts > 0) { - ImGuiPlots::instance().observeFloat(PLOT_HOST_FRAMETIME, (float)((decodeUnit->rtpTimestamp - lastHostPts) / 90.0f)); - } - lastHostPts = decodeUnit->rtpTimestamp; + static uint32_t lastHostPts = 0; + if (lastHostPts != 0) { + const uint32_t delta90k = (uint32_t)(decodeUnit->rtpTimestamp - lastHostPts); // wrap-safe + ImGuiPlots::instance().observeFloat(PLOT_HOST_FRAMETIME, (float)(delta90k / 90.0f)); + } + lastHostPts = (uint32_t)decodeUnit->rtpTimestamp;
262-275: Fix snprintf bound check type-safety.ret is int; length - offset is size_t. Cast to avoid signed/unsigned pitfalls.
- if (ret < 0 || ret >= length - offset) { + if (ret < 0 || (size_t)ret >= (length - offset)) { Utils::Log("Error: stringifyVideoStats length overflow\n"); return; }Repeat for similar checks below.
Streaming/Pacer.h (1)
43-44: Clarify waitForVBlank preconditions and stop behavior.IDXGIOutput::WaitForVBlank blocks until next vblank; it can’t be interrupted by m_Stopping. Document that deinit() will only return after the next vblank and ensure m_dxgiOutput remains valid during teardown.
- Add a comment to waitForVBlank() with the above contract.
- Optionally add a fast-path using FrameLatencyWaitable when DXGIOutput is unavailable.
Common/DirectXHelper.cpp (1)
35-45: Avoid busy-waiting + accidental pipeline flush; bound polling and skip NaNs in avg.
- Use D3D11_ASYNC_GETDATA_DONOTFLUSH to prevent implicit flushes.
- Poll with a small spin/yield or bail if not ready.
- When computing average, ignore NaNs.
- while (context->GetData(m_disjointQuery[ReadQuery].Get(), &disjointData, sizeof(disjointData), 0) != S_OK); + while (context->GetData(m_disjointQuery[ReadQuery].Get(), &disjointData, sizeof(disjointData), D3D11_ASYNC_GETDATA_DONOTFLUSH) == S_FALSE) { /* brief spin or yield */ } - while (context->GetData(m_startTimestampQuery[ReadQuery].Get(), &start, sizeof(start), 0) != S_OK); + while (context->GetData(m_startTimestampQuery[ReadQuery].Get(), &start, sizeof(start), D3D11_ASYNC_GETDATA_DONOTFLUSH) == S_FALSE) { /* brief spin or yield */ } - while (context->GetData(m_endTimestampQuery[ReadQuery].Get(), &end, sizeof(end), 0) != S_OK); + while (context->GetData(m_endTimestampQuery[ReadQuery].Get(), &end, sizeof(end), D3D11_ASYNC_GETDATA_DONOTFLUSH) == S_FALSE) { /* brief spin or yield */ } @@ - m_processingTimeHistory[m_processingTimeHistoryIndex] = m_processingTimeMs; + m_processingTimeHistory[m_processingTimeHistoryIndex] = m_processingTimeMs; m_processingTimeHistoryIndex = (m_processingTimeHistoryIndex + 1) % TimeHistoryCount; - - m_processingTimeAvgMs = std::accumulate(m_processingTimeHistory, m_processingTimeHistory + TimeHistoryCount, 0.0f) / static_cast<float>(TimeHistoryCount); + // Compute avg ignoring NaNs + float sum = 0.0f; int cnt = 0; + for (float v : m_processingTimeHistory) { if (!std::isnan(v)) { sum += v; ++cnt; } } + m_processingTimeAvgMs = (cnt > 0) ? (sum / cnt) : std::numeric_limits<float>::quiet_NaN();Also applies to: 67-71
Common/DeviceResources.cpp (2)
468-471: SetVsync() doesn’t recreate swap chain; Present() recovers via device-lost.Works but is heavy. Consider recreating the swapchain when m_enableVsync changes to avoid a device-loss cycle and logs.
611-631: Consolidate Xbox detection helpers.You have both is_running_on_xbox() and DeviceResources::isXbox(). Keep one and reuse to avoid divergence.
Also applies to: 633-658
Streaming/VideoRenderer.cpp (3)
511-517: 10‑bit offsets applied only for Rec.2020.If hosts send 10‑bit SDR Rec.709, k_Offsets_10bit_* may also be needed. If that’s not expected in practice, ignore; otherwise add a 10‑bit path for 709.
542-561: Use RAII for decoder mutex in SetHDR().Replace manual lock/unlock with scoped_lock to avoid accidental leaks on early returns/exceptions.
- FFMpegDecoder::instance().mutex.lock(); + std::scoped_lock<std::recursive_mutex> lk(FFMpegDecoder::instance().mutex); ... - FFMpegDecoder::instance().mutex.unlock();
144-146: Remove unused global.renderedOneFrame is not used.
-bool renderedOneFrame = false;Streaming/FFmpegDecoder.h (2)
39-41: Public recursive_mutex is a sharp edge.Prefer encapsulating internal synchronization or exposing narrower ops that perform their own locking. If external locking remains, document invariants.
6-15: Include cycle claim is incorrect; VideoRenderer.h include appears unused in header.No include cycle exists—VideoRenderer.h does not include FFmpegDecoder.h. Additionally, FFmpegDecoder class does not use any VideoRenderer types or instantiate VideoRenderer in the header, making the
#include "VideoRenderer.h"include potentially unnecessary.However, the suggested forward declarations for FFmpeg types won't work as written: types like
AVCodec,AVCodecContext,AVHWDeviceContext, andAVD3D11VADeviceContextare used as member pointers in the class (requiring full type definitions). Only types used in function signatures (not member variables) can be forward-declared.If VideoRenderer.h is only used in the .cpp file, moving it there is valid. Otherwise, verify its actual usage before refactoring.
Streaming/FFmpegDecoder.cpp (2)
162-169: Multithread protection: check QueryInterface failure path.If QueryInterface fails, log once so we know we’re running without MT-protection.
- if (SUCCEEDED(hr)) { + if (SUCCEEDED(hr)) { pMultithread->SetMultithreadProtected(TRUE); pMultithread->Release(); } + else { + Utils::Log("Warning: ID3D11Multithread not available; proceeding without SetMultithreadProtected.\n"); + }
298-301: Global ffmpeg log level.Dropping to AV_LOG_QUIET globally may hinder troubleshooting. Consider AV_LOG_ERROR after first success instead of QUIET.
- if (av_log_get_level() > 0) { - av_log_set_level(AV_LOG_QUIET); - } + if (av_log_get_level() > AV_LOG_ERROR) { + av_log_set_level(AV_LOG_ERROR); + }Streaming/Pacer.cpp (1)
133-161: Initialize and log with explicit newlines, minor nit.Log in init prints without newline; add for consistency.
- Utils::Logf("Pacer: init target %.2f Hz with %d FPS stream", + Utils::Logf("Pacer: init target %.2f Hz with %d FPS stream\n", m_RefreshRate, m_StreamFps);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Assets/Shader/d3d11_bt2020lim_pixel.hlslis excluded by!**/*.hlsl
📒 Files selected for processing (42)
.clang-format(2 hunks).gitmodules(1 hunks)Common/DeviceResources.cpp(14 hunks)Common/DeviceResources.h(3 hunks)Common/DirectXHelper.cpp(1 hunks)Common/DirectXHelper.h(1 hunks)Common/TextConsole.cpp(2 hunks)Pages/AppPage.xaml.cpp(0 hunks)Pages/HostSettingsPage.xaml(2 hunks)Pages/HostSettingsPage.xaml.cpp(4 hunks)Pages/HostSettingsPage.xaml.h(1 hunks)Pages/StreamPage.xaml(0 hunks)Pages/StreamPage.xaml.cpp(1 hunks)Pages/StreamPage.xaml.h(0 hunks)Plot/ImGuiPlots.cpp(1 hunks)Plot/PlotDesc.h(2 hunks)State/ApplicationState.cpp(0 hunks)State/MoonlightClient.cpp(2 hunks)State/MoonlightHost.h(0 hunks)State/Stats.cpp(9 hunks)State/Stats.h(2 hunks)State/StreamConfiguration.h(0 hunks)Streaming/FFmpegDecoder.cpp(4 hunks)Streaming/FFmpegDecoder.h(1 hunks)Streaming/LogRenderer.cpp(5 hunks)Streaming/LogRenderer.h(1 hunks)Streaming/Pacer.cpp(1 hunks)Streaming/Pacer.h(1 hunks)Streaming/PacerRational.h(1 hunks)Streaming/StatsRenderer.cpp(8 hunks)Streaming/VideoRenderer.cpp(9 hunks)Streaming/VideoRenderer.h(3 hunks)Streaming/moonlight_xbox_dxMain.cpp(6 hunks)Streaming/moonlight_xbox_dxMain.h(2 hunks)Utils/FloatBuffer.cpp(1 hunks)Utils/FloatBuffer.h(1 hunks)moonlight-xbox-dx.vcxproj(4 hunks)moonlight-xbox-dx.vcxproj.filters(3 hunks)pch.h(3 hunks)third_party/moonlight-common-c(1 hunks)vcpkg(1 hunks)vcpkg.json(1 hunks)
💤 Files with no reviewable changes (6)
- State/StreamConfiguration.h
- Pages/StreamPage.xaml.h
- Pages/StreamPage.xaml
- State/MoonlightHost.h
- State/ApplicationState.cpp
- Pages/AppPage.xaml.cpp
🧰 Additional context used
🧬 Code graph analysis (21)
Utils/FloatBuffer.h (1)
Utils/FloatBuffer.cpp (2)
copyInto(56-83)copyInto(56-56)
Streaming/LogRenderer.cpp (2)
Streaming/moonlight_xbox_dxMain.cpp (2)
CreateWindowSizeDependentResources(93-98)CreateWindowSizeDependentResources(93-93)Streaming/StatsRenderer.cpp (4)
CreateWindowSizeDependentResources(193-214)CreateWindowSizeDependentResources(193-193)ReleaseDeviceDependentResources(216-218)ReleaseDeviceDependentResources(216-216)
State/MoonlightClient.cpp (3)
Utils.cpp (2)
Logf(90-99)Logf(90-90)Streaming/FFmpegDecoder.cpp (4)
instance(42-45)instance(42-42)getDecoder(319-328)getDecoder(319-319)Streaming/AudioPlayer.cpp (2)
getDecoder(49-60)getDecoder(49-49)
State/Stats.h (1)
State/Stats.cpp (4)
SubmitPacerTime(117-125)SubmitPacerTime(117-117)SubmitPresentPacing(128-131)SubmitPresentPacing(128-128)
Pages/HostSettingsPage.xaml.h (1)
Pages/HostSettingsPage.xaml.cpp (2)
getDefaultBitrate(182-229)getDefaultBitrate(182-182)
Streaming/VideoRenderer.h (1)
Streaming/VideoRenderer.cpp (4)
EnsureYuvTargets(106-142)EnsureYuvTargets(106-106)Render(146-211)Render(146-146)
Streaming/LogRenderer.h (2)
Common/TextConsole.h (2)
DX(25-88)TextConsole(27-87)Common/TextConsole.cpp (2)
TextConsole(28-36)TextConsole(40-40)
Streaming/Pacer.h (1)
Streaming/Pacer.cpp (33)
Pacer(80-91)instance(75-78)instance(75-75)deinit(93-131)deinit(93-93)init(133-161)init(133-133)waitForFrame(508-527)waitForFrame(508-508)renderOnMainThread(530-549)renderOnMainThread(530-530)waitUntilPresentTarget(622-628)waitUntilPresentTarget(622-622)afterPresent(631-650)afterPresent(631-631)submitFrame(674-685)submitFrame(674-674)vsyncEmulator(183-278)vsyncEmulator(183-183)vsyncHardware(280-359)vsyncHardware(280-280)backPacer(361-395)backPacer(361-361)handleVsync(397-493)handleVsync(397-397)enqueueFrameForRenderingAndUnlock(495-504)enqueueFrameForRenderingAndUnlock(495-495)frontPacer(551-619)frontPacer(551-551)dropFrameForEnqueue(654-672)dropFrameForEnqueue(654-654)waitForVBlank(689-692)waitForVBlank(689-689)
Streaming/StatsRenderer.cpp (4)
Streaming/LogRenderer.cpp (8)
CreateDeviceDependentResources(60-75)CreateDeviceDependentResources(60-60)Render(51-58)Render(51-51)CreateWindowSizeDependentResources(77-87)CreateWindowSizeDependentResources(77-77)ToggleVisible(95-98)ToggleVisible(95-95)Common/TextConsole.cpp (2)
Render(54-85)Render(54-54)pch.h (2)
QpcNow(55-59)QpcToMs(82-84)Plot/ImGuiPlots.cpp (2)
instance(14-18)instance(14-14)
Common/DeviceResources.cpp (3)
Common/DirectXHelper.h (2)
ThrowIfFailed(8-19)ThrowIfFailed(21-32)Utils.cpp (2)
Logf(90-99)Logf(90-90)Utils.hpp (1)
Logf(15-15)
Streaming/VideoRenderer.cpp (3)
Common/DirectXHelper.h (2)
ThrowIfFailed(8-19)ThrowIfFailed(21-32)Utils.cpp (6)
Logf(90-99)Logf(90-90)Log(57-82)Log(57-57)Log(84-88)Log(84-84)Streaming/FFmpegDecoder.cpp (2)
instance(42-45)instance(42-42)
Common/TextConsole.cpp (2)
Utils.cpp (2)
Logf(90-99)Logf(90-90)Utils.hpp (1)
Logf(15-15)
Common/DirectXHelper.cpp (1)
Common/DirectXHelper.h (1)
GpuPerformanceTimer(85-120)
Streaming/Pacer.cpp (6)
State/Stats.h (1)
moonlight_xbox_dx(47-87)Streaming/VideoRenderer.h (1)
moonlight_xbox_dx(12-75)Plot/ImGuiPlots.cpp (2)
instance(14-18)instance(14-14)Streaming/Pacer.h (1)
Pacer(14-68)pch.h (7)
f(47-51)QpcFreq(46-53)UsToQpc(61-65)QpcNow(55-59)QpcToMs(82-84)QpcToUs(67-76)MsToQpc(86-89)Streaming/PacerRational.h (1)
hzNum(13-48)
Common/DeviceResources.h (1)
Common/DeviceResources.cpp (14)
SetVsync(468-471)SetVsync(468-468)ValidateDevice(474-523)ValidateDevice(474-474)HandleDeviceLost(526-546)HandleDeviceLost(526-526)Present(565-609)Present(565-565)GetUWPPixelDimensions(660-681)GetUWPPixelDimensions(660-660)GetUWPRefreshRate(683-700)GetUWPRefreshRate(683-683)isXbox(619-630)isXbox(619-619)
Common/DirectXHelper.h (4)
Common/DeviceResources.cpp (1)
DeviceResources(59-86)Common/DeviceResources.h (2)
DeviceResources(16-135)DX(6-136)Common/DirectXHelper.cpp (13)
GpuPerformanceTimer(8-23)StartTimerForFrame(25-77)StartTimerForFrame(25-25)EndTimerForFrame(79-84)EndTimerForFrame(79-79)GetFrameTime(86-89)GetFrameTime(86-86)GetAvgFrameTime(91-94)GetAvgFrameTime(91-91)GetMinFrameTime(96-99)GetMinFrameTime(96-96)GetMaxFrameTime(101-104)GetMaxFrameTime(101-101)Streaming/moonlight_xbox_dxMain.h (1)
DX(9-11)
Streaming/FFmpegDecoder.cpp (4)
State/Stats.h (1)
moonlight_xbox_dx(47-87)Streaming/FFmpegDecoder.h (2)
moonlight_xbox_dx(26-57)FFMpegDecoder(28-56)Streaming/Pacer.cpp (2)
instance(75-78)instance(75-75)Utils.cpp (6)
Logf(90-99)Logf(90-90)Log(57-82)Log(57-57)Log(84-88)Log(84-84)
Streaming/FFmpegDecoder.h (3)
Streaming/FFmpegDecoder.cpp (13)
FFMpegDecoder(47-59)instance(42-45)instance(42-42)CompleteInitialization(78-81)CompleteInitialization(78-78)Init(83-181)Init(83-83)Cleanup(183-195)Cleanup(183-183)SubmitDecodeUnit(216-304)SubmitDecodeUnit(216-216)getDecoder(319-328)getDecoder(319-319)Streaming/Pacer.cpp (2)
instance(75-78)instance(75-75)Common/DeviceResources.h (2)
DX(6-136)DeviceResources(16-135)
Streaming/moonlight_xbox_dxMain.cpp (5)
Utils.cpp (2)
Logf(90-99)Logf(90-90)Streaming/VideoRenderer.cpp (2)
Render(146-211)Render(146-146)Streaming/Pacer.cpp (4)
instance(75-78)instance(75-75)afterPresent(631-650)afterPresent(631-631)Plot/ImGuiPlots.cpp (2)
instance(14-18)instance(14-14)pch.h (2)
QpcNow(55-59)QpcToMs(82-84)
Streaming/moonlight_xbox_dxMain.h (3)
Common/DirectXHelper.h (2)
DX(6-121)GpuPerformanceTimer(85-120)Common/DirectXHelper.cpp (1)
GpuPerformanceTimer(8-23)Streaming/VideoRenderer.h (1)
VideoRenderer(23-74)
State/Stats.cpp (4)
Plot/ImGuiPlots.cpp (2)
instance(14-18)instance(14-14)Streaming/FFmpegDecoder.cpp (2)
instance(42-45)instance(42-42)pch.h (1)
QpcToUs(67-76)Utils.cpp (4)
Log(57-82)Log(57-57)Log(84-88)Log(84-84)
🪛 Clang (14.0.6)
Streaming/PacerRational.h
[error] 5-5: 'cstdint' file not found
(clang-diagnostic-error)
Streaming/Pacer.h
[error] 3-3: 'atomic' file not found
(clang-diagnostic-error)
Common/DirectXHelper.cpp
[error] 1-1: 'pch.h' file not found
(clang-diagnostic-error)
Streaming/Pacer.cpp
[error] 2-2: 'pch.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (32)
Streaming/LogRenderer.h (1)
26-26: LGTM!The addition of
m_warningConsoleas astd::unique_ptrfollows the same pattern as the existingm_consolemember and ensures proper resource management.Streaming/LogRenderer.cpp (3)
13-20: LGTM!The warning console initialization is properly implemented with appropriate color settings for warnings.
54-57: LGTM!Both consoles are properly rendered when visible.
89-93: LGTM!Both consoles properly release their device resources during cleanup.
vcpkg (1)
1-1: Submodule pointer correctly aligns with vcpkg.json FFmpeg pinning.Verification confirms the vcpkg submodule pointer update to
120deac3062162151622ca4860575a33844ba10bis intentional and properly configured. Thevcpkg.jsonfile explicitly sets this same commit hash as itsbuiltin-baselineand pins FFmpeg to version 7.1.1 in the overrides section. The changes are correctly coordinated.third_party/moonlight-common-c (1)
1-1: Verify the rationale for the submodule fork and URL change.The submodule pointer is being updated to a fork (
andygrundman/moonlight-common-c) instead of the official upstream. This introduces a maintenance dependency on an external fork.Please clarify:
- What specific changes in the fork are required for this PR's frame pacing and decoder optimizations?
- Is this fork temporary or intended to be long-term?
- Are there plans to upstream these changes to the official repository?
- Will other developers working on this codebase need to maintain this fork, or will it be merged back to upstream?
Understanding these details will help assess the maintainability and long-term implications of this change.
Streaming/PacerRational.h (1)
37-48: Step logic looks solidFractional carry via accum/den is correct for 59.94/119.88 style rates. No changes requested.
Pages/HostSettingsPage.xaml.h (1)
99-99: FPS selector handler wiring confirmed; no manual unsubscription requiredThe handler is correctly wired in XAML (Pages/HostSettingsPage.xaml:56). In UWP, XAML-wired event handlers are managed by the framework and don't require explicit unsubscription since the event source (FPSSelector ComboBox) is destroyed with the page. No handler leak risk.
moonlight-xbox-dx.vcxproj.filters (2)
383-388: New sources added under correct filter — LGTM.Streaming\Pacer.cpp and Common\DirectXHelper.cpp are properly placed under “Source Files”.
472-480: Header additions wired correctly.Pacer.h, PacerCompat.h, and PacerRational.h are properly included under “Header Files”.
Pages/HostSettingsPage.xaml (3)
56-56: One-way SelectedItem + handler is fine here.Binding SelectedItem OneWay and using SelectionChanged to update Host.FPS is acceptable given you recompute bitrate first.
Ensure Host.FPS updates programmatically still reflect in the ComboBox as expected.
81-84: Hiding V-Sync controls — comment OK.Commenting out the V-Sync UI with a clarifying note is fine given automatic handling.
85-99: Stats/graphs bindings — LGTM.Graphs checkbox correctly gated by EnableStats and bound TwoWay.
Streaming/VideoRenderer.h (2)
8-10: Correct C linkage for FFmpeg headers — LGTM.Including libavcodec under extern "C" avoids mangling issues.
69-70: Atomic loading flag — LGTM.Switch to std::atomic is appropriate; use acquire/release as you did in cpp.
Pages/HostSettingsPage.xaml.cpp (1)
81-87: Codec gating for Xbox One — LGTM.Disabling codec selection on OG Xbox One is a sensible UX constraint.
vcpkg.json (2)
3-3: ****The CI workflow (.github/workflows/msbuild.yml) downloads pre-built packages from a release archive and does not use vcpkg's manifest resolution system. The
builtin-baselinein vcpkg.json is not referenced or used by CI, so baseline alignment with the CI vcpkg instance is not a concern for this build pipeline. Ifbuiltin-baselineis intended for local development, it's orthogonal to the CI configuration shown here.Likely an incorrect or invalid review comment.
25-29: No issues found — FFmpeg 7.1.1 is available for x64-uwp.vcpkg includes FFmpeg v7.1.1 and supports installation for the x64-uwp triplet. The version pinning in vcpkg.json is safe and properly supported.
Streaming/moonlight_xbox_dxMain.h (2)
9-11: Forward declaration OKMinimizes header coupling; no issues.
52-52: No actionable issues found—original review comment is unfounded.The verification confirms a single owner (
moonlight_xbox_dxMainviamake_shared), no stored references inPacer, and no circular dependencies.Pacerreceives theshared_ptrby reference but does not store it, andVideoRenderercontains no back-references. While passingshared_ptrby reference to temporary consumers is unconventional style, it poses no correctness risk—no lifetime extension, no cycles, no hidden multiple owners.Likely an incorrect or invalid review comment.
Pages/StreamPage.xaml.cpp (1)
245-249: BackRequested unsubscription on unloadGood cleanup; prevents handler leaks.
State/Stats.h (1)
35-38: Review comment is incorrect—zero-initialization and aggregation are already implementedThe codebase already properly handles all concerns raised:
- Zero-initialization: Constructor at Stats.cpp:9 uses
ZeroMemory()to initializem_ActiveWndVideoStats,m_LastWndVideoStats, andm_GlobalVideoStats- Aggregation: Both
totalPacerTimeUs(line 144) andtotalPresentDisplayMs(line 146) are already aggregated inaddVideoStats()- Formatting: Both fields are already used in
formatVideoStats()(lines 349-350)No action is needed; the code is correct as-is.
Likely an incorrect or invalid review comment.
Plot/ImGuiPlots.cpp (1)
20-31: Initializer and array definitions verified—code is correctAll three new plots (PLOT_VSYNC_INTERVAL, PLOT_PRESENT_PACING, PLOT_ETC) are properly defined in the enum, have matching entries in kPlotDescs (size = PlotCount = 8), and the initializer correctly uses all 8 plots without out-of-range indexing.
moonlight-xbox-dx.vcxproj (4)
260-261: Confirm Release PDB intentGenerateDebugInformation=true in Release increases package size; ensure Store packaging and perf goals are OK. If not required on retail builds, consider a dedicated “Symbols” config instead.
348-351: Good: Pacer headers included in projectHeader adds align with new pacing system; no action needed.
386-387: Good: Adds DirectXHelper.cpp to buildRequired for GpuPerformanceTimer and helper APIs referenced elsewhere.
485-486: Good: Adds Pacer.cpp to buildEnsures singleton and pacing logic is linked.
State/MoonlightClient.cpp (1)
294-296: Decoder init path LGTMSingleton init + getDecoder usage aligns with the new async decoder design.
Streaming/StatsRenderer.cpp (1)
176-177: Good: Overhead measured via QPC deltaThis complements the new Pacer plots cleanly.
Common/DeviceResources.h (1)
25-25: Unify VSync setters to avoid duplication.Verification confirms SetVsync is the primary public interface (used in StreamPage.xaml.cpp), while SetEnableVsync is unused elsewhere. Both directly mutate m_enableVsync. Refactor SetEnableVsync to delegate to SetVsync for a single source of truth.
- void SetEnableVsync(bool ev) { m_enableVsync = ev; } + void SetEnableVsync(bool ev) { SetVsync(ev); }State/Stats.cpp (1)
115-125: All verification concerns confirmed — no issues found.✓ Zero-initialization verified: All three VIDEO_STATS instances (m_ActiveWndVideoStats, m_LastWndVideoStats, m_GlobalVideoStats) are properly zero-initialized via ZeroMemory in the constructor (State/Stats.cpp:13-15) and on window rotation (State/Stats.cpp:47).
✓ New fields confirmed in struct: Both totalPacerTimeUs (uint64_t) and totalPresentDisplayMs (double) are declared in the VIDEO_STATS typedef (State/Stats.h:35, 37).
✓ Serialization/persistence: Not applicable — no stats persistence mechanisms exist in the codebase. Stats are only formatted for display output via formatVideoStats.
The pacer/present metrics implementation is sound. New fields are properly initialized, consistently accumulated with += operations, and correctly used in reporting.
Streaming/VideoRenderer.cpp (1)
106-142: SRV format mappings for NV12/P010 are correct; shader code not accessible for full validation.The Y and UV plane formats are correctly mapped per D3D11 specifications (R8_UNORM/R8G8_UNORM for NV12; R16_UNORM/R16G16_UNORM for P010). The dual-SRV approach—creating separate resource views of the same texture with different format interpretations—is the standard pattern for planar YUV video in Direct3D 11. Driver support for this approach is well-established across all compliant D3D11 implementations.
However, shader source code is not present in the repository, so verification that shader sampling code correctly uses these plane views cannot be completed. Ensure shader code reads Y values from slot 0 (single-channel) and UV values from slot 1 (dual-channel interleaved format).
|
OK Rabbit, I'll fix some of these if you stop writing VSYNC in all caps. |
|
@TheElixZammuto What do you think about this one it found? Without fully understanding this input code, it does seem like KeyUp ought to be |
moonlight-xbox-dx.vcxproj.filters: remove duplicate items Remove unused Microsoft code in DirectXHelper LogRenderer: more efficient warning message merge with dxhelper Streaming/PacerRational.h: cleanup intrinsic VideoRenderer: include libavutil headers HostSettingsPage: include cmath PlotDesc.h: check size of kPlotDescs array moonlight_xbox_dxMain Input: loop should run at 1000hz MoonlightClient.cpp: use std::lround when constructing config.clientRefreshRateX100 State/Stats.cpp: minor type fixes FFmpegDecoder: remove unused code Set ffmpeg log level to AV_LOG_ERROR Pacer: fix newline in log message Fix minor CR nitsquashs HostSettingsPage: fix HDR4KNote GridColumn HostSettingsPage: handle empty fps list MsToQpc: fix rounding FQLog: fix counter FFmpegDecoder: correctly unref hw_device_ctx FFmpegDecoder: free pkt correctly Pacer: fix m_Running to be atomic Pacer: pass QpcFreq to rational StatsRenderer: fix row2 FloatBuffer: return size_t instead of int vcpkg: remove ffmpeg features: avdevice, x264
|
|
Testing went well with 1080p 60fps on Xbox Series S Other uses cases were tested by other people, so more than happy to merge! |
2 participants
I think this is ready for a proper release. It's received pretty positive reviews from a handful of testers.
Frame Pacing:
120fps:
Misc:
Summary by CodeRabbit
Release Notes
New Features
UI Changes
Improvements