Fix concurrency bugs, DDC reliability, crashes, and memory leaks#1874
Open
MyronKoch wants to merge 1 commit into
Open
Fix concurrency bugs, DDC reliability, crashes, and memory leaks#1874MyronKoch wants to merge 1 commit into
MyronKoch wants to merge 1 commit into
Conversation
Thread safety: - Fix swBrightnessSemaphore misuse causing races in software brightness - Signal semaphore before async dispatch to prevent deadlock with polling job - Wrap CGSetDisplayTransferByTable in DispatchQueue.main.sync for thread safety - Wrap shade alpha updates in DispatchQueue.main.async for AppKit safety - Move DDC probe work off main thread with configureID staleness guard - Defer gamma interference alert to next run loop iteration DDC reliability: - Only persist DDC write state after confirming hardware write succeeded - Clear stale arm64avService handles before rematching on wake/reconfig - Validate DDC/CI reply header fields before accepting read values - Cap DDC retry attempts at 30 to prevent multi-minute startup hangs - Guard against division by zero in DDC value normalization Crash fixes: - Replace force-unwrap on currentDisplay in MenuHandler - Handle gamma table acquisition failure safely - Guard against zero defaultGammaTablePeak in getSwBrightness Cleanup: - Remove shadeGrave memory leak (closed windows retained forever) - Remove dead sortDisplays() with stray Turkish comments - Remove duplicate audioSpeakerVolume DDC default assignment - Remove tautological self-comparisons in SliderHandler - Replace deprecated SMLoginItemSetEnabled with SMAppService on macOS 13+ - Replace deprecated kIOMasterPortDefault with kIOMainPortDefault on macOS 12+ - Replace direct plist read with UserDefaults.persistentDomain API Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR is a broad cleanup/hardening pass for MonitorControl that addresses thread-safety hazards in software brightness/gamma handling, crash-prone force-unwraps, DDC read/write reliability, and several memory/code-hygiene issues, while modernizing a few deprecated APIs.
Changes:
- Thread/concurrency hardening for software brightness, shade updates, gamma table writes, and the configure pipeline (with
configureID/reconfigureID/sleepIDstaleness guards moving DDC setup off the main thread). - DDC reliability fixes: persist write state only after success, validate Arm64 DDC reply header, cap retries at 30, clear stale
arm64avServicehandles before rematching, guard divide-by-zero in DDC normalization. - Removals/replacements: dead
sortDisplays()andshadeGraveleak gone, tautologicalself.percentageBox == self.percentageBoxchecks removed, duplicate audio default removed,SMLoginItemSetEnabled→SMAppServiceandkIOMasterPortDefault→kIOMainPortDefault(one site),getSystemSettingsswitched from direct plist read toUserDefaults.persistentDomain.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| MonitorControl/Support/SliderHandler.swift | Drops two tautological self.percentageBox == self.percentageBox guards. |
| MonitorControl/Support/MenuHandler.swift | Replaces currentDisplay! force-unwrap with optional-aware filtering for "relevant" sliders. |
| MonitorControl/Support/DisplayManager.swift | Removes shadeGrave leak and dead sortDisplays(); clears stale arm64 service handles before rematching. |
| MonitorControl/Support/Arm64DDC.swift | Validates DDC reply header, caps retries at 30, switches to kIOMainPortDefault on macOS 12+. |
| MonitorControl/Support/AppDelegate.swift | Adds configureID staleness guard and moves setupOtherDisplays to a background queue; migrates login-item registration to SMAppService; rewrites getSystemSettings via UserDefaults. |
| MonitorControl/Model/OtherDisplay.swift | Only persists writeDDCLastSavedValue/isTouched after the DDC write actually succeeds; guards divide-by-zero in DDC value (de)normalization; drops duplicate audio default. |
| MonitorControl/Model/Display.swift | Handles CGGetDisplayTransferByTable failure, guards zero defaultGammaTablePeak, signals swBrightnessSemaphore before async smooth dispatch, wraps shade/gamma updates with DispatchQueue.main.async/sync, defers gamma-interference alert. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+244
to
+246
| DispatchQueue.main.sync { | ||
| CGSetDisplayTransferByTable(self.identifier, self.defaultGammaTableSampleCount, gammaTableRed, gammaTableGreen, gammaTableBlue) | ||
| } |
Comment on lines
+342
to
344
| func getSystemSettings() -> [String: Any]? { | ||
| UserDefaults.standard.persistentDomain(forName: ".GlobalPreferences") | ||
| } |
Comment on lines
+103
to
+104
| let attempts = max(1, Int(min(numOfRetryAttemps ?? 4, UInt8(30)))) | ||
| for _ in 0 ..< attempts { |
Comment on lines
+326
to
+339
| if #available(macOS 13.0, *) { | ||
| let service = SMAppService.loginItem(identifier: identifier) | ||
| do { | ||
| if enabled { | ||
| try service.register() | ||
| } else { | ||
| try service.unregister() | ||
| } | ||
| } catch { | ||
| os_log("Unable to update login item registration: %{public}@", type: .error, error.localizedDescription) | ||
| } | ||
| } else { | ||
| SMLoginItemSetEnabled(identifier as CFString, enabled) | ||
| } |
Comment on lines
+103
to
+104
| let attempts = max(1, Int(min(numOfRetryAttemps ?? 4, UInt8(30)))) | ||
| for _ in 0 ..< attempts { |
Comment on lines
229
to
+251
| if smooth { | ||
| self.swBrightnessSemaphore.signal() | ||
| DispatchQueue.global(qos: .userInteractive).async { | ||
| for transientValue in stride(from: currentValue, to: newValue, by: 0.005 * (currentValue > newValue ? -1 : 1)) { | ||
| guard app.reconfigureID == 0 else { | ||
| self.swBrightnessSemaphore.signal() | ||
| return | ||
| } | ||
| if self.isVirtual || self.readPrefAsBool(key: .avoidGamma) { | ||
| _ = DisplayManager.shared.setShadeAlpha(value: 1 - transientValue, displayID: DisplayManager.resolveEffectiveDisplayID(self.identifier)) | ||
| DispatchQueue.main.async { | ||
| _ = DisplayManager.shared.setShadeAlpha(value: 1 - transientValue, displayID: DisplayManager.resolveEffectiveDisplayID(self.identifier)) | ||
| } | ||
| } else { | ||
| let gammaTableRed = self.defaultGammaTableRed.map { $0 * transientValue } | ||
| let gammaTableGreen = self.defaultGammaTableGreen.map { $0 * transientValue } | ||
| let gammaTableBlue = self.defaultGammaTableBlue.map { $0 * transientValue } | ||
| CGSetDisplayTransferByTable(self.identifier, self.defaultGammaTableSampleCount, gammaTableRed, gammaTableGreen, gammaTableBlue) | ||
| DispatchQueue.main.sync { | ||
| CGSetDisplayTransferByTable(self.identifier, self.defaultGammaTableSampleCount, gammaTableRed, gammaTableGreen, gammaTableBlue) | ||
| } | ||
| } | ||
| Thread.sleep(forTimeInterval: 0.001) // Let's make things quick if not performed in the background | ||
| } | ||
| } | ||
| return true |
Comment on lines
+244
to
+246
| DispatchQueue.main.sync { | ||
| CGSetDisplayTransferByTable(self.identifier, self.defaultGammaTableSampleCount, gammaTableRed, gammaTableGreen, gammaTableBlue) | ||
| } |
Comment on lines
+325
to
+339
| let identifier = "\(Bundle.main.bundleIdentifier!)Helper" | ||
| if #available(macOS 13.0, *) { | ||
| let service = SMAppService.loginItem(identifier: identifier) | ||
| do { | ||
| if enabled { | ||
| try service.register() | ||
| } else { | ||
| try service.unregister() | ||
| } | ||
| } catch { | ||
| os_log("Unable to update login item registration: %{public}@", type: .error, error.localizedDescription) | ||
| } | ||
| } else { | ||
| SMLoginItemSetEnabled(identifier as CFString, enabled) | ||
| } |
Comment on lines
+239
to
+245
| let masterPort: mach_port_t | ||
| if #available(macOS 12.0, *) { | ||
| masterPort = kIOMainPortDefault | ||
| } else { | ||
| masterPort = kIOMasterPortDefault | ||
| } | ||
| let ioregRoot: io_registry_entry_t = IORegistryGetRootEntry(masterPort) |
Comment on lines
+403
to
426
| var writeAttempted = false | ||
| var writeSucceeded = true | ||
| for controlCode in controlCodes { | ||
| if Arm64DDC.isArm64 { | ||
| if self.arm64ddc { | ||
| _ = Arm64DDC.write(service: self.arm64avService, command: controlCode, value: value) | ||
| writeAttempted = true | ||
| writeSucceeded = Arm64DDC.write(service: self.arm64avService, command: controlCode, value: value) | ||
| } | ||
| } else { | ||
| _ = self.ddc?.write(command: controlCode, value: value, errorRecoveryWaitTime: 2000) ?? false | ||
| if self.ddc != nil { | ||
| writeAttempted = true | ||
| writeSucceeded = self.ddc?.write(command: controlCode, value: value, errorRecoveryWaitTime: 2000) ?? false | ||
| } | ||
| } | ||
| if writeAttempted, !writeSucceeded { | ||
| break | ||
| } | ||
| } | ||
| if writeAttempted, writeSucceeded { | ||
| self.writeDDCQueue.async(flags: .barrier) { | ||
| self.writeDDCLastSavedValue[command] = value | ||
| self.savePref(true, key: PrefKey.isTouched, for: command) | ||
| } | ||
| } |
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.
Summary
Comprehensive fix pass addressing thread safety, DDC communication reliability, crash-inducing edge cases, and resource leaks.
Thread Safety
DDC Reliability
Crash Fixes
Cleanup
Testing
Tested on M3 Max with 4 displays (built-in + 3 external TVs via HDMI/DisplayLink). Verified smooth brightness, sleep/wake, slider responsiveness. All surgical fixes, no new features or UI changes.