From 3e25faea2ca980afd7a3360f8204b540033a9f62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Mon, 27 Dec 2021 21:44:41 +0100 Subject: [PATCH 1/4] Decouple testing form normal SoundSourceProxy constructor This fixes lp1955840 for more than two SoundSources per file extension --- src/sources/soundsourceproxy.cpp | 104 ++++++++++++++++--------------- src/sources/soundsourceproxy.h | 18 +++--- 2 files changed, 64 insertions(+), 58 deletions(-) diff --git a/src/sources/soundsourceproxy.cpp b/src/sources/soundsourceproxy.cpp index 9ffaa12ee31..a292d58e30d 100644 --- a/src/sources/soundsourceproxy.cpp +++ b/src/sources/soundsourceproxy.cpp @@ -348,31 +348,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,47 +423,52 @@ 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 { diff --git a/src/sources/soundsourceproxy.h b/src/sources/soundsourceproxy.h index 72d91309590..8f0b7841bd2 100644 --- a/src/sources/soundsourceproxy.h +++ b/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,7 @@ 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); const TrackPointer m_pTrack; @@ -196,11 +197,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 nextProviderWithOpenMode(mixxx::SoundSource::OpenMode); From 095110a893cab7c86e8527c0354f7dfcea53797f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Mon, 27 Dec 2021 22:46:19 +0100 Subject: [PATCH 2/4] Decouple openSoundSource() from openAudioSource() --- src/sources/soundsourceproxy.cpp | 47 +++++++++++++++++--------------- src/sources/soundsourceproxy.h | 3 ++ 2 files changed, 28 insertions(+), 22 deletions(-) diff --git a/src/sources/soundsourceproxy.cpp b/src/sources/soundsourceproxy.cpp index a292d58e30d..6400df59dc6 100644 --- a/src/sources/soundsourceproxy.cpp +++ b/src/sources/soundsourceproxy.cpp @@ -311,7 +311,7 @@ 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 @@ -319,27 +319,25 @@ SoundSourceProxy::exportTrackMetadataBeforeSaving( // 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" @@ -771,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; @@ -781,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" @@ -811,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. @@ -843,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; } diff --git a/src/sources/soundsourceproxy.h b/src/sources/soundsourceproxy.h index 8f0b7841bd2..2fd930329cd 100644 --- a/src/sources/soundsourceproxy.h +++ b/src/sources/soundsourceproxy.h @@ -186,6 +186,9 @@ class SoundSourceProxy { // for writing metadata immediately before the TIO is destroyed. explicit SoundSourceProxy(const QUrl& url); + bool openSoundSource( + const mixxx::AudioSource::OpenParams& params = mixxx::AudioSource::OpenParams()); + const TrackPointer m_pTrack; const QUrl m_url; From 406feb5a88b9863e19183755e90957abc589832b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sun, 2 Jan 2022 03:04:10 +0100 Subject: [PATCH 3/4] Rename function to findProviderAndInitSoundSource() and initSoundSourceWithProvider() --- src/sources/soundsourceproxy.cpp | 14 +++++++------- src/sources/soundsourceproxy.h | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/sources/soundsourceproxy.cpp b/src/sources/soundsourceproxy.cpp index 6400df59dc6..20b8a09fcc1 100644 --- a/src/sources/soundsourceproxy.cpp +++ b/src/sources/soundsourceproxy.cpp @@ -353,20 +353,20 @@ SoundSourceProxy::SoundSourceProxy( : m_pTrack(std::move(pTrack)), m_url(m_pTrack ? m_pTrack->getFileInfo().toQUrl() : QUrl()), m_providerRegistrationIndex(-1) { - initSoundSource(std::move(pProvider)); + initSoundSourceWithProvider(std::move(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(); + findProviderAndInitSoundSource(); } SoundSourceProxy::SoundSourceProxy(const QUrl& url) : m_url(url), m_providerRegistrations(allProviderRegistrationsForUrl(m_url)) { - findAndInitSoundSource(); + findProviderAndInitSoundSource(); } mixxx::SoundSourceProviderPointer SoundSourceProxy::primaryProvider() { @@ -421,7 +421,7 @@ SoundSourceProxy::nextProviderWithOpenMode( } } -void SoundSourceProxy::findAndInitSoundSource() { +void SoundSourceProxy::findProviderAndInitSoundSource() { DEBUG_ASSERT(!m_pProvider); DEBUG_ASSERT(!m_pSoundSource); for (m_providerRegistrationIndex = 0; @@ -433,7 +433,7 @@ void SoundSourceProxy::findAndInitSoundSource() { VERIFY_OR_DEBUG_ASSERT(pProvider) { continue; } - if (initSoundSource(std::move(pProvider))) { + if (initSoundSourceWithProvider(std::move(pProvider))) { return; // Success } } @@ -444,7 +444,7 @@ void SoundSourceProxy::findAndInitSoundSource() { } } -bool SoundSourceProxy::initSoundSource( +bool SoundSourceProxy::initSoundSourceWithProvider( mixxx::SoundSourceProviderPointer&& pProvider) { DEBUG_ASSERT(!m_pProvider); DEBUG_ASSERT(!m_pSoundSource); @@ -821,7 +821,7 @@ bool SoundSourceProxy::openSoundSource( openMode = nextProviderWithOpenModePair.second; m_pProvider.reset(); m_pSoundSource.reset(); - initSoundSource(std::move(pNextProvider)); + initSoundSourceWithProvider(std::move(pNextProvider)); // try again } // All available providers have returned OpenResult::Aborted when diff --git a/src/sources/soundsourceproxy.h b/src/sources/soundsourceproxy.h index 2fd930329cd..fab94dd989d 100644 --- a/src/sources/soundsourceproxy.h +++ b/src/sources/soundsourceproxy.h @@ -200,9 +200,9 @@ class SoundSourceProxy { // provider and is initialized with -1 if no int m_providerRegistrationIndex; - void findAndInitSoundSource(); + void findProviderAndInitSoundSource(); - bool initSoundSource( + bool initSoundSourceWithProvider( mixxx::SoundSourceProviderPointer&& pProvider); mixxx::SoundSourceProviderPointer primaryProvider(); From 483439d9a20e99bffad8a8b512fd98e40d7c1612 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sun, 2 Jan 2022 03:08:18 +0100 Subject: [PATCH 4/4] Reduce nesting --- src/sources/soundsourceproxy.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/sources/soundsourceproxy.cpp b/src/sources/soundsourceproxy.cpp index 20b8a09fcc1..396b3f8a821 100644 --- a/src/sources/soundsourceproxy.cpp +++ b/src/sources/soundsourceproxy.cpp @@ -840,11 +840,11 @@ mixxx::AudioSourcePointer SoundSourceProxy::openAudioSource( 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); + if (!openSoundSource(params)) { + return nullptr; } - return nullptr; + // Overwrite metadata with actual audio properties + m_pTrack->updateStreamInfoFromSource( + m_pSoundSource->getStreamInfo()); + return mixxx::AudioSourceTrackProxy::create(m_pTrack, m_pSoundSource); }