[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.

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


-----------------------------------------------------------
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/20130801/05e158db/attachment.html>