Review Request: frame plasmoid: image load and scaling with threads

[Sebastian Kügler]

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


I'm not sure about this patch:

- The loading of images in a thread is nice
- The "loads huge picture and keeps them in mem" is not changed (in fact, it's worse now since QImage forces you to keep them in system memory, whereas QPixmaps can be stored on the graphics card)
- It basically disables hardware acceleration of the image scaling, QImage vs. QPixmap

With QPixmaps, we can blit into a smaller rect which is very fast, with QImage, we have to do the scaling in software and then convert it to a QPixmap to draw it. This happens on every resize, so it's a hot path and blocks direct user interaction. In my tests on different hardware (ATI, Intel, NVidia), the QPixmap based approach "felt" faster (smoother, more frames per second while resizing). The performance here depends on the painting backend used (image vs raster vs native) though, and thus per driver.

Maybe we can only do the image loading in a thread (since that can potentially take quite long for large images), and keep a smaller copy (screen resolution?) stored in a QPixmap which is then used to resize. (If a size larger than our screen resolution is required, we can on-demand load the full image (40000x30000px if you wish ;)) and scale from that. (Rationale upscaling == bad, downscaling == OK, saving mem == good)

The patch as it's now is not good enough IMO.


/trunk/KDE/kdeplasma-addons/applets/frame/frame.cpp
<http://reviewboard.kde.org/r/3162/#comment3907>

    This is a hot path (it gets called whenever the applet resizes, which can be quite often during drags). Doing a QImage -> QPixmap conversion here is not a good idea.
    
    The QPixmap-based version would just blit the pixmap into a smaller rect which is fast (it can be hw-accelerated)


- Sebastian


On 2010-03-08 02:27:47, Davide Bettio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3162/
> -----------------------------------------------------------
> 
> (Updated 2010-03-08 02:27:47)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> First of all: **This is a preview of the patch** I'm still working on it but I need some advices.
> Picture Frame does 2 really bad things (when used with huge images):
> * it loads huge images (in my case 4000x3000 pixels) into X11 pixmaps
> * plasma is blocked while the image is scaled
> As side effect also X11 freezes.
> 
> This patch solves both issues using threads.
> 
> I think that we should remove "smooth scaling" option from our configuration UI. what do you think?
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdeplasma-addons/applets/frame/CMakeLists.txt 1093624 
>   /trunk/KDE/kdeplasma-addons/applets/frame/appearanceSettings.ui 1093624 
>   /trunk/KDE/kdeplasma-addons/applets/frame/configdialog.h 1093624 
>   /trunk/KDE/kdeplasma-addons/applets/frame/configdialog.cpp 1093624 
>   /trunk/KDE/kdeplasma-addons/applets/frame/frame.h 1093624 
>   /trunk/KDE/kdeplasma-addons/applets/frame/frame.cpp 1093624 
>   /trunk/KDE/kdeplasma-addons/applets/frame/imageloader.cpp PRE-CREATION 
>   /trunk/KDE/kdeplasma-addons/applets/frame/imagescaler.cpp PRE-CREATION 
>   /trunk/KDE/kdeplasma-addons/applets/frame/picture.h 1093624 
>   /trunk/KDE/kdeplasma-addons/applets/frame/picture.cpp 1093624 
>   /trunk/KDE/kdeplasma-addons/applets/frame/slideshow.h 1093624 
>   /trunk/KDE/kdeplasma-addons/applets/frame/slideshow.cpp 1093624 
> 
> Diff: http://reviewboard.kde.org/r/3162/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Davide
> 
>