<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://reviewboard.kde.org/r/4444/">http://reviewboard.kde.org/r/4444/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 25th, 2010, 1:05 p.m., <b>jmho</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="/r/4444/diff/2/?file=29757#file29757line207" style="color: black; font-weight: bold; text-decoration: underline;">trunk/KDE/kdeedu/marble/src/lib/TileLoader.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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;">QImage * TileLoader::scaledLowerLevelTile( TileId const & id )</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;">QImage TileLoader::scaledLowerLevelTile( TileId const & id )</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">200</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"><span class="n">QImage</span><span class="hl"> </span><span class="o"><span class="hl">*</span></span> <span class="n">TileLoader</span><span class="o">::</span><span class="n">scaledLowerLevelTile</span><span class="p">(</span> <span class="n">TileId</span> <span class="k">const</span> <span class="o">&</span> <span class="n">id</span> <span class="p">)</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">197</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"><span class="n">QImage</span> <span class="n">TileLoader</span><span class="o">::</span><span class="n">scaledLowerLevelTile</span><span class="p">(</span> <span class="n">TileId</span> <span class="k">const</span> <span class="o">&</span> <span class="n">id</span> <span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre>This now returns a copy. Is this really necessary? Could we not try to not copy QImages at all?</pre>
</blockquote>
<p>On June 25th, 2010, 1:17 p.m., <b>Torsten Rahn</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre>Does it create a deep copy or a shallow copy? Where does the detach happen? :-)</pre>
</blockquote>
<p>On June 25th, 2010, 1:19 p.m., <b>Bernhard Beschow</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre>As explained above, copying a QImage is very fast. Why not use the convenience that QImage provides?</pre>
</blockquote>
<p>On June 25th, 2010, 1:26 p.m., <b>Bernhard Beschow</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre>@Torsten: It creates a shallow copy. The detach happens when TextureTile::setImage() is called.</pre>
</blockquote>
<p>On June 25th, 2010, 1:50 p.m., <b>Torsten Rahn</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre>@Bernhard: Huh? Why should TextureTile::setImage() create a detach?
inline void TextureTile::setImage( QImage const &image )
{
Q_ASSERT( !image.isNull() );
m_image = image;
}
I don't see a detach happening there.
</pre>
</blockquote>
<p>On June 25th, 2010, 2:08 p.m., <b>Bernhard Beschow</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre>@Torsten: It must detach from the old data m_image is attached to before the assignment.</pre>
</blockquote>
<p>On June 25th, 2010, 2:57 p.m., <b>Torsten Rahn</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre>Bernhard: No. In this case there's just an assignment happening. That only involves reference count changes but no deep copy. Since the reference count would be > 1 afterwards this would prepare for a possible detach that would happen if the m_image would get modified at a later point. But that would only happen inside a different method way after the execution of setImage() is finished.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em">Torsten: Convinced :)</pre>
<br />
<p>- Bernhard</p>
<br />
<p>On June 25th, 2010, 12:49 p.m., Bernhard Beschow wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://reviewboard.kde.orgrb/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 marble.</div>
<div>By Bernhard Beschow.</div>
<p style="color: grey;"><i>Updated 2010-06-25 12:49:24</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;">Motivation: It may happen that Marble receives invalid image data over the network or loads invalid image data from disk, making Marble really slow. This is caused by heavy debug output from QImage while Marble tries to access invalid image data. This will be qFatal in the upcoming Qt 4.7.
Solution: Check if the image is valid ater loading data into a QImage instead of blindly loading data into it. Also Q_ASSERT every image assignment in TextureTile.
Note that by this patch, Marble will abort when TileLoader::scaledLowerLevelTile() is not able to produce a scaled image. This could happen when the level-zero tile is missing.</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;">Works for me. Please test if it works for you as well!</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>trunk/KDE/kdeedu/marble/src/lib/TextureTile.h <span style="color: grey">(1142673)</span></li>
<li>trunk/KDE/kdeedu/marble/src/lib/TextureTile.cpp <span style="color: grey">(1142673)</span></li>
<li>trunk/KDE/kdeedu/marble/src/lib/TileLoader.h <span style="color: grey">(1142673)</span></li>
<li>trunk/KDE/kdeedu/marble/src/lib/TileLoader.cpp <span style="color: grey">(1142673)</span></li>
</ul>
<p><a href="http://reviewboard.kde.org/r/4444/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>