From a15c47784716ee10df12e64bec9f0d3bf5fafb63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Tue, 1 Oct 2024 21:36:11 +0200 Subject: [PATCH 1/8] Move paintItemBackground to remove unnecessary painting. --- src/library/coverartdelegate.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/library/coverartdelegate.cpp b/src/library/coverartdelegate.cpp index e4349df01e9..9fce9a37718 100644 --- a/src/library/coverartdelegate.cpp +++ b/src/library/coverartdelegate.cpp @@ -103,8 +103,6 @@ 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; @@ -157,8 +155,12 @@ 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 From c36e5661d8ddfea072f3d190a26db6383ddfca01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Mon, 30 Sep 2024 23:15:59 +0200 Subject: [PATCH 2/8] Keep m_cacheMissRows small with only visible tracks. --- src/library/coverartdelegate.cpp | 36 +++++++++++++++++++++++++------- src/library/coverartdelegate.h | 5 +++-- 2 files changed, 31 insertions(+), 10 deletions(-) diff --git a/src/library/coverartdelegate.cpp b/src/library/coverartdelegate.cpp index 9fce9a37718..e5ea9b5a19b 100644 --- a/src/library/coverartdelegate.cpp +++ b/src/library/coverartdelegate.cpp @@ -5,6 +5,7 @@ #include #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, @@ -57,7 +59,11 @@ void CoverArtDelegate::emitRowsChanged( void CoverArtDelegate::slotInhibitLazyLoading( bool inhibitLazyLoading) { m_inhibitLazyLoading = inhibitLazyLoading; - if (m_inhibitLazyLoading || m_cacheMissRows.isEmpty()) { + if (m_inhibitLazyLoading) { + return; + } + cleanCacheMissRows(); + if (m_cacheMissRows.isEmpty()) { return; } // If we can request non-cache covers now, request updates @@ -65,9 +71,8 @@ void CoverArtDelegate::slotInhibitLazyLoading( // 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 staleRows = std::move(m_cacheMissRows); - DEBUG_ASSERT(m_cacheMissRows.isEmpty()); - emitRowsChanged(std::move(staleRows)); + emitRowsChanged(std::move(m_cacheMissRows.toList())); + m_cacheMissRows.clear(); } void CoverArtDelegate::slotCoverFound( @@ -120,9 +125,11 @@ void CoverArtDelegate::paintItem( // 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()); + // miss. Maintain them in a list for later lookup + if (!m_cacheMissRows.contains(index.row())) { + cleanCacheMissRows(); + m_cacheMissRows.insert(index.row()); + } } else { if (coverInfo.imageDigest().isEmpty()) { // This happens if we have the legacy hash @@ -168,3 +175,16 @@ void CoverArtDelegate::paintItem( 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())) { + it = m_cacheMissRows.erase(it); + } else { + ++it; + } + } +} diff --git a/src/library/coverartdelegate.h b/src/library/coverartdelegate.h index 84c69c5604a..8dc8b980502 100644 --- a/src/library/coverartdelegate.h +++ b/src/library/coverartdelegate.h @@ -55,15 +55,16 @@ class CoverArtDelegate : public TableItemDelegate { private: void emitRowsChanged( QList&& rows); - TrackPointer loadTrackByLocation( const QString& trackLocation) const; + void cleanCacheMissRows() 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 m_cacheMissRows; + mutable QSet m_cacheMissRows; mutable QMultiHash m_pendingCacheRows; }; From 6c841bd4b2320f6dd4ebf0599c4f3d6697caf9e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Tue, 1 Oct 2024 23:24:06 +0200 Subject: [PATCH 3/8] Keep m_cacheMissRows small with only visible tracks. --- src/library/coverartdelegate.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/library/coverartdelegate.cpp b/src/library/coverartdelegate.cpp index e5ea9b5a19b..41dd0db6cb2 100644 --- a/src/library/coverartdelegate.cpp +++ b/src/library/coverartdelegate.cpp @@ -182,6 +182,8 @@ void CoverArtDelegate::cleanCacheMissRows() const { 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; From 77847a275d4d149663643814b56d6b064e1ee5aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Tue, 1 Oct 2024 21:42:17 +0200 Subject: [PATCH 4/8] Improve use of m_pTrackModel --- src/library/coverartdelegate.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/library/coverartdelegate.cpp b/src/library/coverartdelegate.cpp index 41dd0db6cb2..a8cd3b6e0a1 100644 --- a/src/library/coverartdelegate.cpp +++ b/src/library/coverartdelegate.cpp @@ -108,16 +108,16 @@ void CoverArtDelegate::paintItem( QPainter* painter, const QStyleOptionViewItem& option, const QModelIndex& index) const { - 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(parent())->devicePixelRatioF(); + const double scaleFactor = m_pTableView->devicePixelRatioF(); QPixmap pixmap = CoverArtCache::getCachedCover( coverInfo, static_cast(option.rect.width() * scaleFactor)); From c7a10eb0774573215bf82a8d4acebac247c22925 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Tue, 1 Oct 2024 23:02:11 +0200 Subject: [PATCH 5/8] Avoid extra painting, after m_inhibitLazyLoading is set false. Now request the missing covers right away. --- src/library/coverartdelegate.cpp | 68 +++++++++++++++----------------- src/library/coverartdelegate.h | 6 ++- 2 files changed, 36 insertions(+), 38 deletions(-) diff --git a/src/library/coverartdelegate.cpp b/src/library/coverartdelegate.cpp index a8cd3b6e0a1..be76fcc9153 100644 --- a/src/library/coverartdelegate.cpp +++ b/src/library/coverartdelegate.cpp @@ -56,22 +56,43 @@ 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) { return; } - cleanCacheMissRows(); - if (m_cacheMissRows.isEmpty()) { + 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! - emitRowsChanged(std::move(m_cacheMissRows.toList())); + const double scaleFactor = m_pTableView->devicePixelRatioF(); + const int width = static_cast(m_pTableView->columnWidth(m_column) * scaleFactor); + + for (int row : 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(); } @@ -95,15 +116,6 @@ 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, @@ -118,9 +130,8 @@ void CoverArtDelegate::paintItem( return; } const double scaleFactor = m_pTableView->devicePixelRatioF(); - QPixmap pixmap = CoverArtCache::getCachedCover( - coverInfo, - static_cast(option.rect.width() * scaleFactor)); + const int width = static_cast(option.rect.width() * scaleFactor); + QPixmap pixmap = CoverArtCache::getCachedCover(coverInfo, width); if (pixmap.isNull()) { // Cache miss if (m_inhibitLazyLoading) { @@ -131,22 +142,7 @@ void CoverArtDelegate::paintItem( m_cacheMissRows.insert(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(option.rect.width() * scaleFactor)); - } else { - // This is the fast path with an internal temporary track - CoverArtCache::requestUncachedCover( - this, - coverInfo, - static_cast(option.rect.width() * scaleFactor)); - } - m_pendingCacheRows.insert(coverInfo.cacheKey(), index.row()); + requestUncachedCover(coverInfo, width, index.row()); } } else { // Cache hit diff --git a/src/library/coverartdelegate.h b/src/library/coverartdelegate.h index 8dc8b980502..a7022378cd1 100644 --- a/src/library/coverartdelegate.h +++ b/src/library/coverartdelegate.h @@ -55,9 +55,11 @@ class CoverArtDelegate : public TableItemDelegate { private: void emitRowsChanged( QList&& 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; From 18d556d82b2450746df7a313b96d76a90a288d61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Wed, 2 Oct 2024 10:50:31 +0200 Subject: [PATCH 6/8] Use std::as_const do avoid detached loop --- src/library/coverartdelegate.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/library/coverartdelegate.cpp b/src/library/coverartdelegate.cpp index be76fcc9153..e107328fe51 100644 --- a/src/library/coverartdelegate.cpp +++ b/src/library/coverartdelegate.cpp @@ -85,7 +85,7 @@ void CoverArtDelegate::slotInhibitLazyLoading( const double scaleFactor = m_pTableView->devicePixelRatioF(); const int width = static_cast(m_pTableView->columnWidth(m_column) * scaleFactor); - for (int row : m_cacheMissRows) { + 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); From 070dbb0cc7a9d26f8eea78d28e248197315f3efd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sat, 5 Oct 2024 00:00:55 +0200 Subject: [PATCH 7/8] Don't look up cover infos for not visible covers Co-authored-by: ronso0 --- src/library/coverartdelegate.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/library/coverartdelegate.cpp b/src/library/coverartdelegate.cpp index e107328fe51..475c84618dd 100644 --- a/src/library/coverartdelegate.cpp +++ b/src/library/coverartdelegate.cpp @@ -76,7 +76,7 @@ void CoverArtDelegate::requestUncachedCover( void CoverArtDelegate::slotInhibitLazyLoading( bool inhibitLazyLoading) { m_inhibitLazyLoading = inhibitLazyLoading; - if (m_inhibitLazyLoading) { + if (m_inhibitLazyLoading || m_cacheMissRows.isEmpty()) { return; } VERIFY_OR_DEBUG_ASSERT(m_pTrackModel) { @@ -86,10 +86,10 @@ void CoverArtDelegate::slotInhibitLazyLoading( const int width = static_cast(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); + const QModelIndex index = m_pTableView->model()->index(row, m_column); + const QRect rect = m_pTableView->visualRect(index); if (rect.intersects(m_pTableView->rect())) { + const CoverInfo coverInfo = m_pTrackModel->getCoverInfo(index); requestUncachedCover(coverInfo, width, row); } } From ce671a358655cb7d2a41c2e44608040a3144684b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sun, 6 Oct 2024 08:02:54 +0200 Subject: [PATCH 8/8] improve constant correctness Co-authored-by: ronso0 --- src/library/coverartdelegate.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/library/coverartdelegate.cpp b/src/library/coverartdelegate.cpp index 475c84618dd..21d386168dc 100644 --- a/src/library/coverartdelegate.cpp +++ b/src/library/coverartdelegate.cpp @@ -175,8 +175,8 @@ void CoverArtDelegate::paintItem( 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); + const QModelIndex index = m_pTableView->model()->index(*it, m_column); + const 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