diff options
| -rw-r--r-- | src/library/relocatedtrack.h | 11 | ||||
| -rw-r--r-- | src/sources/audiosource.cpp | 49 | ||||
| -rw-r--r-- | src/sources/audiosource.h | 63 | ||||
| -rw-r--r-- | src/sources/soundsourcem4a.h | 8 | ||||
| -rw-r--r-- | src/sources/soundsourceproxy.cpp | 111 |
5 files changed, 158 insertions, 84 deletions
diff --git a/src/library/relocatedtrack.h b/src/library/relocatedtrack.h index e2293ce0a6..3899876327 100644 --- a/src/library/relocatedtrack.h +++ b/src/library/relocatedtrack.h @@ -19,11 +19,12 @@ class RelocatedTrack final { DEBUG_ASSERT(m_missingTrackRef.isValid()); DEBUG_ASSERT(m_missingTrackRef.hasId()); DEBUG_ASSERT(m_missingTrackRef.hasLocation()); - DEBUG_ASSERT(m_addedTrackRef.isValid()); - // NOTE(uklotzde, 2019-12-16): - // m_addedTrackRef might not have a valid TrackId - // if the relocated track has not been added as a - // new track to the internal collection before. + // NOTE: m_addedTrackRef might not be valid! + // It might not have a valid TrackId if the relocated track has + // not been added as a new track to the internal collection before. + // It may also not have a valid canonical location, e.g. when + // relinking directories and the track is missing at both the old + // and the new location. DEBUG_ASSERT(m_addedTrackRef.hasLocation()); DEBUG_ASSERT(updatedTrackRef().isValid()); DEBUG_ASSERT(updatedTrackRef().hasId()); diff --git a/src/sources/audiosource.cpp b/src/sources/audiosource.cpp index 91ab264d91..d603ff7b0c 100644 --- a/src/sources/audiosource.cpp +++ b/src/sources/audiosource.cpp @@ -8,6 +8,12 @@ namespace { const Logger kLogger("AudioSource"); +// Maximum number of sample frames to verify that decoding the audio +// stream works. +// NOTE(2020-05-01): A single frame is sufficient to reliably detect +// the broken FAAD2 v2.9.1 library. +const SINT kVerifyReadableMaxFrameCount = 1; + } // anonymous namespace AudioSource::AudioSource(QUrl url) @@ -132,7 +138,9 @@ bool AudioSource::initBitrateOnce(audio::Bitrate bitrate) { return true; } -bool AudioSource::verifyReadable() const { +bool AudioSource::verifyReadable() { + // No early return desired! All tests should be performed, even + // if some fail. bool result = true; DEBUG_ASSERT(m_signalInfo.getSampleLayout()); if (!m_signalInfo.getChannelCount().isValid()) { @@ -170,13 +178,44 @@ bool AudioSource::verifyReadable() const { // to decode audio data! } } + if (!result) { + // Invalid or inconsistent properties detected. We can abort + // at this point and do not need to perform any read tests. + return false; + } if (frameIndexRange().empty()) { kLogger.warning() - << "No audio data available"; - // Don't set the result to false, even if reading from an - // empty source is pointless! + << "No audio data available, i.e. stream is empty"; + // Don't return false, even if reading from an empty source + // is pointless. It is still a valid audio stream. + return true; } - return result; + // Try to read some test frames to ensure that decoding actually works! + // + // Counterexample: The broken FAAD version 2.9.1 is able to open a file + // but then fails to decode any sample frames. + const SINT numSampleFrames = + math_min(kVerifyReadableMaxFrameCount, frameIndexRange().length()); + SampleBuffer sampleBuffer( + m_signalInfo.frames2samples(numSampleFrames)); + WritableSampleFrames writableSampleFrames( + frameIndexRange().splitAndShrinkFront(numSampleFrames), + SampleBuffer::WritableSlice(sampleBuffer)); + auto readableSampleFrames = readSampleFrames(writableSampleFrames); + DEBUG_ASSERT( + readableSampleFrames.frameIndexRange() <= + writableSampleFrames.frameIndexRange()); + if (readableSampleFrames.frameIndexRange() < + writableSampleFrames.frameIndexRange()) { + kLogger.warning() + << "Read test failed:" + << "expected =" + << writableSampleFrames.frameIndexRange() + << ", actual =" + << readableSampleFrames.frameIndexRange(); + return false; + } + return true; } WritableSampleFrames AudioSource::clampWritableSampleFrames( diff --git a/src/sources/audiosource.h b/src/sources/audiosource.h index 5ca3ec752b..0118148eb0 100644 --- a/src/sources/audiosource.h +++ b/src/sources/audiosource.h @@ -151,32 +151,50 @@ class AudioSource : public UrlResource, public virtual /*implements*/ IAudioSour // left and right channel respectively. static constexpr audio::SampleLayout kSampleLayout = mixxx::kEngineSampleLayout; + /// Defines how thoroughly the stream properties should be verified + /// when opening an audio stream. enum class OpenMode { - // In Strict mode the opening operation should be aborted - // as soon as any inconsistencies are detected. + /// In Strict mode the opening operation should be aborted + /// as soon as any inconsistencies of the stream properties + /// are detected when setting up the decoding. Strict, - // Opening in Permissive mode is used only after opening - // in Strict mode has been aborted by all available - // SoundSource implementations. + + /// Opening in Permissive mode is used only after opening + /// in Strict mode has been aborted by all available + /// SoundSource implementations. + /// + /// Example: Assume safe default values if mandatory stream + /// properties could not be determined when setting up the + /// decoding. Permissive, }; + /// Result of opening an audio stream. enum class OpenResult { + /// The file has been opened successfully and should be readable. Succeeded, - // If a SoundSource is not able to open a file because of - // internal errors of if the format of the content is not - // supported it should return Aborted. This gives SoundSources - // with a lower priority the chance to open the same file. - // Example: A SoundSourceProvider has been registered for - // files with a certain extension, but the corresponding - // SoundSource does only support a subset of all possible - // data formats that might be stored in files with this - // extension. + + /// If a SoundSource is not able to open a file because of + /// internal errors of if the format of the content is not + /// supported it should return Aborted. + /// + /// Returing this error result gives other decoders with a + /// lower priority the chance to open the same file. + /// Example: A SoundSourceProvider has been registered for + /// files with a certain extension, but the corresponding + /// SoundSource does only support a subset of all possible + /// data formats that might be stored in files with this + /// extension. + Aborted, - // If a SoundSource return Failed while opening a file - // the entire operation will fail immediately. No other - // sources with lower priority will be given the chance - // to open the same file. + /// If a SoundSource return Failed while opening a file the + /// file itself is supposed to be corrupt. + /// + /// If this happens during OpenMode::Strict then other decoders + /// with a lower priority are still given a chance to try + /// opening the file. In OpenMode::Permissive the first + /// SoundSource that returns Failed will declare this file + /// corrupt. Failed, }; @@ -286,10 +304,11 @@ class AudioSource : public UrlResource, public virtual /*implements*/ IAudioSour return getSignalInfo().frames2secs(frameLength()); } - // Verifies various properties to ensure that the audio data is - // actually readable. Warning messages are logged for properties - // with invalid values for diagnostic purposes. - bool verifyReadable() const; + /// Verifies various properties to ensure that the audio data is + /// actually readable. Warning messages are logged for properties + /// with invalid values for diagnostic purposes. Also performs a + /// basic read test. + bool verifyReadable(); ReadableSampleFrames readSampleFrames( WritableSampleFrames sampleFrames); diff --git a/src/sources/soundsourcem4a.h b/src/sources/soundsourcem4a.h index 33c2b6c3df..d13cba6875 100644 --- a/src/sources/soundsourcem4a.h +++ b/src/sources/soundsourcem4a.h @@ -18,6 +18,14 @@ namespace mixxx { +/// Decode M4A (AAC in MP4) files using FAAD2. +/// +/// NOTE(2020-05-01): Decoding in version v2.9.1 of the library +/// is broken and any attempt to open files will fail. +/// +/// https://github.com/mixxxdj/mixxx/pull/2738 +/// https://github.com/knik0/faad2/commit/a8dc3f8ce67f4069cfa4d5cf0fcc2c6e8ef2c2aa +/// https://github.com/knik0/faad2/commit/920ec985a74c6f88fe507181df07a0cd7e51d519 class SoundSourceM4A : public SoundSource { public: explicit SoundSourceM4A(const QUrl& url); diff --git a/src/sources/soundsourceproxy.cpp b/src/sources/soundsourceproxy.cpp index 6bfcc3ae21..3826e8ec26 100644 --- a/src/sources/soundsourceproxy.cpp +++ b/src/sources/soundsourceproxy.cpp @@ -481,68 +481,75 @@ QImage SoundSourceProxy::importCoverImage() const { mixxx::AudioSourcePointer SoundSourceProxy::openAudioSource(const mixxx::AudioSource::OpenParams& params) { DEBUG_ASSERT(m_pTrack); auto openMode = mixxx::SoundSource::OpenMode::Strict; + int attemptCount = 0; while (m_pSoundSource && !m_pAudioSource) { - // NOTE(uklotzde): Log unconditionally (with debug level) to - // identify files in the log file that might have caused a - // crash while importing metadata or decoding audio subsequently. - kLogger.debug() << "Opening file" - << getUrl().toString() - << "with provider" - << getSoundSourceProvider()->getName() - << "using mode" - << openMode; + ++attemptCount; const mixxx::SoundSource::OpenResult openResult = m_pSoundSource->open(openMode, params); - if ((openResult == mixxx::SoundSource::OpenResult::Aborted) || - ((openMode == mixxx::SoundSource::OpenMode::Strict) && (openResult == mixxx::SoundSource::OpenResult::Failed))) { - kLogger.warning() << "Unable to open file" - << getUrl().toString() - << "with provider" - << getSoundSourceProvider()->getName() - << "using mode" - << openMode; - // Continue with the next SoundSource provider - nextSoundSourceProvider(); - if (!getSoundSourceProvider() && (openMode == mixxx::SoundSource::OpenMode::Strict)) { - // No provider was able to open the source in Strict mode. - // Retry to open the file in Permissive mode starting with - // the first provider... - m_soundSourceProviderRegistrationIndex = 0; - openMode = mixxx::SoundSource::OpenMode::Permissive; - } - initSoundSource(); - continue; // try again - } - if ((openResult == mixxx::SoundSource::OpenResult::Succeeded) && m_pSoundSource->verifyReadable()) { - m_pAudioSource = mixxx::AudioSourceTrackProxy::create(m_pTrack, m_pSoundSource); - DEBUG_ASSERT(m_pAudioSource); - if (m_pAudioSource->frameIndexRange().empty()) { - kLogger.warning() << "File is empty" - << getUrl().toString(); - } - // Overwrite metadata with actual audio properties - if (m_pTrack) { - m_pTrack->updateAudioPropertiesFromStream( - m_pAudioSource->getStreamInfo()); + if (openResult == mixxx::SoundSource::OpenResult::Succeeded) { + if (m_pSoundSource->verifyReadable()) { + m_pAudioSource = mixxx::AudioSourceTrackProxy::create(m_pTrack, m_pSoundSource); + DEBUG_ASSERT(m_pAudioSource); + // Overwrite metadata with actual audio properties + if (m_pTrack) { + m_pTrack->updateAudioPropertiesFromStream( + m_pAudioSource->getStreamInfo()); + } + return m_pAudioSource; } + kLogger.warning() + << "Failed to read file" + << getUrl().toString() + << "with provider" + << getSoundSourceProvider()->getName(); + m_pSoundSource->close(); // cleanup } else { - kLogger.warning() << "Failed to open file" - << getUrl().toString() - << "with provider" - << getSoundSourceProvider()->getName(); - if (openResult == mixxx::SoundSource::OpenResult::Succeeded) { - m_pSoundSource->close(); // cleanup + kLogger.warning() + << "Failed to open file" + << getUrl().toString() + << "with provider" + << getSoundSourceProvider()->getName() + << "using mode" + << openMode; + if (openMode == mixxx::SoundSource::OpenMode::Permissive) { + if (openResult == mixxx::SoundSource::OpenResult::Failed) { + // 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. + DEBUG_ASSERT(!m_pAudioSource); + return m_pAudioSource; + } else { + // Continue and give other providers the chance to open the file + // in turn. + DEBUG_ASSERT(openResult == mixxx::SoundSource::OpenResult::Aborted); + } + } else { + // Continue and give other providers the chance to open the file + // in turn, independent of why opening the file failed. + DEBUG_ASSERT(openMode == mixxx::SoundSource::OpenMode::Strict); } - // Do NOT retry with the next SoundSource provider if the file - // itself is the cause! - DEBUG_ASSERT(!m_pAudioSource); } - return m_pAudioSource; // either success or failure + // Continue with the next SoundSource provider + nextSoundSourceProvider(); + if (!getSoundSourceProvider() && + (openMode == mixxx::SoundSource::OpenMode::Strict)) { + // No provider was able to open the source in Strict mode. + // Retry to open the file in Permissive mode starting with + // the first provider... + m_soundSourceProviderRegistrationIndex = 0; + openMode = mixxx::SoundSource::OpenMode::Permissive; + } + initSoundSource(); + // try again } // All available providers have returned OpenResult::Aborted when // getting here. m_pSoundSource might already be invalid/null! - kLogger.warning() << "Unable to decode file" - << getUrl().toString(); + kLogger.warning() + << "Giving up to open file" + << getUrl().toString() + << "after" + << attemptCount + << "unsuccessful attempts"; DEBUG_ASSERT(!m_pAudioSource); return m_pAudioSource; } |