Do metadata detach simpler than full detach

Avoid a full data detach when only metadata changes. This paradigm was already used one place, and made generic. Fixes: QTBUG-81674 Change-Id: I605253babc6ad9fc130e19e8cef3812690933ac5 Reviewed-by: Eirik Aavitsland <[email protected]>
leadtobetter · Jun 1, 2022 · f9df851 · f9df851
1 parent 8c5b1f3
commit f9df851
Show file tree

Hide file tree

Showing 3 changed files with 58 additions and 14 deletions.
diff --git a/src/gui/image/qimage.cpp b/src/gui/image/qimage.cpp
@@ -1104,6 +1104,28 @@ void QImage::detach()
 }
 
 
+/*!
+    \internal
+
+    A variant for metadata-only detach, which will not detach readonly image data,
+    and only invalidate caches of the image data if asked to.
+
+    \sa detach(), isDetached()
+*/
+void QImage::detachMetadata(bool invalidateCache)
+{
+    if (d) {
+        if (d->is_cached && d->ref.loadRelaxed() == 1)
+            QImagePixmapCleanupHooks::executeImageHooks(cacheKey());
+
+        if (d->ref.loadRelaxed() != 1)
+            *this = copy();
+
+        if (d && invalidateCache)
+            ++d->detach_no;
+    }
+}
+
 static void copyPhysicalMetadata(QImageData *dst, const QImageData *src)
 {
     dst->dpmx = src->dpmx;
@@ -1385,7 +1407,7 @@ void QImage::setColorTable(const QList<QRgb> &colors)
 {
     if (!d)
         return;
-    detach();
+    detachMetadata(true);
 
     // In case detach() ran out of memory
     if (!d)
@@ -1459,7 +1481,7 @@ void QImage::setDevicePixelRatio(qreal scaleFactor)
     if (scaleFactor == d->devicePixelRatio)
         return;
 
-    detach();
+    detachMetadata();
     if (d)
         d->devicePixelRatio = scaleFactor;
 }
@@ -1542,7 +1564,7 @@ void QImage::setColor(int i, QRgb c)
         qWarning("QImage::setColor: Index out of bound %d", i);
         return;
     }
-    detach();
+    detachMetadata(true);
 
     // In case detach() run out of memory
     if (!d)
@@ -2084,7 +2106,7 @@ void QImage::setColorCount(int colorCount)
         return;
     }
 
-    detach();
+    detachMetadata(true);
 
     // In case detach() ran out of memory
     if (!d)
@@ -4056,9 +4078,9 @@ int QImage::dotsPerMeterY() const
 */
 void QImage::setDotsPerMeterX(int x)
 {
-    if (!d || !x)
+    if (!d || !x || d->dpmx == x)
         return;
-    detach();
+    detachMetadata();
 
     if (d)
         d->dpmx = x;
@@ -4078,9 +4100,9 @@ void QImage::setDotsPerMeterX(int x)
 */
 void QImage::setDotsPerMeterY(int y)
 {
-    if (!d || !y)
+    if (!d || !y || d->dpmy == y)
         return;
-    detach();
+    detachMetadata();
 
     if (d)
         d->dpmy = y;
@@ -4110,9 +4132,9 @@ QPoint QImage::offset() const
 */
 void QImage::setOffset(const QPoint& p)
 {
-    if (!d)
+    if (!d || d->offset == p)
         return;
-    detach();
+    detachMetadata();
 
     if (d)
         d->offset = p;
@@ -4182,7 +4204,7 @@ void QImage::setText(const QString &key, const QString &value)
 {
     if (!d)
         return;
-    detach();
+    detachMetadata();
 
     if (d)
         d->text.insert(key, value);
@@ -4936,9 +4958,9 @@ void QImage::setColorSpace(const QColorSpace &colorSpace)
         return;
     if (d->colorSpace == colorSpace)
         return;
-    if (!isDetached()) // Detach only if shared, not for read-only data.
-        detach();
-    d->colorSpace = colorSpace;
+    detachMetadata(false);
+    if (d)
+        d->colorSpace = colorSpace;
 }
 
 /*!

diff --git a/src/gui/image/qimage.h b/src/gui/image/qimage.h
@@ -294,6 +294,8 @@ class Q_GUI_EXPORT QImage : public QPaintDevice
     bool convertToFormat_inplace(Format format, Qt::ImageConversionFlags flags);
     QImage smoothScaled(int w, int h) const;
 
+    void detachMetadata(bool invalidateCache = false);
+
 private:
     QImageData *d;
 

diff --git a/tests/auto/gui/image/qimage/tst_qimage.cpp b/tests/auto/gui/image/qimage/tst_qimage.cpp
@@ -226,6 +226,8 @@ private slots:
     void largeFillScale();
     void largeRasterScale();
 
+    void metadataChangeWithReadOnlyPixels();
+
 #if defined(Q_OS_WIN)
     void toWinHBITMAP_data();
     void toWinHBITMAP();
@@ -4038,6 +4040,24 @@ void tst_QImage::largeRasterScale()
 //    image.save("largeRasterScale.png", "PNG");
 }
 
+void tst_QImage::metadataChangeWithReadOnlyPixels()
+{
+    const QRgb data[3] = { qRgb(255, 0, 0), qRgb(0, 255, 0), qRgb(0, 0, 255) };
+    QImage image((const uchar *)data, 3, 1, QImage::Format_RGB32);
+
+    QCOMPARE(image.constBits(), (const uchar *)data);
+    image.setDotsPerMeterX(100);
+    QCOMPARE(image.constBits(), (const uchar *)data);
+
+    QImage image2 = image;
+    QCOMPARE(image2.constBits(), (const uchar *)data);
+    image2.setDotsPerMeterX(200);
+    // Pixels and metadata has the same sharing mechanism, so a change of a shared
+    // image metadata forces pixel detach (remove this sub-test if that ever changes).
+    QVERIFY(image2.constBits() != (const uchar *)data);
+    QCOMPARE(image.constBits(), (const uchar *)data);
+}
+
 #if defined(Q_OS_WIN)
 
 static inline QColor COLORREFToQColor(COLORREF cr)