[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-commits
Subject:    KDE/kdegames/kpat/libkcardgame
From:       Parker Coates <parker.coates () kdemail ! net>
Date:       2011-01-16 18:36:33
Message-ID: 20110116183633.0899F3E1F1 () svn ! kde ! org
[Download RAW message or body]

SVN commit 1214866 by coates:

Fix a bug where painting caused near 100% CPU usage and mis-sized cards.

This one was a bit of a doozy. The route of the problem is KImageCache's
in memory pixmap cache was occasionally returning the wrong pixmap for
a given key. Why that was happening I still don't know. So a card would
go to paint itself and notice that its pixmap was the wrong size, so it
would ask for a new one. The KAbstractCardDeck would take the request,
notice that the requested image/size was in the cache and return the
cached version, but unfornuately this was actually the wrong pixmap and
had the wrong size. The card happily used this pixmap and painted itself
but the next time it went to paint it noticed that its pixmap was the
wrong size and the loop repeated, hence the high processor usage.

I've disabled the in memory pixmap cache as the benefit wasn't very
significant and I couldn't figure out what the issue actually was. There
were also some problems with the KCard and KAbstractCardDeck code that
made the problem worse than it should have been. Those have been fixed
as well.

BUG:255933

 M  +6 -1      common.cpp  
 M  +17 -24    kabstractcarddeck.cpp  
 M  +4 -0      kcard.cpp  


--- trunk/KDE/kdegames/kpat/libkcardgame/common.cpp #1214865:1214866
@@ -42,9 +42,14 @@
 
     QString cacheName = QString( cacheNameTemplate ).arg( theme.dirName() );
     cache = new KImageCache( cacheName, 3 * 1024 * 1024 );
-    cache->setPixmapCaching( true );
     cache->setEvictionPolicy( KSharedDataCache::EvictLeastRecentlyUsed );
 
+    // Enabling the pixmap cache has caused issues: we were getting back
+    // different pixmaps than we had inserted. We keep a partial cache of the
+    // pixmaps in KAbstractCardDeck already, so the builtin pixmap caching
+    // doesn't really add that much benefit anyway.
+    cache->setPixmapCaching( false );
+
     QDateTime cacheTimeStamp;
     if ( !cacheFind( cache, timeStampKey, &cacheTimeStamp )
          || cacheTimeStamp < theme.lastModified() )
--- trunk/KDE/kdegames/kpat/libkcardgame/kabstractcarddeck.cpp #1214865:1214866
@@ -88,7 +88,7 @@
         {
             kDebug() << "Renderering" << key << "in rendering thread.";
 
-            QImage img = QImage( m_size, QImage::Format_ARGB32 );
+            QImage img = QImage( m_size, QImage::Format_ARGB32_Premultiplied );
             img.fill( Qt::transparent );
             QPainter p( &img );
             {
@@ -144,18 +144,12 @@
 
 QPixmap KAbstractCardDeckPrivate::renderCard( const QString & element )
 {
-    QPixmap pix;
-    if ( !theme.isValid() || currentCardSize.isEmpty() )
-        return pix;
-
     QString key = keyForPixmap( element , currentCardSize );
-    if ( !cache->findPixmap( key, &pix ) )
-    {
         kDebug() << "Renderering" << key << "in main thread.";
 
-        pix = QPixmap( currentCardSize );
-        pix.fill( Qt::transparent );
-        QPainter p( &pix );
+    QImage img( currentCardSize, QImage::Format_ARGB32_Premultiplied );
+    img.fill( Qt::transparent );
+    QPainter p( &img );
         {
             QMutexLocker l( &rendererMutex );
             if ( renderer()->elementExists( element ) )
@@ -165,19 +159,18 @@
             else
             {
                 kWarning() << "Could not find" << element << "in SVG.";
-                p.fillRect( QRect( 0, 0, pix.width(), pix.height() ), Qt::white );
+            p.fillRect( QRect( 0, 0, img.width(), img.height() ), Qt::white );
                 p.setPen( Qt::red );
-                p.drawLine( 0, 0, pix.width(), pix.height() );
-                p.drawLine( pix.width(), 0, 0, pix.height() );
+            p.drawLine( 0, 0, img.width(), img.height() );
+            p.drawLine( img.width(), 0, 0, img.height() );
                 p.end();
             }
         }
         p.end();
 
-        cache->insertPixmap( key, pix );
-    }
+    cache->insertImage( key, img );
 
-    return pix;
+    return QPixmap::fromImage( img );
 }
 
 
@@ -223,6 +216,7 @@
             else
                 stored = stored.scaled( currentCardSize );
         }
+        Q_ASSERT( stored.size() == currentCardSize );
     }
     return stored;
 }
@@ -241,6 +235,11 @@
 {
     QPixmap pix;
 
+    // If the currentCardSize has changed since the rendering was performed,
+    // we sadly just have to throw it away.
+    if ( image.size() != currentCardSize )
+        return;
+
     // The RenderingThread just put the image in the cache, but due to the
     // volatility of the cache there's no guarantee that it'll still be there
     // by the time this slot is called, in which case we convert the QImage
@@ -400,6 +399,8 @@
 
     if ( newSize != d->currentCardSize )
     {
+        d->deleteThread();
+
         d->currentCardSize = newSize;
 
         if ( !d->theme.isValid() )
@@ -407,14 +408,6 @@
 
         cacheInsert( d->cache, lastUsedSizeKey, d->currentCardSize );
 
-        foreach ( KCard * c, d->cards )
-        {
-            c->setFrontPixmap( d->requestPixmap( c->id(), true ) );
-            c->setBackPixmap( d->requestPixmap( c->id(), false ) );
-        }
-
-        d->deleteThread();
-
         QStringList elementsToRender = d->frontIndex.keys() + d->backIndex.keys();
         d->thread = new RenderingThread( d, d->currentCardSize, elementsToRender );
         d->thread->start();
--- trunk/KDE/kdegames/kpat/libkcardgame/kcard.cpp #1214865:1214866
@@ -321,6 +321,10 @@
             setFrontPixmap( newPix );
         else
             setBackPixmap( newPix );
+
+        // Changing the pixmap will call update() and force a repaint, so we
+        // might as well return early.
+        return;
     }
 
     // Enable smooth pixmap transformation only if the card is rotated. We
[prev in list] [next in list] [prev in thread] [next in thread]