[prev in list] [next in list] [prev in thread] [next in thread]
List: kwin
Subject: Re: Review Request: Better handling of OpenGL1/2 specific code pathes in effects
From: Fredrik_Höglund <fredrik () kde ! org>
Date: 2012-09-21 14:30:06
Message-ID: 20120921143006.520.32057 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106521/#review19268
-----------------------------------------------------------
Following the Qt naming style ShaderHelper should be called ShaderBinder (see \
QMutexLocker). I think it should also have a method that returns a pointer to the \
shader it bound.
That way you can write:
ShaderBinder binder(ShaderManager::GenericShader);
GLShader *shader = binder.shader();
Also if the methods are inline there is no runtime overhead from using the class.
- Fredrik Höglund
On Sept. 21, 2012, 9:44 a.m., Martin Gräßlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106521/
> -----------------------------------------------------------
>
> (Updated Sept. 21, 2012, 9:44 a.m.)
>
>
> Review request for kwin.
>
>
> Description
> -------
>
> <summary>
> Two new compositing types are introduced: OpenGL1Compositing and \
> OpenGL2Compositing. This allows us to better check whether an effect is supported \
> and we no longer have to abuse the ShaderManager::instance()->isValid() to check \
> whether we are on OpenGL 1 or 2. (With 1 it's always invalid, with 2 it's always \
> valid).
> Additionally a new "ShaderHelper" class is introduced which handles the common use \
> case of testing whether we are on OpenGL 2, pushing a shader, checking again \
> whether we are on OpenGL 2 and popping the shader. If anyone has an idea for a \
> better name, please let me know. </summary>
>
>
> Introduce a helper class to automatically push/pop Shaders
>
> The ShaderHelper class can be used for the case that a block of code
> should be executed with a given Shader being bound. This is useful for
> all the cases where there is a if-block for OpenGL2 execution with a
> Shader being pushed in the first line to the ShaderManager and popped in
> the last line of the block. With the helper this can be simplified to:
>
> ShaderHelper helper(myCustomShader);
>
> or
>
> ShaderHelper helper(ShaderManager::GenericShader);
>
> The ctor of ShaderHelper pushes the given Shader to the stack and once
> the helper goes out of scope it will be popped again from the stack.
>
> In addition the helper can take care of OpenGL 1 compositing, that is it
> just does nothing. So it can also be used where there is a shared OpenGL1
> and OpenGL2 code path where the Shader should only be pushed in OpenGL2.
> This basically removes all the checks for the compositing type before
> pushing/popping a Shader to the stack.
>
> Do not use ShaderManager::isValid to check for OpenGL2 compositing
>
> The main usage of ShaderManager::isValid was to have OpenGL2 specific
> code pathes. Now we have an actual OpenGL2Compositing type and we know
> that the ShaderManager is valid if we have this compositing type and we
> know that it is not valid on OpenGL1Compositing. This gives us a much
> better check and allows us to use the isValid method just for where we
> want to check whether the shaders compiled successfully.
>
> In addition some effects require OpenGL2, so we do not need to check
> again that the ShaderManager is valid. Such usages are removed.
>
> Introduce dedicated OpenGL1 and OpenGL2 compositing types
>
> The CompositingType enum turns into flags and two new values are
> introduced: OpenGL1Compositing and OpenGL2Compositing.
>
> Those new values are or-ed to OpenGLCompositing so that a simple check
> for the flag OpenGLCompositing works in case of one of those two new
> values. To make the generic check for OpenGL compositing easier a method
> in EffectsHandler is introduced to just check for this.
>
> The scenes now return either OpenGL1Compositing or OpenGL2Compositing
> depending on which Scene implementation. None returns OpenGLCompositing.
>
>
> Diffs
> -----
>
> kwin/client.cpp fefdb4036c46a85566962748e04809316fec6445
> kwin/composite.cpp 11958961a74593b23744dfcbff1397da972a551b
> kwin/effects.cpp dabb96aeca162302cc4c6ecea475723ba23b8196
> kwin/effects/blur/blur.cpp 2b32b3a49cc9da3a012bb21e1ca6ec19fe14e5d2
> kwin/effects/blur/blurshader.cpp 1564c5a7f2a5908bb92523e2e66f48796eb31a44
> kwin/effects/coverswitch/coverswitch.cpp 22c9576a69990ec6a419321aa27a44989ccb29af
> kwin/effects/cube/cube.cpp 4b1391776d53d91b51f7567417e64f047e90943a
> kwin/effects/cube/cubeslide.cpp 1d0f02fc73bacefeddc289e1cab41652491ad67c
> kwin/effects/explosion/explosion.cpp 15fdc6e0a1793d74aacbd8be86e83a5fcbbdbfda
> kwin/effects/flipswitch/flipswitch.cpp a88a6d3933e54ca6f9d7cf9d2ccb556b3c939c38
> kwin/effects/glide/glide.cpp 540844fece0b15a1be944a4fa1b248f6fd1f1987
> kwin/effects/invert/invert.cpp 258e2ecadd01783c082f1dbcaafab24a22e2471e
> kwin/effects/logout/logout.cpp a78460d1bbf0348aa6da884e071d7eb06277192f
> kwin/effects/lookingglass/lookingglass.cpp c532fe1a3d814dfabf43cd57e2e3dba10b3510fe \
> kwin/effects/magiclamp/magiclamp.cpp c151ed1175f96d7572ebf40c32e741e4a1ee44a8
> kwin/effects/magnifier/magnifier.cpp 0103257fbd104f6b8685f9a4a500ae6447e5b63c
> kwin/effects/mousemark/mousemark.cpp 0a3229b3ec0cd256d883bf6d219cff37d772c64f
> kwin/effects/presentwindows/presentwindows.cpp \
> 96d9a68a27884a48592183b4735f6a50e5b97e3f kwin/effects/resize/resize.cpp \
> 410b6748f6b82a0164bc5bed78e4f6785010a400 kwin/effects/screenshot/screenshot.cpp \
> ea56e80c8b6443a2d09b024396d5f78cf098a0b1 kwin/effects/sheet/sheet.cpp \
> 58af9b4c0d0536de0637a2b741c81450ffb18263 kwin/effects/showfps/showfps.cpp \
> b3dd9b74943becfb1545f3159bb5aec8d8efb31e kwin/effects/showpaint/showpaint.cpp \
> c17178ce54beb4c77c45c2688b2fdc9630c64381 kwin/effects/snaphelper/snaphelper.cpp \
> 7d4be144b4041b61e547b0aaa74c7b80d3c38314 \
> kwin/effects/startupfeedback/startupfeedback.cpp \
> cab7d43dcfd765bd24006322de3436b88035302e \
> kwin/effects/taskbarthumbnail/taskbarthumbnail.cpp \
> 0c67a3b84a72b22e9b6d1f6b9e34db8c3445962e kwin/effects/trackmouse/trackmouse.cpp \
> cf6575456a03259fb8de10b2792291edcb06ea1a \
> kwin/effects/wobblywindows/wobblywindows.cpp \
> dbfbdc35167dfb9301aadd15ae653c031d1868a6 kwin/effects/zoom/zoom.cpp \
> 587de0328d2390b2200e024dbff7f90c24d68308 kwin/lanczosfilter.cpp \
> 654d2cf95ad5da4707bab7fef9a0019702c57d0f kwin/libkwineffects/kwineffects.h \
> 2c2f7bf5edae3c0f93e255833faab842939f63d1 kwin/libkwineffects/kwineffects.cpp \
> bae85e79b5b38314e3eb964dd6922bf62528c3e2 kwin/libkwineffects/kwinglobals.h \
> 689fe4da2a1604bee9d2022377ce7f7940105a53 kwin/libkwineffects/kwinglutils.h \
> 14fdd5ba9b5e645eb839cc530e7f6b1d91007589 kwin/libkwineffects/kwinglutils.cpp \
> 5abae528fcf06d13575eac8a0c8cb3e114a240d6 kwin/scene_opengl.h \
> 8ff0006a650b47f426fb775e7681724f2315e513 kwin/scene_opengl.cpp \
> 3e4290a556e58cf5580278b554d2f199bb2c9a60 kwin/shadow.cpp \
> de989c7955fa267aa533f40126066e6e0ccb26d9 kwin/workspace.cpp \
> 5b37aee2bc85181ed2b11bd00c21e38fd61a38dd
> Diff: http://git.reviewboard.kde.org/r/106521/diff/
>
>
> Testing
> -------
>
> running the code right now and seems to work
>
>
> Thanks,
>
> Martin Gräßlin
>
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/106521/">http://git.reviewboard.kde.org/r/106521/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Following the Qt naming \
style ShaderHelper should be called ShaderBinder (see QMutexLocker). I think it \
should also have a method that returns a pointer to the shader it bound.
That way you can write:
ShaderBinder binder(ShaderManager::GenericShader);
GLShader *shader = binder.shader();
Also if the methods are inline there is no runtime overhead from using the \
class.</pre> <br />
<p>- Fredrik</p>
<br />
<p>On September 21st, 2012, 9:44 a.m., Martin Gräßlin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for kwin.</div>
<div>By Martin Gräßlin.</div>
<p style="color: grey;"><i>Updated Sept. 21, 2012, 9:44 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><summary> Two new compositing types are introduced: \
OpenGL1Compositing and OpenGL2Compositing. This allows us to better check whether an \
effect is supported and we no longer have to abuse the \
ShaderManager::instance()->isValid() to check whether we are on OpenGL 1 or 2. \
(With 1 it's always invalid, with 2 it's always valid).
Additionally a new "ShaderHelper" class is introduced which handles the \
common use case of testing whether we are on OpenGL 2, pushing a shader, checking \
again whether we are on OpenGL 2 and popping the shader. If anyone has an idea for a \
better name, please let me know. </summary>
Introduce a helper class to automatically push/pop Shaders
The ShaderHelper class can be used for the case that a block of code
should be executed with a given Shader being bound. This is useful for
all the cases where there is a if-block for OpenGL2 execution with a
Shader being pushed in the first line to the ShaderManager and popped in
the last line of the block. With the helper this can be simplified to:
ShaderHelper helper(myCustomShader);
or
ShaderHelper helper(ShaderManager::GenericShader);
The ctor of ShaderHelper pushes the given Shader to the stack and once
the helper goes out of scope it will be popped again from the stack.
In addition the helper can take care of OpenGL 1 compositing, that is it
just does nothing. So it can also be used where there is a shared OpenGL1
and OpenGL2 code path where the Shader should only be pushed in OpenGL2.
This basically removes all the checks for the compositing type before
pushing/popping a Shader to the stack.
Do not use ShaderManager::isValid to check for OpenGL2 compositing
The main usage of ShaderManager::isValid was to have OpenGL2 specific
code pathes. Now we have an actual OpenGL2Compositing type and we know
that the ShaderManager is valid if we have this compositing type and we
know that it is not valid on OpenGL1Compositing. This gives us a much
better check and allows us to use the isValid method just for where we
want to check whether the shaders compiled successfully.
In addition some effects require OpenGL2, so we do not need to check
again that the ShaderManager is valid. Such usages are removed.
Introduce dedicated OpenGL1 and OpenGL2 compositing types
The CompositingType enum turns into flags and two new values are
introduced: OpenGL1Compositing and OpenGL2Compositing.
Those new values are or-ed to OpenGLCompositing so that a simple check
for the flag OpenGLCompositing works in case of one of those two new
values. To make the generic check for OpenGL compositing easier a method
in EffectsHandler is introduced to just check for this.
The scenes now return either OpenGL1Compositing or OpenGL2Compositing
depending on which Scene implementation. None returns OpenGLCompositing.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">running the code right now and seems to work</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kwin/client.cpp <span style="color: \
grey">(fefdb4036c46a85566962748e04809316fec6445)</span></li>
<li>kwin/composite.cpp <span style="color: \
grey">(11958961a74593b23744dfcbff1397da972a551b)</span></li>
<li>kwin/effects.cpp <span style="color: \
grey">(dabb96aeca162302cc4c6ecea475723ba23b8196)</span></li>
<li>kwin/effects/blur/blur.cpp <span style="color: \
grey">(2b32b3a49cc9da3a012bb21e1ca6ec19fe14e5d2)</span></li>
<li>kwin/effects/blur/blurshader.cpp <span style="color: \
grey">(1564c5a7f2a5908bb92523e2e66f48796eb31a44)</span></li>
<li>kwin/effects/coverswitch/coverswitch.cpp <span style="color: \
grey">(22c9576a69990ec6a419321aa27a44989ccb29af)</span></li>
<li>kwin/effects/cube/cube.cpp <span style="color: \
grey">(4b1391776d53d91b51f7567417e64f047e90943a)</span></li>
<li>kwin/effects/cube/cubeslide.cpp <span style="color: \
grey">(1d0f02fc73bacefeddc289e1cab41652491ad67c)</span></li>
<li>kwin/effects/explosion/explosion.cpp <span style="color: \
grey">(15fdc6e0a1793d74aacbd8be86e83a5fcbbdbfda)</span></li>
<li>kwin/effects/flipswitch/flipswitch.cpp <span style="color: \
grey">(a88a6d3933e54ca6f9d7cf9d2ccb556b3c939c38)</span></li>
<li>kwin/effects/glide/glide.cpp <span style="color: \
grey">(540844fece0b15a1be944a4fa1b248f6fd1f1987)</span></li>
<li>kwin/effects/invert/invert.cpp <span style="color: \
grey">(258e2ecadd01783c082f1dbcaafab24a22e2471e)</span></li>
<li>kwin/effects/logout/logout.cpp <span style="color: \
grey">(a78460d1bbf0348aa6da884e071d7eb06277192f)</span></li>
<li>kwin/effects/lookingglass/lookingglass.cpp <span style="color: \
grey">(c532fe1a3d814dfabf43cd57e2e3dba10b3510fe)</span></li>
<li>kwin/effects/magiclamp/magiclamp.cpp <span style="color: \
grey">(c151ed1175f96d7572ebf40c32e741e4a1ee44a8)</span></li>
<li>kwin/effects/magnifier/magnifier.cpp <span style="color: \
grey">(0103257fbd104f6b8685f9a4a500ae6447e5b63c)</span></li>
<li>kwin/effects/mousemark/mousemark.cpp <span style="color: \
grey">(0a3229b3ec0cd256d883bf6d219cff37d772c64f)</span></li>
<li>kwin/effects/presentwindows/presentwindows.cpp <span style="color: \
grey">(96d9a68a27884a48592183b4735f6a50e5b97e3f)</span></li>
<li>kwin/effects/resize/resize.cpp <span style="color: \
grey">(410b6748f6b82a0164bc5bed78e4f6785010a400)</span></li>
<li>kwin/effects/screenshot/screenshot.cpp <span style="color: \
grey">(ea56e80c8b6443a2d09b024396d5f78cf098a0b1)</span></li>
<li>kwin/effects/sheet/sheet.cpp <span style="color: \
grey">(58af9b4c0d0536de0637a2b741c81450ffb18263)</span></li>
<li>kwin/effects/showfps/showfps.cpp <span style="color: \
grey">(b3dd9b74943becfb1545f3159bb5aec8d8efb31e)</span></li>
<li>kwin/effects/showpaint/showpaint.cpp <span style="color: \
grey">(c17178ce54beb4c77c45c2688b2fdc9630c64381)</span></li>
<li>kwin/effects/snaphelper/snaphelper.cpp <span style="color: \
grey">(7d4be144b4041b61e547b0aaa74c7b80d3c38314)</span></li>
<li>kwin/effects/startupfeedback/startupfeedback.cpp <span style="color: \
grey">(cab7d43dcfd765bd24006322de3436b88035302e)</span></li>
<li>kwin/effects/taskbarthumbnail/taskbarthumbnail.cpp <span style="color: \
grey">(0c67a3b84a72b22e9b6d1f6b9e34db8c3445962e)</span></li>
<li>kwin/effects/trackmouse/trackmouse.cpp <span style="color: \
grey">(cf6575456a03259fb8de10b2792291edcb06ea1a)</span></li>
<li>kwin/effects/wobblywindows/wobblywindows.cpp <span style="color: \
grey">(dbfbdc35167dfb9301aadd15ae653c031d1868a6)</span></li>
<li>kwin/effects/zoom/zoom.cpp <span style="color: \
grey">(587de0328d2390b2200e024dbff7f90c24d68308)</span></li>
<li>kwin/lanczosfilter.cpp <span style="color: \
grey">(654d2cf95ad5da4707bab7fef9a0019702c57d0f)</span></li>
<li>kwin/libkwineffects/kwineffects.h <span style="color: \
grey">(2c2f7bf5edae3c0f93e255833faab842939f63d1)</span></li>
<li>kwin/libkwineffects/kwineffects.cpp <span style="color: \
grey">(bae85e79b5b38314e3eb964dd6922bf62528c3e2)</span></li>
<li>kwin/libkwineffects/kwinglobals.h <span style="color: \
grey">(689fe4da2a1604bee9d2022377ce7f7940105a53)</span></li>
<li>kwin/libkwineffects/kwinglutils.h <span style="color: \
grey">(14fdd5ba9b5e645eb839cc530e7f6b1d91007589)</span></li>
<li>kwin/libkwineffects/kwinglutils.cpp <span style="color: \
grey">(5abae528fcf06d13575eac8a0c8cb3e114a240d6)</span></li>
<li>kwin/scene_opengl.h <span style="color: \
grey">(8ff0006a650b47f426fb775e7681724f2315e513)</span></li>
<li>kwin/scene_opengl.cpp <span style="color: \
grey">(3e4290a556e58cf5580278b554d2f199bb2c9a60)</span></li>
<li>kwin/shadow.cpp <span style="color: \
grey">(de989c7955fa267aa533f40126066e6e0ccb26d9)</span></li>
<li>kwin/workspace.cpp <span style="color: \
grey">(5b37aee2bc85181ed2b11bd00c21e38fd61a38dd)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/106521/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
kwin mailing list
kwin@kde.org
https://mail.kde.org/mailman/listinfo/kwin
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic