Import metadata and cover image without temporary track object

src/library/browse/browsethread.cpp

@@ -161,95 +161,105 @@ void BrowseThread::populateModel() {
                 mixxx::FileInfo(fileIt.next()),
                 thisPath.token());
         {
-            const TrackPointer pTrack =
-                    SoundSourceProxy::importTemporaryTrack(fileAccess);
+            mixxx::TrackMetadata trackMetadata;
+            SoundSourceProxy::importTrackMetadataAndCoverImageFromFile(
+                    fileAccess,
+                    &trackMetadata,
+                    nullptr);
 
-            item = new QStandardItem(pTrack->getFileInfo().fileName());
+            item = new QStandardItem(fileAccess.info().fileName());
             item->setToolTip(item->text());
             item->setData(item->text(), Qt::UserRole);
             row_data.insert(COLUMN_FILENAME, item);
 
-            item = new QStandardItem(pTrack->getArtist());
+            item = new QStandardItem(trackMetadata.getTrackInfo().getArtist());
             item->setToolTip(item->text());
             item->setData(item->text(), Qt::UserRole);
             row_data.insert(COLUMN_ARTIST, item);
 
-            item = new QStandardItem(pTrack->getTitle());
+            item = new QStandardItem(trackMetadata.getTrackInfo().getTitle());
             item->setToolTip(item->text());
             item->setData(item->text(), Qt::UserRole);
             row_data.insert(COLUMN_TITLE, item);
 
-            item = new QStandardItem(pTrack->getAlbum());
+            item = new QStandardItem(trackMetadata.getAlbumInfo().getTitle());
             item->setToolTip(item->text());
             item->setData(item->text(), Qt::UserRole);
             row_data.insert(COLUMN_ALBUM, item);
 
-            item = new QStandardItem(pTrack->getTrackNumber());
+            item = new QStandardItem(trackMetadata.getTrackInfo().getTrackNumber());
             item->setToolTip(item->text());
             item->setData(item->text().toInt(), Qt::UserRole);
             row_data.insert(COLUMN_TRACK_NUMBER, item);
 
-            const QString year(pTrack->getYear());
+            const QString year(trackMetadata.getTrackInfo().getYear());
             item = new YearItem(year);
             item->setToolTip(year);
             // The year column is sorted according to the numeric calendar year
             item->setData(mixxx::TrackMetadata::parseCalendarYear(year), Qt::UserRole);
             row_data.insert(COLUMN_YEAR, item);
 
-            item = new QStandardItem(pTrack->getGenre());
+            item = new QStandardItem(trackMetadata.getTrackInfo().getGenre());
             item->setToolTip(item->text());
             item->setData(item->text(), Qt::UserRole);
             row_data.insert(COLUMN_GENRE, item);
 
-            item = new QStandardItem(pTrack->getComposer());
+            item = new QStandardItem(trackMetadata.getTrackInfo().getComposer());
             item->setToolTip(item->text());
             item->setData(item->text(), Qt::UserRole);
             row_data.insert(COLUMN_COMPOSER, item);
 
-            item = new QStandardItem(pTrack->getComment());
+            item = new QStandardItem(trackMetadata.getTrackInfo().getComment());
             item->setToolTip(item->text());
             item->setData(item->text(), Qt::UserRole);
             row_data.insert(COLUMN_COMMENT, item);
 
-            QString duration = pTrack->getDurationText(mixxx::Duration::Precision::SECONDS);
+            QString duration = trackMetadata.getDurationText(
+                    mixxx::Duration::Precision::SECONDS);
             item = new QStandardItem(duration);
             item->setToolTip(item->text());
-            item->setData(pTrack->getDuration(), Qt::UserRole);
+            item->setData(trackMetadata.getStreamInfo()
+                                  .getDuration()
+                                  .toDoubleSeconds(),
+                    Qt::UserRole);
             row_data.insert(COLUMN_DURATION, item);
 
-            item = new QStandardItem(pTrack->getBpmText());
+            item = new QStandardItem(trackMetadata.getTrackInfo().getBpmText());
             item->setToolTip(item->text());
-            item->setData(pTrack->getBpm(), Qt::UserRole);
+            item->setData(trackMetadata.getTrackInfo().getBpm().getValue(), Qt::UserRole);
             row_data.insert(COLUMN_BPM, item);
 
-            item = new QStandardItem(pTrack->getKeyText());
+            item = new QStandardItem(trackMetadata.getTrackInfo().getKey());
             item->setToolTip(item->text());
             item->setData(item->text(), Qt::UserRole);
             row_data.insert(COLUMN_KEY, item);
 
-            item = new QStandardItem(pTrack->getType());
+            item = new QStandardItem(fileAccess.info().suffix());
             item->setToolTip(item->text());
             item->setData(item->text(), Qt::UserRole);
             row_data.insert(COLUMN_TYPE, item);
 
-            item = new QStandardItem(pTrack->getBitrateText());
+            item = new QStandardItem(trackMetadata.getBitrateText());
             item->setToolTip(item->text());
-            item->setData(pTrack->getBitrate(), Qt::UserRole);
+            item->setData(
+                    static_cast<qlonglong>(
+                            trackMetadata.getStreamInfo().getBitrate().value()),
+                    Qt::UserRole);
             row_data.insert(COLUMN_BITRATE, item);
 
-            QString location = pTrack->getLocation();
+            QString location = fileAccess.info().location();
             QString nativeLocation = QDir::toNativeSeparators(location);
             item = new QStandardItem(nativeLocation);
             item->setToolTip(nativeLocation);
             item->setData(location, Qt::UserRole);
             row_data.insert(COLUMN_NATIVELOCATION, item);
 
-            item = new QStandardItem(pTrack->getAlbumArtist());
+            item = new QStandardItem(trackMetadata.getAlbumInfo().getArtist());
             item->setToolTip(item->text());
             item->setData(item->text(), Qt::UserRole);
             row_data.insert(COLUMN_ALBUMARTIST, item);
 
-            item = new QStandardItem(pTrack->getGrouping());
+            item = new QStandardItem(trackMetadata.getTrackInfo().getGrouping());
             item->setToolTip(item->text());
             item->setData(item->text(), Qt::UserRole);
             row_data.insert(COLUMN_GROUPING, item);
@@ -270,13 +280,13 @@ void BrowseThread::populateModel() {
             item->setData(fileCreated, Qt::UserRole);
             row_data.insert(COLUMN_FILE_CREATION_TIME, item);
 
-            const mixxx::ReplayGain replayGain(pTrack->getReplayGain());
+            const mixxx::ReplayGain replayGain(trackMetadata.getTrackInfo().getReplayGain());
             item = new QStandardItem(
                     mixxx::ReplayGain::ratioToString(replayGain.getRatio()));
             item->setToolTip(item->text());
             item->setData(item->text(), Qt::UserRole);
             row_data.insert(COLUMN_REPLAYGAIN, item);
-        } // implicitly release track pointer and unlock cache
+        }
 
         rows.append(row_data);
         ++row;

src/library/coverartutils.cpp

@@ -45,7 +45,12 @@ QString CoverArtUtils::supportedCoverArtExtensionsRegex() {
 //static
 QImage CoverArtUtils::extractEmbeddedCover(
         mixxx::FileAccess trackFileAccess) {
-    return SoundSourceProxy::importTemporaryCoverImage(std::move(trackFileAccess));
+    QImage image;
+    SoundSourceProxy::importTrackMetadataAndCoverImageFromFile(
+            std::move(trackFileAccess),
+            nullptr,
+            &image);
+    return image;
 }
 
 //static

src/library/dlgtrackinfo.cpp

@@ -606,16 +606,32 @@ void DlgTrackInfo::slotKeyTextChanged() {
 }
 
 void DlgTrackInfo::slotImportMetadataFromFile() {
-    if (m_pLoadedTrack) {
-        // Allocate a temporary track object for reading the metadata.
-        // We cannot reuse m_pLoadedTrack, because it might already been
-        // modified and we want to read fresh metadata directly from the
-        // file. Otherwise the changes in m_pLoadedTrack would be lost.
-        TrackPointer pTrack = SoundSourceProxy::importTemporaryTrack(
-                m_pLoadedTrack->getFileAccess());
-        DEBUG_ASSERT(pTrack);
-        populateFields(*pTrack);
+    if (!m_pLoadedTrack) {
+        return;
+    }
+    mixxx::TrackMetadata trackMetadata;
+    QImage coverImage;
+    const auto [importResult, metadataSynchronized] =
+            SoundSourceProxy(m_pLoadedTrack)
+                    .importTrackMetadataAndCoverImage(
+                            &trackMetadata, &coverImage);
+    if (importResult != mixxx::MetadataSource::ImportResult::Succeeded) {
+        return;
     }
+    auto fileAccess = m_pLoadedTrack->getFileAccess();
+    auto guessedCoverInfo = CoverInfoGuesser().guessCoverInfo(
+            fileAccess.info(),
+            trackMetadata.getAlbumInfo().getTitle(),
+            coverImage);
+    // Allocate a temporary track object for repopulating the fields.
+    // We cannot reuse m_pLoadedTrack, because it might already been
+    // modified and we don't want to lose those changes.
+    // TODO: Populate fields from TrackRecord instead of Track
+    TrackPointer pTrack = Track::newTemporary(std::move(fileAccess));
+    DEBUG_ASSERT(pTrack);
+    pTrack->importMetadata(std::move(trackMetadata));
+    pTrack->setCoverInfo(std::move(guessedCoverInfo));
+    populateFields(*pTrack);
 }
 
 void DlgTrackInfo::slotTrackChanged(TrackId trackId) {

src/sources/soundsourceproxy.cpp

@@ -302,36 +302,6 @@ SoundSourceProxy::allProviderRegistrationsForUrl(
     return providerRegistrations;
 }
 
-//static
-TrackPointer SoundSourceProxy::importTemporaryTrack(
-        mixxx::FileAccess trackFileAccess) {
-    TrackPointer pTrack = Track::newTemporary(std::move(trackFileAccess));
-    // Lock the track cache while populating the temporary track
-    // object to ensure that no metadata is exported into any file
-    // while reading from this file. Since locking individual files
-    // is not possible and the whole cache is locked.
-    GlobalTrackCacheLocker locker;
-    SoundSourceProxy(pTrack).updateTrackFromSource();
-    return pTrack;
-}
-
-//static
-QImage SoundSourceProxy::importTemporaryCoverImage(
-        mixxx::FileAccess trackFileAccess) {
-    if (!trackFileAccess.info().checkFileExists()) {
-        // Silently ignore missing files to avoid spaming the log:
-        // https://bugs.launchpad.net/mixxx/+bug/1875237
-        return QImage();
-    }
-    TrackPointer pTrack = Track::newTemporary(std::move(trackFileAccess));
-    // Lock the track cache while populating the temporary track
-    // object to ensure that no metadata is exported into any file
-    // while reading from this file. Since locking individual files
-    // is not possible and the whole cache is locked.
-    GlobalTrackCacheLocker locker;
-    return SoundSourceProxy(pTrack).importCoverImage();
-}
-
 //static
 ExportTrackMetadataResult
 SoundSourceProxy::exportTrackMetadataBeforeSaving(Track* pTrack, UserSettingsPointer pConfig) {
@@ -489,6 +459,59 @@ void SoundSourceProxy::initSoundSource(
     }
 }
 
+namespace {
+
+inline std::pair<mixxx::MetadataSource::ImportResult, QDateTime>
+importTrackMetadataAndCoverImageUnavailable() {
+    return std::make_pair(mixxx::MetadataSource::ImportResult::Unavailable, QDateTime());
+}
+
+} // anonymous namespace
+
+//static
+std::pair<mixxx::MetadataSource::ImportResult, QDateTime>
+SoundSourceProxy::importTrackMetadataAndCoverImageFromFile(
+        mixxx::FileAccess trackFileAccess,
+        mixxx::TrackMetadata* pTrackMetadata,
+        QImage* pCoverImage) {
+    if (!trackFileAccess.info().checkFileExists()) {
+        // Silently ignore missing files to avoid spaming the log:
+        // https://bugs.launchpad.net/mixxx/+bug/1875237
+        return importTrackMetadataAndCoverImageUnavailable();
+    }
+    TrackPointer pTrack;
+    // Lock the global track cache while accessing the file to ensure
+    // that no metadata is written. Since locking individual files
+    // is not possible the whole cache has to be locked.
+    GlobalTrackCacheLocker locker;
+    pTrack = locker.lookupTrackByRef(TrackRef::fromFileInfo(trackFileAccess.info()));
+    if (pTrack) {
+        // We can safely unlock the cache if the track object is already cached.
+        locker.unlockCache();
+    } else {
+        // If the track object is not cached we need to keep the cache
+        // locked and create a temporary track object instead.
+        pTrack = Track::newTemporary(std::move(trackFileAccess));
+    }
+    return SoundSourceProxy(pTrack).importTrackMetadataAndCoverImage(
+            pTrackMetadata,
+            pCoverImage);
+}
+
+std::pair<mixxx::MetadataSource::ImportResult, QDateTime>
+SoundSourceProxy::importTrackMetadataAndCoverImage(
+        mixxx::TrackMetadata* pTrackMetadata,
+        QImage* pCoverImage) const {
+    if (!m_pSoundSource) {
+        // The file doesn't seem to be readable or the file format
+        // is not supported.
+        return importTrackMetadataAndCoverImageUnavailable();
+    }
+    return m_pSoundSource->importTrackMetadataAndCoverImage(
+            pTrackMetadata,
+            pCoverImage);
+}
+
 void SoundSourceProxy::updateTrackFromSource(
         ImportTrackMetadataMode importTrackMetadataMode) {
     DEBUG_ASSERT(m_pTrack);
@@ -684,26 +707,7 @@ void SoundSourceProxy::updateTrackFromSource(
         DEBUG_ASSERT(coverInfo.source == CoverInfo::GUESSED);
         m_pTrack->setCoverInfo(coverInfo);
     }
-}
 
-mixxx::MetadataSource::ImportResult SoundSourceProxy::importTrackMetadata(mixxx::TrackMetadata* pTrackMetadata) const {
-    if (m_pSoundSource) {
-        return m_pSoundSource->importTrackMetadataAndCoverImage(pTrackMetadata, nullptr).first;
-    } else {
-        return mixxx::MetadataSource::ImportResult::Unavailable;
-    }
-}
-
-QImage SoundSourceProxy::importCoverImage() const {
-    if (m_pSoundSource) {
-        QImage coverImg;
-        if (m_pSoundSource->importTrackMetadataAndCoverImage(nullptr, &coverImg).first ==
-                mixxx::MetadataSource::ImportResult::Succeeded) {
-            return coverImg;
-        }
-    }
-    // Failed or unavailable
-    return QImage();
 }
 
 mixxx::AudioSourcePointer SoundSourceProxy::openAudioSource(

src/sources/soundsourceproxy.h

@@ -47,13 +47,6 @@ class SoundSourceProxy {
     static mixxx::SoundSourceProviderPointer getPrimaryProviderForFileExtension(
             const QString& fileExtension);
 
-    // The following import functions ensure that the file will not be
-    // written while reading it!
-    static TrackPointer importTemporaryTrack(
-            mixxx::FileAccess trackFileAccess);
-    static QImage importTemporaryCoverImage(
-            mixxx::FileAccess trackFileAccess);
-
     explicit SoundSourceProxy(
             TrackPointer pTrack,
             const mixxx::SoundSourceProviderPointer& pProvider = nullptr);
@@ -80,6 +73,36 @@ class SoundSourceProxy {
         return m_pProvider;
     }
 
+    /// Import both track metadata and/or cover image from a file.
+    ///
+    /// Pass nullptr for an out parameter if the corresponding data
+    /// is not needed.
+    ///
+    /// This function is thread-safe and can be invoked from any thread.
+    /// It ensures that no other thread writes the file concurrently
+    /// by keeping the corresponding file location in GlobalTrackCache
+    /// while reading.
+    static std::pair<mixxx::MetadataSource::ImportResult, QDateTime>
+    importTrackMetadataAndCoverImageFromFile(
+            mixxx::FileAccess trackFileAccess,
+            mixxx::TrackMetadata* pTrackMetadata,
+            QImage* pCoverImage);
+
+    /// Import both track metadata and/or the cover image of the
+    /// captured track object from the corresponding file.
+    ///
+    /// The captured track object is not modified, i.e. the data is read
+    /// from the file directly into the provided out parameters. Pass nullptr
+    /// for an out parameter if the corresponding data is not needed.
+    ///
+    /// If the captured track pointer is managed by GlobalTrackCache
+    /// reading from the file is safe, i.e. the read operation could
+    /// not be interleaved with a write operation when exporting metadata.
+    std::pair<mixxx::MetadataSource::ImportResult, QDateTime>
+    importTrackMetadataAndCoverImage(
+            mixxx::TrackMetadata* pTrackMetadata,
+            QImage* pCoverImage) const;
+
     /// Controls which (metadata/coverart) and how tags are (re-)imported from
     /// audio files when creating a SoundSourceProxy.
     enum class ImportTrackMetadataMode {
@@ -119,11 +142,6 @@ class SoundSourceProxy {
     void updateTrackFromSource(
             ImportTrackMetadataMode importTrackMetadataMode = ImportTrackMetadataMode::Default);
 
-    /// Parse only the metadata from the file without modifying
-    /// the referenced track.
-    mixxx::MetadataSource::ImportResult importTrackMetadata(
-            mixxx::TrackMetadata* pTrackMetadata) const;
-
     /// Opening the audio source through the proxy will update the
     /// audio properties of the corresponding track object. Returns
     /// a null pointer on failure.
@@ -157,10 +175,6 @@ class SoundSourceProxy {
             const QUrl& url,
             const mixxx::SoundSourceProviderPointer& pProvider = nullptr);
 
-    // Parse only the cover image from the file without modifying
-    // the referenced track.
-    QImage importCoverImage() const;
-
     const TrackPointer m_pTrack;
 
     const QUrl m_url;