From f29b3c5a6c1b6b73b7f5ff8d58dd65a7cb3a57e6 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sat, 11 Feb 2017 09:09:35 +0100 Subject: [PATCH] Fix decoding of the last MP3 frame --- src/sources/soundsourcemp3.cpp | 38 ++++++++++++++++++++++++++++++---- src/sources/soundsourcemp3.h | 2 ++ src/test/soundproxy_test.cpp | 16 ++------------ 3 files changed, 38 insertions(+), 18 deletions(-) diff --git a/src/sources/soundsourcemp3.cpp b/src/sources/soundsourcemp3.cpp index 1634c21adb14..2518c48be360 100644 --- a/src/sources/soundsourcemp3.cpp +++ b/src/sources/soundsourcemp3.cpp @@ -12,6 +12,8 @@ namespace { // MP3 does only support 1 or 2 channels const SINT kChannelCountMax = AudioSource::kChannelCountStereo; +const SINT kMaxBytesPerMp3Frame = 1441; + // mp3 supports 9 different sampling rates const int kSamplingRateCount = 9; @@ -161,7 +163,8 @@ SoundSourceMp3::SoundSourceMp3(const QUrl& url) m_pFileData(nullptr), m_avgSeekFrameCount(0), m_curFrameIndex(getMinFrameIndex()), - m_madSynthCount(0) { + m_madSynthCount(0), + m_leftoverBuffer(kMaxBytesPerMp3Frame + MAD_BUFFER_GUARD) { m_seekFrameList.reserve(kSeekFrameListCapacity); initDecoding(); } @@ -584,9 +587,36 @@ SINT SoundSourceMp3::readSampleFrames( if (MAD_ERROR_BUFLEN == m_madStream.error) { // Abort when reaching the end of the stream DEBUG_ASSERT(isUnrecoverableError(m_madStream)); - if (m_curFrameIndex < getMaxFrameIndex()) { - qWarning() << "End of MP3 stream is unreachable:" - << m_curFrameIndex << "<" << getMaxFrameIndex(); + if (m_madStream.next_frame != nullptr) { + // Decoding of the last MP3 frame fails if it is not padded + // with 0 bytes. MAD requires that the last frame ends with + // at least MAD_BUFFER_GUARD of 0 bytes. + // https://www.mars.org/pipermail/mad-dev/2001-May/000262.html + // "The reason for MAD_BUFFER_GUARD has to do with the way decoding is performed. + // In Layer III, Huffman decoding may inadvertently read a few bytes beyond the + // end of the buffer in the case of certain invalid input. This is not detected + // until after the fact. To prevent this from causing problems, and also to + // ensure the next frame's main_data_begin pointer is always accessible, MAD + // requires MAD_BUFFER_GUARD (currently 8) bytes to be present in the buffer past + // the end of the current frame in order to decode the frame." + const SINT remainingBytes = m_madStream.bufend - m_madStream.next_frame; + DEBUG_ASSERT(remainingBytes <= kMaxBytesPerMp3Frame); // only last MP3 frame + const SINT leftoverBytes = remainingBytes + MAD_BUFFER_GUARD; + if ((remainingBytes > 0) && (leftoverBytes <= SINT(m_leftoverBuffer.size()))) { + // Copy the data of the last MP3 frame into the leftover buffer... + unsigned char* pLeftoverBuffer = &*m_leftoverBuffer.begin(); + std::copy(m_madStream.next_frame, m_madStream.next_frame + remainingBytes, pLeftoverBuffer); + // ...append the required guard bytes... + std::fill(pLeftoverBuffer + remainingBytes, pLeftoverBuffer + leftoverBytes, 0); + // ...and retry decoding. + mad_stream_buffer(&m_madStream, pLeftoverBuffer, leftoverBytes); + m_madStream.error = MAD_ERROR_NONE; + continue; + } + if (m_curFrameIndex < getMaxFrameIndex()) { + qWarning() << "Failed to decode the end of the MP3 stream" + << m_curFrameIndex << "<" << getMaxFrameIndex(); + } } break; } diff --git a/src/sources/soundsourcemp3.h b/src/sources/soundsourcemp3.h index c7872ad457a1..89a11c180a19 100644 --- a/src/sources/soundsourcemp3.h +++ b/src/sources/soundsourcemp3.h @@ -76,6 +76,8 @@ class SoundSourceMp3: public SoundSource { mad_synth m_madSynth; SINT m_madSynthCount; // left overs from the previous read + + std::vector m_leftoverBuffer; }; class SoundSourceProviderMp3: public SoundSourceProvider { diff --git a/src/test/soundproxy_test.cpp b/src/test/soundproxy_test.cpp index 6ebcdc7ae67b..3abab5c3266d 100644 --- a/src/test/soundproxy_test.cpp +++ b/src/test/soundproxy_test.cpp @@ -309,20 +309,8 @@ TEST_F(SoundSourceProxyTest, seekBoundaries) { // Seek to boundaries (alternating) EXPECT_EQ(pSeekReadSource->getMinFrameIndex(), pSeekReadSource->seekSampleFrame(pSeekReadSource->getMinFrameIndex())); -#ifdef __MAD__ - // TODO(XXX): Seeking near the end of an MP3 stream - // is currently broken for SoundSourceMP3 (libmad) - if (filePath.endsWith(".mp3")) { - qWarning() - << "TODO(XXX): Fix seeking near end of stream for MP3 files" - << "and re-enable this test!"; - } else { -#endif // __MAD__ - EXPECT_EQ(pSeekReadSource->getMaxFrameIndex() - 1, - pSeekReadSource->seekSampleFrame(pSeekReadSource->getMaxFrameIndex() - 1)); -#ifdef __MAD__ - } -#endif // __MAD__ + EXPECT_EQ(pSeekReadSource->getMaxFrameIndex() - 1, + pSeekReadSource->seekSampleFrame(pSeekReadSource->getMaxFrameIndex() - 1)); EXPECT_EQ(pSeekReadSource->getMinFrameIndex() + 1, pSeekReadSource->seekSampleFrame(pSeekReadSource->getMinFrameIndex() + 1)); EXPECT_EQ(pSeekReadSource->getMaxFrameIndex(),