Ignore fake MPEG frame headers when seeking them.

diff --git a/taglib/mpeg/mpegfile.cpp b/taglib/mpeg/mpegfile.cpp
index c634eeb87e9c1c6dbcfb3f4e2c390b7ac145d9df..6860053bc9500ee2391b77d1827ceb15a1d812bf 100644 (file)
--- a/taglib/mpeg/mpegfile.cpp
+++ b/taglib/mpeg/mpegfile.cpp
@@ -346,55 +346,52 @@ void MPEG::File::setID3v2FrameFactory(const ID3v2::FrameFactory *factory)
 
 long MPEG::File::nextFrameOffset(long position)
 {
-  bool foundLastSyncPattern = false;
-
-  ByteVector buffer;
+  char frameSyncBytes[2] = {};
 
   while(true) {
     seek(position);
-    buffer = readBlock(bufferSize());
-
-    if(buffer.size() <= 0)
+    const ByteVector buffer = readBlock(bufferSize());
+    if(buffer.size() == 0)
       return -1;
 
-    if(foundLastSyncPattern && secondSynchByte(buffer[0]))
-      return position - 1;
-
-    for(unsigned int i = 0; i < buffer.size() - 1; i++) {
-      if(firstSyncByte(buffer[i]) && secondSynchByte(buffer[i + 1]))
-        return position + i;
+    for(unsigned int i = 0; i < buffer.size(); ++i) {
+      frameSyncBytes[0] = frameSyncBytes[1];
+      frameSyncBytes[1] = buffer[i];
+      if(firstSyncByte(frameSyncBytes[0]) && secondSynchByte(frameSyncBytes[1])) {
+        Header header(this, position + i - 1, true);
+        if(header.isValid())
+          return position + i - 1;
+      }
     }
 
-    foundLastSyncPattern = firstSyncByte(buffer[buffer.size() - 1]);
-    position += buffer.size();
+    position += bufferSize();
   }
 }
 
 long MPEG::File::previousFrameOffset(long position)
 {
-  bool foundFirstSyncPattern = false;
-  ByteVector buffer;
+  char frameSyncBytes[2] = {};
 
-  while (position > 0) {
-    long size = std::min<long>(position, bufferSize());
+  while(position > 0) {
+    const long size = std::min<long>(position, bufferSize());
     position -= size;
 
     seek(position);
-    buffer = readBlock(size);
-
-    if(buffer.size() <= 0)
-      break;
-
-    if(foundFirstSyncPattern && firstSyncByte(buffer[buffer.size() - 1]))
-      return position + buffer.size() - 1;
+    const ByteVector buffer = readBlock(bufferSize());
+    if(buffer.size() == 0)
+      return -1;
 
-    for(int i = buffer.size() - 2; i >= 0; i--) {
-      if(firstSyncByte(buffer[i]) && secondSynchByte(buffer[i + 1]))
-        return position + i;
+    for(int i = buffer.size() - 1; i >= 0; i--) {
+      frameSyncBytes[1] = frameSyncBytes[0];
+      frameSyncBytes[0] = buffer[i];
+      if(firstSyncByte(frameSyncBytes[0]) && secondSynchByte(frameSyncBytes[1])) {
+        Header header(this, position + i, true);
+        if(header.isValid())
+          return position + i + header.frameLength();
+      }
     }
-
-    foundFirstSyncPattern = secondSynchByte(buffer[0]);
   }
+
   return -1;
 }
 

diff --git a/taglib/mpeg/mpegproperties.cpp b/taglib/mpeg/mpegproperties.cpp
index 6e7bb8232be15e95e3e73831de1ddb759704627b..d8b7bd5064ef4c40958e0c65bec383af9a9735ef 100644 (file)
--- a/taglib/mpeg/mpegproperties.cpp
+++ b/taglib/mpeg/mpegproperties.cpp
@@ -157,23 +157,13 @@ void MPEG::Properties::read(File *file)
 {
   // Only the first valid frame is required if we have a VBR header.
 
-  long firstFrameOffset = file->firstFrameOffset();
+  const long firstFrameOffset = file->firstFrameOffset();
   if(firstFrameOffset < 0) {
     debug("MPEG::Properties::read() -- Could not find an MPEG frame in the stream.");
     return;
   }
 
-  Header firstHeader(file, firstFrameOffset);
-
-  while(!firstHeader.isValid()) {
-    firstFrameOffset = file->nextFrameOffset(firstFrameOffset + 1);
-    if(firstFrameOffset < 0) {
-      debug("MPEG::Properties::read() -- Could not find a valid first MPEG frame in the stream.");
-      return;
-    }
-
-    firstHeader = Header(file, firstFrameOffset);
-  }
+  const Header firstHeader(file, firstFrameOffset, false);
 
   // Check for a VBR header that will help us in gathering information about a
   // VBR stream.
@@ -207,24 +197,13 @@ void MPEG::Properties::read(File *file)
 
     // Look for the last MPEG audio frame to calculate the stream length.
 
-    long lastFrameOffset = file->lastFrameOffset();
+    const long lastFrameOffset = file->lastFrameOffset();
     if(lastFrameOffset < 0) {
       debug("MPEG::Properties::read() -- Could not find an MPEG frame in the stream.");
       return;
     }
 
-    Header lastHeader(file, lastFrameOffset, false);
-
-    while(!lastHeader.isValid()) {
-      lastFrameOffset = file->previousFrameOffset(lastFrameOffset);
-      if(lastFrameOffset < 0) {
-        debug("MPEG::Properties::read() -- Could not find a valid last MPEG frame in the stream.");
-        return;
-      }
-
-      lastHeader = Header(file, lastFrameOffset, false);
-    }
-
+    const Header lastHeader(file, lastFrameOffset, false);
     const long streamLength = lastFrameOffset - firstFrameOffset + lastHeader.frameLength();
     if(streamLength > 0)
       d->length = static_cast<int>(streamLength * 8.0 / d->bitrate + 0.5);

diff --git a/tests/data/garbage.mp3 b/tests/data/garbage.mp3
new file mode 100644 (file)
index 0000000..730b74e
Binary files /dev/null and b/tests/data/garbage.mp3 differ

diff --git a/tests/test_mpeg.cpp b/tests/test_mpeg.cpp
index c1179ee5cb680021f70176a7f6dc7e150bceb024..5a990580791598e614ee7696dfa3aaccd87e8f3a 100644 (file)
--- a/tests/test_mpeg.cpp
+++ b/tests/test_mpeg.cpp
@@ -64,6 +64,7 @@ class TestMPEG : public CppUnit::TestFixture
   CPPUNIT_TEST(testEmptyID3v2);
   CPPUNIT_TEST(testEmptyID3v1);
   CPPUNIT_TEST(testEmptyAPE);
+  CPPUNIT_TEST(testIgnoreGarbage);
   CPPUNIT_TEST_SUITE_END();
 
 public:
@@ -119,13 +120,8 @@ public:
     CPPUNIT_ASSERT_EQUAL(44100, f.audioProperties()->sampleRate());
     CPPUNIT_ASSERT(!f.audioProperties()->xingHeader());
 
-    long last = f.lastFrameOffset();
-    MPEG::Header lastHeader(&f, last, false);
-
-    while(!lastHeader.isValid()) {
-      last = f.previousFrameOffset(last);
-      lastHeader = MPEG::Header(&f, last, false);
-    }
+    const long last = f.lastFrameOffset();
+    const MPEG::Header lastHeader(&f, last, false);
 
     CPPUNIT_ASSERT_EQUAL(28213L, last);
     CPPUNIT_ASSERT_EQUAL(209, lastHeader.frameLength());
@@ -163,7 +159,7 @@ public:
     CPPUNIT_ASSERT(f.audioProperties());
     CPPUNIT_ASSERT_EQUAL(0, f.audioProperties()->length());
     CPPUNIT_ASSERT_EQUAL(0, f.audioProperties()->lengthInSeconds());
-    CPPUNIT_ASSERT_EQUAL(176, f.audioProperties()->lengthInMilliseconds());
+    CPPUNIT_ASSERT_EQUAL(183, f.audioProperties()->lengthInMilliseconds());
     CPPUNIT_ASSERT_EQUAL(320, f.audioProperties()->bitrate());
     CPPUNIT_ASSERT_EQUAL(2, f.audioProperties()->channels());
     CPPUNIT_ASSERT_EQUAL(44100, f.audioProperties()->sampleRate());
@@ -421,6 +417,27 @@ public:
     }
   }
 
+  void testIgnoreGarbage()
+  {
+    const ScopedFileCopy copy("garbage", ".mp3");
+    {
+      MPEG::File f(copy.fileName().c_str());
+      CPPUNIT_ASSERT(f.isValid());
+      CPPUNIT_ASSERT(f.hasID3v2Tag());
+      CPPUNIT_ASSERT_EQUAL(2255L, f.firstFrameOffset());
+      CPPUNIT_ASSERT_EQUAL(6015L, f.lastFrameOffset());
+      CPPUNIT_ASSERT_EQUAL(String("Title A"), f.ID3v2Tag()->title());
+      f.ID3v2Tag()->setTitle("Title B");
+      f.save();
+    }
+    {
+      MPEG::File f(copy.fileName().c_str());
+      CPPUNIT_ASSERT(f.isValid());
+      CPPUNIT_ASSERT(f.hasID3v2Tag());
+      CPPUNIT_ASSERT_EQUAL(String("Title B"), f.ID3v2Tag()->title());
+    }
+  }
+
 };
 
 CPPUNIT_TEST_SUITE_REGISTRATION(TestMPEG);