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

4 files changed, 118 insertions, 13 deletions

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
--- /dev/null
+++ b/tests/benchmarks/qgraphicsview/images/wine-big.jpeg
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 @@
 <qresource>
     <file>images/designer.png</file>
     <file>images/wine.jpeg</file>
+    <file>images/wine-big.jpeg</file>
     <file>random.data</file>
 </qresource>
 </RCC>
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<bool>("updatePartial");
+    QTest::addColumn<bool>("rotation");
+    QTest::addColumn<int>("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<QGraphicsItem *> 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"