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

@@ -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<const unsigned char *>(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<offset_t>(position, bufferSize());
+    const offset_t bufferLength = std::min<offset_t>(position, frameScanBlockSize);
     position -= bufferLength;
 
     seek(position);
     const ByteVector buffer = readBlock(bufferLength);
+    const auto data = reinterpret_cast<const unsigned char *>(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<const unsigned char *>(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<unsigned char>(headerID[0]) &&
+         previous == static_cast<unsigned char>(headerID[1]) &&
+         current == static_cast<unsigned char>(headerID[2]))
         return position + i - 2;
+
+      beforePrevious = previous;
+      previous = current;
     }
 
-    position += bufferSize();
+    position += buffer.size();
   }
 }

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<unsigned char>(bytes[offset + 0]),
+                           static_cast<unsigned char>(bytes[offset + 1]));
+      }
+
     }  // namespace
   }  // namespace MPEG
 }  // namespace TagLib

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<offset_t>(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<offset_t>(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");