[colorcorrection] Add "Constant" mode
ClosedPublic

Authored by zzag on Jun 21 2019, 8:58 AM.

Details

Summary

Currently, the Night Color manager supports three operation modes:

  • Automatic: in this mode, screen color temperature is computed based on

the current position of the Sun. In order to calculate sunrise & sunset
times, the manager needs coordinates of the user, which are provided by
Plasma;

  • Location: this mode is very similar to the Automatic, except one minor

detail: user needs to provide his/her/their location. This mode can be
very useful if coordinates provided by Plasma are incorrect;

  • Timings: unfortunately we can't compute timings of the Sun for people

living near Earth poles. This mode allows the user to specify timings of
sunrise and sunset as well the transition time.

This change introduces another mode, called Constant. With this mode the
screen color temperature is constant throughout the day. The new mode
can be useful for people wishing constant screen color temperature or
just for people living near Earth's North or South poles.

Diff Detail

Repository
R108 KWin
Branch
native-redshift
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 13056
Build 13074: arc lint + arc unit
zzag created this revision.Jun 21 2019, 8:58 AM
Restricted Application added a project: KWin. · View Herald TranscriptJun 21 2019, 8:58 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Jun 21 2019, 8:58 AM
zzag updated this revision to Diff 60734.Jun 27 2019, 4:22 PM

Grammar

zzag updated this revision to Diff 60736.Jun 27 2019, 4:25 PM

Grammar

zzag updated this revision to Diff 60737.Jun 27 2019, 4:28 PM
  • Grammar
  • Typo (compused -> computed)
romangg added a subscriber: romangg.EditedJun 27 2019, 4:36 PM

The idea for constant changes to screen temperature was to have this in a separate facility for generic color correction. This would then allow to:

  • Change overall color temperature, brightness and contrast
  • Or change gamma curves directly.

Like KGamma. The constant change would be the baseline and could then be multiplied by Night Color values. A separate mode to Night Color does not allow that. And it is in my opinion also structurally not the place for it.

If you want to do constant color adjustment create a new class for it independent of NightColor and create a method to combine these input variables into unique ramps sent to crtcs.

EDIT: I have some branches from last year also with changes to Plasma for allowing per display gamma curve adjustments. But this needs integration of wlroot's gamma protocol or something else to hand the ramps over via Wayland protocols. Different color correction techniques like temperature then become a concern of the desktop.

I fully support a better split of colour correct and night mode, with nightmode being an interface into colour correction.
I didn't like really "nightmode" being an explicit part of kwin.

However imho we still want a constant toggle on the nightmode public api to integrate into something that can be feature complete with the current redshift plasmoid. Having that go into a different api would be messy and have potential conflicts at runtime.

I fully support a better split of colour correct and night mode, with nightmode being an interface into colour correction.
I didn't like really "nightmode" being an explicit part of kwin.

I didn't say Night Color should be an interface only per se. Currently we do the timing calculation in KWin to always have the correct timings and color values at hand immediately without flashing frames with wrong temperature. To do this out of process we would need to block KWin's present on hardware until the temperature supplying process gives us the current value. Another aspect is when temperature changes over time. Do we have a timer in KWin or do we send progressively new values from the other process?

But it's worth investigating. Sun calculations don't really belong in a window compositor.

However imho we still want a constant toggle on the nightmode public api to integrate into something that can be feature complete with the current redshift plasmoid. Having that go into a different api would be messy and have potential conflicts at runtime.

The redshift plasmoid targeted X and imo feature completeness with it shouldn't be a development target for us. The constant factor you can apply there is just for convenience on X because you never know what other ui you have in the DE the user runs for doing such things. In our Wayland session we can do this in an integrated way once instead of being spread over several little tools. And to be honest there is not much stopping us from doing it the same way on X11. KGamma just sucks.

zzag added a comment.Jun 27 2019, 5:25 PM

While I agree that splitting the night color manager into two entities makes sense, the requested changes are orthogonal to changes this patch introduces. No matter how you turn it, we need a switch to turn on/off the constant mode.

Generic color correction manager should only deal with setting 1D/3D LUTs (3D LUTs is a topic for another discussion), gamma, and so on. Screen color temperature is night color manager's abstraction.

In D21948#487391, @zzag wrote:

While I agree that splitting the night color manager into two entities makes sense, the requested changes are orthogonal to changes this patch introduces.

How are they orthogonal? You want constant color temperature adjustment independent of time and I tell you that's not a part of Night Color, because Night Color as the name says deals with a specific time of day: night, which is time dependent and not constant throughout day. That you have to add several conditionals to turn off timers is just one sign that a constant mode is misplaced here.

No matter how you turn it, we need a switch to turn on/off the constant mode.

First off, I turn nothing. I'm neither in a defensive position nor am I trying to engage in sophistries. And reading your sentence it is still not clear to me why we "need" a switch.

Generic color correction manager should only deal with setting 1D/3D LUTs (3D LUTs is a topic for another discussion), gamma, and so on. Screen color temperature is night color manager's abstraction.

Color temperature is a notion independent of Night Color. Yes, generic color correction shouldn't deal with it in KWin, but it's probably a good idea to expose some slider to ui in plasma-desktop next to other sliders for contrast, brightness and so on. Then compute gamma ramps out of it as we do in KWin for Night Color, send these ramps to KWin and further process them here in the "generic color correction manager" as gamma ramps and not as color temperature value.

And LUTs from my understanding make sense for color management or professional grading. The goal for us is at the moment to just support some simple color correction that 95% of users can understand. That can be done with simple mathematical operations on the gamma ramps in plasma-desktop, then these ramps being sent to KWin. And advanced users could still edit the ramps manually in config files.

There is a task for color/gamma correction already open for a long time: T4465. It contains some more information, which you can enhance or directly work on the task. We should have also discussed the idea of the proposed change in there beforehand.

You're right, lets step it back to discussing purely in terms of UX, get on the same page then that will direct the code.

I used to be a redshift user when my laptop was on X, I used it at night - but I have the sleep schedule consistency of a typical programmer. I don't want the screen to dim at 9pm if I'm still going to have a halogen desk lamp shining on full blast anyway. I only ever used manual invoking as times weren't consistent and the location of the sun was even less relevant!

I find myself not currently using the wayland equivalent.

We know that the plasma-redshift users also had demand to have a simpler toggle which isn't time based and the similar function "LiveDisplay" on my phone also has a toggle of "Off/Night/Automatic".
We never set out purely to make a clone, but we should learn from what others set out to solve.

zzag added a comment.Jun 28 2019, 7:42 AM

How are they orthogonal?

You're asking to redesign color correction infrastructure while this patch adds sort of unrelated feature.

I tell you that's not a part of Night Color, because Night Color as the name says deals with a specific time of day: night,

This argument is not convincing. The Night Color manager is a blue light filter. If the name is main blocker, let's rename the manager. "Blue Light Filter" is a good candidate, it's generic and accurate.

And reading your sentence it is still not clear to me why we "need" a switch.

Because the Night Color manager has to be responsible for management of screen color temperature and all three existing modes are dynamic.

Introducing separate path for the constant mode is a bad idea because two actors will be involved in management of color temperature, which means more complexity. Managing those two actors will make life harder for us developers and for clients. On server side, we would have to take some extra measures to ensure that there are no conflicts between the Night Color manager and the constant mode implementation. On client side, it means two different apis to interact with.

Another problem with two paths is that it just doesn't make sense conceptually...

You're right, lets step it back to discussing purely in terms of UX, get on the same page then that will direct the code.

Okay, that's a good idea.

You're right, lets step it back to discussing purely in terms of UX, get on the same page then that will direct the code.

I used to be a redshift user when my laptop was on X, I used it at night - but I have the sleep schedule consistency of a typical programmer. I don't want the screen to dim at 9pm if I'm still going to have a halogen desk lamp shining on full blast anyway. I only ever used manual invoking as times weren't consistent and the location of the sun was even less relevant!

I find myself not currently using the wayland equivalent.

Hmm, when people wanted this functionality in the past it seemed to me more like they wanted to use it as a workaround for white-point balancing their screen. You use it manually like this because you think an automatic mode depending on time does not suit your work habits particularly well, right? I imagine it to be kind of cumbersome to do it always like this, but it's still a valid use case.

We know that the plasma-redshift users also had demand to have a simpler toggle which isn't time based and the similar function "LiveDisplay" on my phone also has a toggle of "Off/Night/Automatic".
We never set out purely to make a clone, but we should learn from what others set out to solve.

If other major OS feature it already in their Ui, that's a good argument for providing it as well. But Android does not allow color correcting the screen in general, right? What I don't want is to present basically the same functionality in two different ways: a constant color temperature or white point balancing slider should be part of generic color correction in a Display Colors KCM and if people can achieve something similar via Night Color I feel there will be confusion on how a user is supposed to do it (imagine a user wants to adjust white-balance for a faulty screen and then doing it via Night Color constant mode, later connecting a non-faulty second screen which then has wrong colors because of Night Color setting). Still a constant mode with quick toggle on/off could make sense if people really then only use it like you described yourself used redshift in the past. On the other side we don't need to support every 1% use case. For example GNOME's Night Light does not feature a constant mode.

I think we first should have generic color correction Ui (including white point balancing) in place. And then we can decide with user feedback if an additional constant Night Color mode makes sense. With generic color correction in place it would then also be less tempting (and we could design the Ui in such a way) to use Night Color constant mode for makeshift white point balancing.

I think it makes sense to have a "constant/on demand" mode here irrespective of what we do for full-screen color correction. That's useful (in fact critical) too, but not connected to the desire to turn on Night color on or off manually. +1 for @zzag's approach proposed in this patch.

zzag added a comment.Jun 28 2019, 10:12 PM

For example GNOME's Night Light does not feature a constant mode.

Yes, that's correct! Though you can make Night Light be always on by adjusting "from" and "to" times. See https://gitlab.gnome.org/GNOME/gnome-settings-daemon/merge_requests/6

I think I see where the difference of opinion is coming from.

If constant means "always on", at which point I would 100% fully agree with you.

I'm coming from it (and I assume vlad too) from the assumption that we'll also have 409083 (and whatever other UI) implemented at some point.

Then it's got the same goal, avoiding having two UIs to do what if effectively the same thing, regardless of whether you have a slow colour fade or an explicit one.

Any chance we can come to some agreement on this?

davidedmundson accepted this revision.Jul 21 2019, 1:46 PM

As discussed, approved.

This revision is now accepted and ready to land.Jul 21 2019, 1:46 PM

I'm still for exposing the Ui only after a generic color management Ui is available to not misguide users in thinking this is a way to do generic color correction.

I don't think users will make that mistake. Night Color is very clearly a RedShift-type thing, not a generic color correction tool. If we need to make this even clearer, I'd suggest adding a label in the KCM that explains what it does and what it's for.

This revision was automatically updated to reflect the committed changes.
This comment was removed by lavender.