This revision was automatically updated to reflect the committed changes.
Closed by commit R108:82d286046907: [colorcorrection] Night Color - blue light
filter at nighttime (authored by romangg).
CHANGED PRIOR TO COMMIT
https://phabricator.kde.org/D5928?vs=23698=23749#toc
REPOSITORY
R108
romangg marked an inline comment as done.
REPOSITORY
R108 KWin
REVISION DETAIL
https://phabricator.kde.org/D5928
To: romangg, #kwin, graesslin
Cc: maspons, evpokp, HuShifang, jriddell, ngraham, leezu, behrmann, cfeck,
graesslin, davidedmundson, plasma-devel, kwin, bwowk, ZrenBot,
maspons added inline comments.
INLINE COMMENTS
> constants.h:81
> +1., 0.89576933, 0.79989606,
> +1., 0.90198230, 0.81465502, /* 5K */
> +1., 0.90963069, 0.82838210,
5000K ?
REPOSITORY
R108 KWin
REVISION DETAIL
https://phabricator.kde.org/D5928
graesslin accepted this revision.
This revision is now accepted and ready to land.
REPOSITORY
R108 KWin
REVISION DETAIL
https://phabricator.kde.org/D5928
To: romangg, #kwin, graesslin
Cc: evpokp, HuShifang, jriddell, ngraham, leezu, behrmann, cfeck, graesslin,
davidedmundson, plasma-devel,
romangg added inline comments.
INLINE COMMENTS
> graesslin wrote in constants.h:34-39
> What's the license for this? Looking at the linked readme I do not see any
> license. If the Redshift license applies it would be GPLv3+ which is
> something I have not allowed so far in KWin/Core as it
romangg marked an inline comment as done.
REPOSITORY
R108 KWin
REVISION DETAIL
https://phabricator.kde.org/D5928
To: romangg, #kwin, graesslin
Cc: evpokp, HuShifang, jriddell, ngraham, leezu, behrmann, cfeck, graesslin,
davidedmundson, plasma-devel, kwin, bwowk, ZrenBot, alexeymin,
romangg updated this revision to Diff 23698.
romangg marked 10 inline comments as done.
romangg added a comment.
- Cover comments,
- simplify sun timing calculation,
- merge current master
REPOSITORY
R108 KWin
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D5928?vs=20972=23698
graesslin requested changes to this revision.
graesslin added a subscriber: jriddell.
graesslin added a comment.
This revision now requires changes to proceed.
The general concept looks good to me.
INLINE COMMENTS
> constants.h:34-39
> +/* Whitepoint values for temperatures at 100K
subdiff marked 2 inline comments as done.
REPOSITORY
R108 KWin
REVISION DETAIL
https://phabricator.kde.org/D5928
To: subdiff, #kwin
Cc: ngraham, leezu, behrmann, cfeck, graesslin, davidedmundson, plasma-devel,
kwin, bwowk, ZrenBot, alexeymin, progwolff, lesliezhai, ali-mohamed, hardening,
subdiff marked 4 inline comments as done.
subdiff added inline comments.
INLINE COMMENTS
> subdiff wrote in nightcolor.cpp:111
> Well, in my tests logind always changed the value of this property way later
> then the notifier was triggered (like 1 second), so I assumed that this is
> always
subdiff marked an inline comment as done.
REPOSITORY
R108 KWin
REVISION DETAIL
https://phabricator.kde.org/D5928
To: subdiff, #kwin
Cc: ngraham, leezu, behrmann, cfeck, graesslin, davidedmundson, plasma-devel,
kwin, bwowk, ZrenBot, alexeymin, progwolff, lesliezhai, ali-mohamed, hardening,
subdiff marked an inline comment as done.
REPOSITORY
R108 KWin
REVISION DETAIL
https://phabricator.kde.org/D5928
To: subdiff, #kwin
Cc: ngraham, leezu, behrmann, cfeck, graesslin, davidedmundson, plasma-devel,
kwin, bwowk, ZrenBot, alexeymin, progwolff, lesliezhai, ali-mohamed, hardening,
subdiff updated this revision to Diff 20972.
subdiff added a comment.
Ifdef Linux specific time change detection code.
REPOSITORY
R108 KWin
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D5928?vs=20971=20972
REVISION DETAIL
https://phabricator.kde.org/D5928
AFFECTED FILES
subdiff updated this revision to Diff 20971.
subdiff added a comment.
- Merge current master
- Ifdef Linux only code for time change detection
REPOSITORY
R108 KWin
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D5928?vs=14916=20971
REVISION DETAIL
subdiff added a comment.
In https://phabricator.kde.org/D5928#154584, @davidedmundson wrote:
> If you're busy, tell me and I'll make those minor changes I want and follow
this up.
Thanks for the offer, but I should be able to do it myself early enough
before next release. Would
davidedmundson added a comment.
If you're busy, tell me and I'll make those minor changes I want and follow
this up.
REPOSITORY
R108 KWin
REVISION DETAIL
https://phabricator.kde.org/D5928
To: subdiff, #kwin
Cc: ngraham, leezu, behrmann, cfeck, graesslin, davidedmundson, plasma-devel,
subdiff added a comment.
In https://phabricator.kde.org/D5928#154362, @ngraham wrote:
> What's the status of this patch?
There is some minor problem @davidedmundson pointed to. When I've finished
the thesis I currently need to work on, I'll look into that, rebase on current
ngraham added a comment.
What's the status of this patch?
REPOSITORY
R108 KWin
REVISION DETAIL
https://phabricator.kde.org/D5928
To: subdiff, #kwin
Cc: ngraham, leezu, behrmann, cfeck, graesslin, davidedmundson, plasma-devel,
kwin, bwowk, ZrenBot, alexeymin, progwolff, lesliezhai,
behrmann added a comment.
In https://phabricator.kde.org/D5928#112398, @subdiff wrote:
> In https://phabricator.kde.org/D5928#112368, @cfeck wrote:
>
> > First, the difference between "color management" and "color correction"
is that color correction is only one step in the color
cfeck added a comment.
Yes, let's not argue about the names. I just wanted to make sure you
understand the difference between 1D and 3D LUTs, and why color management
(should it ever come back to KWin), needs 3D LUTs.
REPOSITORY
R108 KWin
REVISION DETAIL
subdiff added a comment.
In https://phabricator.kde.org/D5928#112368, @cfeck wrote:
> First, the difference between "color management" and "color correction" is
that color correction is only one step in the color management workflow. KWin
would need (again) be able to apply color
cfeck added a comment.
In https://phabricator.kde.org/D5928#112364, @subdiff wrote:
> In https://phabricator.kde.org/D5928#112359, @cfeck wrote:
>
> > http://www.color.org/faqs.xalter
>
>
> Sorry, but I can't find the relevant part here. Can you quote it?
I wanted you to
subdiff added a comment.
In https://phabricator.kde.org/D5928#112359, @cfeck wrote:
> http://www.color.org/faqs.xalter
Sorry, but I can't find the relevant part here. Can you quote it?
REPOSITORY
R108 KWin
REVISION DETAIL
https://phabricator.kde.org/D5928
To: subdiff, #kwin
cfeck added a comment.
In https://phabricator.kde.org/D5928#112357, @subdiff wrote:
> In https://phabricator.kde.org/D5928#112339, @cfeck wrote:
>
> > The difference between gamma correction and color correction is that the
former only uses three 1D LUTs, while the latter can use
subdiff added inline comments.
INLINE COMMENTS
> davidedmundson wrote in nightcolor.cpp:111
> How confusing. In any case we have a race condition.
>
> We're triggering this on timeChangedFd so we have behaviour that basically
> changes depending on whether logind has processed resuming and
subdiff added a comment.
In https://phabricator.kde.org/D5928#112339, @cfeck wrote:
> The difference between gamma correction and color correction is that the
former only uses three 1D LUTs, while the latter can use three 3D LUTs. In
other words, gamma correction cannot tint your blue
subdiff added a comment.
In https://phabricator.kde.org/D5928#111959, @davidedmundson wrote:
> As for your comments:
>
> > The first one is, that the fading phases generate much DBus traffic, as
you said. To solve this we would need to do the fading in KWin. For that we
need timers,
davidedmundson added inline comments.
INLINE COMMENTS
> subdiff wrote in nightcolor.cpp:111
> Yes: https://www.freedesktop.org/wiki/Software/systemd/logind/
>
> It's true between signal `PrepareForSleep` is emitted with arg `true` and
> after sleep again with `false`.
How confusing. In any
subdiff added inline comments.
INLINE COMMENTS
> davidedmundson wrote in nightcolor.cpp:52
> rename the class or the file
Is there a general rule to it? Can I call the class NightColorManager instead
and leave the file name untouched?
Is this about the generic class name or about the class
subdiff updated this revision to Diff 14916.
subdiff marked 2 inline comments as done.
subdiff added a comment.
- No m_platform in ColorCorrect::Manager
- Dbus iface methods renamed
- Give GammaRamp object to DrmCrtc (and use its ramp size)
- Separate GammaRamp struct for better includes
cfeck added a comment.
I agree that this code should not talk about night mode at all. Indeed we
only need code in KWin to control the gamma LUTs per screen, and maybe even
compute the LUTs from a number of gamma/contrast/brightness control points so
that we do not have to transfer a lot of
cfeck added a comment.
In https://phabricator.kde.org/D5928#112334, @subdiff wrote:
> In https://phabricator.kde.org/D5928#111282, @graesslin wrote:
>
> > I wouldn't call it color correction as we used to have a mechanism called
color correction in the past which did something very
subdiff added a comment.
In https://phabricator.kde.org/D5928#111282, @graesslin wrote:
> I wouldn't call it color correction as we used to have a mechanism called
color correction in the past which did something very different. Personal
suggestion: gamma correction or color temperature
davidedmundson added a comment.
As for your comments:
> The first one is, that the fading phases generate much DBus traffic, as you
said. To solve this we would need to do the fading in KWin. For that we need
timers, at least one. But it would make sense to have two timers, one for
davidedmundson added inline comments.
INLINE COMMENTS
> nightcolor.cpp:52
> +
> +Manager::Manager(Platform *platform)
> +: QObject(platform),
rename the class or the file
> nightcolor.cpp:111
> +if (reply.isValid()) {
> +comingFromSuspend = reply.value().toBool();
> +
graesslin added a comment.
I wouldn't call it color correction as we used to have a mechanism called
color correction in the past which did something very different. Personal
suggestion: gamma correction or color temperature correction.
Sorry cannot do a full review for the next few
subdiff added a dependent revision: D5931: ColorCorrect Library - for
configuring KWin's native color correction (in particular Night Color).
REPOSITORY
R108 KWin
REVISION DETAIL
https://phabricator.kde.org/D5928
To: subdiff, #kwin
Cc: davidedmundson, plasma-devel, kwin, ZrenBot, spstarr,
subdiff added a comment.
In https://phabricator.kde.org/D5928#110990, @davidedmundson wrote:
> In my opinion, and the impression you gave me when we talked about the
design.
>
> We shouldn't have Night mode stuff in kwin.
> We should have generic colour correction in kwin (albeit
davidedmundson added a comment.
In my opinion, and the impression you gave me when we talked about the
design.
We shouldn't have Night mode stuff in kwin.
We should have generic colour correction in kwin (albeit with fading
happening in kwin to reduce traffic)
I guess this
subdiff created this revision.
subdiff added projects: KWin, Plasma on Wayland.
Restricted Application added subscribers: kwin, plasma-devel.
REVISION SUMMARY
Introduction
With Wayland KWin needs to provide certain services, which were provided
before that by the Xserver.
40 matches
Mail list logo
