Sound proxy fix and clean up

src/sources/soundsourceproxy.cpp

@@ -311,35 +311,33 @@ SoundSourceProxy::exportTrackMetadataBeforeSaving(
         const UserSettingsPointer& pConfig) {
     DEBUG_ASSERT(pTrack);
     const auto fileInfo = pTrack->getFileInfo();
-    mixxx::MetadataSourcePointer pMetadataSource;
+    mixxx::SoundSourcePointer pSoundSource;
     {
         auto proxy = SoundSourceProxy(fileInfo.toQUrl());
         // Ensure that the actual audio properties of the
         // stream are available before exporting metadata.
         // This might be needed for converting sample positions
         // to time positions and vice versa.
         if (!pTrack->hasStreamInfoFromSource()) {
-            auto pAudioSource = proxy.openAudioSource();
-            if (pAudioSource) {
+            if (proxy.openSoundSource()) {
+                pSoundSource = proxy.m_pSoundSource;
                 pTrack->updateStreamInfoFromSource(
-                        pAudioSource->getStreamInfo());
+                        pSoundSource->getStreamInfo());
                 DEBUG_ASSERT(pTrack->hasStreamInfoFromSource());
                 // Make sure that the track file is closed after reading.
                 // Otherwise writing of track metadata into file tags
                 // might fail.
-                pAudioSource->close();
+                pSoundSource->close();
             } else {
                 kLogger.warning()
                         << "Failed to update stream info from audio "
                            "source before exporting metadata";
             }
         }
-        pMetadataSource = proxy.m_pSoundSource;
+        pSoundSource = proxy.m_pSoundSource;
     }
-    if (pMetadataSource) {
-        return pTrack->exportMetadata(
-                *pMetadataSource,
-                pConfig);
+    if (pSoundSource) {
+        return pTrack->exportMetadata(*pSoundSource, pConfig);
     } else {
         kLogger.warning()
                 << "Unable to export track metadata into file"
@@ -348,31 +346,30 @@ SoundSourceProxy::exportTrackMetadataBeforeSaving(
     }
 }
 
+// Used during tests only
 SoundSourceProxy::SoundSourceProxy(
         TrackPointer pTrack,
-        const mixxx::SoundSourceProviderPointer& pProvider)
+        mixxx::SoundSourceProviderPointer pProvider)
         : m_pTrack(std::move(pTrack)),
           m_url(m_pTrack ? m_pTrack->getFileInfo().toQUrl() : QUrl()),
-          m_providerRegistrations(allProviderRegistrationsForUrl(m_url)),
           m_providerRegistrationIndex(-1) {
-    initSoundSource(pProvider);
+    initSoundSource(std::move(pProvider));
 }
 
-SoundSourceProxy::SoundSourceProxy(
-        const QUrl& url,
-        const mixxx::SoundSourceProviderPointer& pProvider)
+SoundSourceProxy::SoundSourceProxy(TrackPointer pTrack)
+        : m_pTrack(std::move(pTrack)),
+          m_url(m_pTrack ? m_pTrack->getFileInfo().toQUrl() : QUrl()),
+          m_providerRegistrations(allProviderRegistrationsForUrl(m_url)) {
+    findAndInitSoundSource();
+}
+
+SoundSourceProxy::SoundSourceProxy(const QUrl& url)
         : m_url(url),
-          m_providerRegistrations(allProviderRegistrationsForUrl(m_url)),
-          m_providerRegistrationIndex(-1) {
-    initSoundSource(pProvider);
+          m_providerRegistrations(allProviderRegistrationsForUrl(m_url)) {
+    findAndInitSoundSource();
 }
 
-mixxx::SoundSourceProviderPointer SoundSourceProxy::primaryProvider(
-        const mixxx::SoundSourceProviderPointer& pProvider) {
-    if (pProvider) {
-        m_providerRegistrationIndex = -1;
-        return pProvider;
-    }
+mixxx::SoundSourceProviderPointer SoundSourceProxy::primaryProvider() {
     m_providerRegistrationIndex = 0;
     if (m_providerRegistrationIndex < m_providerRegistrations.size()) {
         return m_providerRegistrations[m_providerRegistrationIndex].getProvider();
@@ -424,49 +421,54 @@ SoundSourceProxy::nextProviderWithOpenMode(
     }
 }
 
-void SoundSourceProxy::initSoundSource(
-        const mixxx::SoundSourceProviderPointer& pProvider) {
+void SoundSourceProxy::findAndInitSoundSource() {
     DEBUG_ASSERT(!m_pProvider);
     DEBUG_ASSERT(!m_pSoundSource);
-    auto pNextProvider = primaryProvider(pProvider);
-    while (!m_pSoundSource && pNextProvider) {
-        m_pSoundSource = pNextProvider->newSoundSource(m_url);
-        if (m_pSoundSource) {
-            m_pProvider = std::move(pNextProvider);
-            if (kLogger.debugEnabled()) {
-                kLogger.debug() << "SoundSourceProvider"
-                                << m_pProvider->getDisplayName()
-                                << "created a SoundSource for file"
-                                << getUrl().toString()
-                                << "of type"
-                                << m_pSoundSource->getType();
-            }
-            // Done
-            return;
-        }
-        kLogger.warning() << "SoundSourceProvider"
-                          << pNextProvider->getDisplayName()
-                          << "failed to create a SoundSource for file"
-                          << getUrl().toString();
-        if (pProvider) {
-            // Only a single attempt for the given provider
-            return;
-        }
-        // Switch to next available provider
-        pNextProvider = nextProvider();
-        if (pNextProvider) {
+    for (m_providerRegistrationIndex = 0;
+            m_providerRegistrationIndex < m_providerRegistrations.size();
+            ++m_providerRegistrationIndex) {
+        mixxx::SoundSourceProviderPointer pProvider =
+                m_providerRegistrations[m_providerRegistrationIndex]
+                        .getProvider();
+        VERIFY_OR_DEBUG_ASSERT(pProvider) {
             continue;
         }
-        if (!getUrl().isEmpty()) {
-            kLogger.warning()
-                    << "No SoundSourceProvider for file"
-                    << getUrl().toString();
+        if (initSoundSource(std::move(pProvider))) {
+            return; // Success
         }
-        // Abort after failure
-        return;
+    }
+    if (!getUrl().isEmpty()) {
+        kLogger.warning()
+                << "No SoundSourceProvider for file"
+                << getUrl().toString();
     }
 }
 
+bool SoundSourceProxy::initSoundSource(
+        mixxx::SoundSourceProviderPointer&& pProvider) {
+    DEBUG_ASSERT(!m_pProvider);
+    DEBUG_ASSERT(!m_pSoundSource);
+    DEBUG_ASSERT(pProvider);
+    m_pSoundSource = pProvider->newSoundSource(m_url);
+    if (m_pSoundSource) {
+        m_pProvider = pProvider;
+        if (kLogger.debugEnabled()) {
+            kLogger.debug() << "SoundSourceProvider"
+                            << m_pProvider->getDisplayName()
+                            << "created a SoundSource for file"
+                            << getUrl().toString()
+                            << "of type"
+                            << m_pSoundSource->getType();
+        }
+        return true;
+    }
+    kLogger.warning() << "SoundSourceProvider"
+                      << pProvider->getDisplayName()
+                      << "failed to create a SoundSource for file"
+                      << getUrl().toString();
+    return false;
+}
+
 namespace {
 
 inline std::pair<mixxx::MetadataSource::ImportResult, QDateTime>
@@ -767,7 +769,7 @@ bool SoundSourceProxy::updateTrackFromSource(
     return true;
 }
 
-mixxx::AudioSourcePointer SoundSourceProxy::openAudioSource(
+bool SoundSourceProxy::openSoundSource(
         const mixxx::AudioSource::OpenParams& params) {
     auto openMode = mixxx::SoundSource::OpenMode::Strict;
     int attemptCount = 0;
@@ -777,16 +779,7 @@ mixxx::AudioSourcePointer SoundSourceProxy::openAudioSource(
                 m_pSoundSource->open(openMode, params);
         if (openResult == mixxx::SoundSource::OpenResult::Succeeded) {
             if (m_pSoundSource->verifyReadable()) {
-                // The internal m_pTrack might be null when opening the AudioSource
-                // before exporting metadata. In this case the caller (this class)
-                // is responsible for updating the stream info if needed.
-                if (!m_pTrack) {
-                    return m_pSoundSource;
-                }
-                // Overwrite metadata with actual audio properties
-                m_pTrack->updateStreamInfoFromSource(
-                        m_pSoundSource->getStreamInfo());
-                return mixxx::AudioSourceTrackProxy::create(m_pTrack, m_pSoundSource);
+                return true;
             }
             kLogger.warning()
                     << "Failed to read file"
@@ -807,7 +800,7 @@ mixxx::AudioSourcePointer SoundSourceProxy::openAudioSource(
                     // Do NOT retry with the next SoundSource provider if the file
                     // itself seems to be the cause when opening still fails during
                     // the 2nd (= permissive) round.
-                    return nullptr;
+                    return false;
                 } else {
                     // Continue and give other providers the chance to open the file
                     // in turn.
@@ -839,5 +832,19 @@ mixxx::AudioSourcePointer SoundSourceProxy::openAudioSource(
             << "after"
             << attemptCount
             << "unsuccessful attempts";
+    return false;
+}
+
+mixxx::AudioSourcePointer SoundSourceProxy::openAudioSource(
+        const mixxx::AudioSource::OpenParams& params) {
+    VERIFY_OR_DEBUG_ASSERT(m_pTrack) {
+        return nullptr;
+    }
+    if (openSoundSource(params)) {
+        // Overwrite metadata with actual audio properties
+        m_pTrack->updateStreamInfoFromSource(
+                m_pSoundSource->getStreamInfo());
+        return mixxx::AudioSourceTrackProxy::create(m_pTrack, m_pSoundSource);
+    }
     return nullptr;
 }

src/sources/soundsourceproxy.h

@@ -47,9 +47,12 @@ class SoundSourceProxy {
     static mixxx::SoundSourceProviderPointer getPrimaryProviderForFileExtension(
             const QString& fileExtension);
 
+    explicit SoundSourceProxy(TrackPointer pTrack);
+
+    // Used during unit tests to check SoundSources explicit
     explicit SoundSourceProxy(
             TrackPointer pTrack,
-            const mixxx::SoundSourceProviderPointer& pProvider = nullptr);
+            mixxx::SoundSourceProviderPointer pProvider);
 
     /// The track object that has been passed at construction.
     ///
@@ -181,9 +184,10 @@ class SoundSourceProxy {
 
     // Special case: Construction from a url is needed
     // for writing metadata immediately before the TIO is destroyed.
-    explicit SoundSourceProxy(
-            const QUrl& url,
-            const mixxx::SoundSourceProviderPointer& pProvider = nullptr);
+    explicit SoundSourceProxy(const QUrl& url);
+
+    bool openSoundSource(
+            const mixxx::AudioSource::OpenParams& params = mixxx::AudioSource::OpenParams());
 
     const TrackPointer m_pTrack;
 
@@ -196,11 +200,12 @@ class SoundSourceProxy {
     // provider and is initialized with -1 if no
     int m_providerRegistrationIndex;
 
-    void initSoundSource(
-            const mixxx::SoundSourceProviderPointer& pProvider);
+    void findAndInitSoundSource();
+
+    bool initSoundSource(
+            mixxx::SoundSourceProviderPointer&& pProvider);
 
-    mixxx::SoundSourceProviderPointer primaryProvider(
-            const mixxx::SoundSourceProviderPointer& pProvider = nullptr);
+    mixxx::SoundSourceProviderPointer primaryProvider();
     mixxx::SoundSourceProviderPointer nextProvider();
     std::pair<mixxx::SoundSourceProviderPointer, mixxx::SoundSource::OpenMode>
             nextProviderWithOpenMode(mixxx::SoundSource::OpenMode);