From c999065d5090a64192f96bed78c5224490409d6a Mon Sep 17 00:00:00 2001 From: Alexis Menard Date: Tue, 29 Sep 2009 09:50:07 +0200 Subject: Fix a bug in QPixmapCache when the cache is trimmed by QCache. There was a bug in QPixmapCache when QCache trims the content, some keys were not invalidated. The ifdef for WinCE (that i removed) was a wrong fix, it let the auto-test pass but it doesn't fix the bug. The approach here is to add a QPixmapCacheEntry that release the key it owns when QCache deletes it : we are now sure that nothing happen in our back. Reviewed-by:paul Reviewed-by:trond --- src/gui/image/qimage.h | 2 +- src/gui/image/qpixmap.h | 2 +- src/gui/image/qpixmap_raster_p.h | 2 +- src/gui/image/qpixmapcache.cpp | 66 +++++++++++++++------------- src/gui/image/qpixmapcache_p.h | 6 ++- tests/auto/qpixmapcache/tst_qpixmapcache.cpp | 22 +++++----- 6 files changed, 52 insertions(+), 48 deletions(-) diff --git a/src/gui/image/qimage.h b/src/gui/image/qimage.h index 89d7de5..1ac56a7 100644 --- a/src/gui/image/qimage.h +++ b/src/gui/image/qimage.h @@ -314,7 +314,7 @@ private: QImageData *d; friend class QRasterPixmapData; - friend class QDetachedPixmap; + friend class QPixmapCacheEntry; friend Q_GUI_EXPORT qint64 qt_image_id(const QImage &image); friend const QVector *qt_image_colortable(const QImage &image); diff --git a/src/gui/image/qpixmap.h b/src/gui/image/qpixmap.h index a891637..d11bd03 100644 --- a/src/gui/image/qpixmap.h +++ b/src/gui/image/qpixmap.h @@ -270,7 +270,7 @@ private: friend class QWidgetPrivate; friend class QRasterPaintEngine; friend class QRasterBuffer; - friend class QDetachedPixmap; + friend class QPixmapCacheEntry; #if !defined(QT_NO_DATASTREAM) friend Q_GUI_EXPORT QDataStream &operator>>(QDataStream &, QPixmap &); #endif diff --git a/src/gui/image/qpixmap_raster_p.h b/src/gui/image/qpixmap_raster_p.h index 2af2399..da0405e 100644 --- a/src/gui/image/qpixmap_raster_p.h +++ b/src/gui/image/qpixmap_raster_p.h @@ -88,7 +88,7 @@ protected: private: friend class QPixmap; friend class QBitmap; - friend class QDetachedPixmap; + friend class QPixmapCacheEntry; friend class QRasterPaintEngine; }; diff --git a/src/gui/image/qpixmapcache.cpp b/src/gui/image/qpixmapcache.cpp index 8029977..f12d397 100644 --- a/src/gui/image/qpixmapcache.cpp +++ b/src/gui/image/qpixmapcache.cpp @@ -167,7 +167,7 @@ QPixmapCache::Key &QPixmapCache::Key::operator =(const Key &other) return *this; } -class QPMCache : public QObject, public QCache +class QPMCache : public QObject, public QCache { Q_OBJECT public: @@ -215,7 +215,7 @@ uint qHash(const QPixmapCache::Key &k) QPMCache::QPMCache() : QObject(0), - QCache(cache_limit * 1024), + QCache(cache_limit * 1024), keyArray(0), theid(0), ps(0), keyArraySize(0), freeKey(0), t(false) { } @@ -238,7 +238,6 @@ void QPMCache::timerEvent(QTimerEvent *) { int mc = maxCost(); bool nt = totalCost() == ps; - QList keys = QCache::keys(); setMaxCost(nt ? totalCost() * 3 / 4 : totalCost() -1); setMaxCost(mc); ps = totalCost(); @@ -252,10 +251,6 @@ void QPMCache::timerEvent(QTimerEvent *) ++it; } } - for (int i = 0; i < keys.size(); ++i) { - if (!contains(keys.at(i))) - releaseKey(keys.at(i)); - } if (!size()) { killTimer(theid); @@ -274,11 +269,10 @@ QPixmap *QPMCache::object(const QString &key) const const_cast(this)->cacheKeys.remove(key); return 0; } - QPixmap *ptr = QCache::object(cacheKey); + QPixmap *ptr = QCache::object(cacheKey); //We didn't find the pixmap in the cache, the key is not valid anymore if (!ptr) { const_cast(this)->cacheKeys.remove(key); - const_cast(this)->releaseKey(cacheKey); } return ptr; } @@ -286,7 +280,7 @@ QPixmap *QPMCache::object(const QString &key) const QPixmap *QPMCache::object(const QPixmapCache::Key &key) const { Q_ASSERT(key.d->isValid); - QPixmap *ptr = QCache::object(key); + QPixmap *ptr = QCache::object(key); //We didn't find the pixmap in the cache, the key is not valid anymore if (!ptr) const_cast(this)->releaseKey(key); @@ -299,13 +293,14 @@ bool QPMCache::insert(const QString& key, const QPixmap &pixmap, int cost) QPixmapCache::Key oldCacheKey = cacheKeys.value(key); //If for the same key we add already a pixmap we should delete it if (oldCacheKey.d) { - QCache::remove(oldCacheKey); - cacheKey = oldCacheKey; - } else { - cacheKey = createKey(); - } + QCache::remove(oldCacheKey); + cacheKeys.remove(key); + } + + //we create a new key the old one has been removed + cacheKey = createKey(); - bool success = QCache::insert(cacheKey, new QDetachedPixmap(pixmap), cost); + bool success = QCache::insert(cacheKey, new QPixmapCacheEntry(cacheKey, pixmap), cost); if (success) { cacheKeys.insert(key, cacheKey); if (!theid) { @@ -322,7 +317,7 @@ bool QPMCache::insert(const QString& key, const QPixmap &pixmap, int cost) QPixmapCache::Key QPMCache::insert(const QPixmap &pixmap, int cost) { QPixmapCache::Key cacheKey = createKey(); - bool success = QCache::insert(cacheKey, new QDetachedPixmap(pixmap), cost); + bool success = QCache::insert(cacheKey, new QPixmapCacheEntry(cacheKey, pixmap), cost); if (success) { if (!theid) { theid = startTimer(30000); @@ -338,13 +333,21 @@ QPixmapCache::Key QPMCache::insert(const QPixmap &pixmap, int cost) bool QPMCache::replace(const QPixmapCache::Key &key, const QPixmap &pixmap, int cost) { Q_ASSERT(key.d->isValid); - //If for the same key we add already a pixmap we should delete it - QCache::remove(key); + //If for the same key we had already an entry so we should delete the pixmap and use the new one + QCache::remove(key); + + QPixmapCache::Key cacheKey = createKey(); - bool success = QCache::insert(key, new QDetachedPixmap(pixmap), cost); - if (success && !theid) { - theid = startTimer(30000); - t = false; + bool success = QCache::insert(cacheKey, new QPixmapCacheEntry(cacheKey, pixmap), cost); + if (success) { + if(!theid) { + theid = startTimer(30000); + t = false; + } + const_cast(key) = cacheKey; + } else { + //Insertion failed we released the key + releaseKey(cacheKey); } return success; } @@ -356,16 +359,12 @@ bool QPMCache::remove(const QString &key) if (!cacheKey.d) return false; cacheKeys.remove(key); - releaseKey(cacheKey); - return QCache::remove(cacheKey); + return QCache::remove(cacheKey); } bool QPMCache::remove(const QPixmapCache::Key &key) { - bool result = QCache::remove(key); - //We release the key after we removed it from the cache - releaseKey(key); - return result; + return QCache::remove(key); } void QPMCache::resizeKeyArray(int size) @@ -409,10 +408,10 @@ void QPMCache::clear() freeKey = 0; keyArraySize = 0; //Mark all keys as invalid - QList keys = QCache::keys(); + QList keys = QCache::keys(); for (int i = 0; i < keys.size(); ++i) keys.at(i).d->isValid = false; - QCache::clear(); + QCache::clear(); } QPixmapCache::KeyData* QPMCache::getKeyData(QPixmapCache::Key *key) @@ -424,6 +423,11 @@ QPixmapCache::KeyData* QPMCache::getKeyData(QPixmapCache::Key *key) Q_GLOBAL_STATIC(QPMCache, pm_cache) +QPixmapCacheEntry::~QPixmapCacheEntry() +{ + pm_cache()->releaseKey(key); +} + /*! \obsolete \overload diff --git a/src/gui/image/qpixmapcache_p.h b/src/gui/image/qpixmapcache_p.h index 33f93bc..84e4a03 100644 --- a/src/gui/image/qpixmapcache_p.h +++ b/src/gui/image/qpixmapcache_p.h @@ -76,10 +76,10 @@ public: }; // XXX: hw: is this a general concept we need to abstract? -class QDetachedPixmap : public QPixmap +class QPixmapCacheEntry : public QPixmap { public: - QDetachedPixmap(const QPixmap &pix) : QPixmap(pix) + QPixmapCacheEntry(const QPixmapCache::Key &key, const QPixmap &pix) : QPixmap(pix), key(key) { if (data && data->classId() == QPixmapData::RasterClass) { QRasterPixmapData *d = static_cast(data.data()); @@ -91,6 +91,8 @@ public: } } } + ~QPixmapCacheEntry(); + QPixmapCache::Key key; }; QT_END_NAMESPACE diff --git a/tests/auto/qpixmapcache/tst_qpixmapcache.cpp b/tests/auto/qpixmapcache/tst_qpixmapcache.cpp index 6d262ba..b487d74 100644 --- a/tests/auto/qpixmapcache/tst_qpixmapcache.cpp +++ b/tests/auto/qpixmapcache/tst_qpixmapcache.cpp @@ -171,7 +171,7 @@ void tst_QPixmapCache::setCacheLimit() QPixmapCache::setCacheLimit(0); QPixmapCache::setCacheLimit(1000); QPixmapCache::Key key2 = QPixmapCache::insert(*p1); - QCOMPARE(getPrivate(key2)->key, 2); + QCOMPARE(getPrivate(key2)->key, 1); QVERIFY(QPixmapCache::find(key, &p2) == 0); QVERIFY(QPixmapCache::find(key2, &p2) != 0); QCOMPARE(p2, *p1); @@ -200,7 +200,7 @@ void tst_QPixmapCache::find() { QPixmap p1(10, 10); p1.fill(Qt::red); - QPixmapCache::insert("P1", p1); + QVERIFY(QPixmapCache::insert("P1", p1)); QPixmap p2; QVERIFY(QPixmapCache::find("P1", p2)); @@ -222,13 +222,13 @@ void tst_QPixmapCache::find() QCOMPARE(p1, p2); QPixmapCache::clear(); + QPixmapCache::setCacheLimit(128); key = QPixmapCache::insert(p1); //The int part of the API - // make sure it doesn't explode QList keys; - for (int i = 0; i < 40000; ++i) + for (int i = 0; i < 4000; ++i) QPixmapCache::insert(p1); //at that time the first key has been erase because no more place in the cache @@ -293,8 +293,6 @@ void tst_QPixmapCache::insert() estimatedNum = (1024 * QPixmapCache::cacheLimit()) / ((p1.width() * p1.height() * p1.depth()) / 8); QVERIFY(num <= estimatedNum); - QPixmapCache::insert(p3); - } void tst_QPixmapCache::replace() @@ -307,13 +305,16 @@ void tst_QPixmapCache::replace() p2.fill(Qt::yellow); QPixmapCache::Key key = QPixmapCache::insert(p1); + QCOMPARE(getPrivate(key)->isValid, true); QPixmap p3; QVERIFY(QPixmapCache::find(key, &p3) == 1); - QPixmapCache::replace(key,p2); + QPixmapCache::replace(key, p2); QVERIFY(QPixmapCache::find(key, &p3) == 1); + QCOMPARE(getPrivate(key)->isValid, true); + QCOMPARE(getPrivate(key)->key, 1); QCOMPARE(p3.width(), 10); QCOMPARE(p3.height(), 10); @@ -392,11 +393,8 @@ void tst_QPixmapCache::clear() QPixmap p1(10, 10); p1.fill(Qt::red); -#ifdef Q_OS_WINCE - const int numberOfKeys = 10000; -#else - const int numberOfKeys = 20000; -#endif + const int numberOfKeys = 40000; + for (int i = 0; i < numberOfKeys; ++i) QVERIFY(QPixmapCache::find("x" + QString::number(i)) == 0); -- cgit v0.12