<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/111838/">http://git.reviewboard.kde.org/r/111838/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 1st, 2013, 5:17 p.m. UTC, <b>Torsten Rahn</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;">Is this really a good idea? While I agree on the choice for view classes (like GeoGraphicsItems). I'm concerned about usage of QPixmap in GeoData* classes: They are models and could be generated in threads. However creation of QPixmaps in threads is a bad idea since they are created e.g. on the X-Server which only knows about the GUI thread. We already get warnings when using the graphics system X11 for this reason. For this reason I suggest to use QPixmap only in the view classes while preferring QImages on the model side. </pre>
</blockquote>
<p>On August 1st, 2013, 5:22 p.m. UTC, <b>Bernhard Beschow</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;">For memory efficiency, it would be better to store pixmaps in the GeoData* classes and use them directly for rendering. For example, consider loading a pixmap in a single style instance once and use it for 1000+ GeoGraphicsItems.</pre>
</blockquote>
<p>On August 1st, 2013, 5:29 p.m. UTC, <b>Torsten Rahn</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;">So what do you suggest for the quite common usecase where a GeoDataDocument is created in a thread? I think this is something we should support since it does happen already and since it can barely be reasonably discouraged?
Your intentions are reasonable. But maybe we should do it differently altogether and just store the Url to the icon in the style instead of instantiating the pixmap/image already. Only once rendered the image would then get loaded.</pre>
</blockquote>
<p>On August 1st, 2013, 6:10 p.m. UTC, <b>Bernhard Beschow</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;">Yes, delaying pixmap loading from an URL until actually needed sounds like feasible approach to me.
We are, however, also creating a lot of (default) styles in GeoDataFeature. These styles are created in the GUI thread, which means that we could create QPixmaps rather than QImages right there and pass them to the styles. This way, we could (kind of) make sure that no pixmap conversions are happening later on.</pre>
</blockquote>
<p>On August 30th, 2013, 5:44 p.m. UTC, <b>Bernhard Beschow</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;">Thinking more about the discussion we had yesterday, it seems to me that we could indeed (ab)use QPixmapCache to save memory. First of all, I agree that we should only store URLs inside the GeoData tree and we should avoid loading QImages or even QPixmaps there.
As pointed out yesterday, finding the optimal size for QPixmapCache (in bytes!) such that all pixmaps needed for painting are in the cache seems close to impossible to me. Instead, we could store the pixmaps directly in the graphics items. Now, in order to avoid lots of copies of the same pixmaps in memory, we could also insert them into QPixmapCache. So before a graphics item creates a pixmap, it would first check its availability in QPixmapCache. If successful, it would reuse the pixmap data that was inserted by another graphics item.
The implementation could be similar to this one:
if ( !QPixmapCache::find( url, &m_pixmap ) ) {
// not in cache
m_pixmap.load( url ); // store pixmap in our own graphics item
QPixmapCache::insert( url, m_pixmap ); // insert into cache for sharing with other graphics items
}
else {
// nothing to do, m_pixmap's data is valid and is shared with another graphics item
}
Now, what happens if a pixmap is "garbage-collected" from QPixmapCache? All pixmaps in the graphics items remain valid, because each has its own reference to the data and QPixmaps are implicitly shared. Of course, if a new graphics item needs that pixmap as well, it must load another copy into memory, possibly sharing its data with even more new graphics items. So obviously, this implementation is not optimal w.r.t. memory consumption, but it may help to reduce it at least.
What do you think?</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Actually, QPixmap::load() does exactly what I described above. Pretty simple... nice.</pre>
<br />
<p>- Bernhard</p>
<br />
<p>On August 1st, 2013, 4:25 p.m. UTC, René Küttner wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://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 Marble.</div>
<div>By René Küttner.</div>
<p style="color: grey;"><i>Updated Aug. 1, 2013, 4:25 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;">GeoDataIconStyle stores its icon as a QImage at the moment. Since these styles are meant for rendering and QImage is rather slow for this purpose, I changed it to use QPixmap internally.</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;">I applied the patch to master and did compile marble. I also compared the visual output of the icon style before and after the patch.</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/lib/MarbleWidgetPopupMenu.cpp <span style="color: grey">(ba27f36)</span></li>
<li>src/lib/VisiblePlacemark.cpp <span style="color: grey">(42c1103)</span></li>
<li>src/lib/geodata/data/GeoDataFeature.h <span style="color: grey">(4cd0374)</span></li>
<li>src/lib/geodata/data/GeoDataFeature.cpp <span style="color: grey">(8a99a85)</span></li>
<li>src/lib/geodata/data/GeoDataIconStyle.h <span style="color: grey">(5b4644f)</span></li>
<li>src/lib/geodata/data/GeoDataIconStyle.cpp <span style="color: grey">(d68d0bb)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/111838/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>