QIcon/QIconLoader: fix usage of scaledPixmap()

QIconEngine::scaledPixmap() gets the unscaled size of the pixmap, not
the scaled one. This is correctly handled by QSvgIconEngine,
QAppleIconEngine, QAndroidPlatformIconEngine and QWindowsIconEngine
but not internally. Therefore fix this here and also make sure the
pixmap with the correct dpr is saved in the QPixmapCache to avoid a
detach resulting in an increased memory usage when the pixmap dpr did
not match the expected dpr.

Fixes: QTBUG-124573
Change-Id: Ic75d7a89dae89da326c72cac326490f49d135fa7
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>
(cherry picked from commit 245bdc8ec31755d6ab38b796014bdcab6a1d17ae)

src/gui/image/qabstractfileiconengine.cpp

@@ -28,6 +28,11 @@ using namespace Qt::StringLiterals;
 */
 QPixmap QAbstractFileIconEngine::pixmap(const QSize &size, QIcon::Mode mode,
                                         QIcon::State state)
+{
+    return scaledPixmap(size, mode, state, 1.0);
+}
+
+QPixmap QAbstractFileIconEngine::scaledPixmap(const QSize &size, QIcon::Mode mode, QIcon::State state, qreal scale)
 {
     Q_UNUSED(mode);
     Q_UNUSED(state);
@@ -37,13 +42,13 @@ QPixmap QAbstractFileIconEngine::pixmap(const QSize &size, QIcon::Mode mode,
 
     QString key = cacheKey();
     if (key.isEmpty())
-        return filePixmap(size, mode, state);
+        return filePixmap(size * scale, mode, state);
 
     key += u'_' + QString::number(size.width());
 
     QPixmap result;
     if (!QPixmapCache::find(key, &result)) {
-        result = filePixmap(size, mode, state);
+        result = filePixmap(size * scale, mode, state);
         if (!result.isNull())
             QPixmapCache::insert(key, result);
     }
@@ -51,12 +56,6 @@ QPixmap QAbstractFileIconEngine::pixmap(const QSize &size, QIcon::Mode mode,
     return result;
 }
 
-QPixmap QAbstractFileIconEngine::scaledPixmap(const QSize &size, QIcon::Mode mode, QIcon::State state, qreal scale)
-{
-    Q_UNUSED(scale); // (size is pre-multiplied by scale)
-    return pixmap(size, mode, state);
-}
-
 QSize QAbstractFileIconEngine::actualSize(const QSize &size, QIcon::Mode mode,
                                           QIcon::State state)
 {

src/gui/image/qicon.cpp

@@ -209,7 +209,7 @@ static QPixmapIconEngineEntry *bestSizeScaleMatch(const QSize &size, qreal scale
         return (qAbs(ascore) < qAbs(bscore)) ? pa : pb;
     }
 
-    qint64 s = area(size);
+    qint64 s = area(size * scale);
     if (pa->size == QSize() && pa->pixmap.isNull()) {
         pa->pixmap = QPixmap(pa->fileName);
         pa->size = pa->pixmap.size();
@@ -346,13 +346,15 @@ QPixmap QPixmapIconEngine::scaledPixmap(const QSize &size, QIcon::Mode mode, QIc
         return scaledPixmap(size, mode, state, scale);
     }
 
-    const auto actualSize = adjustSize(size, pm.size());
+    const auto actualSize = adjustSize(size * scale, pm.size());
+    const auto calculatedDpr = QIconPrivate::pixmapDevicePixelRatio(scale, size, actualSize);
     QString key = "qt_"_L1
                   % HexString<quint64>(pm.cacheKey())
                   % HexString<quint8>(pe->mode)
                   % HexString<quint64>(QGuiApplication::palette().cacheKey())
                   % HexString<uint>(actualSize.width())
-                  % HexString<uint>(actualSize.height());
+                  % HexString<uint>(actualSize.height())
+                  % HexString<quint16>(qRound(calculatedDpr * 1000));
 
     if (mode == QIcon::Active) {
         if (QPixmapCache::find(key % HexString<quint8>(mode), &pm))
@@ -376,6 +378,7 @@ QPixmap QPixmapIconEngine::scaledPixmap(const QSize &size, QIcon::Mode mode, QIc
             if (!generated.isNull())
                 pm = generated;
         }
+        pm.setDevicePixelRatio(calculatedDpr);
         QPixmapCache::insert(key % HexString<quint8>(mode), pm);
     }
     return pm;
@@ -415,7 +418,8 @@ QList<QSize> QPixmapIconEngine::availableSizes(QIcon::Mode mode, QIcon::State st
 void QPixmapIconEngine::addPixmap(const QPixmap &pixmap, QIcon::Mode mode, QIcon::State state)
 {
     if (!pixmap.isNull()) {
-        QPixmapIconEngineEntry *pe = tryMatch(pixmap.size(), pixmap.devicePixelRatio(), mode, state);
+        QPixmapIconEngineEntry *pe = tryMatch(pixmap.size() / pixmap.devicePixelRatio(),
+                                              pixmap.devicePixelRatio(), mode, state);
         if (pe && pe->size == pixmap.size() && pe->pixmap.devicePixelRatio() == pixmap.devicePixelRatio()) {
             pe->pixmap = pixmap;
             pe->fileName.clear();
@@ -905,7 +909,7 @@ QPixmap QIcon::pixmap(const QSize &size, qreal devicePixelRatio, Mode mode, Stat
     }
 
     // Try get a pixmap that is big enough to be displayed at device pixel resolution.
-    QPixmap pixmap = d->engine->scaledPixmap(size * devicePixelRatio, mode, state, devicePixelRatio);
+    QPixmap pixmap = d->engine->scaledPixmap(size, mode, state, devicePixelRatio);
     pixmap.setDevicePixelRatio(d->pixmapDevicePixelRatio(devicePixelRatio, size, pixmap.size()));
     return pixmap;
 }

src/gui/image/qiconengine.cpp

@@ -42,6 +42,9 @@ QT_BEGIN_NAMESPACE
 /*!  Returns the actual size of the icon the engine provides for the
   requested \a size, \a mode and \a state. The default implementation
   returns the given \a size.
+
+  The returned size is in device-independent pixels (This
+  is relevant for high-dpi pixmaps).
  */
 QSize QIconEngine::actualSize(const QSize &size, QIcon::Mode /*mode*/, QIcon::State /*state*/)
 {
@@ -234,10 +237,10 @@ void QIconEngine::virtual_hook(int id, void *data)
 {
     switch (id) {
     case QIconEngine::ScaledPixmapHook: {
-        // We don't have any notion of scale besides "@nx", so just call pixmap() here.
         QIconEngine::ScaledPixmapArgument &arg =
             *reinterpret_cast<QIconEngine::ScaledPixmapArgument*>(data);
-        arg.pixmap = pixmap(arg.size, arg.mode, arg.state);
+        // try to get a pixmap with the correct size, the dpr is adjusted later on
+        arg.pixmap = pixmap(arg.size * arg.scale, arg.mode, arg.state);
         break;
     }
     default:
@@ -282,7 +285,7 @@ bool QIconEngine::isNull()
     Returns a pixmap for the given \a size, \a mode, \a state and \a scale.
 
     The \a scale argument is typically equal to the \l {High DPI}
-    {device pixel ratio} of the display.
+    {device pixel ratio} of the display. The size is given in device-independent pixels.
 
     \include qiconengine-virtualhookhelper.qdocinc
 

src/gui/image/qiconloader.cpp

@@ -900,7 +900,7 @@ QSize QIconLoaderEngine::actualSize(const QSize &size, QIcon::Mode mode,
     return QSize(0, 0);
 }
 
-QPixmap PixmapEntry::pixmap(const QSize &size, QIcon::Mode mode, QIcon::State state)
+QPixmap PixmapEntry::pixmap(const QSize &size, QIcon::Mode mode, QIcon::State state, qreal scale)
 {
     Q_UNUSED(state);
 
@@ -911,13 +911,15 @@ QPixmap PixmapEntry::pixmap(const QSize &size, QIcon::Mode mode, QIcon::State st
 
     // If the size of the best match we have (basePixmap) is larger than the
     // requested size, we downscale it to match.
-    const auto actualSize = QPixmapIconEngine::adjustSize(size, basePixmap.size());
+    const auto actualSize = QPixmapIconEngine::adjustSize(size * scale, basePixmap.size());
+    const auto calculatedDpr = QIconPrivate::pixmapDevicePixelRatio(scale, size, actualSize);
     QString key = "$qt_theme_"_L1
                   % HexString<quint64>(basePixmap.cacheKey())
                   % HexString<quint8>(mode)
                   % HexString<quint64>(QGuiApplication::palette().cacheKey())
                   % HexString<uint>(actualSize.width())
-                  % HexString<uint>(actualSize.height());
+                  % HexString<uint>(actualSize.height())
+                  % HexString<quint16>(qRound(calculatedDpr * 1000));
 
     QPixmap cachedPixmap;
     if (QPixmapCache::find(key, &cachedPixmap)) {
@@ -929,32 +931,24 @@ QPixmap PixmapEntry::pixmap(const QSize &size, QIcon::Mode mode, QIcon::State st
             cachedPixmap = basePixmap;
         if (QGuiApplication *guiApp = qobject_cast<QGuiApplication *>(qApp))
             cachedPixmap = static_cast<QGuiApplicationPrivate*>(QObjectPrivate::get(guiApp))->applyQIconStyleHelper(mode, cachedPixmap);
+        cachedPixmap.setDevicePixelRatio(calculatedDpr);
         QPixmapCache::insert(key, cachedPixmap);
     }
     return cachedPixmap;
 }
 
-QPixmap ScalableEntry::pixmap(const QSize &size, QIcon::Mode mode, QIcon::State state)
+QPixmap ScalableEntry::pixmap(const QSize &size, QIcon::Mode mode, QIcon::State state, qreal scale)
 {
     if (svgIcon.isNull())
         svgIcon = QIcon(filename);
 
-    // Bypass QIcon API, as that will scale by device pixel ratio of the
+    return svgIcon.pixmap(size, scale, mode, state);
-    // highest DPR screen since we're not passing on any QWindow.
-    if (QIconEngine *engine = svgIcon.data_ptr() ? svgIcon.data_ptr()->engine : nullptr)
-        return engine->pixmap(size, mode, state);
-
-    return QPixmap();
 }
 
 QPixmap QIconLoaderEngine::pixmap(const QSize &size, QIcon::Mode mode,
                                  QIcon::State state)
 {
-    QIconLoaderEngineEntry *entry = entryForSize(m_info, size);
+    return scaledPixmap(size, mode, state, 1.0);
-    if (entry)
-        return entry->pixmap(size, mode, state);
-
-    return QPixmap();
 }
 
 QString QIconLoaderEngine::key() const
@@ -975,8 +969,8 @@ bool QIconLoaderEngine::isNull()
 QPixmap QIconLoaderEngine::scaledPixmap(const QSize &size, QIcon::Mode mode, QIcon::State state, qreal scale)
 {
     const int integerScale = qCeil(scale);
-    QIconLoaderEngineEntry *entry = entryForSize(m_info, size / integerScale, integerScale);
+    QIconLoaderEngineEntry *entry = entryForSize(m_info, size, integerScale);
-    return entry ? entry->pixmap(size, mode, state) : QPixmap();
+    return entry ? entry->pixmap(size, mode, state, scale) : QPixmap();
 }
 
 QList<QSize> QIconLoaderEngine::availableSizes(QIcon::Mode mode, QIcon::State state)

src/gui/image/qiconloader_p.h

@@ -66,20 +66,21 @@ public:
     virtual ~QIconLoaderEngineEntry() {}
     virtual QPixmap pixmap(const QSize &size,
                            QIcon::Mode mode,
-                           QIcon::State state) = 0;
+                           QIcon::State state,
+                           qreal scale) = 0;
     QString filename;
     QIconDirInfo dir;
 };
 
 struct ScalableEntry : public QIconLoaderEngineEntry
 {
-    QPixmap pixmap(const QSize &size, QIcon::Mode mode, QIcon::State state) override;
+    QPixmap pixmap(const QSize &size, QIcon::Mode mode, QIcon::State state, qreal scale) override;
     QIcon svgIcon;
 };
 
 struct PixmapEntry : public QIconLoaderEngineEntry
 {
-    QPixmap pixmap(const QSize &size, QIcon::Mode mode, QIcon::State state) override;
+    QPixmap pixmap(const QSize &size, QIcon::Mode mode, QIcon::State state, qreal scale) override;
     QPixmap basePixmap;
 };