From 3982086d1c427c8caee6b7b965ed629f256af661 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 13 May 2021 18:58:42 +0200 Subject: [PATCH 1/2] Track: Return a copy of TrackMetadata/TrackRecord TrackMetadata and TrackRecord are both movable and can be returned efficiently using copy elision. https://en.cppreference.com/w/cpp/language/copy_elision --- src/library/dao/trackdao.cpp | 15 +++++------ src/library/dlgtagfetcher.cpp | 6 ++--- src/sources/soundsourceproxy.cpp | 6 ++--- src/test/trackupdate_test.cpp | 45 +++++++++++++------------------- src/track/track.cpp | 16 +++++------- src/track/track.h | 7 +++-- 6 files changed, 38 insertions(+), 57 deletions(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index ce68d034aab..3d934e9c3bb 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -745,14 +745,11 @@ TrackId TrackDAO::addTracksAddTrack(const TrackPointer& pTrack, bool unremove) { } // Time stamps are stored with timezone UTC in the database - mixxx::TrackRecord trackRecord; - pTrack->readTrackRecord(&trackRecord); - const mixxx::BeatsPointer pBeats = pTrack->getBeats(); const auto trackDateAdded = QDateTime::currentDateTimeUtc(); if (!insertTrackLibrary( m_pQueryLibraryInsert.get(), - trackRecord, - pBeats, + pTrack->getRecord(), + pTrack->getBeats(), trackLocationId, fileAccess.info(), trackDateAdded)) { @@ -1619,10 +1616,10 @@ bool TrackDAO::updateTrack(Track* pTrack) const { query.bindValue(":track_id", trackId.toVariant()); - mixxx::TrackRecord trackRecord; - pTrack->readTrackRecord(&trackRecord); - const mixxx::BeatsPointer pBeats = pTrack->getBeats(); - bindTrackLibraryValues(&query, trackRecord, pBeats); + bindTrackLibraryValues( + &query, + pTrack->getRecord(), + pTrack->getBeats()); VERIFY_OR_DEBUG_ASSERT(query.exec()) { LOG_FAILED_QUERY(query); diff --git a/src/library/dlgtagfetcher.cpp b/src/library/dlgtagfetcher.cpp index 8947763ebb0..34b220906a9 100644 --- a/src/library/dlgtagfetcher.cpp +++ b/src/library/dlgtagfetcher.cpp @@ -11,8 +11,7 @@ namespace { QStringList trackColumnValues( const Track& track) { - mixxx::TrackMetadata trackMetadata; - track.readTrackMetadata(&trackMetadata); + const mixxx::TrackMetadata trackMetadata = track.getMetadata(); const QString trackNumberAndTotal = TrackNumbers::joinAsString( trackMetadata.getTrackInfo().getTrackNumber(), trackMetadata.getTrackInfo().getTrackTotal()); @@ -166,8 +165,7 @@ void DlgTagFetcher::apply() { DEBUG_ASSERT(resultIndex < m_data.m_results.size()); const mixxx::musicbrainz::TrackRelease& trackRelease = m_data.m_results[resultIndex]; - mixxx::TrackMetadata trackMetadata; - m_track->readTrackMetadata(&trackMetadata); + mixxx::TrackMetadata trackMetadata = m_track->getMetadata(); if (!trackRelease.artist.isEmpty()) { trackMetadata.refTrackInfo().setArtist( trackRelease.artist); diff --git a/src/sources/soundsourceproxy.cpp b/src/sources/soundsourceproxy.cpp index ca7d8e630c0..e34760716d2 100644 --- a/src/sources/soundsourceproxy.cpp +++ b/src/sources/soundsourceproxy.cpp @@ -537,9 +537,9 @@ bool SoundSourceProxy::updateTrackFromSource( // values if the corresponding file tags are missing. Depending // on the file type some kind of tags might even not be supported // at all and this information would get lost entirely otherwise! - mixxx::TrackMetadata trackMetadata; bool metadataSynchronized = false; - m_pTrack->readTrackMetadata(&trackMetadata, &metadataSynchronized); + mixxx::TrackMetadata trackMetadata = + m_pTrack->getMetadata(&metadataSynchronized); // If the file tags have already been parsed at least once, the // existing track metadata should not be updated implicitly, i.e. // if the user did not explicitly choose to (re-)import metadata @@ -587,7 +587,7 @@ bool SoundSourceProxy::updateTrackFromSource( << "from file" << getUrl().toString(); // make sure that the trackMetadata was not messed up due to the failure - m_pTrack->readTrackMetadata(&trackMetadata, &metadataSynchronized); + trackMetadata = m_pTrack->getMetadata(&metadataSynchronized); } // Partial import diff --git a/src/test/trackupdate_test.cpp b/src/test/trackupdate_test.cpp index 529709aade6..60ce68f70bf 100644 --- a/src/test/trackupdate_test.cpp +++ b/src/test/trackupdate_test.cpp @@ -1,10 +1,9 @@ #include -#include "test/mixxxtest.h" - -#include "track/track.h" #include "library/coverart.h" #include "sources/soundsourceproxy.h" +#include "test/mixxxtest.h" +#include "track/track.h" namespace { @@ -55,17 +54,15 @@ TEST_F(TrackUpdateTest, parseModifiedCleanOnce) { auto pTrack = newTestTrackParsedModified(); pTrack->markClean(); - mixxx::TrackMetadata trackMetadataBefore; - pTrack->readTrackMetadata(&trackMetadataBefore); - auto coverInfoBefore = pTrack->getCoverInfo(); + const auto trackMetadataBefore = pTrack->getMetadata(); + const auto coverInfoBefore = pTrack->getCoverInfo(); // Re-update from source should have no effect ASSERT_FALSE(SoundSourceProxy(pTrack).updateTrackFromSource( SoundSourceProxy::UpdateTrackFromSourceMode::Once)); - mixxx::TrackMetadata trackMetadataAfter; - pTrack->readTrackMetadata(&trackMetadataAfter); - auto coverInfoAfter = pTrack->getCoverInfo(); + const auto trackMetadataAfter = pTrack->getMetadata(); + const auto coverInfoAfter = pTrack->getCoverInfo(); // Verify that the track has not been modified ASSERT_TRUE(pTrack->isMetadataSynchronized()); @@ -78,16 +75,14 @@ TEST_F(TrackUpdateTest, parseModifiedCleanAgainSkipCover) { auto pTrack = newTestTrackParsedModified(); pTrack->markClean(); - mixxx::TrackMetadata trackMetadataBefore; - pTrack->readTrackMetadata(&trackMetadataBefore); - auto coverInfoBefore = pTrack->getCoverInfo(); + const auto trackMetadataBefore = pTrack->getMetadata(); + const auto coverInfoBefore = pTrack->getCoverInfo(); EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource( SoundSourceProxy::UpdateTrackFromSourceMode::Again)); - mixxx::TrackMetadata trackMetadataAfter; - pTrack->readTrackMetadata(&trackMetadataAfter); - auto coverInfoAfter = pTrack->getCoverInfo(); + const auto trackMetadataAfter = pTrack->getMetadata(); + const auto coverInfoAfter = pTrack->getCoverInfo(); // Updated EXPECT_TRUE(pTrack->isMetadataSynchronized()); @@ -104,16 +99,14 @@ TEST_F(TrackUpdateTest, parseModifiedCleanAgainUpdateCover) { pTrack->setCoverInfo(coverInfo); pTrack->markClean(); - mixxx::TrackMetadata trackMetadataBefore; - pTrack->readTrackMetadata(&trackMetadataBefore); - auto coverInfoBefore = pTrack->getCoverInfo(); + const auto trackMetadataBefore = pTrack->getMetadata(); + const auto coverInfoBefore = pTrack->getCoverInfo(); EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource( SoundSourceProxy::UpdateTrackFromSourceMode::Again)); - mixxx::TrackMetadata trackMetadataAfter; - pTrack->readTrackMetadata(&trackMetadataAfter); - auto coverInfoAfter = pTrack->getCoverInfo(); + const auto trackMetadataAfter = pTrack->getMetadata(); + const auto coverInfoAfter = pTrack->getCoverInfo(); // Updated EXPECT_TRUE(pTrack->isMetadataSynchronized()); @@ -125,16 +118,14 @@ TEST_F(TrackUpdateTest, parseModifiedCleanAgainUpdateCover) { TEST_F(TrackUpdateTest, parseModifiedDirtyAgain) { auto pTrack = newTestTrackParsedModified(); - mixxx::TrackMetadata trackMetadataBefore; - pTrack->readTrackMetadata(&trackMetadataBefore); - auto coverInfoBefore = pTrack->getCoverInfo(); + const auto trackMetadataBefore = pTrack->getMetadata(); + const auto coverInfoBefore = pTrack->getCoverInfo(); EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource( SoundSourceProxy::UpdateTrackFromSourceMode::Again)); - mixxx::TrackMetadata trackMetadataAfter; - pTrack->readTrackMetadata(&trackMetadataAfter); - auto coverInfoAfter = pTrack->getCoverInfo(); + const auto trackMetadataAfter = pTrack->getMetadata(); + const auto coverInfoAfter = pTrack->getCoverInfo(); // Updated EXPECT_TRUE(pTrack->isMetadataSynchronized()); diff --git a/src/track/track.cpp b/src/track/track.cpp index aa0104005aa..421c8be731a 100644 --- a/src/track/track.cpp +++ b/src/track/track.cpp @@ -239,26 +239,22 @@ bool Track::mergeImportedMetadata( return true; } -void Track::readTrackMetadata( - mixxx::TrackMetadata* pTrackMetadata, +mixxx::TrackMetadata Track::getMetadata( bool* pMetadataSynchronized) const { - DEBUG_ASSERT(pTrackMetadata); - QMutexLocker lock(&m_qMutex); - *pTrackMetadata = m_record.getMetadata(); + const QMutexLocker locked(&m_qMutex); if (pMetadataSynchronized) { *pMetadataSynchronized = m_record.getMetadataSynchronized(); } + return m_record.getMetadata(); } -void Track::readTrackRecord( - mixxx::TrackRecord* pTrackRecord, +mixxx::TrackRecord Track::getRecord( bool* pDirty) const { - DEBUG_ASSERT(pTrackRecord); - QMutexLocker lock(&m_qMutex); - *pTrackRecord = m_record; + const QMutexLocker locked(&m_qMutex); if (pDirty) { *pDirty = m_bDirty; } + return m_record; } mixxx::ReplayGain Track::getReplayGain() const { diff --git a/src/track/track.h b/src/track/track.h index 982cfe7bd51..8bf499a5f22 100644 --- a/src/track/track.h +++ b/src/track/track.h @@ -338,11 +338,10 @@ class Track : public QObject { bool mergeImportedMetadata( const mixxx::TrackMetadata& importedMetadata); - void readTrackMetadata( - mixxx::TrackMetadata* pTrackMetadata, + mixxx::TrackMetadata getMetadata( bool* pMetadataSynchronized = nullptr) const; - void readTrackRecord( - mixxx::TrackRecord* pTrackRecord, + + mixxx::TrackRecord getRecord( bool* pDirty = nullptr) const; // Mark the track dirty if it isn't already. From 03a929729d67c625d19ce81d550401a977d18bda Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 14 May 2021 17:45:11 +0200 Subject: [PATCH 2/2] Log imported track metadata for debugging --- src/library/dao/trackdao.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 3d934e9c3bb..747725ff068 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1472,6 +1472,12 @@ TrackPointer TrackDAO::getTrackById(TrackId trackId) const { // before, so just check and try for every track that has been // freshly loaded from the database. SoundSourceProxy(pTrack).updateTrackFromSource(); + if (kLogger.debugEnabled() && pTrack->isDirty()) { + kLogger.debug() + << "Updated track metadata from file tags:" + << pTrack->getFileInfo().lastModified() + << pTrack->getMetadata(); + } } // Validate and refresh cover image hash values if needed.