Fix unnecessary painting in CoverArtDelegate

src/library/coverartdelegate.cpp

@@ -5,6 +5,7 @@
 #include <algorithm>
 
 #include "library/coverartcache.h"
+#include "library/dao/trackschema.h"
 #include "library/trackmodel.h"
 #include "moc_coverartdelegate.cpp"
 #include "track/track.h"
@@ -28,7 +29,8 @@ CoverArtDelegate::CoverArtDelegate(QTableView* parent)
         : TableItemDelegate(parent),
           m_pTrackModel(asTrackModel(parent)),
           m_pCache(CoverArtCache::instance()),
-          m_inhibitLazyLoading(false) {
+          m_inhibitLazyLoading(false),
+          m_column(m_pTrackModel->fieldIndex(LIBRARYTABLE_COVERART)) {
     if (m_pCache) {
         connect(m_pCache,
                 &CoverArtCache::coverFound,
@@ -54,20 +56,44 @@ void CoverArtDelegate::emitRowsChanged(
     emit rowsChanged(std::move(rows));
 }
 
+void CoverArtDelegate::requestUncachedCover(
+        const CoverInfo& coverInfo,
+        int width,
+        int row) const {
+    if (coverInfo.imageDigest().isEmpty()) {
+        // This happens if we have the legacy hash
+        // The CoverArtCache will take care of the update
+        const auto pTrack = m_pTrackModel->getTrackByRef(
+                TrackRef::fromFilePath(coverInfo.trackLocation));
+        CoverArtCache::requestUncachedCover(this, pTrack, width);
+    } else {
+        // This is the fast path with an internal temporary track
+        CoverArtCache::requestUncachedCover(this, coverInfo, width);
+    }
+    m_pendingCacheRows.insert(coverInfo.cacheKey(), row);
+}
+
 void CoverArtDelegate::slotInhibitLazyLoading(
         bool inhibitLazyLoading) {
     m_inhibitLazyLoading = inhibitLazyLoading;
-    if (m_inhibitLazyLoading || m_cacheMissRows.isEmpty()) {
+    if (m_inhibitLazyLoading) {
+        return;
+    }
+    VERIFY_OR_DEBUG_ASSERT(m_pTrackModel) {
         return;
     }
-    // If we can request non-cache covers now, request updates
-    // for all rows that were cache misses since the last time.
-    // Reset the member variable before mutating the aggregated
-    // rows list (-> implicit sharing) and emitting a signal that
-    // in turn may trigger new signals for CoverArtDelegate!
-    QList<int> staleRows = std::move(m_cacheMissRows);
-    DEBUG_ASSERT(m_cacheMissRows.isEmpty());
-    emitRowsChanged(std::move(staleRows));
+    const double scaleFactor = m_pTableView->devicePixelRatioF();
+    const int width = static_cast<int>(m_pTableView->columnWidth(m_column) * scaleFactor);
+
+    for (int row : std::as_const(m_cacheMissRows)) {
+        QModelIndex index = m_pTableView->model()->index(row, m_column);
+        CoverInfo coverInfo = m_pTrackModel->getCoverInfo(index);
+        QRect rect = m_pTableView->visualRect(index);
+        if (rect.intersects(m_pTableView->rect())) {
+            requestUncachedCover(coverInfo, width, row);
+        }
+    }
+    m_cacheMissRows.clear();
 }
 
 void CoverArtDelegate::slotCoverFound(
@@ -90,58 +116,33 @@ void CoverArtDelegate::slotCoverFound(
     }
 }
 
-TrackPointer CoverArtDelegate::loadTrackByLocation(
-        const QString& trackLocation) const {
-    VERIFY_OR_DEBUG_ASSERT(m_pTrackModel) {
-        return TrackPointer();
-    }
-    return m_pTrackModel->getTrackByRef(
-            TrackRef::fromFilePath(trackLocation));
-}
-
 void CoverArtDelegate::paintItem(
         QPainter* painter,
         const QStyleOptionViewItem& option,
         const QModelIndex& index) const {
-    paintItemBackground(painter, option, index);
-
-    CoverInfo coverInfo = m_pTrackModel->getCoverInfo(index);
     VERIFY_OR_DEBUG_ASSERT(m_pTrackModel) {
         return;
     }
+    CoverInfo coverInfo = m_pTrackModel->getCoverInfo(index);
     bool drewPixmap = false;
     if (coverInfo.hasImage()) {
         VERIFY_OR_DEBUG_ASSERT(m_pCache) {
             return;
         }
-        const double scaleFactor = qobject_cast<QWidget*>(parent())->devicePixelRatioF();
-        QPixmap pixmap = CoverArtCache::getCachedCover(
-                coverInfo,
-                static_cast<int>(option.rect.width() * scaleFactor));
+        const double scaleFactor = m_pTableView->devicePixelRatioF();
+        const int width = static_cast<int>(option.rect.width() * scaleFactor);
+        QPixmap pixmap = CoverArtCache::getCachedCover(coverInfo, width);
         if (pixmap.isNull()) {
             // Cache miss
             if (m_inhibitLazyLoading) {
                 // We are requesting cache-only covers and got a cache
-                // miss. Record this row so that when we switch to requesting
-                // non-cache we can request an update.
-                m_cacheMissRows.append(index.row());
-            } else {
-                if (coverInfo.imageDigest().isEmpty()) {
-                    // This happens if we have the legacy hash
-                    // The CoverArtCache will take care of the update
-                    const auto pTrack = loadTrackByLocation(coverInfo.trackLocation);
-                    CoverArtCache::requestUncachedCover(
-                            this,
-                            pTrack,
-                            static_cast<int>(option.rect.width() * scaleFactor));
-                } else {
-                    // This is the fast path with an internal temporary track
-                    CoverArtCache::requestUncachedCover(
-                            this,
-                            coverInfo,
-                            static_cast<int>(option.rect.width() * scaleFactor));
+                // miss. Maintain them in a list for later lookup
+                if (!m_cacheMissRows.contains(index.row())) {
+                    cleanCacheMissRows();
+                    m_cacheMissRows.insert(index.row());
                 }
-                m_pendingCacheRows.insert(coverInfo.cacheKey(), index.row());
+            } else {
+                requestUncachedCover(coverInfo, width, index.row());
             }
         } else {
             // Cache hit
@@ -157,12 +158,31 @@ void CoverArtDelegate::paintItem(
     // Since the background color is calculated from the cover art image
     // it is optional and may not always be available. The background
     // color may even be set manually without having a cover image.
-    if (!drewPixmap && coverInfo.color) {
-        painter->fillRect(option.rect, mixxx::RgbColor::toQColor(coverInfo.color));
+    if (!drewPixmap) {
+        if (coverInfo.color) {
+            painter->fillRect(option.rect, mixxx::RgbColor::toQColor(coverInfo.color));
+        } else {
+            paintItemBackground(painter, option, index);
+        }
     }
 
     // Draw a border if the cover art cell has focus
     if (option.state & QStyle::State_HasFocus) {
         drawBorder(painter, m_pFocusBorderColor, option.rect);
     }
 }
+
+void CoverArtDelegate::cleanCacheMissRows() const {
+    auto it = m_cacheMissRows.begin();
+    while (it != m_cacheMissRows.end()) {
+        QModelIndex index = m_pTableView->model()->index(*it, m_column);
+        QRect rect = m_pTableView->visualRect(index);
+        if (!rect.intersects(m_pTableView->rect())) {
+            // Cover image row is no longer shown. We keep the set
+            // small which likely reuses the allocatd memory later
+            it = m_cacheMissRows.erase(it);
+        } else {
+            ++it;
+        }
+    }
+}

src/library/coverartdelegate.h

@@ -55,15 +55,18 @@ class CoverArtDelegate : public TableItemDelegate {
   private:
     void emitRowsChanged(
             QList<int>&& rows);
-
-    TrackPointer loadTrackByLocation(
-            const QString& trackLocation) const;
+    void cleanCacheMissRows() const;
+    void requestUncachedCover(
+            const CoverInfo& coverInfo,
+            int width,
+            int row) const;
 
     CoverArtCache* const m_pCache;
     bool m_inhibitLazyLoading;
+    int m_column;
 
     // We need to record rows in paint() (which is const) so
     // these are marked mutable.
-    mutable QList<int> m_cacheMissRows;
+    mutable QSet<int> m_cacheMissRows;
     mutable QMultiHash<mixxx::cache_key_t, int> m_pendingCacheRows;
 };