[Marble-devel] Review Request 111838: Change GeoDataIconStyle to use QPixmap instead of QImage internally

[Bernhard Beschow]

> On Aug. 1, 2013, 5:17 p.m., Torsten Rahn wrote:
> > 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.
> 
> Bernhard Beschow wrote:
>     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.
> 
> Torsten Rahn wrote:
>     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.
> 
> Bernhard Beschow wrote:
>     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.
> 
> Bernhard Beschow wrote:
>     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?

Actually, QPixmap::load() does exactly what I described above. Pretty simple... nice.


- Bernhard


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111838/#review36925
-----------------------------------------------------------


On Aug. 1, 2013, 4:25 p.m., René Küttner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111838/
> -----------------------------------------------------------
> 
> (Updated Aug. 1, 2013, 4:25 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   src/lib/MarbleWidgetPopupMenu.cpp ba27f36 
>   src/lib/VisiblePlacemark.cpp 42c1103 
>   src/lib/geodata/data/GeoDataFeature.h 4cd0374 
>   src/lib/geodata/data/GeoDataFeature.cpp 8a99a85 
>   src/lib/geodata/data/GeoDataIconStyle.h 5b4644f 
>   src/lib/geodata/data/GeoDataIconStyle.cpp d68d0bb 
> 
> Diff: http://git.reviewboard.kde.org/r/111838/diff/
> 
> 
> Testing
> -------
> 
> 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.
> 
> 
> Thanks,
> 
> René Küttner
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20130901/d315deec/attachment.html>