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

[Sebastian Kügler]

On Monday 08 March 2010 14:33:12 Davide Bettio wrote:
> > On 2010-03-08 11:47:49, Sebastian Kügler wrote:
> > > 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.
> 
> We must do software scaling because loading a 4000x3000 pixmap into X11 and
> asking X11 to scale the pixmap is a bad idea and it freezes the whole X11
> environment including plasma and kwin for at least a couple of seconds
> (that happpens on a recent nvidia quadro, and I wonder what happens on an
> ATI video card powered with buggy drivers). With this patch during resize
> paintInterface is called and the cached QPixmap is used, in the meantime
> another thread with a completely async behaviour scales the image.
> 
> > 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)
> 
> Do you know how to load our image on graphics card memory without blocking
> everything? (don't forget that 4000x3000 px images are about 45 mb of raw
> ARGB data) 

The trick is: 

- do not load the data more often than necessary
- keep a scaled version that can be assumed to be big enough in most cases in memory 
  to do the most often occurring transformation on (usually scaling, as it's a hot 
  path)

> And moreover what's the point of using video card memory since
> system memory is larger (think about laptops too) when moreover we don't
> need to use any video card feature?

Scaling on the GPU is a video card feature. Using a QImage (and thus having graphics 
data in system memory). Your assumption that system memory is larger than video 
memory is bogus, that way why should one use video memory at all? (Exercise for the 
reader: What's the cost of copying the image from system mem into the graphics mem, 
and does a QImage not use graphics memory at all?)

> I can try anyway to reduce the huge in
> memory image footprint, but honestly I think that I should commit this
> patch before and then I can solve that problem. 

I think that's a very bad idea, given your history. I've seen more than once that you 
commit stuff and then just ignore requests that have been made after someone saw 
those patches. One that comes to my mind specifically is where I asked you to not 
move the power management KCM to the advanced section in system settings, which you 
committed without review and subsequently just ignored the follow-up email.

I'm afraid "committing, then solve problems" is not the way to go.

> Anyway a solution might be
> to start after N seconds a thread with really low priority that replaces
> the huge in memory image with a screen-resolution-one (doing this task
> during image load and display process might be a bad idea).

That can be done, yes, not sure how much of a difference it makes though.

BTW, did you actually *measure* how much time the loading takes vs. scaling the 
image, and then using QImage vs. QPixmap? Scaling using a QPixmap and drawRect() 
should be very fast (which is why I've done it this way in the picture frame applet).

> > Maybe we can only do the image loading in a thread (since that can
> > potentially take quite long for large images).
> 
> This will prevent to freeze plasma during image loading but then X11 will
> be freezed anyway.

That's the problem of QImage usage. Solution: 
(1) load the image in a thread
(2) try to prevent format changes (QImage vs. QPixmap) as much as possible
(3) use hw acceleration for image scaling

Your patch does (1) but makes (2) worse since it mixes QImage and QPixmaps. The 
QImage usage makes (3) impossible.

> I might try to improve also the behaviour using a thread with a bit lower
> priority than the default priority.

The priority of the thread has nothing to do with the issues at hand.

> Anyway don't forget that we are using software scaling also for wallpapers
> since KDE 4.0 and it works nicely.

Wallpapers aren't normally resized, so the issue is much less important.

So still NAK.
-- 
sebas

http://www.kde.org | http://vizZzion.org | GPG Key ID: 9119 0EF9