From 8875f397d953658a11b14c197e35757b9f01c497 Mon Sep 17 00:00:00 2001 From: Ryan Francesconi <2917795+ryanfrancesconi@users.noreply.github.com> Date: Sat, 31 Oct 2026 23:26:20 -0700 Subject: [PATCH] MPEG: Scan raw bytes in 64 KB blocks when searching for frames (#842) findID3v2, nextFrameOffset and previousFrameOffset read raw bytes in 64 KB blocks instead of building a ByteVector per byte from 1 KB reads, with the sync check in an isFrameSync(b1, b2) overload. Results are unchanged; junk before or after the audio no longer makes opening slow. --- taglib/mpeg/mpegfile.cpp | 57 ++++++++++++++++++++++++---------------- taglib/mpeg/mpegutils.h | 10 ++++--- tests/test_mpeg.cpp | 33 +++++++++++++++++++++++ 3 files changed, 74 insertions(+), 26 deletions(-) diff --git a/taglib/mpeg/mpegfile.cpp b/taglib/mpeg/mpegfile.cpp index be1c2421..ba2b8077 100644 --- a/taglib/mpeg/mpegfile.cpp +++ b/taglib/mpeg/mpegfile.cpp @@ -46,6 +46,10 @@ using namespace TagLib; namespace { + // Searching for a frame sync reads this much at a time; with 1 kB reads the + // seeks dominate when much of the file is not audio. + constexpr unsigned int frameScanBlockSize = 64 * 1024; + enum { ID3v2Index = 0, APEIndex = 1, ID3v1Index = 2 }; } // namespace @@ -385,45 +389,49 @@ bool MPEG::File::strip(int tags, bool freeMemory) offset_t MPEG::File::nextFrameOffset(offset_t position) { - ByteVector frameSyncBytes(2, '\0'); + // The byte before the current one, carried across buffers. + unsigned char previous = 0; while(true) { seek(position); - const ByteVector buffer = readBlock(bufferSize()); + const ByteVector buffer = readBlock(frameScanBlockSize); if(buffer.isEmpty()) return -1; + const auto data = reinterpret_cast(buffer.data()); for(unsigned int i = 0; i < buffer.size(); ++i) { - frameSyncBytes[0] = frameSyncBytes[1]; - frameSyncBytes[1] = buffer[i]; - if(isFrameSync(frameSyncBytes)) { + const unsigned char current = data[i]; + if(isFrameSync(previous, current)) { if(const Header header(this, position + i - 1, true); header.isValid()) return position + i - 1; } + previous = current; } - position += bufferSize(); + position += buffer.size(); } } offset_t MPEG::File::previousFrameOffset(offset_t position) { - ByteVector frameSyncBytes(2, '\0'); + // The byte following the current one, carried across buffers. + unsigned char next = 0; while(position > 0) { - const offset_t bufferLength = std::min(position, bufferSize()); + const offset_t bufferLength = std::min(position, frameScanBlockSize); position -= bufferLength; seek(position); const ByteVector buffer = readBlock(bufferLength); + const auto data = reinterpret_cast(buffer.data()); for(int i = buffer.size() - 1; i >= 0; --i) { - frameSyncBytes[1] = frameSyncBytes[0]; - frameSyncBytes[0] = buffer[i]; - if(isFrameSync(frameSyncBytes)) { + const unsigned char current = data[i]; + if(isFrameSync(current, next)) { if(const Header header(this, position + i, true); header.isValid()) return position + i + header.frameLength(); } + next = current; } } @@ -533,31 +541,34 @@ offset_t MPEG::File::findID3v2(Properties::ReadStyle readStyle) // Look for an ID3v2 tag until reaching the first valid MPEG frame. - ByteVector frameSyncBytes(2, '\0'); - ByteVector tagHeaderBytes(3, '\0'); - long position = 0; + // The two bytes before the current one, carried across buffers. + unsigned char beforePrevious = 0; + unsigned char previous = 0; + offset_t position = 0; while(true) { seek(position); - const ByteVector buffer = readBlock(bufferSize()); + const ByteVector buffer = readBlock(frameScanBlockSize); if(buffer.isEmpty()) return -1; + const auto data = reinterpret_cast(buffer.data()); for(unsigned int i = 0; i < buffer.size(); ++i) { - frameSyncBytes[0] = frameSyncBytes[1]; - frameSyncBytes[1] = buffer[i]; - if(isFrameSync(frameSyncBytes)) { + const unsigned char current = data[i]; + if(isFrameSync(previous, current)) { if(const Header header(this, position + i - 1, true); header.isValid()) return -1; } - tagHeaderBytes[0] = tagHeaderBytes[1]; - tagHeaderBytes[1] = tagHeaderBytes[2]; - tagHeaderBytes[2] = buffer[i]; - if(tagHeaderBytes == headerID) + if(beforePrevious == static_cast(headerID[0]) && + previous == static_cast(headerID[1]) && + current == static_cast(headerID[2])) return position + i - 2; + + beforePrevious = previous; + previous = current; } - position += bufferSize(); + position += buffer.size(); } } diff --git a/taglib/mpeg/mpegutils.h b/taglib/mpeg/mpegutils.h index 51943328..1902b383 100644 --- a/taglib/mpeg/mpegutils.h +++ b/taglib/mpeg/mpegutils.h @@ -45,15 +45,19 @@ namespace TagLib * \note This does not check the length of the vector, since this is an * internal utility function. */ - inline bool isFrameSync(const ByteVector &bytes, unsigned int offset = 0) + inline bool isFrameSync(unsigned char b1, unsigned char b2) { // 0xFF in the second byte is possible in theory, but it's very unlikely. - const unsigned char b1 = bytes[offset + 0]; - const unsigned char b2 = bytes[offset + 1]; return (b1 == 0xFF && b2 != 0xFF && (b2 & 0xE0) == 0xE0); } + inline bool isFrameSync(const ByteVector &bytes, unsigned int offset = 0) + { + return isFrameSync(static_cast(bytes[offset + 0]), + static_cast(bytes[offset + 1])); + } + } // namespace } // namespace MPEG } // namespace TagLib diff --git a/tests/test_mpeg.cpp b/tests/test_mpeg.cpp index 14f68588..e3de6275 100644 --- a/tests/test_mpeg.cpp +++ b/tests/test_mpeg.cpp @@ -31,6 +31,7 @@ #include "tstring.h" #include "tpropertymap.h" #include "mpegfile.h" +#include "tbytevectorstream.h" #include "id3v2tag.h" #include "id3v1tag.h" #ifdef TAGLIB_WITH_APE @@ -74,6 +75,7 @@ class TestMPEG : public CppUnit::TestFixture CPPUNIT_TEST(testEmptyAPE); #endif CPPUNIT_TEST(testIgnoreGarbage); + CPPUNIT_TEST(testFrameSearchThroughJunk); CPPUNIT_TEST(testExtendedHeader); CPPUNIT_TEST(testReadStyleFast); CPPUNIT_TEST(testID3v22Properties); @@ -577,6 +579,37 @@ public: } } + void testFrameSearchThroughJunk() + { + // 100 MPEG-1 Layer III frames, 128 kbps at 44.1 kHz: 417 bytes each, no + // padding and no Xing header, so the length comes from the frame search. + ByteVector frames; + for(int i = 0; i < 100; ++i) + frames.append(ByteVector("\xff\xfb\x90\x00", 4) + ByteVector(413, '\0')); + const ByteVector junk(1024 * 1024, '\0'); + + ByteVectorStream cleanStream(frames); + MPEG::File clean(&cleanStream); + CPPUNIT_ASSERT(clean.isValid()); + CPPUNIT_ASSERT_EQUAL(static_cast(0), clean.firstFrameOffset()); + CPPUNIT_ASSERT(clean.audioProperties()->lengthInMilliseconds() > 0); + + ByteVectorStream tailStream(frames + junk); + MPEG::File tail(&tailStream); + CPPUNIT_ASSERT_EQUAL(clean.firstFrameOffset(), tail.firstFrameOffset()); + CPPUNIT_ASSERT_EQUAL(clean.lastFrameOffset(), tail.lastFrameOffset()); + CPPUNIT_ASSERT_EQUAL(clean.audioProperties()->lengthInMilliseconds(), + tail.audioProperties()->lengthInMilliseconds()); + + ByteVectorStream leadStream(junk + frames); + MPEG::File lead(&leadStream); + const auto shift = static_cast(junk.size()); + CPPUNIT_ASSERT_EQUAL(clean.firstFrameOffset() + shift, lead.firstFrameOffset()); + CPPUNIT_ASSERT_EQUAL(clean.lastFrameOffset() + shift, lead.lastFrameOffset()); + CPPUNIT_ASSERT_EQUAL(clean.audioProperties()->lengthInMilliseconds(), + lead.audioProperties()->lengthInMilliseconds()); + } + void testExtendedHeader() { const ScopedFileCopy copy("extended-header", ".mp3");