Import metadata and cover image without temporary track object

2 files changed, 83 insertions, 65 deletions

diff --git a/src/sources/soundsourceproxy.cpp b/src/sources/soundsourceproxy.cpp
index a1282a9a96..3d6cdaa30f 100644
--- a/src/sources/soundsourceproxy.cpp
+++ b/src/sources/soundsourceproxy.cpp
@@ -303,36 +303,6 @@ SoundSourceProxy::allProviderRegistrationsForUrl(
 }
 
 //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) {
     DEBUG_ASSERT(pTrack);
@@ -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(
diff --git a/src/sources/soundsourceproxy.h b/src/sources/soundsourceproxy.h
index 7e42d1ea72..02594658a7 100644
--- a/src/sources/soundsourceproxy.h
+++ b/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;