From 5308d8f1cf0343550b39d61d7d2714eefec63e9c Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 6 Dec 2017 18:13:58 +0100 Subject: [PATCH 1/6] Add a regression test for Windows M4A infinite recursion --- src/test/soundproxy_test.cpp | 64 +++++++++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-) diff --git a/src/test/soundproxy_test.cpp b/src/test/soundproxy_test.cpp index e3a3c9341b6..8af58ab9a14 100644 --- a/src/test/soundproxy_test.cpp +++ b/src/test/soundproxy_test.cpp @@ -437,11 +437,34 @@ TEST_F(SoundSourceProxyTest, seekBoundaries) { seekFrameIndices.push_back( pSeekReadSource->frameIndexMin() + pSeekReadSource->frameLength() / 2); - // ...and to the boundaries again in opposite order. + // ...and to the boundaries again in opposite order... seekFrameIndices.push_back(pSeekReadSource->frameIndexMax()); seekFrameIndices.push_back(pSeekReadSource->frameIndexMin() + 1); seekFrameIndices.push_back(pSeekReadSource->frameIndexMax() - 1); seekFrameIndices.push_back(pSeekReadSource->frameIndexMin()); + // ...near the end and back to middle of the stream... + seekFrameIndices.push_back( + pSeekReadSource->frameIndexMax() + - 4 * kReadFrameCount); + seekFrameIndices.push_back( + pSeekReadSource->frameIndexMin() + + pSeekReadSource->frameLength() / 2); + // ...before the middle and then near the end of the stream... + seekFrameIndices.push_back( + pSeekReadSource->frameIndexMin() + + pSeekReadSource->frameLength() / 2 + - 4 * kReadFrameCount); + seekFrameIndices.push_back( + pSeekReadSource->frameIndexMax() + - 4 * kReadFrameCount); + // ...to the moddle of the stream and then skipping kReadFrameCount samples. + seekFrameIndices.push_back( + pSeekReadSource->frameIndexMin() + + pSeekReadSource->frameLength() / 2); + seekFrameIndices.push_back( + pSeekReadSource->frameIndexMin() + + pSeekReadSource->frameLength() / 2 + + 2 * kReadFrameCount); // Read and verify results for (SINT seekFrameIndex: seekFrameIndices) { @@ -541,3 +564,42 @@ TEST_F(SoundSourceProxyTest, readBeyondEnd) { mixxx::SampleBuffer::WritableSlice(readBuffer))).frameIndexRange()); } } + +TEST_F(SoundSourceProxyTest, regressionTestCachingReaderChunkJumpForward) { + const SINT kReadFrameCount = 8192; // like caching reader + + for (const auto& filePath: getFilePaths()) { + ASSERT_TRUE(SoundSourceProxy::isFileNameSupported(filePath)); + + mixxx::AudioSourcePointer pAudioSource(openAudioSource(filePath)); + // Obtaining an AudioSource may fail for unsupported file formats, + // even if the corresponding file extension is supported, e.g. + // AAC vs. ALAC in .m4a files + if (!pAudioSource) { + // skip test file + continue; + } + mixxx::SampleBuffer readBuffer( + pAudioSource->frames2samples(kReadFrameCount)); + + // Read chunk from beginning + auto firstChunkRange = mixxx::IndexRange::forward( + pAudioSource->frameIndexMin(), kReadFrameCount); + EXPECT_EQ( + firstChunkRange, + pAudioSource->readSampleFrames( + mixxx::WritableSampleFrames( + firstChunkRange, + mixxx::SampleBuffer::WritableSlice(readBuffer))).frameIndexRange()); + + // Read chunk from near the end, rounded to chunk boundary + auto secondChunkRange = mixxx::IndexRange::forward( + ((pAudioSource->frameIndexMax() - 2 * kReadFrameCount) / kReadFrameCount) * kReadFrameCount, kReadFrameCount); + EXPECT_EQ( + secondChunkRange, + pAudioSource->readSampleFrames( + mixxx::WritableSampleFrames( + secondChunkRange, + mixxx::SampleBuffer::WritableSlice(readBuffer))).frameIndexRange()); + } +} From 10b50055988c98263607016cb62543a3a7b6c815 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 8 Dec 2017 00:35:40 +0100 Subject: [PATCH 2/6] Fix infinite recursion error --- .../soundsourcemediafoundation.cpp | 57 ++++++++++++------- 1 file changed, 36 insertions(+), 21 deletions(-) diff --git a/plugins/soundsourcemediafoundation/soundsourcemediafoundation.cpp b/plugins/soundsourcemediafoundation/soundsourcemediafoundation.cpp index 8fd750fa0f3..faf6e04e8cd 100755 --- a/plugins/soundsourcemediafoundation/soundsourcemediafoundation.cpp +++ b/plugins/soundsourcemediafoundation/soundsourcemediafoundation.cpp @@ -11,9 +11,11 @@ namespace { const mixxx::Logger kLogger("SoundSourceMediaFoundation"); -const SINT kBytesPerSample = sizeof(CSAMPLE); -const SINT kBitsPerSample = kBytesPerSample * 8; -const SINT kLeftoverSize = 4096; // in CSAMPLE's, this seems to be the size MF AAC +constexpr SINT kUnknownFrameIndex = -1; + +constexpr SINT kBytesPerSample = sizeof(CSAMPLE); +constexpr SINT kBitsPerSample = kBytesPerSample * 8; +constexpr SINT kLeftoverSize = 4096; // in CSAMPLE's, this seems to be the size MF AAC // Decoding will be restarted one or more blocks of samples // before the actual position after seeking randomly in the @@ -24,10 +26,10 @@ const SINT kLeftoverSize = 4096; // in CSAMPLE's, this seems to be the size MF A // "It must also be assumed that without an explicit value, the playback // system will trim 2112 samples from the AAC decoder output when starting // playback from any point in the bistream." -const SINT kNumberOfPrefetchFrames = 2112; +constexpr SINT kNumberOfPrefetchFrames = 2112; // Only read the first audio stream -const DWORD kStreamIndex = MF_SOURCE_READER_FIRST_AUDIO_STREAM; +constexpr DWORD kStreamIndex = MF_SOURCE_READER_FIRST_AUDIO_STREAM; /** Microsoft examples use this snippet often. */ template static void safeRelease(T **ppT) { @@ -136,7 +138,7 @@ void SoundSourceMediaFoundation::close() { void SoundSourceMediaFoundation::seekSampleFrame(SINT frameIndex) { DEBUG_ASSERT(isValidFrameIndex(frameIndex)); - if (m_currentFrameIndex < frameIndex) { + if (isValidFrameIndex(m_currentFrameIndex) && (m_currentFrameIndex < frameIndex)) { // seeking forward const auto skipFrames = IndexRange::between(m_currentFrameIndex, frameIndex); // When to prefer skipping over seeking: @@ -163,7 +165,7 @@ void SoundSourceMediaFoundation::seekSampleFrame(SINT frameIndex) { // Discard decoded samples m_sampleBuffer.clear(); - // Invalidate current position (end of stream) + // Invalidate current position (end of stream to prevent further reading) m_currentFrameIndex = frameIndexMax(); if (m_pSourceReader == nullptr) { @@ -177,6 +179,7 @@ void SoundSourceMediaFoundation::seekSampleFrame(SINT frameIndex) { // some frames in advance to produce the same result at // each position in the stream. SINT seekIndex = std::max(SINT(frameIndex - kNumberOfPrefetchFrames), frameIndexMin()); + DEBUG_ASSERT(isValidFrameIndex(seekIndex)); LONGLONG seekPos = m_streamUnitConverter.fromFrameIndex(seekIndex); DEBUG_ASSERT(seekPos >= 0); @@ -206,6 +209,8 @@ void SoundSourceMediaFoundation::seekSampleFrame(SINT frameIndex) { } else { // We need to fetch at least 1 sample from the reader to obtain the // current position! + m_currentFrameIndex = kUnknownFrameIndex; // prevent further seeking + DEBUG_ASSERT(!isValidFrameIndex(m_currentFrameIndex)); if (skipFrames != readSampleFramesClamped( WritableSampleFrames(skipFrames)).frameIndexRange()) { kLogger.warning() @@ -215,6 +220,9 @@ void SoundSourceMediaFoundation::seekSampleFrame(SINT frameIndex) { } // Now m_currentFrameIndex reflects the actual position of the reader if (m_currentFrameIndex < frameIndex) { + // Skip more frames if the seek has taken us to a position before + // the requested target position (see comment above about the behavior + // of SetCurrentPosition() skipFrames = IndexRange::between(m_currentFrameIndex, frameIndex); // Skip more samples if frameIndex has not yet been reached if (skipFrames != readSampleFramesClamped( @@ -230,7 +238,7 @@ void SoundSourceMediaFoundation::seekSampleFrame(SINT frameIndex) { << "Seeking to frame" << frameIndex << "failed"; - // Jump to end of stream (= invalidate current position) + // Jump to end of stream to prevent further reading m_currentFrameIndex = frameIndexMax(); } } @@ -247,19 +255,25 @@ ReadableSampleFrames SoundSourceMediaFoundation::readSampleFramesClamped( WritableSampleFrames writableSampleFrames) { const SINT firstFrameIndex = writableSampleFrames.frameIndexRange().start(); - - seekSampleFrame(firstFrameIndex); - if (m_currentFrameIndex != firstFrameIndex) { - kLogger.warning() - << "Failed to position reader at beginning of decoding range" - << writableSampleFrames.frameIndexRange(); - // Abort - return ReadableSampleFrames( - mixxx::IndexRange::between( - m_currentFrameIndex, - m_currentFrameIndex)); + if (m_currentFrameIndex != kUnknownFrameIndex) { + seekSampleFrame(firstFrameIndex); + if (m_currentFrameIndex != firstFrameIndex) { + kLogger.warning() + << "Failed to position reader at beginning of decoding range" + << writableSampleFrames.frameIndexRange(); + // Abort + return ReadableSampleFrames( + mixxx::IndexRange::between( + m_currentFrameIndex, + m_currentFrameIndex)); + } + DEBUG_ASSERT(m_curFrameIndex == firstFrameIndex); + } else { + // Unknown position should only occur after seeking + // when all temporary buffers are empty + DEBUG_ASSERT(!isValidFrameIndex(m_currentFrameIndex)); + DEBUG_ASSERT(m_sampleBuffer.empty()); } - DEBUG_ASSERT(m_curFrameIndex == firstFrameIndex); const SINT numberOfFramesTotal = writableSampleFrames.frameLength(); @@ -272,6 +286,7 @@ ReadableSampleFrames SoundSourceMediaFoundation::readSampleFramesClamped( DEBUG_ASSERT(readableSlice.length() <= frames2samples(numberOfFramesRemaining)); if (readableSlice.length() > 0) { + DEBUG_ASSERT(isValidFrameIndex(m_currentFrameIndex)); DEBUG_ASSERT(m_currentFrameIndex < frameIndexMax()); if (pSampleBuffer) { SampleUtil::copy( @@ -337,7 +352,7 @@ ReadableSampleFrames SoundSourceMediaFoundation::readSampleFramesClamped( DEBUG_ASSERT(pSample != nullptr); SINT readerFrameIndex = m_streamUnitConverter.toFrameIndex(streamPos); DEBUG_ASSERT( - (m_currentFrameIndex == frameIndexMax()) || // unknown position after seeking + (m_currentFrameIndex == kUnknownFrameIndex) || // unknown position after seeking (m_currentFrameIndex == readerFrameIndex)); m_currentFrameIndex = readerFrameIndex; From e885a6d149236b6a3d54f4dcced517f676a09a9e Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 10 Dec 2017 10:34:17 +0100 Subject: [PATCH 3/6] Test audio decoding for various buffer sizes --- src/test/soundproxy_test.cpp | 288 +++++++++++++++++++---------------- 1 file changed, 155 insertions(+), 133 deletions(-) diff --git a/src/test/soundproxy_test.cpp b/src/test/soundproxy_test.cpp index 8af58ab9a14..5a53262e7a5 100644 --- a/src/test/soundproxy_test.cpp +++ b/src/test/soundproxy_test.cpp @@ -15,7 +15,24 @@ namespace { const QDir kTestDir(QDir::current().absoluteFilePath("src/test/id3-test-data")); -const SINT kMaxReadFrameCount = 10000; +const SINT kBufferSizes[] = { + 256, + 512, + 768, + 1024, + 1536, + 2048, + 3072, + 4096, + 6144, + 8192, + 12288, + 16384, + 24576, + 32768, +}; + +const SINT kMaxReadFrameCount = kBufferSizes[sizeof(kBufferSizes) / sizeof(kBufferSizes[0]) - 1]; } // anonymous namespace @@ -295,115 +312,115 @@ TEST_F(SoundSourceProxyTest, seekForwardBackward) { } TEST_F(SoundSourceProxyTest, skipAndRead) { - const SINT kReadFrameCount = 1000; - - for (const auto& filePath: getFilePaths()) { - ASSERT_TRUE(SoundSourceProxy::isFileNameSupported(filePath)); - - qDebug() << "Skip and read test:" << filePath; - - mixxx::AudioSourcePointer pContReadSource(openAudioSource(filePath)); - // Obtaining an AudioSource may fail for unsupported file formats, - // even if the corresponding file extension is supported, e.g. - // AAC vs. ALAC in .m4a files - if (!pContReadSource) { - // skip test file - continue; - } - SINT contFrameIndex = pContReadSource->frameIndexMin(); + for (auto kReadFrameCount: kBufferSizes) { + for (const auto& filePath: getFilePaths()) { + ASSERT_TRUE(SoundSourceProxy::isFileNameSupported(filePath)); - mixxx::AudioSourcePointer pSkipReadSource(openAudioSource(filePath)); - ASSERT_FALSE(!pSkipReadSource); - ASSERT_EQ(pContReadSource->channelCount(), pSkipReadSource->channelCount()); - ASSERT_EQ(pContReadSource->frameIndexRange(), pSkipReadSource->frameIndexRange()); - SINT skipFrameIndex = pSkipReadSource->frameIndexMin(); + qDebug() << "Skip and read test:" << filePath; - mixxx::SampleBuffer contReadData( - pContReadSource->frames2samples(kReadFrameCount)); - mixxx::SampleBuffer skipReadData( - pSkipReadSource->frames2samples(kReadFrameCount)); - - SINT minFrameIndex = pContReadSource->frameIndexMin(); - SINT skipCount = 1; - while (pContReadSource->frameIndexRange().containsIndex(minFrameIndex += skipCount)) { - skipCount = minFrameIndex / 4 + 1; // for next iteration + mixxx::AudioSourcePointer pContReadSource(openAudioSource(filePath)); + // Obtaining an AudioSource may fail for unsupported file formats, + // even if the corresponding file extension is supported, e.g. + // AAC vs. ALAC in .m4a files + if (!pContReadSource) { + // skip test file + continue; + } + SINT contFrameIndex = pContReadSource->frameIndexMin(); - qDebug() << "Skipping to:" << minFrameIndex; + mixxx::AudioSourcePointer pSkipReadSource(openAudioSource(filePath)); + ASSERT_FALSE(!pSkipReadSource); + ASSERT_EQ(pContReadSource->channelCount(), pSkipReadSource->channelCount()); + ASSERT_EQ(pContReadSource->frameIndexRange(), pSkipReadSource->frameIndexRange()); + SINT skipFrameIndex = pSkipReadSource->frameIndexMin(); - const auto readFrameIndexRange = - mixxx::IndexRange::forward(minFrameIndex, kReadFrameCount); - - // Read (and discard samples) until reaching the desired frame index - // and read next chunk - ASSERT_LE(contFrameIndex, minFrameIndex); - while (contFrameIndex < minFrameIndex) { - auto skippingFrameIndexRange = - mixxx::IndexRange::forward( - contFrameIndex, - std::min(minFrameIndex - contFrameIndex, kReadFrameCount)); - auto const skippedSampleFrames = + mixxx::SampleBuffer contReadData( + pContReadSource->frames2samples(kReadFrameCount)); + mixxx::SampleBuffer skipReadData( + pSkipReadSource->frames2samples(kReadFrameCount)); + + SINT minFrameIndex = pContReadSource->frameIndexMin(); + SINT skipCount = 1; + while (pContReadSource->frameIndexRange().containsIndex(minFrameIndex += skipCount)) { + skipCount = minFrameIndex / 4 + 1; // for next iteration + + qDebug() << "Skipping to:" << minFrameIndex; + + const auto readFrameIndexRange = + mixxx::IndexRange::forward(minFrameIndex, kReadFrameCount); + + // Read (and discard samples) until reaching the desired frame index + // and read next chunk + ASSERT_LE(contFrameIndex, minFrameIndex); + while (contFrameIndex < minFrameIndex) { + auto skippingFrameIndexRange = + mixxx::IndexRange::forward( + contFrameIndex, + std::min(minFrameIndex - contFrameIndex, kReadFrameCount)); + auto const skippedSampleFrames = + pContReadSource->readSampleFrames( + mixxx::WritableSampleFrames( + skippingFrameIndexRange, + mixxx::SampleBuffer::WritableSlice(contReadData))); + ASSERT_FALSE(skippedSampleFrames.frameIndexRange().empty()); + ASSERT_EQ(skippedSampleFrames.frameIndexRange().start(), contFrameIndex); + contFrameIndex += skippedSampleFrames.frameLength(); + } + ASSERT_EQ(minFrameIndex, contFrameIndex); + const auto contSampleFrames = pContReadSource->readSampleFrames( mixxx::WritableSampleFrames( - skippingFrameIndexRange, + readFrameIndexRange, mixxx::SampleBuffer::WritableSlice(contReadData))); - ASSERT_FALSE(skippedSampleFrames.frameIndexRange().empty()); - ASSERT_EQ(skippedSampleFrames.frameIndexRange().start(), contFrameIndex); - contFrameIndex += skippedSampleFrames.frameLength(); - } - ASSERT_EQ(minFrameIndex, contFrameIndex); - const auto contSampleFrames = - pContReadSource->readSampleFrames( - mixxx::WritableSampleFrames( - readFrameIndexRange, - mixxx::SampleBuffer::WritableSlice(contReadData))); - ASSERT_FALSE(contSampleFrames.frameIndexRange().empty()); - ASSERT_LE(contSampleFrames.frameIndexRange(), readFrameIndexRange); - ASSERT_EQ(contSampleFrames.frameIndexRange().start(), readFrameIndexRange.start()); - contFrameIndex += contSampleFrames.frameLength(); - - const SINT sampleCount = - pContReadSource->frames2samples(contSampleFrames.frameLength()); + ASSERT_FALSE(contSampleFrames.frameIndexRange().empty()); + ASSERT_LE(contSampleFrames.frameIndexRange(), readFrameIndexRange); + ASSERT_EQ(contSampleFrames.frameIndexRange().start(), readFrameIndexRange.start()); + contFrameIndex += contSampleFrames.frameLength(); + + const SINT sampleCount = + pContReadSource->frames2samples(contSampleFrames.frameLength()); + + // Skip until reaching the frame index and read next chunk + ASSERT_LE(skipFrameIndex, minFrameIndex); + while (skipFrameIndex < minFrameIndex) { + auto const skippedFrameIndexRange = + skipSampleFrames(pSkipReadSource, + mixxx::IndexRange::between(skipFrameIndex, minFrameIndex)); + ASSERT_FALSE(skippedFrameIndexRange.empty()); + ASSERT_EQ(skippedFrameIndexRange.start(), skipFrameIndex); + skipFrameIndex += skippedFrameIndexRange.length(); + } + ASSERT_EQ(minFrameIndex, skipFrameIndex); + const auto skippedSampleFrames = + pSkipReadSource->readSampleFrames( + mixxx::WritableSampleFrames( + readFrameIndexRange, + mixxx::SampleBuffer::WritableSlice(skipReadData))); - // Skip until reaching the frame index and read next chunk - ASSERT_LE(skipFrameIndex, minFrameIndex); - while (skipFrameIndex < minFrameIndex) { - auto const skippedFrameIndexRange = - skipSampleFrames(pSkipReadSource, - mixxx::IndexRange::between(skipFrameIndex, minFrameIndex)); - ASSERT_FALSE(skippedFrameIndexRange.empty()); - ASSERT_EQ(skippedFrameIndexRange.start(), skipFrameIndex); - skipFrameIndex += skippedFrameIndexRange.length(); - } - ASSERT_EQ(minFrameIndex, skipFrameIndex); - const auto skippedSampleFrames = - pSkipReadSource->readSampleFrames( - mixxx::WritableSampleFrames( - readFrameIndexRange, - mixxx::SampleBuffer::WritableSlice(skipReadData))); + skipFrameIndex += skippedSampleFrames.frameLength(); - skipFrameIndex += skippedSampleFrames.frameLength(); + // Both buffers should be equal + ASSERT_EQ(contSampleFrames.frameIndexRange(), skippedSampleFrames.frameIndexRange()); + #ifdef __OPUS__ + if (filePath.endsWith(".opus")) { + expectDecodedSamplesEqualOpus( + sampleCount, + &contReadData[0], + &skipReadData[0], + "Decoding mismatch after skipping"); + } else { + #endif // __OPUS__ + expectDecodedSamplesEqual( + sampleCount, + &contReadData[0], + &skipReadData[0], + "Decoding mismatch after skipping"); + #ifdef __OPUS__ + } + #endif // __OPUS__ - // Both buffers should be equal - ASSERT_EQ(contSampleFrames.frameIndexRange(), skippedSampleFrames.frameIndexRange()); -#ifdef __OPUS__ - if (filePath.endsWith(".opus")) { - expectDecodedSamplesEqualOpus( - sampleCount, - &contReadData[0], - &skipReadData[0], - "Decoding mismatch after skipping"); - } else { -#endif // __OPUS__ - expectDecodedSamplesEqual( - sampleCount, - &contReadData[0], - &skipReadData[0], - "Decoding mismatch after skipping"); -#ifdef __OPUS__ + minFrameIndex = contFrameIndex; } -#endif // __OPUS__ - - minFrameIndex = contFrameIndex; } } } @@ -566,40 +583,45 @@ TEST_F(SoundSourceProxyTest, readBeyondEnd) { } TEST_F(SoundSourceProxyTest, regressionTestCachingReaderChunkJumpForward) { - const SINT kReadFrameCount = 8192; // like caching reader - - for (const auto& filePath: getFilePaths()) { - ASSERT_TRUE(SoundSourceProxy::isFileNameSupported(filePath)); - - mixxx::AudioSourcePointer pAudioSource(openAudioSource(filePath)); - // Obtaining an AudioSource may fail for unsupported file formats, - // even if the corresponding file extension is supported, e.g. - // AAC vs. ALAC in .m4a files - if (!pAudioSource) { - // skip test file - continue; + // NOTE(uklotzde, 2017-12-10): Potential regression test for an infinite + // seek/read loop in SoundSourceMediaFoundation. Unfortunately this + // test doesn't fail even prior to fixing the reported bug. + // https://github.com/mixxxdj/mixxx/pull/1317#issuecomment-349674161 + + for (auto kReadFrameCount: kBufferSizes) { + for (const auto& filePath: getFilePaths()) { + ASSERT_TRUE(SoundSourceProxy::isFileNameSupported(filePath)); + + mixxx::AudioSourcePointer pAudioSource(openAudioSource(filePath)); + // Obtaining an AudioSource may fail for unsupported file formats, + // even if the corresponding file extension is supported, e.g. + // AAC vs. ALAC in .m4a files + if (!pAudioSource) { + // skip test file + continue; + } + mixxx::SampleBuffer readBuffer( + pAudioSource->frames2samples(kReadFrameCount)); + + // Read chunk from beginning + auto firstChunkRange = mixxx::IndexRange::forward( + pAudioSource->frameIndexMin(), kReadFrameCount); + EXPECT_EQ( + firstChunkRange, + pAudioSource->readSampleFrames( + mixxx::WritableSampleFrames( + firstChunkRange, + mixxx::SampleBuffer::WritableSlice(readBuffer))).frameIndexRange()); + + // Read chunk from near the end, rounded to chunk boundary + auto secondChunkRange = mixxx::IndexRange::forward( + ((pAudioSource->frameIndexMax() - 2 * kReadFrameCount) / kReadFrameCount) * kReadFrameCount, kReadFrameCount); + EXPECT_EQ( + secondChunkRange, + pAudioSource->readSampleFrames( + mixxx::WritableSampleFrames( + secondChunkRange, + mixxx::SampleBuffer::WritableSlice(readBuffer))).frameIndexRange()); } - mixxx::SampleBuffer readBuffer( - pAudioSource->frames2samples(kReadFrameCount)); - - // Read chunk from beginning - auto firstChunkRange = mixxx::IndexRange::forward( - pAudioSource->frameIndexMin(), kReadFrameCount); - EXPECT_EQ( - firstChunkRange, - pAudioSource->readSampleFrames( - mixxx::WritableSampleFrames( - firstChunkRange, - mixxx::SampleBuffer::WritableSlice(readBuffer))).frameIndexRange()); - - // Read chunk from near the end, rounded to chunk boundary - auto secondChunkRange = mixxx::IndexRange::forward( - ((pAudioSource->frameIndexMax() - 2 * kReadFrameCount) / kReadFrameCount) * kReadFrameCount, kReadFrameCount); - EXPECT_EQ( - secondChunkRange, - pAudioSource->readSampleFrames( - mixxx::WritableSampleFrames( - secondChunkRange, - mixxx::SampleBuffer::WritableSlice(readBuffer))).frameIndexRange()); } } From 62ccd7f3953c535937c55fe4ef978af102e04254 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 11 Dec 2017 18:10:02 +0100 Subject: [PATCH 4/6] Fix analyzer crash with empty file --- src/analyzer/analyzerqueue.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/analyzer/analyzerqueue.cpp b/src/analyzer/analyzerqueue.cpp index e3b95041a16..f574f79cd9e 100644 --- a/src/analyzer/analyzerqueue.cpp +++ b/src/analyzer/analyzerqueue.cpp @@ -349,8 +349,7 @@ void AnalyzerQueue::execThread() { if (!pAudioSource) { kLogger.warning() << "Failed to open file for analyzing:" - << nextTrack->getLocation() - << *pAudioSource; + << nextTrack->getLocation(); emptyCheck(); continue; } From 728bcf56f1860dd33285786dae1b0c69f7cd5a8b Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 11 Dec 2017 18:14:31 +0100 Subject: [PATCH 5/6] Add a test for opening empty audio files --- src/test/soundproxy_test.cpp | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/test/soundproxy_test.cpp b/src/test/soundproxy_test.cpp index e3a3c9341b6..e3dd7fdc99e 100644 --- a/src/test/soundproxy_test.cpp +++ b/src/test/soundproxy_test.cpp @@ -1,3 +1,4 @@ +#include #include #include "test/mixxxtest.h" @@ -171,6 +172,23 @@ TEST_F(SoundSourceProxyTest, open) { } } +TEST_F(SoundSourceProxyTest, openEmptyFile) { + for (const auto& fileNameSuffix: getFileNameSuffixes()) { + QTemporaryFile tempFile("emptyXXXXXX" + fileNameSuffix); + qDebug() << "Created testing to open empty file:" + << tempFile.fileName(); + tempFile.open(); + tempFile.close(); + + ASSERT_TRUE(SoundSourceProxy::isFileNameSupported(tempFile.fileName())); + auto pTrack = Track::newTemporary(tempFile.fileName()); + SoundSourceProxy proxy(pTrack); + + auto pAudioSource = proxy.openAudioSource(); + EXPECT_TRUE(!pAudioSource); + } +} + TEST_F(SoundSourceProxyTest, readArtist) { auto pTrack = Track::newTemporary( kTestDir.absoluteFilePath("artist.mp3")); From 2a1c3ca5defbd2a0264891c6ddd286f36774eb2f Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 11 Dec 2017 18:15:14 +0100 Subject: [PATCH 6/6] Fix unused parameters --- src/test/soundproxy_test.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/soundproxy_test.cpp b/src/test/soundproxy_test.cpp index e3dd7fdc99e..2a670552793 100644 --- a/src/test/soundproxy_test.cpp +++ b/src/test/soundproxy_test.cpp @@ -75,7 +75,7 @@ class SoundSourceProxyTest: public MixxxTest { // to test the upscaling of channels mixxx::AudioSource::OpenParams openParams; openParams.setChannelCount(2); - auto pAudioSource = proxy.openAudioSource(); + auto pAudioSource = proxy.openAudioSource(openParams); EXPECT_FALSE(!pAudioSource); if (pAudioSource->channelCount() != 2) { // Wrap into proxy object @@ -158,7 +158,7 @@ TEST_F(SoundSourceProxyTest, open) { for (const auto& filePath: getFilePaths()) { ASSERT_TRUE(SoundSourceProxy::isFileNameSupported(filePath)); - mixxx::AudioSourcePointer pAudioSource(openAudioSource(filePath)); + mixxx::AudioSourcePointer pAudioSource = openAudioSource(filePath); // Obtaining an AudioSource may fail for unsupported file formats, // even if the corresponding file extension is supported, e.g. // AAC vs. ALAC in .m4a files