[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: Martin_Gräßlin <kde () martin-graesslin ! com>
Date: 2013-01-10 8:08:58
Message-ID: 20130110080858.25156.68566 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On Jan. 10, 2013, 7:53 a.m., Martin Gräßlin wrote:
> > kwin/libkwineffects/kwinglcolorcorrection.cpp, lines 331-332
> > <http://git.reviewboard.kde.org/r/107754/diff/6/?file=104920#file104920line331>
> >
> > could you just use checkGLError which takes care of looping through all errors \
> > and doesn't swallow the errors. If there is one we still want to know that there \
> > has been one, but that just removes all.
> > Also I must admit that I don't like while loops with no body ;-)
>
> Casian Andrei wrote:
> Something like checkGLError("foomethod-beginning") is ok?
it's not optimal, but good enough. That way one sees at least an area where the error \
was. Maybe instead of "beginning" something with "clear".
- Martin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107754/#review25135
-----------------------------------------------------------
On Jan. 5, 2013, 9:02 a.m., Casian Andrei wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107754/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2013, 9:02 a.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/options.h f9f623f
> 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 January 10th, 2013, 7:53 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/6/?file=104920#file104920line331" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kwin/libkwineffects/kwinglcolorcorrection.cpp</a> <span \
style="font-weight: normal;">
(Diff revision 6)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; \
">ColorCorrectionPrivate::ColorCorrectionPrivate(ColorCorrection *parent)</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">331</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="c1">// Clear any previous GL errors</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">332</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">while</span> <span class="p">(</span><span class="n">glGetError</span><span \
class="p">()</span> <span class="o">!=</span> <span class="n">GL_NO_ERROR</span><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;">could you just use \
checkGLError which takes care of looping through all errors and doesn't swallow \
the errors. If there is one we still want to know that there has been one, but that \
just removes all.
Also I must admit that I don't like while loops with no body ;-)</pre>
</blockquote>
<p>On January 10th, 2013, 7:57 a.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;">Something like \
checkGLError("foomethod-beginning") is ok?</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;">it's \
not optimal, but good enough. That way one sees at least an area where the error was. \
Maybe instead of "beginning" something with "clear".</pre> <br />
<p>- Martin</p>
<br />
<p>On January 5th, 2013, 9:02 a.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. 5, 2013, 9:02 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;">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/options.h <span style="color: grey">(f9f623f)</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