Merge pull request #2756 from uklotzde/trackdao

TrackDAO: Reformat code / Avoid mutex locks on Track object

2 files changed, 182 insertions, 138 deletions

diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp
index 66c41e9cfd..5b85406c2b 100644
--- a/src/library/dao/trackdao.cpp
+++ b/src/library/dao/trackdao.cpp
@@ -81,6 +81,12 @@ TrackDAO::~TrackDAO() {
     addTracksFinish(true);
 }
 
+void TrackDAO::initialize(
+        const QSqlDatabase& database) {
+    DEBUG_ASSERT(!m_database.isOpen());
+    m_database = database;
+}
+
 void TrackDAO::finish() {
     qDebug() << "TrackDAO::finish()";
 
@@ -399,155 +405,179 @@ void TrackDAO::addTracksFinish(bool rollback) {
 }
 
 namespace {
-    bool insertTrackLocation(QSqlQuery* pTrackLocationInsert, const Track& track) {
-        DEBUG_ASSERT(nullptr != pTrackLocationInsert);
-        const auto trackFile = track.getFileInfo();
-        pTrackLocationInsert->bindValue(":location", trackFile.location());
-        pTrackLocationInsert->bindValue(":directory", trackFile.directory());
-        pTrackLocationInsert->bindValue(":filename", trackFile.fileName());
-        pTrackLocationInsert->bindValue(":filesize", trackFile.fileSize());
-        pTrackLocationInsert->bindValue(":fs_deleted", 0);
-        pTrackLocationInsert->bindValue(":needs_verification", 0);
-        if (pTrackLocationInsert->exec()) {
-            return true;
-        } else {
-            LOG_FAILED_QUERY(*pTrackLocationInsert)
+
+bool insertTrackLocation(
+        QSqlQuery* pTrackLocationInsert,
+        const TrackFile& trackFile) {
+    DEBUG_ASSERT(pTrackLocationInsert);
+    pTrackLocationInsert->bindValue(":location", trackFile.location());
+    pTrackLocationInsert->bindValue(":directory", trackFile.directory());
+    pTrackLocationInsert->bindValue(":filename", trackFile.fileName());
+    pTrackLocationInsert->bindValue(":filesize", trackFile.fileSize());
+    pTrackLocationInsert->bindValue(":fs_deleted", 0);
+    pTrackLocationInsert->bindValue(":needs_verification", 0);
+    if (pTrackLocationInsert->exec()) {
+        return true;
+    } else {
+        LOG_FAILED_QUERY(*pTrackLocationInsert)
                 << "Skip inserting duplicate track location" << trackFile.location();
-            return false;
-        }
+        return false;
     }
+}
 
-    // Bind common values for insert/update
-    void bindTrackLibraryValues(QSqlQuery* pTrackLibraryQuery, const Track& track) {
-        pTrackLibraryQuery->bindValue(":artist", track.getArtist());
-        pTrackLibraryQuery->bindValue(":title", track.getTitle());
-        pTrackLibraryQuery->bindValue(":album", track.getAlbum());
-        pTrackLibraryQuery->bindValue(":album_artist", track.getAlbumArtist());
-        pTrackLibraryQuery->bindValue(":year", track.getYear());
-        pTrackLibraryQuery->bindValue(":genre", track.getGenre());
-        pTrackLibraryQuery->bindValue(":composer", track.getComposer());
-        pTrackLibraryQuery->bindValue(":grouping", track.getGrouping());
-        pTrackLibraryQuery->bindValue(":tracknumber", track.getTrackNumber());
-        pTrackLibraryQuery->bindValue(":tracktotal", track.getTrackTotal());
-        pTrackLibraryQuery->bindValue(":filetype", track.getType());
-        pTrackLibraryQuery->bindValue(":color", mixxx::RgbColor::toQVariant(track.getColor()));
-        pTrackLibraryQuery->bindValue(":comment", track.getComment());
-        pTrackLibraryQuery->bindValue(":url", track.getURL());
-        pTrackLibraryQuery->bindValue(":rating", track.getRating());
-        pTrackLibraryQuery->bindValue(":cuepoint", track.getCuePoint().getPosition());
-        pTrackLibraryQuery->bindValue(":bpm_lock", track.isBpmLocked()? 1 : 0);
-        pTrackLibraryQuery->bindValue(":replaygain", track.getReplayGain().getRatio());
-        pTrackLibraryQuery->bindValue(":replaygain_peak", track.getReplayGain().getPeak());
-
-        pTrackLibraryQuery->bindValue(":channels", track.getChannels());
-        pTrackLibraryQuery->bindValue(":samplerate", track.getSampleRate());
-        pTrackLibraryQuery->bindValue(":bitrate", track.getBitrate());
-        pTrackLibraryQuery->bindValue(":duration", track.getDuration());
-
-        pTrackLibraryQuery->bindValue(":header_parsed", track.isMetadataSynchronized() ? 1 : 0);
-
-        const PlayCounter playCounter(track.getPlayCounter());
-        pTrackLibraryQuery->bindValue(":timesplayed", playCounter.getTimesPlayed());
-        pTrackLibraryQuery->bindValue(":played", playCounter.isPlayed() ? 1 : 0);
-
-        const CoverInfoRelative coverInfo(track.getCoverInfo());
-        pTrackLibraryQuery->bindValue(":coverart_source", coverInfo.source);
-        pTrackLibraryQuery->bindValue(":coverart_type", coverInfo.type);
-        pTrackLibraryQuery->bindValue(":coverart_location", coverInfo.coverLocation);
-        pTrackLibraryQuery->bindValue(":coverart_hash", coverInfo.hash);
-
-        QByteArray beatsBlob;
-        QString beatsVersion;
-        QString beatsSubVersion;
-        // Fall back on cached BPM
-        double dBpm = track.getBpm();
-        const mixxx::BeatsPointer pBeats(track.getBeats());
-        if (!pBeats.isNull()) {
-            beatsBlob = pBeats->toByteArray();
-            beatsVersion = pBeats->getVersion();
-            beatsSubVersion = pBeats->getSubVersion();
-            dBpm = pBeats->getBpm();
-        }
-        pTrackLibraryQuery->bindValue(":bpm", dBpm);
-        pTrackLibraryQuery->bindValue(":beats_version", beatsVersion);
-        pTrackLibraryQuery->bindValue(":beats_sub_version", beatsSubVersion);
-        pTrackLibraryQuery->bindValue(":beats", beatsBlob);
-
-        QByteArray keysBlob;
-        QString keysVersion;
-        QString keysSubVersion;
-        QString keyText;
-        mixxx::track::io::key::ChromaticKey key = mixxx::track::io::key::INVALID;
-        const Keys keys(track.getKeys());
-        if (keys.isValid()) {
-            keysBlob = keys.toByteArray();
-            keysVersion = keys.getVersion();
-            keysSubVersion = keys.getSubVersion();
-            key = keys.getGlobalKey();
-            keyText = KeyUtils::getGlobalKeyText(keys);
-        }
-        pTrackLibraryQuery->bindValue(":keys", keysBlob);
-        pTrackLibraryQuery->bindValue(":keys_version", keysVersion);
-        pTrackLibraryQuery->bindValue(":keys_sub_version", keysSubVersion);
-        pTrackLibraryQuery->bindValue(":key", keyText);
-        pTrackLibraryQuery->bindValue(":key_id", static_cast<int>(key));
-    }
+// Bind common values for insert/update
+void bindTrackLibraryValues(
+        QSqlQuery* pTrackLibraryQuery,
+        const mixxx::TrackRecord& track,
+        const mixxx::BeatsPointer& pBeats) {
+    const mixxx::TrackMetadata& trackMetadata = track.getMetadata();
+    const mixxx::TrackInfo& trackInfo = trackMetadata.getTrackInfo();
+    const mixxx::AlbumInfo& albumInfo = trackMetadata.getAlbumInfo();
+
+    pTrackLibraryQuery->bindValue(":artist", trackInfo.getArtist());
+    pTrackLibraryQuery->bindValue(":title", trackInfo.getTitle());
+    pTrackLibraryQuery->bindValue(":album", albumInfo.getTitle());
+    pTrackLibraryQuery->bindValue(":album_artist", albumInfo.getArtist());
+    pTrackLibraryQuery->bindValue(":year", trackInfo.getYear());
+    pTrackLibraryQuery->bindValue(":genre", trackInfo.getGenre());
+    pTrackLibraryQuery->bindValue(":composer", trackInfo.getComposer());
+    pTrackLibraryQuery->bindValue(":grouping", trackInfo.getGrouping());
+    pTrackLibraryQuery->bindValue(":tracknumber", trackInfo.getTrackNumber());
+    pTrackLibraryQuery->bindValue(":tracktotal", trackInfo.getTrackTotal());
+    pTrackLibraryQuery->bindValue(":filetype", track.getFileType());
+    pTrackLibraryQuery->bindValue(":color", mixxx::RgbColor::toQVariant(track.getColor()));
+    pTrackLibraryQuery->bindValue(":comment", trackInfo.getComment());
+    pTrackLibraryQuery->bindValue(":url", track.getUrl());
+    pTrackLibraryQuery->bindValue(":rating", track.getRating());
+    pTrackLibraryQuery->bindValue(":cuepoint", track.getCuePoint().getPosition());
+    pTrackLibraryQuery->bindValue(":bpm_lock", track.getBpmLocked() ? 1 : 0);
+    pTrackLibraryQuery->bindValue(":replaygain", trackInfo.getReplayGain().getRatio());
+    pTrackLibraryQuery->bindValue(":replaygain_peak", trackInfo.getReplayGain().getPeak());
+
+    pTrackLibraryQuery->bindValue(":channels",
+            static_cast<uint>(trackMetadata.getChannelCount()));
+    pTrackLibraryQuery->bindValue(":samplerate",
+            static_cast<uint>(trackMetadata.getSampleRate()));
+    pTrackLibraryQuery->bindValue(":bitrate",
+            static_cast<uint>(trackMetadata.getBitrate()));
+    pTrackLibraryQuery->bindValue(":duration",
+            trackMetadata.getDuration().toDoubleSeconds());
+
+    pTrackLibraryQuery->bindValue(":header_parsed",
+            track.getMetadataSynchronized() ? 1 : 0);
+
+    const PlayCounter& playCounter = track.getPlayCounter();
+    pTrackLibraryQuery->bindValue(":timesplayed", playCounter.getTimesPlayed());
+    pTrackLibraryQuery->bindValue(":played", playCounter.isPlayed() ? 1 : 0);
+
+    const CoverInfoRelative& coverInfo = track.getCoverInfo();
+    pTrackLibraryQuery->bindValue(":coverart_source", coverInfo.source);
+    pTrackLibraryQuery->bindValue(":coverart_type", coverInfo.type);
+    pTrackLibraryQuery->bindValue(":coverart_location", coverInfo.coverLocation);
+    pTrackLibraryQuery->bindValue(":coverart_hash", coverInfo.hash);
+
+    QByteArray beatsBlob;
+    QString beatsVersion;
+    QString beatsSubVersion;
+    // Fall back on cached BPM
+    double dBpm = trackInfo.getBpm().getValue();
+    if (!pBeats.isNull()) {
+        beatsBlob = pBeats->toByteArray();
+        beatsVersion = pBeats->getVersion();
+        beatsSubVersion = pBeats->getSubVersion();
+        dBpm = pBeats->getBpm();
+    }
+    pTrackLibraryQuery->bindValue(":bpm", dBpm);
+    pTrackLibraryQuery->bindValue(":beats_version", beatsVersion);
+    pTrackLibraryQuery->bindValue(":beats_sub_version", beatsSubVersion);
+    pTrackLibraryQuery->bindValue(":beats", beatsBlob);
+
+    QByteArray keysBlob;
+    QString keysVersion;
+    QString keysSubVersion;
+    QString keyText;
+    mixxx::track::io::key::ChromaticKey globalKey = mixxx::track::io::key::INVALID;
+    const Keys& keys = track.getKeys();
+    if (keys.isValid()) {
+        keysBlob = keys.toByteArray();
+        keysVersion = keys.getVersion();
+        keysSubVersion = keys.getSubVersion();
+        globalKey = keys.getGlobalKey();
+        keyText = KeyUtils::getGlobalKeyText(keys);
+    }
+    pTrackLibraryQuery->bindValue(":keys", keysBlob);
+    pTrackLibraryQuery->bindValue(":keys_version", keysVersion);
+    pTrackLibraryQuery->bindValue(":keys_sub_version", keysSubVersion);
+    pTrackLibraryQuery->bindValue(":key_id", static_cast<int>(globalKey));
+    pTrackLibraryQuery->bindValue(":key", keyText);
+}
 
-    bool insertTrackLibrary(QSqlQuery* pTrackLibraryInsert, const Track& track, DbId trackLocationId, QDateTime trackDateAdded) {
-        bindTrackLibraryValues(pTrackLibraryInsert, track);
+bool insertTrackLibrary(
+        QSqlQuery* pTrackLibraryInsert,
+        const mixxx::TrackRecord& trackRecord,
+        const mixxx::BeatsPointer& pBeats,
+        DbId trackLocationId,
+        const TrackFile& trackFile,
+        QDateTime trackDateAdded) {
+    bindTrackLibraryValues(pTrackLibraryInsert, trackRecord, pBeats);
 
-        if (!track.getDateAdded().isNull()) {
-            qDebug() << "insertTrackLibrary: Track was added" << "track.getDateAdded()" << "and purged";
-        }
-        pTrackLibraryInsert->bindValue(":datetime_added", trackDateAdded);
+    if (!trackRecord.getDateAdded().isNull()) {
+        qDebug() << "insertTrackLibrary: Track"
+                 << trackFile
+                 << "was added"
+                 << trackRecord.getDateAdded();
+    }
+    pTrackLibraryInsert->bindValue(":datetime_added", trackDateAdded);
 
-        // Written only once upon insert
-        pTrackLibraryInsert->bindValue(":location", trackLocationId.toVariant());
+    // Written only once upon insert
+    pTrackLibraryInsert->bindValue(":location", trackLocationId.toVariant());
 
-        // Column datetime_added is set implicitly
-        //pTrackLibraryInsert->bindValue(":datetime_added", track.getDateAdded());
+    // Column datetime_added is set implicitly
+    //pTrackLibraryInsert->bindValue(":datetime_added", track.getDateAdded());
 
-        pTrackLibraryInsert->bindValue(":mixxx_deleted", 0);
+    pTrackLibraryInsert->bindValue(":mixxx_deleted", 0);
 
-        // We no longer store the wavesummary in the library table.
-        pTrackLibraryInsert->bindValue(":wavesummaryhex", QVariant(QVariant::ByteArray));
+    // We no longer store the wavesummary in the library table.
+    pTrackLibraryInsert->bindValue(":wavesummaryhex", QVariant(QVariant::ByteArray));
 
-        if (pTrackLibraryInsert->exec()) {
-            return true;
-        } else {
-            // We failed to insert the track. Maybe it is already in the library
-            // but marked deleted? Skip this track.
-            LOG_FAILED_QUERY(*pTrackLibraryInsert)
-                    << "Failed to insert new track into library:"
-                    << track.getLocation();
-            return false;
-        }
+    VERIFY_OR_DEBUG_ASSERT(pTrackLibraryInsert->exec()) {
+        // We failed to insert the track. Maybe it is already in the library
+        // but marked deleted? Skip this track.
+        LOG_FAILED_QUERY(*pTrackLibraryInsert)
+                << "Failed to insert new track into library:"
+                << trackFile;
+        return false;
     }
+    return true;
+}
+
 } // anonymous namespace
 
 TrackId TrackDAO::addTracksAddTrack(const TrackPointer& pTrack, bool unremove) {
     DEBUG_ASSERT(pTrack);
+    const auto trackFile = pTrack->getFileInfo();
+
     VERIFY_OR_DEBUG_ASSERT(m_pQueryLibraryInsert || m_pQueryTrackLocationInsert ||
-        m_pQueryLibrarySelect || m_pQueryTrackLocationSelect) {
+            m_pQueryLibrarySelect || m_pQueryTrackLocationSelect) {
         qDebug() << "TrackDAO::addTracksAddTrack: needed SqlQuerys have not "
-                "been prepared. Skipping track"
-                << pTrack->getFileInfo();
+                    "been prepared. Skipping track"
+                 << trackFile;
         return TrackId();
     }
 
     qDebug() << "TrackDAO: Adding track"
-            << pTrack->getFileInfo();
+             << trackFile;
 
     TrackId trackId;
 
     // Insert the track location into the corresponding table. This will fail
     // silently if the location is already in the table because it has a UNIQUE
     // constraint.
-    if (!insertTrackLocation(m_pQueryTrackLocationInsert.get(), *pTrack)) {
+    if (!insertTrackLocation(m_pQueryTrackLocationInsert.get(), trackFile)) {
         DEBUG_ASSERT(pTrack->getDateAdded().isValid());
         // Inserting into track_locations failed, so the file already
         // exists. Query for its trackLocationId.
-        m_pQueryTrackLocationSelect->bindValue(":location", pTrack->getLocation());
+        m_pQueryTrackLocationSelect->bindValue(":location", trackFile.location());
         if (!m_pQueryTrackLocationSelect->exec()) {
             // We can't even select this, something is wrong.
             LOG_FAILED_QUERY(*m_pQueryTrackLocationSelect)
@@ -568,10 +598,10 @@ TrackId TrackDAO::addTracksAddTrack(const TrackPointer& pTrack, bool unremove) {
 
         m_pQueryLibrarySelect->bindValue(":location", trackLocationId.toVariant());
         if (!m_pQueryLibrarySelect->exec()) {
-             LOG_FAILED_QUERY(*m_pQueryLibrarySelect)
-                     << "Failed to query existing track: "
-                     << pTrack->getFileInfo();
-             return TrackId();
+            LOG_FAILED_QUERY(*m_pQueryLibrarySelect)
+                    << "Failed to query existing track: "
+                    << trackFile;
+            return TrackId();
         }
         if (m_queryLibraryIdColumn == UndefinedRecordIndex) {
             QSqlRecord queryLibraryRecord = m_pQueryLibrarySelect->record();
@@ -593,7 +623,7 @@ TrackId TrackDAO::addTracksAddTrack(const TrackPointer& pTrack, bool unremove) {
             if (!m_pQueryLibraryUpdate->exec()) {
                 LOG_FAILED_QUERY(*m_pQueryLibraryUpdate)
                         << "Failed to unremove existing track: "
-                        << pTrack->getFileInfo();
+                        << trackFile;
                 return TrackId();
             }
         }
@@ -622,8 +652,17 @@ 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(), *pTrack, trackLocationId, trackDateAdded)) {
+        if (!insertTrackLibrary(
+                    m_pQueryLibraryInsert.get(),
+                    trackRecord,
+                    pBeats,
+                    trackLocationId,
+                    trackFile,
+                    trackDateAdded)) {
             return TrackId();
         }
         trackId = TrackId(m_pQueryLibraryInsert->lastInsertId());
@@ -805,7 +844,8 @@ bool TrackDAO::onPurgingTracks(
     }
 
     QStringList idList;
-    for (const auto& trackId: trackIds) {
+    idList.reserve(trackIds.size());
+    for (const auto& trackId : trackIds) {
         GlobalTrackCacheLocker().purgeTrackId(trackId);
         idList.append(trackId.toString());
     }
@@ -1419,11 +1459,15 @@ bool TrackDAO::updateTrack(Track* pTrack) const {
             "coverart_source=:coverart_source,"
             "coverart_type=:coverart_type,"
             "coverart_location=:coverart_location,"
-            "coverart_hash=:coverart_hash"
-            " WHERE id=:track_id");
+            "coverart_hash=:coverart_hash "
+            "WHERE id=:track_id");
 
     query.bindValue(":track_id", trackId.toVariant());
-    bindTrackLibraryValues(&query, *pTrack);
+
+    mixxx::TrackRecord trackRecord;
+    pTrack->readTrackRecord(&trackRecord);
+    const mixxx::BeatsPointer pBeats = pTrack->getBeats();
+    bindTrackLibraryValues(&query, trackRecord, pBeats);
 
     VERIFY_OR_DEBUG_ASSERT(query.exec()) {
         LOG_FAILED_QUERY(query);
@@ -1870,10 +1914,10 @@ void TrackDAO::detectCoverArtForTracksWithoutCover(volatile const bool* pCancel,
     QSqlQuery updateQuery(m_database);
     updateQuery.prepare(
         "UPDATE library SET "
-        "  coverart_type=:coverart_type,"
-        "  coverart_source=:coverart_source,"
-        "  coverart_hash=:coverart_hash,"
-        "  coverart_location=:coverart_location "
+        "coverart_type=:coverart_type,"
+        "coverart_source=:coverart_source,"
+        "coverart_location=:coverart_location, "
+        "coverart_hash=:coverart_hash "
         "WHERE id=:track_id");
 
 
@@ -1905,13 +1949,14 @@ void TrackDAO::detectCoverArtForTracksWithoutCover(volatile const bool* pCancel,
                         embeddedCover);
         DEBUG_ASSERT(coverInfo.source != CoverInfo::UNKNOWN);
 
+        updateQuery.bindValue(":track_id", track.trackId.toVariant());
         updateQuery.bindValue(":coverart_type",
                               static_cast<int>(coverInfo.type));
         updateQuery.bindValue(":coverart_source",
                               static_cast<int>(coverInfo.source));
-        updateQuery.bindValue(":coverart_hash", coverInfo.hash);
         updateQuery.bindValue(":coverart_location", coverInfo.coverLocation);
-        updateQuery.bindValue(":track_id", track.trackId.toVariant());
+        updateQuery.bindValue(":coverart_hash", coverInfo.hash);
+
         if (!updateQuery.exec()) {
             LOG_FAILED_QUERY(updateQuery) << "failed to write file or none cover";
         } else {
diff --git a/src/library/dao/trackdao.h b/src/library/dao/trackdao.h
index bb2177c207..9843d70e18 100644
--- a/src/library/dao/trackdao.h
+++ b/src/library/dao/trackdao.h
@@ -42,9 +42,8 @@ class TrackDAO : public QObject, public virtual DAO, public virtual GlobalTrackC
             UserSettingsPointer pConfig);
     ~TrackDAO() override;
 
-    void initialize(const QSqlDatabase& database) override {
-        m_database = database;
-    }
+    void initialize(
+            const QSqlDatabase& database) override;
     void finish();
 
     QList<TrackId> resolveTrackIds(