[prev in list] [next in list] [prev in thread] [next in thread]
List: kwin
Subject: Re: Review Request: Abort color correction initialization and disable it in case of errors
From: Thomas_Lübking <thomas.luebking () web ! de>
Date: 2013-01-03 20:35:17
Message-ID: 20130103203517.3222.98752 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On Dec. 23, 2012, 8:55 a.m., Martin Gräßlin wrote:
> > kwin/scene_opengl.cpp, lines 583-587
> > <http://git.reviewboard.kde.org/r/107754/diff/3/?file=99720#file99720line583>
> >
> > why do you not define this slot in Options? Oh and please add a:
> > // TODO: Qt5: replace by lambda
>
> Casian Andrei wrote:
> It looks to me like it does not fit in Options - there are no slots for errors \
> there, I wouldn't know even where to place it there. SceneOpenGL2 owns the color \
> correction object, so it makes sense for it to handle the error.
> With that lambda feature, this slot will dissappear with Qt5 anyway :)
Q_SLOTS:
void disableColorCorrection();
under all those Q_SIGNALS: and above private.
The reason that there's no slot atm. is that we don't alter the *options* at runtime \
at all.
I'd say the scene should know (and tell on request dbug dbus interface request \
pending ...) whether it's color corrected and initiallize that from the options and \
not touch options.
@Martin
The pointer should (in this context) maybe ro to stress that?
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107754/#review23884
-----------------------------------------------------------
On Jan. 3, 2013, 7:04 p.m., Casian Andrei wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107754/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2013, 7:04 p.m.)
>
>
> Review request for kwin, Thomas Lübking and Martin Gräßlin.
>
>
> Description
> -------
>
> Abort color correction initialization and disable it in case of errors
>
> Checks are now performed for GL errors and in case of errors everything
> is aborted. The error handling mechanism introduced for this purpose
> somewhat improves the color correction code.
>
> Tracked down and fixed a GL invalid operation when first attempting to set the \
> lookup texture uniform for color correction.
>
> Diffs
> -----
>
> kwin/libkwineffects/kwinglcolorcorrection.h ec080c0
> kwin/libkwineffects/kwinglcolorcorrection.cpp b5127e6
> kwin/libkwineffects/kwinglcolorcorrection_p.h f54aa49
> kwin/scene_opengl.h e6142c0
> kwin/scene_opengl.cpp 147a248
>
> Diff: http://git.reviewboard.kde.org/r/107754/diff/
>
>
> Testing
> -------
>
> Tested when a GL invalid operation error occurs - color correction is disabled and \
> will not be enabled again because of the error. However, the check box in KCM \
> remains checked - but this is ok I think.
> Tested when no GL invalid operation is detected when doing color correction stuff. \
> Everything works fine from my point of view.
>
> Thanks,
>
> Casian Andrei
>
>
[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/107754/">http://git.reviewboard.kde.org/r/107754/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On December 23rd, 2012, 8:55 a.m., <b>Martin \
Gräßlin</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;"> <thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;"> <a \
href="http://git.reviewboard.kde.org/r/107754/diff/3/?file=99720#file99720line583" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kwin/scene_opengl.cpp</a> <span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; \
"></pre></td> <td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: \
0; ">void SceneOpenGL2::slotColorCorrectionError()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">582</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="kt">void</span> <span class="n">SceneOpenGL2</span><span \
class="o">::</span><span class="n">slotColorCorrectionError</span><span \
class="p">()</span></pre></td> </tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">583</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="p">{</span></pre></td> </tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">584</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="c1">// This should also trigger slotColorCorrectedChanged()</span></pre></td> \
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">585</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">options</span><span class="o">-></span><span \
class="n">setColorCorrected</span><span class="p">(</span><span \
class="kc">false</span><span class="p">);</span></pre></td> </tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">586</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="p">}</span></pre></td> </tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">why do you not define \
this slot in Options? Oh and please add a: // TODO: Qt5: replace by lambda</pre>
</blockquote>
<p>On January 3rd, 2013, 7:04 p.m., <b>Casian Andrei</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">It looks to me like it \
does not fit in Options - there are no slots for errors there, I wouldn't know \
even where to place it there. SceneOpenGL2 owns the color correction object, so it \
makes sense for it to handle the error.
With that lambda feature, this slot will dissappear with Qt5 anyway :)</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Q_SLOTS: \
void disableColorCorrection();
under all those Q_SIGNALS: and above private.
The reason that there's no slot atm. is that we don't alter the *options* at \
runtime at all.
I'd say the scene should know (and tell on request dbug dbus interface request \
pending ...) whether it's color corrected and initiallize that from the options \
and not touch options.
@Martin
The pointer should (in this context) maybe ro to stress that?</pre>
<br />
<p>- Thomas</p>
<br />
<p>On January 3rd, 2013, 7:04 p.m., Casian Andrei 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, Thomas Lübking and Martin Gräßlin.</div>
<div>By Casian Andrei.</div>
<p style="color: grey;"><i>Updated Jan. 3, 2013, 7:04 p.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;">Abort color correction initialization and disable it in case of errors
Checks are now performed for GL errors and in case of errors everything
is aborted. The error handling mechanism introduced for this purpose
somewhat improves the color correction code.
Tracked down and fixed a GL invalid operation when first attempting to set the lookup \
texture uniform for color correction.</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;">Tested when a GL invalid operation error occurs - color correction is \
disabled and will not be enabled again because of the error. However, the check box \
in KCM remains checked - but this is ok I think.
Tested when no GL invalid operation is detected when doing color correction stuff. \
Everything works fine from my point of view. </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/libkwineffects/kwinglcolorcorrection.h <span style="color: \
grey">(ec080c0)</span></li>
<li>kwin/libkwineffects/kwinglcolorcorrection.cpp <span style="color: \
grey">(b5127e6)</span></li>
<li>kwin/libkwineffects/kwinglcolorcorrection_p.h <span style="color: \
grey">(f54aa49)</span></li>
<li>kwin/scene_opengl.h <span style="color: grey">(e6142c0)</span></li>
<li>kwin/scene_opengl.cpp <span style="color: grey">(147a248)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107754/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