[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: Review Request 117824: Discard window thumbnail pixmap after texture got destroyed by SceneGraph
From: Martin_Gräßlin <mgraesslin () kde ! org>
Date: 2014-04-30 11:17:07
Message-ID: 20140430111707.27257.33187 () probe ! kde ! org
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On April 30, 2014, 12:30 p.m., David Edmundson wrote:
> > src/declarativeimports/core/windowthumbnail.cpp, line 317
> > <https://git.reviewboard.kde.org/r/117824/diff/2/?file=269508#file269508line317>
> >
> > Do you still need this after the change above?
> >
> > I'm all for safety guards, but not if it's unneeded code.
> >
> > Maybe add a debug and see if it comes up IRL.
yep, it's still needed. I added an assert (Q_ASSERT(m_glxPixmap == XCB_PIXMAP_NONE)) \
to see whether the code path is still hit and got an abort in that condition using \
the steps to reproduce the crashy situation.
- Martin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117824/#review56961
-----------------------------------------------------------
On April 30, 2014, 7:44 a.m., Martin Gräßlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117824/
> -----------------------------------------------------------
>
> (Updated April 30, 2014, 7:44 a.m.)
>
>
> Review request for Plasma and Alexander Richardson.
>
>
> Bugs: 333482
> http://bugs.kde.org/show_bug.cgi?id=333482
>
>
> Repository: plasma-framework
>
>
> Description
> -------
>
> Discard window thumbnail pixmap after texture got destroyed by SceneGraph
>
> If the window holding the WindowThumbnail item goes away the texture hold
> by the node is destroyed but our bound window pixmap is not yet freed.
> This results in incorrect state the next time the WindowThumbnail is
> shown.
>
> To get back into a clean state discardPixmap() is called if there is no
> texture but a bound low level pixmap.
>
> BUG: 333482
>
>
> Diffs
> -----
>
> src/declarativeimports/core/windowthumbnail.cpp \
> d1a7fef1fc5fd119592710d80274d2abe0c8b3b1
> Diff: https://git.reviewboard.kde.org/r/117824/diff/
>
>
> Testing
> -------
>
>
> 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="https://git.reviewboard.kde.org/r/117824/">https://git.reviewboard.kde.org/r/117824/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On April 30th, 2014, 12:30 p.m. CEST, <b>David \
Edmundson</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="https://git.reviewboard.kde.org/r/117824/diff/2/?file=269508#file269508line317" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/declarativeimports/core/windowthumbnail.cpp</a> <span \
style="font-weight: normal;">
(Diff revision 2)
</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; ">void \
WindowThumbnail::bindEGLTexture()</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">317</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">if</span> <span class="p">(</span><span class="o">!</span><span \
class="n">textureNode</span><span class="o">-></span><span \
class="n">texture</span><span class="p">())</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;">Do you still need this \
after the change above?
I'm all for safety guards, but not if it's unneeded code.
Maybe add a debug and see if it comes up IRL.</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;">yep, \
it's still needed. I added an assert (Q_ASSERT(m_glxPixmap == XCB_PIXMAP_NONE)) \
to see whether the code path is still hit and got an abort in that condition using \
the steps to reproduce the crashy situation.</pre> <br />
<p>- Martin</p>
<br />
<p>On April 30th, 2014, 7:44 a.m. CEST, Martin Gräßlin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for Plasma and Alexander Richardson.</div>
<div>By Martin Gräßlin.</div>
<p style="color: grey;"><i>Updated April 30, 2014, 7:44 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=333482">333482</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-framework
</div>
<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;">Discard window thumbnail pixmap after texture got destroyed by \
SceneGraph
If the window holding the WindowThumbnail item goes away the texture hold
by the node is destroyed but our bound window pixmap is not yet freed.
This results in incorrect state the next time the WindowThumbnail is
shown.
To get back into a clean state discardPixmap() is called if there is no
texture but a bound low level pixmap.
BUG: 333482</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>src/declarativeimports/core/windowthumbnail.cpp <span style="color: \
grey">(d1a7fef1fc5fd119592710d80274d2abe0c8b3b1)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/117824/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic