From e5427c2c5a94273830e1299df474444dd6e805d9 Mon Sep 17 00:00:00 2001 From: Jason Date: Tue, 25 Aug 2026 23:54:16 +0200 Subject: [PATCH 1/2] Bound the display handle enumeration and stop stranding vibrance (#138) Three defects, all reachable without a hand-edited settings file. EnumerateDisplayHandles looped until the prebuilt vibranceDLL returned -1 and nothing capped the index. Issue #138 reports extreme CPU usage on a laptop whose NVIDIA GPU lives in a Thunderbolt enclosure: unplug it and nvapi.dll is still installed, but the enumeration never terminates. In an x86 process the unbounded List then exhausts the address space and the OutOfMemoryException surfaces as "failed to initialize". Bound the loop at NvapiMaxDisplays, derived from the NvapiMaxPhysicalGpus constant this class already uses to size its GPU handle arrays. The real enumeration on a three-monitor machine terminates at index 3, so the bound cannot truncate a legitimate display. Deduplicate the handles while here. A driver stuck returning the same handle would otherwise fill the list with copies, each one costing its own setDVCLevel call on the restore path. This is a latent cost, not the cause of #138 - pre-fix the restore path was unreachable, because a loop that never returns means isInitialized is never set and OnWinEventHook is never subscribed. OnWinEventHook wrapped its entire body in "if (_applicationSettings.Count > 0)", the else restore branch included, in both the NVIDIA and AMD proxies. Removing the last saved game while its game held the foreground therefore stranded the vibrance level and the resolution change with no way back short of restarting. Compute the match conditionally instead so the restore branch is always reachable. The apply and restore branches are unchanged: the bulk of the diff is a mechanical dedent, verified inert by normalising whitespace and comments and comparing against upstream/master. listApplications_DoubleClick indexed SelectedItems[0] with no count check. ListView raises DoubleClick for the whole control, empty space included, where the indexer throws ArgumentOutOfRangeException out of a UI event handler with nothing on the path to catch it. --- vibrance.GUI/AMD/AmdDynamicVibranceProxy.cs | 81 ++++----- .../NVIDIA/NvidiaDynamicVibranceProxy.cs | 154 ++++++++++++------ vibrance.GUI/common/VibranceGUI.cs | 5 + 3 files changed, 148 insertions(+), 92 deletions(-) diff --git a/vibrance.GUI/AMD/AmdDynamicVibranceProxy.cs b/vibrance.GUI/AMD/AmdDynamicVibranceProxy.cs index 7e98d3d..5e1d51d 100644 --- a/vibrance.GUI/AMD/AmdDynamicVibranceProxy.cs +++ b/vibrance.GUI/AMD/AmdDynamicVibranceProxy.cs @@ -107,51 +107,54 @@ public VibranceInfo GetVibranceInfo() private void OnWinEventHook(object sender, WinEventHookEventArgs e) { - if (_applicationSettings.Count > 0) + //an empty list still has to reach the restore branch below. Gating the whole handler on + //Count > 0 stranded vibrance and the resolution change whenever the last entry was + //removed while its game held the foreground, with no way back short of restarting. + ApplicationSetting applicationSetting = _applicationSettings.Count > 0 + ? _applicationSettings.FirstOrDefault(x => string.Equals(x.Name, e.ProcessName, StringComparison.OrdinalIgnoreCase)) + : null; + + if (applicationSetting != null) { - ApplicationSetting applicationSetting = _applicationSettings.FirstOrDefault(x => string.Equals(x.Name, e.ProcessName, StringComparison.OrdinalIgnoreCase)); - if (applicationSetting != null) + //test if a resolution change is needed + Screen screen = Screen.FromHandle(e.Handle); + if (_vibranceInfo.neverChangeResolution == false && + applicationSetting.IsResolutionChangeNeeded && + IsResolutionChangeNeeded(screen, applicationSetting.ResolutionSettings) && + _windowsResolutionSettings.ContainsKey(screen.DeviceName) && + _windowsResolutionSettings[screen.DeviceName].Item2.Contains(applicationSetting.ResolutionSettings)) { - //test if a resolution change is needed - Screen screen = Screen.FromHandle(e.Handle); - if (_vibranceInfo.neverChangeResolution == false && - applicationSetting.IsResolutionChangeNeeded && - IsResolutionChangeNeeded(screen, applicationSetting.ResolutionSettings) && - _windowsResolutionSettings.ContainsKey(screen.DeviceName) && - _windowsResolutionSettings[screen.DeviceName].Item2.Contains(applicationSetting.ResolutionSettings)) - { - _gameScreen = screen; - PerformResolutionChange(screen, applicationSetting.ResolutionSettings); - } - - _amdAdapter.SetSaturationOnAllDisplays(_vibranceInfo.userVibranceSettingDefault); - if (_vibranceInfo.affectPrimaryMonitorOnly) - { - _amdAdapter.SetSaturationOnDisplay(applicationSetting.IngameLevel, screen.DeviceName); - } - else - { - _amdAdapter.SetSaturationOnAllDisplays(applicationSetting.IngameLevel); - } + _gameScreen = screen; + PerformResolutionChange(screen, applicationSetting.ResolutionSettings); + } + + _amdAdapter.SetSaturationOnAllDisplays(_vibranceInfo.userVibranceSettingDefault); + if (_vibranceInfo.affectPrimaryMonitorOnly) + { + _amdAdapter.SetSaturationOnDisplay(applicationSetting.IngameLevel, screen.DeviceName); } else { - IntPtr processHandle = e.Handle; - if (GetForegroundWindow() != processHandle) - return; - - //test if a resolution change is needed - Screen screen = Screen.FromHandle(processHandle); - if (_vibranceInfo.neverChangeResolution == false && - _gameScreen != null && _gameScreen.Equals(screen) && - _windowsResolutionSettings.ContainsKey(screen.DeviceName) && - IsResolutionChangeNeeded(screen, _windowsResolutionSettings[screen.DeviceName].Item1)) - { - PerformResolutionChange(screen, _windowsResolutionSettings[screen.DeviceName].Item1); - } - - _amdAdapter.SetSaturationOnAllDisplays(_vibranceInfo.userVibranceSettingDefault); + _amdAdapter.SetSaturationOnAllDisplays(applicationSetting.IngameLevel); + } + } + else + { + IntPtr processHandle = e.Handle; + if (GetForegroundWindow() != processHandle) + return; + + //test if a resolution change is needed + Screen screen = Screen.FromHandle(processHandle); + if (_vibranceInfo.neverChangeResolution == false && + _gameScreen != null && _gameScreen.Equals(screen) && + _windowsResolutionSettings.ContainsKey(screen.DeviceName) && + IsResolutionChangeNeeded(screen, _windowsResolutionSettings[screen.DeviceName].Item1)) + { + PerformResolutionChange(screen, _windowsResolutionSettings[screen.DeviceName].Item1); } + + _amdAdapter.SetSaturationOnAllDisplays(_vibranceInfo.userVibranceSettingDefault); } } diff --git a/vibrance.GUI/NVIDIA/NvidiaDynamicVibranceProxy.cs b/vibrance.GUI/NVIDIA/NvidiaDynamicVibranceProxy.cs index 8f2a5b3..bc57d83 100644 --- a/vibrance.GUI/NVIDIA/NvidiaDynamicVibranceProxy.cs +++ b/vibrance.GUI/NVIDIA/NvidiaDynamicVibranceProxy.cs @@ -117,6 +117,27 @@ class NvidiaDynamicVibranceProxy : IVibranceProxy public const int NvapiMaxPhysicalGpus = 64; + + // Each physical GPU can drive more than one display, so the bound below (issue #138) scales + // by a display-per-GPU headroom - not a quoted nvapi.h constant: no copy of nvapi.h is + // vendored in this repo, so the exact ceiling NvAPI itself uses cannot be confirmed here. + // A bound that must never truncate a real display should over-approximate rather than try + // to match that ceiling exactly. + public const int NvapiAdvancedDisplayHeads = 4; + + // The ceiling EnumerateDisplayHandles() (below) loops up to. NvapiMaxPhysicalGpus is + // already trusted to size the GPU handle arrays in InitializeProxy(), so deriving this + // bound from it is internally consistent with the rest of the class. + // + // Pre-fix (issue #138): with no NVIDIA GPU present the prebuilt vibranceDLL.dll never + // returned -1, so the loop spun forever. InitializeProxy() never returned in that state, so + // isInitialized was never set and the constructor never reached the OnWinEventHook + // subscription below - nothing ever "walked" the growing list. In this x86 process it + // instead ran the unbounded List out of address space, throwing OutOfMemoryException, + // which the constructor's catch (Exception) block turns into the "failed to initialize" + // dialog. + public const int NvapiMaxDisplays = NvapiMaxPhysicalGpus * NvapiAdvancedDisplayHeads; + public const int NvapiMaxLevel = 63; public const int NvapiDefaultLevel = 0; @@ -208,62 +229,65 @@ private void InitializeProxy() private static void OnWinEventHook(object sender, WinEventHookEventArgs e) { - if (_applicationSettings.Count > 0) + //an empty list still has to reach the restore branch below. Gating the whole handler on + //Count > 0 stranded vibrance and the resolution change whenever the last entry was + //removed while its game held the foreground, with no way back short of restarting. + ApplicationSetting applicationSetting = _applicationSettings.Count > 0 + ? _applicationSettings.FirstOrDefault(x => string.Equals(x.Name, e.ProcessName, StringComparison.OrdinalIgnoreCase)) + : null; + + if (applicationSetting != null) { - ApplicationSetting applicationSetting = _applicationSettings.FirstOrDefault(x => string.Equals(x.Name, e.ProcessName, StringComparison.OrdinalIgnoreCase)); - if (applicationSetting != null) - { - int displayHandle = GetApplicationDisplayHandle(e.Handle); - //test if changing the vibrance value is needed - if (displayHandle != -1 && !equalsDVCLevel(displayHandle, applicationSetting.IngameLevel)) + int displayHandle = GetApplicationDisplayHandle(e.Handle); + //test if changing the vibrance value is needed + if (displayHandle != -1 && !equalsDVCLevel(displayHandle, applicationSetting.IngameLevel)) + { + //test if a resolution change is needed + Screen screen = Screen.FromHandle(e.Handle); + if (_vibranceInfo.neverChangeResolution == false && + applicationSetting.IsResolutionChangeNeeded && + IsResolutionChangeNeeded(screen, applicationSetting.ResolutionSettings) && + _windowsResolutionSettings.ContainsKey(screen.DeviceName) && + _windowsResolutionSettings[screen.DeviceName].Item2.Contains(applicationSetting.ResolutionSettings)) { - //test if a resolution change is needed - Screen screen = Screen.FromHandle(e.Handle); - if (_vibranceInfo.neverChangeResolution == false && - applicationSetting.IsResolutionChangeNeeded && - IsResolutionChangeNeeded(screen, applicationSetting.ResolutionSettings) && - _windowsResolutionSettings.ContainsKey(screen.DeviceName) && - _windowsResolutionSettings[screen.DeviceName].Item2.Contains(applicationSetting.ResolutionSettings)) - { - PerformResolutionChange(screen, applicationSetting.ResolutionSettings); - } - _gameScreen = screen; - _vibranceInfo.defaultHandle = displayHandle; - setDVCLevel(_vibranceInfo.defaultHandle, applicationSetting.IngameLevel); + PerformResolutionChange(screen, applicationSetting.ResolutionSettings); } + _gameScreen = screen; + _vibranceInfo.defaultHandle = displayHandle; + setDVCLevel(_vibranceInfo.defaultHandle, applicationSetting.IngameLevel); } - else + } + else + { + IntPtr processHandle = e.Handle; + + if (!isWindowActive(ref processHandle)) + return; + + //test if a resolution change is needed + Screen currentScreen = Screen.FromHandle(processHandle); + if (_vibranceInfo.neverChangeResolution == false && + _gameScreen != null && + _gameScreen.Equals(currentScreen) && + _windowsResolutionSettings.ContainsKey(currentScreen.DeviceName) && + IsResolutionChangeNeeded(currentScreen, _windowsResolutionSettings[currentScreen.DeviceName].Item1)) { - IntPtr processHandle = e.Handle; + PerformResolutionChange(currentScreen, _windowsResolutionSettings[currentScreen.DeviceName].Item1); + } - if (!isWindowActive(ref processHandle)) - return; - - //test if a resolution change is needed - Screen currentScreen = Screen.FromHandle(processHandle); - if (_vibranceInfo.neverChangeResolution == false && - _gameScreen != null && - _gameScreen.Equals(currentScreen) && - _windowsResolutionSettings.ContainsKey(currentScreen.DeviceName) && - IsResolutionChangeNeeded(currentScreen, _windowsResolutionSettings[currentScreen.DeviceName].Item1)) + //test if changing the vibrance value is needed + if (_vibranceInfo.affectPrimaryMonitorOnly && !equalsDVCLevel(_vibranceInfo.defaultHandle, _vibranceInfo.userVibranceSettingDefault)) + { + if(_gameScreen != null && !_gameScreen.DeviceName.Equals(currentScreen.DeviceName)) { - PerformResolutionChange(currentScreen, _windowsResolutionSettings[currentScreen.DeviceName].Item1); + return; } - //test if changing the vibrance value is needed - if (_vibranceInfo.affectPrimaryMonitorOnly && !equalsDVCLevel(_vibranceInfo.defaultHandle, _vibranceInfo.userVibranceSettingDefault)) - { - if(_gameScreen != null && !_gameScreen.DeviceName.Equals(currentScreen.DeviceName)) - { - return; - } - - setDVCLevel(_vibranceInfo.defaultHandle, _vibranceInfo.userVibranceSettingDefault); - } - else if (!_vibranceInfo.affectPrimaryMonitorOnly && !_vibranceInfo.displayHandles.TrueForAll(handle => equalsDVCLevel(handle, _vibranceInfo.userVibranceSettingDefault))) - { - _vibranceInfo.displayHandles.ForEach(handle => setDVCLevel(handle, _vibranceInfo.userVibranceSettingDefault)); - } + setDVCLevel(_vibranceInfo.defaultHandle, _vibranceInfo.userVibranceSettingDefault); + } + else if (!_vibranceInfo.affectPrimaryMonitorOnly && !_vibranceInfo.displayHandles.TrueForAll(handle => equalsDVCLevel(handle, _vibranceInfo.userVibranceSettingDefault))) + { + _vibranceInfo.displayHandles.ForEach(handle => setDVCLevel(handle, _vibranceInfo.userVibranceSettingDefault)); } } } @@ -285,15 +309,39 @@ private static void PerformResolutionChange(Screen screen, ResolutionModeWrapper private void EnumerateDisplayHandles() { - for (int i = 0, displayHandle = 0; displayHandle != -1; i++) + _vibranceInfo.displayHandles = EnumerateDisplayHandles(enumerateNvidiaDisplayHandle); + } + + // The loop body on its own, taking the enumerator as a delegate instead of calling the + // P/Invoke directly, so StabilityFixture can drive it with a stub and cover the bound and + // the dedupe without the real DLL. + // + // Bounded at NvapiMaxDisplays (issue #138): the prebuilt vibranceDLL.dll never returns -1 + // when no NVIDIA GPU is present, so an unbounded loop here spun forever. A legitimate + // enumeration can never reach NvapiMaxDisplays, so the bound never cuts off real displays. + // + // Deduped: a driver stuck returning the same handle repeatedly would otherwise fill the + // list with copies of it, each one then getting its own setDVCLevel call on every restore + // (OnWinEventHook's displayHandles.ForEach(...) below). That is a latent cost on the + // restore path now that the loop above is bounded - it did not cause #138: pre-fix, the + // unbounded loop kept InitializeProxy() from ever returning, so OnWinEventHook was never + // even subscribed and the restore path could not run regardless of duplicates. + // + // Always returns an allocated (possibly empty) list, never null: OnWinEventHook calls + // TrueForAll/ForEach on _vibranceInfo.displayHandles unconditionally on the restore path. + internal static List EnumerateDisplayHandles(Func enumerateDisplayHandle) + { + List displayHandles = new List(); + for (int i = 0; i < NvapiMaxDisplays; i++) { - if (_vibranceInfo.displayHandles == null) - _vibranceInfo.displayHandles = new List(); + int displayHandle = enumerateDisplayHandle(i); + if (displayHandle == -1) + break; - displayHandle = enumerateNvidiaDisplayHandle(i); - if (displayHandle != -1) - _vibranceInfo.displayHandles.Add(displayHandle); + if (!displayHandles.Contains(displayHandle)) + displayHandles.Add(displayHandle); } + return displayHandles; } private static int GetApplicationDisplayHandle(IntPtr hWnd) diff --git a/vibrance.GUI/common/VibranceGUI.cs b/vibrance.GUI/common/VibranceGUI.cs index fe5804c..eb514bf 100644 --- a/vibrance.GUI/common/VibranceGUI.cs +++ b/vibrance.GUI/common/VibranceGUI.cs @@ -503,6 +503,11 @@ public void ListViewItem_SetSpacing(ListView listview, short leftPadding, short private void listApplications_DoubleClick(object sender, EventArgs e) { + //ListView raises DoubleClick for the whole control, empty space included, where + //SelectedItems is empty and the indexer below would throw ArgumentOutOfRangeException. + if (this.listApplications.SelectedItems.Count == 0) + return; + ListViewItem selectedItem = this.listApplications.SelectedItems[0]; if (selectedItem != null) { From f53c867f584009a3096aeaa955194033bd4d8778 Mon Sep 17 00:00:00 2001 From: Jason Date: Tue, 25 Aug 2026 23:54:24 +0200 Subject: [PATCH 2/2] Add a regression fixture for the stability pass Add StabilityFixture and --selftest-stability, following no existing self-test convention here since this codebase does not have one yet - this introduces it. Six checks: the display handle enumeration bound, the dedupe against both a constant and an interleaved enumerator, the non-null empty list the restore path dereferences, an empty settings list still reaching restore, and the apply path still applying with the matched setting's own level. The restore check fails against the pre-fix AMD proxy, so it is real evidence; the apply check passes against the pre-fix proxy too and is forward coverage rather than proof of that fix. No GUI, no live GPU driver: the enumeration checks drive a stub delegate instead of the prebuilt DLL, and the restore/apply checks run through the AMD proxy's mockable IAmdAdapter interface. Wired ahead of GPU vendor detection in Main so it needs neither. --- vibrance.GUI/Program.cs | 11 + vibrance.GUI/common/StabilityFixture.cs | 276 ++++++++++++++++++++++++ vibrance.GUI/vibrance.GUI.csproj | 1 + 3 files changed, 288 insertions(+) create mode 100644 vibrance.GUI/common/StabilityFixture.cs diff --git a/vibrance.GUI/Program.cs b/vibrance.GUI/Program.cs index 472db5b..b8410aa 100644 --- a/vibrance.GUI/Program.cs +++ b/vibrance.GUI/Program.cs @@ -19,6 +19,7 @@ static class Program private const string ErrorGraphicsAdapterUnknown = "Failed to determine your Graphic GraphicsAdapter type (NVIDIA/AMD). Make sure you have installed a proper GPU driver. Intel laptops are not supported as stated on the website. When installing your GPU driver did not work, please contact @juvlarN at twitter. Press Yes to open twitter in your browser now. Error: "; private const string ErrorGraphicsAdapterAmbiguous = "Both NVIDIA and AMD graphic drivers have been found on your system. This can happen when you recently switched your graphic card and did not uninstall the old drivers. Make sure to uninstall unused graphic drivers to keep your system safe and stable. Use the program \"Display Driver Uninstaller\" to uninstall your old drivers!\n\nPress Yes to open \"Display Driver Uninstaller\" download website now.\nPress No to quit vibranceGUI."; private const string MessageBoxCaption = "vibranceGUI Error"; + private const string StabilitySelfTestMessageBoxCaption = "vibranceGUI stability fixes self test"; [STAThread] static void Main(string[] args) @@ -31,6 +32,16 @@ static void Main(string[] args) return; } + // Placed ahead of GPU vendor detection: the display handle enumeration bound/dedupe is + // driven by a stub, and the restore branch check runs through the AMD proxy's mockable + // adapter interface, so neither one touches a driver or the prebuilt NVIDIA DLL. + if (args.Contains("--selftest-stability")) + { + MessageBox.Show(string.Join(Environment.NewLine, StabilityFixture.Run().ToArray()), + StabilitySelfTestMessageBoxCaption, MessageBoxButtons.OK, MessageBoxIcon.Information); + return; + } + Application.EnableVisualStyles(); Application.SetCompatibleTextRenderingDefault(false); NativeMethods.SetDllDirectory(CommonUtils.GetVibrance_GUI_AppDataPath()); diff --git a/vibrance.GUI/common/StabilityFixture.cs b/vibrance.GUI/common/StabilityFixture.cs new file mode 100644 index 0000000..14bcd79 --- /dev/null +++ b/vibrance.GUI/common/StabilityFixture.cs @@ -0,0 +1,276 @@ +using System; +using System.Collections.Generic; +using System.Reflection; +using System.Runtime.InteropServices; +using vibrance.GUI.AMD; +using vibrance.GUI.AMD.vendor; +using vibrance.GUI.NVIDIA; + +namespace vibrance.GUI.common +{ + /// + /// Regression coverage for the stability pass: the unbounded NVIDIA display handle enumeration + /// (issue #138), a separate latent duplicate-handle bug on its restore path that the same fix + /// also closed, and the "an empty settings list must still reach the restore branch" fix + /// already applied to both OnWinEventHook handlers - both the empty-list side and the + /// still-matches side of it. No GUI, no live GPU driver. Run by vibrance.GUI.exe + /// --selftest-stability. + /// + public static class StabilityFixture + { + [DllImport("user32.dll")] + private static extern IntPtr GetDesktopWindow(); + + public static List Run() + { + Checklist checklist = new Checklist(); + checklist.Lines.Add("vibranceGUI stability fixes self test"); + checklist.Lines.Add(string.Empty); + + CheckDisplayHandleEnumeration(checklist); + CheckEmptyApplicationSettingsReachesRestore(checklist); + CheckMatchedApplicationSettingApplies(checklist); + + checklist.Lines.Add(string.Empty); + checklist.Lines.Add(string.Format("PASSED {0}/{1}", checklist.Passed, checklist.Total)); + return checklist.Lines; + } + + // The bound below closes issue #138 on its own; the dedupe closes a separate, latent bug + // on the restore path that the same fix touched (see the per-check comments for why the + // two are not the same thing). A stub enumerator stands in for enumerateNvidiaDisplayHandle() + // so both are provable without the prebuilt DLL or a GPU of either vendor. + private static void CheckDisplayHandleEnumeration(Checklist checklist) + { + checklist.Lines.Add("NVIDIA display handle enumeration - bounded and deduped (issue #138):"); + + // Every call hands back a fresh handle, so nothing but the bound itself can end the + // loop - this is the shape that spun #138's loop forever pre-fix. What that actually + // did to the process is not reproduced here (see EnumerateDisplayHandles's own comment): + // this only proves the bound now ends it. + List unbounded = NvidiaDynamicVibranceProxy.EnumerateDisplayHandles( + delegate(int index) { return index; }); + checklist.Check(unbounded.Count == NvidiaDynamicVibranceProxy.NvapiMaxDisplays, + string.Format("an enumerator that never returns -1 stops at NvapiMaxDisplays ({0}), got {1}", + NvidiaDynamicVibranceProxy.NvapiMaxDisplays, unbounded.Count)); + + // Not #138's cause - the loop above being unbounded is (see EnumerateDisplayHandles's + // own comment for why the restore path was unreachable pre-fix regardless). This guards + // a separate, latent bug on that restore path: a driver stuck on one handle would fill + // the list with copies of it, each getting its own setDVCLevel call on every foreground + // change. + List constant = NvidiaDynamicVibranceProxy.EnumerateDisplayHandles( + delegate(int index) { return 7; }); + checklist.Check(constant.Count == 1 && constant[0] == 7, + "a driver that always returns the same handle yields exactly one entry, not " + + NvidiaDynamicVibranceProxy.NvapiMaxDisplays + " copies of it"); + + // Not just an immediate repeat - a duplicate recurring later in the sequence is + // dropped too, and the first-seen order of the survivors is kept. + int[] sequence = { 1, 2, 1, 3, 2 }; + List interleaved = NvidiaDynamicVibranceProxy.EnumerateDisplayHandles( + delegate(int index) { return index < sequence.Length ? sequence[index] : -1; }); + checklist.Check(SequenceEqual(interleaved, new List { 1, 2, 3 }), + "duplicates are dropped wherever they recur in the sequence"); + + // OnWinEventHook's restore path calls TrueForAll/ForEach on this unconditionally - + // it must be an allocated empty list, never null, even when nothing enumerates. + List none = NvidiaDynamicVibranceProxy.EnumerateDisplayHandles( + delegate(int index) { return -1; }); + checklist.Check(none != null && none.Count == 0, + "an enumerator that returns -1 immediately yields a non-null, empty list"); + + checklist.Lines.Add(string.Empty); + } + + // The already-applied fix: OnWinEventHook used to be gated on "if (_applicationSettings.Count + // > 0)" for its *entire* body, so removing the last saved game stranded vibrance and the + // resolution on whatever level the game last set, with the restore branch never reachable + // again short of restarting. Exercised through the AMD proxy, whose GPU access sits behind + // the mockable IAmdAdapter interface: the NVIDIA proxy's own restore branch calls straight + // into the prebuilt native DLL and is not reachable from a self test without a live NVIDIA + // driver, so that side is not covered here. + private static void CheckEmptyApplicationSettingsReachesRestore(Checklist checklist) + { + checklist.Lines.Add("An empty settings list still reaches the restore branch (AMD proxy):"); + + FakeAmdAdapter adapter = new FakeAmdAdapter(); + List emptySettings = new List(); + Dictionary>> windowsResolutionSettings = + new Dictionary>>(); + + AmdDynamicVibranceProxy proxy = new AmdDynamicVibranceProxy(adapter, emptySettings, windowsResolutionSettings); + + // OnWinEventHook is private - there is no other seam into it, and adding one is out of + // scope for this fix. FakeAmdAdapter.IsAvailable() returning false (below) kept the + // constructor from installing a real, process-lifetime SetWinEventHook, so this + // reflection call is the only thing that runs. + MethodInfo onWinEventHook = typeof(AmdDynamicVibranceProxy).GetMethod( + "OnWinEventHook", BindingFlags.NonPublic | BindingFlags.Instance); + + // The restore branch's first line is "if (GetForegroundWindow() != processHandle) + // return;", and there is no seam to fake that check through, so the event has to name + // the real foreground window. That makes this test dependent on nothing else changing + // focus between the read below and the call - QA measured that race directly (0 + // failures in 90,000+ passive iterations, 2 in 65,000 under deliberately engineered + // focus contention) and the reviewer preferred living with the rare loss over adding a + // production-only seam. Re-reading GetForegroundWindow() after the call and skipping + // rather than failing when it moved is the agreed middle ground. + IntPtr foregroundBefore = AmdDynamicVibranceProxy.GetForegroundWindow(); + WinEventHookEventArgs args = new WinEventHookEventArgs + { + Handle = foregroundBefore, + ProcessName = "doesnotmatter" + }; + + onWinEventHook.Invoke(proxy, new object[] { null, args }); + + if (AmdDynamicVibranceProxy.GetForegroundWindow() != foregroundBefore) + { + checklist.Skip("empty settings list reaches the restore branch - the foreground " + + "window changed mid test, its precondition was destroyed by a real focus change"); + checklist.Lines.Add(string.Empty); + return; + } + + checklist.Check(adapter.SetSaturationOnAllDisplaysCallCount == 1, + "the restore call ran once (pre-fix, Count > 0 gated the whole handler and it never ran at all)"); + + checklist.Lines.Add(string.Empty); + } + + // The counterpart to the empty-list case above: a settings list with a real match must + // still reach the *apply* half of OnWinEventHook and finish with the matched setting's own + // level, not the Windows default. + // + // Checked against git show upstream/master:.../AmdDynamicVibranceProxy.cs: unlike the + // empty-list case, this one PASSES unchanged there too - the dedent that fix made was inert + // for Count > 0, exactly as code review already established from a whitespace-normalised + // diff. So this case is not evidence for that fix; it is general regression coverage so a + // future change to the apply path - this one, or the same kind of dedent elsewhere - cannot + // silently break which level gets applied. + // + // Unlike the restore branch, the apply branch (the "if (applicationSetting != null)" half + // of OnWinEventHook) never calls GetForegroundWindow() - it only needs *some* valid window + // handle, for Screen.FromHandle(e.Handle). Using GetDesktopWindow(), a handle that never + // changes, keeps this test free of the restore branch's focus race rather than reproducing + // it and then working around it. + private static void CheckMatchedApplicationSettingApplies(Checklist checklist) + { + checklist.Lines.Add("A matched settings list applies that setting's own level, not the Windows default:"); + + const int ingameLevel = 77; + ApplicationSetting matchingSetting = new ApplicationSetting(); + matchingSetting.Name = "TestGame"; + matchingSetting.IngameLevel = ingameLevel; + + List settings = new List(); + settings.Add(matchingSetting); + + FakeAmdAdapter adapter = new FakeAmdAdapter(); + Dictionary>> windowsResolutionSettings = + new Dictionary>>(); + + AmdDynamicVibranceProxy proxy = new AmdDynamicVibranceProxy(adapter, settings, windowsResolutionSettings); + // Not what this test checks, but left off, the resolution branch would try to compare + // against this machine's real display mode below - off is the safe default here. + proxy.SetNeverSwitchResolution(true); + + MethodInfo onWinEventHook = typeof(AmdDynamicVibranceProxy).GetMethod( + "OnWinEventHook", BindingFlags.NonPublic | BindingFlags.Instance); + + WinEventHookEventArgs args = new WinEventHookEventArgs + { + Handle = GetDesktopWindow(), + ProcessName = "TestGame" + }; + + onWinEventHook.Invoke(proxy, new object[] { null, args }); + + // The apply branch always sets the Windows default level first, unconditionally, then + // branches on affectPrimaryMonitorOnly (default false, so a fresh proxy takes the "all + // displays" branch exercised here) to set the matched level - two calls, not one. Only + // the last of them is what this check is about. + checklist.Check(adapter.SetSaturationOnAllDisplaysCallCount == 2 && + adapter.LastSetSaturationOnAllDisplaysLevel == ingameLevel, + string.Format("SetSaturationOnAllDisplays' last call used the matched setting's IngameLevel ({0}), not the Windows default", + ingameLevel)); + + checklist.Lines.Add(string.Empty); + } + + private static bool SequenceEqual(List actual, List expected) + { + if (actual.Count != expected.Count) + return false; + for (int i = 0; i < actual.Count; i++) + { + if (actual[i] != expected[i]) + return false; + } + return true; + } + + // Everything IAmdAdapter exposes, none of it touching real hardware - call counters and + // the last level handed to each method, for the restore and apply branches to check. + private class FakeAmdAdapter : IAmdAdapter + { + public int SetSaturationOnAllDisplaysCallCount; + public int LastSetSaturationOnAllDisplaysLevel = int.MinValue; + + public int SetSaturationOnDisplayCallCount; + public int LastSetSaturationOnDisplayLevel = int.MinValue; + public string LastSetSaturationOnDisplayName; + + public void SetSaturationOnAllDisplays(int vibranceLevel) + { + SetSaturationOnAllDisplaysCallCount++; + LastSetSaturationOnAllDisplaysLevel = vibranceLevel; + } + + public void SetSaturationOnDisplay(int vibranceLevel, string displayName) + { + SetSaturationOnDisplayCallCount++; + LastSetSaturationOnDisplayLevel = vibranceLevel; + LastSetSaturationOnDisplayName = displayName; + } + + public bool IsAvailable() + { + return false; + } + + public void Init() + { + } + + public void Dispose() + { + } + } + + private class Checklist + { + public readonly List Lines = new List(); + public int Passed; + public int Total; + + public void Check(bool condition, string description) + { + Total++; + if (condition) + Passed++; + Lines.Add(string.Format("[{0}] {1}", condition ? "PASS" : "FAIL", description)); + } + + // Deliberately not counted in Total/Passed: a check whose precondition was destroyed + // by something outside the test (see the GetForegroundWindow() race above) proves + // nothing either way, and folding it into PASSED n/m would let a real regression hide + // behind an unrelated, unlucky focus change. + public void Skip(string description) + { + Lines.Add(string.Format("[SKIP] {0}", description)); + } + } + } +} diff --git a/vibrance.GUI/vibrance.GUI.csproj b/vibrance.GUI/vibrance.GUI.csproj index 71e63a8..198f7fc 100644 --- a/vibrance.GUI/vibrance.GUI.csproj +++ b/vibrance.GUI/vibrance.GUI.csproj @@ -131,6 +131,7 @@ +