Skip to content

Draft: Introduce setBrightnessMultiplier() as better dimming API

Builds on top of !361 and prior commits in the chain. Part of the ongoing effort of allowing per-display brightness controls ( #19). CC @zamundaaa, @nclarius

Commit message

Prior to this commit, DimDisplay used to track brightness values of the only display it was told about by ScreenBrightnessController. When dimming stopped, the original brightness value was restored.

This doesn't work very well when multiple displays are present, each presumably with a different brightness value. DimDisplay will have an even harder time with edge cases such as restoring brightness values for a display that was temporarily unplugged.

ScreenBrightnessController is a better place for this complexity. It can exempt displays from dimming, reset the newly introduced brightness multiplier to 1.0 when a new display or external brightness change is detected, and pretend that brightness is still the same to avoid confusing other clients.

I'm a little uncomfortable with all the various flows within onDetectorDisplaysChanged(), and ideally we should add autotests for some of the expected scenarios, but at least it's all localized in one function. The rest is pretty straightforward.

CCBUG: 476550 (partial fix)

Bugs introduced

Great section to have in an MR, right?

This will behave poorly when self-induced brightness changes are detected as external brightness changes by a DisplayBrightness implementation. Reason for that being that an external brightness change will (1) adopt the externally changed brightness as the new 100% unmultiplied brightness, (2) reset dimming to 100% for all dimmable displays, and (3) dim it again later, but now down from the lower baseline that we incorrectly detected while dimming down.

In particular, this is bad for BacklightBrightness which has unreliable change event suppression. We can fix this by either completely owning the brightness value, in which case we don't care about external changes and just overwrite them again next time, or by making change event suppression robust at least for dimming animations. Let's start the latter by getting !342 reviewed and merged, and then I'll submit the follow-up.

We can remove draft status once that's fixed and I've done some more testing with more recent revisions of this MR.

Conceptually though, I attempted to keep the behavior largely similar to the current one in order to avoid regressions in general. In particular, the multiplier only affects the "legacy API" display set a.k.a. anything other than the display(s) you can already control with the brightness slider. The idea is that we keep our existing features and bugs for legacy display behavior, everything else only reacts to direct slider drags and don't participate in dimming whatsoever. In case anything goes wrong, one can fix the brightness without per-display controls available in the applet yet. We can expand the scope at a later point.

Test plan

Various combinations of the following:

  • Set brightness manually via applet or keyboard shortcut
  • Idle until the dimming timeout and check if it dims correctly
  • Use input devices to come back from dimming, check that the original brightness is back
  • Plug/unplug displays to confuse the code, ideally while dimmed, even better: during the dimming brightness animation
  • Change brightness via sysfs brightness file to confuse the code (e.g. SSH or sleep to write it while dimmed)

Not fixed, but related & making progress

  • CCBUG: 452492 (external screen brightness is not restored after wakeup)
    • Plan for later: Introduce an additional ScreenBrightnessController::setSuspended(bool), called from DPMS action, and restore correct (now available) dimming state/brightness upon setSuspended(true).
  • CCBUG: 448623 (Only laptop screen is dimmed, external monitors are not affected)
    • Plan for later: Set willUseMultiplier = true; also for external monitors once we have proper wake-up & brightness storage figured out (currently only set for the legacy API display set).
Edited by Jakob Petsovits

Merge request reports