From kwin Mon Aug 27 16:04:15 2012 From: =?utf-8?q?Thomas_L=C3=BCbking?= Date: Mon, 27 Aug 2012 16:04:15 +0000 To: kwin Subject: Re: Review Request: Implement color correction (per output) Message-Id: <20120827160415.12215.30901 () vidsolbach ! de> X-MARC-Message: https://marc.info/?l=kwin&m=134608347515522 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============5625015705345819016==" --===============5625015705345819016== Content-Type: multipart/alternative; boundary="===============4641385099147814806==" --===============4641385099147814806== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable > On Aug. 25, 2012, 2:58 p.m., Fredrik H=C3=B6glund wrote: > > kwin/libkwineffects/kwinglcolorcorrection.cpp, line 672 > > > > > > I'm pretty sure you want GL_CLAMP_TO_EDGE here. > = > Casian Andrei wrote: > Thanks, who knows how much time would have taken me to figure this ou= t. (I took the code from CompICC without looking on the documentation, thin= king it's correct for this case). FYI: it's a SUCH popular mistake that the nvidia driver until now (conditio= nally) took GL_CLAMP as GL_CLAMP_TO_EDGE Since the 300 drivers there's a GUI option for this (defaults to "on" ie. y= ou suddenly got clamping errors (black lines) all over the place ;-) = > On Aug. 25, 2012, 2:58 p.m., Fredrik H=C3=B6glund wrote: > > kwin/libkwineffects/kwinglutils.cpp, line 322 > > > > > > You can't do this unconditionally. KWin has shaders that operate on= pixels that have already been color corrected. > = > Casian Andrei wrote: > This is a tricky issue. > = > I guess the applications should use the X Color Management stuff to s= pecify which regions are color corrected and if there are some that don't n= eed any correction. If they do that, Kolor Server will see it and update th= e regions that will end up in KWin. So for areas not to be corrected by KWi= n, a dummy lookup table will be used for correction, sRGB -> sRGB (basicall= y does nothing). > = > If the applications don't use the X Color Management and color correc= tion is enabled in KWin too, then I think it sucks. > = > Perhaps my mentor could give details :/ I don't think this is what Fredrik meant - some shaders will operate on a p= reviously buffered (and color corrected) output. If you inject color correc= tion here as well, you'll "correct" twice. - Thomas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106141/#review17979 ----------------------------------------------------------- On Aug. 23, 2012, 2:56 p.m., Casian Andrei wrote: > = > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106141/ > ----------------------------------------------------------- > = > (Updated Aug. 23, 2012, 2:56 p.m.) > = > = > Review request for kwin, Thomas L=C3=BCbking and Martin Gr=C3=A4=C3=9Flin. > = > = > Description > ------- > = > These are the results of my GSoC project, for KWin. > = > This color correction needs KolorManager installed, 'kded' branch. That o= ne needs Oyranos. > git://anongit.kde.org/kolor-manager.git http://www.oyranos.org/do= wnloads > If it's not installed, the ColorCorrection class should fail without caus= ing any issues to the user (that's if it's enabled from the KCM). Probably = a notification could be appropriate in that case? "Failed to initialize CC = because cannot contact Kolor-Server KDED..." something like that? > = > Change summary: > -- > Add an option to kcmcompositing in the 'Advanced' tab, to enable or disab= le color correction. It is specified that it's experimental and it needs Ko= lor Manager. This implies some small changes to options.h and cpp. > = > In composite.cpp, the ColorCorrection class instance is initialized or cl= eaned up (it's like being owned by the compositor). In workspace, there's a= signal connected to reset compositing if the color correction setting is c= hanged. > = > Scene::finalPaintWindow code is moved into SceneOpenGL::performPaint. > SceneOpenGL has the declarations of some methods changed to include the s= creen number - those could be changed to pass around PaintData's probably. = SceneOpenGL's newly implemented finalDrawWindow splits the rendering if col= or correction is enabled and there are multiple screens. Before painting fo= r a particular screen, ColorCorrection::setupForOutput should be called. > = > There was a problem with the blending function causing artifacts with col= or correction enabled so it is set up to src_alpha, one_minus_src alpha in = that case. > = > Now, inside KWinEffects: > -- > A screen property is added for WindowPaintData. > In kwinglutils, The fragment shaders are intercepted before being compile= d and they get a couple of lines of code inserted in order to do the color = stuff. This happens only when color correction is enabled, of course. > = > And there is the big ColorCorrection class. The public and private stuff = are well separated. More additions to the public stuff shouldn't be necessa= ry in the future as far as I see it. Everything is there I think. The regio= n stuff is not used at the moment, and the 2 reset methods aren't used eith= er (for now). > = > The private stuff consists of the private data and the D-Bus interface. R= egarding D-Bus, everything is async (I think). > = > The implementation basically manages a set of color lookup tables for dif= ferent outputs and for different window regions. These are taken via D-Bus.= Each lookup table has around 700 KB. I hope I am not wrong in assuming it'= s not that big of an issue time-wise, since they are transferred only once = or twice for an entire session. I hope D-Bus doesn't do any time consuming = business to transfer those QVector's. > = > There is an issue with OpenGL ES, apparently it doesn't support 3D textur= es (probably it needs an extension or something). Until we envision a clear= solution, I have disabled the color correction for OpenGL ES. > = > In case of problems, it should not affect the rest of KWin in any way. > = > Next priorities: > -- > Implement per-window-region stuff. It should be a matter of extending Kol= orServer's capabilities and doing some small modifications in SceneOpenGL::= Window. The ColorCorrection class from KWinEffects has everything prepared. > = > Fix issues with transparency / some weird colors. > = > -- > There's another thing... I don't know when I'm supposed to be able to do = improvements and modifications, since the GSoC project ended. Probably if I= keep the new modifications well separated, there should be no issues. > = > = > Diffs > ----- > = > kwin/composite.cpp c65716b = > kwin/kcmkwin/kwincompositing/main.cpp 7a89db4 = > kwin/kcmkwin/kwincompositing/main.ui cff0f63 = > kwin/libkwineffects/CMakeLists.txt 14a2747 = > kwin/libkwineffects/kwineffects.h 2c2f7bf = > kwin/libkwineffects/kwineffects.cpp bae85e7 = > kwin/libkwineffects/kwinglcolorcorrection.h PRE-CREATION = > kwin/libkwineffects/kwinglcolorcorrection.cpp PRE-CREATION = > kwin/libkwineffects/kwinglcolorcorrection_p.h PRE-CREATION = > kwin/libkwineffects/kwinglutils.h 7a7c3c9 = > kwin/libkwineffects/kwinglutils.cpp 016d31e = > kwin/options.h 9e4fcc6 = > kwin/options.cpp 72f168d = > kwin/scene.h 3891198 = > kwin/scene.cpp 5782510 = > kwin/scene_opengl.h de33ce4 = > kwin/scene_opengl.cpp cbcc0ca = > kwin/workspace.cpp cf3f308 = > = > Diff: http://git.reviewboard.kde.org/r/106141/diff/ > = > = > Testing > ------- > = > Restarted KDED making sure Kolor-Manager is installed properly - this sho= uld make the KDED module available. > = > Restarted KWin, enabled color correction in KCM compositing, advanced tab. > = > Played a bit with commands like: > oyranos-monitor -d 0 XYZ.icc > oyranos-monitor -d 1 Lab.icc > Those set up particular profiles for the monitors. However, it seems ther= e is a problem with reverting to the normal ones. The code regarding that i= s taken directly from CompICC, so I don't know whether it's a bug or a feat= ure. Anyway, it's from Kolor-Server. > = > Apparently, everything works as planned. But I cannot distinguish visible= differences for the profiles used for the different monitors by default. > = > There is an issue with the transparent areas - they are darkened somehow.= This should be easy to fix, I have left the issue around because I hadn't = time to finish before the GSoC deadline and it makes it possible to quickly= find out whether color correction is enabled or not :). Perhaps, it's beca= use of the blending function and not because of the correction. Also, there= 's something wrong with the highlights in gitk, I don't know exactly what. = So, small issues are present. > = > = > Screenshots > ----------- > = > Output 0 Lab.icc Output 1 XYZ.icc > http://git.reviewboard.kde.org/r/106141/s/695/ > = > = > Thanks, > = > Casian Andrei > = > --===============4641385099147814806== Content-Type: text/html; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable
This is an automatically generated e-mail. To reply, visit: http://git.revie= wboard.kde.org/r/106141/

On August 25th, 2012, 2:58 p.m., Fredrik H= =C3=B6glund wrote:

= = =
kwin/libkwineffects/kwinglcolorcorrection.cpp (Diff revision 1) 

   
void ColorCorrectionPrivate::setupCCTexture(GLuint texture, const C=
lut& clut)


  

 

  

    
    

    672
    
    glTexParameteri(GL_TEXTURE_3D, GL_TEXTURE_WRAP_S, GL_CLAMP);

  


 




  
I'm p=
retty sure you want GL_CLAMP_TO_EDGE here.

 




 
On August 27th, 2012, 2:56 p.m., Casian Andrei wrote:

 

  
Thanks, w=
ho knows how much time would have taken me to figure this out. (I took the =
code from CompICC without looking on the documentation, thinking it's c=
orrect for this case).

 











FYI: it's a SUCH popular mistake that the nvidia driver until no=
w (conditionally) took GL_CLAMP as GL_CLAMP_TO_EDGE
Since the 300 drivers there's a GUI option for this (defaults to "=
on" ie. you suddenly got clamping errors (black lines) all over the pl=
ace ;-) 






 
On August 25th, 2012, 2:58 p.m., Fredrik H=
=C3=B6glund wrote:

 

  =








 =

 =





 

 
  

   
    kwin/libkwineffects/kwinglutils.cpp
    

     (Diff revision 1)

    
   
  

 

 
  


   
bool GLShader::loadFromFiles(const QString &vertexFile, const Q=
String &fragmentFile)


  

 

  

    321
    
    const char* src =
=3D ba.=
constData();
=

    320
    
    // Inject color correct=
ion code for fragment shaders, if possible

  


 




  
You can&#=
39;t do this unconditionally. KWin has shaders that operate on pixels that =
have already been color corrected.

 




 
On August 27th, 2012, 2:56 p.m., Casian Andrei wrote:

 

  
This is a=
 tricky issue.

I guess the applications should use the X Color Management stuff to specify=
 which regions are color corrected and if there are some that don't nee=
d any correction. If they do that, Kolor Server will see it and update the =
regions that will end up in KWin. So for areas not to be corrected by KWin,=
 a dummy lookup table will be used for correction, sRGB -> sRGB (basical=
ly does nothing).

If the applications don't use the X Color Management and color correcti=
on is enabled in KWin too, then I think it sucks.

Perhaps my mentor could give details :/

 











I don't think this is what Fredrik meant - some shaders will ope=
rate on a previously buffered (and color corrected) output. If you inject c=
olor correction here as well, you'll "correct" twice.








- Thomas






On August 23rd, 2012, 2:56 p.m., Casian Andrei wrote:









 

  


Review request for kwin, Thomas L=C3=BCbking and Martin Gr=C3=A4=C3=9F=
lin.


By Casian Andrei.




Updated Aug. 23, 2012, 2:56 p.m.








Descripti=
on 

 

 

  
   
These are the results of my GSoC project, for KWin.

This color correction needs KolorManager installed, 'kded' branch. =
That one needs Oyranos.
git://anongit.kde.org/kolor-manager.git         http://www.oyranos.org/down=
loads
If it's not installed, the ColorCorrection class should fail without ca=
using any issues to the user (that's if it's enabled from the KCM).=
 Probably a notification could be appropriate in that case? "Failed to=
 initialize CC because cannot contact Kolor-Server KDED..." something =
like that?

Change summary:
--
Add an option to kcmcompositing in the 'Advanced' tab, to enable or=
 disable color correction. It is specified that it's experimental and i=
t needs Kolor Manager. This implies some small changes to options.h and cpp.

In composite.cpp, the ColorCorrection class instance is initialized or clea=
ned up (it's like being owned by the compositor). In workspace, there&#=
39;s a signal connected to reset compositing if the color correction settin=
g is changed.

Scene::finalPaintWindow code is moved into SceneOpenGL::performPaint.
SceneOpenGL has the declarations of some methods changed to include the scr=
een number - those could be changed to pass around PaintData's probably=
. SceneOpenGL's newly implemented finalDrawWindow splits the rendering =
if color correction is enabled and there are multiple screens. Before paint=
ing for a particular screen, ColorCorrection::setupForOutput should be call=
ed.

There was a problem with the blending function causing artifacts with color=
 correction enabled so it is set up to src_alpha, one_minus_src alpha in th=
at case.

Now, inside KWinEffects:
--
A screen property is added for WindowPaintData.
In kwinglutils, The fragment shaders are intercepted before being compiled =
and they get a couple of lines of code inserted in order to do the color st=
uff. This happens only when color correction is enabled, of course.

And there is the big ColorCorrection class. The public and private stuff ar=
e well separated. More additions to the public stuff shouldn't be neces=
sary in the future as far as I see it. Everything is there I think. The reg=
ion stuff is not used at the moment, and the 2 reset methods aren't use=
d either (for now).

The private stuff consists of the private data and the D-Bus interface. Reg=
arding D-Bus, everything is async (I think).

The implementation basically manages a set of color lookup tables for diffe=
rent outputs and for different window regions. These are taken via D-Bus. E=
ach lookup table has around 700 KB. I hope I am not wrong in assuming it=
9;s not that big of an issue time-wise, since they are transferred only onc=
e or twice for an entire session. I hope D-Bus doesn't do any time cons=
uming business to transfer those QVector's.

There is an issue with OpenGL ES, apparently it doesn't support 3D text=
ures (probably it needs an extension or something). Until we envision a cle=
ar solution, I have disabled the color correction for OpenGL ES.

In case of problems, it should not affect the rest of KWin in any way.

Next priorities:
--
Implement per-window-region stuff. It should be a matter of extending Kolor=
Server's capabilities and doing some small modifications in SceneOpenGL=
::Window. The ColorCorrection class from KWinEffects has everything prepare=
d.

Fix issues with transparency / some weird colors.

--
There's another thing... I don't know when I'm supposed to be a=
ble to do improvements and modifications, since the GSoC project ended. Pro=
bably if I keep the new modifications well separated, there should be no is=
sues.

  		
 






Testing <=
/h1>


 

  
   
Restarted KDED making sure Kolor-Manager is installed proper=
ly - this should make the KDED module available.

Restarted KWin, enabled color correction in KCM compositing, advanced tab.

Played a bit with commands like:
oyranos-monitor -d 0 XYZ.icc
oyranos-monitor -d 1 Lab.icc
Those set up particular profiles for the monitors. However, it seems there =
is a problem with reverting to the normal ones. The code regarding that is =
taken directly from CompICC, so I don't know whether it's a bug or =
a feature. Anyway, it's from Kolor-Server.

Apparently, everything works as planned. But I cannot distinguish visible d=
ifferences for the profiles used for the different monitors by default.

There is an issue with the transparent areas - they are darkened somehow. T=
his should be easy to fix, I have left the issue around because I hadn'=
t time to finish before the GSoC deadline and it makes it possible to quick=
ly find out whether color correction is enabled or not :). Perhaps, it'=
s because of the blending function and not because of the correction. Also,=
 there's something wrong with the highlights in gitk, I don't know =
exactly what. So, small issues are present.


  		
 








Diffs=
 


    

 
  • kwin/composite.cpp (c65716b)
    • 

 
  • kwin/kcmkwin/kwincompositing/main.cpp (7a8=
9db4)
    • 

 
  • kwin/kcmkwin/kwincompositing/main.ui (cff0=
f63)
    • 

 
  • kwin/libkwineffects/CMakeLists.txt (14a274=
7)
    • 

 
  • kwin/libkwineffects/kwineffects.h (2c2f7bf=
)
    • 

 
  • kwin/libkwineffects/kwineffects.cpp (bae85=
e7)
    • 

 
  • kwin/libkwineffects/kwinglcolorcorrection.h (PRE-CREATION)
    • 

 
  • kwin/libkwineffects/kwinglcolorcorrection_p.h (PRE-CREATION)
    • 

 
  • kwin/libkwineffects/kwinglutils.h (7a7c3c9=
)
    • 

 
  • kwin/libkwineffects/kwinglutils.cpp (016d3=
1e)
    • 

 
  • kwin/options.h (9e4fcc6)
    • 

 
  • kwin/options.cpp (72f168d)
    • 

 
  • kwin/scene.h (3891198)
    • 

 
  • kwin/scene.cpp (5782510)
    • 

 
  • kwin/scene_opengl.h (de33ce4)
    • 

 
  • kwin/scene_opengl.cpp (cbcc0ca)
    • 

 
  • kwin/workspace.cpp (cf3f308)
    • 





View Diff





Screensho=
ts 





 3D"Output





  		
 











  

 



--===============4641385099147814806==--

--===============5625015705345819016==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
kwin mailing list
kwin@kde.org
https://mail.kde.org/mailman/listinfo/kwin

--===============5625015705345819016==--