Restore vibrance to what was changed, not to where focus landed (#60, #36, #144, #95) - #160
Open
SwatX18 wants to merge 2 commits into
Open
Restore vibrance to what was changed, not to where focus landed (#60, #36, #144, #95)#160SwatX18 wants to merge 2 commits into
SwatX18 wants to merge 2 commits into
Conversation
Four issues, two reporters each: juv#60/#36 and juv#144/juv#95. Every launch wrote digital vibrance level 0 to an arbitrary display. InitializeProxy read userVibranceSettingDefault before SetVibranceWindowsLevel had ever run (that happens later, from the background worker), so the value was still the struct default of 0, written via enumerateNvidiaDisplayHandle(0) - a handle with no relationship to the primary display. A second monitor set on purpose in the NVIDIA control panel was reset to neutral at every startup. The write is gone; restore is a no-op until isWindowsLevelKnown is true. The handle it wrote through, defaultHandle, was then hijacked by the next game's own display and never restored, so the Windows level - including on exit - landed wherever the last game had been. The field is removed; a HashSet<string> work-list keyed by Screen.DeviceName (VibranceRestoreHelper) now records every display actually written to. Restore was also gated on whichever screen currently had focus, not on what had actually changed - added in 2017 to stop a game losing vibrance when the user clicked a second screen while still visible. It could only express "the mouse is elsewhere", never "the game is still running", so an alt-tab or the game exiting could leave a monitor saturated indefinitely. The gate is removed; restore now targets the work-list plus the primary display Windows Vibrance Level owns, regardless of where the foreground is. AMD's restore already ignored affectPrimaryMonitorOnly outright, writing every display every time. Its apply path additionally reset every display to the Windows level immediately before applying the game's level - a write immediately overwritten in every case, and one that touched non-game displays it had no business touching at all; removed rather than left in place. Both proxies now drive the same recording and restore. Adds an INvidiaVibranceDevice seam over the four vibranceDLL.dll calls, without which none of this was reachable from a test.
VibranceRestoreFixture, run via --selftest-vibrance. No GUI, no live GPU driver: the NVIDIA half drives ApplyGameVibranceLevel/ RestoreWindowsVibranceLevel (and, through reflection, the real private static OnWinEventHook) against a fake INvidiaVibranceDevice; the AMD half reflects into the real OnWinEventHook against a fake IAmdAdapter. Deliberately has no hardware variant and must never grow one - these issues are themselves about a real display's vibrance changing when it should not have, so a fixture able to do that to a reviewer's own monitor would be the exact bug it exists to catch. 39 checks, five pure (VibranceRestoreHelper.ComposeRestoreTargets), twelve NVIDIA, six AMD (some assert more than one condition), plus the pins that already passed before this fix and are labelled as such. Two checks reflect _gameScreen to a real, different monitor and drive the actual restore branch rather than calling the restore method directly, since a direct call has no "current screen" parameter and so cannot see a gate wrapped around the call site - confirmed by reinstating upstream's original "if (_gameScreen != null && !_gameScreen.DeviceName.Equals( currentScreen.DeviceName)) return;" gate around the NVIDIA call site, which turns only that one check red. Every check was proven to fail against the specific line it protects: each mutation applied, rebuilt, observed red, then reverted and confirmed byte-identical via git diff. Two AMD checks needed fixing during that process - one couldn't distinguish "recorded, then restored" from "never recorded, but restored anyway because the game's screen happened to be the primary" on a machine where the two coincide, so it now also asserts the work-list directly; the other compared the wrong two screens for its "must differ" precondition and so could not have failed on this machine.
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
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.
Four issues — #60, #36, #144, #95 — two reporters each, spanning 2021 to 2026. Three mechanisms, all reachable in the default configuration:
affectPrimaryMonitorOnlydefaults totrue.Split into two commits so the fixes can be taken without the testing convention — see "About the second commit".
1. Every launch writes vibrance level 0 to an arbitrary display
This isn't described as a mechanism in any of the reports, and I think it's the literal cause of #60.
InitializeProxy(NvidiaDynamicVibranceProxy.cs:195-203) setsdefaultHandle = enumerateNvidiaDisplayHandle(0)and then writes_vibranceInfo.userVibranceSettingDefaultto it. But_vibranceInfo = new VibranceInfo()is constructed a couple of lines earlier, andSetVibranceWindowsLevelisn't reached until the background worker runs — after the constructor returns. So the value written is the struct default, 0.The target,
enumerateNvidiaDisplayHandle(0), is the first enumerated handle. It has no defined relationship to the primary display. And the write ignoresaffectPrimaryMonitorOnlyentirely.So a second monitor whose vibrance the user deliberately set in the NVIDIA control panel is reset to neutral at every startup. #60 reads: "Affect Primary Monitor only: On. Affects second screen when changing to the Windows Vibrance Level."
The write is gone. Restore is now a no-op until the real level is known (
isWindowsLevelKnown).2. The handle it writes through is hijacked and never restored
Applying a game's level overwrites
defaultHandleat:231with that game's display, and never puts it back. Both the restore branch andHandleDvcExitwrite through it — so after the first game on a second monitor, every later write of the Windows level lands there, including on exit."Affect Primary Monitor only" actually delivers "only one monitor, whichever the game was last on."
There's a subtlety that makes #144 worse: the hijack sits behind
if (displayHandle != -1 && !equalsDVCLevel(displayHandle, ingameLevel)). If the game's display is already at its ingame level — restart vibranceGUI while the game runs, or add the game mid-session — the apply is skipped, the handle stays pointing somewhere arbitrary, and the restore writes the Windows level to a display that was never touched while the game's own display is never restored at all.defaultHandleis removed fromVibranceInfoentirely.3. Restore is scoped to where focus landed, not to what was changed
:253-262:Alt-tab to a window on a different monitor and restore never runs. #95, word for word: "I shift-tab and press my second monitor to go to my desktop, where the vibrance is not removed and persists."
This gate was deliberate —
9168349, "Refactored detection of process switch" — and its intent is legible: don't kill a game's vibrance when the user clicks something on the other monitor while the game is still visible.It is removed here on #95's explicit request. The reason it can't stay is that it can only express "the mouse is elsewhere", never "the game is still running" — and #144 is what that conflation costs. Someone who liked the old behaviour will notice. That's a real trade, not a free fix, and I'd rather say so than bury it.
Considered and rejected: keep the gate but exempt the case where the game process has exited (
OpenProcess/GetExitCodeProcesson a PID already in hand). It would restore the 2017 behaviour's actual defect — alt-tab to monitor 2 with the game still running leaves monitor 1 saturated, which is #95 — while adding PID-reuse and handle-lifetime surface to the UI thread inside a WinEvent callback. If "keep game vibrance while the game is visible" is wanted later, the honest mechanism is per-display "is a matched game still on this display", and that's a feature.The fix
Vibrance now works from a record of the displays actually written to, keyed by
Screen.DeviceName, unioned with the primary that the Windows Vibrance Level slider owns — and nothing else.The union matters independently of the four issues: with the setting off a game applies to every display, and turning it on mid-game previously restored one handle and stranded the rest permanently. Since #60 and #36 are both "I checked this box and my second screen is wrong", a user toggling it mid-session is a plausible route into either report.
The enabler already existed:
getAssociatedNvidiaDisplayHandle(:93-98) maps aDeviceNameto a display handle, and the apply path already depends on it working. So this adds no new dependency onvibranceDLL.dll— it just uses the mapping the code already trusts.AMD was wrong differently, in three places.
HandleDvcExit(:90) and the restore branch (:153) both calledSetSaturationOnAllDisplaysunconditionally, ignoring the setting the apply path honours. Both now use the same record.The third, at
:127, was in the apply branch: it wrote the Windows level to every display immediately before applying the game's level — a needless write to displays it has no business touching, on every foreground event. Removed. With the flag on it is overwritten for the game's screen and only stomps unrelated displays; with the flag off the followingSetSaturationOnAllDisplays(ingameLevel)overwrites it everywhere. No display's end state changes in either branch.Testing
New
INvidiaVibranceDeviceseam over the fourvibranceDLL.dllcalls the handler makes. Without it none of this is reachable from a test — which is why three of these four issues shipped.New
--selftest-vibrance: 23 checks, 39 assertions, driven entirely by fakes. No GPU, no display, no driver.Every check was proven to fail against a targeted mutation, then restored byte-identically and hash-verified.
The one that matters most:
CheckNvidiaRestoreReachesRealCallSiteRegardlessOfGameScreenreflects into the real private staticOnWinEventHookrestore branch with_gameScreenforced to a display other than the event's. Reinstating this repo's exact gate — thereturnabove — turns it red, and it is the only one of 39 assertions that moves.That check exists because an earlier version of it couldn't fail: it called the restore method directly and argued the method takes no "current screen" parameter, which is true but irrelevant — the gate sits one line above the call. Two reviewers found that independently by putting the bug back and watching a fully green suite. Worth mentioning because the same trap is easy to fall into when reviewing this.
Two more checks needed real fixes during verification rather than re-runs, both environment-masked:
GetDesktopWindow()where the restore branch resolves fromGetForegroundWindow()— handles that usually coincide but needn't.About the second commit
The repo has no test project, so commit 2 adds the fixture and the flag. It is a separate commit specifically so you can drop it and take only the fixes.
--selftest-vibranceis placed early inMain, before adapter detection, so it needs neither vendor's driver.Verification and its limits — please read this part
Warnings unchanged.
masterbuilds with 2 (CS0659ResolutionModeWrapper.cs:8,CS0168WinEventHook.cs:202); this branch builds with the same 2, both configurations. Neither file is touched here.Fody/Costura weaving was never exercised, so the packaged single-file executable was not produced or tested. Fody 1.26.4 fails on VS2022's MSBuild with a Remoting/AppDomain assembly-resolution error — reproducible on a clean
mastercheckout, entirely unrelated to this change. Compilation was verified by skipping the weave step, which runs strictly after the compiler has emitted everything, so the warning comparison above is unaffected. That skip was achieved by a local modification to a gitignored package cache, not a supported option — it will not reproduce on your machine, and I mention it so the "clean build" claim above is read as covering compilation and the fixture only.The .NET 4.0 constraint is verified by inspection and API scan, not by compilation — this machine has no v4.0 reference assemblies, so builds use
-p:TargetFrameworkVersion=v4.8. The csproj still declaresv4.0and was not edited.None of the four issues has been confirmed resolved on a reporter's hardware. The mechanisms are proven from source and the behaviour from fixtures; the end-to-end fix is not.
Found but not fixed
Both proxies gate the entire
OnWinEventHookbody on_applicationSettings.Count > 0. Delete your last saved game while a display still owes a restore, and the next foreground event no-ops instead of draining it. That's a restore-stranding gap adjacent to #144 and #95, but it isn't one of the four mechanisms above and fixing it would widen this change. The fixture works around it with a deliberately non-matchingApplicationSettingwherever a check needs to reach the real restore branch.Notes for review
LogSafelyis added toProgram.cs(upstream'sVibranceGUI.LogcallsFile.AppendTextunguarded, which can throw across a native callback frame). My PR Stop resolution changes stranding the desktop and spamming dialogs (#114, #132) #159 adds an identical one — if both land it's a same-signature, same-body conflict, trivially resolved by keeping either._gameScreen = screeninside the "vibrance differs" gate. I kept that rather than widening when those fire, since it's out of scope here — at the cost of one duplicated resolve plus read-back, at foreground-event rate.affectPrimaryMonitorOnly— it writes only the game's display in both modes. Deliberately unchanged: fixing it would start putting game vibrance on every monitor for users with the box unchecked, which no issue asks for.AmdDynamicVibranceProxy.csgained a trailing newline it didn't have (whole-file rewrite). No semantic effect, flagged so it isn't a surprise in the diff.Happy to drop the fixture commit, split this up, or adjust anything else you'd prefer.