From 80bc757b56953065fafeffe6c4b8fb6fbca287ac Mon Sep 17 00:00:00 2001 From: Alexis Menard Date: Wed, 27 May 2009 10:30:07 +0200 Subject: Avoid deep copies of pixmaps when using cache in QGraphicsView. When we have already a pixmap of an item in the cache, we should remove the entry from the cache otherwise when we will repaint/modify the copy of the pixmap (the copy of the cache) then we will trigger a deep copy ; this is because both entries in the cache and our copy share the same data and if you modify one of them a copy is triggered. I have added a benchmark as well to test that. Reviewed-by:bnilsen Reviewed-by:andreas --- src/gui/graphicsview/qgraphicsscene.cpp | 28 +++--- .../benchmarks/qgraphicsview/images/wine-big.jpeg | Bin 0 -> 12249 bytes tests/benchmarks/qgraphicsview/qgraphicsview.qrc | 1 + .../benchmarks/qgraphicsview/tst_qgraphicsview.cpp | 102 +++++++++++++++++++++ 4 files changed, 118 insertions(+), 13 deletions(-) create mode 100644 tests/benchmarks/qgraphicsview/images/wine-big.jpeg diff --git a/src/gui/graphicsview/qgraphicsscene.cpp b/src/gui/graphicsview/qgraphicsscene.cpp index 126ea5b..1fc4567 100644 --- a/src/gui/graphicsview/qgraphicsscene.cpp +++ b/src/gui/graphicsview/qgraphicsscene.cpp @@ -4784,6 +4784,12 @@ void QGraphicsScenePrivate::drawItemHelper(QGraphicsItem *item, QPainter *painte // Redraw any newly exposed areas. if (itemCache->allExposed || !itemCache->exposed.isEmpty()) { + + //We know that we will modify the pixmap, removing it from the cache + //will detach the one we have and avoid a deep copy + if (pixmapFound) + QPixmapCache::remove(pixmapKey); + // Fit the item's bounding rect into the pixmap's coordinates. QTransform itemToPixmap; if (fixedCacheSize) { @@ -4812,12 +4818,8 @@ void QGraphicsScenePrivate::drawItemHelper(QGraphicsItem *item, QPainter *painte _q_paintIntoCache(&pix, item, pixmapExposed, itemToPixmap, painter->renderHints(), &cacheOption, painterStateProtection); - if (!pixmapFound) { - // insert this pixmap into the cache. - itemCache->key = QPixmapCache::insert(pix); - } else { - QPixmapCache::replace(pixmapKey, pix); - } + // insert this pixmap into the cache. + itemCache->key = QPixmapCache::insert(pix); // Reset expose data. itemCache->allExposed = false; @@ -4944,6 +4946,11 @@ void QGraphicsScenePrivate::drawItemHelper(QGraphicsItem *item, QPainter *painte // Check for newly invalidated areas. if (itemCache->allExposed || !itemCache->exposed.isEmpty() || !scrollExposure.isEmpty()) { + //We know that we will modify the pixmap, removing it from the cache + //will detach the one we have and avoid a deep copy + if (pixmapFound) + QPixmapCache::remove(pixmapKey); + // Construct an item-to-pixmap transform. QPointF p = deviceRect.topLeft(); QTransform itemToPixmap = painter->worldTransform(); @@ -4984,13 +4991,8 @@ void QGraphicsScenePrivate::drawItemHelper(QGraphicsItem *item, QPainter *painte } if (pixModified) { - if (!pixmapFound) { - // Insert this pixmap into the cache. - deviceData->key = QPixmapCache::insert(pix); - } else { - //otherwise we replace the pixmap in the cache - QPixmapCache::replace(pixmapKey, pix); - } + // Insert this pixmap into the cache. + deviceData->key = QPixmapCache::insert(pix); } // Redraw the exposed area using an untransformed painter. This diff --git a/tests/benchmarks/qgraphicsview/images/wine-big.jpeg b/tests/benchmarks/qgraphicsview/images/wine-big.jpeg new file mode 100644 index 0000000..9900a50 Binary files /dev/null and b/tests/benchmarks/qgraphicsview/images/wine-big.jpeg differ diff --git a/tests/benchmarks/qgraphicsview/qgraphicsview.qrc b/tests/benchmarks/qgraphicsview/qgraphicsview.qrc index 5e80029..3681648 100644 --- a/tests/benchmarks/qgraphicsview/qgraphicsview.qrc +++ b/tests/benchmarks/qgraphicsview/qgraphicsview.qrc @@ -2,6 +2,7 @@ images/designer.png images/wine.jpeg + images/wine-big.jpeg random.data diff --git a/tests/benchmarks/qgraphicsview/tst_qgraphicsview.cpp b/tests/benchmarks/qgraphicsview/tst_qgraphicsview.cpp index a06e033..570f744 100644 --- a/tests/benchmarks/qgraphicsview/tst_qgraphicsview.cpp +++ b/tests/benchmarks/qgraphicsview/tst_qgraphicsview.cpp @@ -124,6 +124,8 @@ private slots: void textRiver(); void moveItemCache_data(); void moveItemCache(); + void paintItemCache_data(); + void paintItemCache(); }; tst_QGraphicsView::tst_QGraphicsView() @@ -796,5 +798,105 @@ void tst_QGraphicsView::moveItemCache() } } +class UpdatedPixmapCacheItem : public QGraphicsPixmapItem +{ +public: + UpdatedPixmapCacheItem(bool partial, QGraphicsItem *parent = 0) + : QGraphicsPixmapItem(parent), partial(partial) + { + } + + void paint(QPainter *painter, const QStyleOptionGraphicsItem *option, QWidget *widget = 0) + { + QGraphicsPixmapItem::paint(painter,option,widget); + } +protected: + void advance(int i) + { + if (partial) + update(QRectF(boundingRect().center().x(), boundingRect().center().x(), 30, 30)); + else + update(); + } + +private: + bool partial; +}; + +void tst_QGraphicsView::paintItemCache_data() +{ + QTest::addColumn("updatePartial"); + QTest::addColumn("rotation"); + QTest::addColumn("cacheMode"); + QTest::newRow("Partial Update : ItemCoordinate Cache") << true << false << (int)QGraphicsItem::ItemCoordinateCache; + QTest::newRow("Partial Update : DeviceCoordinate Cache") << true << false << (int)QGraphicsItem::DeviceCoordinateCache; + QTest::newRow("Partial Update : No Cache") << true << false << (int)QGraphicsItem::NoCache; + QTest::newRow("Full Update : ItemCoordinate Cache") << false << false << (int)QGraphicsItem::ItemCoordinateCache; + QTest::newRow("Full Update : DeviceCoordinate Cache") << false << false << (int)QGraphicsItem::DeviceCoordinateCache; + QTest::newRow("Full Update : No Cache") << false << false << (int)QGraphicsItem::NoCache; + QTest::newRow("Partial Update : ItemCoordinate Cache item rotated") << true << true << (int)QGraphicsItem::ItemCoordinateCache; + QTest::newRow("Partial Update : DeviceCoordinate Cache item rotated") << true << true << (int)QGraphicsItem::DeviceCoordinateCache; + QTest::newRow("Partial Update : No Cache item rotated") << true << true << (int)QGraphicsItem::NoCache; + QTest::newRow("Full Update : ItemCoordinate Cache item rotated") << false << true << (int)QGraphicsItem::ItemCoordinateCache; + QTest::newRow("Full Update : DeviceCoordinate Cache item rotated") << false << true << (int)QGraphicsItem::DeviceCoordinateCache; + QTest::newRow("Full Update : No Cache item rotated") << false << true <<(int)QGraphicsItem::NoCache; +} + +void tst_QGraphicsView::paintItemCache() +{ + QFETCH(bool, updatePartial); + QFETCH(bool, rotation); + QFETCH(int, cacheMode); + + QGraphicsScene scene(0, 0, 300, 300); + + CountPaintEventView view(&scene); + view.resize(600, 600); + view.setFrameStyle(0); + view.setHorizontalScrollBarPolicy(Qt::ScrollBarAlwaysOff); + view.setVerticalScrollBarPolicy(Qt::ScrollBarAlwaysOff); + view.show(); + + QPixmap pix(":/images/wine.jpeg"); + QVERIFY(!pix.isNull()); + + QList items; + QFile file(":/random.data"); + QVERIFY(file.open(QIODevice::ReadOnly)); + QDataStream str(&file); + UpdatedPixmapCacheItem *item = new UpdatedPixmapCacheItem(updatePartial); + item->setPixmap(pix); + item->setCacheMode((QGraphicsItem::CacheMode)cacheMode); + if (rotation) + item->setTransform(QTransform().rotate(45)); + item->setPos(-100, -100); + scene.addItem(item); + + QPixmap pix2(":/images/wine-big.jpeg"); + item = new UpdatedPixmapCacheItem(updatePartial); + item->setPixmap(pix2); + item->setCacheMode((QGraphicsItem::CacheMode)cacheMode); + if (rotation) + item->setTransform(QTransform().rotate(45)); + item->setPos(0, 0); + scene.addItem(item); + + view.count = 0; + + QBENCHMARK { +#ifdef CALLGRIND_DEBUG + CALLGRIND_START_INSTRUMENTATION +#endif + for (int i = 0; i < 50; ++i) { + scene.advance(); + while (view.count < (i+1)) + qApp->processEvents(); + } +#ifdef CALLGRIND_DEBUG + CALLGRIND_STOP_INSTRUMENTATION +#endif + } +} + QTEST_MAIN(tst_QGraphicsView) #include "tst_qgraphicsview.moc" -- cgit v0.12