Skip to content

Normalize combined brightness using estimated luminance - #83

Open
alexlee2046 wants to merge 1 commit into
didriksg:mainfrom
alexlee2046:fix/combined-brightness-normalization
Open

Normalize combined brightness using estimated luminance#83
alexlee2046 wants to merge 1 commit into
didriksg:mainfrom
alexlee2046:fix/combined-brightness-normalization

Conversation

@alexlee2046

Copy link
Copy Markdown

What & why

Combined Brightness currently treats slider percentages as comparable across every panel. They are not: Apple's built-in brightness control follows a strongly nonlinear user curve, monitors expose different peak luminance ranges, and Crisp adds a second gamma-dimming stage below 15% on external displays. Equal percentages can therefore produce visibly different luminance.

This change:

  • derives nominal SDR peak luminance for the built-in panel from CoreDisplay and for external monitors from IORegistry/EDID metadata
  • maps Combined Brightness through a shared estimated-nit scale and lets macOS convert the built-in linear target back to its native slider curve
  • accounts for Crisp's stacked DDC + gamma behavior in the low external range
  • uses the same mapping for Auto Brightness's absolute mode so it does not overwrite the calibrated result
  • adds a fine-tuning ratio for remaining panel/EDID differences, with Simplified Chinese localization
  • falls back to the existing proportional mapping when luminance metadata or the private linear-brightness API is unavailable

How tested

  • ./scripts/release.sh v0.0.0-ci — built and signed a universal arm64/x86_64 app and DMG targeting macOS 14
  • full app compile with warnings treated as errors using the command-line Swift toolchain
  • focused mapping harness covering forward/inverse nit conversion and the sub-15% DDC/gamma range
  • runtime smoke test with one Apple built-in panel and two DDC monitors: luminance metadata resolved, linear built-in writes read back correctly, and absolute auto-brightness converged on the same mapping
  • verified the compiled release app exposes the new setting and Simplified Chinese strings
  • added CombinedBrightnessMathTests; the full XCTest target is left to CI because this machine has Command Line Tools but not full Xcode

Checklist

  • Builds locally (./dev.sh, or ./scripts/release.sh v0.0.0-ci for the full release build)
  • Added/updated Crisp/Resources/Localizable.xcstrings for any new user-facing strings
  • Screenshot or short clip for any UI change

Normalize built-in and external displays on an estimated nit scale, use the built-in panel's linear brightness API, and account for Crisp's low-range DDC/gamma blend. Apply the same mapping to absolute auto-brightness, expose a fine-tuning ratio, and add focused math tests and localization.

@didriksg didriksg left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this on. Equal slider percentages really don't look equal across panels, and matching by estimated nits is the right idea. The math holds up: the forward and inverse mappings agree, the sub-15% DDC+gamma inverse is continuous at the boundary, and you registered the test file in project.yml and added the zh-Hans strings, which most first PRs miss.

A few things before I can merge:

  1. swiftlint --strict fails on _CoreDisplayCreateInfoDictionary (identifier_name), and CI lints strict. The dlsym'd names are allowlisted one by one in .swiftlint.yml under identifier_name.excluded; add this one there.

  2. The fallback in setBuiltinLinearBrightness (and the smooth variant) mixes domains. When DisplayServicesSetLinearBrightness doesn't resolve, linearBrightness * 100 goes into the nonlinear native-percent API, so the built-in lands visibly wrong instead of degrading to the old mapping. The description says it falls back to proportional in that case; make it actually do that. Simplest is to expose "linear API available" from BrightnessService and have builtinLinearTarget return nil when it isn't.

  3. Please reuse the existing IORegistry matching instead of a second walk. DDCService.buildAVServiceMapByProximity() already traverses the service plane and matches LegacyManufacturerID/ProductID/SerialNumber through DDCServiceMatcher, and its u32() reinterprets negative Int bit-patterns because real monitors return them. Your number() doesn't, so those monitors would silently never get a nits value. One shared path fixes both.

  4. DisplayInfo.init runs on the main actor and now does the full registry walk inline, per display, on every reconfiguration. loadDetails() in the same class offloads its expensive work for exactly this reason; the nits lookup should get the same treatment so a hot-plug doesn't hitch the panel.

  5. While you're in the absolute branch of AutoBrightnessService: clamp to display.maxBrightness, not 100. The old code had the same cap, but the relative branch right above documents why it's a trap (it drags an Extra Brightness display back down on every tick).

  6. Small ones: the new defaults key ends in .v2 but there's no v1, so drop the suffix before it ships; and pure logic like CombinedBrightnessMath lives in Models/ in this repo.

I'll approve the CI run once these land, and I'll verify on my own hardware (the built-in panel plus an HP 27fw over DDC) before merging, since brightness behavior only really gets proven live here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants